Test-only per-request config overrides: exercise date eq containment in E2E (stacked on #5646) - #5647
Draft
Mikael Weaver (mikaelweave) wants to merge 5 commits into
Draft
Conversation
Implement FHIR-spec-compliant date/time eq containment behind a new EnableFhirDateContainment flag and eliminate the temporal UNION ALL that ScalarTemporalEqualityRewriter previously emitted for birthdate. - Add EnableFhirDateContainment (FhirSqlServerConfiguration), default OFF so out-of-box behavior is identical to main; one-line flip to opt into spec-compliant containment. - Gate ScalarTemporalEqualityRewriter behind BOTH EnableScalarTemporalEqualityRewriter AND EnableFhirDateContainment; it now emits a single End-only DateTimeEnd predicate (index optimization) and never a UnionExpression. Removed BuildDaySplitUnion and the VisitChained chained-skip guard (band-aids that only existed because of the union). EnableScalarTemporalEqualityRewriter is also defaulted OFF (it shipped true on main): with the rewriter gated behind both flags its default is moot while containment is off, and defaulting it off makes the birthdate End-only optimization an explicit opt-in so enabling containment alone yields pure Core containment. - Skip Core's overlap-weakening DateTimeEqualityRewriter when containment is ON so Core's containment predicates (DateTimeStart >= lo AND DateTimeEnd <= hi) flow to SQL system-wide. Containment OFF keeps legacy overlap (true kill-switch). - Refactor ApplyDateEqualitySemantics into a single mutually-exclusive three-combo dispatch so the End-only equals optimization is never layered on top of the containment range; each flag combination invokes exactly one strategy. - Keep all SMART/compartment-shared union infrastructure untouched. - Cosmos stays on overlap (SQL-only scope); Core-level containment flag is a follow-up. - Unit tests: rewriter now asserts no union + End-only; add both-flags pipeline matrix and partial-precision (VP2) ON-vs-OFF coverage. - Add DateEqualityContainmentIntegrationTests (SQL-backed, real index seeding) covering the top-level eq matrices, the partial-precision birthdate VP2 keystone, reverse _has chain, forward chain, _sort, and the kept SMART V2 granular-scope union composed with birthdate=eq across all three flag combinations. - E2E: documented three-scenario run strategy (custom Startup re-registering the SQL config + isolated DB) in the ADR reference appendix; no new E2E servers shipped. - ADR-2606 documents the decision; ADR-2605 marked superseded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| IFhirRequestContext context = null; | ||
|
|
||
| Assert.False(context.TryGetRequestConfigurationOverride(Key, out _)); |
| EnsureArg.IsNotNull(logger, nameof(logger)); | ||
|
|
||
| _next = next; | ||
| _featureConfiguration = fhirServerConfiguration.Value.Features; |
| _logger.LogInformation( | ||
| "Applied {Count} per-request configuration override(s): {Keys}", | ||
| overrides.Count, | ||
| string.Join(", ", overrides.Keys)); |
Comment on lines
+130
to
+141
| foreach (KeyValuePair<string, Microsoft.Extensions.Primitives.StringValues> header in context.Request.Headers) | ||
| { | ||
| if (header.Key.StartsWith(HeaderPrefix, StringComparison.OrdinalIgnoreCase)) | ||
| { | ||
| string overrideKey = header.Key.Substring(HeaderPrefix.Length); | ||
| if (!string.IsNullOrEmpty(overrideKey)) | ||
| { | ||
| // Header wins over a query-string override of the same name. | ||
| overrides[overrideKey] = header.Value.ToString(); | ||
| } | ||
| } | ||
| } |
Comment on lines
+349
to
+352
| catch (Exception e) | ||
| { | ||
| Assert.Fail($"A non-expected '{e.GetType()}' was raised. Url: {Client.HttpClient.BaseAddress}. No Activity Id present. Error: {e.Message}"); | ||
| } |
Expression.SearchParameter already returns SearchParameterExpression, so the explicit casts in the test helpers are redundant. Addresses CodeQL 'Cast to same type' findings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mikael Weaver (mikaelweave)
marked this pull request as draft
June 27, 2026 00:06
Remove DateEqualityContainmentIntegrationTests.cs and its .projitems registration from this PR to keep #5646 minimal and fast to merge. The SQL-backed integration coverage moves to the stacked test-infrastructure PR #5647, which is where the per-request config-override harness lives. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… in E2E Adds a TEST-ONLY request-configuration-override mechanism so E2E tests can toggle FhirSqlServer flags per request (query-string `_config.<Flag>` or `X-FHIRServer-Config-<Flag>` header), gated by a new master feature flag SupportsRequestConfigurationOverrides that is enabled only on the in-proc test server. This lets DateSearchTests and ChainingSearchTests drive the spec-compliant date eq containment path (EnableFhirDateContainment / EnableScalarTemporalEqualityRewriter from PR #5646) without per-server redeploys, which is not possible on the deployed AppService E2E targets. - RequestConfigurationOverrideMiddleware: strips _config.* params before the FHIR parser and stores per-request flag overrides; no-op unless the master flag is on. - SqlServerSearchService reads the overrides at the date-eq dispatch site, falling back to the static FhirSqlServerConfiguration values. - DateSearchTests: containment SkippableTheory asserting only contained values match (container/partial-precision birthdates drop out); scalar ON/OFF assert identical rows. - ChainingSearchTests: containment-ON variants of the chained, reverse-_has, and _sort birthdate shapes, proving the containment SQL path is valid where the temporal UNION previously broke. - Unit tests for the middleware and request-context override helpers. Containment E2E tests Skip.If(!IsUsingInProcTestServer) so deployed servers ignore them. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Moves DateEqualityContainmentIntegrationTests.cs and its .projitems registration here from PR #5646 to keep that PR minimal and fast to merge. These SQL-backed integration tests exercise the date eq equality semantics across the three mutually-exclusive flag combinations and belong with the test-infrastructure changes in this PR. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mikael Weaver (mikaelweave)
force-pushed
the
mikaelweave/date-containment-e2e-overrides
branch
from
June 27, 2026 00:16
c36ded1 to
30d95a1
Compare
Mikael Weaver (mikaelweave)
added a commit
that referenced
this pull request
Jun 30, 2026
* Spec-compliant date eq containment, remove temporal UNION ALL Implement FHIR-spec-compliant date/time eq containment behind a new EnableFhirDateContainment flag and eliminate the temporal UNION ALL that ScalarTemporalEqualityRewriter previously emitted for birthdate. - Add EnableFhirDateContainment (FhirSqlServerConfiguration), default OFF so out-of-box behavior is identical to main; one-line flip to opt into spec-compliant containment. - Gate ScalarTemporalEqualityRewriter behind BOTH EnableScalarTemporalEqualityRewriter AND EnableFhirDateContainment; it now emits a single End-only DateTimeEnd predicate (index optimization) and never a UnionExpression. Removed BuildDaySplitUnion and the VisitChained chained-skip guard (band-aids that only existed because of the union). EnableScalarTemporalEqualityRewriter is also defaulted OFF (it shipped true on main): with the rewriter gated behind both flags its default is moot while containment is off, and defaulting it off makes the birthdate End-only optimization an explicit opt-in so enabling containment alone yields pure Core containment. - Skip Core's overlap-weakening DateTimeEqualityRewriter when containment is ON so Core's containment predicates (DateTimeStart >= lo AND DateTimeEnd <= hi) flow to SQL system-wide. Containment OFF keeps legacy overlap (true kill-switch). - Refactor ApplyDateEqualitySemantics into a single mutually-exclusive three-combo dispatch so the End-only equals optimization is never layered on top of the containment range; each flag combination invokes exactly one strategy. - Keep all SMART/compartment-shared union infrastructure untouched. - Cosmos stays on overlap (SQL-only scope); Core-level containment flag is a follow-up. - Unit tests: rewriter now asserts no union + End-only; add both-flags pipeline matrix and partial-precision (VP2) ON-vs-OFF coverage. - Add DateEqualityContainmentIntegrationTests (SQL-backed, real index seeding) covering the top-level eq matrices, the partial-precision birthdate VP2 keystone, reverse _has chain, forward chain, _sort, and the kept SMART V2 granular-scope union composed with birthdate=eq across all three flag combinations. - E2E: documented three-scenario run strategy (custom Startup re-registering the SQL config + isolated DB) in the ADR reference appendix; no new E2E servers shipped. - ADR-2606 documents the decision; ADR-2605 marked superseded. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Remove redundant SearchParameterExpression casts Expression.SearchParameter already returns SearchParameterExpression, so the explicit casts in the test helpers are redundant. Addresses CodeQL 'Cast to same type' findings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Move date-eq containment integration tests to PR #5647 Remove DateEqualityContainmentIntegrationTests.cs and its .projitems registration from this PR to keep #5646 minimal and fast to merge. The SQL-backed integration coverage moves to the stacked test-infrastructure PR #5647, which is where the per-request config-override harness lives. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Trim verbose date-containment code comments to match surrounding style Condense the XML docs and inline comments added for the date-eq containment work down to the terser level of the surrounding code in each file. The full rationale (flag matrix, verification, spec references) lives in ADR 2606, so the in-code comments only need to state the essential what/why. No production or test logic changes -- every modified line is a comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Condense ADR-2606 to brief language; align Verification with PR scope Halve the ADR (12.2KB -> 5.8KB) to match the terser style of sibling adr-2605: tighten Context/Options/Decision/Consequences prose while preserving the load-bearing gating matrix and the both-flags / defaults-false / SQL-only / supersedes-2605 facts. Reconcile the Verification section with this branch: cite only the tests present here (SqlServerSearchServiceTests.ApplyDateEqualitySemantics, ScalarTemporalEqualityRewriterTests, E2E DateSearchTests/ChainingSearchTests) and defer the real-SQL containment + SMART-union integration coverage to the stacked follow-up PR, where those tests now live. Drop the verbose operator appendix and known-harness-limitation prose. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fold date-eq containment decision into ADR-2605, drop ADR-2606 Update adr-2605-scalar-temporal-equality-rewriter.md in place to document the spec-compliant date `eq` containment / no-union decision (flags, gating matrix, consequences, verification) instead of carrying a separate adr-2606-date-eq-containment-no-union.md. ADR-2605 is the living decision for this rewriter, so its Status returns to Accepted and records that the union-free containment model replaces the original UnionExpression day-split. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Simplify date-equality unit tests in SqlServerSearchServiceTests The flag matrix Theory already proves the per-combination overlap/collapse/ no-union semantics, so the two standalone Facts only duplicated it. Keep the one assertion the matrix does not cover (containment-on/scalar-off returns the expression by reference) as a focused pass-through test and drop the rest. - Collapse the null-input test to a single case: the guard short-circuits before either flag is read, so the extra two flag combinations proved nothing. - Merge the two near-identical builders into BuildExactDayBirthdateEquality. No coverage loss: the only dropped assertion (NotSame on the overlap path) is implied by the retained "overlap predicate is present" matrix assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Trim ApplyDateEqualitySemantics XML doc to its essentials The summary's per-flag wiring prose ("when containment is off… when both on…") duplicated the method body's if-statements and inline comments. Keep the three strategy names, the cref links, and the never-layered / no-UNION-ALL contract; drop the redundant wiring narration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Enhance FHIR date containment: update configuration and expression handling for 'ap' and 'eq' semantics * Refactor test fixtures: enhance ModelInfoProviderFixture documentation and update ScalarTemporalEqualityRewriterTests to use shared fixture --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #5646 (spec-compliant date
eqcontainment + temporal UNION removal). This PR adds a TEST-ONLY per-request configuration-override mechanism so the existing E2E suites can exercise the containment behavior from #5646 without redeploying servers with different flags — which is not possible on our deployed AppService E2E targets.Base branch is
mikaelweave/date-containment-no-unionso the diff here is the Option B delta only.What it adds
RequestConfigurationOverrideMiddleware— reads_config.<Flag>query params (orX-FHIRServer-Config-<Flag>headers), applies them as per-request overrides, and strips the_config.*params before the FHIR search parser runs. No-op unless the master feature flag is on.FeatureConfiguration.SupportsRequestConfigurationOverrides(master flag, default off) — enabled only inInProcTestFhirServer.SqlServerSearchServicereads the per-request overrides at the date-eqdispatch site, falling back to the staticFhirSqlServerConfigurationvalues.E2E wiring
DateSearchTests— new containment[SkippableTheory]asserting only contained values match (container / partial-precision birthdates drop out under containment); scalar ON/OFF rows are identical (SQL-shape-only proof).ChainingSearchTests— containment-ON variants of the chained, reverse-_has, and_sortbirthdate shapes — the shapes that previously failed to generate SQL when the dateeqemitted a temporalUNION.All containment-asserting tests
Skip.If(!Fixture.IsUsingInProcTestServer, ...)so deployed servers (where the master flag is off) ignore them.Pipelines
Opening primarily to see how CI fares on the containment E2E path.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com
AB#195112