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

Revamp Accept header handling #1769

Merged
merged 8 commits into from
Jan 6, 2023
Merged

Conversation

wezm
Copy link
Contributor

@wezm wezm commented Dec 30, 2022

Purpose

This PR revamps the handling of the Accept header to be less adhoc and more in line with the HTTP specification.

Fixes #1766
Fixes #986

Description

Since I have run into two Accept related issues over the years I decided to take a stab at improving the way it's handled. The changes achieve the above purpose by parsing the header according to the grammar in the spec and ordering the media ranges by their weight/qfactor. Lastly it supports wildcards when matching in order to find the client's preferred format.

There are two main concepts introduced:

  1. AcceptList, which is a list of MediaRange sorted by qfactor
  2. MediaRange, which represents a parsed and valid media range. Example media ranges are */*, image/*, text/html, text/plain; charset=UTF-8, and text/plain; charset=utf-8; q=0.8.

With these two classes a matching algorithm is implemented where media ranges that the client accepts are matched against the accepted formats of the action with fallback behaviour that depends on whether the client has a */* wildcard pattern or not.

Checklist

  • - An issue already exists detailing the issue/or feature request that this PR fixes
  • - All specs are formatted with crystal tool format spec src
  • - Inline documentation has been added and/or updated
  • - Lucky builds on docker with ./script/setup
  • - All builds and specs pass on docker with ./script/test

I've left the commits I made as-is but would be open to squashing them if this was preferred.

@Blacksmoke16
Copy link

If you guys are okay with another dependency, could think about using https://athenaframework.org/Negotiation/. Like how it's used in invidious.

@jwoertink
Copy link
Member

Wow! Nice work on this @wezm. I'm not familiar with the header spec, but it looks like you've done quite a bit here. I'll need to take some time to understand what all is going on.

Thanks for the suggestion @Blacksmoke16. I'm also not opposed to an extra dependency if it means we get tons of flexibility.

I guess my main question here is, if I only want my action to support HTML, and no other format, if someone tries to pass an Accept header for anything other than HTML, will this still raise an exception? It looks like it does, but my brain is still waking up.

@wezm
Copy link
Contributor Author

wezm commented Dec 30, 2022

I guess my main question here is, if I only want my action to support HTML, and no other format, if someone tries to pass an Accept header for anything other than HTML, will this still raise an exception? It looks like it does, but my brain is still waking up.

Yes it should. I aimed to preserve all the existing behaviour and all the existing tests continue to pass without edit. In addition the tests I added for the issues I raised also pass now.

src/lucky/mime_type.cr Outdated Show resolved Hide resolved
@jwoertink
Copy link
Member

Yeah, looking through this, it feels a lot more flexible than what we have now. I'm down to give it a shot!

per ameba suggestion
@robacarp
Copy link
Contributor

robacarp commented Jan 5, 2023

This is awesome, the accepts header has needed work for a while and has bitten me several times.

@jwoertink jwoertink merged commit 78d8447 into luckyframework:main Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants