Add UnsupportedContentTypeException javadsl #376
Conversation
mdedetrich
force-pushed
the
add-unsupported-conten-type-exception-javadsl
branch
from
5244c10 to
ab3bcb1
Compare
| class UnsupportedContentTypeException( | ||
| private val _supported: java.util.Set[jm.model.ContentTypeRange], | ||
| private val _actualContentType: Optional[jm.model.ContentType]) | ||
| extends RuntimeException(_supported.asScala.mkString( |
There was a problem hiding this 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.
| @@ -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) | |||
There was a problem hiding this comment.
This will point to org.apache.pekko.http.javadsl.unmarshalling.Unmarshaller.UnsupportedContentTypeException equals method which does the proper structural equality check.
|
|
||
| override def equals(that: Any): Boolean = that match { | ||
| case that: UnsupportedContentTypeException => | ||
| that._supported == this._supported && that._actualContentType == this._actualContentType |
There was a problem hiding this 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).
mdedetrich
requested review from
jrudolph,
raboof,
gmethvin,
He-Pin,
nvollmar,
seglo and
pjfanning
| import pekko.http.scaladsl.unmarshalling.Unmarshaller.{ | ||
| EnhancedFromEntityUnmarshaller, | ||
| UnsupportedContentTypeException | ||
| } |
There was a problem hiding this comment.
How, it passes MiMa? The exception being thrown here is the same.
| } else FastFuture.failed(UnsupportedContentTypeException(Some(entity.contentType), | ||
| ContentTypeRange(t.toRange.asScala))) | ||
| } else FastFuture.failed( | ||
| pekko.http.scaladsl.unmarshalling.Unmarshaller.UnsupportedContentTypeException(Some(entity.contentType), |
There was a problem hiding this comment.
split with a single import line ? this line is too long
There was a problem hiding this comment.
Fixed and pushed.
There was a problem hiding this 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).
|
|
||
| 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 } |
There was a problem hiding this comment.
Do you think of using javadsl instead of jm?
There was a problem hiding this comment.
jm is whats used in the rest of the codebase so I kept the same convention.
mdedetrich
force-pushed
the
add-unsupported-conten-type-exception-javadsl
branch
from
ab3bcb1 to
ffbd611
Compare
mdedetrich
force-pushed
the
add-unsupported-conten-type-exception-javadsl
branch
from
ffbd611 to
3463293
Compare
|
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. |
fine with me, that was the original intent anyways.
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. |
|
Going to go ahead and merge this |
This PR adds a
javadslequivalent oforg.apache.pekko.http.scaladsl.unmarshalling.Unmarshaller.UnsupportedContentTypeExceptionwithout breaking binary compatibility (you can confirm this by runningsbt mimaReportBinaryIssues)While the changes aren't as trivial as what would be considered ideal, they are necessary in order to not break MiMa. Of note is that the core data structure for the exception is being changed from Scala
Set/Optionto JavaSet/Optional. Additionally the javadsl version ofUnsupportedContentTypeExceptiondoesn't support Scala specific language features (i.e.Product/Serializable/canEqual).