Skip to content
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

Fix: Check if bundle resource type is valid #3715

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

abiisnn
Copy link
Contributor

@abiisnn abiisnn commented Feb 14, 2024

Bug

When a customer sends a bundle with invalid "type", currently, we receive a 500 Internal Server Error which is incorrect.

image

Description

Adds validation and return BadRequest when an invalid "type" is received.

Related issues

Addresses [AB#114755].

Testing

  • Added test case
  • Locally tested

image

FHIR Team Checklist

  • Update the title of the PR to be succinct and less than 65 characters
  • Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
  • Tag the PR with the type of update: Bug, Build, Dependencies, Enhancement, New-Feature or Documentation
  • Tag the PR with Open source, Azure API for FHIR (CosmosDB or common code) or Azure Healthcare APIs (SQL or common code) to specify where this change is intended to be released.
  • Tag the PR with Schema Version backward compatible or Schema Version backward incompatible or Schema Version unchanged if this adds or updates Sql script which is/is not backward compatible with the code.
  • CI is green before merge Build Status
  • Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

@abiisnn abiisnn changed the title [DRAFT] Add try-catch for bundle-transactions Fix: Check if sent bundle resource type is valid, if not return BadRequest Feb 22, 2024
@abiisnn abiisnn changed the title Fix: Check if sent bundle resource type is valid, if not return BadRequest Fix: Check if sent bundle resource type is valid Feb 22, 2024
@abiisnn abiisnn marked this pull request as ready for review February 22, 2024 19:29
@abiisnn abiisnn requested a review from a team as a code owner February 22, 2024 19:29
@@ -195,5 +199,16 @@ public void CheckElementsParameter(HttpContext httpContext)
throw new BadRequestException(string.Format(Api.Resources.InvalidElementsParameter, elementsParameterValue));
}
}

public void CheckIfBundleTypeIsInvalid(HttpContext httpContext)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if this check should be in a different place.

@abiisnn abiisnn changed the title Fix: Check if sent bundle resource type is valid Fix: Check if bundle resource type is valid Feb 26, 2024
Comment on lines +91 to +94
fhirRequestContext.BundleIssues.Add(new OperationOutcomeIssue(
OperationOutcomeConstants.IssueSeverity.Error,
OperationOutcomeConstants.IssueType.Exception,
ex.Message));
Copy link
Member

@brendankowitz brendankowitz Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this isn't a recoverable error I don't think this belongs here. BundleIssues are mainly used for reporting issues during Search requests.

...why not just throw BadRequestException here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason if I throw a BadRequest, it does return a 500 error instead of 400 as we want, so I added this as a work around but open to feedback, I will ask Fernando since he has more context!

@abiisnn abiisnn force-pushed the abigailn/add_try_catch_to_bundle branch from fc66020 to 45b68e4 Compare March 28, 2024 18:23
@abiisnn abiisnn force-pushed the abigailn/add_try_catch_to_bundle branch from 45b68e4 to 3e4b22d Compare April 17, 2024 04:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants