Opened 6 years ago

Last modified 2 years ago

#9076 new Bug

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@… 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 6 years ago.
model2.py (1.1 KB) - added by Leonidas 6 years ago.
The model which shows this error too
admin2.py (426 bytes) - added by Leonidas 6 years ago.
The admin interface settings
9076.diff (505 bytes) - added by Leonidas 6 years ago.
A workaround that makes the validation succeed.
inline_queryset.diff (1.7 KB) - added by bthomas 6 years ago.
Use the same QuerySet for all forms in the FormSet
9076.2.diff (1.3 KB) - added by kmtracey 6 years ago.

Download all attachments as: .zip

Change History (39)

Changed 6 years ago by coady

comment:1 Changed 6 years ago by kratorius

  • Component changed from Uncategorized to Forms
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 6 years ago by Leonidas

  • Cc marek@… added

comment:3 Changed 6 years ago by Jonas

  • 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 Changed 6 years ago by Jonas

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 Changed 6 years ago by kratorius

  • Resolution set to worksforme
  • Status changed from new to closed

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.

Changed 6 years ago by Leonidas

The model which shows this error too

Changed 6 years ago by Leonidas

The admin interface settings

comment:6 Changed 6 years ago by Leonidas

  • Resolution worksforme deleted
  • Status changed from closed to reopened

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.

Changed 6 years ago by Leonidas

A workaround that makes the validation succeed.

comment:7 Changed 6 years ago by bthomas

  • 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 Changed 6 years ago by Jonas

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 Changed 6 years ago by Jonas

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 Changed 6 years ago by kmtracey

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

comment:11 Changed 6 years ago by anonymous

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

comment:12 Changed 6 years ago by aptiko

  • Cc anthony@… added

comment:13 Changed 6 years ago by jashugan

  • 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 Changed 6 years ago by kmishler

  • 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 Changed 6 years ago by john

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 Changed 6 years ago by kmtracey

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.)

Changed 6 years ago by bthomas

Use the same QuerySet for all forms in the FormSet

comment:17 Changed 6 years ago by bthomas

  • 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 Changed 6 years ago by kmtracey

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 Changed 6 years ago by kmtracey

  • Triage Stage changed from Unreviewed to Accepted

comment:20 Changed 6 years ago by kmtracey

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 Changed 6 years ago by bthomas

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.

Changed 6 years ago by kmtracey

comment:22 Changed 6 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from reopened to closed

(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 Changed 6 years ago by kmtracey

(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 Changed 6 years ago by carljm

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 Changed 6 years ago by kmtracey

I think remaining instances of this problem may be due to something I mentioned in the last comment I added to #9578 -- 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. #9578 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.

comment:26 Changed 6 years ago by carljm

I don't think you mean #9578 here, but I can't find the one you're referring to. Explanation makes sense, though.

comment:27 Changed 6 years ago by kmtracey

You're right, I meant #9758. Darn typslexia.

comment:28 follow-up: Changed 6 years ago by leitjohn

  • 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.

comment:29 in reply to: ↑ 28 Changed 5 years ago by tumma72

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 Changed 4 years ago by julien

  • Has patch unset
  • Severity set to Normal
  • Type set to Bug

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

comment:31 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:32 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:33 Changed 2 years ago by aaugustin

  • Status changed from reopened to new
Note: See TracTickets for help on using tickets.
Back to Top