Opened 14 years ago
Closed 10 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: | dev |
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 |
Description
Pseudo file structure:
./project somefile.py /app1 models.py
somefile.py has abstract models.Model subclass
models.py uses class from somefile.py
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)
Change History (13)
by , 14 years ago
Attachment: | patch.diff added |
---|
comment:1 by , 14 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Triage Stage: | Unreviewed → Accepted |
the patch looks good, but theres a typo in the comment, and the doctest needs to be a unit test.
by , 14 years ago
Attachment: | 0001-Fixes-13903.diff added |
---|
comment:2 by , 14 years ago
Component: | Uncategorized → Database layer (models, ORM) |
---|---|
Has patch: | set |
Adding improved patch with tests.
comment:3 by , 14 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:4 by , 14 years ago
Patch needs improvement: | set |
---|---|
Triage Stage: | Ready for checkin → Accepted |
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 by , 14 years ago
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 by , 14 years ago
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 by , 14 years ago
Agreed. But as IndexError?
What about RuntimeError with a custom message: "Error detecting model's app_label."
comment:8 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Cleanup/optimization |
comment:11 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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.
bugfix and test case