Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#8245 closed (fixed)

Exceptions in admin.py can get hidden by an AlreadyRegistered exception

Reported by: Owned by: nobody
Component: contrib.admin Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Consider the following admin.py file:

from django.contrib import admin
from testpro.testapp.models import *

admin.site.register(Screenshot)

raise Exception("Exception that can be much less obvious than this staged one (like a missed import).")

It's loaded by admin.autodiscover() in urls.py.

Now start the development server in debug mode and access the admin. What happens is this:

  • On the first request after starting the dev server our test exception is shown as the response.
ImproperlyConfigured at /admin/
Error while importing URLconf 'testpro.urls': Exception that can be much less obvious than this staged one (like a missed import).
  • For every subsequent request this exception is the response:
ImproperlyConfigured at /admin/
Error while importing URLconf 'testpro.urls': The model Screenshot is already registered

Needless to say this can be really frustrating to debug if one does not see the first and correct exception by chance.

Attachments (5)

Misleading-AlreadyRegistered-00.diff (915 bytes) - added by 8 years ago.
Fixes the bug by unregistering models that were registered in the admin.py file before the exception occured so they can be safely registered again
Misleading-AlreadyRegistered-01.diff (1.1 KB) - added by oyvind 8 years ago.
another solution, would this work?
Misleading-AlreadyRegistered-02.diff (1.3 KB) - added by oyvind 8 years ago.
this was what i meant :)
Misleading-AlreadyRegistered-03.diff (985 bytes) - added by 8 years ago.
Simply copy the registry before import and restore on exception. This prevents AlreadyRegistered and NotRegistered exceptions.
8245.diff (1.7 KB) - added by Gary Wilson 8 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 8 years ago by Julien Phalip

This is probably the same issue as in #8118. It would be good to test if it's exclusively an issue with the dev server, or with any server like Apache too.

comment:2 Changed 8 years ago by Brian Rosner

Resolution: duplicate
Status: newclosed

This is indeed a duplicate of #8118.

comment:3 Changed 8 years ago by Brian Rosner

Resolution: duplicate
Status: closedreopened

Wait. I am wrong. My bad.

comment:4 Changed 8 years ago by Brian Rosner

Julien, my guess on this problem is that Python re-imports urls.py again on the second request because the exception preventing it from making it to sys.modules. I would suspect this is a problem anywhere Python is. I just don't know how much can be done without changing the behavior of registration, which I disagree should happen. There may either be something I am missing however.

comment:5 Changed 8 years ago by

Owner: changed from nobody to
Status: reopenednew

I'll try and write a patch for it and test that on Apache also.

Changed 8 years ago by

Fixes the bug by unregistering models that were registered in the admin.py file before the exception occured so they can be safely registered again

comment:6 Changed 8 years ago by Julien Phalip

Owner: changed from to Julien Phalip
Status: newassigned

Just closed #8118. I believe both tickets are different symptoms of the same problem, and jarrow's patch takes the right approach here.

You want to unregister models that have been registered before the exception was raised, as they will be registered again when that admin.py module is re-loaded in subsequent times.

comment:7 Changed 8 years ago by Julien Phalip

Again, it would be worth testing the patch both on the dev server and a regular server. There might be some side effects, but hopefully not.

comment:8 Changed 8 years ago by Julien Phalip

Owner: changed from Julien Phalip to nobody
Status: assignednew

comment:9 Changed 8 years ago by

Has patch: set

I tested the problem on Apache 2, it's also present. The fix works on Apache and the dev server.

The test suite is running fine.

This should work as we are only unregistering models that will be re-registered next time anyway and do not touch the rest. This way even "valid" AlreadyRegistered exceptions (with the previous call to register() in the file or outside) should work correctly.

Changed 8 years ago by oyvind

another solution, would this work?

comment:10 Changed 8 years ago by

No, as the patch doesn't compile ;) Unless I'm missing something this line is pseudo-code:

for app in settings.INSTALLED_APPS and app not in valid_apps_admin:

But seriously, it can't work as the patch only makes sure that admin.py files that had no errors are not loaded again. But that that's the case anyway. It does not change anything for the ones that produce exceptions.

comment:11 Changed 8 years ago by oyvind

oops, my bad, should be a if in there :)

Changed 8 years ago by oyvind

this was what i meant :)

comment:12 Changed 8 years ago by

Yeah. But that still doesn't fix the problem :) I've tested your patch already corrected like in the second version.

If I understand it correctly python only evaluates the statements in the admin.py files the first time they are imported. So what you do -- making sure that they are not imported twice -- does not change anything. The problem only exists when an exception occurs in an admin.py file. Then python has to re-evaluate that file.

If you see any particular problem with my patch plz post it. The tests run fine but that doesn't mean I can't have missed something ...

comment:13 Changed 8 years ago by oyvind

Ah, now I get it, you patch is correct :)

comment:14 Changed 8 years ago by Jacob

milestone: 1.0
Triage Stage: UnreviewedDesign decision needed

Changed 8 years ago by

Simply copy the registry before import and restore on exception. This prevents AlreadyRegistered and NotRegistered exceptions.

comment:15 Changed 8 years ago by

#8282 also mentions the possibility of NotRegistered exceptions on the subsequent requests. This can occur when unregistering models that were in the registry before the import.

The new patch fixes this by simply copying the registry instead of trying to unregister and register. Re-registering wouldn't be possibly anyway as we don't have the necessary information.

Changed 8 years ago by Gary Wilson

Attachment: 8245.diff added

comment:16 Changed 8 years ago by Gary Wilson

milestone: post-1.0
Triage Stage: Design decision neededAccepted

A different approach using a loading flag as suggested by Malcolm in IRC.

comment:17 Changed 8 years ago by Malcolm Tredinnick

What's wrong with just setting the variable to True or False? All you're trying to do is stop the function being run recursively.

comment:18 Changed 8 years ago by Karen Tracey

Another person stumbled over this & reported it as #9417.

comment:19 Changed 8 years ago by Gary Wilson

Resolution: fixed
Status: newclosed

(In [9680]) Fixed #8245 -- Added a LOADING flag to autodiscover to prevent an admin.py module with errors from raising a spurious AlreadyRegistered exception in a subsequent call to autodiscover.

comment:20 Changed 8 years ago by Gary Wilson

Backported to 1.0.X in r9681.

comment:21 Changed 8 years ago by

Cool, thanks for the fix. I think this solution has some drawbacks though:

  • You only see the real exception on the first request
  • All the stuff getting executed behind the exception in the admin.py file is missing (which could cause other exceptions in turn)
  • To get the real exception back and to execute the stuff behind the source the exception one has to restart the server (even the development one)

The previous patch (though admittedly not that pretty) didn't have these issues. Was this the intended behaviour?

comment:22 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

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