abspaths and abspath cache don't work correctly

[wkornewald]

As discussed on IRC:

09:57 <wkornewald> hi, could it be that the template loader doesn't look up cached filenames if they are included from a template that was loaded with an absolute path?
09:59 <wkornewald> it only tries to find the filename given, but if an abs-loaded template includes 'layout.html' it will find the file in the search_path, but not the one in the abs-loaded-template's dir
09:59 <wkornewald> example:
09:59 <wkornewald> search_path is only one dir with:
09:59 <wkornewald> index.html
09:59 <wkornewald> layout.html
09:59 <wkornewald> sub/index.html
09:59 <wkornewald> sub/layout.html
10:00 <wkornewald> sub/index.html includes "layout.html"
10:00 <wkornewald> what I would expect it to get is "sub/layout.html", but it gets "layout.html"
10:03 <wkornewald> part of the problem is that for abs->relative includes it does: search_path = search_path + [dirname]  instead of search_path = [dirname] + search_path
10:03 <wkornewald> thus placing the includer's dirname at the end of the search path
10:06 <wkornewald> but there's also another problem: for abs->relative includes the cache should be checked for each dir in the search path, again forming an abspath

[wkornewald]

patch against trunk

[wkornewald]

added two little unit tests. test_relative_include_with_subdir_override should fail with the previous behavior

[wkornewald]

*ugly* version of the patch with three unnecessary lines of code. better use the clean version ;)

[wkornewald]

much cleaner version. better use this one ;)

[anonymous]

BTW, with the unclean version the following docstring test fails:

>>> loader = TemplateLoader(['/home/bla/templates'])
>>> loader.load('test.html') is loader.load('/home/bla/templates/test.html')
True

IOW, the unclean version returns False.

Also, the following will result in incorrect behavior. Let's say you have these files:

base.html (contains 'base')
index.html (includes 'base.html')
sub/base.html (contains 'sub base')
sub/index.html (includes 'base.html')

Now, if loading in this order with loader1:

1a) index.html
1b) sub/index.html

And then in this with loader2 (i.e., two separate caches):

2b) sub/index.html
2a) index.html

The following should be true:

1a == 2a
1b == 2b

If it's not then the wrong file was retrieved from the cache. I'll upload an updated test-case and clean template with an updated docstring test.

[wkornewald]

this one also checks whether loading order has an influence on which cached template gets returned

[wkornewald]

added a docstring test to check if an abspath-based load returns the same (cached) template as a relpath based load

[Waldemar Kornewald <wkornewald@…>]

Is anything not ok with the patches?

[cmlenz]

The test test_relative_include_with_search_path_fallback in attachment:genshi-abspaths-tests.2.diff​ is wrong: if sub/tmpl2.html wants to include the template tmpl1.html it needs to use the relative path ../tmpl1.html. There should not be any “fallback” to the search path here.

Otherwise, I'm a bit lost with the patches, and have the feeling they change too much at once :P Where's the test case for the search_path = [dirname] + search_path change mentioned in the description, for example?

[Waldemar Kornewald <wkornewald@…>]

Don't fall back to search path base, but always include relative to current subdir in "virtual path". Also, added a test for [dirname] + search_path.

[Waldemar Kornewald <wkornewald@…>]

Now using a simpler mechanism to get abspath-based caching to work half-way acceptably.

[Waldemar Kornewald <wkornewald@…>]

Since it's not using a fully abspath-based cache the following can happen:

* /abspath/loaded.html => includes 'to_load.html'
* /abspath/to_load.html
* search_path/to_load.html

When first loading 'to_load.html' (relpath) and then loading '/abspath/loaded.html' (abspath) it will try to include 'to_load.html', but find that the cache already has such a file and use 'search_path/to_load.html' instead of '/abspath/to_load.html'. To implement this correctly one has to use abspath based caching.

[Waldemar Kornewald <wkornewald@…>]

As an alternative, this does abspaths caching 100% correctly, but requires caching of _nonexistent templates, so it doesn't waste CPU cycles trying to open() them in the for loop.

[Waldemar Kornewald <wkornewald@…>]

The alternative patch is a cleaned up version of the previous abspath-works-clean series which uses the template's abspath as the key for the LRUCache.

[Waldemar Kornewald <wkornewald@…>]

Still not satisfied? :)

[cmlenz]

Alternative patch

[cmlenz]

I've attached a new patch​ that is simpler and seems to fix the unit tests you added in genshi-abspaths-with-cache.diff​.

Can you please check whether it works for you?

Also note that I checked in the prefix stuff and other loader improvements earlier today ;)

[Waldemar Kornewald <wkornewald@…>]

Maybe your prefix patch is already sufficient for me. :) Thanks for committing it!

What I noticed, though, is that the _cache distinguishes between unicode and str keys, so in some cases there can be multiple instances of the same template. That's why I'd rather add a

cachekey = unicode(filename)

Also, if an abspath-loaded template includes a template that lives in its folder the cache will never be used:

* /abspath/bla.html (includes 'layout.html')
* /abspath/layout.html
* /search_path/layout.html

The folder /search_path/ is the search path. If we abspath-load /abspath/bla.html and that includes 'layout.html' (i.e., relpath) it will lookup the cache for 'layout.html' (*not* '/abspath/layout.html') and it will always find /search_path/layout.html because the cachekey for the correct layout.html file would be '/abspath/layout.html'. Well, actually there is one annoying exception if /search_path/layout.html is not cached, yet. In that case the cache does not yet contain a 'layout.html' entry and it'll return the /abspath/layout.html.

From that point of view, the previous search_path = search_path + [dirname] code was more consistent because it would always give files in the search path precedence, no matter if they are cached or not.

I seriously think that if you don't want to implement abspaths 100% correctly (e.g., because that's too much ugly code) then it's better to not support them, at all. Otherwise, people will see unexpected behavior.

[cmlenz]

Replying to Waldemar Kornewald <wkornewald@freenet.de>:

What I noticed, though, is that the _cache distinguishes between unicode and str keys, so in some cases there can be multiple instances of the same template. That's why I'd rather add a

cachekey = unicode(filename)

Are you sure about this? 'foo' == u'foo', it should be the same cache key. Except of course you're using bytestrings outside the ASCII range, which you really shouldn't (but in that case unicode(filename) would fail with a decoding error).

From that point of view, the previous search_path = search_path + [dirname] code was more consistent because it would always give files in the search path precedence, no matter if they are cached or not.

Good point.

[cmlenz]

Fixed by [815].

I can't reproduce the unicode/str issue you mentioned. If you can show the problem with a unit test, please reopen this ticket.

[Waldemar Kornewald]

Replying to cmlenz:

Fixed by [815].

I can't reproduce the unicode/str issue you mentioned. If you can show the problem with a unit test, please reopen this ticket.

I'll have to take a look at it again. I can only remember that when I once ran

print loader._cache

it displayed several templates where the one passed to load() used normal strings and files xi:included were cached as u'/path/...'. Of course, that's only a problem if a file gets loaded directly and also included from other templates.