#11319: ForeignKey filters use the wrong field to prepare values for database
---------------------------------------------------+------------------------
Reporter: russellm | Owner: carljm
Status: new | Milestone: 1.3
Component: Database layer (models, ORM) | Version: 1.0
Resolution: | Keywords:
Stage: Accepted | Has_patch: 1
Needs_docs: 0 | Needs_tests: 0
Needs_better_patch: 1 |
---------------------------------------------------+------------------------
Comment (by carljm):
The change in query.py may indeed be wrong, but if so it's not because it
needs to be broader; rather because it (arguably?) shouldn't be there at
all.
The "to_field" argument to a ForeignKey generally only applies to the
target model of that FK, obviously: if you're traversing that same
relationship in the reverse direction, the lookup should just use the
primary key of the model on which the FK is defined.
The problem is that the same RelatedField (ForeignKey or OneToOneField)
object is asked to "get_prep_lookup" the lookup value in either case,
regardless of whether the lookup is taking place in a forwards or reverse
direction across that RelatedField. And currently, it's asked to do so
without ever being informed which type of lookup is happening!
I guard the to_field lookup in RelatedField._pk_trace with an isinstance
check, to make sure the value is actually an instance of the model the FK
points to (or a subclass of it). In most cases, this is enough to prevent
use of to_field on a reverse FK traversal, because normally the two sides
of an FK are two different model classes. But in the case of a recursive
FK that uses to_field, both sides of the FK are the same model class, so
the isinstance check passes either way, and to_field is used on the
reverse lookup too.
The change I'd made in query.py worked around this specific case by also
using the to_field column name rather than the PK column name in the
recursive reverse lookup case. Then it's ok that _pk_trace doesn't know
the difference and preps the to_field value in either case.
If you expect recursive FKs to work just like other FKs, this is the wrong
fix; the to_field should only be used for lookups in the forwards
direction. If you expect to_field to be used for lookups in both
directions on a recursive FK (only), because in that specific case it can,
it might be the right fix. I'm not actually sure how to choose between
those - either one should result in a correct query.
I've now updated my branch with the alternative fix, which requires
significantly more invasive changes, because the knowledge of whether it's
a forward or reverse lookup currently exists only in Query.setup_joins(),
and has to be passed through the Constraint object in order to get to
RelatedField.get_prep_lookup(). It also requires adding a new
get_prep_lookup_reverse() method to Fields, in order to avoid requiring
all custom Field classes to update their get_prep_lookup() methods with a
new parameter.
I've run this fix through the same tests as above (full test suite on
SQLite and Postgres, "queries" and "delete_regress" on MySQL, ISAM and
InnoDB), and everything passes this way as well.
I think a case can be made for either version of the fix.
The case for the first fix is that it's much less invasive, and it's not
unreasonable to apply to_field in lookups both directions on a recursive
FK; with the same model on each end, there's no reason to_field can't work
just fine both directions.
The case for the second fix is twofold: 1) That it really seems like
RelatedField _ought_ to know whether it's prepping a value for a forward
or reverse lookup, so perhaps the invasiveness is justified, and 2) That
the behavior of ForeignKey.to_field shouldn't change just because its a
recursive FK.
There is one concrete difference between the two fixes. It changes whether
you need to use the PK value or the to_field value if specifying the value
directly in the lookup, rather than providing an object. In that sense,
the first (less-invasive) fix is backwards-incompatible for anyone
currently working around this bug on a recursive FK by providing using
"=obj.pk" instead of "=obj" in their lookup; they'd have to switch to
"=obj.to_field_name" (or just switch to "=obj").
Sorry for the novel - thoughts on this?
--
Ticket URL: <http://code.djangoproject.com/ticket/11319#comment:15>
Django <http://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
--
You received this message because you are subscribed to the Google Groups
"Django updates" group.
To post to this group, send email to django-updates@googlegroups.com.
To unsubscribe from this group, send email to
django-updates+unsubscr...@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/django-updates?hl=en.
