Refactor a bunch of stuff into mixins, more tests

djangorestframework/content.py

@@ -29,10 +29,10 @@ class OverloadedContentMixin(ContentMixin):
     """HTTP request content behaviour that also allows arbitrary content to be tunneled in form data."""
     
     """The name to use for the content override field in the POST form."""
-    FORM_PARAM_CONTENT = '_content'
+    CONTENT_PARAM = '_content'
 
     """The name to use for the content-type override field in the POST form."""
-    FORM_PARAM_CONTENTTYPE = '_contenttype'
+    CONTENTTYPE_PARAM = '_contenttype'
 
     def determine_content(self, request):
         """If the request contains content return a tuple of (content_type, content) otherwise return None.
@@ -42,14 +42,14 @@ class OverloadedContentMixin(ContentMixin):
         
         content_type = request.META.get('CONTENT_TYPE', None)
 
-        if (request.method == 'POST' and self.FORM_PARAM_CONTENT and
+        if (request.method == 'POST' and self.CONTENT_PARAM and
-            request.POST.get(self.FORM_PARAM_CONTENT, None) is not None):
+            request.POST.get(self.CONTENT_PARAM, None) is not None):
 
             # Set content type if form contains a none empty FORM_PARAM_CONTENTTYPE field
             content_type = None
-            if self.FORM_PARAM_CONTENTTYPE and request.POST.get(self.FORM_PARAM_CONTENTTYPE, None):
+            if self.CONTENTTYPE_PARAM and request.POST.get(self.CONTENTTYPE_PARAM, None):
-                content_type = request.POST.get(self.FORM_PARAM_CONTENTTYPE, None)
+                content_type = request.POST.get(self.CONTENTTYPE_PARAM, None)
 
-            return (content_type, request.POST[self.FORM_PARAM_CONTENT])
+            return (content_type, request.POST[self.CONTENT_PARAM])
 
         return (content_type, request.raw_post_data)

djangorestframework/emitters.py

@@ -8,6 +8,7 @@ from django.template import RequestContext, loader
 from django import forms
 
 from djangorestframework.response import NoContent
+from djangorestframework.validators import FormValidatorMixin
 from djangorestframework.utils import dict2xml, url_resolves
 
 from urllib import quote_plus
@@ -82,13 +83,17 @@ class DocumentingTemplateEmitter(BaseEmitter):
         In the absence on of the Resource having an associated form then
         provide a form that can be used to submit arbitrary content."""
         # Get the form instance if we have one bound to the input
-        form_instance = resource.form_instance
+        #form_instance = resource.form_instance
+        # TODO! Reinstate this
 
+        form_instance = None
+
+        if isinstance(self, FormValidatorMixin):
             # Otherwise if this isn't an error response
             # then attempt to get a form bound to the response object
             if not form_instance and resource.response.has_content_body:
                 try:
-                form_instance = resource.get_form(resource.response.raw_content)
+                    form_instance = resource.get_bound_form(resource.response.raw_content)
                     if form_instance:
                         form_instance.is_valid()
                 except:
@@ -97,7 +102,7 @@ class DocumentingTemplateEmitter(BaseEmitter):
             # If we still don't have a form instance then try to get an unbound form
             if not form_instance:
                 try:
-                form_instance = self.resource.get_form()
+                    form_instance = self.resource.get_bound_form()
                 except:
                     pass
 

djangorestframework/methods.py

@@ -24,12 +24,12 @@ class OverloadedPOSTMethodMixin(MethodMixin):
     """Provide for overloaded POST behaviour."""
 
     """The name to use for the method override field in the POST form."""
-    FORM_PARAM_METHOD = '_method'
+    METHOD_PARAM = '_method'
 
     def determine_method(self, request):
         """Simply return GET, POST etc... as appropriate, allowing for POST overloading
         by setting a form field with the requested method name."""
         method = request.method.upper()
-        if method == 'POST' and self.FORM_PARAM_METHOD and request.POST.has_key(self.FORM_PARAM_METHOD):
+        if method == 'POST' and self.METHOD_PARAM and request.POST.has_key(self.METHOD_PARAM):
-            method = request.POST[self.FORM_PARAM_METHOD].upper()
+            method = request.POST[self.METHOD_PARAM].upper()
         return method

djangorestframework/parsers.py

@@ -5,7 +5,38 @@ try:
 except ImportError:
     import simplejson as json
 
