zeroshade commented on code in PR #1614:
URL: https://github.com/apache/iceberg-go/pull/1614#discussion_r3732158267
##########
catalog/catalog.go:
##########
@@ -255,6 +255,10 @@ func validateIdentifier(ident table.Identifier,
notFoundErr error) error {
notFoundErr, strings.Join(ident, "."))
}
+ return validateIdentifierComponents(ident, notFoundErr)
Review Comment:
Worth simplifying in a follow-up: `validateIdentifierComponents` still has
only this one caller, while the new namespace validator deliberately does not
share these component rules. Keeping the loop in `validateIdentifier` would
avoid a dead abstraction left over from the earlier shared-validation design.
##########
catalog/catalog.go:
##########
@@ -272,6 +276,25 @@ func validateIdentifier(ident table.Identifier,
notFoundErr error) error {
return nil
}
+// ValidateNamespaceIdentifier checks that an identifier contains at least one
+// namespace level and no null characters. Other namespace component rules are
+// intentionally left to the catalog implementation so existing namespaces
+// remain readable across clients.
+func ValidateNamespaceIdentifier(ident table.Identifier) error {
+ if len(ident) < 1 {
+ return fmt.Errorf("%w: empty namespace identifier",
ErrNoSuchNamespace)
+ }
+
+ for _, part := range ident {
+ if strings.ContainsRune(part, '\x00') {
Review Comment:
Non-blocking: consider rejecting the decoded REST namespace separator as
well as NUL. With the default separator, `PathEscape("\x1f") == "%1F"`, so one
level containing `0x1F` collides with two separate levels after encoding.
##########
catalog/rest/rest_test.go:
##########
@@ -807,6 +807,33 @@ func (r *RestCatalogSuite)
TestListNamespaceWithParent200() {
r.Equal([]table.Identifier{{"accounting", "tax"}}, results)
}
+func (r *RestCatalogSuite) TestNamespaceOperationsRejectNull() {
+ requestCount := 0
+ r.mux.HandleFunc("/v1/namespaces", func(w http.ResponseWriter, req
*http.Request) {
+ requestCount++
+ })
+ r.mux.HandleFunc("/v1/namespaces/", func(w http.ResponseWriter, req
*http.Request) {
+ requestCount++
+ })
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL,
rest.WithOAuthToken(TestToken))
+ r.Require().NoError(err)
+
+ namespace := table.Identifier{"bad\x00name"}
Review Comment:
Consider adding `0x1F` coverage alongside the NUL case, including both
`[]string{"a\x1fb"}` and `[]string{"a", "b"}`. Optional coverage of
`ListTables`, `ListViews`, and `ListFunctions` would also pin the call sites
newly reached through `checkValidNamespace`.
--
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]
