Opened 16 years ago

Closed 3 years ago

#9076 closed Bug (fixed)

Inline forms can result in a "Please correct the errors below." message with no errors listed.

Reported by: coady Owned by: nobody
Component: Forms Version: 1.0
Severity: Normal Keywords:
Cc: marek@…, django@…, bthomas@…, anthony@…, jashugan@…, kmishler@…, leitjohn@…, desecho@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This error can reproduced with the poll app in the tutorial with a minor change. Remove the choice field from the Choice model (file attached). Edit a poll by adding two choices; enter any numbers in the votes fields. Any subsequent save will result in the "Please correct the errors below." message with no highlighted errors.

The problem occurs in BaseModelFormSet._construct_form where an index access on the queryset is returning duplicate pks. This in turn leads to validation failing in BaseModelForm.validate_unique. The error occurs in 1.0 and the trunk, probably introduced around the time core=True was removed.

Attachments (6)

models.py (553 bytes ) - added by coady 16 years ago.
model2.py (1.1 KB ) - added by Leonidas 16 years ago.
The model which shows this error too
admin2.py (426 bytes ) - added by Leonidas 16 years ago.
The admin interface settings
9076.diff (505 bytes ) - added by Leonidas 16 years ago.
A workaround that makes the validation succeed.
inline_queryset.diff (1.7 KB ) - added by Bob Thomas 16 years ago.
Use the same QuerySet for all forms in the FormSet
9076.2.diff (1.3 KB ) - added by Karen Tracey 16 years ago.

Download all attachments as: .zip

Change History (39)

by coady, 16 years ago

Attachment: models.py added

comment:1 by Ivan Giuliani, 16 years ago

Component: UncategorizedForms

comment:2 by Leonidas, 16 years ago

Cc: marek@… added

comment:3 by Jonas von Poser, 16 years ago

Cc: django@… added

I can confirm this bug and will try to find a fix ASAP. It's 11pm over here and our site is supposed to go live tomorrow morning...

comment:4 by Jonas von Poser, 16 years ago

Ok, because of time constraints we simply changed the affected pages directly in the database. If anybody finds a patch for this bug, I'd be *very* grateful.

comment:5 by Ivan Giuliani, 16 years ago

Resolution: worksforme
Status: newclosed

I can't reproduce the error. Remember that you can't just comment the choice field out, you need to update your database schema as well. If you're still getting this error, please reopen this ticket providing as much information as possible.

by Leonidas, 16 years ago

Attachment: model2.py added

The model which shows this error too

by Leonidas, 16 years ago

Attachment: admin2.py added

The admin interface settings

comment:6 by Leonidas, 16 years ago

Resolution: worksforme
Status: closedreopened

I can reproduce this error very well, though it only happens some times. Be sure to create some objects to test them with, because when there is only a few objects it works. It also works when editing stuff for the first time, but then it stops working. Modifying the models via the ORM API works without a problem, exactly like Jonas says.

This is clearly a bug in the validation. I'm also attaching a workaround that coady sent me on the django-users mailinglist which makes this problem go away.

by Leonidas, 16 years ago

Attachment: 9076.diff added

A workaround that makes the validation succeed.

comment:7 by Bob Thomas, 16 years ago

Cc: bthomas@… added

I just ran into this issue as well. I noticed the SQL for getting the inline models (SELECT * from table where parent_id = 123 LIMIT 1 OFFSET x) apparently gives inconsistent results on PostgreSQL with default ordering in some cases. I set an ordering on my model and the problem went away.

The admin site probably needs to evaluate a queryset for the inlines, and pass that to the formset to prevent it from creating a new queryset for each inline and indexing into it. (Side note: it takes a long time to load admin pages that have several hundred hundred inline models. Yes, I probably shouldn't do that, but I don't think Django should run a separate query for each one :p)

comment:8 by Jonas von Poser, 16 years ago

So far I confirmed that behaviour on Postgres, running off SQLite with the exact same data doesn't throw this error. I also noticed long loading times for the inline models, even for just 5-10 objects, but haven't looked into it yet.

comment:9 by Jonas von Poser, 16 years ago

Ok, even after spending some time with pdb I haven't been able to find the source of the error. Will try some more later. I can confirm the workaround though: setting an unique ordering makes the error go away.

This means, when using e.g. ordering=('order',) on the inline model, with order being the same in several inline instances, it does not work. After setting ordering=('order','id') on the inline model it works again.

