Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#19652 closed Bug (fixed)

Fix for #19524 introduced a backward compatiblity issue with related managers on 1.5.X

Reported by: Simon Charette Owned by: Simon Charette
Component: Database layer (models, ORM) Version: 1.5-beta-1
Severity: Release blocker Keywords: none EmptyQuerySet none
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The fix for #19524 which was backported to the 1.5.X branch caused the following regression:

class ObjectQuerySet(models.query.QuerySet):
    def extra_qs_method(self):
        pass


class ObjectManager(models.Manager):
    use_for_related_fields = True

    def get_query_set(self):
        return ObjectQuerySet(self.model, using=self._db)


class RelatedObject(models.Model):
    pass


class Object(models.Model):
    related = models.ForeignKey(RelatedObject, related_name='objs')

    objects = ObjectManager()


RelatedObject().objs.extra_qs_method()

Raises

AttributeError: 'EmptyQuerySet' object has no attribute 'extra_qs_method'

It works perfectly on master since QuerySet.none() returns an instance of the correct class while setting it's underlying's query to empty and on < 1.5.X since prior to this backport there was no instance.pk check to return an EmptyQuerySet.

Attachments (1)

0001-Fixed-19652-Fixed-a-regression-introduced-by-9d6d0de.patch (3.0 KB ) - added by Simon Charette 11 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 by Aymeric Augustin, 11 years ago

Severity: NormalRelease blocker

comment:2 by Anssi Kääriäinen, 11 years ago

The likely fix is to do queryset.filter(pk=None) instead of queryset.none(). User visible effect is the same as what is now done in master.

comment:3 by Aymeric Augustin, 11 years ago

Isn't it technically possible to have a nullable custom foreign key field? (There could be at most one object with a null pk.)

comment:4 by Anssi Kääriäinen, 11 years ago

Triage Stage: UnreviewedAccepted

A pk can't be NULL, but if you use to_field, then you could have NULLs on both sides. We definitely want the semantics of SQL's JOIN ON (lhs.fk_val = rhs.to_field). This means that if either/both of those are NULL, then the result is "no match". (SQL's funny trinary boolean logic, NULL = NULL => UNKNOWN.)

If we go an try to change this we will have endless amount of problems.

comment:5 by Aymeric Augustin, 11 years ago

Right, that makes sense.

comment:6 by Anssi Kääriäinen, 11 years ago

Owner: changed from nobody to Anssi Kääriäinen
Status: newassigned

comment:7 by Michael Manfre, 11 years ago

To avoid the NULL = NULL issue, you could just go with a more explicit queryset.filter(Q(pk=1) & ~Q(pk=1)).

comment:8 by Anssi Kääriäinen, 11 years ago

The correct approach seems to be to filter using pk__in=[] which ensures no query is executed.

I am too tired to write a patch today. If anybody else wants to tackle this I won't object...

comment:9 by Simon Charette, 11 years ago

Owner: changed from Anssi Kääriäinen to Simon Charette

I'll tackle it.

comment:10 by Simon Charette, 11 years ago

Has patch: set

Added a patch based on stable/1.5.x with fix and failing testcase. It looks like the regression was introduced by the fix for #17541 after all.

I don't know if we should also add the tests to the master branch.

comment:11 by Anssi Kääriäinen, 11 years ago

Every use of .none() pre-1.6 will cause this same bug. This is a "feature" of using .none(), and the reason .none() was changed in 1.6. Still, I think it is a good idea to fix this one as this is a regression. Existing code will fail, and the fix is trivial.

I will commit the patch to 1.5.

comment:12 by Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In f4132140f52c88b67d11743d4062a9d455959ffc:

[1.5.x] Fixed #19652 -- Fixed .none() regression in related fields

The regression was caused by using .none() when querying for related
models, and the origin field's value was None. This resulted in missing
custom related manager subclass as .none() returns plain QuerySet.

This isn't backport from master, in master .none() correctly preserves
the queryset's class.

Patch provided by Simon Charette, with some minor polish by committer.

comment:13 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 9328ef0e84db2559af56d24c40e6b24b74e29a6f:

[1.5.x] Avoided a possible regression in 5097d3c5.

QuerySet.none() returns an instance of EmptyQuerySet, which may have
undesirable side effects in the presence of custom query set classes.

The implementation of .none() was refactored in master to have the same
effect as .filter(pkin=[]).

Refs #19652.

Thanks Simon Charrette for the report.

comment:14 by Anssi Kääriäinen <akaariai@…>, 11 years ago

In f4132140f52c88b67d11743d4062a9d455959ffc:

[1.5.x] Fixed #19652 -- Fixed .none() regression in related fields

The regression was caused by using .none() when querying for related
models, and the origin field's value was None. This resulted in missing
custom related manager subclass as .none() returns plain QuerySet.

This isn't backport from master, in master .none() correctly preserves
the queryset's class.

Patch provided by Simon Charette, with some minor polish by committer.

comment:15 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 9328ef0e84db2559af56d24c40e6b24b74e29a6f:

[1.5.x] Avoided a possible regression in 5097d3c5.

QuerySet.none() returns an instance of EmptyQuerySet, which may have
undesirable side effects in the presence of custom query set classes.

The implementation of .none() was refactored in master to have the same
effect as .filter(pkin=[]).

Refs #19652.

Thanks Simon Charrette for the report.

comment:16 by Anssi Kääriäinen, 11 years ago

Component: ORM aggregationDatabase layer (models, ORM)
Note: See TracTickets for help on using tickets.
Back to Top