-# TODO: Make all parsers only list a single media_type, rather than a list
+
+class ParserMixin(object):
+    parsers = ()
+
+    def parse(self, content_type, content):
+        # See RFC 2616 sec 3 - http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.7
+        split = content_type.split(';', 1)
+        if len(split) > 1:
+            content_type = split[0]
+        content_type = content_type.strip()
+
+        media_type_to_parser = dict([(parser.media_type, parser) for parser in self.parsers])
+
+        try:
+            parser = media_type_to_parser[content_type]
+        except KeyError:
+            raise ResponseException(status.HTTP_415_UNSUPPORTED_MEDIA_TYPE,
+                                    {'error': 'Unsupported media type in request \'%s\'.' % content_type})
+        
+        return parser(self).parse(content)
+
+    @property
+    def parsed_media_types(self):
+        """Return an list of all the media types that this ParserMixin can parse."""
+        return [parser.media_type for parser in self.parsers]
+    
+    @property
+    def default_parser(self):
+        """Return the ParerMixin's most prefered emitter.
+        (This has no behavioural effect, but is may be used by documenting emitters)"""        
+        return self.parsers[0]
+
 
 class BaseParser(object):
     """All parsers should extend BaseParser, specifing a media_type attribute,

djangorestframework/resource.py

@@ -2,6 +2,10 @@ from django.contrib.sites.models import Site
 from django.core.urlresolvers import reverse
 from django.http import HttpResponse
 
+from djangorestframework.parsers import ParserMixin
+from djangorestframework.validators import FormValidatorMixin
+from djangorestframework.content import OverloadedContentMixin
+from djangorestframework.methods import OverloadedPOSTMethodMixin 
 from djangorestframework import emitters, parsers, authenticators
 from djangorestframework.response import status, Response, ResponseException
 
@@ -20,7 +24,7 @@ __all__ = ['Resource']
 _MSIE_USER_AGENT = re.compile(r'^Mozilla/[0-9]+\.[0-9]+ \([^)]*; MSIE [0-9]+\.[0-9]+[a-z]?;[^)]*\)(?!.* Opera )')
 
 
-class Resource(object):
+class Resource(ParserMixin, FormValidatorMixin, OverloadedContentMixin, OverloadedPOSTMethodMixin):
     """Handles incoming requests and maps them to REST operations,
     performing authentication, input deserialization, input validation, output serialization."""
 
@@ -53,10 +57,7 @@ class Resource(object):
 
     # Some reserved parameters to allow us to use standard HTML forms with our resource
     # Override any/all of these with None to disable them, or override them with another value to rename them.
-    ACCEPT_QUERY_PARAM = '_accept'        # Allow override of Accept header in URL query params
+    ACCEPT_QUERY_PARAM = '_accept'        # Allow override of Accept header in URL query params    CONTENTTYPE_PARAM = '_contenttype'    # Allow override of Content-Type header in form params (allows sending arbitrary content with standard forms)
-    METHOD_PARAM = '_method'              # Allow POST overloading in form params
-    CONTENTTYPE_PARAM = '_contenttype'    # Allow override of Content-Type header in form params (allows sending arbitrary content with standard forms)
-    CONTENT_PARAM = '_content'            # Allow override of body content in form params (allows sending arbitrary content with standard forms) 
     CSRF_PARAM = 'csrfmiddlewaretoken'    # Django's CSRF token used in form params
 
     _MUNGE_IE_ACCEPT_HEADER = True
@@ -112,18 +113,6 @@ class Resource(object):
         (This emitter is used if the client does not send and Accept: header, or sends Accept: */*)"""
         return self.emitters[0]
 
-    @property
-    def parsed_media_types(self):
-        """Return an list of all the media types that this resource can emit."""
-        return [parser.media_type for parser in self.parsers]
-    
-    @property
-    def default_parser(self):
-        """Return the resource's most prefered emitter.
-        (This has no behavioural effect, but is may be used by documenting emitters)"""        
-        return self.parsers[0]
-
-
     def get(self, request, auth, *args, **kwargs):
         """Must be subclassed to be implemented."""
         self.not_implemented('GET')
@@ -173,17 +162,6 @@ class Resource(object):
         return self.request.build_absolute_uri(path)
 
 
-    def determine_method(self, request):
-        """Determine the HTTP method that this request should be treated as.
-        Allows PUT and DELETE tunneling via the _method parameter if METHOD_PARAM is set."""
-        method = request.method.upper()
-
-        if method == 'POST' and self.METHOD_PARAM and request.POST.has_key(self.METHOD_PARAM):
-            method = request.POST[self.METHOD_PARAM].upper()
-        
-        return method
-
-
     def authenticate(self, request):
         """Attempt to authenticate the request, returning an authentication context or None.
         An authentication context may be any object, although in many cases it will be a User instance."""
