amogh-jahagirdar commented on code in PR #8760:
URL: https://github.com/apache/iceberg/pull/8760#discussion_r1351999949
##########
core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponse.java:
##########
@@ -22,18 +22,17 @@
import java.io.StringWriter;
import java.util.Arrays;
import java.util.List;
-import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
import org.apache.iceberg.rest.RESTResponse;
/** Standard response body for all API errors */
public class ErrorResponse implements RESTResponse {
private String message;
private String type;
- private int code;
+ private Integer code;
Review Comment:
I think this needs to still be `int`, since `code` is required?
https://github.com/apache/iceberg/blob/master/open-api/rest-catalog-open-api.yaml#L1108
, which makes sense we should always have some known status code.
##########
core/src/main/java/org/apache/iceberg/rest/responses/ErrorResponseParser.java:
##########
@@ -76,17 +76,20 @@ public static ErrorResponse fromJson(JsonNode jsonNode) {
jsonNode != null && jsonNode.isObject(),
"Cannot parse error response from non-object value: %s",
jsonNode);
- Preconditions.checkArgument(jsonNode.has(ERROR), "Cannot parse missing
field: error");
- JsonNode error = JsonUtil.get(ERROR, jsonNode);
- String message = JsonUtil.getStringOrNull(MESSAGE, error);
- String type = JsonUtil.getStringOrNull(TYPE, error);
- Integer code = JsonUtil.getIntOrNull(CODE, error);
- List<String> stack = JsonUtil.getStringListOrNull(STACK, error);
- return ErrorResponse.builder()
- .withMessage(message)
- .withType(type)
- .responseCode(code)
- .withStackTrace(stack)
- .build();
+ if (jsonNode.has(ERROR)) {
+ JsonNode error = JsonUtil.get(ERROR, jsonNode);
+ String message = JsonUtil.getStringOrNull(MESSAGE, error);
+ String type = JsonUtil.getStringOrNull(TYPE, error);
+ Integer code = JsonUtil.getIntOrNull(CODE, error);
+ List<String> stack = JsonUtil.getStringListOrNull(STACK, error);
+ return ErrorResponse.builder()
+ .withMessage(message)
+ .withType(type)
+ .responseCode(code)
+ .withStackTrace(stack)
+ .build();
+ } else {
+ return ErrorResponse.builder().build();
+ }
Review Comment:
I maybe missing something but this is `ErrorResponseParser` no? I'd expect
whatever JSON that gets passed to this to have the error. I just assumed the
response model isn't marked as required in the REST spec, because it depends on
if an error is thrown as part of the call. If it is thrown, I'd expect all
these fields to be set. Maybe there's a better way to convey it in the spec.
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
