Add unit tests for SearchParameter duplicate URL check - #5718
Merged
Conversation
… SQL error 2627 ICM-833659983 / AB#198804 Root cause: TryGetValue + Remove on the shared HTTP request-context Properties dictionary were not atomic. Under parallel bundle processing, two concurrent threads could both read the same PendingSearchParameterStatus before either removed it, causing pendingStatuses = [URI, URI] in MergeAsync. This produced a duplicate row in the @searchParams TVP, violating the UNIQUE (Uri) constraint on dbo.SearchParamList and returning SQL error 2627 / HTTP 500. Fix: wrap TryGetValue + Remove in lock(properties) so only one thread can claim the pending status; subsequent threads see the key already removed. Tests added: - Unit: SetAndClearPendingSearchParameterStatus_WhenCalledConcurrently_Only OneResourceReceivesStatus (SqlServerFhirDataStoreUnitTests) — deterministic, proves race with Parallel.For, fails before fix / passes after. - E2E regression guard: GivenParallelTransactionBundleWithSearchParamAndPatients _WhenPosted_ShouldNotThrowUniqueKeyConstraint (ReindexTests) — probabilistic, runs in CI/EUAP.
- catch (Exception) instead of bare catch (CodeQL SA1025) - Assert bundle response NotNull + correct entry count before checking failures - Fail fast on reflection field-not-found instead of silent ?.SetValue - Remove DistinctBy from E2E XML doc comment (lock-only fix was applied) - Soften 'DETERMINISTIC' claim in unit test summary comment
…rallel coverage Finding 1 (medium): add code comment to SetAndClearPendingSearchParameterStatus documenting the pre-existing single-slot limitation — bundles with multiple SearchParameter entries can lose earlier statuses due to key overwrite before consume. Pre-existing behavior, tracked separately. Finding 2 (medium): update E2E test XML doc to explain why Transaction was chosen over Batch and confirm that both parallel paths are protected by the same lock fix via the shared SetAndClearPendingSearchParameterStatus code path. Finding 3 (low): no code change needed — the lock prevents duplicates regardless of URL length; the 128-char boundary is a TVP schema constraint unrelated to the concurrency fix.
…different resource A PUT with a new unique resource ID but an existing URL violates the 1-URL-per-resource invariant. Two resources sharing a URL causes SQL 2627 errors in bundle operations: DELETEs operate by resource ID and derive SearchParameter URLs at runtime — if two IDs share a URL, MergeAsync receives @searchParams TVP rows [URL, URL] → UNIQUE constraint violation. Fix: in CreateOrUpdateSearchParameterBehavior, when prevSearchParamResource is null (brand-new resource being PUT), reject if the URL is already registered in the SearchParameter definition manager. Note: this fix is complementary to the ICM-833659983 lock fix (concurrent SetAndClearPendingSearchParameterStatus race) committed earlier in this PR. With Bug 187119 fixed, duplicate URLs cannot enter the system via PUT, eliminating the DELETE-derived duplicate scenario. The race-condition lock remains as defence-in-depth for the parallel-bundle scenario.
…dingHardDelete) TryGetSearchParameter without excludePendingDelete returns ALL states including PendingHardDelete. The previous check would block legitimate recreation of a SearchParameter after hard-delete (the URL is still in the definition manager as PendingHardDelete until a reindex cleans it up). Only reject when the existing SP is in an active state (Supported, Disabled). Allow the PUT when the existing holder is PendingDelete or PendingHardDelete. This preserves the GivenBulkDeleteRequest_WhenSearchParametersDeleted test flow: HardDelete -> URL enters PendingHardDelete state -> PUT recreates the same SP.
PUT was already guarded. POST (CreateAsync) had no URL-uniqueness check, so posting a SearchParameter with a URL already owned by an active resource silently created a second resource, accumulating duplicate-URL pairs over repeated EUAP test runs. Root cause of the aaaa URL errors on EUAP: GivenAnExistingSearchParameter_WhenUpdatingWithUrlLongerThan128 PUTs a SearchParameter with Url = prefix + 88 * 'a' (128 chars, always the same) on every run but does not clean it up. After N runs, N resources share URL=aaaa. Any parallel delete bundle (e.g. ReindexTests.InitializeAsync DeleteResourcesAsync) derives URL=aaaa for each, hitting the TVP UNIQUE constraint (SQL 2627). Same guard as PUT: reject only when existing SP is in an active state (Supported / Disabled), allow when PendingDelete / PendingHardDelete.
…ated unit test The lock was added to prevent SQL 2627 caused by duplicate URIs in the @searchParams TVP. The actual root cause is Bug 187119: PUT/POST allowed creating a new SearchParameter resource with a URL already owned by a different active resource. With that fix in place, duplicate-URL resources can no longer accumulate, so the parallel-delete scenario that triggered the TVP violation cannot arise. Revert the lock and remove its unit test. The E2E regression guard in ReindexTests remains.
…rlLongerThan128 test Without cleanup, each test run left an active SearchParameter with URL=aaaa (128 chars, always the same) in the database. Accumulation over repeated EUAP runs meant N resources shared the same URL — the parallel delete bundles in ReindexTests.InitializeAsync would then fail with SQL 2627. Add a finally block to hard-delete the created SP so each run is self-contained.
…actor test helpers
…l form with char 'c'
… cleanup in URL-too-long test
apurvabhaleMS
added
Enhancement-Test
apurvabhaleMS
changed the title
apurvabhaleMS
marked this pull request as ready for review
August 11, 2026 17:32
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5718 +/- ##
=======================================
Coverage ? 77.16%
=======================================
Files ? 1007
Lines ? 37180
Branches ? 5662
=======================================
Hits ? 28691
Misses ? 7120
Partials ? 1369 🚀 New features to boost your workflow:
|
Jared Erwin (feordin)
approved these changes
Aug 11, 2026
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
3 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.
Description
Adds two unit tests to
SearchParameterBehaviorTeststo cover the duplicate URL guard added in the companion fix:GivenAnUpsertResourceRequest_WhenUrlAlreadyOwnedBySameResource_ThenNoExceptionThrown— verifies that updating a SearchParameter does not throw when the URL found in the store belongs to the same resource being updatedGivenAnUpsertResourceRequest_WhenUrlAlreadyOwnedByDifferentResource_ThenBadRequestThrown— verifies that aBadRequestExceptionis thrown when a different resource already owns the target URLAlso adds a default mock for
GetSearchParametersByUrlsAsyncin the test constructor so existing tests continue to pass without changes.Related issues
Addresses AB198804, AB187119
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)