Core: Adding additional information in RestException #14927
Conversation
| } | ||
|
|
||
| throw new RESTException("Unable to process: %s", error.message()); | ||
| throw new RESTException( |
There was a problem hiding this comment.
while we are here can we also add error code and type in
There was a problem hiding this comment.
But here it's clear because this exception is triggered when the code is 422, which is Unprocessable Entity, then the current message for me is enough.
There was a problem hiding this comment.
IMHO its more about consistency than enhancing, for example we can say all the rest exception follow this pattern, since we don't have defined exception for it, it would be great to have this consistently have an error message format, WDYT ?
| throw new RESTException("Unable to process: %s", error.message()); | ||
| throw new RESTException( | ||
| "Unable to process (code: %s, type: %s): %s", | ||
| error.code(), error.type() != null ? error.type() : "unknown", error.message()); |
There was a problem hiding this comment.
i wonder if making the null as unknown adds more value ? how about just having error.type() ?
| case 409: | ||
| throw new AlreadyExistsException("%s", error.message()); | ||
| case 422: | ||
| throw new RESTException("Unable to process: %s", error.message()); |
There was a problem hiding this comment.
The namespace handler had a inconsistency in the 422 case, maybe we can remove this to fall back to the new behavior in DefaultErrorHandler?
| .withMessage("Invalid input") | ||
| .build(); | ||
|
|
||
| assertThatThrownBy(() -> ErrorHandlers.defaultErrorHandler().accept(error)) |
There was a problem hiding this comment.
Can we add some tests with out the message and type as they're optional
| import java.util.stream.Stream; | ||
| import org.apache.iceberg.catalog.Namespace; | ||
| import org.apache.iceberg.exceptions.RESTException; | ||
| import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap; |
There was a problem hiding this comment.
Nit: This test is for ErrorHandlers, not HTTPRequest. Wdyt about making a test file for this behavior.
3 participants
There are scenarios where there are
RESTExceptionissues but the message is providing value. For instance:So, the idea is to provide
codeandtypeinRESTExceptionto get more context.