Skip to content

Add content::{Accept, ContentType} headers #270

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

Merged
merged 9 commits into from
Dec 18, 2020
Merged

Add content::{Accept, ContentType} headers #270

merged 9 commits into from
Dec 18, 2020

Conversation

yoshuawuyts
Copy link
Member

Adds the content::{Accept, ContentType} headers, implementing content negotation for media types. Supersedes #269 which only implemented content::ContentType. Ref #99. Thanks!

Copy link
Member

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

I don't think we should merge this until the API for content-type is flushed out more.

As far as I am aware, there are 4 main uses for Content-Type:

  • Accept, i.e. negotiation. Covered in this PR.
  • Determining file type from http data
  • Determining http data from file and/or path
  • Determining if a response should be compressed on-demand

The latter 3 are not covered here and are, in my experience, the more necessary usage.

Here's some 23:00 ideas:

  • ext() -> &str file extension as string
  • From<Path> & from_ext(&str)
  • compressible() -> bool

All of these require some kind of in-memory database. I was trying to figure out what structure to use for that but really wasn't sure. However we should populate that database from mime-db, since it is by far the most comprehensive source out there (it is an aggregate plus more).

We could also do mime sniffing... but that's kinda costly, requires intercepting bodies, is possibly less reliable, etc

@yoshuawuyts
Copy link
Member Author

yoshuawuyts commented Nov 4, 2020

@Fishrock123 To me it sounds like the features you're suggesting may be out of scope for this PR, even if they seem useful. This PR introduces two typed headers only:

  • Accept which is used for content negotiation.
  • Content-Type which is used to communicate the media type used.

Both headers implement the full spec, and in addition to that we also include the content negotiation algorithm to actually make Accept useful.

Your suggestions around mime-db likely best be implemented as extensions to our existing http_types::mime::Mime implementation. Mime already has some capabilities to infer the type either by reading magic bytes (Mime::sniff) or by attempting to guess the mime from the file extension (Mime::from_extension).

We also already have a direct "create a mime from a file" conversion available through (Body::from_file), which applies both the from_extension and sniff methods to determine the file type. Finally Mime also has a Display impl that can be used to convert it back out to a string. Though a Mime -> file extension conversion may indeed be useful.

In short: I think your suggestions are good, but they don't need to block this PR. I've filed separate issues to track what's not been implemented so we can remember to implement them later:

@Fishrock123
Copy link
Member

I don't think we should merge this until the API for content-type is flushed out more.

Ah, well, this and the issues from #274 are what I meant by this - i.e. that we have at least made an effort to think about it.

Copy link
Member

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

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

Overall looks good, with some minor fixes & questions.

// NOTE: Firefox populates Accept-Accept as `gzip, deflate, br`. This means
// when parsing media_types we should choose the last value in the list under
// equal weights. This impl doesn't know which value was passed later, so that
// behavior needs to be handled separately.
Copy link
Member

Choose a reason for hiding this comment

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

This comment seems to only be for Accept-Encoding

yoshuawuyts and others added 2 commits December 18, 2020 16:03
Co-authored-by: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@yoshuawuyts yoshuawuyts merged commit 9bafdd6 into main Dec 18, 2020
@delete-merged-branch delete-merged-branch bot deleted the accept branch December 18, 2020 15:11
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.

2 participants