New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support parameters for custom media types #2005

Merged
merged 1 commit into from May 15, 2018

Conversation

Projects
None yet
4 participants
@richdougherty
Contributor

richdougherty commented May 2, 2018

When a custom media type is matched we now add parameters. In particular this now means that users can support the charset parameter in custom media types.

This change will be used downstream in Play's Akka HTTP backend. Play will be able to override the application/json media type to support an explicit charset parameter instead of having an implicit, fixed charset of UTF-8. Allowing an explicit charset parameter will fix several Play bugs related to interop with other HTTP agents.

See:

Support parameters for custom media types
When a custom media type is matched we now add parameters. In
particular this now means that users can support the charset
parameter in custom media types.

This change will be used downstream in Play's Akka HTTP backend.
Play will be able to override the application/json media type to
support an explicit charset parameter instead of having an implicit,
fixed charset of UTF-8. Allowing an explicit charset parameter
will fix several Play bugs related to interop with other HTTP
agents.
@@ -32,24 +31,27 @@ private[parser] trait CommonActions {
case custom MediaType.customMultipart(custom, params)
}
case mainLower
if (areNoCustomMediaTypesDefined) // try to prevent closure creation for usual case without custom media types

This comment has been minimized.

@richdougherty

richdougherty May 2, 2018

Contributor

Rather than having this condition I have simply inlined Option.fold. This allows the areNoCustomMediaTypesDefined method to be removed and thegetPredefinedOrCreateCustom method can be inlined and removed.

To me this reads better, but I could revert this if desired.

This comment has been minimized.

@raboof

raboof May 3, 2018

Member

Doesn't this change mean customMediaTypes(mainLower, subLower) will now be called even when there are no custom media types defined?

