From 912a897e2d9fe1512a1e2bc545b185550f5d4386 Mon Sep 17 00:00:00 2001 From: Mjumbe Wawatu Poe Date: Sat, 25 Aug 2012 01:15:59 -0400 Subject: [PATCH 1/8] Add a method to cache the model mixin. - Specifically can cache the instance on a read, update, or delete operation. - Also adds a get_instance_or_404 method - Useful for retrieving the instance from a permission class without doing extra queries --- djangorestframework/mixins.py | 56 ++++++++++++++--------------- djangorestframework/tests/mixins.py | 8 +++++ 2 files changed, 36 insertions(+), 28 deletions(-) diff --git a/djangorestframework/mixins.py b/djangorestframework/mixins.py index 4a4539574..6706c62d9 100644 --- a/djangorestframework/mixins.py +++ b/djangorestframework/mixins.py @@ -504,12 +504,6 @@ class ModelMixin(object): return all_kw_args - def get_instance(self, **kwargs): - """ - Get a model instance for read/update/delete requests. - """ - return self.get_queryset().get(**kwargs) - def get_queryset(self): """ Return the queryset for this view. @@ -524,20 +518,34 @@ class ModelMixin(object): return getattr(self.resource, 'ordering', None) -class ReadModelMixin(ModelMixin): +class ExistingInstanceMixin (object): + """ + Assume a single instance for the view. Caches the instance object on self. + """ + + def get_instance(self): + if not hasattr(self, 'model_instance'): + query_kwargs = self.get_query_kwargs( + self.request, *self.args, **self.kwargs) + self.model_instance = self.get_queryset().get(**query_kwargs) + return self.model_instance + + def get_instance_or_404(self): + model = self.resource.model + + try: + return self.get_instance() + except model.DoesNotExist: + raise ErrorResponse(status.HTTP_404_NOT_FOUND) + + +class ReadModelMixin(ModelMixin, ExistingInstanceMixin): """ Behavior to read a `model` instance on GET requests """ def get(self, request, *args, **kwargs): - model = self.resource.model - query_kwargs = self.get_query_kwargs(request, *args, **kwargs) - - try: - self.model_instance = self.get_instance(**query_kwargs) - except model.DoesNotExist: - raise ErrorResponse(status.HTTP_404_NOT_FOUND) - - return self.model_instance + instance = self.get_instance_or_404() + return instance class CreateModelMixin(ModelMixin): @@ -580,18 +588,17 @@ class CreateModelMixin(ModelMixin): return Response(status.HTTP_201_CREATED, instance, headers) -class UpdateModelMixin(ModelMixin): +class UpdateModelMixin(ModelMixin, ExistingInstanceMixin): """ Behavior to update a `model` instance on PUT requests """ def put(self, request, *args, **kwargs): model = self.resource.model - query_kwargs = self.get_query_kwargs(request, *args, **kwargs) # 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: - self.model_instance = self.get_instance(**query_kwargs) + self.model_instance = self.get_instance() for (key, val) in self.CONTENT.items(): setattr(self.model_instance, key, val) @@ -601,19 +608,12 @@ class UpdateModelMixin(ModelMixin): return self.model_instance -class DeleteModelMixin(ModelMixin): +class DeleteModelMixin(ModelMixin, ExistingInstanceMixin): """ Behavior to delete a `model` instance on DELETE requests """ def delete(self, request, *args, **kwargs): - model = self.resource.model - query_kwargs = self.get_query_kwargs(request, *args, **kwargs) - - try: - instance = self.get_instance(**query_kwargs) - except model.DoesNotExist: - raise ErrorResponse(status.HTTP_404_NOT_FOUND, None, {}) - + instance = self.get_instance_or_404() instance.delete() return diff --git a/djangorestframework/tests/mixins.py b/djangorestframework/tests/mixins.py index 8268fdca7..d840cbba6 100644 --- a/djangorestframework/tests/mixins.py +++ b/djangorestframework/tests/mixins.py @@ -30,6 +30,10 @@ class TestModelRead(TestModelsTestCase): mixin = ReadModelMixin() mixin.resource = GroupResource + mixin.request = request + mixin.args = () + mixin.kwargs = {'id': group.id} + response = mixin.get(request, id=group.id) self.assertEquals(group.name, response.name) @@ -41,6 +45,10 @@ class TestModelRead(TestModelsTestCase): mixin = ReadModelMixin() mixin.resource = GroupResource + mixin.request = request + mixin.args = () + mixin.kwargs = {'id': 12345} + self.assertRaises(ErrorResponse, mixin.get, request, id=12345) From 3cef6bd02c3f594ab3ea3260fb181c2d9bb37d6d Mon Sep 17 00:00:00 2001 From: Mjumbe Wawatu Poe Date: Sat, 25 Aug 2012 10:54:21 -0400 Subject: [PATCH 2/8] PUT returns a 201 status when instance was created. Note: This behavior is still idempotent, as the state of the system is the same after multiple PUT requests as it is after one. However, it is helpful to know whether an instance was created or whether it already existed. --- djangorestframework/mixins.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/djangorestframework/mixins.py b/djangorestframework/mixins.py index 6706c62d9..53c4fda5a 100644 --- a/djangorestframework/mixins.py +++ b/djangorestframework/mixins.py @@ -181,7 +181,7 @@ class RequestMixin(object): return parser.parse(stream) raise ErrorResponse(status.HTTP_415_UNSUPPORTED_MEDIA_TYPE, - {'detail': 'Unsupported media type in request \'%s\'.' % + {'detail': 'Unsupported media type in request \'%s\'.' % content_type}) @property @@ -522,14 +522,14 @@ class ExistingInstanceMixin (object): """ Assume a single instance for the view. Caches the instance object on self. """ - + def get_instance(self): if not hasattr(self, 'model_instance'): query_kwargs = self.get_query_kwargs( self.request, *self.args, **self.kwargs) self.model_instance = self.get_queryset().get(**query_kwargs) return self.model_instance - + def get_instance_or_404(self): model = self.resource.model @@ -538,7 +538,7 @@ class ExistingInstanceMixin (object): except model.DoesNotExist: raise ErrorResponse(status.HTTP_404_NOT_FOUND) - + class ReadModelMixin(ModelMixin, ExistingInstanceMixin): """ Behavior to read a `model` instance on GET requests @@ -602,10 +602,13 @@ class UpdateModelMixin(ModelMixin, ExistingInstanceMixin): for (key, val) in self.CONTENT.items(): setattr(self.model_instance, key, val) + + self.model_instance.save() + return self.model_instance except model.DoesNotExist: self.model_instance = model(**self.get_instance_data(model, self.CONTENT, *args, **kwargs)) - self.model_instance.save() - return self.model_instance + self.model_instance.save() + return Response(status.HTTP_201_CREATED, self.model_instance) class DeleteModelMixin(ModelMixin, ExistingInstanceMixin): From 2c91357b535d33a25aa3f380aacc62cab3bd2e47 Mon Sep 17 00:00:00 2001 From: Mjumbe Wawatu Poe Date: Sat, 25 Aug 2012 18:58:45 -0400 Subject: [PATCH 3/8] Move instance creation logic into the EditInstanceMixin. --- djangorestframework/mixins.py | 77 +++++++++++++++++++++++------------ 1 file changed, 51 insertions(+), 26 deletions(-) diff --git a/djangorestframework/mixins.py b/djangorestframework/mixins.py index 53c4fda5a..b26a77c9b 100644 --- a/djangorestframework/mixins.py +++ b/djangorestframework/mixins.py @@ -538,21 +538,8 @@ class ExistingInstanceMixin (object): except model.DoesNotExist: raise ErrorResponse(status.HTTP_404_NOT_FOUND) - -class ReadModelMixin(ModelMixin, ExistingInstanceMixin): - """ - Behavior to read a `model` instance on GET requests - """ - def get(self, request, *args, **kwargs): - instance = self.get_instance_or_404() - return instance - - -class CreateModelMixin(ModelMixin): - """ - Behavior to create a `model` instance on POST requests - """ - def post(self, request, *args, **kwargs): +class EditInstanceMixin (object): + def _separate_m2m_data_from_content(self, model): model = self.resource.model # Copy the dict to keep self.CONTENT intact @@ -566,12 +553,17 @@ class CreateModelMixin(ModelMixin): ) del content[field.name] - instance = model(**self.get_instance_data(model, content, *args, **kwargs)) - instance.save() + return content, m2m_data + def _set_m2m_data(self, instance, m2m_data): for fieldname in m2m_data: manager = getattr(instance, fieldname) + # If we are updating an existing model, we want to clear out + # existing relationships. + if hasattr(manager, 'clear'): + manager.clear() + if hasattr(manager, 'add'): manager.add(*m2m_data[fieldname][1]) else: @@ -582,33 +574,66 @@ class CreateModelMixin(ModelMixin): data[m2m_data[fieldname][0]] = related_item manager.through(**data).save() + +class ReadModelMixin(ModelMixin, ExistingInstanceMixin): + """ + Behavior to read a `model` instance on GET requests + """ + def get(self, request, *args, **kwargs): + instance = self.get_instance_or_404() + return instance + + +class CreateModelMixin(ModelMixin, EditInstanceMixin): + """ + Behavior to create a `model` instance on POST requests + """ + def post(self, request, *args, **kwargs): + model = self.resource.model + + content, m2m_data = self._separate_m2m_data_from_content(model) + instance_data = self.get_instance_data(model, content, *args, **kwargs) + + instance = model(**instance_data) + instance.save() + + self._set_m2m_data(instance, m2m_data) + headers = {} if hasattr(self.resource, 'url'): headers['Location'] = self.resource(self).url(instance) return Response(status.HTTP_201_CREATED, instance, headers) -class UpdateModelMixin(ModelMixin, ExistingInstanceMixin): +class UpdateModelMixin(ModelMixin, ExistingInstanceMixin, EditInstanceMixin): """ Behavior to update a `model` instance on PUT requests """ def put(self, request, *args, **kwargs): model = self.resource.model + instance_is_new = False + + content, m2m_data = self._separate_m2m_data_from_content(model) + instance_data = self.get_instance_data(model, content, *args, **kwargs) + # 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: - self.model_instance = self.get_instance() + instance = self.get_instance() - for (key, val) in self.CONTENT.items(): - setattr(self.model_instance, key, val) + for (key, val) in instance_data.items(): + setattr(instance, key, val) - self.model_instance.save() - return self.model_instance except model.DoesNotExist: - self.model_instance = model(**self.get_instance_data(model, self.CONTENT, *args, **kwargs)) - self.model_instance.save() - return Response(status.HTTP_201_CREATED, self.model_instance) + instance_is_new = True + instance = model(**instance_data) + + instance.save() + self._set_m2m_data(instance, m2m_data) + + return (Response(status.HTTP_201_CREATED, instance) if instance_is_new + else instance) class DeleteModelMixin(ModelMixin, ExistingInstanceMixin): From 2e7325910df39c92f843d8110c3d9c50fbb4f9d3 Mon Sep 17 00:00:00 2001 From: Mjumbe Wawatu Poe Date: Sat, 25 Aug 2012 19:04:59 -0400 Subject: [PATCH 4/8] Rename ExistingInstanceMixin to InstanceReaderMixin and EditInstanceMixin to InstanceWriterMixin. --- djangorestframework/mixins.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/djangorestframework/mixins.py b/djangorestframework/mixins.py index b26a77c9b..bee03d7c6 100644 --- a/djangorestframework/mixins.py +++ b/djangorestframework/mixins.py @@ -518,7 +518,7 @@ class ModelMixin(object): return getattr(self.resource, 'ordering', None) -class ExistingInstanceMixin (object): +class InstanceReaderMixin (object): """ Assume a single instance for the view. Caches the instance object on self. """ @@ -538,7 +538,7 @@ class ExistingInstanceMixin (object): except model.DoesNotExist: raise ErrorResponse(status.HTTP_404_NOT_FOUND) -class EditInstanceMixin (object): +class InstanceWriterMixin (object): def _separate_m2m_data_from_content(self, model): model = self.resource.model @@ -575,7 +575,7 @@ class EditInstanceMixin (object): manager.through(**data).save() -class ReadModelMixin(ModelMixin, ExistingInstanceMixin): +class ReadModelMixin(ModelMixin, InstanceReaderMixin): """ Behavior to read a `model` instance on GET requests """ @@ -584,7 +584,7 @@ class ReadModelMixin(ModelMixin, ExistingInstanceMixin): return instance -class CreateModelMixin(ModelMixin, EditInstanceMixin): +class CreateModelMixin(ModelMixin, InstanceWriterMixin): """ Behavior to create a `model` instance on POST requests """ @@ -605,7 +605,7 @@ class CreateModelMixin(ModelMixin, EditInstanceMixin): return Response(status.HTTP_201_CREATED, instance, headers) -class UpdateModelMixin(ModelMixin, ExistingInstanceMixin, EditInstanceMixin): +class UpdateModelMixin(ModelMixin, InstanceReaderMixin, InstanceWriterMixin): """ Behavior to update a `model` instance on PUT requests """ @@ -636,7 +636,7 @@ class UpdateModelMixin(ModelMixin, ExistingInstanceMixin, EditInstanceMixin): else instance) -class DeleteModelMixin(ModelMixin, ExistingInstanceMixin): +class DeleteModelMixin(ModelMixin, InstanceReaderMixin): """ Behavior to delete a `model` instance on DELETE requests """ From 8f4590e8344dd67d4eb644f766f5cef10ebf4f3a Mon Sep 17 00:00:00 2001 From: Mjumbe Wawatu Poe Date: Sat, 25 Aug 2012 19:35:06 -0400 Subject: [PATCH 5/8] - Move get_instance_data to the InstanceWriterMixin - Add comments to the InstanceReaderMixin and InstanceWriterMixin classes --- djangorestframework/mixins.py | 269 ++++++++++++++++++++-------------- 1 file changed, 161 insertions(+), 108 deletions(-) diff --git a/djangorestframework/mixins.py b/djangorestframework/mixins.py index bee03d7c6..8386b429e 100644 --- a/djangorestframework/mixins.py +++ b/djangorestframework/mixins.py @@ -454,9 +454,6 @@ class ModelMixin(object): 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. """ queryset = None @@ -477,7 +474,104 @@ class ModelMixin(object): return kwargs - def get_instance_data(self, model, content, **kwargs): + def get_queryset(self): + """ + Return the queryset for this view. + """ + return getattr(self.resource, 'queryset', + self.resource.model.objects.all()) + + def get_ordering(self): + """ + Return the ordering for this view. + """ + return getattr(self.resource, 'ordering', None) + + +class InstanceReaderMixin (object): + """ + Assume a single instance for the view. Caches the instance object on self. + """ + + def get_instance(self): + """ + Return the instance for this view. Raises a DoesNotExist error if + instance does not exist. + """ + if not hasattr(self, 'model_instance'): + query_kwargs = self.get_query_kwargs( + self.request, *self.args, **self.kwargs) + self.model_instance = self.get_queryset().get(**query_kwargs) + return self.model_instance + + def get_instance_or_404(self): + """ + Return the instance for this view, or raise a 404 response if the + instance is not found. + """ + model = self.resource.model + + try: + return self.get_instance() + except model.DoesNotExist: + raise ErrorResponse(status.HTTP_404_NOT_FOUND) + +class InstanceWriterMixin (object): + """ + Abstracts out the logic for applying many-to-many data to a single instance. + """ + + def _separate_m2m_data_from_content(self, model, content): + """ + Split the view's CONTENT into atomic data and many-to-many data. + Retrns a pair of (non_m2m_data, m2m_data) + + Arguments: + - model: model class (django.db.models.Model subclass) to work with + - content: a dictionary with instance data + """ + # Copy the dict to keep it intact + content = dict(content) + m2m_data = {} + + for field in model._meta.many_to_many: + if field.name in content: + m2m_data[field.name] = ( + field.m2m_reverse_field_name(), content[field.name] + ) + del content[field.name] + + non_m2m_data = content + return non_m2m_data, m2m_data + + def _set_m2m_data(self, instance, m2m_data): + """ + Apply the many-to-many data to the given instance. + + Arguments: + - instance: model instance to work with + - m2m_data: a mapping from fieldname to list of identifiers of related + objects + """ + for fieldname in m2m_data: + manager = getattr(instance, fieldname) + + # If we are updating an existing model, we want to clear out + # existing relationships. + if hasattr(manager, 'clear'): + manager.clear() + + if hasattr(manager, 'add'): + manager.add(*m2m_data[fieldname][1]) + else: + data = {} + data[manager.source_field_name] = instance + + for related_item in m2m_data[fieldname][1]: + data[m2m_data[fieldname][0]] = related_item + manager.through(**data).save() + + def _get_instance_data(self, model, content, **kwargs): """ Returns the dict with the data for model instance creation/update. @@ -504,118 +598,35 @@ class ModelMixin(object): return all_kw_args - def get_queryset(self): + def create_instance(self): """ - Return the queryset for this view. + Create the instance for this view. """ - return getattr(self.resource, 'queryset', - self.resource.model.objects.all()) - - def get_ordering(self): - """ - Return the ordering for this view. - """ - return getattr(self.resource, 'ordering', None) - - -class InstanceReaderMixin (object): - """ - Assume a single instance for the view. Caches the instance object on self. - """ - - def get_instance(self): - if not hasattr(self, 'model_instance'): - query_kwargs = self.get_query_kwargs( - self.request, *self.args, **self.kwargs) - self.model_instance = self.get_queryset().get(**query_kwargs) - return self.model_instance - - def get_instance_or_404(self): model = self.resource.model - try: - return self.get_instance() - except model.DoesNotExist: - raise ErrorResponse(status.HTTP_404_NOT_FOUND) - -class InstanceWriterMixin (object): - def _separate_m2m_data_from_content(self, model): - model = self.resource.model - - # Copy the dict to keep self.CONTENT intact - content = dict(self.CONTENT) - m2m_data = {} - - for field in model._meta.many_to_many: - if field.name in content: - m2m_data[field.name] = ( - field.m2m_reverse_field_name(), content[field.name] - ) - del content[field.name] - - return content, m2m_data - - def _set_m2m_data(self, instance, m2m_data): - for fieldname in m2m_data: - manager = getattr(instance, fieldname) - - # If we are updating an existing model, we want to clear out - # existing relationships. - if hasattr(manager, 'clear'): - manager.clear() - - if hasattr(manager, 'add'): - manager.add(*m2m_data[fieldname][1]) - else: - data = {} - data[manager.source_field_name] = instance - - for related_item in m2m_data[fieldname][1]: - data[m2m_data[fieldname][0]] = related_item - manager.through(**data).save() - - -class ReadModelMixin(ModelMixin, InstanceReaderMixin): - """ - Behavior to read a `model` instance on GET requests - """ - def get(self, request, *args, **kwargs): - instance = self.get_instance_or_404() - return instance - - -class CreateModelMixin(ModelMixin, InstanceWriterMixin): - """ - Behavior to create a `model` instance on POST requests - """ - def post(self, request, *args, **kwargs): - model = self.resource.model - - content, m2m_data = self._separate_m2m_data_from_content(model) - instance_data = self.get_instance_data(model, content, *args, **kwargs) + content, m2m_data = self._separate_m2m_data_from_content(model, + self.CONTENT) + instance_data = self._get_instance_data(model, content, + *self.args, **self.kwargs) instance = model(**instance_data) instance.save() self._set_m2m_data(instance, m2m_data) + return instance - headers = {} - if hasattr(self.resource, 'url'): - headers['Location'] = self.resource(self).url(instance) - return Response(status.HTTP_201_CREATED, instance, headers) - - -class UpdateModelMixin(ModelMixin, InstanceReaderMixin, InstanceWriterMixin): - """ - Behavior to update a `model` instance on PUT requests - """ - def put(self, request, *args, **kwargs): + def create_or_update_instance(self): + """ + Update the instance for this view, or create it if it does not yet + exist. Assumes the view is also an InstanceReaderMixin + """ model = self.resource.model instance_is_new = False - - content, m2m_data = self._separate_m2m_data_from_content(model) - instance_data = self.get_instance_data(model, content, *args, **kwargs) + content, m2m_data = self._separate_m2m_data_from_content(model, + self.CONTENT) + instance_data = self._get_instance_data(model, content, + *self.args, **self.kwargs) # 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 @@ -632,17 +643,59 @@ class UpdateModelMixin(ModelMixin, InstanceReaderMixin, InstanceWriterMixin): instance.save() self._set_m2m_data(instance, m2m_data) - return (Response(status.HTTP_201_CREATED, instance) if instance_is_new - else instance) + return instance, instance_is_new + + def delete_instance(self): + """ + Delete the instance for this view. Assumes the view is also an + InstanceReaderMixin. + """ + instance = self.get_instance_or_404() + instance.delete() -class DeleteModelMixin(ModelMixin, InstanceReaderMixin): +class ReadModelMixin(ModelMixin, InstanceReaderMixin): + """ + Behavior to read a `model` instance on GET requests + """ + def get(self, request, *args, **kwargs): + instance = self.get_instance_or_404() + return instance + + +class CreateModelMixin(ModelMixin, InstanceWriterMixin): + """ + Behavior to create a `model` instance on POST requests + """ + def post(self, request, *args, **kwargs): + instance = self.create_instance() + + headers = {} + if hasattr(self.resource, 'url'): + headers['Location'] = self.resource(self).url(instance) + + return Response(status.HTTP_201_CREATED, instance, headers) + + +class UpdateModelMixin(ModelMixin, InstanceReaderMixin, InstanceWriterMixin): + """ + Behavior to update a `model` instance on PUT requests + """ + def put(self, request, *args, **kwargs): + instance, instance_is_new = self.create_or_update_instance() + + if instance_is_new: + return Response(status.HTTP_201_CREATED, instance) + else: + return instance + + +class DeleteModelMixin(ModelMixin, InstanceReaderMixin, InstanceWriterMixin): """ Behavior to delete a `model` instance on DELETE requests """ def delete(self, request, *args, **kwargs): - instance = self.get_instance_or_404() - instance.delete() + self.delete_instance(instance) return From b685a22f2ca6570898624825b97717b97c16c1bd Mon Sep 17 00:00:00 2001 From: Mjumbe Wawatu Poe Date: Sat, 25 Aug 2012 19:36:55 -0400 Subject: [PATCH 6/8] Add tests for the new functionality. - Cover the InstanceReaderMixin and InstanceWriterMixin classes - New tests to make sure that UpdateModelMixin can write models with many-to-many data --- djangorestframework/tests/mixins.py | 181 +++++++++++++++++++++++++++- 1 file changed, 178 insertions(+), 3 deletions(-) diff --git a/djangorestframework/tests/mixins.py b/djangorestframework/tests/mixins.py index d840cbba6..315513b8c 100644 --- a/djangorestframework/tests/mixins.py +++ b/djangorestframework/tests/mixins.py @@ -4,7 +4,8 @@ from django.utils import simplejson as json from djangorestframework import status from djangorestframework.compat import RequestFactory from django.contrib.auth.models import Group, User -from djangorestframework.mixins import CreateModelMixin, PaginatorMixin, ReadModelMixin +from djangorestframework.mixins import (CreateModelMixin, PaginatorMixin, + ReadModelMixin, UpdateModelMixin) from djangorestframework.resources import ModelResource from djangorestframework.response import Response, ErrorResponse from djangorestframework.tests.models import CustomUser @@ -33,7 +34,7 @@ class TestModelRead(TestModelsTestCase): mixin.request = request mixin.args = () mixin.kwargs = {'id': group.id} - + response = mixin.get(request, id=group.id) self.assertEquals(group.name, response.name) @@ -48,7 +49,7 @@ class TestModelRead(TestModelsTestCase): mixin.request = request mixin.args = () mixin.kwargs = {'id': 12345} - + self.assertRaises(ErrorResponse, mixin.get, request, id=12345) @@ -71,6 +72,10 @@ class TestModelCreation(TestModelsTestCase): mixin.resource = GroupResource mixin.CONTENT = form_data + mixin.request = request + mixin.args = () + mixin.kwargs = {} + response = mixin.post(request) self.assertEquals(1, Group.objects.count()) self.assertEquals('foo', response.cleaned_content.name) @@ -97,6 +102,10 @@ class TestModelCreation(TestModelsTestCase): mixin.resource = UserResource mixin.CONTENT = cleaned_data + mixin.request = request + mixin.args = () + mixin.kwargs = {} + response = mixin.post(request) self.assertEquals(1, User.objects.count()) self.assertEquals(1, response.cleaned_content.groups.count()) @@ -120,6 +129,10 @@ class TestModelCreation(TestModelsTestCase): mixin.resource = UserResource mixin.CONTENT = cleaned_data + mixin.request = request + mixin.args = () + mixin.kwargs = {} + response = mixin.post(request) self.assertEquals(1, CustomUser.objects.count()) self.assertEquals(0, response.cleaned_content.groups.count()) @@ -135,6 +148,10 @@ class TestModelCreation(TestModelsTestCase): mixin.resource = UserResource mixin.CONTENT = cleaned_data + mixin.request = request + mixin.args = () + mixin.kwargs = {} + response = mixin.post(request) self.assertEquals(2, CustomUser.objects.count()) self.assertEquals(1, response.cleaned_content.groups.count()) @@ -151,6 +168,10 @@ class TestModelCreation(TestModelsTestCase): mixin.resource = UserResource mixin.CONTENT = cleaned_data + mixin.request = request + mixin.args = () + mixin.kwargs = {} + response = mixin.post(request) self.assertEquals(3, CustomUser.objects.count()) self.assertEquals(2, response.cleaned_content.groups.count()) @@ -158,6 +179,160 @@ class TestModelCreation(TestModelsTestCase): self.assertEquals('foo2', response.cleaned_content.groups.all()[1].name) +class TestModelUpdate(TestModelsTestCase): + """Tests on UpdateModelMixin""" + + def setUp(self): + super(TestModelsTestCase, self).setUp() + self.req = RequestFactory() + + def test_update(self): + group = Group.objects.create(name='my group') + + self.assertEquals(1, Group.objects.count()) + + class GroupResource(ModelResource): + model = Group + + # Update existing + form_data = {'name': 'my renamed group'} + request = self.req.put('/groups/' + str(group.pk), data=form_data) + mixin = UpdateModelMixin() + mixin.resource = GroupResource + mixin.CONTENT = form_data + + mixin.request = request + mixin.args = () + mixin.kwargs = {'pk': group.pk} + + response = mixin.put(request, **mixin.kwargs) + self.assertEquals(1, Group.objects.count()) + self.assertEquals('my renamed group', response.name) + + # Create new + form_data = {'name': 'other group'} + request = self.req.put('/groups/' + str(group.pk + 1), data=form_data) + mixin = UpdateModelMixin() + mixin.resource = GroupResource + mixin.CONTENT = form_data + + mixin.request = request + mixin.args = () + mixin.kwargs = {'pk': group.pk + 1} + + response = mixin.put(request, **mixin.kwargs) + self.assertEquals(2, Group.objects.count()) + self.assertEquals('other group', response.cleaned_content.name) + self.assertEquals(201, response.status) + + def test_update_with_m2m_relation(self): + class UserResource(ModelResource): + model = User + + def url(self, instance): + return "/users/%i" % instance.id + + group = Group(name='foo') + group.save() + + user = User.objects.create_user(username='bar', password='blah') + self.assertEquals(1, User.objects.count()) + self.assertEquals(0, user.groups.count()) + + form_data = { + 'password': 'baz', + 'groups': [group.id] + } + request = self.req.post('/users/' + str(user.pk), data=form_data) + cleaned_data = dict(form_data) + cleaned_data['groups'] = [group] + mixin = UpdateModelMixin() + mixin.resource = UserResource + mixin.CONTENT = cleaned_data + + mixin.request = request + mixin.args = () + mixin.kwargs = {'pk': user.pk} + + response = mixin.put(request, **mixin.kwargs) + self.assertEquals(1, User.objects.count()) + self.assertEquals(1, response.groups.count()) + self.assertEquals('foo', response.groups.all()[0].name) + self.assertEquals('bar', response.username) + + def test_update_with_m2m_relation_through(self): + """ + Tests updating where the m2m relation uses a through table + """ + class UserResource(ModelResource): + model = CustomUser + + def url(self, instance): + return "/customusers/%i" % instance.id + + user = CustomUser.objects.create(username='bar') + + # Update existing resource with empty relation + form_data = {'username': 'bar0', 'groups': []} + request = self.req.put('/users/' + str(user.pk), data=form_data) + cleaned_data = dict(form_data) + cleaned_data['groups'] = [] + mixin = UpdateModelMixin() + mixin.resource = UserResource + mixin.CONTENT = cleaned_data + + mixin.request = request + mixin.args = () + mixin.kwargs = {'pk': user.pk} + + response = mixin.put(request, **mixin.kwargs) + self.assertEquals(1, CustomUser.objects.count()) + self.assertEquals(0, response.groups.count()) + + # Update existing resource with one relation + group = Group(name='foo1') + group.save() + + form_data = {'username': 'bar1', 'groups': [group.id]} + request = self.req.put('/users/' + str(user.pk), data=form_data) + cleaned_data = dict(form_data) + cleaned_data['groups'] = [group] + mixin = UpdateModelMixin() + mixin.resource = UserResource + mixin.CONTENT = cleaned_data + + mixin.request = request + mixin.args = () + mixin.kwargs = {'pk': user.pk} + + response = mixin.put(request, **mixin.kwargs) + self.assertEquals(1, CustomUser.objects.count()) + self.assertEquals(1, response.groups.count()) + self.assertEquals('foo1', response.groups.all()[0].name) + + # Update existing resource with more than one relation + group2 = Group(name='foo2') + group2.save() + + form_data = {'username': 'bar2', 'groups': [group.id, group2.id]} + request = self.req.put('/users/' + str(user.pk), data=form_data) + cleaned_data = dict(form_data) + cleaned_data['groups'] = [group, group2] + mixin = UpdateModelMixin() + mixin.resource = UserResource + mixin.CONTENT = cleaned_data + + mixin.request = request + mixin.args = () + mixin.kwargs = {'pk': user.pk} + + response = mixin.put(request, **mixin.kwargs) + self.assertEquals(1, CustomUser.objects.count()) + self.assertEquals(2, response.groups.count()) + self.assertEquals('foo1', response.groups.all()[0].name) + self.assertEquals('foo2', response.groups.all()[1].name) + + class MockPaginatorView(PaginatorMixin, View): total = 60 From a07c61590580127e98344290e025cee20f67ff3b Mon Sep 17 00:00:00 2001 From: Mjumbe Wawatu Poe Date: Sat, 25 Aug 2012 20:16:10 -0400 Subject: [PATCH 7/8] Add `email` kwarg to create_user for Django <1.4 compatibility (thanks @travisbot !) --- djangorestframework/tests/mixins.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/djangorestframework/tests/mixins.py b/djangorestframework/tests/mixins.py index 315513b8c..e99409b0c 100644 --- a/djangorestframework/tests/mixins.py +++ b/djangorestframework/tests/mixins.py @@ -235,7 +235,7 @@ class TestModelUpdate(TestModelsTestCase): group = Group(name='foo') group.save() - user = User.objects.create_user(username='bar', password='blah') + user = User.objects.create_user(username='bar', password='blah', email="bar@example.com") self.assertEquals(1, User.objects.count()) self.assertEquals(0, user.groups.count()) From eb407374f0f2f1f71b9b1f465d9f76ebf7d8f713 Mon Sep 17 00:00:00 2001 From: Mjumbe Wawatu Poe Date: Tue, 28 Aug 2012 16:10:05 -0400 Subject: [PATCH 8/8] Don't pass an instance in to delete_instance, as it should be inferred --- djangorestframework/mixins.py | 2 +- djangorestframework/tests/mixins.py | 44 +++++++++++++++++++++++++++-- 2 files changed, 43 insertions(+), 3 deletions(-) diff --git a/djangorestframework/mixins.py b/djangorestframework/mixins.py index 8386b429e..90c883072 100644 --- a/djangorestframework/mixins.py +++ b/djangorestframework/mixins.py @@ -695,7 +695,7 @@ class DeleteModelMixin(ModelMixin, InstanceReaderMixin, InstanceWriterMixin): Behavior to delete a `model` instance on DELETE requests """ def delete(self, request, *args, **kwargs): - self.delete_instance(instance) + self.delete_instance() return diff --git a/djangorestframework/tests/mixins.py b/djangorestframework/tests/mixins.py index e99409b0c..2727b2c13 100644 --- a/djangorestframework/tests/mixins.py +++ b/djangorestframework/tests/mixins.py @@ -4,8 +4,9 @@ from django.utils import simplejson as json from djangorestframework import status from djangorestframework.compat import RequestFactory from django.contrib.auth.models import Group, User -from djangorestframework.mixins import (CreateModelMixin, PaginatorMixin, - ReadModelMixin, UpdateModelMixin) +from djangorestframework.mixins import (CreateModelMixin, DeleteModelMixin, + PaginatorMixin, ReadModelMixin, + UpdateModelMixin) from djangorestframework.resources import ModelResource from djangorestframework.response import Response, ErrorResponse from djangorestframework.tests.models import CustomUser @@ -333,6 +334,45 @@ class TestModelUpdate(TestModelsTestCase): self.assertEquals('foo2', response.groups.all()[1].name) +class TestModelDelete(TestModelsTestCase): + """Tests on DeleteModelMixin""" + + def setUp(self): + super(TestModelsTestCase, self).setUp() + self.req = RequestFactory() + + def test_delete(self): + group = Group.objects.create(name='my group') + + self.assertEquals(1, Group.objects.count()) + + class GroupResource(ModelResource): + model = Group + + # Delete existing + request = self.req.delete('/groups/' + str(group.pk)) + mixin = DeleteModelMixin() + mixin.resource = GroupResource + + mixin.request = request + mixin.args = () + mixin.kwargs = {'pk': group.pk} + + response = mixin.delete(request, **mixin.kwargs) + self.assertEquals(0, Group.objects.count()) + + # Delete at non-existing + request = self.req.delete('/groups/' + str(group.pk)) + mixin = DeleteModelMixin() + mixin.resource = GroupResource + + mixin.request = request + mixin.args = () + mixin.kwargs = {'pk': group.pk} + + self.assertRaises(ErrorResponse, mixin.delete, request, **mixin.kwargs) + + class MockPaginatorView(PaginatorMixin, View): total = 60