Opened 10 years ago

Closed 9 years ago

Last modified 6 years ago

#7512 closed (fixed)

including a nullable foreign key reference in Meta ordering has unexpected results

Reported by: R. Bailey <bailey@…> Owned by: Malcolm Tredinnick
Component: Database layer (models, ORM) Version: master
Severity: Keywords: 1.0-blocker
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: UI/UX:


In a model such as

model Stuff(models.Model):
  whatever = models.ForeignKey(Whatever, null=True)

class Meta:
   ordering = ['whatever__someattribute', ... ]

selecting via Stuff.objects.all will yield only those rows for which the foreign key reference is not null.

further, selecting via a Stuff.objects.get(pk=someval), where the corresponding row in Stuff has a null in the foreign key reference will yield a no such PK exception.

the cause is the inner join in the resulting query.

I don't know if I'd argue that either all() or get() should actually allow sorting by a nullable field, but I do think that the exception should be distinct in situations where a PK is missing from join results, and not from the table corresponding to the model being selected against.

Attachments (1)

null_fk_ordering_r7722.patch (8.3 KB) - added by George Vilches 10 years ago.
Fixes null ForeignKey ordering against r7722.

Download all attachments as: .zip

Change History (14)

comment:1 Changed 10 years ago by R. Bailey <bailey@…>

Summary: including a nullable field in Meta ordering has unexpected resultsincluding a nullable foreign key reference in Meta ordering has unexpected results

I just realized my summary was inaccurate- to be clear this isn't sorting on nullable fields, but rather nullable foreign key refs

comment:2 Changed 10 years ago by George Vilches

Owner: changed from nobody to George Vilches

comment:3 Changed 10 years ago by George Vilches

Component: Core frameworkDatabase wrapper
Has patch: set

Any time ordering occurs on null FKs, we have to force promote=True on django.db.models.sql.query.join() to get the LOUTER behavior. The attached patch does so against r7722, and updates some unit tests that are very old and were not designed for the behavior and results of null FKs.

Changed 10 years ago by George Vilches

Fixes null ForeignKey ordering against r7722.

comment:4 Changed 10 years ago by George Vilches

Note that this patch also includes a rather involved unit test to cover different nestings of FK=null behavior. There is no new unit test for M2M null behavior, as the updated unit tests from regressiontests.queries demonstrate the new behavior adequately.

comment:5 Changed 10 years ago by Eric Holscher

milestone: 1.0
Triage Stage: UnreviewedAccepted

comment:6 Changed 10 years ago by Malcolm Tredinnick

Patch needs improvement: set

This patch isn't correct. It will lead to far too many outer joins. It only addresses the situation where setup_joins() is called for ordering, but, much more importantly, setup_joins() is used by add_filter(). And when a filter is being incorporated that is not comparing against potentially NULL values, we should not be using outer joins. That is why there is a subsequent pass in add_filter() to do some join promotion if the comparison is against something that could be null, rather than doing that in setup_joins().

So the test case could well be useful, but the changes to setup_joins() are not appropriate.

comment:7 Changed 9 years ago by George Vilches

Well, besides add_filter, what other places should the promotion code be added? Anything related to ordering, if it orders across a null FK, would need the same handling code, correct?

The second test case in the attached patch still fails as of r8752.

Also, I'm not sure why this would create an outer join when add_filter() isn't comparing against null values. By adding a "print names" to the beginning of setup_joins(), I get this for the last unit test in the attached patch:

    [<Comment: Another first comment>, <Comment: My second comment>, <Comment: Another second comment>, <Comment: My first comment>]
    names ['post', 'forum', 'system_info', 'system_name']
    names ['comment_text']
    [<Comment: Another second comment>, <Comment: My first comment>]

Those names are the only ones being used by ordering or filtering, so it's not looking at all fields in the model, only the ones involved in the FK path. So how would it be promoting a query to an outer join that it shouldn't? If there's a null key in the relation at all, shouldn't it always be promoted to an outer join? Even if it's on a filter that's not explicitly requesting a __isnull type check, if it generates any relations, those should be pulled in as outer joins, not inner joins, no matter the case, I would think. There's never a reason that a FK(null=True) relationship should be pulled as an INNER JOIN that I can think of.

Is this still on slate for 1.0? It does break code with any reliance on FK(null=True) of a depth greater than one.

comment:8 Changed 9 years ago by Jacob

Keywords: 1.0-blocker added

comment:9 Changed 9 years ago by Malcolm Tredinnick

The fact that one of the tests in regressiontests/queries/ that was testing for the absence of outer joins had to be changed to show that there was a (completely unnecessary) outer join shows that this patch is doing something wrong. It's not that test that is wrong; it's the resulting query that is now wrong. We should never do outer joins unless they are required. They are much more expensive at the database side.

Unconditionally promoting joins just because the foreign key can be nullable is therefore not the way to do this, because you don't need an outer join unless you care about null values -- not the case when comparing across a nullable join to a non-null value.

comment:10 Changed 9 years ago by Malcolm Tredinnick

Owner: changed from George Vilches to Malcolm Tredinnick
Status: newassigned

comment:11 Changed 9 years ago by Malcolm Tredinnick

So once I started looking at this, it was looking familiar. Then I realised I had mostly fixed it in r7761 (see #7181). I just missed one portion that George's test here shows up: chained models with a nullable join leading to a non-nullable join. So I've added the test (thanks!) and the two line fix.

comment:12 Changed 9 years ago by Malcolm Tredinnick

Resolution: fixed
Status: assignedclosed

(In [8783]) Fixed #7512 -- Fixed an oversight when I first fixed ordering on nullable
foreign keys (r7761). Thanks to George Vilches for the test case here.

comment:13 Changed 6 years ago by Jacob

milestone: 1.0

Milestone 1.0 deleted

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