Django

Code

Ticket #7512 (closed: fixed)

Opened 6 months ago

Last modified 3 months ago

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

Reported by: R. Bailey <bailey@akamai.com> Assigned to: mtredinnick
Milestone: 1.0 Component: Database layer (models, ORM)
Version: SVN Keywords: 1.0-blocker
Cc: Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 1

Description

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

null_fk_ordering_r7722.patch (8.3 kB) - added by gav on 06/23/08 00:50:45.
Fixes null ForeignKey? ordering against r7722.

Change History

06/20/08 09:24:41 changed by R. Bailey <bailey@akamai.com>

  • needs_better_patch changed.
  • summary changed from including a nullable field in Meta ordering has unexpected results to including a nullable foreign key reference in Meta ordering has unexpected results.
  • needs_tests changed.
  • needs_docs changed.

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

06/22/08 23:59:49 changed by gav

  • owner changed from nobody to gav.

06/23/08 00:50:18 changed by gav

  • has_patch set to 1.
  • component changed from Core framework to Database wrapper.

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.

06/23/08 00:50:45 changed by gav

  • attachment null_fk_ordering_r7722.patch added.

Fixes null ForeignKey? ordering against r7722.

06/23/08 00:51:47 changed by gav

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.

08/08/08 14:23:17 changed by ericholscher

  • stage changed from Unreviewed to Accepted.
  • milestone set to 1.0.

08/20/08 23:42:48 changed by mtredinnick

  • needs_better_patch set to 1.

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.

08/30/08 22:24:45 changed by gav

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.

08/31/08 15:46:41 changed by jacob

  • keywords set to 1.0-blocker.

08/31/08 21:08:19 changed by mtredinnick

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.

08/31/08 21:08:28 changed by mtredinnick

  • owner changed from gav to mtredinnick.
  • status changed from new to assigned.

08/31/08 21:40:15 changed by mtredinnick

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.

08/31/08 21:43:56 changed by mtredinnick

  • status changed from assigned to closed.
  • resolution set to fixed.

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

Add/Change #7512 (including a nullable foreign key reference in Meta ordering has unexpected results)




Change Properties
Action