Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4144 closed (fixed)

ModelBase should only check `sys.modules` when it needs to

Reported by: Marty Alchin <gulopine@…> Owned by: adrian
Component: Database layer (models, ORM) Version: master
Severity: Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Currently, ModelBase.__new__ retrieves a model's module through sys.modules, in order to detect the model's app_label. However, this check is made even if the model provides an app_label on its Meta class. Since retrieval of app_label is the only use of this check, I've attached a patch that moves it into the if block, so it only executes if app_label was not provided in the model's Meta.

For what it's worth, this is important for a project of mine, as I'm hoping to generate models during runtime, rather than through Python source. I'm looking to design models interactively, even load them with data and test out relationships. Sort of a rapid-prototyping for Django, capable of being used even by non-coders, since it wouldn't rely on typing any Python code.

I know this is very much a corner case, but the fact remains that this check is really only used within that if statement, and it seems it could just as easily go there without any trouble.

Attachments (3)

base.py.diff (910 bytes) - added by Marty Alchin <gulopine@…> 8 years ago.
Just moves the one line inside the if block
base.py.2.diff (769 bytes) - added by Marty Alchin <gulopine@…> 8 years ago.
Proper diff, against revision 5065
dynamic_models.diff (10.5 KB) - added by Marty Alchin <gulopine@…> 8 years ago.
Some basic tests for dynamic models

Download all attachments as: .zip

Change History (17)

Changed 8 years ago by Marty Alchin <gulopine@…>

Just moves the one line inside the if block

comment:1 Changed 8 years ago by Marty Alchin <gulopine@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

That's the last time I create a diff by hand because I don't have proper diff tools here at work. I'll fix it when I get home.

Changed 8 years ago by Marty Alchin <gulopine@…>

Proper diff, against revision 5065

comment:2 Changed 8 years ago by Collin Grady <cgrady@…>

  • Summary changed from [patch] ModelBase should only check `sys.modules` when it needs to to ModelBase should only check `sys.modules` when it needs to

comment:3 Changed 8 years ago by Simon G. <dev@…>

  • Triage Stage changed from Unreviewed to Design decision needed

Do we need tests for this?

comment:4 Changed 8 years ago by Marty Alchin <gulopine@…>

  • Needs tests set

I expect the existing tests should adequately ensure that this patch doesn't break any existing functionality. However, as for the new options it would enable, it probably would be best to write some new tests for dynamic model creation.

Since this is ultimately a new way of creating models, the test suite would need to cover as many aspects of model creation as possible, even though much of it will essentially duplicate existing tests. I've started on a suite, and I'll get it up as soon as I can.

Changed 8 years ago by Marty Alchin <gulopine@…>

Some basic tests for dynamic models

comment:5 Changed 8 years ago by Marty Alchin <gulopine@…>

  • Needs tests unset

Attached are some tests for creating, installing and using models dynamically at run-time. They fail without this patch, but will pass beautifully with it. I'm sure the tests don't cover as many aspects of model creation as would be prudent, so I'd appreciate somebody else looking it over to see what I missed.

comment:6 Changed 8 years ago by Simon G. <dev@…>

  • Triage Stage changed from Design decision needed to Ready for checkin

comment:7 Changed 8 years ago by Marty Alchin <gulopine@…>

The future of this ticket may well depend on the fate of #3591, see my comments there. The tests provided here would still be useful though, whichever way this ends up being handled.

comment:8 Changed 8 years ago by Vinay Sajip <vinay_sajip@…>

In my latest working copy of base.py, the model_module line is gone completely. We'll wait to see what feedback comes on the #3591 patch.

comment:9 Changed 8 years ago by mtredinnick

I'm going to leave the tests out of this commit. Creating dynamic models is very much an edge-case and not something we're likely to put a lot of effort into supporting. Happy to make the suggested change, but adding all the overhead (setup and tear-down of the databases, etc) required for another test file doesn't feel justified here.

comment:10 Changed 8 years ago by mtredinnick

  • Resolution set to fixed
  • Status changed from new to closed

(In [5163]) Fixed #4144 -- Only check sys.modules when required during Model class
construction. Patch from Marty Alchin.

comment:11 Changed 8 years ago by mtredinnick

(In [5164]) Omitted AUTHORS change from last [5163]. Refs #4144.

comment:12 Changed 8 years ago by Marty Alchin <gulopine@…>

I'm perfectly happy with that decision on the tests. They were at least sufficient to prove (for the case of this ticket anyway) what was capable with this patch. Thanks for working so quickly!

comment:13 Changed 8 years ago by mtredinnick

Marty: if you feel keen, it might be worth creating a wiki page describing the process in those tests. It would be a shame to completely lose the information.

comment:14 Changed 8 years ago by Marty Alchin <gulopine@…>

I've just created DynamicModels. Hopefully it'll help somebody out there.

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