[GitHub] [incubator-pekko-connectors] mdedetrich commented on a diff in pull request #84: Add S3 put plus get bucketWithVersioning API

["mdedetrich (via GitHub)" <gi...@apache.org>]

mdedetrich commented on code in PR #84:
URL: https://github.com/apache/incubator-pekko-connectors/pull/84#discussion_r1174534616


##########
s3/src/main/scala/org/apache/pekko/stream/connectors/s3/impl/Marshalling.scala:
##########
@@ -32,6 +32,19 @@ import scala.xml.NodeSeq
 @InternalApi private[impl] object Marshalling {
   import ScalaXmlSupport._
 
+  implicit val bucketVersioningUnmarshaller: FromEntityUnmarshaller[BucketVersioningResult] = {
+    nodeSeqUnmarshaller(MediaTypes.`application/xml`, ContentTypes.`application/octet-stream`).map {
+      case NodeSeq.Empty => throw Unmarshaller.NoContentException
+      case x =>
+        val status = (x \ "Status").headOption.map(_.text match {
+          case "Enabled"   => BucketVersioningStatus.Enabled
+          case "Suspended" => BucketVersioningStatus.Suspended

Review Comment:
   So I am slightly torn on what to do here. As explained previously I have an impression that there could be a chance that a new value gets added to this field (since its `Suspended` rather than just being `Disabled`). If this happens then you would get a runtime error here, there are alternatives however any of these alternatives would be a completely new case for `Marshalling.scala`, i.e.
   
   * If we get a value other than `"Enabled"`/`"Suspended"` we can just return "Suspended" but then log an error saying that we discovered a new field. This means we won't break end users but it might also cause unintended behaviour
   * Throw a proper exception rather than just a match error, no such exception actually exists currently since this would be a first



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@pekko.apache.org
For additional commands, e-mail: notifications-help@pekko.apache.org