@@ -214,58 +192,6 @@ class Resource(object):
                                     {'detail': 'You do not have permission to access this resource. ' +
                                      'You may need to login or otherwise authenticate the request.'})
 
-    def get_form(self, data=None):
-        """Optionally return a Django Form instance, which may be used for validation
-        and/or rendered by an HTML/XHTML emitter.
-        
-        If data is not None the form will be bound to data."""
-
-        if self.form:
-            if data:
-                return self.form(data)
-            else:
-                return self.form()
-        return None
-  
-  
-    def cleanup_request(self, data, form_instance):
-        """Perform any resource-specific data deserialization and/or validation
-        after the initial HTTP content-type deserialization has taken place.
-        
-        Returns a tuple containing the cleaned up data, and optionally a form bound to that data.
-        
-        By default this uses form validation to filter the basic input into the required types."""
-
-        if form_instance is None:
-            return data
-        
-        # Default form validation does not check for additional invalid fields
-        non_existent_fields = []
-        for key in set(data.keys()) - set(form_instance.fields.keys()):
-            non_existent_fields.append(key)
-
-        if not form_instance.is_valid() or non_existent_fields:
-            if not form_instance.errors and not non_existent_fields:
-                # If no data was supplied the errors property will be None
-                details = 'No content was supplied'
-                
-            else:
-                # Add standard field errors
-                details = dict((key, map(unicode, val)) for (key, val) in form_instance.errors.iteritems() if key != '__all__')
-
-                # Add any non-field errors
-                if form_instance.non_field_errors():
-                    details['errors'] = form_instance.non_field_errors()
-
-                # Add any non-existent field errors
-                for key in non_existent_fields:
-                    details[key] = ['This field does not exist']
-
-            # Bail.  Note that we will still serialize this response with the appropriate content type 
-            raise ResponseException(status.HTTP_400_BAD_REQUEST, {'detail': details})
-
-        return form_instance.cleaned_data
-
 
     def cleanup_response(self, data):
         """Perform any resource-specific data filtering prior to the standard HTTP
@@ -275,37 +201,6 @@ class Resource(object):
         return data
 
 
-    def determine_parser(self, request):
-        """Return the appropriate parser for the input, given the client's 'Content-Type' header,
-        and the content types that this Resource knows how to parse."""
-        content_type = request.META.get('CONTENT_TYPE', 'application/x-www-form-urlencoded')
-        raw_content = request.raw_post_data
-    
-        split = content_type.split(';', 1)
-        if len(split) > 1:
-            content_type = split[0]
-        content_type = content_type.strip()
-        
-        # If CONTENTTYPE_PARAM is turned on, and this is a standard POST form then allow the content type to be overridden
-        if (content_type == 'application/x-www-form-urlencoded' and
-            request.method == 'POST' and
-            self.CONTENTTYPE_PARAM and
-            self.CONTENT_PARAM and
-            request.POST.get(self.CONTENTTYPE_PARAM, None) and
-            request.POST.get(self.CONTENT_PARAM, None)):
-            raw_content = request.POST[self.CONTENT_PARAM]
-            content_type = request.POST[self.CONTENTTYPE_PARAM]
-
-        # Create a list of list of (media_type, Parser) tuples
-        media_type_to_parser = dict([(parser.media_type, parser) for parser in self.parsers])
-
-        try:
-            return (media_type_to_parser[content_type], raw_content)
-        except KeyError:
-            raise ResponseException(status.HTTP_415_UNSUPPORTED_MEDIA_TYPE,
-                                    {'detail': 'Unsupported media type \'%s\'' % content_type})
-
-
     def determine_emitter(self, request):
         """Return the appropriate emitter for the output, given the client's 'Accept' header,
         and the content types that this Resource knows how to serve.
@@ -407,11 +302,10 @@ class Resource(object):
             # Either generate the response data, deserializing and validating any request data
             # TODO: Add support for message bodys on other HTTP methods, as it is valid.
             if method in ('PUT', 'POST'):
-                (parser, raw_content) = self.determine_parser(request)
+                (content_type, content) = self.determine_content(request)
-                data = parser(self).parse(raw_content)
+                parser_content = self.parse(content_type, content)
-                self.form_instance = self.get_form(data)
+                cleaned_content = self.validate(parser_content)
-                data = self.cleanup_request(data, self.form_instance)
+                response = func(request, auth_context, cleaned_content, *args, **kwargs)
-                response = func(request, auth_context, data, *args, **kwargs)
 
             else:
                 response = func(request, auth_context, *args, **kwargs)

djangorestframework/tests/content.py

