zeroshade commented on code in PR #1259:
URL: https://github.com/apache/iceberg-go/pull/1259#discussion_r3493439697
##########
io/gocloud/blob.go:
##########
@@ -69,33 +68,138 @@ 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
-func defaultKeyExtractor(bucketName string) KeyExtractor {
+var errEmptyObjectKey = errors.New("object key is empty")
+
+type objectLocation struct {
+ authority string
+ key string
+ uriKey string
+ uriPrefix string
+ hasAuthority bool
+}
+
+func splitObjectLocation(location string) (objectLocation, error) {
+ scheme, rest, ok := strings.Cut(location, "://")
+ if !ok {
+ return objectLocation{key: location, uriKey: 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) {
+ if rest[authorityEnd] != '/' {
+ return objectLocation{}, fmt.Errorf("URI authority %q
must be followed by an object path: %s",
+ authority, location)
+ }
+
+ key = strings.TrimPrefix(rest[authorityEnd:], "/")
+ }
+
+ return objectLocation{
+ authority: authority,
+ key: key,
+ uriKey: key,
+ uriPrefix: scheme + "://" + authority + "/",
+ hasAuthority: true,
+ }, nil
+}
+
+type keyExtractorConfig struct {
+ strictAuthorityValidation bool
+}
+
+type keyExtractorOption func(*keyExtractorConfig)
+
+func withStrictAuthorityValidation() keyExtractorOption {
+ return func(cfg *keyExtractorConfig) {
+ cfg.strictAuthorityValidation = true
+ }
+}
+
+type objectLocationExtractor func(location string) (objectLocation, error)
+
+func keyExtractorFromObjectLocation(extract objectLocationExtractor)
KeyExtractor {
return func(location string) (string, error) {
- _, after, found := strings.Cut(location, "://")
- if found {
- location = after
+ parsed, err := extract(location)
+ if err != nil {
+ return "", err
+ }
+
+ return parsed.key, nil
+ }
+}
+
+// Non-strict cross-authority paths still fold into a storage key so existing
+// manifests keep loading until the FileIO can open per-authority buckets.
+func legacyAuthorityKey(parsed objectLocation) string {
+ if parsed.key == "" {
+ return parsed.authority + "/"
+ }
+
+ return parsed.authority + "/" + parsed.key
+}
+
+func defaultObjectLocationExtractor(bucketName string, opts
...keyExtractorOption) objectLocationExtractor {
+ var cfg keyExtractorConfig
+ for _, opt := range opts {
+ opt(&cfg)
+ }
+
+ return func(location string) (objectLocation, error) {
+ parsed, err := splitObjectLocation(location)
+ if err != nil {
+ return objectLocation{}, err
}
- key := strings.TrimPrefix(location, bucketName+"/")
+ if parsed.hasAuthority {
+ if parsed.authority != bucketName {
+ if cfg.strictAuthorityValidation {
+ return objectLocation{},
fmt.Errorf("URI authority %q does not match configured authority %q",
+ parsed.authority, bucketName)
+ }
+
+ // Preserve the legacy fold-by-authority
behavior on the default
+ // path so existing cross-authority manifests
keep loading until
+ // the FileIO can open per-authority buckets.
+ parsed.key = legacyAuthorityKey(parsed)
+ }
+ } else {
+ parsed.key = strings.TrimPrefix(location,
bucketName+"/")
+ parsed.uriKey = parsed.key
+ }
- if key == "" {
- return "", fmt.Errorf("URI path is empty: %s", location)
+ if parsed.key == "" {
+ return parsed, fmt.Errorf("%w: %s", errEmptyObjectKey,
location)
}
- return key, nil
+ return parsed, 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, opts ...keyExtractorOption)
KeyExtractor {
+ return
keyExtractorFromObjectLocation(defaultObjectLocationExtractor(bucketName,
opts...))
+}
+
type blobFileIO struct {
*blob.Bucket
- keyExtractor KeyExtractor
- ctx context.Context
+ keyExtractor KeyExtractor
Review Comment:
`blobFileIO` stores both `keyExtractor` and `extractObject`, but
`keyExtractor` is always derived from `extractObject` via
`keyExtractorFromObjectLocation` in every path. Storing both invites drift —
consider keeping only `extractObject` and deriving the extractor where needed
(and dropping it from the `createBlobFS` signature).
##########
io/gocloud/register.go:
##########
@@ -30,6 +31,14 @@ func init() {
registerAzureSchemes()
}
+func keyExtractorOptions(props map[string]string) []keyExtractorOption {
+ if enabled, err :=
strconv.ParseBool(props[icebergio.ObjectStoreStrictAuthorityValidation]); err
== nil && enabled {
Review Comment:
Strict-mode opt-in silently swallows `ParseBool` errors — a typo like
`"ture"`/`"yes"` disables validation invisibly. Since the prop only accepts the
two bool spellings, consider returning the parse error to the factory instead
of ignoring it.
##########
io/gocloud/blob.go:
##########
@@ -198,25 +302,64 @@ func (bfs *blobFileIO) NewWriter(ctx context.Context,
path string, overwrite boo
nil
}
-func createBlobFS(ctx context.Context, bucket *blob.Bucket, keyExtractor
KeyExtractor) icebergio.IO {
- return &blobFileIO{Bucket: bucket, keyExtractor: keyExtractor, ctx: ctx}
+func createBlobFS(ctx context.Context, bucket *blob.Bucket, keyExtractor
KeyExtractor, extractObject objectLocationExtractor) icebergio.IO {
+ return &blobFileIO{Bucket: bucket, keyExtractor: keyExtractor,
extractObject: extractObject, ctx: ctx}
+}
+
+func (bfs *blobFileIO) objectLocation(root string) (objectLocation, error) {
+ if bfs.extractObject == nil {
+ return objectLocation{}, errors.New("blob file IO missing
object location extractor")
+ }
+
+ return bfs.extractObject(root)
+}
+
+func walkedURIPath(location objectLocation, walked string) string {
+ if walked == "." {
+ return location.uriPrefix
+ }
+
+ uriKey := walked
+ if location.key != location.uriKey {
+ storageRoot := strings.TrimSuffix(location.key, "/")
+ if walked == location.key || walked == storageRoot {
+ uriKey = location.uriKey
+ } else {
+ prefix := storageRoot + "/"
+ if suffix, ok := strings.CutPrefix(walked, prefix); ok {
Review Comment:
If `walked` matches none of `location.key` / `storageRoot` /
`storageRoot+"/"`, this `ok=false` path leaves `uriKey = walked`, producing
`uriPrefix + walked` with the bucket doubled (e.g.
`s3://other-bucket/other-bucket/foo`). Unreachable today since `WalkDir` only
emits descendants, but a silent fallback — consider an explicit error or an
invariant comment.
--
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]
