Use path.Clean instead of filepath.Clean in iofs.Open#195
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Windows incompatibility in the helper/iofs adapter by switching path normalization in Open() to use path.Clean (io/fs slash semantics) instead of filepath.Clean (OS-specific semantics), and re-enables fstest.TestFS on Windows with an added regression test.
Changes:
- Replace
filepath.Cleanwithpath.CleaninadapterFs.Open()to avoid rejecting valid forward-slash paths on Windows. - Remove the Windows-only skip in
TestWithFSTestsofstest.TestFSruns cross-platform. - Add a regression test to ensure
Open()accepts forward-slash paths on all platforms.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| helper/iofs/iofs.go | Updates Open() path cleaning to be io/fs compliant across platforms. |
| helper/iofs/iofs_test.go | Removes Windows skip for fstest.TestFS and adds a forward-slash regression test. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
pjbgf
left a comment
There was a problem hiding this comment.
@puerco thanks for proposing this change, overall LGTM. Please rebase and fix the linting issue:
/home/runner/work/go-billy/go-billy/build/tools/golangci-lint-v2.11.1 run
Error: helper/iofs/iofs_test.go:48:6: TestOpenForwardSlashPath's subtests should call t.Parallel (tparallel)
func TestOpenForwardSlashPath(t *testing.T) {
^
|
Thanks @pjbgf I've added the missing t,Parallel and the linter is now passing :) |
|
Ugh let me get access to an actual windows machine (🤢) to see what's going on with the tests and I'll chime back |
This commit uses fs.ValidPath to verify the paths passed to Open() instead of trying to implement the same checks in the Open() method. Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>
Signed-off-by: Adolfo García Veytia (Puerco) <adolfo.garcia@uservers.net>
Signed-off-by: Adolfo García Veytia (Puerco) <puerco@carabiner.dev>
|
OK, I've fixed the core issue where running on windows would reject simple paths like Unfortunately at the core of the problem there seems to be a fundamental incompatibility between the adapter and memfs. In io.FS, a path like I think this is what using Clean() was trying to handle (overzealously). We now check the paths using fs.ValidPath() (instead if the partial logic checking for absolute paths, etc) but to handle the above incompatibility (and scope the check to only that), we reject any path with a backslash as io.FS and memfs interpret them differently. The tests now run on all platforms and are passing on all: |
|
Here is the cherry pick in case you think it's a candidate for backporting: |
As detailed in #194, this PR updates the iofs adapter Open() method to remove the platform specific filepath.Clean() call when validating paths. Since io.FS requires all slashes to be forward slashes regardless of platform, the Open() validation is failing as on windows as
filepath.Clean()returns backslashes:path string: signature/signature.json
filepath.Clean(): signature\signature.json (on windows, interpreted by fs.FS as a file called "signature\signature.json" )In order to handle the exception this check was trying to prevent (see note below), we check and reject attempts to open files with backslashes in them which was already done inconsistently across platforms with the Clean() check.
This also removes a test skip that was not running the tests on windows because of the same bug.
I've added a regression check to force checking paths.
Closes #194