kbendick commented on a change in pull request #4422:
URL: https://github.com/apache/iceberg/pull/4422#discussion_r836787865
##########
File path:
core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponseParser.java
##########
@@ -91,7 +91,7 @@ public static ErrorResponse fromJson(JsonNode jsonNode) {
JsonNode error = jsonNode.get(ERROR);
String message = JsonUtil.getStringOrNull(MESSAGE, error);
String type = JsonUtil.getStringOrNull(TYPE, error);
- Integer code = JsonUtil.getIntOrNull(CODE, error);
+ int code = JsonUtil.getInt(CODE, error);
Review comment:
Question: if `CODE` isn't present, what exception will this throw?
##########
File path:
core/src/test/java/org/apache/iceberg/rest/responses/TestErrorResponseParser.java
##########
@@ -40,14 +41,25 @@ public void testErrorResponseToJson() {
public void testErrorResponseFromJson() {
String message = "The given namespace does not exist";
String type = "NoSuchNamespaceException";
- Integer code = 404;
+ int code = 404;
String errorModelJson =
String.format("{\"message\":\"%s\",\"type\":\"%s\",\"code\":%d}", message,
type, code);
String json = "{\"error\":" + errorModelJson + "}";
ErrorResponse expected =
ErrorResponse.builder().withMessage(message).withType(type).responseCode(code).build();
assertEquals(expected, ErrorResponseParser.fromJson(json));
}
+ @Test
+ public void testErrorResponseFromJsonWithoutCode() {
+ String message = "The given namespace does not exist";
+ String type = "NoSuchNamespaceException";
+ String errorModelJson =
String.format("{\"message\":\"%s\",\"type\":\"%s\"}", message, type);
+ String json = "{\"error\":" + errorModelJson + "}";
+
+ AssertHelpers.assertThrows("ErrorResponse should contain code",
IllegalArgumentException.class,
+ "Cannot parse missing int code", () ->
ErrorResponseParser.fromJson(json));
+ }
Review comment:
My concern here is that the error code can still be parsed from other
places (like the response itself). I agree it _should_ be there, but I'm not
sure if adding this additional constraint provides that much benefit.
The number of boxed `Integer`s is still likely to be relatively small, as
most operations will still go through normal `FileIO`.
I think this optimization is possibly a bit premature, but I'll let others
weigh in as well first.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
