-
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
Search Parameter validation failure issues logged #3341
Conversation
src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs
Show resolved
Hide resolved
src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs
Outdated
Show resolved
Hide resolved
{ | ||
issueDetails.Append(issue.Diagnostics); | ||
issueDetails.AppendLine(); | ||
} | ||
} | ||
catch (InvalidDefinitionException ex) |
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.
Maybe we can simplify this part of the code with:
catch (FhirException ex)
{
StringBuilder issueDetails = new StringBuilder();
foreach (OperationOutcomeIssue issue in ex.Issues)
{
issueDetails.Append(issue.Diagnostics).Append("; ");
}
_logger.LogWarning(ex, "Error loading search parameter {Url} from data store. Issues: {Issues}", searchParam.GetStringScalar("url"), issueDetails.ToString());
}
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.
As InvalidDefinitionException and SearchParameterNotSupportedException inherit from FhirException, a single catch block can simplify the code.
Description
This PR is about logging the actual reason that caused Search Parameter validation failure and logs InvalidDefinitionException in Exception Telemetry for different accounts in search parameter definition builder.
Current code captures details about what is causing validation failure but not logging it. I modified code to log the cause that fails validation.
Related issues
Addresses [issue # 88668].
Testing
Do not have invalid search parameters in my local or Test.
So tested it by breaking the code intentionally to confirm detailed logs are showing up.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)