diff options
| -rw-r--r-- | AUTHORS | 2 | ||||
| -rw-r--r-- | djangorestframework/mixins.py | 138 | ||||
| -rw-r--r-- | djangorestframework/parsers.py | 2 | ||||
| -rw-r--r-- | djangorestframework/renderers.py | 4 | ||||
| -rw-r--r-- | djangorestframework/templatetags/add_query_param.py | 12 | ||||
| -rw-r--r-- | djangorestframework/tests/renderers.py | 180 | ||||
| -rw-r--r-- | djangorestframework/tests/views.py | 16 | ||||
| -rw-r--r-- | djangorestframework/views.py | 61 |
8 files changed, 326 insertions, 89 deletions
@@ -24,6 +24,8 @@ Ewoud Kohl van Wijngaarden <ekohl> Michael Ding <yandy> Mjumbe Poe <mjumbewu> Natim <natim> +Sebastian Żurek <sebzur> +Benoit C <dzen> THANKS TO: diff --git a/djangorestframework/mixins.py b/djangorestframework/mixins.py index e01de3fc..09688eb5 100644 --- a/djangorestframework/mixins.py +++ b/djangorestframework/mixins.py @@ -6,6 +6,7 @@ classes that can be added to a `View`. from django.contrib.auth.models import AnonymousUser from django.core.paginator import Paginator from django.db.models.fields.related import ForeignKey +from django.db.models.query import Q from django.http import HttpResponse from urlobject import URLObject @@ -482,7 +483,82 @@ class InstanceMixin(object): ########## Model Mixins ########## -class ReadModelMixin(object): +class ModelMixin(object): + """ Implements mechanisms used by other classes (like *ModelMixin group) to + define a query that represents Model instances the Mixin is working with. + + If a *ModelMixin is going to retrive an instance (or queryset) using args and kwargs + passed by as URL arguments, it should provied arguments to objects.get and objects.filter + methods wrapped in by `build_query` + + If a *ModelMixin is going to create/update an instance get_instance_data + handles the instance data creation/preaparation. + """ + + def build_query(self, *args, **kwargs): + """ Returns django.db.models.Q object to be used for the objects retrival. + + Arguments: + - args: unnamed URL arguments + - kwargs: named URL arguments + + If a URL passes any arguments to the view being the QueryMixin subclass + build_query manages the arguments and provides the Q object that will be + used for the objects retrival with filter/get queryset methods. + + Technically, neither args nor kwargs have to be provided, however the default + behaviour is to map all kwargs as the query constructors so that if this + method is not overriden only kwargs keys being model fields are valid. + + If positional args are provided, the last one argument is understood + as the primary key. However this usage should be considered + deperecated, and will be removed in a future version. + """ + + tmp = dict(kwargs) + + # If the URLconf includes a .(?P<format>\w+) pattern to match against + # a .json, .xml suffix, then drop the 'format' kwarg before + # constructing the query. + if BaseRenderer._FORMAT_QUERY_PARAM in tmp: + del tmp[BaseRenderer._FORMAT_QUERY_PARAM] + + if args: + # If we have any no kwargs then assume the last arg represents the + # primrary key. Otherwise assume the kwargs uniquely identify the + # model. + tmp.update({'pk': args[-1]}) + return Q(**tmp) + + def get_instance_data(self, model, content, **kwargs): + """ + Returns the dict with the data for model instance creation/update query. + + Arguments: + - model: model class (django.db.models.Model subclass) to work with + - content: a dictionary with instance data + - kwargs: a dict of URL provided keyword arguments + + The create/update queries are created basicly with the contet provided + with POST/PUT HTML methods and kwargs passed in the URL. This methods + simply merges the URL data and the content preaparing the ready-to-use + data dictionary. + """ + + tmp = dict(kwargs) + + for field in model._meta.fields: + if isinstance(field, ForeignKey) and field.name in tmp: + # translate 'related_field' kwargs into 'related_field_id' + tmp[field.name + '_id'] = tmp[field.name] + del tmp[field.name] + + all_kw_args = dict(content.items() + tmp.items()) + + return all_kw_args + + +class ReadModelMixin(ModelMixin): """ Behavior to read a `model` instance on GET requests """ @@ -490,22 +566,21 @@ class ReadModelMixin(object): model = self.resource.model try: - if args: - # If we have any none kwargs then assume the last represents the primrary key - self.model_instance = model.objects.get(pk=args[-1], **kwargs) - else: - # Otherwise assume the kwargs uniquely identify the model - filtered_keywords = kwargs.copy() - if BaseRenderer._FORMAT_QUERY_PARAM in filtered_keywords: - del filtered_keywords[BaseRenderer._FORMAT_QUERY_PARAM] - self.model_instance = model.objects.get(**filtered_keywords) + self.model_instance = model.objects.get(self.build_query(*args, **kwargs)) except model.DoesNotExist: raise ErrorResponse(status.HTTP_404_NOT_FOUND) return self.model_instance + def build_query(self, *args, **kwargs): + # Build query is overriden to filter the kwargs priori + # to use them as build_query argument + filtered_keywords = kwargs.copy() + + return super(ReadModelMixin, self).build_query(*args, **filtered_keywords) + -class CreateModelMixin(object): +class CreateModelMixin(ModelMixin): """ Behavior to create a `model` instance on POST requests """ @@ -516,25 +591,14 @@ class CreateModelMixin(object): content = dict(self.CONTENT) m2m_data = {} - for field in model._meta.fields: - if isinstance(field, ForeignKey) and kwargs.has_key(field.name): - # translate 'related_field' kwargs into 'related_field_id' - kwargs[field.name + '_id'] = kwargs[field.name] - del kwargs[field.name] - for field in model._meta.many_to_many: - if content.has_key(field.name): + if field.name in content: m2m_data[field.name] = ( field.m2m_reverse_field_name(), content[field.name] ) del content[field.name] - all_kw_args = dict(content.items() + kwargs.items()) - - if args: - instance = model(pk=args[-1], **all_kw_args) - else: - instance = model(**all_kw_args) + instance = model(**self.get_instance_data(model, content, *args, **kwargs)) instance.save() for fieldname in m2m_data: @@ -556,7 +620,7 @@ class CreateModelMixin(object): return Response(status.HTTP_201_CREATED, instance, headers) -class UpdateModelMixin(object): +class UpdateModelMixin(ModelMixin): """ Behavior to update a `model` instance on PUT requests """ @@ -565,24 +629,17 @@ class UpdateModelMixin(object): # TODO: update on the url of a non-existing resource url doesn't work correctly at the moment - will end up with a new url try: - if args: - # If we have any none kwargs then assume the last represents the primary key - self.model_instance = model.objects.get(pk=args[-1], **kwargs) - else: - # Otherwise assume the kwargs uniquely identify the model - self.model_instance = model.objects.get(**kwargs) + self.model_instance = model.objects.get(self.build_query(*args, **kwargs)) for (key, val) in self.CONTENT.items(): setattr(self.model_instance, key, val) except model.DoesNotExist: - self.model_instance = model(**self.CONTENT) - self.model_instance.save() - + self.model_instance = model(**self.get_instance_data(model, self.CONTENT, *args, **kwargs)) self.model_instance.save() return self.model_instance -class DeleteModelMixin(object): +class DeleteModelMixin(ModelMixin): """ Behavior to delete a `model` instance on DELETE requests """ @@ -590,12 +647,7 @@ class DeleteModelMixin(object): model = self.resource.model try: - if args: - # If we have any none kwargs then assume the last represents the primrary key - instance = model.objects.get(pk=args[-1], **kwargs) - else: - # Otherwise assume the kwargs uniquely identify the model - instance = model.objects.get(**kwargs) + instance = model.objects.get(self.build_query(*args, **kwargs)) except model.DoesNotExist: raise ErrorResponse(status.HTTP_404_NOT_FOUND, None, {}) @@ -603,7 +655,7 @@ class DeleteModelMixin(object): return -class ListModelMixin(object): +class ListModelMixin(ModelMixin): """ Behavior to list a set of `model` instances on GET requests """ @@ -635,7 +687,7 @@ class ListModelMixin(object): if ordering: args = as_tuple(ordering) queryset = queryset.order_by(*args) - return queryset.filter(**kwargs) + return queryset.filter(self.build_query(**kwargs)) ########## Pagination Mixins ########## diff --git a/djangorestframework/parsers.py b/djangorestframework/parsers.py index 7d765022..c218e5ee 100644 --- a/djangorestframework/parsers.py +++ b/djangorestframework/parsers.py @@ -31,7 +31,7 @@ __all__ = ( 'FormParser', 'MultiPartParser', 'YAMLParser', - 'XMLParser' + 'XMLParser' ) diff --git a/djangorestframework/renderers.py b/djangorestframework/renderers.py index 240de69e..42d80bd4 100644 --- a/djangorestframework/renderers.py +++ b/djangorestframework/renderers.py @@ -169,7 +169,7 @@ if yaml: if obj is None: return '' - return yaml.dump(obj) + return yaml.safe_dump(obj) else: YAMLRenderer = None @@ -215,7 +215,7 @@ class DocumentingTemplateRenderer(BaseRenderer): """ # Find the first valid renderer and render the content. (Don't use another documenting renderer.) - renderers = [renderer for renderer in view.renderers if not isinstance(renderer, DocumentingTemplateRenderer)] + renderers = [renderer for renderer in view.renderers if not issubclass(renderer, DocumentingTemplateRenderer)] if not renderers: return '[No renderers were found]' diff --git a/djangorestframework/templatetags/add_query_param.py b/djangorestframework/templatetags/add_query_param.py index 94833bce..ce175b81 100644 --- a/djangorestframework/templatetags/add_query_param.py +++ b/djangorestframework/templatetags/add_query_param.py @@ -1,17 +1,11 @@ from django.template import Library -from urlparse import urlparse, urlunparse -from urllib import quote +from urlobject import URLObject register = Library() + def add_query_param(url, param): (key, sep, val) = param.partition('=') - param = '%s=%s' % (key, quote(val)) - (scheme, netloc, path, params, query, fragment) = urlparse(url) - if query: - query += "&" + param - else: - query = param - return urlunparse((scheme, netloc, path, params, query, fragment)) + return unicode(URLObject(url) & (key, val)) register.filter('add_query_param', add_query_param) diff --git a/djangorestframework/tests/renderers.py b/djangorestframework/tests/renderers.py index bd0d360c..e80f0f20 100644 --- a/djangorestframework/tests/renderers.py +++ b/djangorestframework/tests/renderers.py @@ -5,7 +5,7 @@ from djangorestframework import status from djangorestframework.views import View from djangorestframework.compat import View as DjangoView from djangorestframework.renderers import BaseRenderer, JSONRenderer, YAMLRenderer, \ - XMLRenderer, JSONPRenderer + XMLRenderer, JSONPRenderer, DocumentingHTMLRenderer from djangorestframework.parsers import JSONParser, YAMLParser from djangorestframework.mixins import ResponseMixin from djangorestframework.response import Response @@ -46,15 +46,30 @@ class MockView(ResponseMixin, DjangoView): class MockGETView(View): + def get(self, request, **kwargs): return {'foo': ['bar', 'baz']} +class HTMLView(View): + renderers = (DocumentingHTMLRenderer, ) + + def get(self, request, **kwargs): + return 'text' + +class HTMLView1(View): + renderers = (DocumentingHTMLRenderer, JSONRenderer) + + def get(self, request, **kwargs): + return 'text' + urlpatterns = patterns('', url(r'^.*\.(?P<format>.+)$', MockView.as_view(renderers=[RendererA, RendererB])), url(r'^$', MockView.as_view(renderers=[RendererA, RendererB])), url(r'^jsonp/jsonrenderer$', MockGETView.as_view(renderers=[JSONRenderer, JSONPRenderer])), url(r'^jsonp/nojsonrenderer$', MockGETView.as_view(renderers=[JSONPRenderer])), + url(r'^html$', HTMLView.as_view()), + url(r'^html1$', HTMLView1.as_view()), ) @@ -327,3 +342,166 @@ class XMLRendererTestCase(TestCase): self.assertTrue(xml.startswith('<?xml version="1.0" encoding="utf-8"?>\n<root>')) self.assertTrue(xml.endswith('</root>')) self.assertTrue(string in xml, '%r not in %r' % (string, xml)) + +class HTMLView(View): + renderers = (DocumentingHTMLRenderer) + + def get(self, request, **kwargs): + return 'text' + +urlpatterns += patterns('', + url(r'^/html$', HTMLView.as_view()), +) + +class Issue122Tests(TestCase): + """ + Tests that cover issues. + """ + + urls = 'djangorestframework.tests.renderers' + + def test_without_callback_with_json_renderer(self): + """ + Test JSONP rendering with View JSON Renderer. + """ + resp = self.client.get('/jsonp/jsonrenderer', + HTTP_ACCEPT='application/json-p') + self.assertEquals(resp.status_code, 200) + self.assertEquals(resp['Content-Type'], 'application/json-p') + self.assertEquals(resp.content, 'callback(%s);' % _flat_repr) + + def test_without_callback_without_json_renderer(self): + """ + Test JSONP rendering without View JSON Renderer. + """ + resp = self.client.get('/jsonp/nojsonrenderer', + HTTP_ACCEPT='application/json-p') + self.assertEquals(resp.status_code, 200) + self.assertEquals(resp['Content-Type'], 'application/json-p') + self.assertEquals(resp.content, 'callback(%s);' % _flat_repr) + + def test_with_callback(self): + """ + Test JSONP rendering with callback function name. + """ + callback_func = 'myjsonpcallback' + resp = self.client.get('/jsonp/nojsonrenderer?callback=' + callback_func, + HTTP_ACCEPT='application/json-p') + self.assertEquals(resp.status_code, 200) + self.assertEquals(resp['Content-Type'], 'application/json-p') + self.assertEquals(resp.content, '%s(%s);' % (callback_func, _flat_repr)) + + +if YAMLRenderer: + _yaml_repr = 'foo: [bar, baz]\n' + + class YAMLRendererTests(TestCase): + """ + Tests specific to the JSON Renderer + """ + + def test_render(self): + """ + Test basic YAML rendering. + """ + obj = {'foo': ['bar', 'baz']} + renderer = YAMLRenderer(None) + content = renderer.render(obj, 'application/yaml') + self.assertEquals(content, _yaml_repr) + + def test_render_and_parse(self): + """ + Test rendering and then parsing returns the original object. + IE obj -> render -> parse -> obj. + """ + obj = {'foo': ['bar', 'baz']} + + renderer = YAMLRenderer(None) + parser = YAMLParser(None) + + content = renderer.render(obj, 'application/yaml') + (data, files) = parser.parse(StringIO(content)) + self.assertEquals(obj, data) + + +class XMLRendererTestCase(TestCase): + """ + Tests specific to the XML Renderer + """ + + def test_render_string(self): + """ + Test XML rendering. + """ + renderer = XMLRenderer(None) + content = renderer.render({'field': 'astring'}, 'application/xml') + self.assertXMLContains(content, '<field>astring</field>') + + def test_render_integer(self): + """ + Test XML rendering. + """ + renderer = XMLRenderer(None) + content = renderer.render({'field': 111}, 'application/xml') + self.assertXMLContains(content, '<field>111</field>') + + def test_render_datetime(self): + """ + Test XML rendering. + """ + renderer = XMLRenderer(None) + content = renderer.render({ + 'field': datetime.datetime(2011, 12, 25, 12, 45, 00) + }, 'application/xml') + self.assertXMLContains(content, '<field>2011-12-25 12:45:00</field>') + + def test_render_float(self): + """ + Test XML rendering. + """ + renderer = XMLRenderer(None) + content = renderer.render({'field': 123.4}, 'application/xml') + self.assertXMLContains(content, '<field>123.4</field>') + + def test_render_decimal(self): + """ + Test XML rendering. + """ + renderer = XMLRenderer(None) + content = renderer.render({'field': Decimal('111.2')}, 'application/xml') + self.assertXMLContains(content, '<field>111.2</field>') + + def test_render_none(self): + """ + Test XML rendering. + """ + renderer = XMLRenderer(None) + content = renderer.render({'field': None}, 'application/xml') + self.assertXMLContains(content, '<field></field>') + + def assertXMLContains(self, xml, string): + self.assertTrue(xml.startswith('<?xml version="1.0" encoding="utf-8"?>\n<root>')) + self.assertTrue(xml.endswith('</root>')) + self.assertTrue(string in xml, '%r not in %r' % (string, xml)) + + + +class Issue122Tests(TestCase): + """ + Tests that covers #122. + """ + + urls = 'djangorestframework.tests.renderers' + + def test_only_html_renderer(self): + """ + Test if no recursion occurs. + """ + resp = self.client.get('/html') + + def test_html_renderer_is_first(self): + """ + Test if no recursion occurs. + """ + resp = self.client.get('/html1') + diff --git a/djangorestframework/tests/views.py b/djangorestframework/tests/views.py index e03f6615..ab5d75d6 100644 --- a/djangorestframework/tests/views.py +++ b/djangorestframework/tests/views.py @@ -1,4 +1,5 @@ from django.conf.urls.defaults import patterns, url +from django.http import HttpResponse from django.test import TestCase from django.test import Client from django import forms @@ -16,6 +17,13 @@ class MockView(View): """This is a basic mock view""" pass + +class MockViewFinal(View): + """View with final() override""" + + def final(self, request, response, *args, **kwargs): + return HttpResponse('{"test": "passed"}', content_type="application/json") + class ResourceMockView(View): """This is a resource-based mock view""" @@ -43,6 +51,7 @@ urlpatterns = patterns('djangorestframework.utils.staticviews', url(r'^accounts/login$', 'api_login'), url(r'^accounts/logout$', 'api_logout'), url(r'^mock/$', MockView.as_view()), + url(r'^mock/final/$', MockViewFinal.as_view()), url(r'^resourcemock/$', ResourceMockView.as_view()), url(r'^model/$', ListOrCreateModelView.as_view(resource=MockResource)), url(r'^model/(?P<pk>[^/]+)/$', InstanceModelView.as_view(resource=MockResource)), @@ -52,6 +61,13 @@ class BaseViewTests(TestCase): """Test the base view class of djangorestframework""" urls = 'djangorestframework.tests.views' + def test_view_call_final(self): + response = self.client.options('/mock/final/') + self.assertEqual(response['Content-Type'].split(';')[0], "application/json") + parser = JSONParser(None) + (data, files) = parser.parse(StringIO(response.content)) + self.assertEqual(data['test'], 'passed') + def test_options_method_simple_view(self): response = self.client.options('/mock/') self._verify_options_response(response, diff --git a/djangorestframework/views.py b/djangorestframework/views.py index 4606e50b..12c94bb6 100644 --- a/djangorestframework/views.py +++ b/djangorestframework/views.py @@ -59,7 +59,6 @@ class View(ResourceMixin, RequestMixin, ResponseMixin, AuthMixin, DjangoView): """ permissions = ( permissions.FullAnonAccess, ) - @classmethod def as_view(cls, **initkwargs): """ @@ -71,7 +70,6 @@ class View(ResourceMixin, RequestMixin, ResponseMixin, AuthMixin, DjangoView): view.cls_instance = cls(**initkwargs) return view - @property def allowed_methods(self): """ @@ -79,7 +77,6 @@ class View(ResourceMixin, RequestMixin, ResponseMixin, AuthMixin, DjangoView): """ return [method.upper() for method in self.http_method_names if hasattr(self, method)] - def http_method_not_allowed(self, request, *args, **kwargs): """ Return an HTTP 405 error if an operation is called which does not have a handler method. @@ -87,15 +84,34 @@ class View(ResourceMixin, RequestMixin, ResponseMixin, AuthMixin, DjangoView): raise ErrorResponse(status.HTTP_405_METHOD_NOT_ALLOWED, {'detail': 'Method \'%s\' not allowed on this resource.' % self.method}) - def initial(self, request, *args, **kargs): """ Hook for any code that needs to run prior to anything else. Required if you want to do things like set `request.upload_handlers` before the authentication and dispatch handling is run. """ - pass + # Calls to 'reverse' will not be fully qualified unless we set the + # scheme/host/port here. + self.orig_prefix = get_script_prefix() + if not (self.orig_prefix.startswith('http:') or self.orig_prefix.startswith('https:')): + prefix = '%s://%s' % (request.is_secure() and 'https' or 'http', request.get_host()) + set_script_prefix(prefix + self.orig_prefix) + + def final(self, request, response, *args, **kargs): + """ + Hook for any code that needs to run after everything else in the view. + """ + # Restore script_prefix. + set_script_prefix(self.orig_prefix) + # Always add these headers. + response.headers['Allow'] = ', '.join(self.allowed_methods) + # sample to allow caching using Vary http header + response.headers['Vary'] = 'Authenticate, Accept' + + # merge with headers possibly set at some point in the view + response.headers.update(self.headers) + return self.render(response) def add_header(self, field, value): """ @@ -103,7 +119,6 @@ class View(ResourceMixin, RequestMixin, ResponseMixin, AuthMixin, DjangoView): """ self.headers[field] = value - # Note: session based authentication is explicitly CSRF validated, # all other authentication is CSRF exempt. @csrf_exempt @@ -113,12 +128,6 @@ class View(ResourceMixin, RequestMixin, ResponseMixin, AuthMixin, DjangoView): self.kwargs = kwargs self.headers = {} - # Calls to 'reverse' will not be fully qualified unless we set the scheme/host/port here. - orig_prefix = get_script_prefix() - if not (orig_prefix.startswith('http:') or orig_prefix.startswith('https:')): - prefix = '%s://%s' % (request.is_secure() and 'https' or 'http', request.get_host()) - set_script_prefix(prefix + orig_prefix) - try: self.initial(request, *args, **kwargs) @@ -143,30 +152,13 @@ class View(ResourceMixin, RequestMixin, ResponseMixin, AuthMixin, DjangoView): else: response = Response(status.HTTP_204_NO_CONTENT) - if request.method == 'OPTIONS': - # do not filter the response for HTTP OPTIONS, else the response fields are lost, - # as they do not correspond with model fields - response.cleaned_content = response.raw_content - else: - # Pre-serialize filtering (eg filter complex objects into natively serializable types) - response.cleaned_content = self.filter_response(response.raw_content) + # Pre-serialize filtering (eg filter complex objects into natively serializable types) + response.cleaned_content = self.filter_response(response.raw_content) except ErrorResponse, exc: response = exc.response - # Always add these headers. - # - # TODO - this isn't actually the correct way to set the vary header, - # also it's currently sub-optimal for HTTP caching - need to sort that out. - response.headers['Allow'] = ', '.join(self.allowed_methods) - response.headers['Vary'] = 'Authenticate, Accept' - - # merge with headers possibly set at some point in the view - response.headers.update(self.headers) - - set_script_prefix(orig_prefix) - - return self.render(response) + return self.final(request, response, *args, **kwargs) def options(self, request, *args, **kwargs): response_obj = { @@ -181,7 +173,10 @@ class View(ResourceMixin, RequestMixin, ResponseMixin, AuthMixin, DjangoView): for name, field in form.fields.iteritems(): field_name_types[name] = field.__class__.__name__ response_obj['fields'] = field_name_types - return response_obj + # Note 'ErrorResponse' is misleading, it's just any response + # that should be rendered and returned immediately, without any + # response filtering. + raise ErrorResponse(status.HTTP_200_OK, response_obj) class ModelView(View): |