[Do Not Review] Fixing custom search parameter bugs. #5024
Conversation
v-isyamauchi-gh
added
Bug
v-isyamauchi-gh
changed the title
@microsoft-github-policy-service agree company="Microsoft" |
5 similar comments
@microsoft-github-policy-service agree company="Microsoft" |
@microsoft-github-policy-service agree company="Microsoft" |
@microsoft-github-policy-service agree company="Microsoft" |
@microsoft-github-policy-service agree company="Microsoft" |
@microsoft-github-policy-service agree company="Microsoft" |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| catch (Exception ex) | ||
| { | ||
| logger.LogError(ex, "Unexpected exception during comparing search parameter expressions."); | ||
| return null; | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the issue, we should replace the generic catch (Exception ex) block with specific exception types that are expected during the execution of the searchParameterComparer.CompareExpression method. If there are multiple possible exceptions, each should be handled in its own catch block. Any unexpected exceptions should propagate to higher levels for proper handling.
Steps:
- Identify the specific exceptions that
searchParameterComparer.CompareExpressionmight throw. For example, if it involves parsing or evaluation, exceptions likeArgumentExceptionorInvalidOperationExceptionmight be relevant. - Replace the generic
catchblock with specificcatchblocks for these exceptions. - Optionally, add a fallback
catchblock to rethrow unexpected exceptions if necessary.
| @@ -408,5 +408,10 @@ | ||
| } | ||
| catch (Exception ex) | ||
| catch (ArgumentException ex) | ||
| { | ||
| logger.LogError(ex, "Unexpected exception during comparing search parameter expressions."); | ||
| logger.LogError(ex, "Argument exception during comparing search parameter expressions."); | ||
| return null; | ||
| } | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| logger.LogError(ex, "Invalid operation exception during comparing search parameter expressions."); | ||
| return null; |
| // Note: this test ensures that the compare can parse and compare all expressiosn in search-parameters.json. | ||
| // The comparison logic is tested by other tests in this class. | ||
| var fhirCoreAssembly = Assembly.Load("Microsoft.Health.Fhir.Core"); | ||
| var currentAssembly = Assembly.GetExecutingAssembly(); |
Check warning
Code scanning / CodeQL
Useless assignment to local variable Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the issue, the assignment to currentAssembly on line 267 should be removed entirely, as the variable is not used anywhere in the provided code. This will eliminate the unnecessary assignment and improve code clarity. No additional changes are required since the removal does not affect the program's logic or functionality.
| @@ -266,3 +266,3 @@ | ||
| var fhirCoreAssembly = Assembly.Load("Microsoft.Health.Fhir.Core"); | ||
| var currentAssembly = Assembly.GetExecutingAssembly(); | ||
| // Removed the unused assignment to currentAssembly. | ||
|
|
| var bundle = rawResource.ToITypedElement(ModelInfoProvider.Instance).ToPoco() as Bundle; | ||
| Assert.NotNull(bundle); | ||
|
|
||
| foreach (var entry in bundle.Entry) |
Check warning
Code scanning / CodeQL
Dereferenced variable may be null Warning
Copilot Autofix
AI 3 days ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| var searchParameter = entry.Resource as SearchParameter; | ||
| Assert.NotNull(searchParameter); | ||
|
|
||
| if (!string.IsNullOrEmpty(searchParameter.Expression)) |
Check warning
Code scanning / CodeQL
Dereferenced variable may be null Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the issue, we need to ensure that searchParameter is not null before it is dereferenced on line 297. This can be achieved by adding an explicit null check for searchParameter before calling string.IsNullOrEmpty(searchParameter.Expression). This approach ensures that the code is safe even if assertions are disabled or bypassed.
| @@ -296,3 +296,3 @@ | ||
|
|
||
| if (!string.IsNullOrEmpty(searchParameter.Expression)) | ||
| if (searchParameter != null && !string.IsNullOrEmpty(searchParameter.Expression)) | ||
| { |
| catch (Exception ex) | ||
| { | ||
| DebugOutput($"Failed to compare an expression '{searchParameter.Expression}':{Environment.NewLine}{ex}"); | ||
| } |
Check notice
Code scanning / CodeQL
Generic catch clause Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the issue, replace the generic catch clause with specific exception types that are relevant to the operation being performed. For example, if the _comparer.CompareExpression method can throw specific exceptions like InvalidOperationException or ArgumentException, those should be caught explicitly. Any other exceptions should be allowed to propagate. Additionally, logging can be retained for visibility into the error.
The changes will be made in the file src/Microsoft.Health.Fhir.Shared.Core.UnitTests/Features/Search/SearchParameters/SearchParameterComparerTests.cs. Specifically, the catch block starting at line 305 will be updated to catch specific exceptions.
| @@ -304,5 +304,9 @@ | ||
| } | ||
| catch (Exception ex) | ||
| catch (InvalidOperationException ex) | ||
| { | ||
| DebugOutput($"Failed to compare an expression '{searchParameter.Expression}':{Environment.NewLine}{ex}"); | ||
| DebugOutput($"Failed to compare an expression '{searchParameter.Expression}' due to an invalid operation:{Environment.NewLine}{ex}"); | ||
| } | ||
| catch (ArgumentException ex) | ||
| { | ||
| DebugOutput($"Failed to compare an expression '{searchParameter.Expression}' due to an argument error:{Environment.NewLine}{ex}"); | ||
| } |
| { | ||
| if (x == null || y == null) | ||
| { | ||
| return x == y; |
Check warning
Code scanning / CodeQL
Reference equality test on System.Object Warning
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the issue, replace the reference equality check x == y with a content-based equality check. The most appropriate method for comparing two IEnumerable<T> collections is Enumerable.SequenceEqual, which checks whether two sequences have the same elements in the same order. Since the order of elements may not matter in this context, we can use SetEquals from HashSet<T> to perform an unordered comparison. This ensures that the collections are considered equal if they contain the same elements, regardless of order.
Changes to be made:
- Replace
x == ywith a content-based comparison usingSetEquals. - Add a null check for both
xandyto handle cases where one or both are null.
| @@ -157,3 +157,10 @@ | ||
| { | ||
| return x == y; | ||
| return x == null && y == null; | ||
| } | ||
|
|
||
| var setX = new HashSet<(string definition, string expression)>(x); | ||
| var setY = new HashSet<(string definition, string expression)>(y); | ||
| if (!setX.SetEquals(setY)) | ||
| { | ||
| return false; | ||
| } |
| foreach (var cx in componetX) | ||
| { | ||
| if (!componetY.TryGetValue(cx.Key, out var cy) || CompareExpression(cx.Value, cy) != 0) | ||
| { | ||
| return false; | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Where Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the issue, we will replace the foreach loop on line 168 with a LINQ Where clause to filter the elements of componetX before iterating over them. This will make the filtering logic explicit and reduce the nesting depth of the code. Specifically:
- Use the
Wheremethod to filtercomponetXbased on the condition!componetY.TryGetValue(cx.Key, out var cy) || CompareExpression(cx.Value, cy) != 0. - If the condition is met, return
falseimmediately, as the loop's purpose is to check for mismatches.
No additional imports or dependencies are required, as LINQ is already being used in the file.
| @@ -167,8 +167,5 @@ | ||
|
|
||
| foreach (var cx in componetX) | ||
| if (componetX.Where(cx => !componetY.TryGetValue(cx.Key, out var cy) || CompareExpression(cx.Value, cy) != 0).Any()) | ||
| { | ||
| if (!componetY.TryGetValue(cx.Key, out var cy) || CompareExpression(cx.Value, cy) != 0) | ||
| { | ||
| return false; | ||
| } | ||
| return false; | ||
| } |
| foreach (var expY in expsY) | ||
| { | ||
| if (Equals(expX, expY)) | ||
| { | ||
| found = true; | ||
| break; | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Where Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the issue, we will replace the nested foreach loop that implicitly filters expsY with a LINQ Where method. This will make the filtering explicit and reduce the nesting depth of the code. Specifically:
- Replace the inner
foreachloop (foreach (var expY in expsY)) with a LINQ query that filtersexpsYusing the conditionEquals(expX, expY). - Check if the filtered sequence contains any elements using
.Any(). - Apply similar changes to the second loop (
foreach (var expX in expsX)) later in the method.
No new imports or definitions are required, as LINQ is already included via using System.Linq.
| @@ -217,11 +217,3 @@ | ||
| { | ||
| var found = false; | ||
| foreach (var expY in expsY) | ||
| { | ||
| if (Equals(expX, expY)) | ||
| { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| var found = expsY.Where(expY => Equals(expX, expY)).Any(); | ||
|
|
||
| @@ -239,11 +231,3 @@ | ||
| { | ||
| var found = false; | ||
| foreach (var expX in expsX) | ||
| { | ||
| if (Equals(expY, expX)) | ||
| { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| var found = expsX.Where(expX => Equals(expY, expX)).Any(); | ||
|
|
| foreach (var expX in expsX) | ||
| { | ||
| if (Equals(expY, expX)) | ||
| { | ||
| found = true; | ||
| break; | ||
| } | ||
| } |
Check notice
Code scanning / CodeQL
Missed opportunity to use Where Note
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the issue, replace the foreach loop on line 241 with a LINQ Where clause to filter expsX explicitly based on the condition Equals(expY, expX). This eliminates the need for the found flag and reduces nesting depth. The filtered sequence can then be checked directly to determine if any matching element exists.
Steps to implement the fix:
- Replace the
foreachloop on line 241 with a LINQWhereclause. - Use the
Anymethod to check if there is at least one matching element in the filtered sequence. - Remove the
foundflag and its associated logic.
| @@ -239,13 +239,3 @@ | ||
| { | ||
| var found = false; | ||
| foreach (var expX in expsX) | ||
| { | ||
| if (Equals(expY, expX)) | ||
| { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (!found) | ||
| if (!expsX.Any(expX => Equals(expY, expX))) | ||
| { |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
@microsoft-github-policy-service agree company="Microsoft" |
Description
The change addresses issues mentioned in the bug below that are related to customer search parameters.
Related issues
Addresses [issue #117004].
Bug 117004: Custom SearchParameter with same Code Affecting Built-In Search Functionality in FHIR Service
Testing
Tested manually with a local server and also by adding UTs and E2E tests.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)