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

Add `content_negociation` extension #21

Merged
merged 10 commits into from Nov 8, 2017

Conversation

2 participants
@davidbgk
Contributor

davidbgk commented Nov 7, 2017

To reject unacceptable client requests based on the Accept header

Add `content_negociation` extension
To reject unacceptable client requests based on the `Accept` header

@davidbgk davidbgk requested a review from yohanboniface Nov 7, 2017

if 'Content-Length' not in self.response.headers:
length = len(self.response.body)
self.response.headers['Content-Length'] = length
if 'Content-Type' not in self.response.headers:

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

I'm not sure this really serve a use case. Plus it costs a not in for each request (not a big deal but…).
I'd better let the user explicitly set it.
Or at least have a DEFAULT_CONTENT_TYPE property, so it can be changed.

This comment has been minimized.

@davidbgk

davidbgk Nov 7, 2017

Contributor

I set a default.

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

After a second thought, I really think this can be treacherous. IMHO Content-Type should be set explicitly (or using dedicated shortcuts like json).

This comment has been minimized.

@davidbgk

davidbgk Nov 7, 2017

Contributor

Any HTTP/1.1 message containing an entity-body SHOULD include a
Content-Type header field defining the media type of that body. If
and only if the media type is not given by a Content-Type field, the
recipient MAY attempt to guess the media type via inspection of its
content and/or the name extension(s) of the URI used to identify the
resource. If the media type remains unknown, the recipient SHOULD
treat it as type "application/octet-stream".

I really think as a web framework we should set a default, if it's easy to override both per view and for the whole app.

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

SHOULD

Means it's valid without ;)

But my point is that we are implicitly setting a value, and we have only one chance over X that this value is useful. So I think setting the correct value, per view, is the role of the user, not the one of the framework (which is blind on this topic).
If you really want to be educational, raise if the header is not found (but that would not be correct given the header is not mandatory as per the HTTP spec you quoted above) ;)

if methods is None:
methods = ['GET']
def wrapper(func):
self.routes.add(path, **{m: func for m in methods})
handlers = {method: func for method in methods}

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

That way we are mixing methods and extra in the same space.
Maybe what we should do: add a has_route method to autoroute, so we can make the payload merge our-selves, and thus have a handlers key.

This comment has been minimized.

@davidbgk

davidbgk Nov 7, 2017

Contributor

I agree that it may be the role of autoroutes to deal with that.

@app.listen('dispatch')
async def reject_unacceptable_requests(request, payload):
accept = request.headers.get('Accept', '*/*')
if get_best_match(accept, payload['accepts']) is None:

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

I wonder if the return value of get_best_match should not be saved in some way?
I guess at some point the value of Accept header should be used to define which format to be used as response body?

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

Other thought: should the route key be accepts, given at the end this is what the view can serve as content type, no?
So accepts seems to me the point of view of the request.

This comment has been minimized.

@davidbgk

davidbgk Nov 7, 2017

Contributor

I wonder if the return value of get_best_match should not be saved in some way?

It's hard to guess without a use-case, I'd say let's wait for it to appear.

I guess at some point the value of Accept header should be used to define which format to be used as response body?

Not exactly, it can be a bit different so it's only for the check.

at the end this is what the view can serve as content type, no?

It can be a bit different, for instance the client asks for Accept: application/vnd.example.resource+json; version=2 and the server returns Content-Type: application/vnd.example.resource+json; version=2.1.3

protocol.on_message_begin()
protocol.on_url(path.encode())
protocol.request.body = body
protocol.request.method = method
protocol.request.headers = headers
return await self.app(protocol.request, protocol.response)
await self.app(protocol.request, protocol.response)

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

I suggest to extract this change to a separate commit. IMHO this should be merged right away.

This comment has been minimized.

@davidbgk

davidbgk Nov 7, 2017

Contributor

Is that this urgent? #lazyme

@@ -1,3 +1,4 @@
autoroutes==0.2.0
httptools==0.0.9
mimetype-match==1.0.4

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

Should we have a requirements_ext.txt, or should we just document the fact that this package needs be installed for the content_negotiation use?
We could easily check in the extension call that the module is installed, and raise if not.

@@ -240,3 +251,12 @@ Fired in case of error, can be at each request.
Use it to customize HTTP error formatting for instance.
Receives `request`, `response` and `error` parameters.
### dispatch

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

Another option: attach the route payload to the request, and let the users use the request hook instead of adding a new one?

davidbgk added some commits Nov 7, 2017

if 'Content-Length' not in self.response.headers:
length = len(self.response.body)
self.response.headers['Content-Length'] = length
if 'Content-Type' not in self.response.headers:

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

After a second thought, I really think this can be treacherous. IMHO Content-Type should be set explicitly (or using dedicated shortcuts like json).

@@ -270,7 +275,7 @@ def __init__(self):
def factory(self):
return self.Protocol(self)
def route(self, path: str, methods: list=None, **extras):
def route(self, path: str, methods: list=None, **extras: dict):

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

Can **extras be something else than a dict?

This comment has been minimized.

@davidbgk

davidbgk Nov 7, 2017

Contributor

Good question, I guess no, it's more as a matter of consistency in my experiment to annotate all parameters but it's quite useless… (for now!)

params, handler = await self.dispatch(request)
if request.method not in request.routing:
raise HttpError(HTTPStatus.METHOD_NOT_ALLOWED)
request.kwargs.update(params)

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

We may merge routing (the route payload) and kwargs (the route dynamic variables, if any) at this point. Maybe not in the same dict, in case of key conflicts?
Maybe a route object (perf arlert), with two properties payload and vars ?

request.kwargs.update(params)
handler = request.routing[request.method]
await handler(request, response, **params)
request.kwargs.update(request.route.vars)

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

You can drop request.kwargs at this point :)

This comment has been minimized.

@davidbgk

davidbgk Nov 7, 2017

Contributor

And renaming it to extras in slots for extra usage? 🙃

This comment has been minimized.

@yohanboniface

yohanboniface Nov 7, 2017

Member

Nah! :)
You really want to do request['user'] or request['session'] not request.extras['user']… ;)
I'll push something in that direction tomorrow :)

davidbgk added some commits Nov 7, 2017

@davidbgk davidbgk merged commit 3270b80 into master Nov 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yohanboniface yohanboniface deleted the content-negociation branch Nov 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment