Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Correctly handle URIs like c: on Windows #4606

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Correctly handle URIs like c: on Windows #4606

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
785ca8a
Correctly handle URI with paths like c: in Windows
Roemer Feb 2, 2024
61e9d18
Added path test for Windows
Roemer Feb 3, 2024
f1a2533
Moved regexp to variable
Roemer Feb 3, 2024
fc92682
Return Windows compatible Path()
Roemer Feb 3, 2024
b6a83e3
Fix parse tests
Roemer Feb 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions storage/uri_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ func TestURIPath(t *testing.T) {
u, err = storage.ParseURI(s)
assert.Nil(t, err)
assert.Equal(t, "example:animal:ferret:nose", u.Path())

s = "file:///C:/over/there"
u, err = storage.ParseURI(s)
assert.Nil(t, err)
assert.Equal(t, "/C:/over/there", u.Path())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a path like this for Windows is wrong. The logic needs to be updated in the Path function to strip out the extra slash for file:// urls on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I added the logic accordingly. There are more and more if runtime.GOOS == "windows" and even more will come when UNC will be supported as well but I don't see a way around this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, a question I guess (and maybe this is one for @andydotxyz) - do we want to use runtime.GOOS as the trigger or do we want to handle "windows-looking" file URLs the same (ie correctly) regardless of what OS we're running on?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is discussed, the handling of UNC paths on Windows should also be discussed.
Simple example:
file://server/path in Windows almost always means that any file operation then should be done on \\server\path, which is the UNC path build out of this URI. All os and io methods on Windows can handle the UNC path correctly but would fail with server/path.

}

func TestURIQuery(t *testing.T) {
Expand Down