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

[Ed Morley]

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.

[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?

[Keryn Knight]

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.

[Quentin Fulsher]

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

[Ed Morley]

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

• b3740a2fdb80bf10276128cc51a77a7b107ea606 of ​https://github.com/mozilla/treeherder
• A vagrant up followed by pip install -U django-rest-swagger==2.0.5
• a ./manage.py runserver and making a GET request to /browserid/csrf/

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?)

[Tim Graham <timograham@…>]

In d4df5e1b:

Fixed #27360 -- Added app or ModelAdmin details for AreadyRegistered exceptions.