Opened 6 years ago

Closed 6 years ago

Last modified 6 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 6 years ago.
Fixed patch for file AttributeError (naive fix).
django_admindocs_filter-2.patch (1.3 KB) - added by Welborn Productions 6 years ago.
Newer patch with suggested changes.
23525.diff (1.3 KB) - added by Tim Graham 6 years ago.

Download all attachments as: .zip

Change History (13)

Changed 6 years ago by Welborn Productions

Fixed patch for file AttributeError (naive fix).

comment:1 Changed 6 years ago by Welborn Productions

Description: modified (diff)

comment:2 Changed 6 years ago by Welborn Productions

Description: modified (diff)

comment:3 Changed 6 years ago by Aymeric Augustin

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,

Changed 6 years ago by Welborn Productions

Newer patch with suggested changes.

comment:4 Changed 6 years ago by Welborn Productions

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 Changed 6 years ago by Tim Graham

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

Is it possible to add tests for this?

comment:6 Changed 6 years ago by Welborn Productions

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 6 years ago by Welborn Productions (previous) (diff)

comment:7 Changed 6 years ago by Tim Graham

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.

Changed 6 years ago by Tim Graham

Attachment: 23525.diff added

comment:8 Changed 6 years ago by Welborn Productions

@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 Changed 6 years ago by Tim Graham <timograham@…>

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 Changed 6 years ago by Tim Graham <timograham@…>

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