Merge ConfigFetchers #181
Merge ConfigFetchers #181
Conversation
| return call.call(); | ||
| } catch (ConfigNotFoundException e) { | ||
| throw new KnownException( | ||
| HttpStatus.NOT_FOUND_404, |
There was a problem hiding this comment.
This is a behavior change. Before we were sending 422. Let me know if 404 is incorrect.
There was a problem hiding this comment.
422 is safer for now. remember that the getXConfig can be called internally in any of these handlers. e.g. connections/create might internally retrieve the sourceImplementation that it's going to be associated with to validate it exists. If it fails to find that sourceImplementation I believe the correct https status code would be 422. There are cases though where we try to fetch a config and failure to do so should return a 404. In our current scheme we sometimes return 422 when we should probably be returning a 404. To get this right we need to go to each invocation for configPersistence.getConfig(...) and throw the appropriate exception either 404 or 422.
There was a problem hiding this comment.
I will restore 422 but I think it needs some re-work. If what you describe happens then the code that validates should catch the ConfigNotFoundException and rethrow it as a ValidationException.
| .withSchema(SchemaConverter.toPersistenceSchema(connectionUpdate.getSyncSchema())) | ||
| .withStatus(toPersistenceStatus(connectionUpdate.getStatus())); | ||
|
|
||
| // get existing schedule | ||
| final StandardSyncSchedule persistedSchedule = getSyncSchedule(connectionId); | ||
| // get retrieve schedule |
There was a problem hiding this comment.
| // get retrieve schedule | |
| // retrieve schedule |
| final UUID connectionId = connectionUpdate.getConnectionId(); | ||
|
|
||
| // get existing sync | ||
| final StandardSync persistedSync = getStandardSync(connectionId) | ||
| // retrieve existing sync |
There was a problem hiding this comment.
| // retrieve existing sync | |
| // retrieve sync |
| return call.call(); | ||
| } catch (ConfigNotFoundException e) { | ||
| throw new KnownException( | ||
| HttpStatus.NOT_FOUND_404, |
There was a problem hiding this comment.
422 is safer for now. remember that the getXConfig can be called internally in any of these handlers. e.g. connections/create might internally retrieve the sourceImplementation that it's going to be associated with to validate it exists. If it fails to find that sourceImplementation I believe the correct https status code would be 422. There are cases though where we try to fetch a config and failure to do so should return a 404. In our current scheme we sometimes return 422 when we should probably be returning a 404. To get this right we need to go to each invocation for configPersistence.getConfig(...) and throw the appropriate exception either 404 or 422.
| return execute(() -> webBackendConnectionsHandler.webBackendGetConnection(connectionIdRequestBody)); | ||
| } | ||
|
|
||
| private <T> T execute(HandlerCall<T> call) { |
There was a problem hiding this comment.
this seems pretty crufty to me. is this a common practice with jersey? i haven't found much in the way of custom error handling that is satisfying.
it would be nice if we could just get the generated code to have reasonable exceptions in the method signature. i looked a little quickly and couldn't figure out how to do it but it is something i plan to look into more.
There was a problem hiding this comment.
I am not the biggest fan of this design either. the thing I like about it though is that it centralizes were exceptions are transformed into responses.
|
|
||
| private interface HandlerCall<T> { | ||
|
|
||
| T call() throws ConfigNotFoundException, IOException, JsonValidationException; |
There was a problem hiding this comment.
how does this scale with more exceptions? do we just keep adding to this method signature? or do we have different handlers for different api calls.
There was a problem hiding this comment.
I am thinking that for now we add more exceptions.
Fixing pointer to be correct for source-jira workflows stream
What
Merge
ConfigFetchers(#187)How
Create a layer on top of
ConfigPersistenceThe Exception mgmt is open for discussion, I made the choice of handling them at the consumer level.
There is one behavior change (see comment)
Recommended reading order