singhpk234 commented on a change in pull request #4422:
URL: https://github.com/apache/iceberg/pull/4422#discussion_r837047534
##########
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:
> I'm not sure if adding this additional constraint provides that much
benefit.
+1, I also expected that code should be present anyways, absence of it
during build() time we check code is not null for creating Response obj, and
throw [IllegalArgs
exception](https://github.com/apache/iceberg/blob/master/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java#L132).
The thing that caught my eye was this :
https://github.com/apache/iceberg/blob/513169d95b379ca723b6217681158b4ea1e09d85/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java#L35
here code is already `int` and in the getter we were boxing it to `Integer`
https://github.com/apache/iceberg/blob/513169d95b379ca723b6217681158b4ea1e09d85/core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java#L58-L60
> I think this optimization is possibly a bit premature, but I'll let others
weigh in as well first.
Agree with you, I don't have a strong opinion of must having this change
either. Happy to close this as well :) !!!
--
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]
