Phase 2: Typed variables, select_identifier filter, FhirR4 template packaging#618
Phase 2: Typed variables, select_identifier filter, FhirR4 template packaging#618Chethan77 wants to merge 1 commit intofeature/fhir-r4-variable-supportfrom
Conversation
…ackaging - Add VariableDefinition model (String, Numeric, Complex types) - Extend VariableValidator with typed validation, schema size limit, null guard - Add IFhirConverter + BaseProcessor + FhirR4Processor typed-variable overloads - Normalize typed values: long/double for Numeric, nested Hash for Complex - Implement SelectIdentifier filter with coding-scoped compound matching - Update DragonPatientMrn.liquid to use select_identifier with selectionCriteria - Add IdentifierSelectionCriteria JSON Schema - Package FhirR4 templates into default template collection - Add filter unit tests, processor tests, functional tests (2a-2g), template-management tests Breaking change: DragonPatientMrn.liquid now requires selectionCriteria complex variable instead of matchCode/dragonSystem string variables. This is intentional; dual-path support deferred to a future change. Tests: 505 unit + 2414 functional, all green.
|
@Chethan77 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
Pull request overview
Implements Phase 2 of variable support for the Liquid converter by introducing typed variables (with JSON Schema validation for complex types), adding a select_identifier Liquid filter for robust Identifier selection, and packaging FHIR R4 templates into the default template collection.
Changes:
- Add typed variable model + validation (String/Numeric/Complex with JSON Schema + size limits) and new converter overloads to accept typed variables.
- Introduce
select_identifierLiquid filter and update the Dragon MRN template to use criteria-based identifier selection. - Package FHIR R4 templates as an embedded default template bundle and update related unit/functional tests.
Reviewed changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Health.Fhir.TemplateManagement/Models/DefaultTemplateInfo.cs | Adds FHIR R4 default template resource and maps microsofthealth/fhirr4templates:default. |
| src/Microsoft.Health.Fhir.TemplateManagement/Microsoft.Health.Fhir.TemplateManagement.csproj | Embeds + builds FhirR4DefaultTemplates.tar.gz from data/Templates/FhirR4. |
| src/Microsoft.Health.Fhir.TemplateManagement/ArtifactProviders/DefaultTemplateCollectionProvider.cs | Extracts FHIR R4 default templates alongside other default bundles. |
| src/Microsoft.Health.Fhir.TemplateManagement.UnitTests/Providers/TemplateCollectionProviderTests.cs | Adds cache + expectations for the new FHIR R4 default template layer. |
| src/Microsoft.Health.Fhir.TemplateManagement.UnitTests/Providers/DefaultTemplateCollectionProviderTests.cs | Adds DefaultRootTemplateParentPath.FhirR4 mapping coverage. |
| src/Microsoft.Health.Fhir.TemplateManagement.UnitTests/Models/ImageInfoTest.cs | Adds microsofthealth/fhirr4templates:default to supported references (case-sensitive + insensitive). |
| src/Microsoft.Health.Fhir.TemplateManagement.UnitTests/Models/DefaultTemplateInfoTest.cs | Validates new DefaultTemplateInfo entry for FHIR R4 templates. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Utilities/VariableValidator.cs | Adds typed-variable validation, schema size limit, and JSON Schema validation for complex variables. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Resources.resx | Adds resource strings for typed-variable and identifier-selection errors. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Resources.Designer.cs | Auto-generated accessors for the newly added resource strings. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Processors/IFhirConverter.cs | Adds typed-variable Convert overloads. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Processors/FhirR4Processor.cs | Implements typed-variable conversion path and runtime normalization (numeric + complex). |
| src/Microsoft.Health.Fhir.Liquid.Converter/Processors/BaseProcessor.cs | Adds virtual typed-variable overload stubs for processors that don’t support typed variables. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Models/VariableDefinition.cs | Introduces VariableDefinition + VariableType (String/Numeric/Complex). |
| src/Microsoft.Health.Fhir.Liquid.Converter/Filters/IdentifierSelectionFilters.cs | Adds select_identifier filter with coding-scoped compound matching. |
| src/Microsoft.Health.Fhir.Liquid.Converter.UnitTests/Processors/FhirR4ProcessorTests.cs | Updates existing tests to typed variables and adds extensive Phase 2 validation/injection coverage. |
| src/Microsoft.Health.Fhir.Liquid.Converter.UnitTests/Filters/IdentifierSelectionFiltersTests.cs | Adds direct unit tests for select_identifier, including cross-coding false-positive prevention. |
| src/Microsoft.Health.Fhir.Liquid.Converter.FunctionalTests/FhirR4ConvertDataFunctionalTests.cs | Adds functional scenarios for typed criteria + filter behavior. |
| data/Templates/FhirR4/TypedVarTest.liquid | Adds template to verify numeric vs string rendering behavior. |
| data/Templates/FhirR4/Schemas/IdentifierSelectionCriteria.json | Adds JSON Schema for selectionCriteria variable validation. |
| data/Templates/FhirR4/DragonPatientMrn.liquid | Switches MRN selection from manual loops to select_identifier + selectionCriteria. |
Files not reviewed (1)
- src/Microsoft.Health.Fhir.Liquid.Converter/Resources.Designer.cs: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!long.TryParse(variable.Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out _) && | ||
| !double.TryParse(variable.Value, NumberStyles.Float | NumberStyles.AllowThousands, CultureInfo.InvariantCulture, out _)) |
There was a problem hiding this comment.
Numeric validation currently accepts values like NaN, Infinity, and -Infinity because double.TryParse returns true for them under invariant culture. Those tokens are not valid JSON numbers and will later fail JSON parsing in post-processing. Tighten validation to reject non-finite doubles (e.g., check double.IsNaN/double.IsInfinity after parsing).
| if (!long.TryParse(variable.Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out _) && | |
| !double.TryParse(variable.Value, NumberStyles.Float | NumberStyles.AllowThousands, CultureInfo.InvariantCulture, out _)) | |
| var isValidInteger = long.TryParse(variable.Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out _); | |
| var isValidFiniteDouble = double.TryParse(variable.Value, NumberStyles.Float | NumberStyles.AllowThousands, CultureInfo.InvariantCulture, out double parsedDouble) && | |
| !double.IsNaN(parsedDouble) && | |
| !double.IsInfinity(parsedDouble); | |
| if (!isValidInteger && !isValidFiniteDouble) |
| } | ||
|
|
||
| // Validate value against schema | ||
| var errors = schema.Validate(variable.Value); |
There was a problem hiding this comment.
valueToken is parsed from variable.Value but never used afterward, and schema.Validate(variable.Value) will parse the JSON again internally. Either remove the redundant JToken.Parse step, or use the parsed token for schema validation (to avoid double parsing) so this code stays warning-free and more efficient.
| var errors = schema.Validate(variable.Value); | |
| var errors = schema.Validate(valueToken); |
| var templates = new List<Dictionary<string, Template>>(); | ||
|
|
||
| // Extract default templates from embeeded resources | ||
| var hl7v2DefaultTemplatesTask = Task.Run(() => templates.Add(ExtractTemplatesFromResource(DefaultTemplateInfo.Hl7v2DefaultTemplatesResource, DefaultRootTemplateParentPath.Hl7v2.ToString())), cancellationToken); | ||
| var ccdaDefaultTemplatesTask = Task.Run(() => templates.Add(ExtractTemplatesFromResource(DefaultTemplateInfo.CcdaDefaultTemplatesResource, DefaultRootTemplateParentPath.Ccda.ToString())), cancellationToken); | ||
| var jsonDefaultTemplatesTask = Task.Run(() => templates.Add(ExtractTemplatesFromResource(DefaultTemplateInfo.JsonDefaultTemplatesResource, DefaultRootTemplateParentPath.Json.ToString())), cancellationToken); | ||
| var stu3ToR4DefaultTemplatesTask = Task.Run(() => templates.Add(ExtractTemplatesFromResource(DefaultTemplateInfo.Stu3ToR4DefaultTemplatesResource, DefaultRootTemplateParentPath.Fhir.ToString())), cancellationToken); | ||
| var fhirToHl7v2DefaultTemplatesTask = Task.Run(() => templates.Add(ExtractTemplatesFromResource(DefaultTemplateInfo.FhirToHl7v2DefaultTemplatesResource, DefaultRootTemplateParentPath.FhirToHl7v2.ToString())), cancellationToken); | ||
| var fhirR4DefaultTemplatesTask = Task.Run(() => templates.Add(ExtractTemplatesFromResource(DefaultTemplateInfo.FhirR4DefaultTemplatesResource, DefaultRootTemplateParentPath.FhirR4.ToString())), cancellationToken); | ||
|
|
||
| await Task.WhenAll(hl7v2DefaultTemplatesTask, ccdaDefaultTemplatesTask, jsonDefaultTemplatesTask, stu3ToR4DefaultTemplatesTask, fhirToHl7v2DefaultTemplatesTask); | ||
| await Task.WhenAll(hl7v2DefaultTemplatesTask, ccdaDefaultTemplatesTask, jsonDefaultTemplatesTask, stu3ToR4DefaultTemplatesTask, fhirToHl7v2DefaultTemplatesTask, fhirR4DefaultTemplatesTask); |
There was a problem hiding this comment.
templates is a plain List<Dictionary<string, Template>> that is being mutated (templates.Add(...)) concurrently from multiple Task.Run calls. List<T> is not thread-safe, so this can lead to race conditions (corruption, missed entries, or runtime exceptions). Consider having each task return its extracted dictionary (e.g., Task<Dictionary<string, Template>>) and combine results after WhenAll, or use a thread-safe collection/lock around Add.
Summary
Phase 2 of the Convert Engine Variable Support spec. Extends Phase 1's string-only variables with typed variables (String, Numeric, Complex + JSON Schema validation), adds a
select_identifiercustom Liquid filter with coding-scoped compound matching, updates the Dragon MRN template, and packages FhirR4 templates into the default template collection.Changes
Core Library
Models/VariableDefinition.cs(new) —VariableTypeenum (String,Numeric,Complex) andVariableDefinitionclassUtilities/VariableValidator.cs—ValidateTypedVariables()with per-type validation, schema size limit (MaxVariableSchemaLength), null entry guard. Preserves all Phase 1 validation rules.Processors/IFhirConverter.cs— 2 newIList<VariableDefinition>overloadsProcessors/BaseProcessor.cs— Virtual stubs for typed overloadsProcessors/FhirR4Processor.cs— Typed overloads with runtime normalization:long/doublefor Numeric (invariant culture),JTokenExtensions.ToObject()→Hash.FromDictionary()for ComplexFilters/IdentifierSelectionFilters.cs(new) —SelectIdentifierfilter with coding-scoped compound matchingResources.resx/Resources.Designer.cs— 13 new resource stringsTemplates & Schema
DragonPatientMrn.liquid— Replaced manual loop matching withselect_identifier: vars.selectionCriteriaSchemas/IdentifierSelectionCriteria.json(new) — JSON Schema for selection criteriaTypedVarTest.liquid(new) — Test template for typed variable injection verificationTemplate Packaging
DefaultTemplateInfo.cs—FhirR4DefaultTemplatesResource+microsofthealth/fhirr4templates:defaultmap entryDefaultTemplateCollectionProvider.cs— FhirR4 extraction taskMicrosoft.Health.Fhir.TemplateManagement.csproj— Embedded resource + prebuild tar commandTests
IdentifierSelectionFiltersTests.cs(new) — 14 direct filter tests including cross-coding false-positive preventionFhirR4ProcessorTests.cs— Typed-variable validation, injection, null entry, oversize schema testsFhirR4ConvertDataFunctionalTests.cs— Test cases 2a–2g + cross-coding verificationDefaultTemplateInfoTest.cs,ImageInfoTest.cs,DefaultTemplateCollectionProviderTests.cs,TemplateCollectionProviderTests.cs— FhirR4 template collection entriesKey Design Decisions
type.coding.systemmatches coding A andtype.coding.codematches coding B on the same identifier.MaxVariableSchemaLength(1 MB) rejects oversize schemas before NJsonSchema parsing to prevent CPU/memory DoS.CultureInfo.InvariantCultureto prevent locale drift.IDictionary<string, string>overloads are unchanged.Breaking Change
DragonPatientMrn.liquidnow requires aselectionCriteriacomplex variable instead ofmatchCode+dragonSystemstring variables. This is intentional for Phase 2; dual-path support deferred.Test Results