Skip to content

Allow for duplicate search parameters #3790

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Allow for duplicate search parameters #3790

wants to merge 17 commits into from

Conversation

feordin
Copy link
Contributor

@feordin feordin commented Mar 29, 2024 •
edited
Loading

Description

There are published IG's with Search Parameters that have identical code values and expression as existing Search Parameters. We want to allow these parameters without causing any reindexing or ambiguous behaviors when searching using the duplicated code values.

This PR also blocks the addition of any Search Parameter resource with a conflicting code value that has a different expression. As this is a bug that currently puts the service into an unstable state.

Related issues

Addresses [issue AB#117004].

Testing

Unit tests are added.
Manual testing using UploadFIG with USCore is done, and all SearchParameters succeeded.
Still TBD: E2E test to check reindex impact.

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)

@feordin feordin changed the title [DRAFT] Allow for "duplicate" search parameters Allow for "duplicate" search parameters Apr 5, 2024
@feordin feordin marked this pull request as ready for review April 5, 2024 18:56
@feordin feordin requested a review from a team as a code owner April 5, 2024 18:56
@feordin feordin force-pushed the personal/jaerwin/search-params branch from 8d40931 to 5ed7eb5 Compare April 5, 2024 18:58
@feordin feordin added Bug Bug bug bug. Area-Search Area related to search. KI-Breaking This is a known issue that causes a breaking change labels Apr 5, 2024
@feordin feordin added this to the S138 milestone Apr 5, 2024
@feordin feordin removed the Bug Bug bug bug. label Apr 5, 2024
@feordin feordin changed the title Allow for "duplicate" search parameters Allow for duplicate search parameters Apr 5, 2024
@feordin feordin added Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Bug Bug bug bug. labels Apr 5, 2024
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 5, 2024
View reviewed changes
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs
Comment on lines +162 to +175
catch (Exception exp)
{
_output.WriteLine($"Attempt {retryCount} of {MaxRetryCount}: CustomSearchParameter test experienced issue attempted to clean up SearchParameter {searchParam.Url}. The exception is {exp}");
if (exp is FhirClientException fhirException && fhirException.OperationOutcome?.Issue != null)
{
foreach (OperationOutcome.IssueComponent issueComponent in fhirException.OperationOutcome.Issue)
{
_output.WriteLine("FhirException OperationOutome message from trying to delete SearchParameter is CustomSearchParam test: {0}", issueComponent.Diagnostics);
}
}

success = false;
await Task.Delay(TimeSpan.FromSeconds(10));
}

Check notice

Code scanning / CodeQL

Generic catch clause Note test

Generic catch clause.

Copilot Autofix

AI 3 months ago

To fix the problem, we should replace the generic catch (Exception exp) clause with more specific exception types that are expected to occur in this context. This will help ensure that only relevant exceptions are caught and handled appropriately, while other exceptions can propagate and be handled elsewhere or cause the program to fail, which can be useful for debugging.

  1. Identify the specific exceptions that are likely to occur in the try block.
  2. Replace the generic catch (Exception exp) clause with specific catch clauses for those exceptions.
  3. Ensure that any logging or retry logic is preserved in the new catch clauses.
Suggested changeset 1
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs
--- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs
+++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs
@@ -166,6 +166,6 @@
                     }
