include & extend don't handle absolute paths correctly

[sauerbraten]

Assuming I have a template at foo/baz.jet, {{ include "foo/bar.jet" }} will make resolveNameSibling try to resolve foo/foo/bar.jet before resolving foo/bar.jet: https://github.com/CloudyKit/jet/blob/33cfc27b3e00072655fdb3af24c3253ea5bffb8f/template.go#L153

name will be "foo/bar.jet", sibling will be "foo/baz.jet".

I would like to simplify template resolving & loading and could fix this issue at the same time. Seeing as we are making breaking changes with v3 anyway, I would like to make jet handle paths like most software does:

• ./asd.jet, asd.jet, ../asd.jet will be treated as relative paths and will be resolved relative to the current template's path
• /foo.jet, /foo/bar.jet will be treated as absolute paths and will be looked up directly in the templates cache and/or the loader, without using the current template's path

Do you want me to proceed with this? Did I miss something?

[tooolbox]

As mentioned in #129 (and @annismckenzie can comment perhaps) I think it's a good idea to get a v3 cut with Go Modules support before we do a ton of breaking changes. So my vote would be to defer this to v4.

Otherwise, I think I track and it seems like an improvement. Taking your example, you're saying that include should resolve foo/foo/bar.jet and should not resolve foo/bar.jet since it's a relative include--do I have that right? That is more in line with how paths are handled in The Rest Of The World so it will probably be more sane.

[sauerbraten]

Sorry, I used the wrong path in my example, it should have been {{ include "/foo/bar.jet" }} inside /foo/baz.jet, which will make resolveNameSibling try to resolve /foo//foo/bar.jet before trying /foo/bar.jet (name will be "/foo/bar.jet", sibling will be "/foo/baz.jet").

So the bug is that it will try to resolve absolute as well as relative paths as paths relative to the including/extending templates' path. If (in the example above) you have /foo/baz.jet, /foo/bar.jet and /foo/foo/bar.jet, /foo/baz.jet will be rendered with the contents from /foo/foo/bar.jet even though the absolute path to /foo/bar.jet was given to include.

But yes, the goal is to be compatible with The Rest Of The World.

[tooolbox]

Ahhh, I'm tracking now. Yes, it does seem like a bug.

I scanned a large template codebase and saw a single absolute path, so at least in my world there's not much of that going on.

I guess the only potential problem in terms of breaking changes is if people are using absolute paths and relying on Jet to make them relative to be able to pick up the templates that they want?

If literally all that's happening is removing the /foo//foo/bar.jet resolution step...I can't think who would rely on behavior like that, so I wouldn't even call it a breaking change. It's like the way the Go authors made map ranging random-access and the people who were relying on the implementation being sequential had to figure out something else.

[sauerbraten]

I know that in our (big!) templates codebase, there are a lot of paths that were written with the intent to have them looked up as absolute paths. Almost every include/extends/import does two lookups right now.

So in our case, because we wrote foo/bar.jet when we really meant /foo/bar.jet, we will have to fix almost all our paths throughout the entire codebase. It's pretty easy to do, since our directory structure makes it obvious which paths are buggy and we can just search-replace them.

But we don't know how other users might be affected, so it's definitely a breaking change and should be noted somewhere.

[tooolbox]

I know that in our (big!) templates codebase, there are a lot of paths that were written with the intent to have them looked up as absolute paths.

Okay cool, that's an interesting contrast.

Almost every include/extends/import does two lookups right now.

Yikes!

So in our case, because we wrote foo/bar.jet when we really meant /foo/bar.jet, we will have to fix almost all our paths throughout the entire codebase.

Well...would you? Seems like the change we're talking about is "don't process absolute paths as relative paths" as in stop doing import "/foo/bar.jet" from "/foo" -> "/foo//foo/bar.jet" The scenario you just mentioned is a relative path include falling back to checking the root folder if it's not found relative to the current template path, which is different. Are you proposing to remove that as well?

Not trying to seem pedantic, just want to name out what we're doing exactly here:

• "Stop processing absolute paths as relative paths" seems like a bugfix that will increase performance and perhaps wouldn't need a major version bump, but yes, should absolutely be noted in a minor release.
• "Stop falling back to the parent/root folder when processing a relative path" is much more of a behavior change which IMO would want a major version bump, as well as I think more discussion about why this is an important change to make.

[sauerbraten]

Yes, I propose doing both of those. I would like to remove the "look up relative paths as absolute paths if the relative lookup fails" functionality because it leads to non-obvious behavior. Say we have (inside our template root) /a/a.jet, /a/b/b.jet and /b/b.jet, where in a.jet we have {{ include "b/b.jet" }}. This will work as expected, until someone deletes the /a/b/b.jet template and forgets to adjust a.jet, or relies on jet's parsing to tell them about templates that used the removed template. a.jet will now silently fall back to including /b/b.jet.

