Non-manager instance assigned to model class' objects is silently ignored

[Shai Berger]

Consider this models file:

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:

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.

[Carlton Gibson]

Hi Shai.

I think we might be able to do something here…

If you get into ​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.

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

[Shai Berger]

We'll see what CI says, but on my machine, the ​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.

[Shai Berger]

For the record, CI agrees with my machine. It's the same two tests that fail.

[Shai Berger]

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.

[Carlton Gibson]

OK, after ​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.)