Opened 12 years ago
Closed 11 years ago
#19949 closed Cleanup/optimization (fixed)
Cached template loader doesn't cache TemplateDoesNotExist
Reported by: | German M. Bravo | Owned by: | Susan Tan |
---|---|---|---|
Component: | Template system | Version: | 1.5 |
Severity: | Normal | Keywords: | templates cache |
Cc: | German M. Bravo | 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 )
In cases where multiple fallback templates are requested to the template loader (i.e. django.template.loader.select_template()
) to find the first one available, the cached template loader does not cache non-existent templates, making cached loader "slow" again if every time it has to try looking for a template that's been already found to be missing.
from django.template.loader import render_to_string render_to_string(['missing_template_1.html', 'missing_template_2.html', ..., 'missing_template_N.html', 'existent_template.html'])
When using django.template.loaders.cached.Loader
, and given that snippet above, and the fact that missing_template_1.html
... missing_template_N.html
do not exist, the filesystem loader or other loaders get hit every time (trying and trying to find already known as missing templates).
I'm attaching a patch to fix this.
Attachments (1)
Change History (14)
by , 12 years ago
Attachment: | #19949-cache_non_existent_templates.diff added |
---|
comment:1 by , 12 years ago
Cc: | added |
---|
comment:2 by , 12 years ago
Component: | Uncategorized → Template system |
---|---|
Has patch: | set |
Keywords: | templates cache added |
Type: | Uncategorized → Cleanup/optimization |
comment:3 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:4 by , 11 years ago
Needs tests: | set |
---|
comment:5 by , 11 years ago
Updated PR: https://github.com/django/django/pull/1501/ Unit test and full test suite pass.
comment:6 by , 11 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | set |
comment:7 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:8 by , 11 years ago
Patch needs improvement: | unset |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
Fixed in f33db5a09acfc3df3085235a5712c46094eb9a0d.
comment:9 by , 11 years ago
Description: | modified (diff) |
---|
comment:10 by , 11 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
I believe we need to work on this ticket a bit more.
The tests added, although correct because they verify something we didn't test before, don't actually test the fix introduced and so didn't fail beforehand.
In particular these two lines:
https://github.com/django/django/blob/master/django/template/loaders/cached.py#L74-L75
gives us a hint about the fact that this needs further investigation.
I've opened a PR with tests changes. We can discuss things there if you wish: https://github.com/django/django/pull/1511
It's not clear to me yet what the right fix would look like. Maybe we can reuse some code from the patch proposed by the OP (refactored to accompany the code evolution)?
Reopening.
comment:11 by , 11 years ago
Thanks for catching the missing test. I forgot we had to test the cache after the loaders tries to load the missing template. I made a few comments on your PR.
comment:13 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
It would be good to add a test for this.