There is a reason why software dealing with paths is strict about them... :)

[tooolbox]

Heh, your point is well made, and I agree that this behavior should be changed.

What do you think, major or minor revision?

[sauerbraten]

Definitely a major version bump, since it may break existing setups.

[alresvor]

hi @sauerbraten

https://github.com/CloudyKit/jet/blob/5afdd4754470afbf979ebc6c8575b37df0e8c44e/template.go#L179-L185

filepath.IsAbs() different behavior on Windows：

fmt.Println(filepath.IsAbs("/abc"))  // false
fmt.Println(filepath.IsAbs("\\abc")) // false

Maybe replace with

func pathIsAbs(path string) bool {
    return path != "" && (path[0] == '/' || path[0] == '\\') ||
        filepath.IsAbs(path)
}

[sauerbraten]

Thanks for spotting that! Fixed in 4b52e03cae5de42d538326f1be71501c49957121 👍

[alresvor]

thanks @sauerbraten But the new code is still wrong

On Windows:

    fmt.Println(filepath.IsAbs("/abc\\bb"))    // false
    fmt.Println(filepath.IsAbs("\\abc\\bb"))   // false
    fmt.Println(filepath.IsAbs("c:\\abc\\bb")) // true
    fmt.Println(filepath.IsAbs("c:/a/b/c"))    // true

    fmt.Println(filepath.Clean("\\aa/b\\c"))   // -> \aa\b\c
    fmt.Println(filepath.ToSlash("\\aa/b\\c")) // -> /aa/b/c

    fmt.Println(filepath.IsAbs(filepath.Clean("/aa")))  // false
    fmt.Println(filepath.IsAbs(filepath.Clean("\\aa"))) // false

    fmt.Println(path.IsAbs(path.Clean("/aa")))  // true
    fmt.Println(path.IsAbs(path.Clean("\\aa"))) // false

    fmt.Println(filepath.IsAbs(filepath.ToSlash("\\aa/b\\c"))) // false
    fmt.Println(path.IsAbs(filepath.ToSlash("/aa\\bb/cc")))    // true
    fmt.Println(path.IsAbs(filepath.ToSlash("\\aa/b\\c")))     // true

can be changed to

path.IsAbs(filepath.ToSlash("/aa"))

[sauerbraten]

@alresvor: the paths that go into that code are never file-system absolute, i.e. there won't be a C:\Users\... path, since the loader always prepends its root directory before looking for the file. The use cases we have to cover look like this:

• ./foo/bar: ./ explicitly tells us the author wants a relative lookup
• ../foo/bar: ../ explicitly tells us the author wants a relative lookup
• foo/bar: for this, we assume a relative lookup is requested, pretty much like a terminal or web browser would
• /foo/bar: path begins with a slash, that means the author does not want jet to look up the path relatively to the current template's path

For people using Windows, we support using a backslash instead of a slash in all of the above paths. https://github.com/CloudyKit/jet/commit/4b52e03cae5de42d538326f1be71501c49957121 fixed detecting UNIX style absolute paths (like /foo/bar) when Jet runs on Windows. We don't need to be able to detect C:\Users\foo\bar as an absolute path. Using paths starting with the partition on windows will make the templates incompatible with other OSes, I don't thinkn anybody should be doing it that way.

Do you expect C:\Users\foo\bar to be detected by Jet as an absolute template path on Windows? If so, I don't think path.IsAbs(filepath.ToSlash("C:\Users\foo\bar")) will return true, since C:/Users/foo/bar is still not absolute according to the path package, not even on Windows.

[alresvor]

Yes, I don’t need to use real windows absolute path, I just use a path like /abc.html. I know that getSiblingTemplate() only detects the path entered by the include function, not the real system path, so why use filepath.IsAbs()? This is what makes me confused.

/foo/bar: path begins with a slash, that means the author does not want jet to look up the path relatively to the current template's path

If you use the filepath.IsAbs() function, it will not be judged as successful on Windows, I think just using path.IsAbs() will do.

[sauerbraten]

I see what you mean now. You're right, it would be better to use filepath.ToSlash() (to allow foo\bar) and then path.IsAbs() to detect absolute paths than filepath.Clean() and filepath.IsAbs().

[sauerbraten]

done in cc9978f14830540788c1b68bd4fe02dd6cb9a9e9

[alresvor]

@sauerbraten Thank you, I didn’t describe it clearly before 😃