Copilot commented on code in PR #10282:
URL: https://github.com/apache/gravitino/pull/10282#discussion_r2894447356
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -151,67 +157,80 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
GravitinoEnv env = GravitinoEnv.getInstance();
NameIdentifier identifier = toEntityIdent(metalake, object);
- Supplier<NoSuchMetadataObjectException> exceptionToThrowSupplier =
- () ->
- new NoSuchMetadataObjectException(
- "Metadata object %s type %s doesn't exist", object.fullName(),
object.type());
-
switch (object.type()) {
case METALAKE:
if (!metalake.equals(object.name())) {
throw new IllegalMetadataObjectException("The metalake object name
must be %s", metalake);
}
NameIdentifierUtil.checkMetalake(identifier);
- check(env.metalakeDispatcher().metalakeExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.metalakeDispatcher().metalakeExists(identifier),
+ () -> new NoSuchMetalakeException("Metalake %s doesn't exist",
object.fullName()));
break;
case CATALOG:
NameIdentifierUtil.checkCatalog(identifier);
- check(env.catalogDispatcher().catalogExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.catalogDispatcher().catalogExists(identifier),
+ () -> new NoSuchCatalogException("Catalog %s doesn't exist",
object.fullName()));
break;
case SCHEMA:
NameIdentifierUtil.checkSchema(identifier);
- check(env.schemaDispatcher().schemaExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.schemaDispatcher().schemaExists(identifier),
+ () -> new NoSuchSchemaException("Schema %s doesn't exist",
object.fullName()));
break;
case FILESET:
NameIdentifierUtil.checkFileset(identifier);
- check(env.filesetDispatcher().filesetExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.filesetDispatcher().filesetExists(identifier),
+ () -> new NoSuchFilesetException("Fileset %s doesn't exist",
object.fullName()));
break;
case TABLE:
NameIdentifierUtil.checkTable(identifier);
- check(env.tableDispatcher().tableExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.tableDispatcher().tableExists(identifier),
+ () -> new NoSuchTableException("Table %s doesn't exist",
object.fullName()));
break;
case COLUMN:
NameIdentifierUtil.checkColumn(identifier);
NameIdentifier tableIdent =
NameIdentifier.of(identifier.namespace().levels());
- check(env.tableDispatcher().tableExists(tableIdent),
exceptionToThrowSupplier);
+ check(
+ env.tableDispatcher().tableExists(tableIdent),
+ () -> new NoSuchTableException("Table %s doesn't exist",
tableIdent.toString()));
break;
case TOPIC:
NameIdentifierUtil.checkTopic(identifier);
- check(env.topicDispatcher().topicExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.topicDispatcher().topicExists(identifier),
+ () -> new NoSuchTopicException("Topic %s doesn't exist",
object.fullName()));
break;
case MODEL:
NameIdentifierUtil.checkModel(identifier);
- check(env.modelDispatcher().modelExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.modelDispatcher().modelExists(identifier),
+ () -> new NoSuchModelException("Model %s doesn't exist",
object.fullName()));
break;
case VIEW:
NameIdentifierUtil.checkView(identifier);
- check(env.viewDispatcher().viewExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.viewDispatcher().viewExists(identifier),
+ () -> new NoSuchViewException("View %s doesn't exist",
object.fullName()));
break;
case ROLE:
AuthorizationUtils.checkRole(identifier);
try {
env.accessControlDispatcher().getRole(metalake, object.fullName());
} catch (NoSuchRoleException nsr) {
- throw exceptionToThrowSupplier.get();
+ throw nsr;
}
Review Comment:
These `try/catch` blocks catch a `NoSuch*Exception` and immediately rethrow
the same exception, which is redundant and adds noise. Consider removing the
`try/catch` and letting the underlying dispatcher exception propagate directly
(unless you intend to add additional context or translation here).
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -151,67 +157,80 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
GravitinoEnv env = GravitinoEnv.getInstance();
NameIdentifier identifier = toEntityIdent(metalake, object);
- Supplier<NoSuchMetadataObjectException> exceptionToThrowSupplier =
- () ->
- new NoSuchMetadataObjectException(
- "Metadata object %s type %s doesn't exist", object.fullName(),
object.type());
-
switch (object.type()) {
Review Comment:
`checkMetadataObject` no longer throws `NoSuchMetadataObjectException` for
missing objects; it now throws type-specific `NotFoundException` subclasses
(e.g., `NoSuchSchemaException`, `NoSuchTableException`). Please update the
method JavaDoc accordingly so callers have accurate expectations.
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestTableOperations.java:
##########
@@ -994,4 +994,41 @@ public static Table mockTable(
return table;
}
+
+ @Test
+ public void testCreateTableWithNonExistentSchema() {
+ String nonExistentSchema = "non_existent_schema";
+ NameIdentifier schemaIdent = NameIdentifier.of(metalake, catalog,
nonExistentSchema);
+
+ // Mock schema not exists
+ when(schemaDispatcher.schemaExists(schemaIdent)).thenReturn(false);
+
+ TableCreateRequest req =
+ new TableCreateRequest(
+ "table1",
+ "comment",
+ new ColumnDTO[] {
+ new ColumnDTO.Builder()
+ .withName("col1")
+ .withDataType(Types.IntegerType.get())
+ .withComment("col_comment")
+ .build()
+ },
+ ImmutableMap.of(),
+ new SortOrderDTO[0],
+ null,
+ new Partitioning[0],
+ new IndexDTO[0]);
+
+ Response resp =
+ target(tablePath(metalake, catalog, nonExistentSchema))
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .post(Entity.entity(req, MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.NOT_FOUND.getStatusCode(),
resp.getStatus());
+
+ ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
Review Comment:
To align with the other REST error-path tests in this class, consider also
asserting `errorResp.getCode()` (e.g., `ErrorConstants.NOT_FOUND_CODE`) in
addition to the HTTP status and `type`. This makes the regression test more
precise about the error payload contract.
```suggestion
ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE,
errorResp.getCode());
```
##########
server/src/test/java/org/apache/gravitino/server/web/rest/TestTableOperations.java:
##########
@@ -994,4 +994,41 @@ public static Table mockTable(
return table;
}
+
+ @Test
+ public void testCreateTableWithNonExistentSchema() {
+ String nonExistentSchema = "non_existent_schema";
+ NameIdentifier schemaIdent = NameIdentifier.of(metalake, catalog,
nonExistentSchema);
+
+ // Mock schema not exists
Review Comment:
Grammar: update the comment to use "does not exist" (or "doesn't exist")
instead of "not exists".
```suggestion
// Mock schema does not exist
```
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -151,67 +157,80 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
GravitinoEnv env = GravitinoEnv.getInstance();
NameIdentifier identifier = toEntityIdent(metalake, object);
- Supplier<NoSuchMetadataObjectException> exceptionToThrowSupplier =
- () ->
- new NoSuchMetadataObjectException(
- "Metadata object %s type %s doesn't exist", object.fullName(),
object.type());
-
switch (object.type()) {
case METALAKE:
if (!metalake.equals(object.name())) {
throw new IllegalMetadataObjectException("The metalake object name
must be %s", metalake);
}
NameIdentifierUtil.checkMetalake(identifier);
- check(env.metalakeDispatcher().metalakeExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.metalakeDispatcher().metalakeExists(identifier),
+ () -> new NoSuchMetalakeException("Metalake %s doesn't exist",
object.fullName()));
break;
case CATALOG:
NameIdentifierUtil.checkCatalog(identifier);
- check(env.catalogDispatcher().catalogExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.catalogDispatcher().catalogExists(identifier),
+ () -> new NoSuchCatalogException("Catalog %s doesn't exist",
object.fullName()));
break;
Review Comment:
By switching from `NoSuchMetadataObjectException` to type-specific
`NotFoundException` subclasses, callers/tests that currently expect/catch
`NoSuchMetadataObjectException` will no longer handle missing-object cases. For
example, `TestRoleOperations` and `TestOwnerOperations` currently assert
`NoSuchMetadataObjectException` from
`MetadataObjectUtil.checkMetadataObject(...)`, and `RoleOperations#createRole`
catches `NoSuchMetadataObjectException` to convert it to
`IllegalMetadataObjectException` (400) instead of 404. Please update those call
sites/tests to catch/expect the specific `NoSuch*Exception` (or broaden to
`NotFoundException`) so behavior is consistent and the build doesn’t break.
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -33,12 +31,20 @@
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.authorization.AuthorizationUtils;
import org.apache.gravitino.exceptions.IllegalMetadataObjectException;
+import org.apache.gravitino.exceptions.NoSuchCatalogException;
+import org.apache.gravitino.exceptions.NoSuchFilesetException;
import org.apache.gravitino.exceptions.NoSuchJobException;
import org.apache.gravitino.exceptions.NoSuchJobTemplateException;
import org.apache.gravitino.exceptions.NoSuchMetadataObjectException;
Review Comment:
`NoSuchMetadataObjectException` is imported but no longer referenced in this
file after switching to specific `NoSuch*Exception`s. This will fail
checkstyle/spotless for unused imports; please remove the unused import (or
reintroduce usage if intended).
```suggestion
```
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -151,67 +157,80 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
GravitinoEnv env = GravitinoEnv.getInstance();
NameIdentifier identifier = toEntityIdent(metalake, object);
- Supplier<NoSuchMetadataObjectException> exceptionToThrowSupplier =
- () ->
- new NoSuchMetadataObjectException(
- "Metadata object %s type %s doesn't exist", object.fullName(),
object.type());
-
switch (object.type()) {
case METALAKE:
if (!metalake.equals(object.name())) {
throw new IllegalMetadataObjectException("The metalake object name
must be %s", metalake);
}
NameIdentifierUtil.checkMetalake(identifier);
- check(env.metalakeDispatcher().metalakeExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.metalakeDispatcher().metalakeExists(identifier),
+ () -> new NoSuchMetalakeException("Metalake %s doesn't exist",
object.fullName()));
break;
case CATALOG:
NameIdentifierUtil.checkCatalog(identifier);
- check(env.catalogDispatcher().catalogExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.catalogDispatcher().catalogExists(identifier),
+ () -> new NoSuchCatalogException("Catalog %s doesn't exist",
object.fullName()));
break;
case SCHEMA:
NameIdentifierUtil.checkSchema(identifier);
- check(env.schemaDispatcher().schemaExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.schemaDispatcher().schemaExists(identifier),
+ () -> new NoSuchSchemaException("Schema %s doesn't exist",
object.fullName()));
break;
case FILESET:
NameIdentifierUtil.checkFileset(identifier);
- check(env.filesetDispatcher().filesetExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.filesetDispatcher().filesetExists(identifier),
+ () -> new NoSuchFilesetException("Fileset %s doesn't exist",
object.fullName()));
break;
case TABLE:
NameIdentifierUtil.checkTable(identifier);
- check(env.tableDispatcher().tableExists(identifier),
exceptionToThrowSupplier);
+ check(
+ env.tableDispatcher().tableExists(identifier),
+ () -> new NoSuchTableException("Table %s doesn't exist",
object.fullName()));
break;
case COLUMN:
NameIdentifierUtil.checkColumn(identifier);
NameIdentifier tableIdent =
NameIdentifier.of(identifier.namespace().levels());
- check(env.tableDispatcher().tableExists(tableIdent),
exceptionToThrowSupplier);
+ check(
+ env.tableDispatcher().tableExists(tableIdent),
+ () -> new NoSuchTableException("Table %s doesn't exist",
tableIdent.toString()));
Review Comment:
In the `COLUMN` branch, the not-found message uses `tableIdent.toString()`
while other branches use `object.fullName()`. This can produce inconsistent
identifiers (likely including the metalake for columns but not for
tables/schemas), which makes error messages harder to interpret. Consider
standardizing on a single representation (e.g., always
`identifier`/`NameIdentifier` or always `object.fullName()`) for these
exception messages.
```suggestion
() -> new NoSuchTableException("Table %s doesn't exist",
object.fullName()));
```
--
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]
