Added logging unsupported content type #1713

oleg-smith · 2017-12-25T05:49:17Z

No description provided.

akka-ci · 2017-12-25T05:50:05Z

Can one of the repo owners verify this patch?

jlprat · 2017-12-25T07:45:16Z

OK TO TEST

akka-ci · 2017-12-25T07:55:14Z

Test FAILed.

oleg-smith · 2017-12-25T08:02:06Z

@jlprat Do you think the binary compatibility problem can be ignored?

jlprat · 2017-12-25T08:20:13Z

Sorry I'm not close to a computer now, I will take a look later, but as a general rule binary compatible problems can only be "ignored" if the class is labeled as @InternalApi or @ApiMayChange. There are other cases when adding methods though.

jlprat · 2017-12-26T19:38:12Z

Hi @oleg-smith
Sorry for the delay, but I was away from a computer. IMO this change is binary incompatible the way it is. For example, the Exception UnsupportedContentTypeException now has a different constructor and any 3rd party library built upon previous versions of Akka HTTP will fail if we introduce this change.
I would try to implement this change in a compatible way, for example, offering 2 constructors and some default value for contentType.

jlprat · 2017-12-26T19:38:59Z

One extra thing, is there an issue for this PR?

oleg-smith · 2017-12-26T21:24:04Z

Yes, #1691

oleg-smith · 2017-12-26T21:24:38Z

Will implement it in compatible way.

# Conflicts: # akka-http/src/main/scala/akka/http/scaladsl/common/StrictForm.scala # akka-http/src/main/scala/akka/http/scaladsl/server/directives/MarshallingDirectives.scala # akka-http/src/main/scala/akka/http/scaladsl/unmarshalling/Unmarshaller.scala

akka-ci · 2017-12-30T22:23:30Z

Test FAILed.

akka-ci · 2017-12-31T16:18:22Z

Test FAILed.

akka-ci · 2018-09-16T15:21:41Z

Test PASSed.

oleg-smith · 2018-09-16T15:22:50Z

@raboof Could you merge this?

johanandren

I think this requires some further work before it can be merged. Mostly small things though.

johanandren · 2018-09-19T14:06:15Z

akka-http-core/src/test/scala/akka/http/scaladsl/GracefulTerminationSpec.scala