// Try user-defined function to get a MediaType
customMediaTypes(mainLower, subLower) match {
case Some(customMediaType) withParams(customMediaType)

This comment has been minimized.

@richdougherty

richdougherty May 2, 2018

Contributor

The main change is here: calling customMediaTypes.withParams(params).

This comment has been minimized.

@raboof

raboof May 3, 2018

Member

So after this change, when a request comes in for which a custom media type is defined for the mainLower/subLower part, it is selected but its parameters are replaced by the parameters from the incoming request.

This does mean registering a custom WithFixedCharset media type becomes weird: if you'd register customWithFixedCharset("foo", "bar", UTF8) and a request with foo/bar; charset=us-ascii comes in, you'd be left with a t: WithFixedCharset where t.charset is still UTF8 but t.params("charset") is us-ascii.

On the other hand, since custom media types are identified by their mainLower/subLower part so you can't have a 'fixed' and an 'open' version of the same type co-exist anyway, accepting the request with a slightly-'weird' media type might be better than rejecting the request outright.

@akka-ci

This comment has been minimized.

Collaborator

akka-ci commented May 2, 2018

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!

@@ -286,6 +286,8 @@ object MediaType {
}
object MediaTypes extends ObjectRegistry[(String, String), MediaType] {
/** Function used to find a custom media type. Called before the predefined media types. Strings will be lowercase. */
type FindCustom = (String, String) Option[MediaType]

This comment has been minimized.

@richdougherty

richdougherty May 2, 2018

Contributor

The public ParserSettings API currently only supports setting this via a Seq[MediaType]. That's fine for Play's needs for now, since Play only needs to patch application/json.

I have written another patch to the ParserSettings API that adds support for using a custom function. I will submit that patch separately after this PR has been merged.

@raboof

This comment has been minimized.

Member

raboof commented May 3, 2018

OK TO TEST

@akka-ci akka-ci added validating tested and removed validating labels May 3, 2018

@akka-ci

This comment has been minimized.

Collaborator

akka-ci commented May 3, 2018

Test PASSed.

@raboof

Unless I'm misreading this dropping areNoCustomMediaTypesDefined means we're now slower in that case, so perhaps it would be good to bring that back?

Regarding the main change, I'm inclined to be OK with it - the 'strange' corner case is a case where there's nothing more sensible we can do (beyond rejecting the request) anyway. Would appreciate another careful review though :).

@@ -32,24 +31,27 @@ private[parser] trait CommonActions {
case custom MediaType.customMultipart(custom, params)
}
case mainLower
if (areNoCustomMediaTypesDefined) // try to prevent closure creation for usual case without custom media types

This comment has been minimized.

@raboof

raboof May 3, 2018

Member

Doesn't this change mean customMediaTypes(mainLower, subLower) will now be called even when there are no custom media types defined?

// Try user-defined function to get a MediaType
customMediaTypes(mainLower, subLower) match {
case Some(customMediaType) withParams(customMediaType)

This comment has been minimized.

@raboof

raboof May 3, 2018

Member

So after this change, when a request comes in for which a custom media type is defined for the mainLower/subLower part, it is selected but its parameters are replaced by the parameters from the incoming request.

This does mean registering a custom WithFixedCharset media type becomes weird: if you'd register customWithFixedCharset("foo", "bar", UTF8) and a request with foo/bar; charset=us-ascii comes in, you'd be left with a t: WithFixedCharset where t.charset is still UTF8 but t.params("charset") is us-ascii.

On the other hand, since custom media types are identified by their mainLower/subLower part so you can't have a 'fixed' and an 'open' version of the same type co-exist anyway, accepting the request with a slightly-'weird' media type might be better than rejecting the request outright.

@johanandren

This comment has been minimized.

Member

johanandren commented May 3, 2018

I looked a bit at this, but felt unsure of the consequences. Will try to take a more careful look tomorrow.

@richdougherty

This comment has been minimized.

Contributor

richdougherty commented May 3, 2018

Doesn't this change mean customMediaTypes(mainLower, subLower) will now be called even when there are no custom media types defined?

Unless I'm misreading this dropping areNoCustomMediaTypesDefined means we're now slower in that case, so perhaps it would be good to bring that back?

According to the comment in the previous code the goal was to eliminate the allocation of a closure. The new code still avoids creating a closure. The previous code avoided the closure by comparing on the function to see if it was known to be a nop. The new code avoid the closure by comparing on the result of the function. In the common case calling the function and comparing the result will be fast.

Before: call areNoCustomMediaTypesDefined, equality check on customMediaTypes function
After: call customMediaTypes, pattern match on result (in fast case the result is always None and the pattern match is a single equality check)

Does that sound right? If so I think it should be about the same speed - one function call and one (reference?) equality check in either case?

IMO the new code seems like a cleaner way to do that, since it doesn't require extra methods across several different source files. The new code will also be faster when their is a custom media type defined but the function returns None since it won't allocate a closure even in that case.

This does mean registering a custom WithFixedCharset media type becomes weird: if you'd register customWithFixedCharset("foo", "bar", UTF8) and a request with foo/bar; charset=us-ascii comes in, you'd be left with a t: WithFixedCharset where t.charset is still UTF8 but t.params("charset") is us-ascii.

That does sound a bit odd. What I've done here is made custom media types behave the same way as predefined ones. What happens in the existing code when a predefined WithFixedCharset media type is given a charset param? Should custom and predefined media types behave differently or in the same way?

I looked a bit at this, but felt unsure of the consequences. Will try to take a more careful look tomorrow.

I probably should have mentioned that this was the approach that I worked out with @jrudolph - at least according to the old notes and code I left in the description of playframework/playframework#8239.

@raboof

This comment has been minimized.

Member

raboof commented May 4, 2018

It's not obvious to me whether the areNoCustomMediaTypesDefined call and the customMediaTypes call are the same cost in the case where no custom media types are defined - could be but I'd have to look a bit more carefully at it. I agree the new code looks cleaner so if it's indeed on par in terms of performance I'm all for it.

@johanandren

This comment has been minimized.

Member

johanandren commented May 4, 2018

I think I'd feel more confident about the consequences of the removed areNoCustomMediaTypesDefined boolean flag with some benchmark numbers, even though the comment said it was primarily about he closure allocation (it is a call to an overloaded method, that then references a function field in a different object etc). I think an additional pro with the old logic can also have been that it could completely JIT-eliminate that entire block in case of no custom media types (which I think is likely the most common case, outside of Play), while in this case the method.-function will always be called.

But as usual gut feeling is worth close to 0 with performance things, benching is the only thing we can trust.

Except for that I think the change looks fine.

@richdougherty

This comment has been minimized.

Contributor

richdougherty commented May 7, 2018

I noticed that @raboof is adding a very helpful microbenchmark over at #2007.

@raboof

This comment has been minimized.

Member

raboof commented May 14, 2018

Looking a bit closer now in the common no-custom-types case customMediaTypes is not some lookup but really the ConstantFun.scalaAnyTwoToNone function, so now I agree this looks fine, and also the benchmark from #2007 does not show any clear difference either way on my machine.

@johanandren

This comment has been minimized.

Member

johanandren commented May 14, 2018

Great, thanks for checking @raboof

@raboof

raboof approved these changes May 14, 2018

@raboof raboof merged commit 9b72bb8 into akka:master May 15, 2018

3 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@raboof raboof added this to the 10.1.2 milestone May 15, 2018

@richdougherty

This comment has been minimized.

Contributor

richdougherty commented May 30, 2018

Thanks guys!

@richdougherty

This comment has been minimized.

Contributor

richdougherty commented May 30, 2018

Hi, can we please get a backport of this to Akka HTTP 10.0.x so that we can support it in the stable 2.6.x branch of Play? That will let us fix this issue: playframework/playframework#8239

(Also do you want to tag this PR with play-akka-http-integration?)

johanandren added a commit to johanandren/akka-http that referenced this pull request May 30, 2018

Support parameters for custom media types (akka#2005)
When a custom media type is matched we now add parameters. In
particular this now means that users can support the charset
parameter in custom media types.

This change will be used downstream in Play's Akka HTTP backend.
Play will be able to override the application/json media type to
support an explicit charset parameter instead of having an implicit,
fixed charset of UTF-8. Allowing an explicit charset parameter
will fix several Play bugs related to interop with other HTTP
agents.

raboof added a commit that referenced this pull request May 31, 2018

Support parameters for custom media types (#2005) (#2037)
When a custom media type is matched we now add parameters. In
particular this now means that users can support the charset
parameter in custom media types.

This change will be used downstream in Play's Akka HTTP backend.
Play will be able to override the application/json media type to
support an explicit charset parameter instead of having an implicit,
fixed charset of UTF-8. Allowing an explicit charset parameter
will fix several Play bugs related to interop with other HTTP
agents.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment