This is an automated email from the ASF dual-hosted git repository.
jshao pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new 1205285d4e [#10660] fix: Move model version request validation inside
try block (#10663)
1205285d4e is described below
commit 1205285d4e035c2ceccd15dc2fc3354adb3480dd
Author: yunhwan <[email protected]>
AuthorDate: Wed Apr 8 16:35:30 2026 +0900
[#10660] fix: Move model version request validation inside try block
(#10663)
### What changes were proposed in this pull request?
Move `request.validate()` inside the `try` block in `alterModelVersion`
and `alterModelVersionByAlias` methods of `ModelOperations.java`, so
that `IllegalArgumentException` from invalid requests is properly caught
by `ExceptionHandlers.handleModelException()`.
### Why are the changes needed?
`request.validate()` was called before the `try/catch` block in both
`alterModelVersion` and `alterModelVersionByAlias`. When validation
fails (e.g., `{"updates": null}`), the `IllegalArgumentException`
bypasses `ExceptionHandlers.handleModelException()` and results in a 500
Internal Server Error instead of the expected 400 Bad Request.
The duplicate `request.validate()` call inside the `try` block (within
the lambda) already exists, so removing the pre-try call is sufficient.
Fix: #10660
### Does this PR introduce _any_ user-facing change?
No. This fixes error response codes for invalid REST API requests from
500 to 400.
### How was this patch tested?
Added two unit tests:
- `testAlterModelVersionWithNullUpdates`: Sends `{"updates":null}` to
the version endpoint, asserts 400 Bad Request with
`ILLEGAL_ARGUMENTS_CODE`
- `testAlterModelVersionByAliasWithNullUpdates`: Sends
`{"updates":null}` to the alias endpoint, asserts 400 Bad Request with
`ILLEGAL_ARGUMENTS_CODE`
All existing `TestModelOperations` tests continue to pass.
---
.../gravitino/server/web/rest/ModelOperations.java | 12 +++----
.../server/web/rest/PartitionOperations.java | 30 ++++++++--------
.../server/web/rest/TestModelOperations.java | 40 ++++++++++++++++++++++
3 files changed, 59 insertions(+), 23 deletions(-)
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/ModelOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/ModelOperations.java
index 66b292b2cf..3c5dd63946 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/ModelOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/ModelOperations.java
@@ -180,13 +180,12 @@ public class ModelOperations {
request.getName());
try {
- request.validate();
- NameIdentifier modelId =
- NameIdentifierUtil.ofModel(metalake, catalog, schema,
request.getName());
-
return Utils.doAs(
httpRequest,
() -> {
+ request.validate();
+ NameIdentifier modelId =
+ NameIdentifierUtil.ofModel(metalake, catalog, schema,
request.getName());
Model m =
modelDispatcher.registerModel(
modelId, request.getComment(), request.getProperties());
@@ -422,11 +421,10 @@ public class ModelOperations {
NameIdentifier modelId = NameIdentifierUtil.ofModel(metalake, catalog,
schema, model);
try {
- request.validate();
-
return Utils.doAs(
httpRequest,
() -> {
+ request.validate();
Map<String, String> tmpUris =
Optional.ofNullable(request.getUris()).orElse(Collections.emptyMap());
ImmutableMap.Builder<String, String> uris =
@@ -575,7 +573,6 @@ public class ModelOperations {
schema,
model,
version);
- request.validate();
try {
NameIdentifier modelId = NameIdentifierUtil.ofModel(metalake, catalog,
schema, model);
@@ -630,7 +627,6 @@ public class ModelOperations {
schema,
model,
alias);
- request.validate();
try {
NameIdentifier modelId = NameIdentifierUtil.ofModel(metalake, catalog,
schema, model);
diff --git
a/server/src/main/java/org/apache/gravitino/server/web/rest/PartitionOperations.java
b/server/src/main/java/org/apache/gravitino/server/web/rest/PartitionOperations.java
index cc46d88bf5..85aebb6fdd 100644
---
a/server/src/main/java/org/apache/gravitino/server/web/rest/PartitionOperations.java
+++
b/server/src/main/java/org/apache/gravitino/server/web/rest/PartitionOperations.java
@@ -181,24 +181,24 @@ public class PartitionOperations {
@PathParam("table") @AuthorizationMetadata(type =
Entity.EntityType.TABLE) String table,
AddPartitionsRequest request) {
try {
- if (request == null || request.getPartitions() == null) {
- throw new IllegalArgumentException("partitions must not be null");
- }
- LOG.info(
- "Received add {} partition(s) request for table {}.{}.{}.{} ",
- request.getPartitions().length,
- metalake,
- catalog,
- schema,
- table);
- Preconditions.checkArgument(
- request.getPartitions().length == 1, "Only one partition is
supported");
-
- request.validate();
-
return Utils.doAs(
httpRequest,
() -> {
+ if (request == null || request.getPartitions() == null) {
+ throw new IllegalArgumentException("partitions must not be
null");
+ }
+ LOG.info(
+ "Received add {} partition(s) request for table {}.{}.{}.{} ",
+ request.getPartitions().length,
+ metalake,
+ catalog,
+ schema,
+ table);
+ Preconditions.checkArgument(
+ request.getPartitions().length == 1, "Only one partition is
supported");
+
+ request.validate();
+
NameIdentifier tableIdent = NameIdentifier.of(metalake, catalog,
schema, table);
Partition p = dispatcher.addPartition(tableIdent,
fromDTO(request.getPartitions()[0]));
Response response =
diff --git
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestModelOperations.java
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestModelOperations.java
index 521096a717..12c1695768 100644
---
a/server/src/test/java/org/apache/gravitino/server/web/rest/TestModelOperations.java
+++
b/server/src/test/java/org/apache/gravitino/server/web/rest/TestModelOperations.java
@@ -1395,6 +1395,46 @@ public class TestModelOperations extends
BaseOperationsTest {
Assertions.assertEquals(uris, modelVersion.uris());
}
+ @Test
+ void testAlterModelVersionWithNullUpdates() {
+ ModelVersionUpdatesRequest req = new ModelVersionUpdatesRequest(null);
+
+ Response resp =
+ target(modelPath())
+ .path("model1")
+ .path("versions")
+ .path("0")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .put(Entity.entity(req, MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
+
+ ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
errorResp.getCode());
+ Assertions.assertEquals(IllegalArgumentException.class.getSimpleName(),
errorResp.getType());
+ }
+
+ @Test
+ void testAlterModelVersionByAliasWithNullUpdates() {
+ ModelVersionUpdatesRequest req = new ModelVersionUpdatesRequest(null);
+
+ Response resp =
+ target(modelPath())
+ .path("model1")
+ .path("aliases")
+ .path("alias1")
+ .request(MediaType.APPLICATION_JSON_TYPE)
+ .accept("application/vnd.gravitino.v1+json")
+ .put(Entity.entity(req, MediaType.APPLICATION_JSON_TYPE));
+
+ Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
+
+ ErrorResponse errorResp = resp.readEntity(ErrorResponse.class);
+ Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
errorResp.getCode());
+ Assertions.assertEquals(IllegalArgumentException.class.getSimpleName(),
errorResp.getType());
+ }
+
@Test
public void testGetModelVersionUri() {
NameIdentifier modelIdent = NameIdentifierUtil.ofModel(metalake, catalog,
schema, "model1");
