Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#23525 closed Bug (fixed)

admin/docs/filters|tags __file__ attribute errors for egg extensions

Reported by: Welborn Productions Owned by: nobody
Component: contrib.admindocs Version: 1.7
Severity: Normal Keywords: __file__ AttributeError filters tags
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Welborn Productions)

Accessing hostname.com/admin/doc/filters and hostname.com/admin/doc/tags causes an Internal Server Error on Django 1.7. The cause is modules that have no __file__ attribute.

In django/contrib/admindocs/views.py, the function load_all_installed_template_libraries() already gracefully fails on OSError when finding python files. However, when the module being checked has no __file__ attribute the error bubbles up and causes an Internal Server Error.

Someone on django-users suggested that it may be because some extensions are installed as eggs. I've attached a naive patch that simply adds AttributeError to the caught exceptions, causing the function to fail gracefully instead of letting it bubble up.

The code that triggers the error:

try:
    libraries = [
        os.path.splitext(p)[0]
        for p in os.listdir(os.path.dirname(upath(mod.__file__)))
        if p.endswith('.py') and p[0].isalpha()
    ]
except OSError:
    libraries = []

I've simply added another error to that block:

try:
   # ...same code from above.
except (OSError, AttributeError):
    libraries = []

I thought about refactoring this and maybe expanding the for-loop so AttributeError is only caught where needed, but chose instead to make the least invasive change possible.

Attachments (3)

django_admindocs_filter.patch (575 bytes ) - added by Welborn Productions 10 years ago.
Fixed patch for file AttributeError (naive fix).
django_admindocs_filter-2.patch (1.3 KB ) - added by Welborn Productions 10 years ago.
Newer patch with suggested changes.
23525.diff (1.3 KB ) - added by Tim Graham 10 years ago.

Download all attachments as: .zip

Change History (13)

by Welborn Productions, 10 years ago

Fixed patch for file AttributeError (naive fix).

comment:1 by Welborn Productions, 10 years ago

Description: modified (diff)

comment:2 by Welborn Productions, 10 years ago

Description: modified (diff)

comment:3 by Aymeric Augustin, 10 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

Django is attempting to find all possible modules, even those that aren't imported yet, which is technically incompatible with Python's import machinery (loaders and importers). pkgutil.iter_modules won't help. So there won't be a perfect solution.

That said, I would find it better to split the try/except in two parts:

  1. attempt to compute os.path.dirname(upath(mod.__file__)), continue if that fails,
  2. attempt to list files within that directory, continue if that fails,

by Welborn Productions, 10 years ago

Newer patch with suggested changes.

comment:4 by Welborn Productions, 10 years ago

Here is a new patch with the suggested changes. It catches AttributeError when calculating the directory, if it fails then continue is used.

On success it will try to list the files in that directory.

I used an else on the this try, so the libraries list is used only when it is successfully initialized. The old method to skip/continue on OSError was to set libraries to an empty list which would then be skipped automatically with the for loop. The new method uses an actual continue.

comment:5 by Tim Graham, 10 years ago

Easy pickings: unset
Needs tests: set
Patch needs improvement: unset

Is it possible to add tests for this?

comment:6 by Welborn Productions, 10 years ago

To be honest I'm not sure how I would test for this. The function has no arguments. It depends on template.get_templatetags_modules() to gather module names, import them, determine the directory for each module, and list the directory contents. How would I introduce a module with no __file__ to test with?

This patch doesn't change the way template libraries are loaded. It just lets it fail with a little more grace. If anyone knows how I might go about testing this I would be glad to put in the work.

Last edited 10 years ago by Welborn Productions (previous) (diff)

comment:7 by Tim Graham, 10 years ago

Needs tests: unset

That's what I thought regarding tests.

After looking at your patch, I would take a slightly different approach; please see the attached patch. If it looks fine (maybe you'd like to test it), I'll go ahead and commit it.

by Tim Graham, 10 years ago

Attachment: 23525.diff added

comment:8 by Welborn Productions, 10 years ago

@timgraham, sorry it has taken me so long to reply. Your approach works fine. I tested it under my setup and it allowed me to use the filters documentation perfectly. I would love to see this fix included in Django.

comment:9 by Tim Graham <timograham@…>, 10 years ago

Resolution: fixed
Status: newclosed

In 01ab84c61330ffa5ac87c637249611c5e5343e57:

Fixed #23525 -- Fixed admindocs crash on apps installed as eggs.

Thanks welbornprod for report and initial patch.

comment:10 by Tim Graham <timograham@…>, 10 years ago

In ac098867c004b7336aac30d0689e70fbd73a7306:

[1.7.x] Fixed #23525 -- Fixed admindocs crash on apps installed as eggs.

Thanks welbornprod for report and initial patch.

Backport of 01ab84c61330ffa5ac87c637249611c5e5343e57 from master

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