[Django] #23525: admin/docs/filters|tags __file__ attribute errors for egg extensions
Django
-------------------------+-------------------------------------------------
Reporter: | Owner: nobody
welbornprod | Status: new
Type: Bug | Version: 1.7
Component: | Keywords: __file__ AttributeError filters
contrib.admindocs | tags
Severity: Normal | Has patch: 1
Triage Stage: | UI/UX: 0
Unreviewed |
Easy pickings: 1 |
-------------------------+-------------------------------------------------
Accessing `hostname.com/admin/doc/filters` and
`hostname.com/admin/doc/tags` causes an Internal Server Error on Django
1.7.
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:
{{{#!python
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:
{{{#!python
try:
# ...same code from above.
except OSError, AttributeError:
libraries = []
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23525>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: welbornprod | Owner: nobody
Type: Bug | Status: new
Component: contrib.admindocs | Version: 1.7
Severity: Normal | Resolution:
Keywords: __file__ | Triage Stage:
AttributeError filters tags | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by welbornprod):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Old description:
New description:
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/23525#comment:1>
Django
Reporter: welbornprod | Owner: nobody
Type: Bug | Status: new
Component: contrib.admindocs | Version: 1.7
Severity: Normal | Resolution:
Keywords: __file__ | Triage Stage:
AttributeError filters tags | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
> 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.
New description:
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:
{{{#!python
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:
{{{#!python
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.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/23525#comment:2>
Django
Reporter: welbornprod | Owner: nobody
Type: Bug | Status: new
Component: contrib.admindocs | Version: 1.7
Severity: Normal | Resolution:
AttributeError filters tags | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by aaugustin):
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
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).
[https://docs.python.org/3/library/pkgutil.html#pkgutil.iter_modules
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,
--
Ticket URL: <https://code.djangoproject.com/ticket/23525#comment:3>
Django
Reporter: welbornprod | Owner: nobody
Type: Bug | Status: new
Component: contrib.admindocs | Version: 1.7
Severity: Normal | Resolution:
Keywords: __file__ | Triage Stage: Accepted
AttributeError filters tags | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by welbornprod):
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`.
--
Ticket URL: <https://code.djangoproject.com/ticket/23525#comment:4>
Django
Reporter: welbornprod | Owner: nobody
Type: Bug | Status: new
Component: contrib.admindocs | Version: 1.7
Severity: Normal | Resolution:
Keywords: __file__ | Triage Stage: Accepted
AttributeError filters tags | Needs documentation: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timgraham):
* needs_better_patch: 1 => 0
* needs_tests: 0 => 1
* easy: 1 => 0
Comment:
Is it possible to add tests for this?
--
Ticket URL: <https://code.djangoproject.com/ticket/23525#comment:5>
Django
Reporter: welbornprod | Owner: nobody
Type: Bug | Status: new
Component: contrib.admindocs | Version: 1.7
Severity: Normal | Resolution:
Keywords: __file__ | Triage Stage: Accepted
AttributeError filters tags | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by welbornprod):
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 could 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23525#comment:6>
Django
Reporter: welbornprod | Owner: nobody
Type: Bug | Status: new
Component: contrib.admindocs | Version: 1.7
Severity: Normal | Resolution:
Keywords: __file__ | Triage Stage: Accepted
AttributeError filters tags | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by timgraham):
* needs_tests: 1 => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23525#comment:7>
Django
Reporter: welbornprod | Owner: nobody
Type: Bug | Status: new
Component: contrib.admindocs | Version: 1.7
Severity: Normal | Resolution:
Keywords: __file__ | Triage Stage: Accepted
AttributeError filters tags |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by welbornprod):
@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.
--
Ticket URL: <https://code.djangoproject.com/ticket/23525#comment:8>
Django
Reporter: welbornprod | Owner: nobody
Component: contrib.admindocs | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: __file__ | Triage Stage: Accepted
AttributeError filters tags |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"01ab84c61330ffa5ac87c637249611c5e5343e57"]:
{{{
#!CommitTicketReference repository=""
revision="01ab84c61330ffa5ac87c637249611c5e5343e57"
Fixed #23525 -- Fixed admindocs crash on apps installed as eggs.
Thanks welbornprod for report and initial patch.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23525#comment:9>
Django
Reporter: welbornprod | Owner: nobody
Type: Bug | Status: closed
Component: contrib.admindocs | Version: 1.7
Severity: Normal | Resolution: fixed
Keywords: __file__ | Triage Stage: Accepted
AttributeError filters tags |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"ac098867c004b7336aac30d0689e70fbd73a7306"]:
{{{
#!CommitTicketReference repository=""
revision="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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/23525#comment:10>