zeroshade commented on code in PR #1259:
URL: https://github.com/apache/iceberg-go/pull/1259#discussion_r3461715826
##########
io/gocloud/blob.go:
##########
@@ -72,25 +72,61 @@ func (f *blobOpenFile) Stat() (fs.FileInfo, error) { return
f, nil }
// KeyExtractor extracts the object key from an input path
type KeyExtractor func(path string) (string, error)
+var errEmptyObjectKey = errors.New("object key is empty")
+
// 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 := url.Parse(location)
+ if err != nil {
+ return "", fmt.Errorf("invalid URI %q: %w", location,
err)
}
- key := strings.TrimPrefix(location, bucketName+"/")
+ var key string
+ if validatesURIAuthority(parsed.Scheme) && (parsed.Scheme != ""
|| parsed.Host != "") {
+ if parsed.Host == "" {
+ return "", fmt.Errorf("URI bucket is empty:
%s", location)
+ }
+ if parsed.Host != bucketName {
+ return "", fmt.Errorf("URI bucket %q does not
match configured bucket %q", parsed.Host, bucketName)
+ }
+
+ key = strings.TrimPrefix(parsed.EscapedPath(), "/")
+ if key != "" {
+ if parsed.RawQuery != "" {
+ key += "?" + parsed.RawQuery
+ }
+ if parsed.Fragment != "" {
+ key += "#" + parsed.EscapedFragment()
+ }
+ }
+ } else {
+ _, after, found := strings.Cut(location, "://")
+ if found {
+ location = after
+ }
+
+ key = strings.TrimPrefix(location, bucketName+"/")
+ }
if key == "" {
- return "", fmt.Errorf("URI path is empty: %s", location)
+ return "", fmt.Errorf("%w: %s", errEmptyObjectKey,
location)
}
return key, nil
}
}
+func validatesURIAuthority(scheme string) bool {
+ switch scheme {
+ case "", "s3", "s3a", "s3n", "oss", "gs":
Review Comment:
isn't this missing azure authorities and others?
##########
io/gocloud/blob.go:
##########
@@ -72,25 +72,61 @@ func (f *blobOpenFile) Stat() (fs.FileInfo, error) { return
f, nil }
// KeyExtractor extracts the object key from an input path
type KeyExtractor func(path string) (string, error)
+var errEmptyObjectKey = errors.New("object key is empty")
Review Comment:
should this be public so that consumers can use `errors.Is`?
##########
io/gocloud/blob.go:
##########
@@ -72,25 +72,61 @@ func (f *blobOpenFile) Stat() (fs.FileInfo, error) { return
f, nil }
// KeyExtractor extracts the object key from an input path
type KeyExtractor func(path string) (string, error)
+var errEmptyObjectKey = errors.New("object key is empty")
+
// 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 := url.Parse(location)
+ if err != nil {
+ return "", fmt.Errorf("invalid URI %q: %w", location,
err)
}
- key := strings.TrimPrefix(location, bucketName+"/")
+ var key string
+ if validatesURIAuthority(parsed.Scheme) && (parsed.Scheme != ""
|| parsed.Host != "") {
+ if parsed.Host == "" {
+ return "", fmt.Errorf("URI bucket is empty:
%s", location)
+ }
+ if parsed.Host != bucketName {
+ return "", fmt.Errorf("URI bucket %q does not
match configured bucket %q", parsed.Host, bucketName)
+ }
+
+ key = strings.TrimPrefix(parsed.EscapedPath(), "/")
+ if key != "" {
+ if parsed.RawQuery != "" {
+ key += "?" + parsed.RawQuery
+ }
+ if parsed.Fragment != "" {
+ key += "#" + parsed.EscapedFragment()
+ }
+ }
+ } else {
+ _, after, found := strings.Cut(location, "://")
+ if found {
+ location = after
+ }
+
+ key = strings.TrimPrefix(location, bucketName+"/")
+ }
Review Comment:
what's the case where the `url.Parse` succeeds but the condition at line 87
fails? can't we still just use `parsed` to figure this out instead of needing
to do `strings.Cut`?
--
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]
