Search Param UrlLookup and TypeLookup mismatch fix#5386
Search Param UrlLookup and TypeLookup mismatch fix#5386jestradaMS wants to merge 19 commits intomainfrom
Conversation
… statuses across UrlLookup and TypeLookup
src/Microsoft.Health.Fhir.Core/Features/Definition/SearchParameterDefinitionManager.cs
Fixed
Show fixed
Hide fixed
…to streamline search parameter status updates
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -357,30 +353,45 @@ private static HashSet<SearchParameterInfo> BuildSearchParameterDefinition( | |||
| var searchParameterDictionary = new ConcurrentDictionary<string, ConcurrentQueue<SearchParameterInfo>>(); | |||
| foreach (SearchParameterInfo searchParam in results) | |||
There was a problem hiding this comment.
Consider changing searchParam name here instead of changing it in many places below
| // URL http://hl7.org/fhir/SearchParameter/Resource-type with type Special, | ||
| // while ResourceTypeSearchParameter uses the same URL with type Token. | ||
| // Choosing the wrong type causes parser failures for _type queries. | ||
| SearchParameterInfo canonicalParam = searchParam; |
There was a problem hiding this comment.
Consider using name that closer identifies what search param origin is. Like searchParamFromUriLookup
| // while ResourceTypeSearchParameter uses the same URL with type Token. | ||
| // Choosing the wrong type causes parser failures for _type queries. | ||
| SearchParameterInfo canonicalParam = searchParam; | ||
| if (searchParam.Url != null && |
There was a problem hiding this comment.
Is searchParam valid at this point? If so, why do we check that Uri is not null?
|
How about changing our lookup data structures such that it is impossible to get different instances of SearchParameterInfo (SPI) by design? |
|
This PR solves atomicity of cache writes for UriLookup. We still don't maintain other 2 data structures (TypeLookup and hash lookup) atomically with UriLookup. It would be interesting to know the reasons for cache components to be out of sync. Is it racing writes or incorrect write workflow?. Please keep in mind that we most likely should remove racing writes and will keep all updates single directional and single threaded (from the database to cache) to have identical logic across all VMs. |
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Reindex/ReindexTests.cs
Show resolved
Hide resolved
…roving canonical parameter lookup
…N files and update SearchParameterDefinitionBuilder to inject _type for missing definitions.
…improved readability
Adding comment to SearchParameterDefinitionDebuilder for R5 vs. R4 difference on Resource-Type search parameter
Description
This pull request addresses a concurrency bug in the FHIR search parameter definition logic and makes related improvements to ensure the consistency and correctness of search parameter handling. The primary change is to ensure that a single canonical instance of each
SearchParameterInfois used across all lookups, preventing issues where updates to search parameter status would not propagate correctly. Additionally, there are minor adjustments to test configurations and a correction to a search parameter type in R5.Concurrency and Consistency Improvements:
SearchParameterDefinitionBuilderto use aConcurrentDictionary<string, SearchParameterInfo>for the URI dictionary and ensure atomic creation and retrieval ofSearchParameterInfoinstances viaGetOrAdd. This guarantees that both UrlLookup and TypeLookup reference the same object, preventing inconsistencies due to race conditions. [1] [2] [3]BuildSearchParameterDefinitionmethod and its recursive calls to accept and use the shareduriDictionary, ensuring canonicalization throughout the search parameter inheritance chain. [1] [2] [3]FHIR Search Parameter Type Correction:
Resource-typesearch parameter from"special"to"token"in the R5 search parameters JSON, aligning with system support and maintaining compatibility with previous FHIR versions.Test Configuration Adjustments:
Related issues
Addresses AB#183574
Testing
Describe how this change was tested.
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)