justinmclean opened a new issue, #10660:
URL: https://github.com/apache/gravitino/issues/10660
### What would you like to be improved?
ModelOperations.alterModelVersion and alterModelVersionByAlias call
request.validate() before entering their try/catch.
Relevant paths:
/server/src/main/java/org/apache/gravitino/server/web/rest/ModelOperations.java
/common/src/main/java/org/apache/gravitino/dto/requests/ModelVersionUpdatesRequest.java
/server/src/main/java/org/apache/gravitino/server/web/rest/ExceptionHandlers.java
ModelVersionUpdatesRequest.validate() throws IllegalArgumentException for
invalid input. Because that validation happens before the endpoint enters the
try, the exception does not reach ExceptionHandlers.handleModelException(...),
which is the code path that maps IllegalArgumentException to a 400 Bad Request.
The server registers JSON parse/mapping exception mappers, but not a general
IllegalArgumentException mapper, so this invalid request can fall into Jersey’s
unhandled exception path instead of returning the endpoint’s intended
bad-request response.
This affects:
PUT /.../{model}/versions/{version}
PUT /.../{model}/aliases/{alias}
### How should we improve?
Move the outer request.validate() call inside the try block in both methods,
or remove the duplicate pre-try validation and keep the validation already
performed inside Utils.doAs(...).
A minimal safe fix is:
- keep a single request.validate() inside the try path before business logic
- ensure both endpoints route validation failures through
ExceptionHandlers.handleModelException(...)
Here's a unit test to help:
```
@Test
void testUpdateModelVersionWithInvalidRequest() {
String invalidRequest = "{\"updates\":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(invalidRequest,
MediaType.APPLICATION_JSON_TYPE));
Assertions.assertEquals(Response.Status.BAD_REQUEST.getStatusCode(),
resp.getStatus());
ErrorResponse errorResponse = resp.readEntity(ErrorResponse.class);
Assertions.assertEquals(ErrorConstants.ILLEGAL_ARGUMENTS_CODE,
errorResponse.getCode());
Assertions.assertEquals(
IllegalArgumentException.class.getSimpleName(),
errorResponse.getType());
Assertions.assertTrue(errorResponse.getMessage().contains("updates
cannot be null"));
verifyNoInteractions(modelDispatcher);
}
```
--
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]
