[Django] #27360: Make it easier to track down the offending models for AlreadyRegistered exceptions
Django
exceptions
-------------------------------+--------------------
Reporter: Ed Morley | Owner: nobody
Type: Uncategorized | Status: new
Component: contrib.admin | Version: 1.8
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27360>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
exceptions
Reporter: Ed Morley | Owner: nobody
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: => 0
* needs_docs: => 0
* type: Uncategorized => Cleanup/optimization
* needs_tests: => 0
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/27360#comment:1>
Django
exceptions
Reporter: Ed Morley | Owner: nobody
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/27360#comment:2>
Django
exceptions
Reporter: Ed Morley | Owner: nobody
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Keryn Knight):
* cc: django@… (added)
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/27360#comment:3>
Django
exceptions
Reporter: Ed Morley | Owner: Quentin
Type: | Fulsher
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Quentin Fulsher):
* cc: qfulsher@… (added)
* status: new => assigned
* has_patch: 0 => 1
* owner: nobody => Quentin Fulsher
Comment:
I made a pull request for this ticket here:
https://github.com/django/django/pull/7423
--
Ticket URL: <https://code.djangoproject.com/ticket/27360#comment:4>
Django
exceptions
Reporter: Ed Morley | Owner: Quentin
Type: | Fulsher
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/27360#comment:5>
Django
exceptions
Reporter: Ed Morley | Owner: Quentin
Type: | Fulsher
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by 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?)
--
Ticket URL: <https://code.djangoproject.com/ticket/27360#comment:6>
Django
exceptions
Reporter: Ed Morley | Owner: Quentin
Type: | Fulsher
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Hasan Ramezani):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/27360#comment:7>
Django
exceptions
Reporter: Ed Morley | Owner: Quentin
Type: | Fulsher
Component: contrib.admin | Version: 1.8
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d4df5e1b0b1c643fe0fc521add0236764ec8e92a" d4df5e1b]:
{{{
#!CommitTicketReference repository=""
revision="d4df5e1b0b1c643fe0fc521add0236764ec8e92a"
Fixed #27360 -- Added app or ModelAdmin details for AreadyRegistered
exceptions.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/27360#comment:8>