Skip to content
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

Update request content-type handling #937

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

potomak
Copy link
Contributor

@potomak potomak commented Apr 5, 2018

In case that a sub-server doesn't support the content-type specified
in the request invoke delayedFail instead of delayedFailFatal in
order to give the chance to other sub-servers to handle the request.

Right now when you want to implement a server that supports multiple
content types on the same path with different handlers you muse use
the Raw and implement a Wai app. Otherwise the first sub-server, if
you try to use Servant type system to define the API it will either
match the content type of the request and serve a response or return
a 415 error.

This update (in theory) makes it possible to serve multiple
content-types with different APIs and their corresponding handlers.

In case that a sub-server doesn't support the content-type specified
in the request invoke `delayedFail` instead of `delayedFailFatal` in
order to give the chance to other sub-servers to handle the request.
@alpmestan
Copy link
Contributor

Hello! It certainly would be nice to support this, as long as it doesn't break subtly in other cases all of a sudden. How sure are we that this is not going to lead to bad surprises? Can you think of any scenario where this breaks existing behaviour? Or are we certain this strictly enhances the router?

@potomak
Copy link
Contributor Author

potomak commented Apr 5, 2018

@alpmestan thanks for responding so quickly. I'm not aware of subtle cases where this update would break existing behavior. Existing tests pass and a new test that I added, to check that the server responds with a 415 in case of content-type not supported, works as well.

I think that also from a performance perspective this change should be fine, because implementing the same behavior using a pattern match on the content-type header value would have a similar complexity as trying to match the routes for all the sub-servers.

@alpmestan
Copy link
Contributor

It does seem like to be a benign change. Perhaps @phadej and @kosmikus can take a quick look too, before we merge? (within a few days, if they get a chance to)

@phadej
Copy link
Contributor

phadej commented Apr 5, 2018

I wonder why

  :<|> "path5" :> (ReqBody '[JSON] Int      :> Post '[PlainText] Int
             :<|>  ReqBody '[PlainText] Int :> Post '[PlainText] Int)

and not simply

  :<|> "path5" :> (ReqBody '[JSON, PlainText] Int      :> Post '[PlainText] Int

You can also do

  :<|> "path5" :> (ReqBody '[JSON, CSV] Foo      :> Post '[JSON, CSV] Bar

so you get four possible combinations, including "give csv, get json". Is there real business application need to route based on content-types (of reqbody or result?)

@potomak
Copy link
Contributor Author

potomak commented Apr 5, 2018

Well I did

  :<|> "path5" :> (ReqBody '[JSON] Int      :> Post '[PlainText] Int
             :<|>  ReqBody '[PlainText] Int :> Post '[PlainText] Int)

instead of

  :<|> "path5" :> (ReqBody '[JSON, PlainText] Int      :> Post '[PlainText] Int

to test that the update worked as expected.

I think I knew you can do:

  :<|> "path5" :> (ReqBody '[JSON, CSV] Foo      :> Post '[JSON, CSV] Bar

but I had a case where I would like to return two different types Bar and Baz.

Maybe less a intrusive approach would be to wrap them in a new type?

Before I submitted this PR I thought that this kind of routing was possible, so I tried it without thinking and I come up with the behavior that I described, that is the first sub-server caught all requests and in case it didn't support the content-type of a request the server returned a 415.

I have a real case application where a common endpoint POST /execute branches according to the request content-type.

I think that this behavior works for the Accept header value, so I was wondering why not doing the same for a request content-type.

@phadej
Copy link
Contributor

phadej commented Apr 5, 2018

Ok, we have this for Accept, I don't see a problem to do this. As we only recover what we should use to parse, but do the (expensive) parsing still only once, I think this is ok.

I cannot think of a case where this would case routing performance degration, as content-type check is even after accept heck in DelayedIO.

We discussed this with @alpmestan on IRC, this is good. 👍.

@phadej phadej merged commit a8cd6e3 into haskell-servant:master Apr 5, 2018
@potomak
Copy link
Contributor Author

potomak commented Apr 5, 2018

Thanks @alpmestan and @phadej.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants