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

Add content_negociation extension #21

Merged
merged 10 commits into from Nov 8, 2017
20 changes: 14 additions & 6 deletions docs/changelog.md
Expand Up @@ -10,13 +10,21 @@ A changelog:

## dev version

* Changed `Roll.hook` signature to also accept kwargs ([#5](https://github.com/pyrates/roll/pull/5))
* `json` shorcut sets `utf-8` charset in `Content-Type` header ([#13](https://github.com/pyrates/roll/pull/13))
* Added `static` extension to serve static files for development ([#16](https://github.com/pyrates/roll/pull/16))
* `cors` accepts `headers` parameter to control `Access-Control-Allow-Headers` ([#12](https://github.com/pyrates/roll/pull/12))
* Changed `Roll.hook` signature to also accept kwargs
([#5](https://github.com/pyrates/roll/pull/5))
* `json` shorcut sets `utf-8` charset in `Content-Type` header
([#13](https://github.com/pyrates/roll/pull/13))
* Added `static` extension to serve static files for development
([#16](https://github.com/pyrates/roll/pull/16))
* `cors` accepts `headers` parameter to control `Access-Control-Allow-Headers`
([#12](https://github.com/pyrates/roll/pull/12))
* Added `content_negociation` extension to reject unacceptable client requests
based on the `Accept` header
* **Breaking changes**:
* `options` extension is no more applied by default ([#16](https://github.com/pyrates/roll/pull/16))
* deprecated `req` pytest fixture is now removed ([#9](https://github.com/pyrates/roll/pull/9))
* `options` extension is no more applied by default
([#16](https://github.com/pyrates/roll/pull/16))
* deprecated `req` pytest fixture is now removed
([#9](https://github.com/pyrates/roll/pull/9))

## 0.5.0 — 2017-09-21

Expand Down
20 changes: 20 additions & 0 deletions docs/reference.md
Expand Up @@ -152,10 +152,19 @@ Combine it with the `cors` extension to handle the preflight request.
- **app**: Roll app to register the extension against


### content_negociation

Deal with content negociation declared during routes definition.
Will return a `406 Not Acceptable` response in case of mismatch between
the `Accept` header from the client and the `accepts` parameter set in
routes. Useful to reject requests which are not expecting the available
response.

#### Parameters

- **app**: Roll app to register the extension against


### traceback

Print the traceback on the server side if any. Handy for debugging.
Expand All @@ -164,6 +173,7 @@ Print the traceback on the server side if any. Handy for debugging.

- **app**: Roll app to register the extension against


### igniter

Display a BIG message when running the server.
Expand All @@ -173,6 +183,7 @@ Quite useless, hence so essential!

- **app**: Roll app to register the extension against


### static

Serve static files. Should not be used in production.
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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


Fired after the route matching at each request.
Use it to customize routes restrictions for instance.

Receives `request` and `payload` parameters. The latter being the result
of the routes matching (dict).
1 change: 1 addition & 0 deletions requirements.txt
@@ -1,3 +1,4 @@
autoroutes==0.2.0
httptools==0.0.9
mimetype-match==1.0.4
Copy link
Member

Choose a reason for hiding this comment

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

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.

uvloop==0.8.1
21 changes: 13 additions & 8 deletions roll/__init__.py
Expand Up @@ -199,10 +199,12 @@ def write(self, *args):
payload = b'HTTP/1.1 %a %b\r\n' % (
self.response.status.value, self.response.status.phrase.encode())
if not isinstance(self.response.body, bytes):
self.response.body = self.response.body.encode()
self.response.body = str(self.response.body).encode()
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:
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I set a default.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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) ;)

self.response.headers['Content-Type'] = 'text/html'
for key, value in self.response.headers.items():
payload += b'%b: %b\r\n' % (key.encode(), str(value).encode())
payload += b'\r\n%b' % self.response.body
Expand Down Expand Up @@ -242,7 +244,7 @@ async def shutdown(self):
async def __call__(self, request: Request, response: Response):
try:
if not await self.hook('request', request, response):
params, handler = self.dispatch(request)
params, handler = await self.dispatch(request)
await handler(request, response, **params)
except Exception as error:
await self.on_error(request, response, error)
Expand All @@ -268,22 +270,25 @@ async def on_error(self, request: Request, response: Response, error):
def factory(self):
return self.Protocol(self)

def route(self, path: str, methods: list=None):
def route(self, path: str, methods: list=None, **extras):
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}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

handlers.update(extras)
self.routes.add(path, **handlers)
return func

return wrapper

def dispatch(self, request: Request):
handlers, params = self.routes.match(request.path)
if request.method not in handlers:
async def dispatch(self, request: Request):
payload, params = self.routes.match(request.path)
await self.hook('dispatch', request, payload)
if request.method not in payload:
raise HttpError(HTTPStatus.METHOD_NOT_ALLOWED)
request.kwargs.update(params)
return params, handlers[request.method]
return params, payload[request.method]

def listen(self, name: str):
def wrapper(func):
Expand Down
15 changes: 13 additions & 2 deletions roll/extensions.py
Expand Up @@ -5,6 +5,8 @@
from pathlib import Path
from traceback import print_exc

from mimetype_match import get_best_match

from . import HttpError


Expand Down Expand Up @@ -48,6 +50,15 @@ async def handle_options(request, response):
return request.method == 'OPTIONS'


def content_negociation(app):

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

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

raise HttpError(HTTPStatus.NOT_ACCEPTABLE)


def traceback(app):

@app.listen('error')
Expand Down Expand Up @@ -108,8 +119,8 @@ async def serve(request, response, path):
content_type, encoding = mimetypes.guess_type(str(abspath))
with abspath.open('rb') as source:
response.body = source.read()
response.headers['Content-Type'] = (content_type
or 'application/octet-stream')
response.headers['Content-Type'] = (content_type or
'application/octet-stream')

@app.listen('startup')
async def register_route():
Expand Down
14 changes: 13 additions & 1 deletion roll/testing.py
Expand Up @@ -4,6 +4,15 @@
import pytest


class Transport:

def write(self, data):
...

def close(self):
...


class Client:

# Default content type for request body encoding, change it to your own
Expand Down Expand Up @@ -36,12 +45,15 @@ async def request(self, path, method='GET', body=b'', headers=None,
headers['Content-Type'] = content_type
body, headers = self.encode_body(body, headers)
protocol = self.app.factory()
protocol.connection_made(Transport())
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)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that this urgent? #lazyme

protocol.write()
return protocol.response

async def get(self, path, **kwargs):
return await self.request(path, method='GET', **kwargs)
Expand Down
12 changes: 6 additions & 6 deletions tests/test_errors.py
Expand Up @@ -14,7 +14,7 @@ async def get(req, resp):

resp = await client.get('/test')
assert resp.status == HTTPStatus.INTERNAL_SERVER_ERROR
assert resp.body == 'Oops.'
assert resp.body == b'Oops.'


async def test_httpstatus_error(client, app):
Expand All @@ -25,7 +25,7 @@ async def get(req, resp):

resp = await client.get('/test')
assert resp.status == HTTPStatus.BAD_REQUEST
assert resp.body == 'Really bad.'
assert resp.body == b'Really bad.'


async def test_error_only_with_status(client, app):
Expand All @@ -36,7 +36,7 @@ async def get(req, resp):

resp = await client.get('/test')
assert resp.status == HTTPStatus.INTERNAL_SERVER_ERROR
assert resp.body == 'Internal Server Error'
assert resp.body == b'Internal Server Error'


async def test_error_only_with_httpstatus(client, app):
Expand All @@ -47,7 +47,7 @@ async def get(req, resp):

resp = await client.get('/test')
assert resp.status == HTTPStatus.INTERNAL_SERVER_ERROR
assert resp.body == 'Internal Server Error'
assert resp.body == b'Internal Server Error'


async def test_error_subclasses_with_super(client, app):
Expand All @@ -63,7 +63,7 @@ async def get(req, resp):

resp = await client.get('/test')
assert resp.status == HTTPStatus.INTERNAL_SERVER_ERROR
assert resp.body == '<h1>Oops.</h1>'
assert resp.body == b'<h1>Oops.</h1>'


async def test_error_subclasses_without_super(client, app):
Expand All @@ -79,4 +79,4 @@ async def get(req, resp):

resp = await client.get('/test')
assert resp.status == HTTPStatus.INTERNAL_SERVER_ERROR
assert resp.body == '<h1>Oops.</h1>'
assert resp.body == b'<h1>Oops.</h1>'
91 changes: 90 additions & 1 deletion tests/test_extensions.py
Expand Up @@ -18,7 +18,7 @@ async def get(req, resp):

resp = await client.get('/test')
assert resp.status == HTTPStatus.OK
assert resp.body == 'test response'
assert resp.body == b'test response'
assert resp.headers['Access-Control-Allow-Origin'] == '*'


Expand Down Expand Up @@ -167,3 +167,92 @@ async def test_can_change_static_prefix(client, app):
resp = await client.get('/foo/index.html')
assert resp.status == HTTPStatus.OK
assert b'Test' in resp.body


async def test_get_accept_content_negociation(client, app):

extensions.content_negociation(app)

@app.route('/test', accepts=['text/html'])
async def get(req, resp):
resp.body = 'accepted'

resp = await client.get('/test', headers={'Accept': 'text/html'})
assert resp.status == HTTPStatus.OK
assert resp.body == b'accepted'
assert resp.headers['Content-Type'] == 'text/html'


async def test_get_accept_content_negociation_if_many(client, app):

extensions.content_negociation(app)

@app.route('/test', accepts=['text/html', 'application/json'])
async def get(req, resp):
if req.headers['Accept'] == 'text/html':
resp.body = '<h1>accepted</h1>'
elif req.headers['Accept'] == 'application/json':
resp.json = {'status': 'accepted'}

resp = await client.get('/test', headers={'Accept': 'text/html'})
assert resp.status == HTTPStatus.OK
assert resp.body == b'<h1>accepted</h1>'
assert resp.headers['Content-Type'] == 'text/html'
resp = await client.get('/test', headers={'Accept': 'application/json'})
assert resp.status == HTTPStatus.OK
assert resp.body == b'{"status":"accepted"}'
assert resp.headers['Content-Type'] == 'application/json; charset=utf-8'


async def test_get_reject_content_negociation(client, app):

extensions.content_negociation(app)

@app.route('/test', accepts=['text/html'])
async def get(req, resp):
resp.body = 'rejected'

resp = await client.get('/test', headers={'Accept': 'text/css'})
assert resp.status == HTTPStatus.NOT_ACCEPTABLE


async def test_get_accept_star_content_negociation(client, app):

extensions.content_negociation(app)

@app.route('/test', accepts=['text/css'])
async def get(req, resp):
resp.body = 'accepted'

resp = await client.get('/test', headers={'Accept': 'text/*'})
assert resp.status == HTTPStatus.OK


async def test_post_accept_content_negociation(client, app):

extensions.content_negociation(app)

@app.route('/test', methods=['POST'], accepts=['application/json'])
async def get(req, resp):
resp.json = {'status': 'accepted'}

client.content_type = 'application/x-www-form-urlencoded'
resp = await client.post('/test', body={'key': 'value'},
headers={'Accept': 'application/json'})
assert resp.status == HTTPStatus.OK
assert resp.headers['Content-Type'] == 'application/json; charset=utf-8'
assert resp.body == b'{"status":"accepted"}'


async def test_post_reject_content_negociation(client, app):

extensions.content_negociation(app)

@app.route('/test', methods=['POST'], accepts=['text/html'])
async def get(req, resp):
resp.json = {'status': 'accepted'}

client.content_type = 'application/x-www-form-urlencoded'
resp = await client.post('/test', body={'key': 'value'},
headers={'Accept': 'application/json'})
assert resp.status == HTTPStatus.NOT_ACCEPTABLE
6 changes: 3 additions & 3 deletions tests/test_hooks.py
Expand Up @@ -13,7 +13,7 @@ async def test_request_hook_can_alter_response(client, app):
@app.listen('request')
async def listener(request, response):
response.status = 400
response.body = 'another response'
response.body = b'another response'
return True # Shortcut the response process.

@app.route('/test')
Expand All @@ -22,7 +22,7 @@ async def get(req, resp):

resp = await client.get('/test')
assert resp.status == HTTPStatus.BAD_REQUEST
assert resp.body == 'another response'
assert resp.body == b'another response'


async def test_response_hook_can_alter_response(client, app):
Expand All @@ -39,7 +39,7 @@ async def get(req, resp):

resp = await client.get('/test')
assert resp.status == HTTPStatus.BAD_REQUEST
assert resp.body == 'another response'
assert resp.body == b'another response'


async def test_error_with_json_format(client, app):
Expand Down