[Bundles] Ensure client errors are HTTP400#5589
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5589 +/- ##
==========================================
+ Coverage 77.33% 77.37% +0.03%
==========================================
Files 996 996
Lines 36534 36585 +51
Branches 5538 5547 +9
==========================================
+ Hits 28255 28309 +54
+ Misses 6912 6903 -9
- Partials 1367 1373 +6 🚀 New features to boost your workflow:
|
fhibf
added
Bug
fhibf
changed the title
fhibf
changed the title
fhibf
changed the title
| { | ||
| // Ensure that, if a transaction fails with a client error, then the exception with the customer error type is prioritized. | ||
| // In the following code, we are prioritizing exceptions of type FhirTransactionFailedException. | ||
| FhirTransactionFailedException customerException = aggregateException.InnerExceptions |
There was a problem hiding this comment.
Do we define what is client exception anywhere? Is there a centralized list? Could be an opprotunity to define somewhere in the future :)
Example - do we return 410 (gone) if a customer is doing a versioned request and the version is not there?
| }; | ||
| } | ||
|
|
||
| // Second entry: no handler set, simulating a route that was not resolved. |
There was a problem hiding this comment.
Will other entries return a 404? Since these are all client errors too would it be best to return a 200 which is not a client error? The method that chooses the return exception takes the first that matches - so this tests depends on that first behavior to return the correct exception. Probably okay - but wondering if this test is fully accurate per the title. :)
| /// <summary> | ||
| /// Given a list of exceptions raised during a bundle execution, prioritizes the exceptions of type <see cref="FhirTransactionFailedException"/>, based on their status code, to determine which exception should be returned to the customer. | ||
| /// </summary> | ||
| public static FhirTransactionFailedException GetPrioritizedClientException(Exception exception) |
There was a problem hiding this comment.
Should we add any unit tests for this class?
| { | ||
| // Ensure that, if a transaction fails with a client error, then the exception with the customer error type is prioritized. | ||
| // In the following code, we are prioritizing exceptions of type FhirTransactionFailedException. | ||
| FhirTransactionFailedException customerException = aggregateException.InnerExceptions |
There was a problem hiding this comment.
How deep can the exceptions go? If we go lower than on level of nesting should ew use Flatten?
https://learn.microsoft.com/en-us/dotnet/api/system.aggregateexception.flatten?view=netstandard-2.1
| get; | ||
| } | ||
|
|
||
| public bool IsTransactionFailedByClientCancellation => _transactionFailedByClientCancellation == 1; |
There was a problem hiding this comment.
do we use this anywhere?
|
|
||
| FhirResponse<Bundle> bundleResponse = await _client.PostBundleAsync(bundle, new FhirBundleOptions() { BundleProcessingLogic = processingLogic }); | ||
|
|
||
| Assert.True(bundleResponse.StatusCode == HttpStatusCode.OK, "HTTP Status code is different than HTTP200."); |
There was a problem hiding this comment.
nit - Assert.Equal is more correct
|
|
||
| private sealed class BundleExecutionContext | ||
| { | ||
| private int _transactionFailedByClientCancellation; |
There was a problem hiding this comment.
Is this read anywhere?
|
|
||
| public bool IsTransactionFailedByClientCancellation => _transactionFailedByClientCancellation == 1; | ||
|
|
||
| public bool IsTransactionFailedByClientError => _transactionFailedByClientError == 1; |
There was a problem hiding this comment.
Should we use a volatile read here for some extra safety?
| public bool IsTransactionFailedByClientError => _transactionFailedByClientError == 1; | |
| public bool IsTransactionFailedByClientError => Volatile.Read(ref _transactionFailedByClientError) == 1; |
3 participants
Description
In Parallel Bundles we are now identifying that one of the operations failed with a HTTP400, and the remaining ones are cancelled and marked with HTTP424 (FailedDependency) in logs.
There are now more tests to ensure that HTTP400 is properly returned in those cases.
These changes will also avoid marking bundles as "Failed by client Error" and "Cancelled by Client timeout".
Related issues
Addresses AB#192974.
Testing
Tested locally and through Unit and E2E tests.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)