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.

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

Correctly handle URIs like c: on Windows #4606

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions internal/repository/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"os"
"path"
"path/filepath"
"regexp"
"runtime"
"strings"

"fyne.io/fyne/v2"
Expand All @@ -16,6 +18,9 @@ import (
// for string processing
const fileSchemePrefix string = "file://"

// Regex pattern to match Windows drive paths
Copy link
Member

Choose a reason for hiding this comment

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

it's a drive identifier isn't it - not a path?

var windowsDrivePathPattern = regexp.MustCompile(`(?i)^/[a-z]:$`)

// declare conformance with repository types
var _ repository.Repository = (*FileRepository)(nil)
var _ repository.WritableRepository = (*FileRepository)(nil)
Expand Down Expand Up @@ -171,6 +176,11 @@ func (r *FileRepository) Parent(u fyne.URI) (fyne.URI, error) {
// trim the scheme
s = strings.TrimPrefix(s, fileSchemePrefix)

// Only for Windows: If the path is a drive root, no parent is possible
if runtime.GOOS == "windows" && windowsDrivePathPattern.MatchString(s) {
return nil, repository.ErrURIRoot
}

// Completely empty URI with just a scheme
if s == "" {
return nil, repository.ErrURIRoot
Expand Down
8 changes: 8 additions & 0 deletions storage/repository/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package repository
import (
"errors"
"path/filepath"
"regexp"
"runtime"
"strings"

Expand All @@ -11,6 +12,9 @@ import (
"fyne.io/fyne/v2"
)

// Pattern to find Windows drive paths
var windowsDrivePrefixhPattern = regexp.MustCompile(`(?i)^[a-z]:`)
Copy link
Member

Choose a reason for hiding this comment

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

is prefixh a typo?


// NewFileURI implements the back-end logic to storage.NewFileURI, which you
// should use instead. This is only here because other functions in repository
// need to call it, and it prevents a circular import.
Expand All @@ -22,6 +26,10 @@ func NewFileURI(path string) fyne.URI {
// or NT style paths, with / or \, but when we reconstruct
// the URI, we want to have / only.
if runtime.GOOS == "windows" {
// Make sure that Windows paths (eg "c:\") also correctly start with a "/"
if windowsDrivePrefixhPattern.MatchString(path) {
path = "/" + path
}
// seems that sometimes we end up with
// double-backslashes
path = filepath.ToSlash(path)
Expand Down
6 changes: 3 additions & 3 deletions storage/repository/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (

func TestNewFileURI(t *testing.T) {
assert.Equal(t, "file:///tmp/foo.txt", NewFileURI("/tmp/foo.txt").String())
assert.Equal(t, "file://C:/tmp/foo.txt", NewFileURI("C:/tmp/foo.txt").String())
assert.Equal(t, "file:///C:/tmp/foo.txt", NewFileURI("C:/tmp/foo.txt").String())
}

func TestParseURI(t *testing.T) {
Expand All @@ -20,9 +20,9 @@ func TestParseURI(t *testing.T) {
assert.Nil(t, err)
assert.Equal(t, "file:///tmp/foo.txt", uri.String())

uri, err = ParseURI("file://C:/tmp/foo.txt")
Copy link
Member

Choose a reason for hiding this comment

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

You've stated that legacy URI will still work - so please leave the old test in so we can ensure it stays working.

uri, err = ParseURI("file:///C:/tmp/foo.txt")
assert.Nil(t, err)
assert.Equal(t, "file://C:/tmp/foo.txt", uri.String())
assert.Equal(t, "file:///C:/tmp/foo.txt", uri.String())
}

func TestParseInvalidURI(t *testing.T) {
Expand Down
8 changes: 8 additions & 0 deletions storage/repository/uri.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"bufio"
"mime"
"path/filepath"
"regexp"
"runtime"
"strings"
"unicode/utf8"

Expand All @@ -13,6 +15,9 @@ import (
// Declare conformance with fyne.URI interface.
var _ fyne.URI = &uri{}

// Regex pattern to match Windows drive paths
Copy link
Member

Choose a reason for hiding this comment

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

This probably needs a note about them being URI related or the slash in the middle may be unobvious or look like duplication of the other prefix pattern

var windowsDrivePathPrefixPattern = regexp.MustCompile(`(?i)^/[a-z]:`)

type uri struct {
scheme string
authority string
Expand Down Expand Up @@ -76,6 +81,9 @@ func (u *uri) Authority() string {
}

func (u *uri) Path() string {
if runtime.GOOS == "windows" && windowsDrivePathPrefixPattern.MatchString(u.path) {
return u.path[1:]
}
return u.path
}

Expand Down
44 changes: 26 additions & 18 deletions storage/uri_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,16 @@ 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)
if runtime.GOOS == "windows" {
// Windows should not have that leading slash
assert.Equal(t, "C:/over/there", u.Path())
} else {
assert.Equal(t, "/C:/over/there", u.Path())
}
}

func TestURIQuery(t *testing.T) {
Expand Down Expand Up @@ -114,7 +124,7 @@ func TestURI_Scheme(t *testing.T) {
func TestURI_Name(t *testing.T) {
assert.Equal(t, "text.txt", storage.NewURI("file:///text.txt").Name())
assert.Equal(t, "library.a", storage.NewURI("file:///library.a").Name())
assert.Equal(t, "directory", storage.NewURI("file://C:/directory/").Name())
assert.Equal(t, "directory", storage.NewURI("file:///C:/directory/").Name())
assert.Equal(t, "image.JPEG", storage.NewFileURI("C:/image.JPEG").Name())
}

Expand All @@ -130,9 +140,9 @@ func TestURI_Parent(t *testing.T) {
assert.Nil(t, err)
assert.Equal(t, "file:///foo/bar/", parent.String())

parent, err = storage.Parent(storage.NewURI("file://C:/foo/bar/baz/"))
parent, err = storage.Parent(storage.NewURI("file:///C:/foo/bar/baz/"))
assert.Nil(t, err)
assert.Equal(t, "file://C:/foo/bar/", parent.String())
assert.Equal(t, "file:///C:/foo/bar/", parent.String())

// TODO hook in an http/https handler
//parent, err = storage.Parent(storage.NewURI("http://foo/bar/baz/"))
Expand All @@ -156,14 +166,17 @@ func TestURI_Parent(t *testing.T) {
if runtime.GOOS == "windows" {
// Only the Windows version of filepath will know how to handle
// backslashes.
uri := storage.NewURI("file://C:\\foo\\bar\\baz\\")
assert.Equal(t, "file://C:/foo/bar/baz/", uri.String())
uri := storage.NewURI("file:///C:\\foo\\bar\\baz\\")
assert.Equal(t, "file:///C:/foo/bar/baz/", uri.String())
uri = storage.NewFileURI("C:\\foo\\bar\\baz\\")
assert.Equal(t, "file://C:/foo/bar/baz/", uri.String())
assert.Equal(t, "file:///C:/foo/bar/baz/", uri.String())
// This is the legacy format but we still accept it
uri = storage.NewURI("file://C:\\foo\\bar\\baz\\")
assert.Equal(t, "file:///C:/foo/bar/baz/", uri.String())

parent, err = storage.Parent(uri)
assert.Nil(t, err)
assert.Equal(t, "file://C:/foo/bar/", parent.String())
assert.Equal(t, "file:///C:/foo/bar/", parent.String())
}

_, err = storage.Parent(storage.NewURI("file:///"))
Expand All @@ -176,20 +189,15 @@ func TestURI_Parent(t *testing.T) {

// This should cause an error, since this is a Windows-style
// path and thus we can't get the parent of a drive letter.
_, err = storage.Parent(storage.NewURI("file://C:/"))
_, err = storage.Parent(storage.NewURI("file:///C:/"))
assert.Equal(t, repository.ErrURIRoot, err)
}

// Windows supports UNIX-style paths. /C:/ is also a valid path.
parent, err = storage.Parent(storage.NewURI("file:///C:/"))
assert.Nil(t, err)
assert.Equal(t, "file:///", parent.String())
}

func TestURI_Extension(t *testing.T) {
assert.Equal(t, ".txt", storage.NewURI("file:///text.txt").Extension())
assert.Equal(t, ".JPEG", storage.NewURI("file://C:/image.JPEG").Extension())
assert.Equal(t, "", storage.NewURI("file://C:/directory/").Extension())
assert.Equal(t, ".JPEG", storage.NewURI("file:///C:/image.JPEG").Extension())
assert.Equal(t, "", storage.NewURI("file:///C:/directory/").Extension())
assert.Equal(t, ".a", storage.NewFileURI("/library.a").Extension())
}

Expand All @@ -203,11 +211,11 @@ func TestURI_Child(t *testing.T) {
if runtime.GOOS == "windows" {
// Only the Windows version of filepath will know how to handle
// backslashes.
uri := storage.NewURI("file://C:\\foo\\bar\\")
assert.Equal(t, "file://C:/foo/bar/", uri.String())
uri := storage.NewURI("file:///C:\\foo\\bar\\")
assert.Equal(t, "file:///C:/foo/bar/", uri.String())

p, _ = storage.Child(uri, "baz")
assert.Equal(t, "file://C:/foo/bar/baz", p.String())
assert.Equal(t, "file:///C:/foo/bar/baz", p.String())
}
}

Expand Down