zeroshade commented on code in PR #1571:
URL: https://github.com/apache/iceberg-go/pull/1571#discussion_r3732123232
##########
catalog/sql/sql.go:
##########
@@ -1717,6 +1725,10 @@ func (c *Catalog) DropView(ctx context.Context,
identifier table.Identifier) err
return errViewsUnsupportedOnV0
}
+ if err := catalog.ValidateViewIdentifier(identifier); err != nil {
Review Comment:
**Blocking — this can make existing rows permanently unremovable through the
API.**
`CreateView` previously had no validation at all, so views with identifiers
like `a/b`, `.`, or embedded control characters can already exist in
`iceberg_tables` in deployed catalogs. Adding validation to `DropView` means
those rows now fail validation on the way *out* — the only API path that could
remove them. The user is left with a view they can neither load nor drop,
recoverable only by direct SQL against the catalog tables.
This is also asymmetric with tables, which have always been validated on
create via `createStagedTable` (`catalog/internal/utils.go:128`), so no
comparable pool of invalid table rows can exist.
**Suggested fix:** exempt `DropView` from component validation — keeping
only a length/shape check sufficient to build the query — so a bad row can
always be cleaned up. If you'd rather keep the guard for consistency, the PR
needs a documented migration note telling operators how to remove pre-existing
invalid views.
##########
catalog/sql/sql.go:
##########
@@ -1672,6 +1676,10 @@ func (c *Catalog) listViewsAll(ctx context.Context,
namespace table.Identifier)
return nil, nil
}
+ if err := checkValidNamespace(namespace); err != nil {
Review Comment:
**Blocking — this guard is not the validator the PR is about, and the test
covering it passes for an unrelated reason.**
`checkValidNamespace` here is the *local* helper at `sql.go:736`, which only
checks `len(ident) < 1`. It is not `catalog.ValidateNamespaceIdentifier`, so no
component rules — no empty levels, no `.`/`..`, no control characters — are
applied on this path.
The `{".."}` test case passes off the pre-existing not-found path, not off
this guard: `..` has length 1, so `checkValidNamespace` returns nil, and the
error comes from `resolveNamespaceKey` below failing to find the namespace.
**The test stays green if you delete this guard entirely** — worth confirming
locally, it's a quick check.
What the line *does* change is real but unintended: `ListViews(ctx, nil)`
now errors where it previously listed root-namespace views. That also diverges
from `listTablesAll` (`sql.go:1387`), which has no such guard, so listing
tables and listing views in the same catalog now behave differently for a nil
namespace.
**Suggested fix:** either use `catalog.ValidateNamespaceIdentifier` here
*and* in `listTablesAll` so the two agree and the guard actually validates
something, or drop this line and let the not-found path handle it as before.
Either way the `{".."}` test needs an assertion that fails when the guard is
removed.
##########
catalog/catalog.go:
##########
@@ -249,12 +249,7 @@ func NamespaceFromIdent(ident table.Identifier)
table.Identifier {
return ident[:len(ident)-1]
}
-func validateIdentifier(ident table.Identifier, notFoundErr error) error {
- if len(ident) < 2 {
- return fmt.Errorf("%w: missing namespace or invalid identifier
%v",
- notFoundErr, strings.Join(ident, "."))
- }
-
+func validateIdentifierComponents(ident table.Identifier, notFoundErr error)
error {
Review Comment:
This extraction leaves `validateIdentifierComponents` with exactly one
caller — `validateIdentifier` on line 276 — and it was previously inline there.
The new `ValidateNamespaceIdentifier` deliberately does *not* use it, since
namespaces are allowed component shapes that tables and views are not.
So the extraction is scaffolding from an earlier design in which namespaces
reused the component rules, and nothing in the shipped code depends on it.
#1614 carries the same leftover, and reusing these rules for namespaces is
specifically what the maintainer pushed back on there.
**Suggested fix:** drop the extraction and inline the loop back into
`validateIdentifier`. It removes a package-level function that reads as shared
infrastructure but has no second consumer, and shrinks the conflict surface
with #1614.
##########
catalog/sql/sql.go:
##########
@@ -1233,7 +1233,7 @@ func (c *Catalog) CheckTableExists(ctx context.Context,
identifier table.Identif
}
func (c *Catalog) CreateNamespace(ctx context.Context, namespace
table.Identifier, props iceberg.Properties) error {
- if err := checkValidNamespace(namespace); err != nil {
+ if err := catalog.ValidateNamespaceIdentifier(namespace); err != nil {
Review Comment:
This rewire is out of scope for a "validate view identifiers" PR —
`CreateNamespace` is neither a view API nor a boundary this PR's title covers —
and #1614 makes the same change to the REST catalog with a broader contract.
It's also only half the story on its own: `DropNamespace` (`sql.go:1288`),
and the load/update-properties paths stay on the length-only
`checkValidNamespace` (`sql.go:736`), so after this the SQL catalog validates
namespace identifiers on create but not on any other namespace operation.
**Suggested fix:** drop this hunk and let the namespace contract be settled
in #1614 in one place, for all operations. If you'd rather keep it here, apply
it to every namespace entry point in this file rather than just
`CreateNamespace`, so the catalog has one consistent rule.
##########
catalog/catalog.go:
##########
@@ -272,6 +267,31 @@ func validateIdentifier(ident table.Identifier,
notFoundErr error) error {
return nil
}
+func validateIdentifier(ident table.Identifier, notFoundErr error) error {
+ if len(ident) < 2 {
+ return fmt.Errorf("%w: missing namespace or invalid identifier
%v",
+ notFoundErr, strings.Join(ident, "."))
+ }
+
+ return validateIdentifierComponents(ident, notFoundErr)
+}
+
+// ValidateNamespaceIdentifier checks that an identifier contains at least one
valid namespace level.
+func ValidateNamespaceIdentifier(ident table.Identifier) error {
Review Comment:
**Blocking as a coordination issue rather than a code defect:** this exact
function is also added by #1614, at this same location, with an identical body.
The godoc differs, and #1614's is the accurate one. This comment says the
function "checks that an identifier contains at least one valid namespace
level," but the implementation never defines or checks what makes a level
*valid* — it rejects only an empty identifier and NUL bytes. #1614 says so
directly ("no null characters ... other rules intentionally left to the
implementation"), which matches what the code does.
**Suggested fix:** rebase onto #1614 and drop this copy. It isn't
view-related, so removing it makes this PR match its own title, and it avoids a
merge conflict where the two godocs would have to be reconciled by hand.
--
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]