comment:10 by Karen Tracey, 16 years ago

#9144 looks like another report of this, with some additional debug info.

comment:11 by anonymous, 16 years ago

This issue is tied closely to #9006.
As pointed out in it ordering just based on id works.

comment:12 by Antonis Christofides, 16 years ago

Cc: anthony@… added

comment:13 by jashugan, 16 years ago

Cc: jashugan@… added

I had this issue as well. As bthomas pointed out, adding some type of ordering to the model makes it work.

comment:14 by kmishler, 16 years ago

Cc: kmishler@… added

We had this problem too and fixed it by adding ordering to the model. Of course it only happened in production, and not in our development or test environments.

comment:15 by John Hensley, 16 years ago

I think anonymous is right and this is the same problem as in #9006. I attached a patch to that ticket that applies a default ordering to querysets if necessary before slicing or indexing them.

comment:16 by Karen Tracey, 16 years ago

Even if this is the same root problem as #9006, it would be nice if the admin didn't display a message telling one to correct the errors below when there are no visible errors below. So I'm inclined to leave this open to fix that problem. (Not that I've looked at a line of code involved here to see how difficult that might be, or how likely it may be to run into this situation with some other underlying error...I just have a feeling it would be better if the admin didn't do that.)

by Bob Thomas, 16 years ago

Attachment: inline_queryset.diff added

Use the same QuerySet for all forms in the FormSet

comment:17 by Bob Thomas, 16 years ago

Has patch: set

Attached a possible fix for this. BaseInlineFormSet was overriding get_queryset, and always returning a new QuerySet instead of caching it like BaseModelFormSet does. I fixed this to pass that QuerySet to the super() call. Not only does this seem to fix the issue, but it can reduce the number of database queries run by 1 query per inline model instance, which can be significant if you have a lot of children for a model. My next project is figuring out how they can all share querysets for their ModelChoiceFields...

I think #9006 can be fixed independently of this. With my patch, the queries run do not use LIMIT/OFFSET anymore.

comment:18 by Karen Tracey, 16 years ago

I'm inclined to check inline_queryset.diff (or a slight variant) in but I'd like some feedback from Joseph and/or Brian first, because the patch undoes part of [7270]. That is, older code used to do what the patch does, but it was changed at one point not to, so I'd like to understand why before just restoring the old behavior.

Prior to [7270], (Base)InlineFormSet __init__ called its get_queryset() method and passed the result in as the qs arg for the superclass init. [7270] removed the call to get_queryset() and also removed the parameter from the superclass init. [7270] also did a lot of other stuff so it's not clear the motivation for this specific part of the change.

An (undesirable) effect is that the queryset caching built in to the superclass' get_queryset() is no longer used. As a result an individual database query is made for each inline element in the set (first limit 1, then offset 1 limit 1, then offset 2 limit 1, etc). Not only does this add a lot of extra queries but apparently #9006 also means sometimes that sequence of queries doesn't actually return all the items, but rather some duplicates, which causes the invisible error.

So, going back to the old way seems like a good idea but I thought I'd check first if anyone could remember why the code that used to do things this way was changed? I'd prefer to not re-introduce a previously fixed bug but rather come up with a solution that fixes both problems...if I knew what the first problem was.

Also, as it is the patch exposes queryset as an arg to BaseInlineFormSet. That doesn't seem necessary to fix the bug and wasn't in the old code so I don't see why it should be done?

comment:19 by Karen Tracey, 16 years ago

Triage Stage: UnreviewedAccepted

comment:20 by Karen Tracey, 16 years ago

The note above is not the whole story...the 'caching' of the queryset by get_queryset() in BaseModelFormSet was never used in old (pre-[7270]) code by BaseInlineFormSet because first it wasn't there until [8805] and second BaseInlineFormSet has always over-ridden get_queryset() so the superclass method was never called for a BaseInlineFormset anyway. So I think the change in [7270] was a general cleanup to not not provide a parameter to the superclass init that wasn't doing anything -- since the subclass had its own get_queryset().

The problem here was introduced in [8805], when the need for caching the queryset was introduced by the addition of the _construct_form method that contains a loop for initial_form_count that contains within it self.get_queryset()[i]. That's what's causing the limit/offset queries. At the same time BaseModelFormSet's get_queryset() was modified to cache the queryset in _queryset, but this isn't used by a BaseInlineFormSet because it has its own get_queryset() method.

