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