@@ -21,8 +21,8 @@ class TestContentMixins(TestCase):
     def test_overloaded_content_mixin_interface(self):
         """Ensure the OverloadedContentMixin interface is as expected."""
         self.assertTrue(issubclass(OverloadedContentMixin, ContentMixin))
-        getattr(OverloadedContentMixin, 'FORM_PARAM_CONTENT')
+        getattr(OverloadedContentMixin, 'CONTENT_PARAM')
-        getattr(OverloadedContentMixin, 'FORM_PARAM_CONTENTTYPE')
+        getattr(OverloadedContentMixin, 'CONTENTTYPE_PARAM')
         getattr(OverloadedContentMixin, 'determine_content')
 
   
@@ -107,14 +107,14 @@ class TestContentMixins(TestCase):
         """Ensure determine_content(request) returns (content type, content) for overloaded POST request"""
         content = 'qwerty'
         content_type = 'text/plain'
-        form_data = {OverloadedContentMixin.FORM_PARAM_CONTENT: content,
+        form_data = {OverloadedContentMixin.CONTENT_PARAM: content,
-                     OverloadedContentMixin.FORM_PARAM_CONTENTTYPE: content_type}
+                     OverloadedContentMixin.CONTENTTYPE_PARAM: content_type}
         request = self.req.post('/', form_data)
         self.assertEqual(OverloadedContentMixin().determine_content(request), (content_type, content))
  
     def test_overloaded_behaviour_allows_content_tunnelling_content_type_not_set(self):
         """Ensure determine_content(request) returns (None, content) for overloaded POST request with content type not set"""
         content = 'qwerty'
-        request = self.req.post('/', {OverloadedContentMixin.FORM_PARAM_CONTENT: content})
+        request = self.req.post('/', {OverloadedContentMixin.CONTENT_PARAM: content})
         self.assertEqual(OverloadedContentMixin().determine_content(request), (None, content))
 

djangorestframework/tests/methods.py

@@ -21,7 +21,7 @@ class TestMethodMixins(TestCase):
     def test_overloaded_method_mixin_interface(self):
         """Ensure the OverloadedPOSTMethodMixin interface is as expected."""
         self.assertTrue(issubclass(OverloadedPOSTMethodMixin, MethodMixin))
-        getattr(OverloadedPOSTMethodMixin, 'FORM_PARAM_METHOD')
+        getattr(OverloadedPOSTMethodMixin, 'METHOD_PARAM')
         getattr(OverloadedPOSTMethodMixin, 'determine_method')
 
     # Behavioural tests
@@ -48,5 +48,5 @@ class TestMethodMixins(TestCase):
     
     def test_overloaded_POST_behaviour_determines_overloaded_method(self):
         """POST requests can be overloaded to another method by setting a reserved form field with OverloadedPOSTMethodMixin"""
-        request = self.req.post('/', {OverloadedPOSTMethodMixin.FORM_PARAM_METHOD: 'DELETE'})
+        request = self.req.post('/', {OverloadedPOSTMethodMixin.METHOD_PARAM: 'DELETE'})
         self.assertEqual(OverloadedPOSTMethodMixin().determine_method(request), 'DELETE')

djangorestframework/validators.py

