Opened 7 years ago

Closed 6 years ago

Last modified 5 years ago

#8245 closed (fixed)

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

Reported by: jarrow 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 jarrow 7 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 7 years ago.
another solution, would this work?
Misleading-AlreadyRegistered-02.diff (1.3 KB) - added by oyvind 7 years ago.
this was what i meant :)
Misleading-AlreadyRegistered-03.diff (985 bytes) - added by jarrow 7 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 gwilson 7 years ago.

Download all attachments as: .zip

Change History (28)

comment:1 Changed 7 years ago by julien

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 7 years ago by brosner

  • Resolution set to duplicate
  • Status changed from new to closed

This is indeed a duplicate of #8118.

comment:3 Changed 7 years ago by brosner

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Wait. I am wrong. My bad.

comment:4 Changed 7 years ago by brosner

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 7 years ago by jarrow

  • Owner changed from nobody to jarrow
  • Status changed from reopened to new

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

Changed 7 years ago by jarrow

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 7 years ago by julien

  • Owner changed from jarrow to julien
  • Status changed from new to assigned

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 7 years ago by julien

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 7 years ago by julien

  • Owner changed from julien to nobody
  • Status changed from assigned to new

comment:9 Changed 7 years ago by jarrow

  • 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 7 years ago by oyvind

another solution, would this work?

comment:10 Changed 7 years ago by jarrow

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 7 years ago by oyvind

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

Changed 7 years ago by oyvind

this was what i meant :)

comment:12 Changed 7 years ago by jarrow

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 7 years ago by oyvind

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

comment:14 Changed 7 years ago by jacob

  • milestone 1.0 deleted
  • Triage Stage changed from Unreviewed to Design decision needed

Changed 7 years ago by jarrow

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

comment:15 Changed 7 years ago by jarrow

#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 7 years ago by gwilson

comment:16 Changed 7 years ago by gwilson

  • milestone set to post-1.0
  • Triage Stage changed from Design decision needed to Accepted

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

comment:17 Changed 7 years ago by mtredinnick

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 6 years ago by kmtracey

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

comment:19 Changed 6 years ago by gwilson

  • Resolution set to fixed
  • Status changed from new to closed

(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 6 years ago by gwilson

Backported to 1.0.X in r9681.

comment:21 Changed 6 years ago by jarrow

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 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

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