vt-elixir / ja_serializer

JSONAPI.org Serialization in Elixir.
Other
640 stars 148 forks source link

Allow accept header with quality param #320

Closed lasseebert closed 5 years ago

lasseebert commented 5 years ago

Related to #138

When requesting with an Accept header with a quality param, e.g. */*;q=0.9, a 406 would be returned.

The quality should be used to prioritize the caller's different acceptable response content-types, but since we only care about jsonapi content type, we should just ignore it.

I also removed a test for Accept header application/vnd.api+json; charset=utf-8, since charset is not a valid param to Accept headers.

I found this when requesting a ja_serializer endpoint with Chrome. Chrome sends by default this accept header:

text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3

Which should be accepted by ja_serializer because of the */*;q=0.8 part.

(The changes looks a bit weird in the github diff editor since I removed one test and added another. This is the test I added)

beerlington commented 5 years ago

@lasseebert this makes sense. The only thing that's not clear is why you removed the 406 response test. Don't we still want that?

lasseebert commented 5 years ago

@beerlington The Accept header only accepts the q param, so my take was that we can throw away other params.

We can also throw away the q param, since we're only interessted in if the caller accepts jsonapi or not.

The deleted test tested that it would 406 on Accept: application/vnd.api+json; charset=utf-8, but IMHO we should just discard any invalid params. And so the application/vnd.api+json; charset=utf-8 with discarded params becomes application/vnd.api+json, which is valid.

lasseebert commented 5 years ago

Ah, I think I missed your point. We should still test 406 response and I deleted the only test that did that. Let me fix it. I'll push in a minute.

beerlington commented 5 years ago

Thanks!

lasseebert commented 5 years ago

Thanks for the quick review 👍