Wider catch for exceptions during applying patch. #2192
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -82,7 +82,7 @@ private Resource GetPatchedJsonResource(FhirJsonNode node, JsonPatchDocument ope | |||
| { | |||
| operations.ApplyTo(node.JsonObject); | |||
| } | |||
| catch (JsonPatchException e) | |||
| catch (Exception e) | |||
There was a problem hiding this comment.
What is the exception case that the general catch will cover that the JsonPatchException wont? I'm concerned about this catching and passing over an exception we would want logged.
There was a problem hiding this comment.
ArgmentNullException is one of them.
I also concern with widening catch net, but there is no way to validate JsonPatchDocument and we wrap only one string of code which we have not much control over.
I can add logging if you think it would be useful.
There was a problem hiding this comment.
My thought would be to have the specific exceptions (JsonPatch, ArgumentNull, etc) just throw the request not valid exception, and have a second catch for the general exception that still throws the request not valid exception but also logs the exception. That way we are aware of what is causing failures and we can either add it to the list of exceptions that don't log or fix it if it is a real bug.
There was a problem hiding this comment.
I revert widening, and add validation instead.
| throw new BadRequestException($"{operation.op} is invalid."); | ||
| } | ||
|
|
||
| if (operation.path == null) |
There was a problem hiding this comment.
Should this be?
| if (operation.path == null) | |
| if (string.IsNullOrWhiteSpace(operation.path)) |
There was a problem hiding this comment.
I think empty path is apply to whole resource, so I think it's valid thing.
Also I don't want to write my own validation for operations, it should come from library, so I'll just try to catch ANE during patching instead of this.
| string patchDocument = | ||
| "[{\"op\":\"replace\",\"value\":\"female\"}, {\"op\":\"remove\",\"path\":\"/address\"}]"; | ||
|
|
||
| var exception = await Assert.ThrowsAsync<FhirException>(() => _client.PatchAsync(response.Resource, patchDocument)); | ||
| Assert.Equal(HttpStatusCode.BadRequest, exception.Response.StatusCode); | ||
|
|
||
| patchDocument = | ||
| "[{\"op\":\"coo\",\"path\":\"/gender\",\"value\":\"female\"}, {\"op\":\"remove\",\"path\":\"/address\"}]"; |
Ivanidzo4ka
added
Bug
Description
JsonPatchDocument can be formatted in a way it would be accepted by server, but during patch it would throw error.
One of the known cases: absence of
pathin operation.I'm making wide catch statement to catch exceptions and return bad request rather than 500.
Related issues
Addresses [#84831].
Testing
Unit test added.
FHIR Team Checklist
Semver Change (docs)
Patch