Return BadRequestException with valid message when input json body is invalid

[apurvabhaleMS]

Description

For invalid json body requests Fhir server returns 500. If the input json is not in the correct format then we are parsing the body at FhirJsonInputFormatter and passing the initial validations for required fields at ModelAttributeValidator.

e.g.
If the body contains Coverage.status = "", then after parsing Coverage.status = null & Coverage.statusElement = null, resulting into minimum cardinality error as expected.
If the body contains Coverage.status = , then after parsing Coverage.status = null & Coverage.statusElement = {value=null}, which passes the Firely validation and CodeToTokenSearchValueConverter returns null causing search index entry to fail.

In this PR we are returning BadRequestException with a valid message instead of 500

Related issues

Addresses 85266

Testing

1. Check POST Coverage request with - "status" : ,
2. Check POST Member-Match request with - "status" : ,
3. Check POST Claim request with - "use": ,
4. Unit tests

FHIR Team Checklist

• Update the title of the PR to be succinct and less than 50 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, Dependencies, Enhancement, or New-Feature
• Tag the PR with Azure API for FHIR if this will release to the Azure API for FHIR managed service (CosmosDB or common code related to service)
• Tag the PR with Azure Healthcare APIs if this will release to the Azure Healthcare APIs managed service (Sql server or common code related to service)
• Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

[apurvabhaleMS]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[LTA-Thinking]

When submitting a Coverage resource I'm getting some unexpected results.
For this resource (which repros the 500 error without your change) the validator isn't catching that it is missing a required field and is creating the resource.

{
    "resourceType": "Coverage",
    "status":,
    "payor": {
        "reference": "something"
    },
    "beneficiary": {
        "reference": "something"
    }
}

But for this resource I'm getting back the expected failure with a message that the status can't be null.

{
    "resourceType": "Coverage",
    "status": "",
    "payor": {
        "reference": "something"
    },
    "beneficiary": {
        "reference": "something"
    }
}

[brendankowitz]

There was a lot of work put into getting the search indexer out of "Shared" and into FHIR agnostic core, a little sad to see it back :(

[apurvabhaleMS]

:( I can see if I can move this test to Microsoft.Health.Fhir.Core.UnitTests from Microsoft.Health.Fhir.Shared.Core.UnitTests

[brendankowitz]

Should the search indexer handle this instead? This still looks like an ArgumentException in this class.

  foreach (ISearchValue searchValue in ExtractSearchValues(
      searchParameter.Url.ToString(),
      searchParameter.Type,
      searchParameter.TargetResourceTypes,
      resource,
      searchParameter.Expression,
      context))
  {
     if(searchValue != null) {
      yield return new SearchIndexEntry(searchParameterInfo, searchValue);
     }
  }

or

  foreach (ISearchValue searchValue in ExtractSearchValues(
      searchParameter.Url.ToString(),
      searchParameter.Type,
      searchParameter.TargetResourceTypes,
      resource,
      searchParameter.Expression,
      context))
  {
     try {
      yield return new SearchIndexEntry(searchParameterInfo, searchValue);
     } catch(ArgumentException) { throw new BadRequestException(...) }
  }

[apurvabhaleMS]

Added the null check in TypedElementSearchIndexer.ProcessNonCompositeSearchParameter. I noticed there is another function ProcessCompositeSearchParameter which calls the SearchIndexEntry too. Wondering if the null check would also apply to the composite Search parameter. Any thoughts?

[brendankowitz]

Possibly related to AB#86850