yuqi1129 commented on code in PR #13551:
URL: https://github.com/apache/gravitino/pull/13551#discussion_r4119973047
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergTableOperationExecutor.java:
##########
@@ -115,6 +118,7 @@ public LoadTableResponse updateTable(
IcebergRequestContext context,
TableIdentifier tableIdentifier,
UpdateTableRequest updateTableRequest) {
+ IcebergColumnFieldValidator.validateUpdate(updateTableRequest);
Review Comment:
[P1] Validate the table name before committing a staged create
This endpoint also creates tables when the request contains
`AssertTableDoesNotExist`; it is not limited to updating an existing table. A
client can submit such a request directly with a 129-character
`tableIdentifier.name()` and a valid schema, bypassing the name check in
`createTable`. The wrapper commits the external table first, and
`IcebergTableHookDispatcher.updateTable` then imports it into Gravitino, where
the new `TableEntity.NAME` constraint rejects it. The request therefore fails
after the external table has been created.
Please validate the identifier name before calling the wrapper for create
commits. Add a regression test with `AssertTableDoesNotExist` and an oversized
identifier that verifies the wrapper is never invoked. I ran a temporary
executor test for this case: the expected `IllegalArgumentException` was not
thrown.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/dispatcher/IcebergNamespaceOperationExecutor.java:
##########
@@ -128,12 +130,16 @@ public LoadTableResponse registerTable(
IcebergRequestContext context,
Namespace namespace,
RegisterTableRequest registerTableRequest) {
+ TableEntity.NAME.validate(registerTableRequest.name(),
Entity.EntityType.TABLE);
IcebergCleanupHelper.rejectIfBeingPurged(
cleanupManager, context.catalogName(), namespace,
registerTableRequest.name());
- return icebergCatalogWrapperManager
- .getCatalogWrapper(context.catalogName())
- .registerTable(namespace, registerTableRequest,
context.requestCredentialVending());
+ LoadTableResponse response =
+ icebergCatalogWrapperManager
+ .getCatalogWrapper(context.catalogName())
+ .registerTable(namespace, registerTableRequest,
context.requestCredentialVending());
+
IcebergColumnFieldValidator.validateSchema(response.tableMetadata().schema());
Review Comment:
[P1] Validate the registered schema before the external catalog mutation
At this point `registerTable` has already mutated the external catalog. If
the metadata contains an oversized top-level column name or comment, this
validation throws an `IllegalArgumentException` (HTTP 400), but the
registration remains committed and the Gravitino import hook is never reached.
With `overwrite=true`, the external table's metadata pointer can already have
been replaced even though the caller receives a rejected-request response. This
also affects already tracked tables, whose hook would otherwise skip importing
them.
Please move schema validation before the actual registration/overwrite,
ensuring that the metadata validated is the metadata subsequently registered.
An unconditional drop on failure would not safely compensate an overwrite. The
current `AfterRegister` tests only verify that the exception occurs after the
wrapper was called; please add a state assertion showing that invalid input
leaves the external registration/metadata pointer unchanged.
--
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]
