#21681 closed Cleanup/optimization (fixed)

Simplify Apps.populate_models

Reported by: aaugustin Owned by: nobody
Component: Database layer (models, ORM) Version: master
Severity: Normal Keywords: app-loading
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

There are two highly dubious things in the current implementation of the app registry (which date back to a refactoring by Malcolm 7 years ago):

1) The "postponing" behavior which is designed to avoid circular imports when populate_models() is triggered during the import of a models module. Depending on the implementation of #21676, that may become unnecessary. ptone once suggested that #18251 removed the need for this behavior, but I'm not sure why.

2) The three uses of get_registered_model. It looks like it's optional. But ModelBase.new is scary...

Change History (9)

comment:1 Changed 19 months ago by aaugustin

  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 19 months ago by aaugustin

Another related issue: the current implementation of populate_models() isn't exactly idempotent. A recursive invocation may return while models aren't fully populated, and it won't set _models_loaded. This could probably be an issue in some cases, but it would be moot if we got rid of the postponing.

comment:4 Changed 19 months ago by aaugustin

Regarding the 2nd point, here's an... interesting... behavior.

# myapp/models.py

from django.db import models

class MyModel(models.Model):
    pass

first_model_class = MyModel  # save a reference

class MyModel(models.Model):
    pass

second_model_class = MyModel  # save a reference

if second_model_class is first_model_class:
    raise RuntimeError("WTF")

Thank you https://github.com/django/django/blob/efddae25/django/db/models/base.py#L153-L156.

comment:5 Changed 19 months ago by aaugustin

This behavior was added without a test in c63dcdda372d4d8a37abd39432deb62b8d4b1d23 to fix #1796.

While the example I'm showing is obviously an unintended consequence, we should proceed carefully.

comment:6 Changed 19 months ago by aaugustin

  • Version changed from 1.6 to master

comment:7 Changed 19 months ago by Aymeric Augustin <aymeric.augustin@…>

In 966de8497373dc47756105516b4b839734ed316e:

Removed postponing in Apps.populate_models.

To the best of my understanding, since populate_models() is now called
as soon as Django starts, it cannot be called while a models module is
being imported, and that removes the need for postponing.

(If hell breaks loose we'll revert this commit.)

Refs #21681.

comment:8 Changed 19 months ago by Aymeric Augustin <aymeric.augustin@…>

In 1c242a297b5b1857d76cab9b24f9f1d3b7f5240d:

Merged Apps.populate_apps() and populate_models().

After the recent series of refactorings, there's no reason to keep
two distinct methods.

Refs #21681.

comment:9 Changed 19 months ago by aaugustin

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

This issue is fixed by the two commits above. The part about ModelBase was moved to #21711.

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