You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@pekko.apache.org by "mdedetrich (via GitHub)" <gi...@apache.org> on 2023/12/21 01:03:13 UTC
[PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
mdedetrich opened a new pull request, #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376
TBD
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#discussion_r1433405525
##########
http/src/main/scala/org/apache/pekko/http/javadsl/unmarshalling/Unmarshaller.scala:
##########
@@ -14,21 +14,20 @@
package org.apache.pekko.http.javadsl.unmarshalling
import java.util.concurrent.CompletionStage
+import java.util.Optional
import org.apache.pekko
import pekko.actor.ClassicActorSystemProvider
import pekko.annotation.InternalApi
import pekko.http.impl.model.JavaQuery
import pekko.http.impl.util.JavaMapping
import pekko.http.impl.util.JavaMapping.Implicits._
-import pekko.http.javadsl.model._
+import pekko.http.{ javadsl => jm }
+import jm.model._
import pekko.http.scaladsl.model.{ ContentTypeRange, ContentTypes }
import pekko.http.scaladsl.unmarshalling
import pekko.http.scaladsl.unmarshalling.FromEntityUnmarshaller
-import pekko.http.scaladsl.unmarshalling.Unmarshaller.{
- EnhancedFromEntityUnmarshaller,
- UnsupportedContentTypeException
-}
Review Comment:
This will break user's code
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "pjfanning (via GitHub)" <gi...@apache.org>.
pjfanning commented on PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#issuecomment-1865926963
added to 1.1.0 milestone - seems ok to me if targeted just at that milestone
I'm not fully convinced that we can't have some binary incompatibility in a 1.1.0 release but the changes here to maintain bin compatibility are not very complicated and look maintainable.
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#discussion_r1433391026
##########
http/src/main/scala/org/apache/pekko/http/scaladsl/unmarshalling/Unmarshaller.scala:
##########
@@ -190,7 +202,7 @@ object Unmarshaller
override def equals(that: Any): Boolean = that match {
case that: UnsupportedContentTypeException =>
- that.canEqual(this) && that.supported == this.supported && that.actualContentType == this.actualContentType
+ that.canEqual(this) && super.equals(this)
Review Comment:
This will point to `org.apache.pekko.http.javadsl.unmarshalling.Unmarshaller.UnsupportedContentTypeException` `equals` method which does the proper structural equality check.
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#discussion_r1433406059
##########
http/src/main/scala/org/apache/pekko/http/javadsl/unmarshalling/Unmarshaller.scala:
##########
@@ -14,21 +14,20 @@
package org.apache.pekko.http.javadsl.unmarshalling
import java.util.concurrent.CompletionStage
+import java.util.Optional
import org.apache.pekko
import pekko.actor.ClassicActorSystemProvider
import pekko.annotation.InternalApi
import pekko.http.impl.model.JavaQuery
import pekko.http.impl.util.JavaMapping
import pekko.http.impl.util.JavaMapping.Implicits._
-import pekko.http.javadsl.model._
+import pekko.http.{ javadsl => jm }
+import jm.model._
import pekko.http.scaladsl.model.{ ContentTypeRange, ContentTypes }
import pekko.http.scaladsl.unmarshalling
import pekko.http.scaladsl.unmarshalling.FromEntityUnmarshaller
-import pekko.http.scaladsl.unmarshalling.Unmarshaller.{
- EnhancedFromEntityUnmarshaller,
- UnsupportedContentTypeException
-}
Review Comment:
How, it passes MiMa? The exception being thrown here is the same.
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#discussion_r1433425460
##########
http/src/main/scala/org/apache/pekko/http/javadsl/unmarshalling/Unmarshaller.scala:
##########
@@ -85,8 +84,9 @@ object Unmarshaller extends pekko.http.javadsl.unmarshalling.Unmarshallers {
val mediaType = t.asScala
if (entity.contentType == ContentTypes.NoContentType || mediaType.matches(entity.contentType.mediaType)) {
um.asScala(entity)
- } else FastFuture.failed(UnsupportedContentTypeException(Some(entity.contentType),
- ContentTypeRange(t.toRange.asScala)))
+ } else FastFuture.failed(
+ pekko.http.scaladsl.unmarshalling.Unmarshaller.UnsupportedContentTypeException(Some(entity.contentType),
Review Comment:
So I had to undo this change because using a single inline import makes the code fail to compile on Scala 3 due to
```
[error] -- [E049] Reference Error: /home/runner/work/incubator-pekko-http/incubator-pekko-http/http/src/main/scala/org/apache/pekko/http/javadsl/unmarshalling/Unmarshaller.scala:88:33
[error] 88 | } else FastFuture.failed(UnsupportedContentTypeException(Some(entity.contentType),
```
A better alternative might be to throw the `javadsl` variant however this is technically a behavioural change (hypothetically users may be relying on the scala specific features of scaladsl when catching the exception).
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#discussion_r1433419881
##########
http/src/main/scala/org/apache/pekko/http/javadsl/unmarshalling/Unmarshaller.scala:
##########
@@ -85,8 +84,9 @@ object Unmarshaller extends pekko.http.javadsl.unmarshalling.Unmarshallers {
val mediaType = t.asScala
if (entity.contentType == ContentTypes.NoContentType || mediaType.matches(entity.contentType.mediaType)) {
um.asScala(entity)
- } else FastFuture.failed(UnsupportedContentTypeException(Some(entity.contentType),
- ContentTypeRange(t.toRange.asScala)))
+ } else FastFuture.failed(
+ pekko.http.scaladsl.unmarshalling.Unmarshaller.UnsupportedContentTypeException(Some(entity.contentType),
Review Comment:
Fixed and pushed.
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#discussion_r1433389812
##########
http/src/main/scala/org/apache/pekko/http/javadsl/unmarshalling/Unmarshaller.scala:
##########
@@ -124,6 +124,40 @@ object Unmarshaller extends pekko.http.javadsl.unmarshalling.Unmarshallers {
implicit mi: JavaMapping[JI, SI]): unmarshalling.Unmarshaller[JI, O] =
um.asInstanceOf[unmarshalling.Unmarshaller[JI, O]] // since guarantee provided by existence of `mi`
+ class UnsupportedContentTypeException(
+ private val _supported: java.util.Set[jm.model.ContentTypeRange],
+ private val _actualContentType: Optional[jm.model.ContentType])
+ extends RuntimeException(_supported.asScala.mkString(
Review Comment:
Converting the java data-structures to Scala is done here so that the runtime exception message is the same as it is currently. This is done just incase currently existing code is matching against the exception message although one can argue that you shouldn't be doing this, furthermore if users are doing this its likely they will just be matching against `Unsupported Content-Type` in which case this will still work.
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "laglangyue (via GitHub)" <gi...@apache.org>.
laglangyue commented on code in PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#discussion_r1433415236
##########
http/src/main/scala/org/apache/pekko/http/javadsl/unmarshalling/Unmarshaller.scala:
##########
@@ -14,21 +14,20 @@
package org.apache.pekko.http.javadsl.unmarshalling
import java.util.concurrent.CompletionStage
+import java.util.Optional
import org.apache.pekko
import pekko.actor.ClassicActorSystemProvider
import pekko.annotation.InternalApi
import pekko.http.impl.model.JavaQuery
import pekko.http.impl.util.JavaMapping
import pekko.http.impl.util.JavaMapping.Implicits._
-import pekko.http.javadsl.model._
+import pekko.http.{ javadsl => jm }
Review Comment:
Do you think of using `javadsl` instead of jm?
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#discussion_r1433418304
##########
http/src/main/scala/org/apache/pekko/http/javadsl/unmarshalling/Unmarshaller.scala:
##########
@@ -14,21 +14,20 @@
package org.apache.pekko.http.javadsl.unmarshalling
import java.util.concurrent.CompletionStage
+import java.util.Optional
import org.apache.pekko
import pekko.actor.ClassicActorSystemProvider
import pekko.annotation.InternalApi
import pekko.http.impl.model.JavaQuery
import pekko.http.impl.util.JavaMapping
import pekko.http.impl.util.JavaMapping.Implicits._
-import pekko.http.javadsl.model._
+import pekko.http.{ javadsl => jm }
Review Comment:
`jm` is whats used in the rest of the codebase so I kept the same convention.
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#issuecomment-1866152832
> added to 1.1.0 milestone - seems ok to me if targeted just at that milestone
fine with me
> I'm not fully convinced that we can't have some binary incompatibility in a 1.1.0 release but the changes here to maintain bin compatibility are not very complicated and look maintainable.
We did decide that both pekko and pekko-http strictly follow semver due to being critical components which is why https://github.com/apache/incubator-pekko-http/blob/38aa25e7c20232a4226c4ea5767813b99bca870a/build.sbt#L59 is set.
In any the point is moot since this PR goes out of its way to **not** break binary compatibility.
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#discussion_r1433389812
##########
http/src/main/scala/org/apache/pekko/http/javadsl/unmarshalling/Unmarshaller.scala:
##########
@@ -124,6 +124,40 @@ object Unmarshaller extends pekko.http.javadsl.unmarshalling.Unmarshallers {
implicit mi: JavaMapping[JI, SI]): unmarshalling.Unmarshaller[JI, O] =
um.asInstanceOf[unmarshalling.Unmarshaller[JI, O]] // since guarantee provided by existence of `mi`
+ class UnsupportedContentTypeException(
+ private val _supported: java.util.Set[jm.model.ContentTypeRange],
+ private val _actualContentType: Optional[jm.model.ContentType])
+ extends RuntimeException(_supported.asScala.mkString(
Review Comment:
Converting the java data-structures to Scala is done here so that the runtime exception message is the same as it is currently. This is done just incase currently existing code is matching against the exception message although one can argue that you shouldn't be doing this, furthermore if users are doing this its likely they will just be matching against `Unsupported Content-Type` in which case this will still work.
In short, I am not against adjust the exception message so it just calls `.toString` on the Java data structures.
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#issuecomment-1870818905
Going to go ahead and merge this
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich merged PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "mdedetrich (via GitHub)" <gi...@apache.org>.
mdedetrich commented on code in PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#discussion_r1433392622
##########
http/src/main/scala/org/apache/pekko/http/javadsl/unmarshalling/Unmarshaller.scala:
##########
@@ -124,6 +124,40 @@ object Unmarshaller extends pekko.http.javadsl.unmarshalling.Unmarshallers {
implicit mi: JavaMapping[JI, SI]): unmarshalling.Unmarshaller[JI, O] =
um.asInstanceOf[unmarshalling.Unmarshaller[JI, O]] // since guarantee provided by existence of `mi`
+ class UnsupportedContentTypeException(
+ private val _supported: java.util.Set[jm.model.ContentTypeRange],
+ private val _actualContentType: Optional[jm.model.ContentType])
+ extends RuntimeException(_supported.asScala.mkString(
+ s"Unsupported Content-Type [${_actualContentType.asScala}], supported: ", ", ", "")) {
+
+ def this(supported: jm.model.ContentTypeRange*) = {
+ this(supported.toSet.asJava, Optional.empty[jm.model.ContentType]())
+ }
+
+ def this(supported: java.util.Set[jm.model.ContentTypeRange]) = {
+ this(supported, Optional.empty[jm.model.ContentType]())
+ }
+
+ def this(contentType: Optional[jm.model.ContentType], supported: jm.model.ContentTypeRange*) = {
+ this(supported.toSet.asJava, contentType)
+ }
+
+ def toScala(): pekko.http.scaladsl.unmarshalling.Unmarshaller.UnsupportedContentTypeException =
+ pekko.http.scaladsl.unmarshalling.Unmarshaller.UnsupportedContentTypeException(
+ _supported.asScala.toSet.asInstanceOf[Set[pekko.http.scaladsl.model.ContentTypeRange]],
+ _actualContentType.asScala)
+
+ def getSupported(): java.util.Set[jm.model.ContentTypeRange] = _supported
+
+ def getActualContentType(): Optional[jm.model.ContentType] = _actualContentType
+
+ override def equals(that: Any): Boolean = that match {
+ case that: UnsupportedContentTypeException =>
+ that._supported == this._supported && that._actualContentType == this._actualContentType
Review Comment:
Since it may not be immediately obvious, both `java.util.Set` and `java.util.Optional` equals methods perform structural equality, not reference (I tested this in Scala repl).
--
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
Re: [PR] Add UnsupportedContentTypeException javadsl [incubator-pekko-http]
Posted by "He-Pin (via GitHub)" <gi...@apache.org>.
He-Pin commented on code in PR #376:
URL: https://github.com/apache/incubator-pekko-http/pull/376#discussion_r1433405927
##########
http/src/main/scala/org/apache/pekko/http/javadsl/unmarshalling/Unmarshaller.scala:
##########
@@ -85,8 +84,9 @@ object Unmarshaller extends pekko.http.javadsl.unmarshalling.Unmarshallers {
val mediaType = t.asScala
if (entity.contentType == ContentTypes.NoContentType || mediaType.matches(entity.contentType.mediaType)) {
um.asScala(entity)
- } else FastFuture.failed(UnsupportedContentTypeException(Some(entity.contentType),
- ContentTypeRange(t.toRange.asScala)))
+ } else FastFuture.failed(
+ pekko.http.scaladsl.unmarshalling.Unmarshaller.UnsupportedContentTypeException(Some(entity.contentType),
Review Comment:
split with a single import line ? this line is too long
--
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