laskoviymishka commented on code in PR #1578:
URL: https://github.com/apache/iceberg-go/pull/1578#discussion_r3719821207
##########
io/local.go:
##########
@@ -28,16 +30,76 @@ import (
// the local file system.
type LocalFS struct{}
+func localPath(name string) (string, error) {
+ if filepath.VolumeName(name) != "" {
+ return name, nil
+ }
+
+ schemeEnd := strings.IndexByte(name, ':')
+ firstSeparator := strings.IndexAny(name, `/\`)
+ if schemeEnd < 0 || (firstSeparator >= 0 && firstSeparator < schemeEnd)
{
+ return name, nil
+ }
+ scheme := name[:schemeEnd]
+ // A colon is valid in a native POSIX filename. Treat non-file names as
+ // URIs only when they have an authority delimiter, preserving paths
such as
+ // "partition:2026/data.parquet" while still rejecting s3:// and similar
+ // locations passed directly to LocalFS.
+ if !strings.EqualFold(scheme, "file") &&
!strings.HasPrefix(name[schemeEnd+1:], "//") {
+ return name, nil
+ }
+ if !strings.EqualFold(scheme, "file") {
+ return "", fmt.Errorf("unsupported local filesystem scheme %q",
scheme)
Review Comment:
Not a blocker, but while we're here: this error and the authority error just
below are plain `fmt.Errorf` strings, so a caller doing `errors.Is(err,
fs.ErrNotExist)` to spot a missing path now gets a different answer than the
old `TrimPrefix` path did, and there's no sentinel to match on if catalog code
wants to detect "this isn't a local path, fall back" without string-matching.
If we expect anyone to branch on these, I'd define a sentinel and wrap with
`%w`; if not, a one-line comment that the string is the contract is enough.
wdyt?
##########
io/local.go:
##########
@@ -28,16 +30,76 @@ import (
// the local file system.
type LocalFS struct{}
+func localPath(name string) (string, error) {
+ if filepath.VolumeName(name) != "" {
+ return name, nil
+ }
+
+ schemeEnd := strings.IndexByte(name, ':')
+ firstSeparator := strings.IndexAny(name, `/\`)
+ if schemeEnd < 0 || (firstSeparator >= 0 && firstSeparator < schemeEnd)
{
+ return name, nil
+ }
+ scheme := name[:schemeEnd]
+ // A colon is valid in a native POSIX filename. Treat non-file names as
+ // URIs only when they have an authority delimiter, preserving paths
such as
+ // "partition:2026/data.parquet" while still rejecting s3:// and similar
+ // locations passed directly to LocalFS.
+ if !strings.EqualFold(scheme, "file") &&
!strings.HasPrefix(name[schemeEnd+1:], "//") {
+ return name, nil
+ }
+ if !strings.EqualFold(scheme, "file") {
+ return "", fmt.Errorf("unsupported local filesystem scheme %q",
scheme)
+ }
+
+ parsed, err := url.Parse(name)
+ if err != nil {
+ return "", fmt.Errorf("invalid local file path %q: %w", name,
err)
+ }
+ if parsed.Host != "" && !strings.EqualFold(parsed.Host, "localhost") {
+ return "", fmt.Errorf("unsupported file URI authority %q",
parsed.Host)
+ }
+ if parsed.Opaque != "" {
+ return filepath.FromSlash(parsed.Opaque), nil
Review Comment:
The two file-URI forms decode differently here.
`file:///path/to/my%20file.parquet` goes through `parsed.Path`, which
`url.Parse` percent-decodes, so we get `/path/to/my file.parquet` (a real
space). But `file:/path/to/my%20file.parquet` (single slash, no authority)
lands in `parsed.Opaque`, which is raw, so we hand back the literal `%20`. Same
file per RFC 8089, two different OS paths out of `localPath`.
At minimum the opaque branch should match the path branch:
`url.PathUnescape(parsed.Opaque)` and handle the error. The bigger question of
whether we should be decoding at all (a raw OS path with a literal `%` that got
concatenated into a `file://` URL now resolves somewhere else) I've put in the
top-level comment, but the two forms disagreeing with each other is a bug
either way.
##########
io/local.go:
##########
@@ -28,16 +30,76 @@ import (
// the local file system.
type LocalFS struct{}
+func localPath(name string) (string, error) {
+ if filepath.VolumeName(name) != "" {
+ return name, nil
+ }
+
+ schemeEnd := strings.IndexByte(name, ':')
+ firstSeparator := strings.IndexAny(name, `/\`)
+ if schemeEnd < 0 || (firstSeparator >= 0 && firstSeparator < schemeEnd)
{
+ return name, nil
+ }
+ scheme := name[:schemeEnd]
+ // A colon is valid in a native POSIX filename. Treat non-file names as
+ // URIs only when they have an authority delimiter, preserving paths
such as
+ // "partition:2026/data.parquet" while still rejecting s3:// and similar
+ // locations passed directly to LocalFS.
+ if !strings.EqualFold(scheme, "file") &&
!strings.HasPrefix(name[schemeEnd+1:], "//") {
+ return name, nil
+ }
+ if !strings.EqualFold(scheme, "file") {
+ return "", fmt.Errorf("unsupported local filesystem scheme %q",
scheme)
+ }
+
+ parsed, err := url.Parse(name)
+ if err != nil {
+ return "", fmt.Errorf("invalid local file path %q: %w", name,
err)
+ }
+ if parsed.Host != "" && !strings.EqualFold(parsed.Host, "localhost") {
Review Comment:
I think this regresses Windows file URIs. On Windows, `file://C:/warehouse`
parses with `parsed.Host == "C"`, so it hits this branch and errors out, where
the old `TrimPrefix` gave `C:/warehouse`, which `os.Open` handles fine. And
`file:///C:/warehouse` parses to `parsed.Path == "/C:/warehouse"`, and that
leading slash before the drive letter isn't a valid Windows path either.
The new `TestLocalFSParsesFileURIs` builds `"file://" +
filepath.ToSlash(path)`, so on a Windows runner it'd construct `file://C:/...`
and fail on this exact line. I'd special-case a single-letter drive host
(rebuild as `host + ":" + path`) and strip the leading slash before a drive
letter. Do we run CI on Windows for this package?
##########
io/local_test.go:
##########
@@ -93,32 +93,112 @@ func TestLocalFSWriteFileCreatesParentDirectories(t
*testing.T) {
content := []byte("content")
for _, tt := range []struct {
- name string
- path string
+ name string
+ path string
+ readPath string
}{
{
- name: "plain path",
- path: filepath.Join(dir, "plain", "nested", "file.txt"),
+ name: "plain path",
+ path: filepath.Join(dir, "plain", "nested",
"file.txt"),
+ readPath: filepath.Join(dir, "plain", "nested",
"file.txt"),
},
{
- name: "file scheme",
- path: "file://" + filepath.Join(dir, "scheme",
"nested", "file.txt"),
+ name: "file scheme",
+ path: fileURI(filepath.Join(dir, "scheme",
"nested", "file.txt")),
+ readPath: filepath.Join(dir, "scheme", "nested",
"file.txt"),
},
} {
t.Run(tt.name, func(t *testing.T) {
require.NoError(t, LocalFS{}.WriteFile(tt.path,
content))
- got, err := os.ReadFile(strings.TrimPrefix(tt.path,
"file://"))
+ got, err := os.ReadFile(tt.readPath)
require.NoError(t, err)
assert.Equal(t, content, got)
})
}
Review Comment:
Good test to have, but it only exercises `ReadFile` and only over `TempDir`
paths, which never contain percent-encodeable characters, so the opaque-vs-path
decode gap ships untested.
I'd add a case that writes a file whose name has a literal space, then reads
it back through both `file:/...` and `file:///...` (percent-encoded), asserting
they resolve to the same file. And since every write method now routes through
`localPath` too, at least one `WriteFile`/`Create` round-trip through a
`file://` URI would catch a wrong decoded path before it silently writes to the
wrong place.
##########
io/local.go:
##########
@@ -55,42 +120,92 @@ func (LocalFS) WriteFile(name string, content []byte)
error {
}
func (LocalFS) Remove(name string) error {
- return os.Remove(strings.TrimPrefix(name, "file://"))
+ path, err := localPath(name)
+ if err != nil {
+ return err
+ }
+
+ return os.Remove(path)
}
func (LocalFS) RemoveAll(name string) error {
- return os.RemoveAll(strings.TrimPrefix(name, "file://"))
+ path, err := localPath(name)
+ if err != nil {
+ return err
+ }
+
+ return os.RemoveAll(path)
}
func (LocalFS) WalkDir(root string, fn fs.WalkDirFunc) error {
- return filepath.WalkDir(strings.TrimPrefix(root, "file://"), fn)
+ path, err := localPath(root)
+ if err != nil {
+ return err
+ }
+
+ return filepath.WalkDir(path, fn)
}
func (LocalFS) ReadDir(name string) ([]fs.DirEntry, error) {
- return os.ReadDir(strings.TrimPrefix(name, "file://"))
+ path, err := localPath(name)
+ if err != nil {
+ return nil, err
+ }
+
+ return os.ReadDir(path)
}
-func (LocalFS) MkdirAll(path string) error {
- return os.MkdirAll(strings.TrimPrefix(path, "file://"), 0o755)
+func (LocalFS) MkdirAll(name string) error {
+ path, err := localPath(name)
+ if err != nil {
+ return err
+ }
+
+ return os.MkdirAll(path, 0o755)
}
-func (LocalFS) Mkdir(path string) error {
- return os.Mkdir(strings.TrimPrefix(path, "file://"), 0o755)
+func (LocalFS) Mkdir(name string) error {
+ path, err := localPath(name)
+ if err != nil {
+ return err
+ }
+
+ return os.Mkdir(path, 0o755)
Review Comment:
Small readability thing: this shadows the `oldpath` param with `:=`, then
the next line reuses `err` with `=`, which is easy to misread. Every other
method in the file uses distinct names, so I'd do the same here:
```go
src, err := localPath(oldpath)
if err != nil {
return err
}
dst, err := localPath(newpath)
if err != nil {
return err
}
return os.Rename(src, dst)
```
Same pattern in `RenameNoReplace` just below.
##########
io/local_test.go:
##########
@@ -93,32 +93,112 @@ func TestLocalFSWriteFileCreatesParentDirectories(t
*testing.T) {
content := []byte("content")
for _, tt := range []struct {
- name string
- path string
+ name string
+ path string
+ readPath string
}{
{
- name: "plain path",
- path: filepath.Join(dir, "plain", "nested", "file.txt"),
+ name: "plain path",
+ path: filepath.Join(dir, "plain", "nested",
"file.txt"),
+ readPath: filepath.Join(dir, "plain", "nested",
"file.txt"),
},
{
- name: "file scheme",
- path: "file://" + filepath.Join(dir, "scheme",
"nested", "file.txt"),
+ name: "file scheme",
+ path: fileURI(filepath.Join(dir, "scheme",
"nested", "file.txt")),
+ readPath: filepath.Join(dir, "scheme", "nested",
"file.txt"),
},
} {
t.Run(tt.name, func(t *testing.T) {
require.NoError(t, LocalFS{}.WriteFile(tt.path,
content))
- got, err := os.ReadFile(strings.TrimPrefix(tt.path,
"file://"))
+ got, err := os.ReadFile(tt.readPath)
require.NoError(t, err)
assert.Equal(t, content, got)
})
}
}
+func TestLocalFSParsesFileURIs(t *testing.T) {
+ t.Parallel()
+
+ dir := t.TempDir()
+ path := filepath.Join(dir, "metadata.json")
Review Comment:
Wrapping each iteration in `t.Run(name, ...)` would tell us which URI form
broke when one fails. Right now a failure just points at the outer test, which
matters more once the Windows forms are in the mix.
`TestLocalFSWriteFileCreatesParentDirectories` just above already does this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
