LuciferYang opened a new issue, #977:
URL: https://github.com/apache/iceberg-cpp/issues/977
## Summary
`InMemoryCatalog` fails to validate that a table's namespace exists before
acting, in two methods.
`CreateTable` writes the table metadata file through `FileIO` before it
checks the namespace. Creating a table under a namespace that does not exist
returns `kNoSuchNamespace`, but on an object-store `FileIO` the call first
writes an orphaned `00000-<uuid>.metadata.json` that nothing ever removes
(`DropTable`'s purge only touches registered tables). On the default local
`FileIO` the stray write fails first, so the caller gets a misleading
`kIOError` instead of `kNoSuchNamespace`.
`RegisterTable` guards with `if
(!root_namespace_->NamespaceExists(identifier.ns))`. `NamespaceExists` returns
`Result<bool>` (`std::expected<bool, Error>`); `!result` tests `has_value()`,
not the contained bool, and a missing namespace is reported as a value `false`,
never an error. The guard therefore never fires, so registering under a missing
namespace falls through and surfaces as `kUnknownError` ("The registry
failed.") instead of `kNoSuchNamespace`.
## Root Cause
`CreateTable` (`src/iceberg/catalog/memory/in_memory_catalog.cc`): the
namespace is only enforced inside `UpdateTableMetadataLocation`, which runs
after `TableMetadataUtil::Write` has already persisted the file.
`TableExists(identifier).value_or(false)` ahead of the write swallows the
`kNoSuchNamespace` from the namespace lookup and reports "table absent", so
control falls through to the write.
`RegisterTable`: `if (!root_namespace_->NamespaceExists(identifier.ns))`
reads as `if (!result.has_value())`. `NamespaceExists` maps a missing namespace
to `Ok(false)`, so the branch is dead code; the error later comes out of the
inner `RegisterTable` and is rewritten to `kUnknownError`.
## Impact
`CreateTable` leaks an orphan metadata file on object-store `FileIO` and
returns the wrong error kind (`kIOError`) on local `FileIO`. `InMemoryCatalog`
is documented as not for production use (unit tests, prototyping,
demonstration), so this is a correctness and robustness issue, not a security
one.
`RegisterTable` returns `kUnknownError` for a missing namespace instead of
`kNoSuchNamespace`. `SqlCatalog::RegisterTable` and the REST error handler both
return `kNoSuchNamespace`, so `InMemoryCatalog` is the outlier here.
## Proposed Fix
Unwrap `NamespaceExists` and return `kNoSuchNamespace` before any metadata
write, in both methods, mirroring `SqlCatalog::CreateTable`.
## Out of scope
- `RegisterTable` still masks `kAlreadyExists` as `kUnknownError` on the
duplicate-registration path (`if (!root_namespace_->RegisterTable(...))`); the
clean fix is `ICEBERG_RETURN_UNEXPECTED(...)`, as `RenameTable` already does.
Follow-up.
- `StageCreateTable` and `UpdateTable`'s create branch share the same
write-before-validate pattern. Follow-up.
--
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]
