From 60cd5363ab1343260c4ed8322d2d26064d56c3d3 Mon Sep 17 00:00:00 2001 From: Fernando Zunino Date: Fri, 1 Jul 2011 03:32:04 -0300 Subject: [PATCH] FIX: ModelViews can have Resources whose models have unique fields. ReadModelMixin and UpdateModelMixin store model instance as a property. This allows ModelResource to bind the ModelForm using the model instance making the form validate the input data against the model instance and not a brand new instance. When the latter happened and the model used unique fields, the form validation failed whenever a PUT was maintaining the previuos value of the unique field. --- djangorestframework/mixins.py | 20 ++++++------ djangorestframework/resources.py | 52 ++++++++++++++++++-------------- 2 files changed, 40 insertions(+), 32 deletions(-) diff --git a/djangorestframework/mixins.py b/djangorestframework/mixins.py index 1b3aa241c..3d74ae191 100644 --- a/djangorestframework/mixins.py +++ b/djangorestframework/mixins.py @@ -491,17 +491,17 @@ class ReadModelMixin(object): 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) + 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] - instance = model.objects.get(**filtered_keywords) + self.model_instance = model.objects.get(**filtered_keywords) except model.DoesNotExist: raise ErrorResponse(status.HTTP_404_NOT_FOUND) - return instance + return self.model_instance class CreateModelMixin(object): @@ -540,19 +540,19 @@ class UpdateModelMixin(object): 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) + self.model_instance = model.objects.get(pk=args[-1], **kwargs) else: # Otherwise assume the kwargs uniquely identify the model - instance = model.objects.get(**kwargs) + self.model_instance = model.objects.get(**kwargs) for (key, val) in self.CONTENT.items(): - setattr(instance, key, val) + setattr(self.model_instance, key, val) except model.DoesNotExist: - instance = model(**self.CONTENT) - instance.save() + self.model_instance = model(**self.CONTENT) + self.model_instance.save() - instance.save() - return instance + self.model_instance.save() + return self.model_instance class DeleteModelMixin(object): diff --git a/djangorestframework/resources.py b/djangorestframework/resources.py index b42bd9528..be361ab80 100644 --- a/djangorestframework/resources.py +++ b/djangorestframework/resources.py @@ -177,14 +177,12 @@ class FormResource(Resource): # Return HTTP 400 response (BAD REQUEST) raise ErrorResponse(400, detail) - - def get_bound_form(self, data=None, files=None, method=None): - """ - Given some content return a Django form bound to that content. - If form validation is turned off (:attr:`form` class attribute is :const:`None`) then returns :const:`None`. - """ + def get_form_class(self, method=None): + """ + Returns the form class used to validate this resource. + """ # A form on the view overrides a form on the resource. form = getattr(self.view, 'form', None) or self.form @@ -200,6 +198,16 @@ class FormResource(Resource): form = getattr(self, '%s_form' % method.lower(), form) form = getattr(self.view, '%s_form' % method.lower(), form) + return form + + + def get_bound_form(self, data=None, files=None, method=None): + """ + Given some content return a Django form bound to that content. + If form validation is turned off (:attr:`form` class attribute is :const:`None`) then returns :const:`None`. + """ + form = self.get_form_class(method) + if not form: return None @@ -306,31 +314,31 @@ class ModelResource(FormResource): If the :attr:`form` class attribute has been explicitly set then that class will be used to create the Form, otherwise the model will be used to create a ModelForm. """ + form = self.get_form_class(method) - form = super(ModelResource, self).get_bound_form(data, files, method=method) - - # Use an explict Form if it exists - if form: - return form - - elif self.model: + if not form and self.model: # Fall back to ModelForm which we create on the fly class OnTheFlyModelForm(forms.ModelForm): class Meta: model = self.model #fields = tuple(self._model_fields_set) - # Instantiate the ModelForm as appropriate - if data and isinstance(data, models.Model): - # Bound to an existing model instance - return OnTheFlyModelForm(instance=content) - elif data is not None: - return OnTheFlyModelForm(data, files) - return OnTheFlyModelForm() + form = OnTheFlyModelForm # Both form and model not set? Okay bruv, whatevs... - return None - + if not form: + return None + + # Instantiate the ModelForm as appropriate + if data is not None or files is not None: + if issubclass(form, forms.ModelForm) and hasattr(self.view, 'model_instance'): + # Bound to an existing model instance + return form(data, files, instance=self.view.model_instance) + else: + return form(data, files) + + return form() + def url(self, instance): """