@@ -0,0 +1,153 @@
+"""Mixin classes that provide a validate(content) function to validate and cleanup request content"""
+from django import forms
+from django.db import models
+from djangorestframework.response import ResponseException
+from djangorestframework.utils import as_tuple
+
+class ValidatorMixin(object):
+    """Base class for all ValidatorMixin classes, which simply defines the interface they provide."""
+
+    def validate(self, content):
+        """Given some content as input return some cleaned, validated content.
+        Raises a ResponseException with status code 400 (Bad Request) on failure.
+        
+        Must be overridden to be implemented."""
+        raise NotImplementedError()
+
+
+class FormValidatorMixin(ValidatorMixin):
+    """Validator Mixin that uses forms for validation.
+    Extends the ValidatorMixin interface to also provide a get_bound_form() method.
+    (Which may be used by some emitters.)"""
+    
+    """The form class that should be used for validation, or None to turn off form validation."""
+    form = None
+
+    def validate(self, content):
+        """Given some content as input return some cleaned, validated content.
+        Raises a ResponseException with status code 400 (Bad Request) on failure.
+        
+        Validation is standard form validation, with an additional constraint that no extra unknown fields may be supplied.
+
+        On failure the ResponseException content is a dict which may contain 'errors' and 'field-errors' keys.
+        If the 'errors' key exists it is a list of strings of non-field errors.
+        If the 'field-errors' key exists it is a dict of {field name as string: list of errors as strings}."""
+        return self._validate(content)
+
+    def _validate(self, content, extra_fields=()):
+        """Wrapped by validate to hide the extra_fields option that the ModelValidatorMixin uses.
+        extra_fields is a list of fields which are not defined by the form, but which we still
+        expect to see on the input."""
+        if self.form is None:
+            return content
+        
+        bound_form = self.get_bound_form(content)
+
+        # In addition to regular validation we also ensure no additional fields are being passed in...
+        unknown_fields = set(content.keys()) - set(self.form().fields.keys()) - set(extra_fields)
+
+        # And that any extra fields we have specified are all present.
+        missing_extra_fields = set(extra_fields) - set(content.keys())
+
+        # Check using both regular validation, and our stricter no additional fields rule
+        if bound_form.is_valid() and not unknown_fields and not missing_extra_fields:
+            return bound_form.cleaned_data
+
+        # Validation failed...
+        detail = {}
+
+        if not bound_form.errors and not unknown_fields and not missing_extra_fields:
+            detail = {u'errors': [u'No content was supplied.']}
+
+        else:       
+            # Add any non-field errors
+            if bound_form.non_field_errors():
+                detail[u'errors'] = bound_form.non_field_errors()
+
+            # Add standard field errors
+            field_errors = dict((key, map(unicode, val)) for (key, val) in bound_form.errors.iteritems() if not key.startswith('__'))
+
+            # Add any unknown field errors
+            for key in unknown_fields:
+                field_errors[key] = [u'This field does not exist.']
+            
+            # Add any missing fields that we required by the extra fields argument
+            for key in missing_extra_fields:
+                field_errors[key] = [u'This field is required.']
+       
+            if field_errors:
+                detail[u'field-errors'] = field_errors
+
+        # Return HTTP 400 response (BAD REQUEST)
+        raise ResponseException(400, detail)
+  
+
+    def get_bound_form(self, content=None):
+        """Given some content return a Django form bound to that content.
+        If form validation is turned off (form class attribute is None) then returns None."""
+        if not self.form:
+            return None
+
+        if content:
+            return self.form(content)
+        return self.form()
+
+
+class ModelFormValidatorMixin(FormValidatorMixin):
+    """Validator Mixin that uses forms for validation and falls back to a model form if no form is set.
+    Extends the ValidatorMixin interface to also provide a get_bound_form() method.
+    (Which may be used by some emitters.)"""
+ 
+    """The form class that should be used for validation, or None to use model form validation."""   
+    form = None
+    
+    """The model class from which the model form should be constructed if no form is set."""
+    model = None
+    
+    """The list of fields we expect to receive as input.  Fields in this list will may be received with
+    raising non-existent field errors, even if they do not exist as fields on the ModelForm."""
+    fields = None
+
+    # TODO: test the different validation here to allow for get get_absolute_url to be supplied on input and not bork out
+    # TODO: be really strict on fields - check they match in the handler methods. (this isn't a validator thing tho.)
+    def validate(self, content):
+        """Given some content as input return some cleaned, validated content.
+        Raises a ResponseException with status code 400 (Bad Request) on failure.
+        
+        Validation is standard form or model form validation,
+        with an additional constraint that no extra unknown fields may be supplied,
+        and that all fields specified by the fields class attribute must be supplied,
+        even if they are not validated by the form/model form.
+
+        On failure the ResponseException content is a dict which may contain 'errors' and 'field-errors' keys.
+        If the 'errors' key exists it is a list of strings of non-field errors.
+        If the 'field-errors' key exists it is a dict of {field name as string: list of errors as strings}."""
+        extra_fields = set(as_tuple(self.fields)) - set(self.get_bound_form().fields)
+        return self._validate(content, extra_fields)
+
+
+    def get_bound_form(self, content=None):
+        """Given some content return a Django form bound to that content.
+
+        If the form class attribute has been explicitly set then use that class to create a Form,
+        otherwise if model is set use that class to create a ModelForm, otherwise return None."""
+        if self.form:
+            # Use explict Form
+            return super(ModelFormValidatorMixin, self).get_bound_form(content)
+
+        elif self.model:
+            # Fall back to ModelForm which we create on the fly
+            class ModelForm(forms.ModelForm):
+                class Meta:
+                    model = self.model
+                    fields = tuple(set.intersection(self.model._meta.fields, self.fields))
+                    
+            # Instantiate the ModelForm as appropriate
+            if content and isinstance(content, models.Model):
+                return ModelForm(instance=content)
+            elif content:
+                return ModelForm(content)
+            return ModelForm()
+
+        # Both form and model not set?  Okay bruv, whatevs...
+        return None