tanmayrauth commented on code in PR #1399:
URL: https://github.com/apache/iceberg-go/pull/1399#discussion_r3525772153
##########
cmd/iceberg/branch_tag.go:
##########
@@ -50,6 +52,12 @@ func runTag(ctx context.Context, output Output, cat
catalog.Catalog, cmd *TagCmd
func runBranchCreate(ctx context.Context, output Output, cat catalog.Catalog,
cmd *BranchCreateCmd) {
tbl := loadTable(ctx, output, cat, cmd.TableID)
meta := tbl.Metadata()
+ if err := validateRefName(cmd.BranchName); err != nil {
Review Comment:
Could this move above `loadTable` at line 51? It only reads
`cmd.BranchName`, so running it first lets an invalid name fail before a full
catalog load (a network round-trip for REST/Glue). Same for the tag path at
line 169.
##########
cmd/iceberg/branch_tag.go:
##########
@@ -197,6 +239,26 @@ func runTagCreate(ctx context.Context, output Output, cat
catalog.Catalog, cmd *
output.RefCreated(result)
}
+func validateRefName(name string) error {
+ if strings.TrimSpace(name) != name || name == "" {
+ return errors.New("name must be non-empty and may not contain
leading/trailing whitespace")
+ }
+
+ if name == "." || name == ".." {
+ return errors.New("name may not be '.' or '..'")
+ }
+
+ if strings.ContainsAny(name, "/\\") {
+ return errors.New("name may not contain path separators")
+ }
+
+ if strings.IndexFunc(name, func(r rune) bool { return
unicode.IsControl(r) }) >= 0 {
Review Comment:
nit: `strings.ContainsFunc(name, unicode.IsControl)` reads a bit cleaner
here
##########
cmd/iceberg/branch_tag.go:
##########
@@ -80,6 +88,13 @@ func runBranchCreate(ctx context.Context, output Output, cat
catalog.Catalog, cm
return
}
+ if d <= 0 {
Review Comment:
Good catch — worth noting in the PR description that this isn't redundant
with the library's `WithMaxRefAgeMs` guard: `setSnapshotRefUpdate.Apply` only
applies etention when the value is > 0, so today a negative `--max-ref-age` is
silently dropped rather than rejected. This surfaces it.
##########
cmd/iceberg/branch_tag.go:
##########
@@ -197,6 +239,26 @@ func runTagCreate(ctx context.Context, output Output, cat
catalog.Catalog, cmd *
output.RefCreated(result)
}
+func validateRefName(name string) error {
Review Comment:
This (and the retention checks) only guard the CLI path —
`table.SetSnapshotRef` / `NewSetSnapshotRefUpdate` still accept the same
inputs, so other iceberg-go consumers get no protection. Fine to keep
CLI-scoped for this PR, but if the goal is data integrity the name/retention
validation arguably belongs in the library so every caller benefits.
##########
cmd/iceberg/branch_tag.go:
##########
@@ -197,6 +239,26 @@ func runTagCreate(ctx context.Context, output Output, cat
catalog.Catalog, cmd *
output.RefCreated(result)
}
+func validateRefName(name string) error {
+ if strings.TrimSpace(name) != name || name == "" {
+ return errors.New("name must be non-empty and may not contain
leading/trailing whitespace")
+ }
+
+ if name == "." || name == ".." {
+ return errors.New("name may not be '.' or '..'")
+ }
+
+ if strings.ContainsAny(name, "/\\") {
Review Comment:
Was the path-separator ban intentional? This makes the CLI stricter than the
iceberg-go `table` layer, the spec, and Java/Spark/PyIceberg, none of which
restrict ref-name characters. Slash-style branch names (`audit/foo`, git-flow
`feature/x`) are common in WAP workflows and created freely by other engines —
after this, a branch that's legal in Spark can't be created with the same
name here. If the motivation is Spark's `branch_<name>` metadata-selector
ambiguity, worth calling that out in a comment; otherwise this may be
over-tight.
--
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]
