[Django] #32640: Non-manager instance assigned to model class' objects is silently ignored
Django
-------------------------------------+-------------------------------------
Reporter: Shai | Owner: nobody
Berger |
Type: Bug | Status: new
Component: Database | Version: 2.2
layer (models, ORM) |
Severity: Normal | Keywords:
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
Consider this models file:
{{{#!python
from django.db import models
class MyManager(models.Manager):
def get_queryset(self):
return self.none()
class MyModel(models.Model):
one = models.IntegerField()
objects = MyManager # Note: Missing parentheses
}}}
The assignment of a manager class, rather than manager instance, is an
easy mistake to make. I know, because I've made it. In a private
discussion, Carlton Gibson suggested another variant of this:
{{{#!python
class MyOtherModel(models.Model):
...
objects = models.Manager.from_queryset(SomeQueryset())
# The above, too, is missing a pair of parentheses
}}}
But what should Django do?
There's two possible behaviors I would consider reasonable:
- Trust the user, and rely on Duck Typing. This would blow up very fast in
this case, as any method invocation would be missing the {{{self}}}
argument.
- Raise an error on model creation, or at least in {{{check}}}.
What Django does instead, is ignore the assignment and use a
{{{models.Manager}}} instance.
Two points to clarify that this is indeed a bug:
- Assigning any object which isn't a {{{models.Manager}}} instance -- say,
the number 17 -- gets the same result.
- This only happens if only one manager is defined. If another manager is
defined, the assignment stands as written (the first "reasonable" behavior
described above).
I found this on 2.2, it is still this way on master.
--
Ticket URL: <https://code.djangoproject.com/ticket/32640>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
Hi Shai.
I think we might be able to do **something** here…
If you get into
[https://github.com/django/django/blob/3b8527e32b665df91622649550813bb1ec9a9251/django/db/models/base.py#L357-L365
this block where the `objects` manager is automatically added]:
{{{
if not opts.managers:
if any(f.name == 'objects' for f in opts.fields):
raise ValueError(
"Model %s must specify a custom Manager, because it
has a "
"field named 'objects'." % cls.__name__
)
manager = Manager()
manager.auto_created = True
cls.add_to_class('objects', manager)
}}}
…then it seems that any value of `objects` is an error 🤔 — but
particularly if `objects` is a `Manager` **class**.
{{{
>>> from django.db import models
>>> class MyModel(models.Model):
... objects = "not-a-manager"
... class Meta:
... app_label = "testing"
...
>>> MyModel.objects
<django.db.models.manager.Manager object at 0x1020f7520>
}}}
I'd think we could at least warn there, but arguably even raise.
Not sure how far down this road it would be worth going.
--
Ticket URL: <https://code.djangoproject.com/ticket/32640#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Shai Berger):
Hmmm. That makes me wonder if the real change we need to make is in
{{{add_to_class()}}} -- make it not override existing attributes so
carelessly. That will fix this issue, and maybe some others like it; I
only wonder what it will break.
I'll try to make a PR for that later.
--
Ticket URL: <https://code.djangoproject.com/ticket/32640#comment:2>
Django
-------------------------------------+-------------------------------------
| Berger
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Shai Berger
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
* status: new => assigned
Comment:
We'll see what CI says, but on my machine, the
[https://github.com/django/django/pull/14260 WIP PR] only seems to break
tests for invalid models, by making them "more invalid" (fail to create
the class instead of failing a check).
I'm not sure if that's a bad thing.
--
Ticket URL: <https://code.djangoproject.com/ticket/32640#comment:3>
Django
-------------------------------------+-------------------------------------
| Berger
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Shai Berger):
For the record, CI agrees with my machine. It's the same two tests that
fail.
--
Ticket URL: <https://code.djangoproject.com/ticket/32640#comment:4>
Django
-------------------------------------+-------------------------------------
| Berger
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32640#comment:5>
Django
-------------------------------------+-------------------------------------
| Berger
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/32640#comment:6>
Django
-------------------------------------+-------------------------------------
| Berger
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/32640#comment:7>
Django
-------------------------------------+-------------------------------------
| Berger
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 1 => 0
* needs_tests: 1 => 0
Comment:
The flags "needs improvement" and "needs tests" were set on this ticket
with vague comments on the PR which basically say the same as the flags,
with no more details. A request for clarification has been left unanswered
for two months. Hence, I'm putting this back on the review queue.
--
Ticket URL: <https://code.djangoproject.com/ticket/32640#comment:8>
Django
-------------------------------------+-------------------------------------
| Berger
Type: Bug | Status: assigned
Component: Database layer | Version: 2.2
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
OK, after
[https://github.com/django/django/pull/14260#pullrequestreview-742687431
looking at the PR], I'm going to mark this as Patch needs improvement
again. (It may be we uncheck as the discussion on the PR continues.)
In summary, two main points from the review:
* The proposal to `add_to_class()` effects all passes through that, rather
than just those where an attribute is added late, and may override an
existing attribute. Making the change means we end with different error
paths for the case on the concrete instance vs inheritance, and the
existing system checks for that seem more tightly targeted for that.
* A specific error raised at the point where the default objects manager
is added would be able to offer a much better error message, whilst
avoiding the above. (A subsidiary System Check could alert to
`extra_manager` cases.)
--
Ticket URL: <https://code.djangoproject.com/ticket/32640#comment:9>