Opened 13 months ago

Last modified 10 months ago

#27360 assigned Cleanup/optimization

Make it easier to track down the offending models for AlreadyRegistered exceptions

Reported by: Ed Morley Owned by: Quentin Fulsher
Component: contrib.admin Version: 1.8
Severity: Normal Keywords:
Cc: django@…, qfulsher@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I've just updated 20+ packages locally in a project's requirements file, one of which has caused:

[2016-10-18 15:00:18,667] ERROR [django.request:256] Internal Server Error: /browserid/csrf/
Traceback (most recent call last):
  File "/home/vagrant/venv/local/lib/python2.7/site-packages/django/core/handlers/base.py", line 223, in get_response
    response = middleware_method(request, response)
  File "/home/vagrant/venv/local/lib/python2.7/site-packages/debug_toolbar/middleware.py", line 129, in process_response
    panel.generate_stats(request, response)
  File "/home/vagrant/venv/local/lib/python2.7/site-packages/debug_toolbar/panels/request.py", line 41, in generate_stats
    match = resolve(request.path)
  File "/home/vagrant/venv/local/lib/python2.7/site-packages/django/core/urlresolvers.py", line 521, in resolve
    return get_resolver(urlconf).resolve(path)
  File "/home/vagrant/venv/local/lib/python2.7/site-packages/django/core/urlresolvers.py", line 365, in resolve
    for pattern in self.url_patterns:
  File "/home/vagrant/venv/local/lib/python2.7/site-packages/django/core/urlresolvers.py", line 401, in url_patterns
    patterns = getattr(self.urlconf_module, "urlpatterns", self.urlconf_module)
  File "/home/vagrant/venv/local/lib/python2.7/site-packages/django/core/urlresolvers.py", line 395, in urlconf_module
    self._urlconf_module = import_module(self.urlconf_name)
  File "/usr/lib/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/home/vagrant/treeherder/treeherder/config/urls.py", line 12, in <module>
    browserid_admin.copy_registry(admin.site)
  File "/home/vagrant/venv/local/lib/python2.7/site-packages/django_browserid/admin.py", line 39, in copy_registry
    self.register(model, modeladmin.__class__)
  File "/home/vagrant/venv/local/lib/python2.7/site-packages/django/contrib/admin/sites.py", line 90, in register
    raise AlreadyRegistered('The model %s is already registered' % model.__name__)
AlreadyRegistered: The model Site is already registered

This model isn't defined in the project's own models so in this particular case it must be django-browserid now clashing with one of the packages that was updated.

Rather than having to bisect, it would be much more helpful if this exception gave more details about the already-registered model, so I know which package/app is clashing with the later app's attempts to re-register it.

Change History (6)

comment:1 Changed 13 months ago by Tim Graham

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:2 Changed 13 months ago by Quentin Fulsher

It might be a good idea to add the model's __module__ field in front of the model name. Suppose for example I defined the model UserModel in app1.models. This would turn the resulting exception's message to AlreadyRegistered: The model app1.models.Site is already registered. Does this sound like a good addition?

comment:3 Changed 13 months ago by Keryn Knight

Cc: django@… added

I'd think that providing the ModelAdmin label would be useful - the model is always going to be the one you're trying to register the "current" ModelAdmin for, so showing it's module or path is only going to be truly useful with multiple models of the same name?

Perhaps something like:
AlreadyRegistered: Cannot use myapp.admin.SecondModelAdmin for myotherapp.MyModel, because myotherapp.admin.MyModelAdmin is already registered

Wording isn't perfect (for one thing, all of them say "AlreadyRegistered: ... already registered"), but you probably get the idea about surfacing as much information as possible.
The previously registered ModelAdmin would have to be extracted from the _registry to get it's label, but that ought to be simple enough.

I don't know whether shortening the model to applabel.modelname is better or worse than including it's actual module -- models are routinely referred to that way everywhere else, but given a ModelAdmin doesn't really have an applabel, that would have to show the full class name/module, so it might be inconsistent and confusing if the Model were shortened.

comment:4 Changed 13 months ago by Quentin Fulsher

Cc: qfulsher@… added
Has patch: set
Owner: changed from nobody to Quentin Fulsher
Status: newassigned

I made a pull request for this ticket here: https://github.com/django/django/pull/7423

comment:5 Changed 13 months ago by Tim Graham

Patch needs improvement: set

comment:6 Changed 10 months ago by Ed Morley

I've gone back to reproduce the original case in the ticket description, using:

I'm not sure if this appeared in the logs previously, but higher up in them I now see:
ImportError: No module named urls

...from the include('rest_framework_swagger.urls') in:
https://github.com/mozilla/treeherder/blob/b3740a2fdb80bf10276128cc51a77a7b107ea606/treeherder/config/urls.py#L19

Commenting out that line prevents the AlreadyRegistered exception (django-rest-swagger 2.x changed the method of registering URLs).
I'm guessing this was some bad interaction between import order and django_browserid's copy_registry():
https://github.com/mozilla/django-browserid/blob/v2.0.1/django_browserid/admin.py#L38-L39

If I try the proposed PR (ie using model._meta.label) under this testcase, I get:
AttributeError: 'Options' object has no attribute 'label'

So I'm not 100% sure how best to improve the error message in this presumably rather rare edge-case, so perhaps not worth fixing this.
(Though in the more common case discussed by others above, an improved error message may still be helpful?)

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