Opened 3 years ago

Closed 2 years ago

Last modified 22 months ago

#21794 closed Cleanup/optimization (fixed)

No warning should be raised when defining an abstract model with no app_label

Reported by: Simon Charette Owned by: Aymeric Augustin
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: app-loading
Cc: Aymeric Augustin Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Since those models are never registered and meant to be used by different apps it doesn't make much sense to raise a warning in this case.

I think this case was just overlooked when Aymeric refactored app-loading.

Attaching a patch fixing the issue and removing an extra space between in and INSTALLED_APPS in the warning message.

Attachments (2)

0001-Don-t-warn-about-missing-app_label-for-abstract-mode.patch (1.7 KB) - added by Simon Charette 3 years ago.
21794.patch (1.4 KB) - added by Aymeric Augustin 3 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 3 years ago by Aymeric Augustin

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

comment:2 Changed 3 years ago by Aymeric Augustin

This deprecation aims at getting rid of the legacy "look one level up from the package or module named 'models'" behavior. (In the face of ambiguity, refuse the temptation to guess.)

app_label is primarily used to determine which application the model should be attached to in the app registry. Since abstract models aren't registered, they may work without one, although I'm not sure if there are other consequences.

When this logic goes away, what app_label do we set for abstract models that aren't in an installed app and don't have an explicit app_label? Options has a default value of None. I assume we can use that.

Changed 3 years ago by Aymeric Augustin

Attachment: 21794.patch added

comment:3 Changed 3 years ago by Aymeric Augustin

Easy pickings: unset
Triage Stage: UnreviewedAccepted

I'm attaching a patch that keeps a warning for abstract models (because the behavior will change) but uses a different message.

comment:4 Changed 3 years ago by Simon Charette

Triage Stage: AcceptedReady for checkin

comment:5 Changed 3 years ago by Aymeric Augustin <aymeric.augustin@…>

Resolution: fixed
Status: assignedclosed

In 2f65b8e14c03a6b43c11d5de791b8d4d91721777:

Fixed #21794 -- Adjusted warning for abstract models.

As far as I can tell, they don't need an app_label.

Thanks Simon Charette for the report and the review.

comment:6 Changed 3 years ago by Aymeric Augustin <aymeric.augustin@…>

In 4a488c14833000eaf5d2b4c2787b6a61c99a4f53:

Silenced a spurious warning.

Thanks Timo for the report. Regression from 2f65b8e1. Refs #21794.

comment:7 Changed 3 years ago by Aymeric Augustin

In fact, this problem existed before the app-loading refactor, see #8108.

comment:8 Changed 2 years ago by Greg Brown

I'm getting this warning when inheriting from an abstract base class in Django 1.7, but if I set app_label = None in my abstract base class, it causes another error, because all the model classes inheriting from it now think they are in an app labelled None. Does this mean that as of Django 1.9 *all* model classes will need to have app_label explicitly set? If not I don't think there should be any warning for an abstract base class in this case, because there's no solution.

I may have misunderstood something here, so please feel free to set me straight!

comment:9 Changed 2 years ago by Greg Brown

Resolution: fixed
Status: closednew

Hi, I'm going to reopen this because I'm pretty confident the issue is still valid. At the moment I'm getting screeds of RemovedInDjango19 warnings and have no way to turn them off. It doesn't make sense for abstract models to have an app_label, because by their very nature they can be used in multiple apps.

comment:10 Changed 2 years ago by Greg Brown

Alternately, the warning could stay in place but the current behaviour where app_label = None is inherited from the abstract model's Meta class would have to change.

comment:11 Changed 2 years ago by Collin Anderson

Actually, could you re-close this ticket and open a new one? Your issue is related to this one but I think it's a slightly different issue. Or, better yet, bring up your issue on one of the mailing lists first.

Last edited 2 years ago by Collin Anderson (previous) (diff)

comment:12 Changed 2 years ago by Greg Brown

Hi Collin, the original submitted patch would have solved my problem, so I don't really think it's a separate issue - I will do as you suggest though and bring it up on django-developers.

Thanks,
Greg

comment:13 Changed 2 years ago by Tim Graham

Triage Stage: Ready for checkinAccepted

Loic's comment on the commit also hasn't been addressed.

comment:14 Changed 2 years ago by Tim Graham

Cc: Aymeric Augustin added

Aymeric, any thoughts here?

comment:15 Changed 2 years ago by Greg Brown

Loic said

Should this message be a little more explicit about what's necessary to fix this deprecation warning?
Usually this kind of message implies that if you manually do what the future version of Django will
do automatically, then you'll get rid of the deprecation warning, but obviously setting app_label to
None has no effect.

My point is that ther isn't anything that can be done to fix this deprecation warning, so personally I think the
original patch – which removes it – does address Loic's concerns, and should be fast-tracked
into a release.

At the moment I'm just ignoring all warnings because most of them are related to this (and hence can't be fixed.)

comment:16 Changed 2 years ago by Carl Meyer

Even though there is a behavior change here (from setting app_label to something probably-bogus, to setting it to None), it is a change that quite likely won't affect anybody. And I don't think we should raise pending-deprecation warnings that don't have a straightforward update path to get rid of them.

I would say this is like any other minor backwards-incompatible change to a semi-internal detail (which is arguably a bugfix anyway, as the currently-set value is bogus): we should just do it (that is, leave app_label as None for abstract models that aren't in an installed app and don't have it set explicitly), note it in the release notes, and be done.

comment:17 Changed 2 years ago by Aymeric Augustin

Yes, let's just remove the warning for abstract models and hope that doesn't break too much code when we release 1.9.

comment:18 Changed 2 years ago by Carl Meyer <carl@…>

Resolution: fixed
Status: newclosed

In e7b9a58b081299b30f807d5c66f7a5d1940efe4c:

Fixed #21794 -- Removed deprecation warning for abstract models outside an app.

comment:19 Changed 2 years ago by Carl Meyer <carl@…>

In ac359dc7710ed8c62b6f058fe5a0ecfd6ce27602:

[1.7.x] Fixed #21794 -- Removed deprecation warning for abstract models outside an app.

Backport of e7b9a58b081299b30f807d5c66f7a5d1940efe4c from master.

comment:20 Changed 22 months ago by Aymeric Augustin <aymeric.augustin@…>

In 1b8af4cfa023161924a45fb572399d2f3071bf5b:

Disallowed importing concrete models without an application.

Removed fragile algorithm to find which application a model belongs to.

Fixed #21680, #21719. Refs #21794.

Note: See TracTickets for help on using tickets.
Back to Top