laskoviymishka commented on code in PR #1614:
URL: https://github.com/apache/iceberg-go/pull/1614#discussion_r3719184357
##########
catalog/identifier_test.go:
##########
@@ -45,6 +45,24 @@ func TestValidateTableIdentifier(t *testing.T) {
}
}
+func TestValidateNamespaceIdentifier(t *testing.T) {
+ require.NoError(t,
catalog.ValidateNamespaceIdentifier(table.Identifier{"namespace"}))
+ require.NoError(t,
catalog.ValidateNamespaceIdentifier(table.Identifier{"parent", "namespace"}))
+
+ for _, ident := range []table.Identifier{
Review Comment:
The error table can't actually tell the two failure modes apart. `nil` and
`{}` fail on the length check; `{""}`, `{"."}`, `{"namespace/child"}` fail on
component validation — but both wrap `ErrNoSuchNamespace`, so if the component
check silently stopped running the suite would still pass.
Could we split these into `t.Run` subtests (or at least `require.ErrorIsf`
with the input) so a failure names the offending case? While we're here, a
multi-component case like `{"parent", "namespace\nchild"}` would cover a
control char in a non-first position — `TestValidateTableIdentifier` already
does that for tables.
##########
catalog/catalog.go:
##########
@@ -272,6 +276,15 @@ func validateIdentifier(ident table.Identifier,
notFoundErr error) error {
return nil
}
+// ValidateNamespaceIdentifier checks that an identifier contains at least one
valid namespace level.
+func ValidateNamespaceIdentifier(ident table.Identifier) error {
+ if len(ident) < 1 {
Review Comment:
tiny thing: `len(ident) == 0` reads as the empty check more directly than `<
1`.
##########
catalog/catalog.go:
##########
@@ -272,6 +276,15 @@ func validateIdentifier(ident table.Identifier,
notFoundErr error) error {
return nil
}
+// ValidateNamespaceIdentifier checks that an identifier contains at least one
valid namespace level.
+func ValidateNamespaceIdentifier(ident table.Identifier) error {
Review Comment:
I think there's a real interop question buried in here. These component
rules (rejecting `.`, `..`, `/`, and control chars) are stricter than the REST
spec, Java's `Namespace.of`, and PyIceberg — Java only rejects the null byte,
so `["ns/child"]` and `["ns\tname"]` round-trip fine through those clients.
Because `ValidateNamespaceIdentifier` guards every REST namespace op,
including the read paths (Load/List/Drop all go through `checkValidNamespace`),
a namespace another client already created with one of these components isn't
just un-creatable here — it's un-loadable and un-droppable, and listing
silently returns nothing.
What's the intended contract? If this is meant as write-side defensiveness
I'd scope it to `CreateNamespace` and loosen it toward Java's rule (null byte
only), and have the godoc note that it's stricter than the spec. wdyt?
--
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]
