Hotfix: Skip Multiary Chained Expression For Rewriter by mikaelweave · Pull Request #5621 · microsoft/fhir-server

mikaelweave · 2026-06-17T17:46:57Z

Description

Adds a skip for the DOB rewriter for more chained / reverse chained queries.
Adds more tests for this rewriter.

Related issues

AB#195112

Testing

Locally, unit, e2e

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.
When changing or adding behavior, if your code modifies the system design or changes design assumptions, please create and include an ADR.
CI is green before merge
Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

* Add SMART search integration timing diagnostics Log SMART integration fixture setup, resource upsert, and search-service timings so the PR pipeline can identify whether the slow SmartSearch shard is dominated by setup or specific query patterns. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Reuse SMART search integration fixture Seed SMART integration data once per version and data store so SmartOnFhir tests avoid rebuilding the expensive storage fixture for every test method. Use unique IDs for mutation tests while preserving update semantics, and dispose cached fixtures when the test process exits. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Simplify SMART search shared fixture Remove process-lifetime fixture caching and diagnostic timing wrappers while keeping shared SMART test data setup under xUnit fixture lifetime. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address SMART fixture review comments Move test setup out of the no-op async lifecycle and inline the one-line upsert wrapper. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding 'CodeQL / Missed 'readonly' opportunity' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * Move SmartSearchSharedFixture into its own file Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Potential fix for pull request finding 'CodeQL / Missed opportunity to use Where' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

* Downgrade unknown ValueSet expand failures to LogWarning (#192757) * Potential fix for pull request finding 'CodeQL / Dereferenced variable may be null' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> * Add test * Return HTTP 404 for unknown ValueSet during $expand Throw ResourceNotFoundException instead of returning OperationOutcome when Firely surfaces an unknown ValueSet (HttpStatusCode.NotFound). The existing OperationOutcomeExceptionFilter maps it to 404. Added catch (ResourceNotFoundException) { throw; } in the handler to prevent double Error-level logging. Only Warning is now logged. FHIR spec only requires error OperationOutcome with non-2xx status; 404 is consistent with Firely NotFound semantics. * Add E2E test verifying 404 for unknown ValueSet expand * Add E2E test for expand by ID returning 404 --------- Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

* Optimize slow query logger: hash-based Query Store lookup, circuit breaker, timeout cap * Remove circuit breaker from slow query logger (moved to separate PR); clean up tests and harden ExtractParameterHash * Use consistent 'StoredProc' lookup tag for stored procedure Query Store path * Restore fire-and-forget query logging behavior tests * Narrow fire-and-forget Query Store catch to specific exception types (CodeQL) * Catch all exceptions in fire-and-forget Query Store guard * Consolidate ExtractParameterHash tests into a single theory * Address PR review: enrich fire-and-forget warning, Ordinal hash match, sync-comment SQL/test rationale - #1 Include exception type/message in the Warning log so operators can diagnose Query Store lookup failures without Debug logging. - #2 Add comment cross-referencing SqlQueryGenerator.ParametersHashStart for the hardcoded '/* HASH ' SQL literal. - #3 Use StringComparison.Ordinal for the hash markers (always emitted uppercase). - #4 Expand QueryStoreLookupTimeoutSeconds test rationale for the upper bound. - #6 Note the three lookup SQL consts share structure and must be kept in sync. * Address review: use class logger, drop redundant exception text, inline effectiveQuery, XML-doc timeout const - Revert AI suggestion: exception is passed to LogWarning (queryable via env_ex_*), so its type/message is no longer duplicated in the message string. - Remove the ILogger parameter from FireAndForgetQueryStoreLookup and LogQueryStoreByTextAsync; use the class _logger field. - Inline the redundant effectiveQuery alias. - Convert the QueryStoreLookupTimeoutSeconds comment to an XML doc comment.

* added max last updated to behavior * Removed incorrect "cycle conclusive" logic. Exclude deleted and pending delete from in cache logic * Added mx last updated logic. * log only for background * Added logic to invoke cache refresh * Fixed delete test * Removed in cache checks * Changed pipreline to process all via mediator * Delete search params on-by-one * Added last updated to status update * Removed concurrency manager * last * deleted * Fixing search param tests * fix context * Fixed reindex job test * Fixed test * cmd using * context in smart search tests * Restored filtering on urls * 500 and comment out theories that show bugs * skip cache refresh * split urls * Revert "Restored filtering on urls" This reverts commit 385a0be. * comments * obsolete * Bad request * comment * common ex logic in single place * Corrected exception check in the test * better error message * Partial implementation of max before validate * Skip 500 for Cosmos * skip cosmos * Test fixes * Second stage * fix unit test * customer message * Remove ==true * min 112 * make date not null * Fix name * Fixing pre-existing exception bug * Use set extension * Comments and removed foreach for included search params * serializing search param deletes * shorter name * removed to list * search result entry * Fix * Retries * Fix to retries * rename * Header * more renames * Add concurrent bundles test * Latest AI suggestion * Simple retries on controller level * Removed obsolete test * fixed deletion serrvice tests * using * using * extra * s * Added conditional * added bug link * added fail * Potential fix for pull request finding 'CodeQL / Useless assignment to local variable' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> --------- Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

* correct pending statuses * corrections * moved imside if * single method

Prevent ScalarTemporalEqualityRewriter from producing day-split UNION expressions while visiting chained search predicates, while preserving the direct birthdate rewrite path. Add SQL Server chained birthdate E2E coverage and a unit guard for chained visitor context. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

* Handling invalid bundle types * Refactoring code to make it simpler. * Upgrating health shared components. * Typo. * Remove unused using statements. * Undo version updated.

## What\n- Skip ScalarTemporalEqualityRewriter day-split union rewrite when the root expression contains any chain expression (forward or reverse).\n- Add regression unit tests covering both reverse-chain and forward-chain root query shapes to ensure birthdate equality stays pass-through.\n- Add an E2E forward-chain ImagingStudy started + patient birthdate + tag POST search test that mirrors the reverse-chain scenario.\n\n## Why\nUnion-expanded CTEs can shift predecessor selection in SqlQueryGenerator for complex chain queries. Skipping the scalar union rewrite for chain-containing roots preserves expected CTE ordering and prevents joins from binding to the wrong CTE branch.\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

- Improved comments in SqlQueryGenerator and ScalarTemporalEqualityRewriter to clarify UNION ALL expansion, SMART scope unions, and DOB day-split handling, documenting current limitations and guardrails. - Refactored chaining search E2E test to use unique tenant tags and dynamic patient creation; removed redundant forward chain test. - Cleaned up Fixture class by removing static tenant tag and patient fields now handled within tests.

Add SQL-only E2E tests for partial birthdate OR/ne/total/sort and leap-day cases in DateSearchTests, plus chained birthdate month/day scenarios in ChainingSearchTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…o local variable' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

…o use Where' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

This reverts commit de59713, reversing changes made to 26e8ea4.

LTA-Thinking · 2026-06-17T18:14:07Z

        }

