Opened 6 years ago

Closed 2 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 6 years ago.
bugfix and test case
0001-Fixes-13903.diff (5.7 KB) - added by henriquebastos 6 years ago.

Download all attachments as: .zip

Change History (13)

Changed 6 years ago by petraszd

bugfix and test case

comment:1 Changed 6 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Owner changed from nobody to anonymous
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

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

Changed 6 years ago by henriquebastos

comment:2 Changed 6 years ago by henriquebastos

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Has patch set

Adding improved patch with tests.

comment:3 Changed 6 years ago by henriquebastos

  • Triage Stage changed from Accepted to Ready for checkin

comment:4 Changed 6 years ago by russellm

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to 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 Changed 6 years ago by henriquebastos

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 6 years ago by russellm

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 6 years ago by henriquebastos

Agreed. But as IndexError?

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

comment:8 Changed 5 years ago by graham_king

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:9 Changed 5 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 5 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:11 Changed 2 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from assigned to 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.

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