Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#19652 closed Bug (fixed)

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

Reported by: charettes Owned by: charettes
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 charettes 2 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 2 years ago by aaugustin

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Severity changed from Normal to Release blocker

comment:2 Changed 2 years ago by akaariai

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 2 years ago by aaugustin

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 2 years ago by akaariai

  • Triage Stage changed from Unreviewed to Accepted

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 2 years ago by aaugustin

Right, that makes sense.

comment:6 Changed 2 years ago by akaariai

  • Owner changed from nobody to akaariai
  • Status changed from new to assigned

comment:7 Changed 2 years ago by 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 2 years ago by akaariai

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 2 years ago by charettes

  • Owner changed from akaariai to charettes

I'll tackle it.

comment:10 Changed 2 years ago by charettes

  • 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 2 years ago by akaariai

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

  • Resolution set to fixed
  • Status changed from assigned to closed

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 2 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 2 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 2 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 2 years ago by akaariai

  • Component changed from ORM aggregation to Database layer (models, ORM)
Note: See TracTickets for help on using tickets.
Back to Top