+        [Fact]
+        public void GivenRootExpressionContainsReverseChainAndAllowListedBirthdateExactDay_WhenRewritten_ThenPassThroughBirthdate()


Nit: should these be a theory?

LTA-Thinking · 2026-06-17T18:16:23Z

+        [Fact]
+        public async Task GivenAChainedSearchExpressionOverBirthdateDayWithAdditionalCriteriaOnTheSameTarget_WhenSearched_ThenCorrectBundleShouldBeReturned()
+        {
+            string query = $"_tag={Fixture.Tag}&subject:Patient.birthdate={Fixture.SmithPatientBirthDate}&subject:Patient.family=Smith";


I don't think this affects your test, but multiple chain searches are applied as OR, not AND like every other FHIR search parameter.

mikaelweave and others added 9 commits June 4, 2026 16:30

Correct search param pending status model (#5605)

777005d

* correct pending statuses * corrections * moved imside if * single method

Fix Bulk Job Creation (#5609)

7769ecc

[Bundles] Handling invalid bundle types (#5610)

5f350b6

* Handling invalid bundle types * Refactoring code to make it simpler. * Upgrating health shared components. * Typo. * Remove unused using statements. * Undo version updated.

Fix copy (#5612)

abca618

mikaelweave requested a review from a team as a code owner June 17, 2026 17:46

mikaelweave and others added 4 commits June 17, 2026 10:48

cleanup for PR

c00c7a5

Add SQL-only DOB E2E coverage

6d9df0e

Add SQL-only E2E tests for partial birthdate OR/ne/total/sort and leap-day cases in DateSearchTests, plus chained birthdate month/day scenarios in ChainingSearchTests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

mikaelweave force-pushed the personal/mikaelw/fix-dob-rewriter-has branch from 41d5968 to 6d9df0e Compare June 17, 2026 17:49

mikaelweave and others added 2 commits June 17, 2026 10:58

Potential fix for pull request finding 'CodeQL / Useless assignment t…

7fe1340

…o local variable' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

Potential fix for pull request finding 'CodeQL / Missed opportunity t…

26e8ea4

…o use Where' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>

mikaelweave added this to the FY26\Q4\2Wk\2Wk26 milestone Jun 17, 2026

mikaelweave added Bug Bug bug bug. Azure Healthcare APIs Label denotes that the issue or PR is relevant to the FHIR service in the Azure Healthcare APIs No-ADR ADR not needed No-PaaS-breaking-change labels Jun 17, 2026

Copilot AI and others added 2 commits June 17, 2026 18:04

Merge origin/main and resolve conflicts

de59713

Revert "Merge origin/main and resolve conflicts"

9484916

This reverts commit de59713, reversing changes made to 26e8ea4.

LTA-Thinking reviewed Jun 17, 2026

View reviewed changes

LTA-Thinking approved these changes Jun 17, 2026

View reviewed changes

fix whitespace

7ac0d3e

LTA-Thinking approved these changes Jun 17, 2026

View reviewed changes

mikaelweave merged commit 5c54e58 into hotfix/skip-chain-birthdate-optimization Jun 17, 2026
15 of 22 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hotfix: Skip Multiary Chained Expression For Rewriter#5621

Hotfix: Skip Multiary Chained Expression For Rewriter#5621
mikaelweave merged 18 commits into
hotfix/skip-chain-birthdate-optimizationfrom
personal/mikaelw/fix-dob-rewriter-has

mikaelweave commented Jun 17, 2026 •

edited by azure-boards Bot

Loading

Uh oh!

LTA-Thinking Jun 17, 2026

Uh oh!

LTA-Thinking Jun 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

mikaelweave commented Jun 17, 2026 • edited by azure-boards Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Testing

FHIR Team Checklist

Semver Change (docs)

Uh oh!

LTA-Thinking Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

LTA-Thinking Jun 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mikaelweave commented Jun 17, 2026 •

edited by azure-boards Bot

Loading