Standardize error structure in json responses. #4170
Conversation
…ising exceptions instead of http status codes peppered throughout the code.
github-actions
bot
added
area/api
| throw new KnownException( | ||
| HttpStatus.UNPROCESSABLE_ENTITY_422, | ||
| String.format("Could not find configuration for %s: %s.", e.getType().toString(), e.getConfigId()), e); | ||
| throw new IdNotFoundException(String.format("Could not find configuration for %s: %s.", e.getType().toString(), e.getConfigId()), e); |
There was a problem hiding this comment.
seeing these various exceptions that extend KnownException next to each other, I'm wondering if there's any naming convention we should adopt so it's clearer what these exceptions are doing. Because they're now not obviously known exceptions its hard to get the queue that these will be handled beautifully in the API. e.g. add KnownException (or some abbreviation, or some other naming convention entirely).
There was a problem hiding this comment.
Yeah, I agree. They should all include KnownException in the name, so that the hierarchy is more obvious, and also so they show up as autocomplete suggestions during IDE searches.
…te json objects. Also uses cleaner helpers for exception mappers.
| @@ -2667,8 +2667,94 @@ components: | |||
| - failed | |||
| reason: | |||
| type: string | |||
| InvalidInputProperty: | |||
There was a problem hiding this comment.
bummer that we have to lose the inheritance. i think it would be worthwhile leaving a comment explaining what the "required" fields of all exceptions are. maybe also worth mentioning why we are duplicating attributes as opposed to using inheritance.
|
|
||
| public NotFoundKnownExceptionInfo getNotFoundKnownExceptionInfo() { | ||
| NotFoundKnownExceptionInfo exceptionInfo = new NotFoundKnownExceptionInfo() | ||
| .exceptionClassName(this.getClass().getName()) |
There was a problem hiding this comment.
i don't think you should have to use this in front of any of these method calls. welcome to keep it if you think it is clearer.
airbyte-server/src/main/java/io/airbyte/server/errors/NotFoundExceptionMapper.java
Outdated
Show resolved
Hide resolved
Standardize error structure in json responses. Use named types for raising exceptions instead of http status codes peppered throughout the code.
What
Various exception mappers created varying json structs for returning exception info. Some provided message and details, while others only provided message. Message was sometimes the details and other times just a title. HTTP status codes were determined in many places in the code, causing code repetition. Resulting errors were unpredictable for the UI to parse and display. Lack of consistency also made it hard to add tracking details later like request id.
How
KnownException has been standardized as the abstract parent of custom exceptions for various scenarios, and returns an ExceptionInfo in the json api response. The API is updated to support this, so the UI can rely on it.
Code now throws more specific named exceptions instead of needing to know an http status code, and status code is determined by the concrete child exception. This also adds stack trace lines for exception detail, and, if present, root cause exception, so that the UI can optionally show these where appropriate.
Recommended reading order
airbyte-api/src/main/openapi/config.yaml(end of file, data structure for ExceptionInfo)airbyte-server/src/main/java/io/airbyte/server/errors/KnownExceptionMapper.javaairbyte-server/src/main/java/io/airbyte/server/errors/KnownException.javaairbyte-server/src/main/java/io/airbyte/server/errors/ValueConflictException.javaairbyte-server/src/main/java/io/airbyte/server/apis/ConfigurationApi.javaCloses #3666