@@ -122,7 +122,7 @@ class GracefulTerminationSpec extends WordSpec with Matchers with BeforeAndAfter

    "in-flight request responses should include Connection: close and connection should be closed" in new TestSetup {
      val r1 = makeRequest() // establish connection
-      val time: FiniteDuration = 3.seconds
+      val time: FiniteDuration = 6.seconds


This is unrelated, revert.

This test fails due to lack of time on Jenkins. How can I make the unrelated test to pass otherwise?

Then it should fail for all other PR validation and our nightlies on Jenkins as well, which I'm pretty sure it doesn't.

No, it should not - it can fail from time to time depending on agent.

johanandren · 2018-09-19T14:11:04Z

akka-http/src/main/scala/akka/http/scaladsl/common/StrictForm.scala

@@ -125,10 +126,10 @@ object StrictForm {
          }

        tryUnmarshalToQueryForm.fast.recoverWith {
-          case Unmarshaller.UnsupportedContentTypeException(supported1) ⇒
+          case UnsupportedContentTypeException(supported1, _) ⇒


if this is required we still have a binary compatibility problem, I think we need the single param unapply or else it could potentially break usage like the old one.

isn't binary compatibility checked automatically so nobody guesses about it?

Unfortunately there are some cases that are not yet caught by MiMa, and this is one of them: lightbend-labs/mima#40

johanandren · 2018-09-19T14:14:33Z

akka-http/src/main/scala/akka/http/scaladsl/server/Rejection.scala

  override def getSupported: java.util.Set[model.ContentTypeRange] =
    scala.collection.mutable.Set(supported.map(_.asJava).toVector: _*).asJava // TODO optimise
+
+  @deprecated("for binary compatibility")


All these should have a version as well, so @deprecated("Something something", since = "10.1.6"), they should also rather mention what to use instead. So, in this case "Use the two parameter constructor instead".

For the copy I guess the alternative now is to create a new object manually, so same there.

johanandren · 2018-09-19T14:16:50Z

akka-http/src/main/scala/akka/http/scaladsl/server/Rejection.scala

+  def copy(
+    supported:   Set[ContentTypeRange] = this.supported,
+    contentType: Option[ContentType]   = this.contentType) =
+    UnsupportedRequestContentTypeRejection(supported, contentType)


This is the new two-param copy so it shouldn't be deprecated?

johanandren · 2018-09-19T14:21:17Z

akka-http/src/main/scala/akka/http/javadsl/server/Rejections.scala

+      supported = supported.asScala.map(m ⇒ scaladsl.model.ContentTypeRange(m.asScala)).toSet,
+      contentType = contentType.asScala.map(_.asScala))
+
+  // for backwards compatibility


should be marked deprecated as well

johanandren · 2018-09-19T14:31:53Z

akka-http/src/main/scala/akka/http/scaladsl/server/RejectionHandler.scala

@@ -257,7 +257,8 @@ object RejectionHandler {
      }
      .handleAll[UnsupportedRequestContentTypeRejection] { rejections ⇒
        val supported = rejections.flatMap(_.supported).mkString(" or ")
-        rejectRequestEntityAndComplete((UnsupportedMediaType, "The request's Content-Type is not supported. Expected:\n" + supported))
+        val unsupportedContentType = rejections.find(_.contentType.isDefined).map(_.contentType)


This will be an Option[Option[ContentType]] now. I think you can just do a rejections.flatMap(_.contentType) instead.

johanandren · 2018-09-19T14:32:45Z

akka-http/src/main/scala/akka/http/scaladsl/unmarshalling/Unmarshaller.scala

+    * Helper for creating a "super-unmarshaller" from a sequence of "sub-unmarshallers", which are tried
+    * in the given order. The first successful unmarshalling of a "sub-unmarshallers" is the one produced by the
+    * "super-unmarshaller".
+    */


This is not the scaladoc style we use, please revert

doesn't validatePullRequest format code properly?

Could be that it doesn't reformat scaladoc, not sure.

I am also surprised that scalariform doesn't update this indentation, but I just checked and indeed it doesn't.

raboof

Are you still interested in having a look at these review comments?

oleg-smith · 2018-10-16T16:12:16Z

@raboof yeah, will do

akka-ci · 2019-02-12T15:27:25Z

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

akka-ci · 2019-03-05T11:38:45Z

Test PASSed.

akka-ci · 2019-03-05T12:15:53Z

Test FAILed.

Rebased/formatted version of #1713

raboof · 2019-07-09T09:03:18Z

(closing due to inactivity, feel free to reopen when someone wants to resume this work!)

Rebased/formatted version of #1713

Rebased/formatted version of #1713 Co-authored-by: oleg <[email protected]>

Added logging unsupported content type

5951ab6

Added logging unsupported content type akka#1691

815bd6d

akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels Dec 25, 2017

oleg added 3 commits December 30, 2017 23:19

Fixed binary compatibility

c184319

Fixed binary compatibility

1810e09

Added content type to rejection

7ff18ce

Fixing binary compatibility

1959cfa

akka-ci added the validating PR that is currently being validated by Jenkins label Jan 1, 2018

akka-ci added tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Sep 16, 2018

johanandren reviewed Sep 19, 2018

View reviewed changes

raboof reviewed Oct 16, 2018

View reviewed changes

raboof assigned oleg-smith Dec 19, 2018

akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Mar 5, 2019

Autoformatted code

a7b5f73

akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Mar 5, 2019

raboof added a commit that referenced this pull request Apr 23, 2019

Add logging unsupported content type

3e8aece

Rebased/formatted version of #1713

raboof mentioned this pull request Apr 23, 2019

Add logging unsupported content type #2512

Merged

raboof closed this Jul 9, 2019

raboof added a commit that referenced this pull request Jul 9, 2019

Add logging unsupported content type

d878c99

Rebased/formatted version of #1713

raboof added a commit that referenced this pull request Jul 9, 2019

Add logging unsupported content type

b29f2c0

Rebased/formatted version of #1713

raboof added a commit that referenced this pull request Jul 9, 2019

Add logging unsupported content type

101fb8d

Rebased/formatted version of #1713

raboof added a commit that referenced this pull request Aug 14, 2019

Add logging unsupported content type

c85685f

Rebased/formatted version of #1713

jrudolph pushed a commit that referenced this pull request Sep 9, 2019

Add logging unsupported content type (#2512)

82b5057

Rebased/formatted version of #1713 Co-authored-by: oleg <[email protected]>

Added logging unsupported content type #1713

Added logging unsupported content type #1713

Conversation

oleg-smith commented Dec 25, 2017

akka-ci commented Dec 25, 2017

jlprat commented Dec 25, 2017

akka-ci commented Dec 25, 2017

oleg-smith commented Dec 25, 2017

jlprat commented Dec 25, 2017

jlprat commented Dec 26, 2017

jlprat commented Dec 26, 2017

oleg-smith commented Dec 26, 2017

oleg-smith commented Dec 26, 2017

akka-ci commented Dec 30, 2017

akka-ci commented Dec 31, 2017

akka-ci commented Sep 16, 2018

oleg-smith commented Sep 16, 2018

johanandren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raboof left a comment

Choose a reason for hiding this comment

oleg-smith commented Oct 16, 2018

akka-ci commented Feb 12, 2019

akka-ci commented Mar 5, 2019

akka-ci commented Mar 5, 2019

raboof commented Jul 9, 2019