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

blob/fileblob: ignore URL's Host and drop leading / on Windows #644

Merged
merged 6 commits into from
Nov 7, 2018
Merged

blob/fileblob: ignore URL's Host and drop leading / on Windows #644

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
512daf7
blob/fileblob: ignore URL's Host and drop leading / on Windows
vangent Nov 7, 2018
51a33b0
actually use path variable
vangent Nov 7, 2018
22450f9
drop travis change of adding windows
vangent Nov 7, 2018
3b33b26
update example to use url.PathEscape
vangent Nov 7, 2018
8bfa193
Merge branch 'master' into escapequery
vangent Nov 7, 2018
6539e14
Merge branch 'master' into escapequery
vangent Nov 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion blob/example_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ import (
"io"
"io/ioutil"
"log"
"net/url"
"os"
"path/filepath"
"strings"

"github.com/google/go-cloud/blob"
"github.com/google/go-cloud/blob/fileblob"
Expand Down Expand Up @@ -335,7 +337,15 @@ func ExampleOpen() {
}
fmt.Println("Got expected error opening a nonexistent path")

if _, err := blob.Open(ctx, "file://"+dir); err != nil {
// Ensure the path has a leading slash; fileblob ignores the URL's
// Host field, so URLs should always start with "file:///". On
// Windows, the leading "/" will be stripped, so "file:///c:/foo"
// will refer to c:/foo.
urlpath := url.PathEscape(filepath.ToSlash(dir))
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we handle converting separators to '/' in package fileblob instead of letting user to it? Is "c:\foo\bar" invalid?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No; according to this, even on Windows, URLs referring to files should use "/" as a path separator. So doing this on the user side is correct.

That said, it might work either way; i.e., if they pass file:///c:\foo\bar.txt, it might work. "" is definitely illegal in the host part, which is why the test was failing before, but it might be legal in the path part, and we are just passing the path through to OpenBucket. I don't want to document it as working though., and the example should do the correct thing.

if !strings.HasPrefix(urlpath, "/") {
urlpath = "/" + urlpath
}
if _, err := blob.Open(ctx, "file://"+urlpath); err != nil {
log.Fatal(err)
}
fmt.Println("Got a bucket for valid path")
Expand Down
15 changes: 11 additions & 4 deletions blob/fileblob/fileblob.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,12 @@
// aren't visible using fileblob.
//
// For blob.Open URLs, fileblob registers for the "file" scheme.
// The URL's Host and Path are concatenated and used as the root directory.
// For example, blob.Open("file:///a/directory") is equivalent to
// fileblob.OpenBucket("/a/directory"). No query options are supported.
// The URL's Path is used as the root directory; the URL's Host is ignored.
// If os.PathSeparator != "/", any leading "/" from the Path is dropped.
// No query options are supported. Examples:
// -- file:///a/directory passes "/a/directory" to OpenBucket.
// -- file://localhost/a/directory also passes "/a/directory".
// -- file:///c:/foo/bar passes "c:/foo/bar".
//
// fileblob does not support any types for As.
package fileblob
Expand All @@ -60,7 +63,11 @@ const defaultPageSize = 1000

func init() {
blob.Register("file", func(_ context.Context, u *url.URL) (driver.Bucket, error) {
return openBucket(u.Host+u.Path, nil)
path := u.Path
if os.PathSeparator != '/' && strings.HasPrefix(path, "/") {
path = path[1:]
}
return openBucket(path, nil)
})
}

Expand Down