jerryshao commented on code in PR #10282:
URL: https://github.com/apache/gravitino/pull/10282#discussion_r2894495137
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -220,7 +239,7 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
try {
env.tagDispatcher().getTag(metalake, object.fullName());
} catch (NoSuchTagException nsr) {
- throw exceptionToThrowSupplier.get();
+ throw nsr;
}
Review Comment:
Same as the `ROLE` case: this `try/catch` is redundant and adds noise.
Remove it and let `NoSuchTagException` propagate directly.
##########
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;
+import org.apache.gravitino.exceptions.NoSuchMetalakeException;
Review Comment:
`NoSuchMetadataObjectException` is imported but no longer used anywhere in
this file after the switch to type-specific exceptions. This will fail
Spotless/Checkstyle — please remove the unused import.
##########
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;
Review Comment:
The error message for the `COLUMN` case uses `tableIdent.toString()` while
every other branch uses `object.fullName()`. This produces inconsistent
identifier formats in error output. Please standardize to `object.fullName()`
here as well:
```suggestion
() -> new NoSuchTableException("Table %s doesn't exist",
object.fullName()));
```
##########
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);
+ Assertions.assertEquals(NoSuchSchemaException.class.getSimpleName(),
errorResp.getType());
+ }
Review Comment:
Consider also asserting `errorResp.getCode()` to align with other tests in
this class and make the error payload contract more explicit:
```suggestion
Assertions.assertEquals(ErrorConstants.NOT_FOUND_CODE,
errorResp.getCode());
Assertions.assertEquals(NoSuchSchemaException.class.getSimpleName(),
errorResp.getType());
```
##########
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:
This `try/catch` block catches `NoSuchRoleException` and immediately
rethrows it unchanged — it is a no-op. Remove the `try/catch` entirely and let
the exception propagate directly from `getRole(...)`.
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -238,7 +257,7 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
try {
env.jobOperationDispatcher().getJob(metalake, object.fullName());
} catch (NoSuchJobException e) {
- throw exceptionToThrowSupplier.get();
+ throw e;
}
Review Comment:
Same as above: redundant `try/catch`. Remove it.
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -247,7 +266,7 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
try {
env.jobOperationDispatcher().getJobTemplate(metalake,
object.fullName());
} catch (NoSuchJobTemplateException e) {
- throw exceptionToThrowSupplier.get();
+ throw e;
}
Review Comment:
Same as above: redundant `try/catch`. Remove it.
##########
core/src/main/java/org/apache/gravitino/utils/MetadataObjectUtil.java:
##########
@@ -229,7 +248,7 @@ public static void checkMetadataObject(String metalake,
MetadataObject object) {
try {
env.policyDispatcher().getPolicy(metalake, object.fullName());
} catch (NoSuchPolicyException nsr) {
- throw checkNotNull(exceptionToThrowSupplier).get();
+ throw nsr;
}
Review Comment:
Same as above: the `try/catch` is a no-op. Remove it.
##########
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);
Review Comment:
Nit: grammar — use "does not exist" or "doesn't exist" instead of "not
exists".
```suggestion
// Mock schema does not exist
```
--
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]
