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

New implementation of Content-Type and Accept header route matchers #424

Merged
merged 2 commits into from
May 20, 2020

Conversation

msrd0
Copy link
Member

@msrd0 msrd0 commented Apr 25, 2020

I have reimplemented the Content-Type and Accept header matchers to suit more advanced needs. This includes a fix for #381. Both now use a HashMap for efficient lookup and also

The Accept matcher

  • now supports more than one type from the header
  • supports the quality-weighted syntax, although qualities are ignored
  • will not only match */* as a wildcard, but also types with subtype-wildcards like image/*
  • will now ignore parameters like charset=utf-8 as they are not supposed to be part of the Accept header, and conflict with the list syntax as both use commas as separators

The Content-Type matcher

  • now matches content types in respect of their parameters, so that text/plain; charset=utf-8 is considered a match when text/plain was expected, however, text/plain; charset=utf-8 is not a match if text/plain; charset=us-ascii was expected
  • allows the user to optionally specify that requests without a Content-Type header are acceptable, which is at least useful during development when your curl commands get shorter

Also, I have added public re-exports in their respective super mod for ContentTypeHeaderRouteMatcher and RouteNonMatch to simplify user code.

@codecov
Copy link

codecov bot commented Apr 25, 2020

Codecov Report

Merging #424 into master will increase coverage by 0.36%.
The diff coverage is 92.30%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #424      +/-   ##
==========================================
+ Coverage   83.05%   83.41%   +0.36%     
==========================================
  Files         106      106              
  Lines        5027     5156     +129     
==========================================
+ Hits         4175     4301     +126     
- Misses        852      855       +3     
Impacted Files Coverage Δ
gotham/src/extractor/internal.rs 89.64% <ø> (ø)
gotham/src/router/mod.rs 89.94% <ø> (ø)
gotham/src/router/route/matcher/mod.rs 64.70% <ø> (ø)
gotham/src/router/route/matcher/content_type.rs 91.78% <91.54%> (+91.78%) ⬆️
gotham/src/router/route/matcher/accept.rs 92.30% <92.00%> (+92.30%) ⬆️
gotham/src/router/route/matcher/lookup_table.rs 100.00% <100.00%> (ø)
misc/borrow_bag/src/lookup.rs 75.00% <0.00%> (-25.00%) ⬇️
examples/templating/askama/src/main.rs 66.66% <0.00%> (-7.41%) ⬇️
gotham/src/helpers/http/request/path.rs 78.57% <0.00%> (-7.15%) ⬇️
gotham/src/handler/mod.rs 77.27% <0.00%> (-4.55%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eeed281...63ec2a9. Read the comment docs.

nyarly
nyarly previously approved these changes Apr 28, 2020
Copy link
Contributor

@nyarly nyarly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fantastic. I've created a couple issues around respecting q parameters, and updating the examples, but I'd rather come closer to the conneg spec than block on perfection.

@msrd0
Copy link
Member Author

msrd0 commented Apr 28, 2020

Well, as I understand the task of the accept header matcher is to see whether there is a match, but has no way to inform the handler as to what quality that match is. Even worse, when a handler has several types it can produce, there is currently no way the matcher can inform the handler which of the types actually produces that match. This is most certainly an issue but sounds like it can't (currently) be solved through a route matcher.

Also, I have kept your documentation on both matcher, although the code contained in them reads more as a test than as a usage example. Let me know if you'd like to see this changed.

@msrd0
Copy link
Member Author

msrd0 commented Apr 28, 2020

I just caught a case I hadn't considered yet and pushed a fix for it.

@msrd0 msrd0 requested a review from nyarly May 5, 2020 14:09
@nyarly nyarly merged commit 992e0e7 into gotham-rs:master May 20, 2020
@msrd0 msrd0 deleted the path-matchers branch May 20, 2020 06:35
@pksunkara
Copy link
Contributor

@msrd0 I have a small question. This is just for matching, right? So, the router looks for matches and if the route doesn't match because of this, it would check other routes and at the end returns 404. But not 406 or 415. Or am I mistaken?

@msrd0
Copy link
Member Author

msrd0 commented May 28, 2020

@pksunkara Well, I guess the idea is for gotham to return the NonMatch status code if this route didn't match and no other matching route was found. However, I found this behaviour to be inconsistent. For example, when I tried to return a NonMatch with status code 404, I got a 405 from the router instead (#434).

This was referenced Aug 26, 2020
@msrd0 msrd0 added this to the 0.5 milestone Aug 26, 2020
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