laskoviymishka commented on code in PR #1571:
URL: https://github.com/apache/iceberg-go/pull/1571#discussion_r3719185600
##########
catalog/sql/sql.go:
##########
@@ -1668,6 +1672,12 @@ func (c *Catalog) ListViews(ctx context.Context,
namespace table.Identifier) ite
}
func (c *Catalog) listViewsAll(ctx context.Context, namespace
table.Identifier) ([]table.Identifier, error) {
+ if len(namespace) > 0 {
+ if err := catalog.ValidateNamespaceIdentifier(namespace); err
!= nil {
Review Comment:
I think this makes `ListViews` stricter than the catalog it's reading from.
`checkValidNamespace` is what `CreateNamespace`, `DropNamespace`, and
`LoadNamespaceProperties` all use, and it only checks `len(ident) >= 1`, never
the components. So `CreateNamespace(ctx, []string{".."}, nil)` succeeds today,
the row lands in the DB, and `ListTables` on it still works since
`listTablesAll` goes straight to `resolveNamespaceKey`. After this change
`ListViews` rejects that same namespace with `ErrNoSuchNamespace` before it
ever reaches the lookup. Same namespace, two different answers depending on
which list call we make.
I'd rather push the component check down into `checkValidNamespace` so the
write path refuses to create those names in the first place and both list paths
see the same universe. If tightening create feels out of scope here, I'd drop
this block and let `resolveNamespaceKey`'s not-found path handle it, matching
`listTablesAll`. Either is defensible, but I don't think we want the split.
wdyt?
##########
catalog/sql/sql_test.go:
##########
@@ -2218,6 +2218,43 @@ func (s *SqliteCatalogTestSuite) TestCreateView() {
s.True(exists)
}
+func TestViewOperationsRejectInvalidIdentifiers(t *testing.T) {
+ t.Parallel()
+
+ invalid := []struct {
+ name string
+ identifier table.Identifier
+ }{
+ {name: "nil", identifier: nil},
+ {name: "empty", identifier: table.Identifier{}},
+ {name: "missing namespace", identifier:
table.Identifier{"view"}},
+ {name: "empty name", identifier: table.Identifier{"ns", ""}},
+ {name: "dot name", identifier: table.Identifier{"ns", "."}},
+ {name: "parent name", identifier: table.Identifier{"ns", ".."}},
+ {name: "path separator", identifier: table.Identifier{"ns",
"nested/view"}},
+ {name: "control character", identifier: table.Identifier{"ns",
"view\nname"}},
+ }
+
+ cat := &sqlcat.Catalog{}
+ for _, test := range invalid {
+ t.Run(test.name, func(t *testing.T) {
+ t.Parallel()
+
+ err := cat.CreateView(context.Background(),
test.identifier, nil, "", nil)
+ assert.ErrorIs(t, err, catalog.ErrNoSuchView)
+
+ err = cat.DropView(context.Background(),
test.identifier)
+ assert.ErrorIs(t, err, catalog.ErrNoSuchView)
+
+ _, err = cat.CheckViewExists(context.Background(),
test.identifier)
+ assert.ErrorIs(t, err, catalog.ErrNoSuchView)
+
+ _, err = cat.LoadView(context.Background(),
test.identifier)
+ assert.ErrorIs(t, err, catalog.ErrNoSuchView)
Review Comment:
`ListViews` is the one view op missing from this table, and it's also the
one with its own validation call (`ValidateNamespaceIdentifier` rather than
`ValidateViewIdentifier`). Its only coverage right now is the integration
assertion down in `TestListViews`. I'd add a case here so a regression in
`listViewsAll` shows up in the fast test too.
##########
catalog/sql/sql_test.go:
##########
@@ -2469,6 +2506,13 @@ func (s *SqliteCatalogTestSuite) TestListViews() {
break
}
+
+ viewsIter = db.ListViews(context.Background(), []string{".."})
+ for _, err := range viewsIter {
Review Comment:
This can pass without asserting anything. If `ListViews` ever returns an
empty iterator instead of one that yields the error, the loop body never runs,
`s.ErrorIs` never fires, and the suite still goes green. It works today because
the error path yields exactly one element, but that's an implicit contract we
aren't checking here.
I'd pull the error out of the loop and assert after it:
```go
var gotErr error
yielded := false
for _, err := range db.ListViews(context.Background(), []string{".."}) {
yielded, gotErr = true, err
break
}
s.True(yielded, "invalid namespace must yield at least one element")
s.ErrorIs(gotErr, catalog.ErrNoSuchNamespace)
```
##########
catalog/sql/sql.go:
##########
@@ -1788,6 +1802,10 @@ func (c *Catalog) DropView(ctx context.Context,
identifier table.Identifier) err
// CheckViewExists returns true if a view exists in the catalog.
func (c *Catalog) CheckViewExists(ctx context.Context, identifier
table.Identifier) (bool, error) {
+ if err := catalog.ValidateViewIdentifier(identifier); err != nil {
Review Comment:
This one shifts V0 behavior. `CheckViewExists` used to return `(false, nil)`
for any identifier on a V0 catalog, and now an invalid one gets an error
instead. `ListViews` has the same shape, where `(nil, nil)` becomes
`ErrNoSuchNamespace`. For `CreateView`, `DropView`, and `LoadView` it's only a
swap of which error comes back, but for these two we're turning a clean
negative into a failure.
I'd move the validation below the `isV0()` guard in all five. That also
matches how `ValidateTableIdentifier` sits after the early exits in `LoadTable`
and `CommitTable`.
One thing to flag: if we do that,
`TestViewOperationsRejectInvalidIdentifiers` stops passing, because the
zero-value catalog it uses is V0. Separate note on that below.
##########
catalog/sql/sql_test.go:
##########
@@ -2218,6 +2218,43 @@ func (s *SqliteCatalogTestSuite) TestCreateView() {
s.True(exists)
}
+func TestViewOperationsRejectInvalidIdentifiers(t *testing.T) {
+ t.Parallel()
+
+ invalid := []struct {
+ name string
+ identifier table.Identifier
+ }{
+ {name: "nil", identifier: nil},
+ {name: "empty", identifier: table.Identifier{}},
+ {name: "missing namespace", identifier:
table.Identifier{"view"}},
+ {name: "empty name", identifier: table.Identifier{"ns", ""}},
+ {name: "dot name", identifier: table.Identifier{"ns", "."}},
+ {name: "parent name", identifier: table.Identifier{"ns", ".."}},
+ {name: "path separator", identifier: table.Identifier{"ns",
"nested/view"}},
+ {name: "control character", identifier: table.Identifier{"ns",
"view\nname"}},
+ }
+
+ cat := &sqlcat.Catalog{}
Review Comment:
`&sqlcat.Catalog{}` leaves `schemaVersion` at its zero value, which is V0,
so this test only passes because the new validation sits above the `isV0()`
guard. It's asserting guard ordering rather than the behavior we actually care
about.
I'd stand up a V1 in-memory SQLite catalog here instead, so the assertions
hold whichever side of `isV0()` the validation ends up on.
##########
catalog/catalog.go:
##########
@@ -272,6 +267,24 @@ 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 {
+ if len(ident) < 1 {
+ return fmt.Errorf("%w: empty namespace identifier",
ErrNoSuchNamespace)
+ }
+
Review Comment:
Since this is exported now, I'd note in the doc comment that it's stricter
than the length-only `checkValidNamespace` the SQL and REST catalogs use
internally. We end up with two namespace validators in tree with different
semantics and nothing pointing that out, so the next catalog implementation
just picks whichever one it happens to find first.
Not a blocker on its own, though it matters more if we keep the asymmetry
(see my note on `listViewsAll`).
--
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]
