Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fixed #22046 -- Unhelpful queryset handling for model formsets with data... #2277

Closed
wants to merge 1 commit into from

2 participants

@dgym

....

BaseModelFormSet now overrides the queryset if it is bound.
It pulls in the instances specified in the data.

Two unit tests showing the previous problems are included.

@dgym dgym Fixed #22046 -- Unhelpful queryset handling for model formsets with d…
…ata.

BaseModelFormSet now overrides the queryset if it is bound.
It pulls in the instances specified in the data.

Two unit tests showing the previous problems are included.
cf31bba
@timgraham
Owner

It seems to me if queryset=Author.objects.filter(pk=10), then specifying an object with ID=20 shouldn't pass validation. Does it?

@dgym

Currently formset's is_valid() "Returns True if every form in self.forms is valid." (this is its doc string). There is nothing that checks that the ids in the forms matches up with the objects returned by the formset's queryset. The formset currently passes validation in such a case.

I feel that whether or not such a form should pass validation is no longer really relevant. Before this patch it may well have been preferable to implement such checks. This would enforce correctness, but the burden would be on the end programmer to set up the formset correctly everywhere one is used to save data. This is non-trivial, requiring more than a little thought and code to get the behaviour that is so obviously desirable.

However, with this patch the necessary logic is bought into formset itself. The case you described above will still pass validation, but for the (in my opinion) very good reason that the queryset you specified is ignored. If you try and process an object with ID=20 in a formset the formset now correctly operates on precisely that object, and doesn't make you jump through hoops getting your specified queryset right when it can just do that for you.

The intent is clear. Currently the programmer can get it wrong and end up with bad data. Adding extra checks would only punish the programmer for getting it wrong and waste their time trying to get it right. Making the system get it right automatically must surely be preferable.

@timgraham
Owner

buildbot, test this please.

@timgraham
Owner

While the security model for formsets is currently somewhat ill-defined, I don't think a security model of "if you expose a formset for a model class, every instance of that model class is available for editing by anyone with access to POST to that formset" is what we want.

@timgraham timgraham closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 14, 2014
  1. @dgym

    Fixed #22046 -- Unhelpful queryset handling for model formsets with d…

    dgym authored
    …ata.
    
    BaseModelFormSet now overrides the queryset if it is bound.
    It pulls in the instances specified in the data.
    
    Two unit tests showing the previous problems are included.
This page is out of date. Refresh to see the latest.
Showing with 62 additions and 1 deletion.
  1. +21 −1 django/forms/models.py
  2. +41 −0 tests/model_formsets/tests.py
View
22 django/forms/models.py
@@ -593,9 +593,29 @@ def _construct_form(self, i, **kwargs):
pass
return super(BaseModelFormSet, self)._construct_form(i, **kwargs)
+ def _get_data_pks(self):
+ """Returns a list of all the pks in the form data."""
+ pks = []
+ pk_field = self.model._meta.pk
+ to_python = self._get_to_python(pk_field)
+ for i in range(self.initial_form_count()):
+ pk_key = "%s-%s" % (self.add_prefix(i), pk_field.name)
+ pk = self.data[pk_key]
+ pks.append(to_python(pk))
+ return pks
+
def get_queryset(self):
if not hasattr(self, '_queryset'):
- if self.queryset is not None:
+ if self.is_bound:
+ # Bound forms must only deal with the instances
+ # specified by the data. Allowing any other filters
+ # could cause the data specified instances to not be
+ # found.
+ qs = self.model._default_manager.get_queryset()
+ qs = qs.filter(**{
+ self.model._meta.pk.name+'__in': self._get_data_pks(),
+ })
+ elif self.queryset is not None:
qs = self.queryset
else:
qs = self.model._default_manager.get_queryset()
View
41 tests/model_formsets/tests.py
@@ -1183,6 +1183,47 @@ def test_model_formset_with_initial_queryset(self):
formset = FormSet(initial=[{'authors': Author.objects.all()}], data=data)
self.assertFalse(formset.extra_forms[0].has_changed())
+ def test_model_formset_with_mismatched_queryset(self):
+ FormSet = modelformset_factory(Author, fields=('name', ))
+ Author.objects.create(pk=10, name='Charles Baudelaire')
+ Author.objects.create(pk=20, name='Arthur Rimbaud')
+ data = {
+ 'form-TOTAL_FORMS': 1,
+ 'form-INITIAL_FORMS': 1,
+ 'form-MAX_NUM_FORMS': 1,
+ 'form-0-id': '20',
+ 'form-0-name': 'A. Rimbaud',
+ }
+ formset = FormSet(data=data, queryset=Author.objects.filter(pk=10))
+ formset.save()
+
+ self.assertEqual(
+ set(Author.objects.values_list('pk', 'name')),
+ set([
+ (10, 'Charles Baudelaire'),
+ (20, 'A. Rimbaud'),
+ ]),
+ )
+
+ def test_model_formset_limits_queryset_to_data(self):
+ FormSet = modelformset_factory(Author, fields=('name', ))
+ Author.objects.create(pk=10, name='Charles Baudelaire')
+ Author.objects.create(pk=20, name='Arthur Rimbaud')
+ data = {
+ 'form-TOTAL_FORMS': 1,
+ 'form-INITIAL_FORMS': 1,
+ 'form-MAX_NUM_FORMS': 1,
+ 'form-0-id': '20',
+ 'form-0-name': 'A. Rimbaud',
+ }
+ formset = FormSet(data=data, queryset=Author.objects.filter(pk=10))
+ self.assertEqual(
+ set(formset.get_queryset().values_list('pk', 'name')),
+ set([
+ (20, 'Arthur Rimbaud'),
+ ]),
+ )
+
def test_prevent_duplicates_from_with_the_same_formset(self):
FormSet = modelformset_factory(Product, fields="__all__", extra=2)
data = {
Something went wrong with that request. Please try again.