It doesn't look to me like there is any good reason for BaseInlnieFormSet to have it's own get_queryset() instead of just computing it during init, passing it in as a parameter to the superclass init, and then letting the superclass get_querset() do its thing. Besides the _queryset caching, BaseInlineFormSet's method is also missing the bit about limiting the query to max_num. So even though only max_num inlines are displayed in the admin, the actual database query is not limited.

So I'm even more inclined to check in a slight variant on the inline_queryset.diff patch (I still don't see why queryset should be exposed as a param to BaseInlineFormSet) barring feedback to the contrary.

comment:21 by Bob Thomas, 16 years ago

Sorry, I didn't mean to cause controversy over the extra init parameter. I think I was just trying to match the signature of BaseModelFormSet. It's not really part of the fix, so feel free to exclude it.

by Karen Tracey, 16 years ago

Attachment: 9076.2.diff added

comment:22 by Karen Tracey, 16 years ago

Resolution: fixed
Status: reopenedclosed

(In [9440]) Fixed #9076 -- Changed BaseInlineFormSet to not override BaseModelFormSet's get_queryset method. BaseInlineFormSet's method did not include a couple of fixes/enhancements that were made to the parent's method, resulting in excessive queries (some of which can return bad data due to #9006) for admin pages with inlines.

comment:23 by Karen Tracey, 16 years ago

(In [9441]) [1.0.X] Fixed #9076 -- Changed BaseInlineFormSet to not override BaseModelFormSet's get_queryset method. BaseInlineFormSet's method did not include a couple of fixes/enhancements that were made to the parent's method, resulting in excessive queries (some of which can return bad data due to #9006) for admin pages with inlines. Thanks bthomas for the initial patch.

r9440 from trunk.

comment:24 by Carl Meyer, 16 years ago

The problem described here still occurs in r9526 of the Django 1.0.x branch (after the above fix). An inline formset of models with no default ordering will sporadically result in mysterious "Please correct the below error(s)" error message. Adding a default ordering fixes the problem.

#9006 is tracking the core issue at the ORM level, but I believe this ticket should be reopened, as the same symptom is still present at the formset level.

comment:25 by Karen Tracey, 16 years ago

I think remaining instances of this problem may be due to something I mentioned in the last comment I added to #9758. -- if the queryset used during POST differs from the one used during GET to create the formset (even just in ordering), then POST data may not be correctly matched up to existing DB instances and formset validation may fail. #9758. is still open so I don't think this needs to be re-opened at this point. I'll add a note there that even changes in ordering can cause a problem.

Last edited 9 years ago by Tim Graham (previous) (diff)

comment:28 by leitjohn, 15 years ago

Cc: leitjohn@… added

I've been having this problem still even at revision [10865], but got it to work after i changed the ordering:

    class Meta:
        verbose_name_plural = "Task Statuses"
        ordering = ("date_completed",)
        #ordering = ["task__phase__order"]

Task and Phase are related objects. I don't think that the ordering plays nice with them.

in reply to:  28 comment:29 by tumma72, 15 years ago

Resolution: fixed
Status: closedreopened

Replying to leitjohn:

I've been having this problem still even at revision [10865], but got it to work after i changed the ordering:

    class Meta:
        verbose_name_plural = "Task Statuses"
        ordering = ("date_completed",)
        #ordering = ["task__phase__order"]

Task and Phase are related objects. I don't think that the ordering plays nice with them.

I am running 1.1.1 now, and in Inline models I still have the same behavior. The first time I change something... works, any other time the Error message comes out and there is no field highlighted. If I remove the inline and create a normal page works, but it is really annoying.

comment:30 by Julien Phalip, 14 years ago

Has patch: unset
Severity: Normal
Type: Bug

Unchecking the "Has patch" flag since there effectively is no patch for the new issues reported after this ticket was "fixed".

comment:31 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:32 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:33 by Aymeric Augustin, 12 years ago

Status: reopenednew

comment:34 by Anton Samarchyan, 8 years ago

Cc: desecho@… added

I can't reproduce the bug on master.

comment:35 by Mariusz Felisiak, 3 years ago

Resolution: fixed
Status: newclosed

Closing as fixed, unless someone can provide a reproducible scenario.

Note: See TracTickets for help on using tickets.
Back to Top