Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#4144 closed (fixed)

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

Reported by: Marty Alchin <gulopine@…> Owned by: Adrian Holovaty
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@…> 10 years ago.
Just moves the one line inside the if block
base.py.2.diff (769 bytes) - added by Marty Alchin <gulopine@…> 10 years ago.
Proper diff, against revision 5065
dynamic_models.diff (10.5 KB) - added by Marty Alchin <gulopine@…> 10 years ago.
Some basic tests for dynamic models

Download all attachments as: .zip

Change History (17)

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

Attachment: base.py.diff added

Just moves the one line inside the if block

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

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 10 years ago by Marty Alchin <gulopine@…>

Attachment: base.py.2.diff added

Proper diff, against revision 5065

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

Summary: [patch] ModelBase should only check `sys.modules` when it needs toModelBase should only check `sys.modules` when it needs to

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

Triage Stage: UnreviewedDesign decision needed

Do we need tests for this?

comment:4 Changed 10 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 10 years ago by Marty Alchin <gulopine@…>

Attachment: dynamic_models.diff added

Some basic tests for dynamic models

comment:5 Changed 10 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 10 years ago by Simon G. <dev@…>

Triage Stage: Design decision neededReady for checkin

comment:7 Changed 10 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 10 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 10 years ago by Malcolm Tredinnick

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 10 years ago by Malcolm Tredinnick

Resolution: fixed
Status: newclosed

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

comment:11 Changed 10 years ago by Malcolm Tredinnick

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

comment:12 Changed 10 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 10 years ago by Malcolm Tredinnick

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 10 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