Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#7922 closed (fixed)

admin.autodiscover() ate my ImportError

Reported by: Jan Rademaker <j.rademaker@…> Owned by: brosner
Component: contrib.admin Version: master
Severity: Keywords: ImportError autodiscover
Cc: brosner Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

Just spend some time figuring out what I had done to break the admin interface. As it turned out I had a typo in one of my import statements and this was silenced by autodiscover(). This patch tries to determine if the import failed because admin.py is missing or if the exception was caused by something else.

Attachments (5)

7922.diff (1.6 KB) - added by Jan Rademaker <j.rademaker@…> 6 years ago.
autodiscover.diff (680 bytes) - added by Alex 6 years ago.
This uses imp to test for the existance of the module.
autodiscover.2.diff (728 bytes) - added by Alex 6 years ago.
Updated, this should work.
autodiscover_getattr_import_option_1.diff (1.5 KB) - added by oyvind 6 years ago.
Getattr import solution 1
autodiscover_getattr_import_option_2.diff (1.3 KB) - added by oyvind 6 years ago.
Getattr import solution 2

Download all attachments as: .zip

Change History (18)

Changed 6 years ago by Jan Rademaker <j.rademaker@…>

Changed 6 years ago by Alex

This uses imp to test for the existance of the module.

comment:1 follow-up: Changed 6 years ago by Alex

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

The technique I use(or testing with imp) can be used to solve similar issues in urlpatterns.

comment:2 in reply to: ↑ 1 Changed 6 years ago by Jan Rademaker <j.rademaker@…>

Replying to Alex:

The technique I use(or testing with imp) can be used to solve similar issues in urlpatterns.

From the imp docs:

    Search for a module.  If path is omitted or None, search for a
    built-in, frozen or special module and continue search in sys.path.
    The module name cannot contain '.'; to search for a submodule of a
    package, pass the submodule name and the package's __path__.

So this won't work for project.app.admin style imports.

comment:3 Changed 6 years ago by Alex

Bah, you are correct, I believe it would be possible to do using imp and passing path, but that may be more trouble than it's worth.

comment:4 Changed 6 years ago by Alex

I lied it should actually be pretty simple

In [39]: imp.find_module('admin', __import__('django.contrib.auth', fromlist=['auth']).__path__)
Out[39]: 
(<open file 'django/contrib/auth/admin.py', mode 'U' at 0xb7896f98>,
 'django/contrib/auth/admin.py',
 ('.py', 'U', 1))

I'm going to update the patch now.

Changed 6 years ago by Alex

Updated, this should work.

comment:5 Changed 6 years ago by oyvind

Fromlist can cause double initialization of modules, see latest patch in #6587 for solution

Changed 6 years ago by oyvind

Getattr import solution 1

Changed 6 years ago by oyvind

Getattr import solution 2

comment:6 Changed 6 years ago by russellm

  • milestone set to 1.0

The issue of eating imports should be addressed before 1.0; it's not strictly a bug, but it is one of those usability things that deserves attention.

comment:7 Changed 6 years ago by brosner

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

comment:8 Changed 6 years ago by brosner

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

(In [8174]) Fixed #7922 -- admin.autodiscover() no longer eats ImportErrors for breakfast. Thanks Jan Rademaker and Alex Gaynor for their work on the ticket.

comment:9 Changed 6 years ago by anonymous

  • Cc brosner added
  • Needs tests set
  • Resolution fixed deleted
  • Status changed from closed to reopened

I have a problem because of the changeset 8174

I solve the problem doing any changes:

#before
# django/contrib/admin/init.py

from django.contrib.admin.options import ModelAdmin, HORIZONTAL, VERTICAL
from django.contrib.admin.options import StackedInline, TabularInline
from django.contrib.admin.sites import AdminSite, site

def autodiscover():
    """
    Auto-discover INSTALLED_APPS admin.py modules and fail silently when 
    not present. This forces an import on them to register any admin bits they
    may want.
    """
    import imp
    from django.conf import settings
    for app in settings.INSTALLED_APPS:
        try:
            imp.find_module("admin", app.split("."))
        except ImportError:
            # there is no admin.py in app, skip it.
            continue
        __import__("%s.admin" % app)

#after
# django/contrib/admin/init.py

from django.contrib.admin.options import ModelAdmin, HORIZONTAL, VERTICAL
from django.contrib.admin.options import StackedInline, TabularInline
from django.contrib.admin.sites import AdminSite, site

def autodiscover():
    """
    Auto-discover INSTALLED_APPS admin.py modules and fail silently when 
    not present. This forces an import on them to register any admin bits they
    may want.
    """
#    import imp
    from django.conf import settings
    for app in settings.INSTALLED_APPS:
        try:
#            imp.find_module("admin", app.split("."))
            __import__("%s.admin" % app)
        except ImportError:
            # there is no admin.py in app, skip it.
            continue

please, review the use of the imp library

comment:10 Changed 6 years ago by brosner

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

Can you please be specific about what the problem is. You are simply showing how to revert the code. Also please do so in another ticket because the problem was solved, but sounds like a *new* problem has occurred.

comment:11 Changed 6 years ago by brosner

  • Resolution fixed deleted
  • Status changed from closed to reopened

Ok, my bad. I made a mistake. anonymous should have been more specific. I have reverted [8174] in [8183]. Looking into it now.

comment:12 Changed 6 years ago by brosner

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

(In [8186]) Pass the correct data as the second parameter to find_module to correct admin.autodiscover(). Fixes #7922. Thanks Alex Gaynor for being smarter than me.

comment:13 Changed 3 years ago by jacob

  • milestone 1.0 deleted

Milestone 1.0 deleted

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.