-                    catch (Exception exp)
+                    catch (FhirClientException fhirException)
                     {
-                        _output.WriteLine($"Attempt {retryCount} of {MaxRetryCount}: CustomSearchParameter test experienced issue attempted to clean up SearchParameter {searchParam.Url}.  The exception is {exp}");
-                        if (exp is FhirClientException fhirException && fhirException.OperationOutcome?.Issue != null)
+                        _output.WriteLine($"Attempt {retryCount} of {MaxRetryCount}: CustomSearchParameter test experienced issue attempted to clean up SearchParameter {searchParam.Url}.  The exception is {fhirException}");
+                        if (fhirException.OperationOutcome?.Issue != null)
                         {
@@ -178,2 +178,14 @@
                         success = false;
+                        await Task.Delay(TimeSpan.FromSeconds(10));
+                    }
+                    catch (HttpRequestException httpRequestException)
+                    {
+                        _output.WriteLine($"Attempt {retryCount} of {MaxRetryCount}: CustomSearchParameter test experienced issue attempted to clean up SearchParameter {searchParam.Url}.  The exception is {httpRequestException}");
+                        success = false;
+                        await Task.Delay(TimeSpan.FromSeconds(10));
+                    }
+                    catch (TaskCanceledException taskCanceledException)
+                    {
+                        _output.WriteLine($"Attempt {retryCount} of {MaxRetryCount}: CustomSearchParameter test experienced issue attempted to clean up SearchParameter {searchParam.Url}.  The exception is {taskCanceledException}");
+                        success = false;
                         await Task.Delay(TimeSpan.FromSeconds(10));
EOF
@@ -166,6 +166,6 @@
}
catch (Exception exp)
catch (FhirClientException fhirException)
{
_output.WriteLine($"Attempt {retryCount} of {MaxRetryCount}: CustomSearchParameter test experienced issue attempted to clean up SearchParameter {searchParam.Url}. The exception is {exp}");
if (exp is FhirClientException fhirException && fhirException.OperationOutcome?.Issue != null)
_output.WriteLine($"Attempt {retryCount} of {MaxRetryCount}: CustomSearchParameter test experienced issue attempted to clean up SearchParameter {searchParam.Url}. The exception is {fhirException}");
if (fhirException.OperationOutcome?.Issue != null)
{
@@ -178,2 +178,14 @@
success = false;
await Task.Delay(TimeSpan.FromSeconds(10));
}
catch (HttpRequestException httpRequestException)
{
_output.WriteLine($"Attempt {retryCount} of {MaxRetryCount}: CustomSearchParameter test experienced issue attempted to clean up SearchParameter {searchParam.Url}. The exception is {httpRequestException}");
success = false;
await Task.Delay(TimeSpan.FromSeconds(10));
}
catch (TaskCanceledException taskCanceledException)
{
_output.WriteLine($"Attempt {retryCount} of {MaxRetryCount}: CustomSearchParameter test experienced issue attempted to clean up SearchParameter {searchParam.Url}. The exception is {taskCanceledException}");
success = false;
await Task.Delay(TimeSpan.FromSeconds(10));
Copilot is powered by AI and may make mistakes. Always verify output.
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs
// for the search parameter/reindex updates to propagate to all instances. Hence we are
// adding some retries below to account for that delay.
int retryCount = 0;
bool success = true;

Check warning

Code scanning / CodeQL

Useless assignment to local variable Warning test

This assignment to
success
Loading
is useless, since its value is never read.

Copilot Autofix

AI 3 months ago

To fix the problem, we should remove the redundant assignment to the success variable on line 153. This will clean up the code and eliminate the unnecessary assignment without changing the existing functionality.

Suggested changeset 1
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs
--- a/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs
+++ b/test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Search/DuplicateSearchParamTests.cs
@@ -152,3 +152,3 @@
                 int retryCount = 0;
-                bool success = true;
+                bool success;
                 do
EOF
@@ -152,3 +152,3 @@
int retryCount = 0;
bool success = true;
bool success;
do
Copilot is powered by AI and may make mistakes. Always verify output.
@mikaelweave mikaelweave removed this from the S138 milestone Jun 18, 2024
rajithaalurims
rajithaalurims reviewed Jun 20, 2024
View reviewed changes
src/Microsoft.Health.Fhir.Core/Features/Search/Parameters/SearchParameterOperations.cs Outdated
}
if (!supportedResult.Supported)
{
throw new SearchParameterNotSupportedException(searchParameterInfo.Url);
Copy link
Contributor

Choose a reason for hiding this comment

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

Customer sees this message "The search parameter with definition URL '{0}' is not supported.". I know this change is not related to this PR but in this scenario, error is thrown if definition has any issues. Do you think we can use better error message to indicate customer that something is wrong with the definition they have sent?

rajithaalurims
rajithaalurims reviewed Jun 20, 2024
View reviewed changes
src/Microsoft.Health.Fhir.Core/Features/Search/Parameters/SearchParameterOperations.cs
await _searchParameterStatusManager.AddSearchParameterStatusAsync(
new List<string> { searchParameterWrapper.Url },
cancellationToken,
duplicateSearchParameter ? SearchParameterStatus.Duplicate : SearchParameterStatus.Supported);
Copy link
Contributor

Choose a reason for hiding this comment

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

We are introducing a new status Duplicate here. How is it going to impact reindex and $status operation?

@tarunmathew12 tarunmathew12 added this to the 2Wk13 milestone Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rajithaalurims rajithaalurims rajithaalurims left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Area-Search Area related to search. Azure API for FHIR Label denotes that the issue or PR is relevant to the Azure API for FHIR Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs Bug Bug bug bug. KI-Breaking This is a known issue that causes a breaking change
Projects
None yet
Milestone
2Wk13
Development

Successfully merging this pull request may close these issues.
4 participants
@feordin @rajithaalurims @mikaelweave @tarunmathew12