luckyframework / lucky

A full-featured Crystal web framework that catches bugs for you, runs incredibly fast, and helps you write code that lasts.
https://luckyframework.org
MIT License
2.57k stars 156 forks source link

Revamp Accept header handling #1769

Closed wezm closed 1 year ago

wezm commented 1 year ago

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

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

Blacksmoke16 commented 1 year ago

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

jwoertink commented 1 year ago

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 commented 1 year ago

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.

jwoertink commented 1 year ago

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

robacarp commented 1 year ago

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