laskoviymishka commented on code in PR #1259:
URL: https://github.com/apache/iceberg-go/pull/1259#discussion_r3466850027
##########
io/gocloud/blob.go:
##########
@@ -69,22 +68,70 @@ func (f *blobOpenFile) Sys() any { return
f.b }
func (f *blobOpenFile) IsDir() bool { return false }
func (f *blobOpenFile) Stat() (fs.FileInfo, error) { return f, nil }
-// KeyExtractor extracts the object key from an input path
+// KeyExtractor extracts the object key from an input path.
type KeyExtractor func(path string) (string, error)
-// defaultKeyExtractor extracts the object key by removing the scheme and
bucket name from the URI
-// e.g., s3://bucket/path/file -> path/file
+// ErrEmptyObjectKey is returned when a location names a bucket or authority
+// without an object key.
+var ErrEmptyObjectKey = errors.New("object key is empty")
+
+type objectLocation struct {
+ authority string
+ key string
+ uriPrefix string
+ hasAuthority bool
+}
+
+func splitObjectLocation(location string) (objectLocation, error) {
+ scheme, rest, ok := strings.Cut(location, "://")
+ if !ok {
+ return objectLocation{key: location}, nil
+ }
+
+ authorityEnd := strings.IndexAny(rest, "/?#")
+ if authorityEnd == -1 {
+ authorityEnd = len(rest)
+ }
+
+ authority := rest[:authorityEnd]
+ if authority == "" {
+ return objectLocation{}, fmt.Errorf("URI authority is empty:
%s", location)
+ }
+
+ key := ""
+ if authorityEnd < len(rest) && rest[authorityEnd] == '/' {
+ key = strings.TrimPrefix(rest[authorityEnd:], "/")
+ }
+
+ return objectLocation{
+ authority: authority,
+ key: key,
+ uriPrefix: scheme + "://" + authority + "/",
+ hasAuthority: true,
+ }, nil
+}
+
+// defaultKeyExtractor extracts the object key by removing the scheme and
bucket name from the URI.
+// e.g., s3://bucket/path/file -> path/file.
func defaultKeyExtractor(bucketName string) KeyExtractor {
return func(location string) (string, error) {
- _, after, found := strings.Cut(location, "://")
- if found {
- location = after
+ parsed, err := splitObjectLocation(location)
+ if err != nil {
+ return "", err
}
- key := strings.TrimPrefix(location, bucketName+"/")
+ key := parsed.key
+ if parsed.hasAuthority {
+ if parsed.authority != bucketName {
Review Comment:
My one real hesitation about merging this: the hard authority check runs on
the default read path, and I think it breaks a supported Iceberg layout.
A table can set `write.data.path = s3://data-bucket/` while its location and
metadata live in `s3://metadata-bucket/...`. Manifests then carry full URIs
like `s3://data-bucket/data/00000.parquet`. As I read it, the scan opens those
through the single table IO keyed to the metadata bucket — so
`parsed.authority` is `data-bucket`, `bucketName` is `metadata-bucket`, and we
now reject a file that Java `S3FileIO` and PyIceberg read without complaint
(neither restricts which bucket a path resolves to).
I'd keep the validation but make it not the default on reads — either
construct a per-authority IO when a data-file URI's authority differs from the
table's, or gate strict equality behind an opt-in property. Before I lean too
hard on this though: can you confirm the scan path really does reuse one IO
across differing data-path authorities? If it already loads an FS per data
path, this concern mostly evaporates and I'd drop it to a nit. wdyt?
##########
io/gocloud/blob.go:
##########
@@ -193,20 +240,30 @@ func createBlobFS(ctx context.Context, bucket
*blob.Bucket, keyExtractor KeyExtr
}
func (bfs *blobFileIO) WalkDir(root string, fn fs.WalkDirFunc) error {
- parsed, err := url.Parse(root)
+ walkPath, err := bfs.preprocess(root)
if err != nil {
- return fmt.Errorf("invalid URL %s: %w", root, err)
- }
+ if !errors.Is(err, ErrEmptyObjectKey) {
+ return &fs.PathError{Op: "walkdir", Path: root, Err:
err}
+ }
- walkPath := strings.TrimPrefix(parsed.Path, "/")
- if walkPath == "" {
walkPath = "."
}
- parsed.Path = ""
+ location, err := splitObjectLocation(root)
Review Comment:
We parse `root` twice here — once via `preprocess` (which runs
`keyExtractor`) and again directly through `splitObjectLocation` — then use the
second parse's `uriPrefix` to reconstruct the callback paths.
That's fine for `defaultKeyExtractor`, but for any extractor whose authority
boundary differs from the naive up-to-`/?#` cut — `adlsKeyExtractor` today, or
any custom one — the prefix used for reconstruction can diverge from what the
extractor actually accepted. The two parsers disagreeing is exactly the kind of
bug that stays invisible until someone plugs in a non-default extractor.
I'd parse once and share the `objectLocation` for both the key extraction
and the prefix reconstruction, so there's a single source of truth for the
authority boundary.
##########
io/gocloud/blob_test.go:
##########
@@ -229,13 +333,67 @@ func TestBlobFileIOWalkDir(t *testing.T) {
require.NoError(t, err)
expected := []string{
+ "s3://test-bucket/data/100%off/file.parquet",
+ "s3://test-bucket/data/city=New York/file.parquet",
"s3://test-bucket/data/file1.parquet",
"s3://test-bucket/data/file2.parquet",
"s3://test-bucket/metadata/snap-123.avro",
}
assert.ElementsMatch(t, expected, walked)
}
+func TestBlobFileIOWalkDirRejectsWrongBucket(t *testing.T) {
+ ctx := context.Background()
+
+ bucket := memblob.OpenBucket(nil)
+ defer bucket.Close()
+
+ require.NoError(t, bucket.WriteAll(ctx, "data/file1.parquet",
[]byte("content"), nil))
+
+ bfs := &blobFileIO{
+ Bucket: bucket,
+ keyExtractor: defaultKeyExtractor("test-bucket"),
+ ctx: ctx,
+ }
+
+ for _, tt := range []struct {
+ name string
+ root string
+ }{
+ {name: "s3", root: "s3://other-bucket/"},
+ {name: "gcs", root: "gs://other-bucket/data"},
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ err := bfs.WalkDir(tt.root, func(string, fs.DirEntry,
error) error {
+ t.Fatal("WalkDir callback should not be called")
+
+ return nil
+ })
+ require.ErrorContains(t, err, "does not match
configured authority")
+ })
+ }
+}
+
+func TestBlobFileIOWalkDirRejectsWrongAzureAuthority(t *testing.T) {
+ ctx := context.Background()
+
+ bucket := memblob.OpenBucket(nil)
+ defer bucket.Close()
+
+ bfs := &blobFileIO{
+ Bucket: bucket,
+ keyExtractor:
defaultKeyExtractor("[email protected]"),
Review Comment:
This test doesn't actually exercise the Azure path. Production Azure/ADLS
schemes are registered with `adlsKeyExtractor` (register.go), which extracts
the path but never compares the authority — so
`WriteFile("abfs://other-container@account.../data/file.parquet")` still
silently lands in the opened container with the old bug intact.
The test only rejects because it wires up `defaultKeyExtractor` here, so
it's proving the S3 extractor's behavior under an Azure-looking string rather
than the real Azure code path. As written it implies a cross-container
protection that doesn't exist.
I'd either add a configured authority + mismatch check to `adlsKeyExtractor`
and point this test at it, or drop the test and note in the PR that ADLS
authority validation is out of scope for now.
##########
io/gocloud/blob.go:
##########
@@ -69,22 +68,70 @@ func (f *blobOpenFile) Sys() any { return
f.b }
func (f *blobOpenFile) IsDir() bool { return false }
func (f *blobOpenFile) Stat() (fs.FileInfo, error) { return f, nil }
-// KeyExtractor extracts the object key from an input path
+// KeyExtractor extracts the object key from an input path.
type KeyExtractor func(path string) (string, error)
-// defaultKeyExtractor extracts the object key by removing the scheme and
bucket name from the URI
-// e.g., s3://bucket/path/file -> path/file
+// ErrEmptyObjectKey is returned when a location names a bucket or authority
+// without an object key.
+var ErrEmptyObjectKey = errors.New("object key is empty")
+
+type objectLocation struct {
+ authority string
+ key string
+ uriPrefix string
+ hasAuthority bool
+}
+
+func splitObjectLocation(location string) (objectLocation, error) {
+ scheme, rest, ok := strings.Cut(location, "://")
+ if !ok {
+ return objectLocation{key: location}, nil
+ }
+
+ authorityEnd := strings.IndexAny(rest, "/?#")
+ if authorityEnd == -1 {
+ authorityEnd = len(rest)
+ }
+
+ authority := rest[:authorityEnd]
+ if authority == "" {
+ return objectLocation{}, fmt.Errorf("URI authority is empty:
%s", location)
+ }
+
+ key := ""
Review Comment:
A malformed authority-only URI quietly degrades into a full bucket walk,
which surprised me. For `s3://my-bucket?foo/bar`, `IndexAny` stops at `?`, the
authority is `my-bucket`, and since `rest[authorityEnd]` is `?` not `/`, `key`
stays empty. That bubbles up as `ErrEmptyObjectKey`, which `WalkDir` then
treats as a bucket root and promotes to `walkPath = "."` — walking the entire
bucket from a malformed input.
Iceberg locations shouldn't carry a query or fragment, so this is an edge,
but silently turning a bad URI into a full walk feels risky. I'd either reject
a non-`/` continuation after the authority explicitly, or at least keep that
case from reaching the walk-everything fallback.
##########
io/gocloud/blob.go:
##########
@@ -193,20 +240,30 @@ func createBlobFS(ctx context.Context, bucket
*blob.Bucket, keyExtractor KeyExtr
}
func (bfs *blobFileIO) WalkDir(root string, fn fs.WalkDirFunc) error {
- parsed, err := url.Parse(root)
+ walkPath, err := bfs.preprocess(root)
if err != nil {
- return fmt.Errorf("invalid URL %s: %w", root, err)
- }
+ if !errors.Is(err, ErrEmptyObjectKey) {
+ return &fs.PathError{Op: "walkdir", Path: root, Err:
err}
+ }
- walkPath := strings.TrimPrefix(parsed.Path, "/")
- if walkPath == "" {
walkPath = "."
}
- parsed.Path = ""
+ location, err := splitObjectLocation(root)
+ if err != nil {
+ return &fs.PathError{Op: "walkdir", Path: root, Err: err}
+ }
return fs.WalkDir(bfs.Bucket, walkPath, func(path string, d
fs.DirEntry, err error) error {
- return fn(parsed.JoinPath(path).String(), d, err)
+ if location.hasAuthority {
+ if path == "." {
+ path = strings.TrimSuffix(location.uriPrefix,
"/")
Review Comment:
The root-entry path changed shape here. The old code returned
`parsed.JoinPath(".")` → `s3://bucket/` for the root entry; this returns
`strings.TrimSuffix(uriPrefix, "/")` → `s3://bucket` (no trailing slash), while
every child is `uriPrefix + path` → `s3://bucket/...`. So the root entry no
longer matches the form callers passed in as the walk root.
Current callers all filter with `!d.IsDir()`, so the root `"."` entry is
skipped and nothing breaks today — but it's a silent behavior change waiting
for the first caller that compares a walked entry against the stored root. I'd
use `uriPrefix` consistently for the `"."` case so the root keeps its trailing
slash.
##########
io/gocloud/blob_test.go:
##########
@@ -57,25 +85,60 @@ func TestDefaultKeyExtractor(t *testing.T) {
input: "gs://my-bucket/path/to/file.parquet",
expectedKey: "path/to/file.parquet",
},
+ {
+ name: "azure URI with path",
+ bucketName: "[email protected]",
+ input:
"abfs://[email protected]/path/to/file.parquet",
+ expectedKey: "path/to/file.parquet",
+ },
+ {
+ name: "s3 URI with different bucket",
+ input:
"s3://other-bucket/path/to/file.parquet",
+ wantErrContains: "does not match configured authority",
+ },
+ {
+ name: "gs URI with different bucket",
+ input:
"gs://other-bucket/path/to/file.parquet",
+ wantErrContains: "does not match configured authority",
+ },
+ {
+ name: "azure URI with different container",
+ bucketName:
"[email protected]",
+ input:
"abfs://[email protected]/path/to/file.parquet",
+ wantErrContains: "does not match configured authority",
+ },
{
name: "URI with query and fragment",
input:
"s3://my-bucket/path/to/file.parquet?param=value#fragment",
expectedKey:
"path/to/file.parquet?param=value#fragment",
},
{
- name: "URI with empty path",
- input: "s3://my-bucket/",
- shouldError: true,
+ name: "URI with empty path",
+ input: "s3://my-bucket/",
+ wantErrContains: "object key is empty",
+ wantErrIs: ErrEmptyObjectKey,
+ },
+ {
+ name: "URI with empty authority",
+ input: "s3:///path/to/file.parquet",
+ wantErrContains: "URI authority is empty",
},
}
- extractor := defaultKeyExtractor("my-bucket")
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
+ bucketName := test.bucketName
+ if bucketName == "" {
+ bucketName = "my-bucket"
+ }
+ extractor := defaultKeyExtractor(bucketName)
key, err := extractor(test.input)
- if test.shouldError {
- assert.Error(t, err, "Expected error for input:
%s", test.input)
+ if test.wantErrContains != "" {
+ assert.ErrorContains(t, err,
test.wantErrContains, "Expected error for input: %s", test.input)
+ if test.wantErrIs != nil {
+ assert.True(t, errors.Is(err,
test.wantErrIs), "expected errors.Is(%v)", test.wantErrIs)
Review Comment:
`assert.ErrorIs(t, err, test.wantErrIs)` reads better here and prints the
actual error chain on failure, versus `assert.True` over a bare bool.
Separately, the gating assertions in this table are `assert`, not `require`
— on a nil-when-error-expected case the `ErrorContains` fails but execution
continues into this `errors.Is` check (and on the success path `NoError`
continues into `Equal`), so one root cause produces two failures. I'd make the
gate a `require`.
##########
io/gocloud/blob.go:
##########
@@ -69,22 +68,70 @@ func (f *blobOpenFile) Sys() any { return
f.b }
func (f *blobOpenFile) IsDir() bool { return false }
func (f *blobOpenFile) Stat() (fs.FileInfo, error) { return f, nil }
-// KeyExtractor extracts the object key from an input path
+// KeyExtractor extracts the object key from an input path.
type KeyExtractor func(path string) (string, error)
-// defaultKeyExtractor extracts the object key by removing the scheme and
bucket name from the URI
-// e.g., s3://bucket/path/file -> path/file
+// ErrEmptyObjectKey is returned when a location names a bucket or authority
+// without an object key.
+var ErrEmptyObjectKey = errors.New("object key is empty")
Review Comment:
`ErrEmptyObjectKey` is exported, but its only consumer is the internal
`errors.Is` in `WalkDir` — bucket-root handling is otherwise transparent to
callers. Exporting it is a permanent API commitment, so I'd either unexport it
to `errEmptyObjectKey`, or document it explicitly as part of the `KeyExtractor`
author contract ("custom extractors must wrap this for bucket-root inputs") —
which also ties into the `adlsKeyExtractor` gap.
##########
io/gocloud/blob.go:
##########
@@ -193,20 +240,30 @@ func createBlobFS(ctx context.Context, bucket
*blob.Bucket, keyExtractor KeyExtr
}
func (bfs *blobFileIO) WalkDir(root string, fn fs.WalkDirFunc) error {
- parsed, err := url.Parse(root)
+ walkPath, err := bfs.preprocess(root)
if err != nil {
- return fmt.Errorf("invalid URL %s: %w", root, err)
- }
+ if !errors.Is(err, ErrEmptyObjectKey) {
+ return &fs.PathError{Op: "walkdir", Path: root, Err:
err}
Review Comment:
Small one: the rest of this file uses space-separated op names (`"write
file"`, `"new writer"`), so I'd make this `"walk dir"` to match.
While we're here — the authority-mismatch error is a plain `fmt.Errorf` with
no sentinel, so callers can't `errors.Is` it the way they can
`ErrEmptyObjectKey`. If we expect anyone to branch on it, an exported
`ErrWrongAuthority` wrapped with `%w` would be more consistent. Not blocking.
##########
io/gocloud/blob_test.go:
##########
@@ -84,6 +147,45 @@ func TestDefaultKeyExtractor(t *testing.T) {
}
}
+func TestBlobFileIORejectsWrongBucketObjectPaths(t *testing.T) {
+ ctx := context.Background()
+
+ bucket := memblob.OpenBucket(nil)
+ defer bucket.Close()
+
+ bfs := &blobFileIO{
+ Bucket: bucket,
+ keyExtractor: defaultKeyExtractor("test-bucket"),
+ ctx: ctx,
+ }
+
+ for _, tt := range []struct {
+ name string
+ path string
+ oldKey string
+ }{
+ {
+ name: "s3",
+ path: "s3://other-bucket/data/file.parquet",
+ oldKey: "other-bucket/data/file.parquet",
+ },
+ {
+ name: "gcs",
+ path: "gs://other-bucket/data/file.parquet",
+ oldKey: "other-bucket/data/file.parquet",
+ },
+ } {
+ t.Run(tt.name, func(t *testing.T) {
+ err := bfs.WriteFile(tt.path, []byte("content"))
+ require.ErrorContains(t, err, "does not match
configured authority")
+
+ exists, err := bucket.Exists(ctx, tt.oldKey)
Review Comment:
This `oldKey` existence check is vacuously true — nothing ever seeds
`oldKey`, so `Exists` returns false even if the write had been misrouted
somewhere else, and the `require.ErrorContains` above already guarantees no
write happened.
If we want this to actually prove "not misrouted to the old key", seed
`oldKey` with a known sentinel first and assert it's unchanged after the failed
write. Otherwise I'd just drop the `Exists` block — the `ErrorContains` carries
the assertion on its own.
--
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]
