Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#32285 closed Cleanup/optimization (fixed)

AppConfig.label should raise an exception when it's not a valid Python identifier.

Reported by: Federico Capoano Owned by: Hasan Ramezani
Component: Core (Other) Version: 3.1
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

Follow up to mailing list discussion.

Setting the app_label of an AppConfig object is allowed and fails later on during migrations (./manage.py migrate) with (too many values to unpack).
It took me a couple of minutes to figure out what was wrong, but it may not be so obvious to an inexperienced user so it may be better to raise a clearer exception.

Maybe we can use system checks and fail earlier if such a problem is detected?

Change History (11)

comment:1 by Mariusz Felisiak, 3 years ago

Component: Core (System checks)Core (Other)
Resolution: needsinfo
Status: newclosed
Summary: AppConfig: app_label with dot should raise exceptionAppConfig.label should raise an exception when it's not a valid Python identifier.
Type: BugCleanup/optimization

Raising ImproperlyConfigured should be enough, however everything works for me with dotted label, e.g. ticket_32285.test_one. Can you provide a sample project?

comment:2 by Mariusz Felisiak, 3 years ago

Resolution: needsinfo
Status: closednew
Triage Stage: UnreviewedAccepted

It looks that it will be good to raise an error to avoid confusions (see #32287), even if I cannot reproduce this crash.

comment:3 by Hasan Ramezani, 3 years ago

Owner: changed from nobody to Hasan Ramezani
Status: newassigned

comment:4 by Federico Capoano, 3 years ago

Have you already tried to create a new django app, put a dot in the app_label field of the AppConfig, ensure the AppConfig is mentioned in the init.py, then create a model and run ./manage.py makemigrations?

in reply to:  4 comment:5 by Mariusz Felisiak, 3 years ago

Replying to Federico Capoano:

Have you already tried to create a new django app, put a dot in the app_label field of the AppConfig, ensure the AppConfig is mentioned in the init.py, then create a model and run ./manage.py makemigrations?

Yes. Can you share a stacktrace? I'm not even sure in which line or file you encountered this issue 🤔

comment:6 by Mariusz Felisiak, 3 years ago

Has patch: set
Patch needs improvement: set

comment:7 by Hasan Ramezani, 3 years ago

Patch needs improvement: unset

comment:8 by Mariusz Felisiak, 3 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

In 110001d:

Refs #32285 -- Made AppConfigStub do not call super().init().

Calling super().init() is unnecessary and enforces the use of
various workarounds.

comment:10 by Mariusz Felisiak <felisiak.mariusz@…>, 3 years ago

Resolution: fixed
Status: assignedclosed

In 8b2a30f:

Fixed #32285 -- Raised ImproperlyConfigured when AppConfig.label is not a valid Python identifier.

comment:11 by Simon Weber, 3 years ago

I was surprised by this error when trying to upgrade a large codebase to 3.2. This codebase's convention is to use app labels with dashes, which I suspect still works fine (unlike labels with periods).

Is there a recommended migration path for projects like these that were using an invalid label unknowingly? As far as I can tell, the only option is to change the label, which looks involved: it affects things like old migrations, db rows like content types, and maybe even table names.

If there's no simple migration path, what about changing this from an exception to a warning? That'd still accomplish the goal of informing users that something may break and allows cases like mine to avoid quite a bit of pain.

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