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

[petraszd]

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.

[petraszd]

bugfix and test case

[anonymous]

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

[Henrique Bastos]

Adding improved patch with tests.

[Russell Keith-Magee]

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.

[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?

[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.

[Henrique Bastos]

Agreed. But as IndexError?

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

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Aymeric Augustin]

Change Easy pickings from NULL to False.

[Aymeric Augustin]

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.