Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

Simplified the implementation of register_model.

register_model is called exactly once in the entire Django code base, at the
bottom of ModelBase.__new__:

    new_class._meta.apps.register_model(new_class._meta.app_label, new_class)

ModelBase.__new__ exits prematurely 120 lines earlier (sigh) if a model with
the same name is already registered:

    if new_class._meta.apps.get_registered_model(new_class._meta.app_label, name):
        return

(This isn't the exact code, but it's equivalent.)

apps.register_model and apps.get_registered_model are essentially a setter and
a getter for apps.all_models, and apps.register_model is the only setter. As a
consequence, new_class._meta.apps.all_models cannot change in-between.

Considering that name == new_class.__name__, we can conclude that
register_model(app_label, model) is always called with such arguments that
get_registered_model(app_label, model.__name__) returns None.

Considering that model._meta.model_name == model.__name__.lower(), and looking
at the implementation of register_model and get_registered_model, this proves
that self.all_models[app_label] doesn't contain model._meta.model_name in
register_model, allowing us to simplify the implementation.
  • Loading branch information...
commit aff57793b46e108c6e48d5ce3b889bf90b513df9 1 parent 3518e9e
Aymeric Augustin aaugustin authored
Showing with 7 additions and 13 deletions.
  1. +7 −13 django/apps/registry.py
20 django/apps/registry.py
View
@@ -261,19 +261,13 @@ def register_model(self, app_label, model):
# perform imports because of the risk of import loops. It mustn't
# call get_app_config().
model_name = model._meta.model_name
- models = self.all_models[app_label]
- if model_name in models:
- # The same model may be imported via different paths (e.g.
- # appname.models and project.appname.models). We use the source
- # filename as a means to detect identity.
- fname1 = os.path.abspath(upath(sys.modules[model.__module__].__file__))
- fname2 = os.path.abspath(upath(sys.modules[models[model_name].__module__].__file__))
- # Since the filename extension could be .py the first time and
- # .pyc or .pyo the second time, ignore the extension when
- # comparing.
- if os.path.splitext(fname1)[0] == os.path.splitext(fname2)[0]:
- return
- models[model_name] = model
+ app_models = self.all_models[app_label]
+ # Defensive check for extra safety.
+ if model_name in app_models:
+ raise RuntimeError(
+ "Conflicting '%s' models in application '%s': %s and %s." %
+ (model_name, app_label, app_models[model_name], model))
+ app_models[model_name] = model
self.get_models.cache_clear()
def has_app(self, app_name):

5 comments on commit aff5779

Sergey Pashinin

It caused this bug: https://code.djangoproject.com/ticket/22280
Although it can be easily fixed (in usual case), it breaks IPython autoreloading feature (./manage.py shell)

[autoreload of db.models failed: Traceback (most recent call last):
  File "/pyenvs/p3/lib/python3.4/site-packages/IPython/extensions/autoreload.py", line 247, in check
    superreload(m, reload, self.old_objects)
RuntimeError: Conflicting 'language' models in application 'db': <class 'db.models.Language'> and <class 'db.models.Language'>.

It can not be fixed at all just correcting an import path. So it just breaks this nice autoreloading IPython feature. Any thoughts?

Sergey Pashinin

I suggest:

if model_name in app_models and str(app_models[model_name]) != str(model):
    ...

It will allow to update the same loaded modules (what is needed while development)

Carl Meyer
Owner

It seems to me that this change is likely fine. That case should never be hit except in module reloading. Do you see a problem with that, @aaugustin ?

@pashinin Can you open a new ticket for the problem, and perhaps send a pull request? Thanks!

Sergey Pashinin

I've already made one: #3310
Using it now and it's fine

Florian Apolloner
Owner

@carljm might be fine, might not be fine. Either way I think that this will open a can of worms, cause it's near to impossible to figure out if this change actually works in every case. If it does not it would result in hard to debug errors. I am -0, to -1 -- will note it on the ticket.

Please sign in to comment.
Something went wrong with that request. Please try again.