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

[R. Bailey <bailey@…>]

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.

[R. Bailey <bailey@…>]

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

[George Vilches]

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.

[George Vilches]

Fixes null ForeignKey ordering against r7722.

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

[Malcolm Tredinnick]

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.

[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:

Expected:
    [<Comment: Another first comment>, <Comment: My second comment>, <Comment: Another second comment>, <Comment: My first comment>]
Got:
    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.

[Malcolm Tredinnick]

The fact that one of the tests in regressiontests/queries/models.py 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.

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

[Malcolm Tredinnick]

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

[Jacob]

Milestone 1.0 deleted