-
Notifications
You must be signed in to change notification settings - Fork 518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updating the error message during conditional reference #4691
base: main
Are you sure you want to change the base?
Conversation
…nts to Multiple Records
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@@ -70,10 +70,14 @@ public async Task ResolveReferencesAsync(Resource resource, IDictionary<string, | |||
|
|||
var results = await GetExistingResourceId(requestUrl, resourceType, conditionalQueries, cancellationToken); | |||
|
|||
if (results == null || results.Count != 1) | |||
if (results == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be safe, can you add results.Count == 0 to this if statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LTA-Thinking Updated the if statement. Please review it.
{ | ||
throw new RequestNotValidException(string.Format(Core.Resources.InvalidConditionalReference, reference.Reference)); | ||
} | ||
else if (results.Count != 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And could you change this check to be if the count is greater than 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For conditional, we want exactly 1, my only other thought here was 0 (if thats possible), which would still give the wrong error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LTA-Thinking , @brendankowitz Updated the if else statement which cover both count = 0 and count >1 condition.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run |
Commenter does not have sufficient privileges for PR 4691 in repo microsoft/fhir-server |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Description
The PR updates the error message of conditional references when multiple resources are found. Instead of stating "does not resolve to a resource," the message will now indicate that the reference resolved to multiple resources.
Related issues
Addresses issue #77964.
Testing
These changes were tested on the OSS FHIR server, and we received the expected error output.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)