Fix SchemaNotFoundException returns 5xx #1246
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mareckmareck
previously approved these changes
Aug 3, 2020
| try { | ||
| if (online) { | ||
| List<SchemaVersion> v = rawSchemaClient.getVersions(topic.getName()); | ||
| versionsCache.put(topic, v); | ||
| return v; | ||
| return SchemaVersionsResponse.succeeded(v); |
There was a problem hiding this comment.
v -> versions? Since in the else branch it is fully named.
|
|
||
| import static java.util.Collections.emptyList; | ||
|
|
||
| public class SchemaVersionsResponse { |
There was a problem hiding this comment.
Not sure if Response is the best suited name here. Perhaps Result would be better?
druminski
reviewed
Aug 3, 2020
| assertThat(response.getStatus()).isEqualTo(INTERNAL_SERVER_ERROR.getStatusCode()); | ||
| assertThat(response.getStatus()).isEqualTo(BAD_REQUEST.getStatusCode()); | ||
| assertThat(response.readEntity(String.class)) | ||
| .isEqualTo("{\"message\":\"Given schema version does not exist\",\"code\":\"SCHEMA_VERSION_DOES_NOT_EXIST\"}"); |
There was a problem hiding this comment.
Can we also return what schema version doesn’t exist in http response? This information will be useful for clients.
druminski
approved these changes
Aug 3, 2020
mareckmareck
approved these changes
Aug 3, 2020
pwolaq
pushed a commit
to pwolaq/hermes
that referenced
this pull request
Oct 15, 2020
Return 400 Bad Request in case of non-existing schema version
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background context
#1241
Changes
SchemaRepository.versionsfromList<SchemaVersion>toSchemaVersionsResponseSchemaRepository.versionswas returning an empty list in case of failed request. To return 400 status to the client in case of non-existing schema version it was necessary to distinguish between the missing schema version and failed request.The first option was to throw an exception, but I decided to wrap the result into
SchemaVersionsResponseto avoid the overhead of throwing an exception. Additionally it would be necessary to handle this exception in all places that invoke this method.SchemaVersionsResponseis a wrapper that contains status of request execution and a list of versions (empty list in case of failure).