Copilot commented on code in PR #12880:
URL: https://github.com/apache/gravitino/pull/12880#discussion_r3922961649
##########
server-common/src/main/java/org/apache/gravitino/server/web/Utils.java:
##########
@@ -189,17 +189,43 @@ public static Response nonEmpty(String type, String
message, Throwable throwable
.build();
}
+ /**
+ * Returns an HTTP 501 response for functionality that the server does not
implement.
+ *
+ * @param message the error message
+ * @return the HTTP response
+ */
public static Response unsupportedOperation(String message) {
return unsupportedOperation(message, null);
}
+ /**
+ * Returns an HTTP 501 response for functionality that the server does not
implement.
+ *
+ * @param message the error message
+ * @param throwable the exception that caused the error
+ * @return the HTTP response
+ */
public static Response unsupportedOperation(String message, Throwable
throwable) {
- return Response.status(Response.Status.METHOD_NOT_ALLOWED)
+ return Response.status(Response.Status.NOT_IMPLEMENTED)
.entity(ErrorResponse.unsupportedOperation(message, throwable))
.type(MediaType.APPLICATION_JSON)
.build();
}
+ /**
+ * Returns an HTTP 405 response when the target resource does not allow the
request method.
+ *
+ * @param message the error message
+ * @return the HTTP response
+ */
+ public static Response methodNotAllowed(String message) {
+ return Response.status(Response.Status.METHOD_NOT_ALLOWED)
+ .entity(ErrorResponse.unsupportedOperation(message))
+ .type(MediaType.APPLICATION_JSON)
+ .build();
Review Comment:
`Utils.methodNotAllowed(...)` builds a 405 response but uses
`ErrorResponse.unsupportedOperation(...)` as the payload, which contradicts the
method’s JavaDoc and can confuse future maintainers. Either document that this
helper intentionally reuses the unsupported-operation error body for backward
compatibility, or introduce a dedicated error payload/type for 405.
##########
server/src/main/java/org/apache/gravitino/server/web/rest/StatisticOperations.java:
##########
@@ -172,7 +172,7 @@ public Response updateStatistics(
MetadataObjects.parse(
fullName,
MetadataObject.Type.valueOf(type.toUpperCase(Locale.ROOT)));
if (object.type() != MetadataObject.Type.TABLE) {
- throw new UnsupportedOperationException(
+ throw new IllegalArgumentException(
"Update statistics is only supported for tables now.");
}
Review Comment:
The switch to `IllegalArgumentException` for non-table object types affects
multiple endpoints (update/drop and partition stats), but the new test coverage
only exercises the list endpoint. To ensure all statistics endpoints
consistently return HTTP 400 for non-table types, add tests that call the
update/drop/partition routes with a non-table `type` (e.g., `CATALOG`) and
assert `400` with `ILLEGAL_ARGUMENTS_CODE`.
--
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]
