Skip to content

Conversation

@NomAnor
Copy link
Contributor

@NomAnor NomAnor commented Mar 13, 2023

First iteration to implement Accept header parsing and content negotioation. Documentation is missing until the code is stable.

Things to discuss/check

  • I'm not sure where to put the helper types, Accept might reside in request.py, the other I don't know.
  • Should MediaType be exposed or only used internally?
  • Are the tests appropiate?
  • Is the ordering correct based on RFC 2616?

MediaType class

Exposing the results as str is obviously easier. But when paramters are involved it gets more complicated:

Accept("text/plain,text/plain;p=test").best_match(["text/plain", "text/html"]) == "text/plain;p=test"

In a handler you would do:

preferred_type = request.accept.best_mach(["text/plain", "text/html"])
if preferred_type == "text/plain":
   return text_response

which would not match because of the missing parameter.

Implementing parameters might just be too cumbersome and so rarely used that it might be best to ignore them, except for q and for specificity. Or my logic could be better and the method should only return elements from provided_types.

PR Checklist

  • Have you followed the guidelines in CONTRIBUTING.rst?
  • Have you got 100% test coverage on new code?
  • Have you updated the prose documentation?
  • Have you updated the reference documentation?

@provinzkraut
Copy link
Member

which would not match because of the missing parameter.

IMO this can be simplified. best_match should always only return one of the types passed in. If you pass in text/plain;p=test, you may handle that case and get back a text/plain;p=test, but if you pass in text/plain, all you should get back is text/plain;p=test, regardless of the header value.

@provinzkraut
Copy link
Member

I'm not sure where to put the helper types, ``Accept` might reside in request.py, the other I don't know.

datastructures. We have an ETag header in headers.py there as well for example.

@peterschutt peterschutt changed the title Implement #1299: Accpet header parsing and content negotiation Implement #1299: Accept header parsing and content negotiation Mar 14, 2023
@NomAnor
Copy link
Contributor Author

NomAnor commented Mar 15, 2023

Should the Accept class inherit from Header and become a pydantic model?
It looks like those classes (ETag, CacheControlHeader) are only used for responses.

@provinzkraut
Copy link
Member

Should the Accept class inherit from Header and become a pydantic model?

Hm. I think we're running into a bit of an issue here. Conceptually, they should have the same base class. But as you pointed out, Header is currently only used for response headers. It should be usable for request headers as well. On the other hand, we don't really want to introduce pydantic models unnecessarily, as we're currently trying to move away from those and towards dataclasses.

On the other hand, there's no reason you can't just implement an Accept class that doesn't inherit from any of those. I think this might be the way to go here for now.

@NomAnor NomAnor force-pushed the content_negotiation branch from f5b34a5 to 7690ca1 Compare March 20, 2023 11:59
@NomAnor
Copy link
Contributor Author

NomAnor commented Mar 20, 2023

I moved the code to the dataclasses directory. I also changed it so the interface uses str and only internally the _MediaType class.
I added a small initial paragraph for content negotiation to the response documentation,but it needs a bit more polish.

…ation

Implement a property to access the parsed value of the 'Accept' header
from the request.

The returned instance allows iteration in the preferred order of media types.
It also provides the 'best_match()' method, that can be used to implement
content negotiation by finding the best matching media type for a list
of provided types.
@NomAnor NomAnor force-pushed the content_negotiation branch from b248f85 to 0d99b7f Compare April 3, 2023 13:05
@NomAnor NomAnor force-pushed the content_negotiation branch from 0d99b7f to 0bdd950 Compare April 3, 2023 13:11
@NomAnor
Copy link
Contributor Author

NomAnor commented Apr 3, 2023

I cleaned everything up, it should be ready for a final review.

I could not get sphinx to reference the accept property. Does anyone know why :attr:`Request.accept <starlite.connection.request.Request.accept>` does not work?

@NomAnor NomAnor marked this pull request as ready for review April 3, 2023 13:18
@NomAnor NomAnor requested a review from a team as a code owner April 3, 2023 13:18
@provinzkraut
Copy link
Member

provinzkraut commented Apr 3, 2023

I cleaned everything up, it should be ready for a final review.

I could not get sphinx to reference the accept property. Does anyone know why :attr:`Request.accept <starlite.connection.request.Request.accept>` does not work?

:attr:`Request.accept <starlite.connection.Request.accept>` or
:attr:`Request.accept <.connection.Request.accept>` should work.

The references don't work by their absolute path, but rather by how they're set up with autodoc. Since we don't expose this internal module structure to the user, we documented it accordingly, so as far as autodoc is concerned, starlite.connection.Request is the absolute path and starlite.connection.request does not exist.

@Goldziher Goldziher merged commit 6d71cbf into litestar-org:main Apr 5, 2023
@provinzkraut provinzkraut linked an issue Apr 8, 2023 that may be closed by this pull request
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.

Enhancement: Content negotiation and Accept header

4 participants