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)
Change History (39)
by , 16 years ago
comment:1 by , 16 years ago
Component: | Uncategorized → Forms |
---|
comment:2 by , 16 years ago
Cc: | added |
---|
comment:3 by , 16 years ago
Cc: | added |
---|
comment:4 by , 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 , 16 years ago
Resolution: | → worksforme |
---|---|
Status: | new → 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.
comment:6 by , 16 years ago
Resolution: | worksforme |
---|---|
Status: | closed → 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.
comment:7 by , 16 years ago
Cc: | 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 , 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 , 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 , 16 years ago
#9144 looks like another report of this, with some additional debug info.
comment:11 by , 16 years ago
This issue is tied closely to #9006.
As pointed out in it ordering just based on id works.
comment:12 by , 16 years ago
Cc: | added |
---|
comment:13 by , 16 years ago
Cc: | 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 , 16 years ago
Cc: | 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 , 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 , 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 , 16 years ago
Attachment: | inline_queryset.diff added |
---|
Use the same QuerySet for all forms in the FormSet
comment:17 by , 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 , 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 , 16 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:20 by , 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 , 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 , 16 years ago
Attachment: | 9076.2.diff added |
---|
comment:22 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → 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 by , 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 , 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 , 16 years ago
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.
follow-up: 29 comment:28 by , 16 years ago
Cc: | 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 by , 15 years ago
Resolution: | fixed |
---|---|
Status: | closed → 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 by , 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:33 by , 12 years ago
Status: | reopened → new |
---|
comment:35 by , 3 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Closing as fixed, unless someone can provide a reproducible scenario.
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...