Repository navigation
fix: smartJoin incorrectly treats relative paths containing special variable names as special paths - #3022
fix: smartJoin incorrectly treats relative paths containing special variable names as special paths#3022znnnnnnn-wil wants to merge 4 commits into
Conversation
|
Um, AI wrote the code, correct? |
|
Yes, Codex helped with part of the implementation and tests. I worked through the issue and the scope of the fix myself, and went over the implementation a few times before opening the PR. I also changed some parts manually after reviewing the generated code. I'm familiar with the behavior this is trying to preserve — in particular, I ran the tests and lint locally and checked the regression cases before submitting. Happy to explain any part of the change or adjust it based on feedback. |
|
Why wouldn't you just do this? |
|
Yeah, I considered that option too. My main concern was that matching only the exact strings might stop some template forms that currently work, for example That's why I went with matching inside template actions instead. It avoids treating literal paths like That said, if only the canonical |
|
Yes, I wondered the same thing, would it matter? Lets assume it would cause problems, then my approach would be to use a regular expression for each item in knownAbsDirs (rather than the string), and precompile those (so package level vars, precompile for performance) and finally apply the regex list in a tight loop (similar to existing code). It would be easier to understand. Or a single regex that considers all 3 possibilities. Probably faster again. |
|
Yeah, that makes sense. A precompiled regex should make this a bit clearer without losing the template variants I was worried about. I'll update Thanks for the suggestion! |
|
@znnnnnnn-wil Looks quite good. You will need to request a review from one of the maintainers (I would suggest @vmaerten). |
|
@vmaerten, when you have a chance, could you take a look at this PR? trulede suggested requesting your review. Thanks! |
|
Hi @vmaerten, just following up on this when you have a chance. The changes are ready for review. Thanks! |
vmaerten
changed the title
| } | ||
| // Heuristically match special directory names within template actions, without | ||
| // matching prefixes of longer identifiers. This does not parse template syntax. | ||
| var specialDirRE = regexp.MustCompile(`\{\{[^{}]*\.(?:ROOT_DIR|TASKFILE_DIR|USER_WORKING_DIR)(?:[^\p{L}\p{Nd}_{}][^{}]*)?\}\}`) |
There was a problem hiding this comment.
The regex is hard to read.
could \{\{[^}]*\.(?:ROOT_DIR|TASKFILE_DIR|USER_WORKING_DIR)\b do the job ?
| package filepathext | ||
|
|
||
| import ( | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestSmartJoinRelativePathContainingSpecialVariableName(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| base := t.TempDir() | ||
| for _, variable := range []string{".ROOT_DIR", ".TASKFILE_DIR", ".USER_WORKING_DIR"} { | ||
| t.Run(variable, func(t *testing.T) { | ||
| t.Parallel() | ||
| for _, relative := range []string{ | ||
| filepath.Join("project"+variable, "file.txt"), | ||
| filepath.Join(variable, "file.txt"), | ||
| filepath.Join("{{.PROJECT}}"+variable, "file.txt"), | ||
| filepath.Join("{{.PROJECT}}"+variable+"{{.SUFFIX}}", "file.txt"), | ||
| filepath.Join("{{ "+variable, "file.txt"), | ||
| } { | ||
| require.False(t, IsAbs(relative), relative) | ||
| require.Equal(t, filepath.Join(base, relative), SmartJoin(base, relative)) | ||
| } | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestSmartJoinAbsoluteAndTemplatePaths(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| base := t.TempDir() | ||
| absolute := filepath.Join(t.TempDir(), "file.txt") | ||
| for _, path := range []string{ | ||
| absolute, | ||
| "{{.ROOT_DIR}}/file.txt", | ||
| "{{ .TASKFILE_DIR }}/file.txt", | ||
| "{{- .USER_WORKING_DIR -}}/file.txt", | ||
| "{{.ROOT_DIR | toSlash}}/file.txt", | ||
| "{{.ROOT_DIR|toSlash}}/file.txt", | ||
| "{{(.ROOT_DIR)}}/file.txt", | ||
| "{{.PROJECT}}/{{.ROOT_DIR}}/file.txt", | ||
| "{{\n.TASKFILE_DIR\n}}/file.txt", | ||
| `{{joinPath .ROOT_DIR "src"}}/file.txt`, | ||
| } { | ||
| require.True(t, IsAbs(path), path) | ||
| require.Equal(t, path, SmartJoin(base, path)) | ||
| } | ||
| } | ||
|
|
||
| func TestSmartJoinUnrelatedTemplatePaths(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| base := t.TempDir() | ||
| for _, path := range []string{ | ||
| "{{.PROJECT}}/file.txt", | ||
| "{{XROOT_DIR}}/file.txt", | ||
| "{{XTASKFILE_DIR}}/file.txt", | ||
| "{{XUSER_WORKING_DIR}}/file.txt", | ||
| } { | ||
| require.False(t, IsAbs(path), path) | ||
| require.Equal(t, filepath.Join(base, path), SmartJoin(base, path)) | ||
| } | ||
| } | ||
|
|
||
| func TestSmartJoinTemplateVariableNameSuffixes(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| base := t.TempDir() | ||
| for _, variable := range []string{".ROOT_DIR", ".TASKFILE_DIR", ".USER_WORKING_DIR"} { | ||
| for _, suffix := range []string{"_SUFFIX", "_EXTRA", "2", "suffix", "变量"} { | ||
| t.Run(variable+suffix, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| path := "{{" + variable + suffix + "}}/file.txt" | ||
| require.False(t, IsAbs(path), path) | ||
| require.Equal(t, filepath.Join(base, path), SmartJoin(base, path)) | ||
| }) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Too many tests for a single regex.
There are 4 functions and about 40 subcases, while SmartJoin only calls IsAbs.
- A single {path, wantAbs} table on
IsAbswould cover everything. t.TempDir()isn't needed for base since nothing touches the disk: a constant is enough.TestSmartJoinUnrelatedTemplatePathsalready passed before this PR, so it can be folded into the table
| package taskfile | ||
|
|
||
| import ( | ||
| "os" | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| func TestFileNodeResolveLiteralSpecialDir(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| dir := t.TempDir() | ||
| entrypoint := filepath.Join(dir, "Taskfile.yml") | ||
| require.NoError(t, os.WriteFile(entrypoint, []byte("version: '3'\n"), 0o600)) | ||
| node, err := NewFileNode(entrypoint, "") | ||
| require.NoError(t, err) | ||
|
|
||
| for _, name := range []string{"project.ROOT_DIR", "project.TASKFILE_DIR", "project.USER_WORKING_DIR"} { | ||
| t.Run(name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| resolvedDir, err := node.ResolveDir(name) | ||
| require.NoError(t, err) | ||
| require.Equal(t, filepath.Join(dir, name), resolvedDir) | ||
|
|
||
| resolvedEntrypoint, err := node.ResolveEntrypoint(filepath.Join(name, "Taskfile.yml")) | ||
| require.NoError(t, err) | ||
| require.Equal(t, filepath.Join(dir, name, "Taskfile.yml"), resolvedEntrypoint) | ||
| }) | ||
| } | ||
| } |
Match complete template actions separately from special directory names, keeping Unicode identifier boundaries and incomplete-template handling. Consolidate path classification coverage into one IsAbs table and remove the redundant taskfile integration test requested during review.
Fixes #3021
Description
SmartJoinincorrectly treats ordinary relative paths containing special variable names such as.ROOT_DIRas absolute/special paths, causing them to bypass the base-directory join.The path should be treated as a normal relative path and joined with
base.From looking at the implementation, this may be caused by
isSpecialDirusingstrings.Containsto detect special variables. As a result, an ordinary directory name containing one of these strings can be matched even when the variable is not actually part of a{{ ... }}template expression.A small portion of the code and tests was generated with assistance from Codex.
Checklist