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)

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

Download all attachments as: .zip

Change History (13)

by petraszd, 14 years ago

Attachment: patch.diff added

bugfix and test case

comment:1 by anonymous, 14 years ago

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.

by Henrique Bastos, 14 years ago

Attachment: 0001-Fixes-13903.diff added

comment:2 by Henrique Bastos, 14 years ago

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

Adding improved patch with tests.

comment:3 by Henrique Bastos, 14 years ago

Triage Stage: AcceptedReady for checkin

comment:4 by Russell Keith-Magee, 14 years ago

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 by Henrique Bastos, 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 Russell Keith-Magee, 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 Henrique Bastos, 14 years ago

Agreed. But as IndexError?

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

comment:8 by Graham King, 14 years ago

Severity: Normal
Type: Cleanup/optimization

comment:9 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:10 by Aymeric Augustin, 13 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:11 by Aymeric Augustin, 10 years ago

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