zeroshade commented on code in PR #1354:
URL: https://github.com/apache/iceberg-go/pull/1354#discussion_r3521447982
##########
catalog/catalog.go:
##########
@@ -214,6 +214,16 @@ func NamespaceFromIdent(ident table.Identifier)
table.Identifier {
return ident[:len(ident)-1]
}
+// ValidateTableIdentifier checks that an identifier contains at least one
namespace level and a table name.
+func ValidateTableIdentifier(ident table.Identifier) error {
+ if len(ident) < 2 {
Review Comment:
This only rejects identifiers with fewer than two elements, so path-altering
components still pass validation: `table.Identifier{"ns", ""}`, `{"ns", "."}`,
`{"ns", ".."}`, and `{"ns", "a/b"}`. In the REST client these flow through
`splitIdentForPath` into `url.URL.JoinPath`, where `/`, `.`, and `..` are
interpreted as path structure/cleaning, so the REST path-injection issue is
only partially fixed. Please reject empty components, `.`, `..`, `/`, and
control characters in every component; preferably also path-encode the final
table/view name like Java's `RESTUtil.encodeString` / pyiceberg's `safe=""`,
and add tests showing no malformed REST path is generated.
##########
catalog/rest/rest.go:
##########
@@ -1156,8 +1155,8 @@ func (r *Catalog) CommitTransaction(ctx context.Context,
commits []table.TableCo
changes := make([]tableChange, len(commits))
for i, c := range commits {
- if len(c.Identifier) == 0 {
- return catalog.ErrMissingIdentifier
+ if err := catalog.ValidateTableIdentifier(c.Identifier); err !=
nil {
+ return fmt.Errorf("%w: %w",
catalog.ErrMissingIdentifier, err)
Review Comment:
Wrapping every malformed identifier as `ErrMissingIdentifier` conflates an
absent identifier with a present-but-invalid one. Please keep
`ErrMissingIdentifier` only for `len(c.Identifier) == 0`, and otherwise return
the original `ValidateTableIdentifier` error.
##########
catalog/rest/rest_test.go:
##########
@@ -2688,13 +2688,41 @@ func (r *RestCatalogSuite)
TestCatalogWithCustomTransport() {
r.Equal("/v1/config", transport.calls[0].path)
// Not expected to succeed
- tbl, err := cat.LoadTable(context.Background(),
table.Identifier{"unknown"})
+ tbl, err := cat.LoadTable(context.Background(),
table.Identifier{"namespace", "unknown"})
r.Error(err)
r.Nil(tbl)
r.Len(transport.calls, 2)
r.Equal("GET", transport.calls[1].method)
- r.Equal("/v1/namespaces/tables/unknown", transport.calls[1].path)
+ r.Equal("/v1/namespaces/namespace/tables/unknown",
transport.calls[1].path)
+}
+
+func (r *RestCatalogSuite) TestTableAndViewIdentifiersRequireNamespace() {
+ var transport mockTransport
+
+ cat, err := rest.NewCatalog(context.Background(), "rest", r.srv.URL,
rest.WithCustomTransport(&transport))
+ r.Require().NoError(err)
+ r.Require().Len(transport.calls, 1)
+
+ _, err = cat.LoadTable(context.Background(), table.Identifier{"table"})
+ r.ErrorIs(err, catalog.ErrNoSuchTable)
+
+ _, err = cat.LoadView(context.Background(), table.Identifier{"view"})
+ r.ErrorIs(err, catalog.ErrNoSuchTable)
Review Comment:
Question: invalid view identifiers now surface `ErrNoSuchTable` rather than
`ErrNoSuchView` via the shared table-named helper. That may be acceptable, but
it is a little surprising for view APIs.
--
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]
