Opened 8 years ago

Closed 4 years ago

#13903 closed Cleanup/optimization (fixed)

Auto app_label creation fails when model is defined at top-level python modules

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


Pseudo file structure:

   /app1 has abstract models.Model subclass uses class from

It throws an Exception, because of the way ModelBase class calculates app_label:

kwargs = {"app_label": model_module.__name__.split('.')[-2]}

module is at top level so it has no -2 index fails.

I understand that there so requirements where models should be defined -- but django should not fail (with strange IndexError) too.

I am adding fix and test case.

Attachments (2)

patch.diff (1.7 KB) - added by petraszd 8 years ago.
bugfix and test case
0001-Fixes-13903.diff (5.7 KB) - added by Henrique Bastos 7 years ago.

Download all attachments as: .zip

Change History (13)

Changed 8 years ago by petraszd

Attachment: patch.diff added

bugfix and test case

comment:1 Changed 8 years ago by anonymous

Owner: changed from nobody to anonymous
Status: newassigned
Triage Stage: UnreviewedAccepted

the patch looks good, but theres a typo in the comment, and the doctest needs to be a unit test.

Changed 7 years ago by Henrique Bastos

Attachment: 0001-Fixes-13903.diff added

comment:2 Changed 7 years ago by Henrique Bastos

Component: UncategorizedDatabase layer (models, ORM)
Has patch: set

Adding improved patch with tests.

comment:3 Changed 7 years ago by Henrique Bastos

Triage Stage: AcceptedReady for checkin

comment:4 Changed 7 years ago by Russell Keith-Magee

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Agreed that Django shouldn't be failing with an IndexError, but I'm not sure I agree that base models should be allowed to live anywhere they want.

Also - procedurally - you shouldn't be marking your own patch as Ready For Checkin.

comment:5 Changed 7 years ago by Henrique Bastos

Sorry about the mistaken status change.

So we have to problems here:

1) Allow or not this behavior.
2) Fix the IndexError.

On the 1st, if someone really want's to work that way, he could define app_label on the model's class Meta. So no patch would be needed.

On the 2nd, it would be really easy to change the attached patch to fix the IndexError. But what would be the proper behavior? If the model isn't on the proper place, it would not crash, but wouldn't work either. How to proceed?

I think at least we could have a specific exception message advising on setting the app_label to clear the obscurity. What do you think?

comment:6 Changed 7 years ago by Russell Keith-Magee

app_label is a really nasty hack. Ideally, it wouldn't be required at all. It was introduced as a workaround for certain nested model module situations.

I don't think we should be encouraging it's use for situations like the one you describe. To my mind, a model should belong to an app. If a model isn't in an app, that's an error, and should be raised as such, early and loudly.

comment:7 Changed 7 years ago by Henrique Bastos

Agreed. But as IndexError?

What about RuntimeError with a custom message: "Error detecting model's app_label."

comment:8 Changed 7 years ago by Graham King

Severity: Normal
Type: Cleanup/optimization

comment:9 Changed 6 years ago by Aymeric Augustin

UI/UX: unset

Change UI/UX from NULL to False.

comment:10 Changed 6 years ago by Aymeric Augustin

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:11 Changed 4 years ago by Aymeric Augustin

Resolution: fixed
Status: assignedclosed

The old logic for guessing app_label was deprecated in favor of a more robust alternative during the app-loading refactor.

The commit that fixed this particular issue is c40209dcc09f19524fb85251f39a4051491bbec0.

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