From a96211d3d1ba246512af5e32c31726a666c467ac Mon Sep 17 00:00:00 2001 From: Tom Christie Date: Sun, 16 Sep 2012 21:48:55 +0100 Subject: [PATCH] Simplify negotiation. Drop MSIE hacks. Etc. --- djangorestframework/exceptions.py | 8 ++ .../{contentnegotiation.py => negotiation.py} | 62 +++----- djangorestframework/response.py | 2 +- djangorestframework/settings.py | 6 +- djangorestframework/tests/renderers.py | 9 -- djangorestframework/tests/response.py | 136 +----------------- docs/api-guide/content-negotiation.md | 2 + docs/topics/rest-hypermedia-hateoas.md | 52 +++++++ 8 files changed, 90 insertions(+), 187 deletions(-) rename djangorestframework/{contentnegotiation.py => negotiation.py} (50%) create mode 100644 docs/topics/rest-hypermedia-hateoas.md diff --git a/djangorestframework/exceptions.py b/djangorestframework/exceptions.py index 6930515df..a405a8858 100644 --- a/djangorestframework/exceptions.py +++ b/djangorestframework/exceptions.py @@ -31,6 +31,14 @@ class PermissionDenied(APIException): self.detail = detail or self.default_detail +class InvalidFormat(APIException): + status_code = status.HTTP_404_NOT_FOUND + default_detail = "Format suffix '.%s' not found." + + def __init__(self, format, detail=None): + self.detail = (detail or self.default_detail) % format + + class MethodNotAllowed(APIException): status_code = status.HTTP_405_METHOD_NOT_ALLOWED default_detail = "Method '%s' not allowed." diff --git a/djangorestframework/contentnegotiation.py b/djangorestframework/negotiation.py similarity index 50% rename from djangorestframework/contentnegotiation.py rename to djangorestframework/negotiation.py index 1c646ca7e..201d32ad9 100644 --- a/djangorestframework/contentnegotiation.py +++ b/djangorestframework/negotiation.py @@ -1,10 +1,6 @@ from djangorestframework import exceptions from djangorestframework.settings import api_settings from djangorestframework.utils.mediatypes import order_by_precedence -from django.http import Http404 -import re - -MSIE_USER_AGENT_REGEX = re.compile(r'^Mozilla/[0-9]+\.[0-9]+ \([^)]*; MSIE [0-9]+\.[0-9]+[a-z]?;[^)]*\)(?!.* Opera )') class BaseContentNegotiation(object): @@ -24,17 +20,23 @@ class DefaultContentNegotiation(object): fallback renderer and media_type. """ try: - return self._negotiate(request, renderers, format) - except (Http404, exceptions.NotAcceptable): + return self.unforced_negotiate(request, renderers, format) + except (exceptions.InvalidFormat, exceptions.NotAcceptable): if force: return (renderers[0], renderers[0].media_type) raise - def _negotiate(self, request, renderers, format=None): + def unforced_negotiate(self, request, renderers, format=None): """ - Actual implementation of negotiate, inside the 'force' wrapper. + As `.negotiate()`, but does not take the optional `force` agument, + or suppress exceptions. """ - renderers = self.filter_renderers(renderers, format) + # Allow URL style format override. eg. "?format=json + format = format or request.GET.get(self.settings.URL_FORMAT_OVERRIDE) + + if format: + renderers = self.filter_renderers(renderers, format) + accepts = self.get_accept_list(request) # Check the acceptable media types against each renderer, @@ -51,42 +53,22 @@ class DefaultContentNegotiation(object): def filter_renderers(self, renderers, format): """ - If there is a '.json' style format suffix, only use - renderers that accept that format. + If there is a '.json' style format suffix, filter the renderers + so that we only negotiation against those that accept that format. """ - if not format: - return renderers - renderers = [renderer for renderer in renderers if renderer.can_handle_format(format)] if not renderers: - raise Http404() + raise exceptions.InvalidFormat(format) + return renderers def get_accept_list(self, request): """ - Given the incoming request, return a tokenised list of - media type strings. + Given the incoming request, return a tokenised list of media + type strings. + + Allows URL style accept override. eg. "?accept=application/json" """ - if self.settings.URL_ACCEPT_OVERRIDE: - # URL style accept override. eg. "?accept=application/json" - override = request.GET.get(self.settings.URL_ACCEPT_OVERRIDE) - if override: - return [override] - - if (self.settings.IGNORE_MSIE_ACCEPT_HEADER and - 'HTTP_USER_AGENT' in request.META and - MSIE_USER_AGENT_REGEX.match(request.META['HTTP_USER_AGENT']) and - request.META.get('HTTP_X_REQUESTED_WITH', '').lower() != 'xmlhttprequest'): - # Ignore MSIE's broken accept behavior except for AJAX requests - # and do something sensible instead - return ['text/html', '*/*'] - - if 'HTTP_ACCEPT' in request.META: - # Standard HTTP Accept negotiation - # Accept header specified - tokens = request.META['HTTP_ACCEPT'].split(',') - return [token.strip() for token in tokens] - - # Standard HTTP Accept negotiation - # No accept header specified - return ['*/*'] + header = request.META.get('HTTP_ACCEPT', '*/*') + header = request.GET.get(self.settings.URL_ACCEPT_OVERRIDE, header) + return [token.strip() for token in header.split(',')] diff --git a/djangorestframework/response.py b/djangorestframework/response.py index 205c0503c..29034e257 100644 --- a/djangorestframework/response.py +++ b/djangorestframework/response.py @@ -25,7 +25,7 @@ class Response(SimpleTemplateResponse): @property def rendered_content(self): - self['Content-Type'] = self.media_type + self['Content-Type'] = self.renderer.media_type if self.data is None: return self.renderer.render() return self.renderer.render(self.data, self.media_type) diff --git a/djangorestframework/settings.py b/djangorestframework/settings.py index 952157129..4dec1a4d3 100644 --- a/djangorestframework/settings.py +++ b/djangorestframework/settings.py @@ -38,7 +38,7 @@ DEFAULTS = { ), 'DEFAULT_PERMISSIONS': (), 'DEFAULT_THROTTLES': (), - 'DEFAULT_CONTENT_NEGOTIATION': 'djangorestframework.contentnegotiation.DefaultContentNegotiation', + 'DEFAULT_CONTENT_NEGOTIATION': 'djangorestframework.negotiation.DefaultContentNegotiation', 'UNAUTHENTICATED_USER': 'django.contrib.auth.models.AnonymousUser', 'UNAUTHENTICATED_TOKEN': None, @@ -46,8 +46,8 @@ DEFAULTS = { 'FORM_METHOD_OVERRIDE': '_method', 'FORM_CONTENT_OVERRIDE': '_content', 'FORM_CONTENTTYPE_OVERRIDE': '_content_type', - 'URL_ACCEPT_OVERRIDE': 'accept', - 'IGNORE_MSIE_ACCEPT_HEADER': True, + 'URL_ACCEPT_OVERRIDE': '_accept', + 'URL_FORMAT_OVERRIDE': 'format', 'FORMAT_SUFFIX_KWARG': 'format' } diff --git a/djangorestframework/tests/renderers.py b/djangorestframework/tests/renderers.py index 718c903f3..650798de3 100644 --- a/djangorestframework/tests/renderers.py +++ b/djangorestframework/tests/renderers.py @@ -169,15 +169,6 @@ class RendererEndToEndTests(TestCase): self.assertEquals(resp.content, RENDERER_B_SERIALIZER(DUMMYCONTENT)) self.assertEquals(resp.status_code, DUMMYSTATUS) - def test_conflicting_format_query_and_accept_ignores_accept(self): - """If a 'format' query is specified that does not match the Accept - header, we should only honor the 'format' query string.""" - resp = self.client.get('/?format=%s' % RendererB.format, - HTTP_ACCEPT='dummy') - self.assertEquals(resp['Content-Type'], RendererB.media_type) - self.assertEquals(resp.content, RENDERER_B_SERIALIZER(DUMMYCONTENT)) - self.assertEquals(resp.status_code, DUMMYSTATUS) - _flat_repr = '{"foo": ["bar", "baz"]}' _indented_repr = '{\n "foo": [\n "bar",\n "baz"\n ]\n}' diff --git a/djangorestframework/tests/response.py b/djangorestframework/tests/response.py index cdddfff4e..5083f6d25 100644 --- a/djangorestframework/tests/response.py +++ b/djangorestframework/tests/response.py @@ -1,4 +1,3 @@ -import json import unittest from django.conf.urls.defaults import patterns, url, include @@ -6,13 +5,11 @@ from django.test import TestCase from djangorestframework.response import Response from djangorestframework.views import APIView -from djangorestframework.compat import RequestFactory -from djangorestframework import status, exceptions +from djangorestframework import status from djangorestframework.renderers import ( BaseRenderer, JSONRenderer, - DocumentingHTMLRenderer, - DEFAULT_RENDERERS + DocumentingHTMLRenderer ) @@ -24,126 +21,6 @@ class MockJsonRenderer(BaseRenderer): media_type = 'application/json' -class TestResponseDetermineRenderer(TestCase): - - def get_response(self, url='', accept_list=[], renderer_classes=[]): - kwargs = {} - if accept_list is not None: - kwargs['HTTP_ACCEPT'] = ','.join(accept_list) - request = RequestFactory().get(url, **kwargs) - return Response(request=request, renderer_classes=renderer_classes) - - def test_determine_accept_list_accept_header(self): - """ - Test that determine_accept_list takes the Accept header. - """ - accept_list = ['application/pickle', 'application/json'] - response = self.get_response(accept_list=accept_list) - self.assertEqual(response._determine_accept_list(), accept_list) - - def test_determine_accept_list_default(self): - """ - Test that determine_accept_list takes the default renderer if Accept is not specified. - """ - response = self.get_response(accept_list=None) - self.assertEqual(response._determine_accept_list(), ['*/*']) - - def test_determine_accept_list_overriden_header(self): - """ - Test Accept header overriding. - """ - accept_list = ['application/pickle', 'application/json'] - response = self.get_response(url='?_accept=application/x-www-form-urlencoded', - accept_list=accept_list) - self.assertEqual(response._determine_accept_list(), ['application/x-www-form-urlencoded']) - - def test_determine_renderer(self): - """ - Test that right renderer is chosen, in the order of Accept list. - """ - accept_list = ['application/pickle', 'application/json'] - renderer_classes = (MockPickleRenderer, MockJsonRenderer) - response = self.get_response(accept_list=accept_list, renderer_classes=renderer_classes) - renderer, media_type = response._determine_renderer() - self.assertEqual(media_type, 'application/pickle') - self.assertTrue(isinstance(renderer, MockPickleRenderer)) - - renderer_classes = (MockJsonRenderer, ) - response = self.get_response(accept_list=accept_list, renderer_classes=renderer_classes) - renderer, media_type = response._determine_renderer() - self.assertEqual(media_type, 'application/json') - self.assertTrue(isinstance(renderer, MockJsonRenderer)) - - def test_determine_renderer_default(self): - """ - Test determine renderer when Accept was not specified. - """ - renderer_classes = (MockPickleRenderer, ) - response = self.get_response(accept_list=None, renderer_classes=renderer_classes) - renderer, media_type = response._determine_renderer() - self.assertEqual(media_type, '*/*') - self.assertTrue(isinstance(renderer, MockPickleRenderer)) - - def test_determine_renderer_no_renderer(self): - """ - Test determine renderer when no renderer can satisfy the Accept list. - """ - accept_list = ['application/json'] - renderer_classes = (MockPickleRenderer, ) - response = self.get_response(accept_list=accept_list, renderer_classes=renderer_classes) - self.assertRaises(exceptions.NotAcceptable, response._determine_renderer) - - -class TestResponseRenderContent(TestCase): - def get_response(self, url='', accept_list=[], content=None, renderer_classes=None): - request = RequestFactory().get(url, HTTP_ACCEPT=','.join(accept_list)) - return Response(request=request, content=content, renderer_classes=renderer_classes or DEFAULT_RENDERERS) - - def test_render(self): - """ - Test rendering simple data to json. - """ - content = {'a': 1, 'b': [1, 2, 3]} - content_type = 'application/json' - response = self.get_response(accept_list=[content_type], content=content) - response = response.render() - self.assertEqual(json.loads(response.content), content) - self.assertEqual(response['Content-Type'], content_type) - - def test_render_no_renderer(self): - """ - Test rendering response when no renderer can satisfy accept. - """ - content = 'bla' - content_type = 'weirdcontenttype' - response = self.get_response(accept_list=[content_type], content=content) - response = response.render() - self.assertEqual(response.status_code, 406) - self.assertIsNotNone(response.content) - - # def test_render_renderer_raises_ImmediateResponse(self): - # """ - # Test rendering response when renderer raises ImmediateResponse - # """ - # class PickyJSONRenderer(BaseRenderer): - # """ - # A renderer that doesn't make much sense, just to try - # out raising an ImmediateResponse - # """ - # media_type = 'application/json' - - # def render(self, obj=None, media_type=None): - # raise ImmediateResponse({'error': '!!!'}, status=400) - - # response = self.get_response( - # accept_list=['application/json'], - # renderers=[PickyJSONRenderer, JSONRenderer] - # ) - # response = response.render() - # self.assertEqual(response.status_code, 400) - # self.assertEqual(response.content, json.dumps({'error': '!!!'})) - - DUMMYSTATUS = status.HTTP_200_OK DUMMYCONTENT = 'dummycontent' @@ -280,15 +157,6 @@ class RendererIntegrationTests(TestCase): self.assertEquals(resp.content, RENDERER_B_SERIALIZER(DUMMYCONTENT)) self.assertEquals(resp.status_code, DUMMYSTATUS) - def test_conflicting_format_query_and_accept_ignores_accept(self): - """If a 'format' query is specified that does not match the Accept - header, we should only honor the 'format' query string.""" - resp = self.client.get('/?format=%s' % RendererB.format, - HTTP_ACCEPT='dummy') - self.assertEquals(resp['Content-Type'], RendererB.media_type) - self.assertEquals(resp.content, RENDERER_B_SERIALIZER(DUMMYCONTENT)) - self.assertEquals(resp.status_code, DUMMYSTATUS) - class Issue122Tests(TestCase): """ diff --git a/docs/api-guide/content-negotiation.md b/docs/api-guide/content-negotiation.md index 01895a4b0..ad98de3b9 100644 --- a/docs/api-guide/content-negotiation.md +++ b/docs/api-guide/content-negotiation.md @@ -1,3 +1,5 @@ + + # Content negotiation > HTTP has provisions for several mechanisms for "content negotiation" - the process of selecting the best representation for a given response when there are multiple representations available. diff --git a/docs/topics/rest-hypermedia-hateoas.md b/docs/topics/rest-hypermedia-hateoas.md new file mode 100644 index 000000000..2bca2ab8f --- /dev/null +++ b/docs/topics/rest-hypermedia-hateoas.md @@ -0,0 +1,52 @@ +> You keep using that word "REST". I do not think it means what you think it means. +> +> — Mike Amundsen, [talking at REST fest 2012][cite]. + +# REST, Hypermedia & HATEOAS + +First off, the disclaimer. The name "Django REST framework" was choosen with a view to making sure the project would be easily found by developers. Throughout the documentation we try to use the more simple and technically correct terminology of "Web APIs". + +If you are serious about designing a Hypermedia APIs, you should look to resources outside of this documentation to help inform your design choices. + +The following fall into the "required reading" category. + +* Fielding's dissertation - [Architectural Styles and +the Design of Network-based Software Architectures][dissertation]. +* Fielding's "[REST APIs must be hypertext-driven][hypertext-driven]" blog post. +* Leonard Richardson & Sam Ruby's [RESTful Web Services][restful-web-services]. +* Mike Amundsen's [Building Hypermedia APIs with HTML5 and Node][building-hypermedia-apis]. +* Steve Klabnik's [Designing Hypermedia APIs][designing-hypermedia-apis]. +* The [Richardson Maturity Model][maturitymodel]. + +For a more thorough background, check out Klabnik's [Hypermedia API reading list][readinglist]. + +# Building Hypermedia APIs with REST framework + +REST framework is an agnositic Web API toolkit. It does help guide you towards building well-connected APIs, and makes it easy to design appropriate media types, but it does not strictly enforce any particular design style. + +### What REST framework *does* provide. + +It is self evident that REST framework makes it possible to build Hypermedia APIs. The browseable API that it offers is built on HTML - the hypermedia language of the web. + +REST framework also includes [serialization] and [parser]/[renderer] components that make it easy to build appropriate media types, [hyperlinked relations][fields] for building well-connected systems, and great support for [content negotiation][conneg]. + +### What REST framework *doesn't* provide. + +What REST framework doesn't do is give you is machine readable hypermedia formats such as [Collection+JSON][collection] by default, or the ability to auto-magically create HATEOAS style APIs. Doing so would involve making opinionated choices about API design that should really remain outside of the framework's scope. + +[cite]: http://vimeo.com/channels/restfest/page:2 +[dissertation]: http://www.ics.uci.edu/~fielding/pubs/dissertation/top.htm +[hypertext-driven]: http://roy.gbiv.com/untangled/2008/rest-apis-must-be-hypertext-driven +[restful-web-services]: +[building-hypermedia-apis]: … +[designing-hypermedia-apis]: http://designinghypermediaapis.com/ +[restisover]: http://blog.steveklabnik.com/posts/2012-02-23-rest-is-over +[readinglist]: http://blog.steveklabnik.com/posts/2012-02-27-hypermedia-api-reading-list +[maturitymodel]: http://martinfowler.com/articles/richardsonMaturityModel.html + +[collection]: http://www.amundsen.com/media-types/collection/ +[serialization]: ../api-guide/serializers.md +[parser]: ../api-guide/parsers.md +[renderer]: ../api-guide/renderers.md +[fields]: ../api-guide/fields.md +[conneg]: ../api-guide/content-negotiation.md \ No newline at end of file