Opened 4 years ago

Closed 4 years ago

Last modified 4 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 4 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 4 years ago by Aymeric Augustin

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Severity: NormalRelease blocker

comment:2 Changed 4 years ago by Anssi Kääriäinen

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 Changed 4 years ago by Aymeric Augustin

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 Changed 4 years ago by Anssi Kääriäinen

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 Changed 4 years ago by Aymeric Augustin

Right, that makes sense.

comment:6 Changed 4 years ago by Anssi Kääriäinen

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

comment:7 Changed 4 years ago by Michael Manfre

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

comment:8 Changed 4 years ago by Anssi Kääriäinen

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 Changed 4 years ago by Simon Charette

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

I'll tackle it.

comment:10 Changed 4 years ago by Simon Charette

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 Changed 4 years ago by Anssi Kääriäinen

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 Changed 4 years ago by Anssi Kääriäinen <akaariai@…>

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 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

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 Changed 4 years ago by Anssi Kääriäinen <akaariai@…>

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 Changed 4 years ago by Aymeric Augustin <aymeric.augustin@…>

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 Changed 4 years ago by Anssi Kääriäinen

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