feat: Add FHIR R4-to-R4 conversion with template variable support (Phase 1)#614
feat: Add FHIR R4-to-R4 conversion with template variable support (Phase 1)#614
Conversation
|
@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
Adds a new FHIR R4 → FHIR R4 conversion path that supports passing named string variables into the Liquid render context (under vars), enabling templates like the provided Dragon MRN injection example and enforcing business rules via a new raise_error Liquid filter.
Changes:
- Introduces
FhirR4ProcessorandIFhirConverterWithVariablesto support variable-aware conversions. - Adds
raise_errorfilter and a sampleDragonPatientMrn.liquidtemplate demonstrating MRN selection + identifier injection. - Expands enums/factory routing and adds unit + functional coverage for the new pathway.
Reviewed changes
Copilot reviewed 24 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Health.Fhir.TemplateManagement.UnitTests/Providers/DefaultTemplateCollectionProviderTests.cs | Adjusts assertions to match currently packaged default template sets. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Resources.resx | Adds variable-validation resource strings. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Resources.Designer.cs | Adds strongly-typed accessors for new resource strings. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Processors/IFhirConverterWithVariables.cs | New interface adding variable-aware Convert overloads. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Processors/FhirR4Processor.cs | New R4→R4 processor with variable injection + validation. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Processors/ConvertProcessorFactory.cs | Routes DataType.FhirR4 + Fhir output to FhirR4Processor. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Models/FhirConverterErrorCode.cs | Adds InvalidVariableName error code. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Models/DefaultRootTemplateParentPath.cs | Adds FhirR4 root template parent path. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Models/DataType.cs | Adds FhirR4 data type. |
| src/Microsoft.Health.Fhir.Liquid.Converter/Filters/ErrorFilters.cs | Adds raise_error template filter. |
| src/Microsoft.Health.Fhir.Liquid.Converter.UnitTests/TestData/Expected/FhirR4/DragonPatientMrn.json | Adds expected output for the Dragon MRN template. |
| src/Microsoft.Health.Fhir.Liquid.Converter.UnitTests/TestConstants.cs | Adds test template directory constant for FhirR4. |
| src/Microsoft.Health.Fhir.Liquid.Converter.UnitTests/Processors/ProcessorTests.cs | Extends shared processor tests to include FhirR4Processor. |
| src/Microsoft.Health.Fhir.Liquid.Converter.UnitTests/Processors/FhirR4ProcessorTests.cs | Adds focused unit tests for variable validation + MRN template behavior. |
| src/Microsoft.Health.Fhir.Liquid.Converter.UnitTests/Processors/ConvertProcessorFactoryTests.cs | Adds factory coverage for FhirR4 and variable-interface assertion. |
| src/Microsoft.Health.Fhir.Liquid.Converter.UnitTests/Filters/ErrorFiltersTests.cs | Adds unit tests for raise_error. |
| src/Microsoft.Health.Fhir.Liquid.Converter.FunctionalTests/TestData/Expected/FhirR4/DragonPatientMrn.json | Adds functional expected output for R4 conversion. |
| src/Microsoft.Health.Fhir.Liquid.Converter.FunctionalTests/FhirR4ConvertDataFunctionalTests.cs | Adds end-to-end functional tests for R4 conversion with variables. |
| data/Templates/FhirR4/Passthrough.liquid | Adds minimal passthrough template for R4 smoke/shared tests. |
| data/Templates/FhirR4/metadata.json | Adds metadata for the new FhirR4 template set. |
| data/Templates/FhirR4/DragonPatientMrn.liquid | Adds sample Dragon MRN injection template using vars + raise_error. |
| data/SampleData/FhirR4/PatientWithIdentifiers.json | Adds sample R4 patient input with identifiers. |
| data/SampleData/FhirR4/PatientNoMR.json | Adds sample R4 patient input with no MR identifier. |
| data/SampleData/FhirR4/PatientMultiCodingMR.json | Adds sample R4 patient input for multi-coding edge case. |
| data/SampleData/FhirR4/PatientDuplicateMR.json | Adds sample R4 patient input with duplicate MR identifiers. |
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 (variables.Count > MaxVariableCount) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.InvalidVariableName, string.Format(Resources.TooManyVariables, MaxVariableCount, variables.Count)); | ||
| } | ||
|
|
||
| foreach (var kvp in variables) | ||
| { | ||
| ValidateVariableName(kvp.Key); | ||
|
|
||
| if (kvp.Value != null && kvp.Value.Length > MaxVariableValueLength) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.InvalidVariableName, string.Format(Resources.VariableValueTooLong, kvp.Key, MaxVariableValueLength)); | ||
| } |
There was a problem hiding this comment.
Variable validation failures for too many variables and oversized values are being reported with FhirConverterErrorCode.InvalidVariableName, which is misleading for callers that want to distinguish name vs value/count problems. Consider introducing dedicated error codes (e.g., TooManyVariables / InvalidVariableValue) or renaming/repurposing the code to cover all variable validation errors consistently.
There was a problem hiding this comment.
Agree with AI comment, more specific error codes are warrented.
There was a problem hiding this comment.
Another alternative is to make the error code more generic i.e. Invalid Variable and let the specific message determine the error .
There was a problem hiding this comment.
Addressed in a26187d. Went with your suggestion — renamed InvalidVariableName → InvalidVariable as a single generic error code. The specific error message (too many variables, value too long, invalid name format, reserved name, duplicate name) carries the detail. This avoids proliferating error codes for every validation scenario while keeping the messages actionable.
| private string InternalConvertWithVariables(string data, string rootTemplate, ITemplateProvider templateProvider, IDictionary<string, string> variables, TraceInfo traceInfo = null) | ||
| { | ||
| if (string.IsNullOrEmpty(rootTemplate)) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.NullOrEmptyRootTemplate, Resources.NullOrEmptyRootTemplate); | ||
| } | ||
|
|
||
| if (templateProvider == null) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.NullTemplateProvider, Resources.NullTemplateProvider); |
There was a problem hiding this comment.
InternalConvertWithVariables duplicates most of the standard conversion pipeline (template lookup, context creation, render, post-process) that already exists in BaseProcessor.InternalConvertFromObject / JsonProcessor.InternalConvert. This duplication increases the risk of behavior drift (e.g., future changes to error handling/timeouts/telemetry not being applied here). Consider refactoring to reuse the shared pipeline (e.g., by extending the base pipeline to accept additional context data like vars).
There was a problem hiding this comment.
If too complex can be skipped for this iteration but adding to the base converter would allow context to work across conversion types.
There was a problem hiding this comment.
Agreed — skipping for this iteration as suggested.
| public static void ValidateVariableName(string name) | ||
| { | ||
| if (string.IsNullOrEmpty(name)) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.InvalidVariableName, Resources.InvalidVariableName); | ||
| } | ||
|
|
||
| if (name.Length > MaxVariableNameLength) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.InvalidVariableName, string.Format(Resources.VariableNameTooLong, MaxVariableNameLength)); | ||
| } | ||
|
|
||
| if (!ValidVariableNameRegex.IsMatch(name)) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.InvalidVariableName, string.Format(Resources.InvalidVariableNameFormat, name)); | ||
| } | ||
|
|
||
| if (ReservedVariableNames.Contains(name)) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.InvalidVariableName, string.Format(Resources.ReservedVariableName, name)); | ||
| } | ||
| } |
There was a problem hiding this comment.
ValidateVariableName is public but appears to be an internal helper used only to support Convert validation (and unit tests). Exposing it publicly expands the library surface area and creates a compatibility contract. Consider making it private/internal (and validating only via Convert), or moving it to an internal utility with InternalsVisibleTo for tests.
There was a problem hiding this comment.
Addressed in a26187d. ValidateVariableName is now internal on a new VariableValidator utility class under Utilities/. Test access is via InternalsVisibleTo in the .csproj. The method is no longer on FhirR4Processor at all.
| { | ||
| public static string RaiseError(string message) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.TemplateRenderingError, message); |
There was a problem hiding this comment.
RaiseError can be invoked with a null/empty message, which results in a RenderException that may surface with the default .NET exception text (or an empty message), making template failures hard to diagnose. Consider normalizing null/empty to a non-empty default message (or throwing with a dedicated resource string) so consumers always get a useful error.
| throw new RenderException(FhirConverterErrorCode.TemplateRenderingError, message); | |
| string normalizedMessage = string.IsNullOrWhiteSpace(message) ? "Template raised an error." : message; | |
| throw new RenderException(FhirConverterErrorCode.TemplateRenderingError, normalizedMessage); |
There was a problem hiding this comment.
Addressed in a26187d. Null/empty/whitespace messages are now normalized to a resource string (Resources.DefaultTemplateError → "Template raised an error.") rather than a hardcoded literal, consistent with the project's localization pattern. Tests assert the exact normalized message.
| private string InternalConvertWithVariables(string data, string rootTemplate, ITemplateProvider templateProvider, IDictionary<string, string> variables, TraceInfo traceInfo = null) | ||
| { | ||
| if (string.IsNullOrEmpty(rootTemplate)) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.NullOrEmptyRootTemplate, Resources.NullOrEmptyRootTemplate); | ||
| } | ||
|
|
||
| if (templateProvider == null) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.NullTemplateProvider, Resources.NullTemplateProvider); |
There was a problem hiding this comment.
If too complex can be skipped for this iteration but adding to the base converter would allow context to work across conversion types.
| if (variables.Count > MaxVariableCount) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.InvalidVariableName, string.Format(Resources.TooManyVariables, MaxVariableCount, variables.Count)); | ||
| } | ||
|
|
||
| foreach (var kvp in variables) | ||
| { | ||
| ValidateVariableName(kvp.Key); | ||
|
|
||
| if (kvp.Value != null && kvp.Value.Length > MaxVariableValueLength) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.InvalidVariableName, string.Format(Resources.VariableValueTooLong, kvp.Key, MaxVariableValueLength)); | ||
| } |
There was a problem hiding this comment.
Agree with AI comment, more specific error codes are warrented.
| if (variables.Count > MaxVariableCount) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.InvalidVariableName, string.Format(Resources.TooManyVariables, MaxVariableCount, variables.Count)); | ||
| } | ||
|
|
||
| foreach (var kvp in variables) | ||
| { | ||
| ValidateVariableName(kvp.Key); | ||
|
|
||
| if (kvp.Value != null && kvp.Value.Length > MaxVariableValueLength) | ||
| { | ||
| throw new RenderException(FhirConverterErrorCode.InvalidVariableName, string.Format(Resources.VariableValueTooLong, kvp.Key, MaxVariableValueLength)); | ||
| } |
There was a problem hiding this comment.
Another alternative is to make the error code more generic i.e. Invalid Variable and let the specific message determine the error .
|
|
||
| namespace Microsoft.Health.Fhir.Liquid.Converter.Processors | ||
| { | ||
| public interface IFhirConverterWithVariables : IFhirConverter |
There was a problem hiding this comment.
Since we will eventually add this to all IFhirConverters I would suggest adding this to the base interface. For existing classes they can throw not implemented exceptions.
|
|
||
| // Verify expected number of templates per data type as per templates packaged from the data/Templates directory. | ||
| foreach (var defaultRootTemplateParentPath in Enum.GetValues<DefaultRootTemplateParentPath>()) | ||
| foreach (var defaultRootTemplateParentPath in _defaultTemplatesFolderInfo.Keys) |
There was a problem hiding this comment.
More context on the necessity of this change is needed. This change looks to be unrelated to the PR.
There was a problem hiding this comment.
Context: Adding DataType.FhirR4 to the enum caused the previous test to break because it iterated all DefaultRootTemplateParentPath enum values and expected each to have a packaged default template collection. FhirR4 is intentionally not packaged as a default template set in Phase 1 — its templates require runtime variables, and the default/embedded template flow has no variable injection mechanism yet.
The fix switches from enum iteration to an explicit dictionary of the 5 currently-packaged template families. This is more correct — it tests what is actually shipped rather than assuming every enum value maps to a packaged template. When FhirR4 templates are added to the default package in a later phase, that work would add the entry here alongside the packaging change.
| public class FhirR4Processor : JsonProcessor, IFhirConverterWithVariables | ||
| { | ||
| private static readonly Regex ValidVariableNameRegex = new Regex(@"^[a-zA-Z_][a-zA-Z0-9_]*$", RegexOptions.Compiled); | ||
| private static readonly HashSet<string> ReservedVariableNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase) { "msg", "vars" }; |
There was a problem hiding this comment.
Similar to the AI comment but it would be good to separate/decouple the vars parsing and validation logic so it isn't embedded only in this processor.
There was a problem hiding this comment.
Addressed in a26187d. Validation logic has been extracted to Utilities/VariableValidator.cs — a shared internal static class with:
ValidateVariables(IDictionary<string, string>) — count limit, per-key name validation, per-value size limit, case-insensitive duplicate detection
ValidateVariableName(string) — null/empty, length, regex format, reserved name check
FhirR4Processor now calls VariableValidator.ValidateVariables(variables) — one line. Any future processor that supports variables reuses the same validator.
| { | ||
| public string Convert(string data, string rootTemplate, ITemplateProvider templateProvider, IDictionary<string, string> variables, TraceInfo traceInfo = null); | ||
|
|
||
| public string Convert(string data, string rootTemplate, ITemplateProvider templateProvider, IDictionary<string, string> variables, CancellationToken cancellationToken, TraceInfo traceInfo = null); |
There was a problem hiding this comment.
This is fine for a first iteration but one downside of just taking the variables as a dictionary is you can't control if they are case sensitive or in sensitive. Ideally that isn't defined by the caller. Internally in the processor the reserved variables are case insensitive and you would want to be consistent with that decision.
| var dictionary = new Dictionary<string, object> { { DataKey, jsonData } }; | ||
| if (variables != null && variables.Count > 0) | ||
| { | ||
| dictionary["vars"] = Hash.FromDictionary(variables.ToDictionary(kv => kv.Key, kv => (object)kv.Value)); |
There was a problem hiding this comment.
Is there a reason for this to go from a dictionary of string/string to string/object and then be wrapped in a Hashset before being assigned to the context.
There was a problem hiding this comment.
Added an inline comment in a26187d explaining the chain. The three conversions are each forced by a different layer:
Dictionary<string, string>(public API) — Phase 1 spec restricts variables to string values.ToDictionary(... (object)kv.Value)—Hash.FromDictionaryrequiresIDictionary<string, object>Hash.FromDictionary()→Hash— DotLiquid's rendering context requiresHashfor nested property resolution ({{ vars.dragonSystem }})Keeping the public API as
string,stringis intentional to enforce the Phase 1 constraint. Phase 2 may widen tostring,objectfor typed variables.
| public class FhirR4Processor : JsonProcessor, IFhirConverterWithVariables | ||
| { | ||
| private static readonly Regex ValidVariableNameRegex = new Regex(@"^[a-zA-Z_][a-zA-Z0-9_]*$", RegexOptions.Compiled); | ||
| private static readonly HashSet<string> ReservedVariableNames = new HashSet<string>(StringComparer.OrdinalIgnoreCase) { "msg", "vars" }; |
There was a problem hiding this comment.
Naming is confusing. Originally thought you were using as reserved values in the provided vars and would error if they were used. That isn't the case, these are the names used for the context dictionary that is in scope for the template. Suggest adding TemplateContext like ReservedTemplateContextVariableNames to be distinguish.
There was a problem hiding this comment.
Addressed in a26187d. Renamed to ReservedTemplateContextVariableNames in the new VariableValidator class. The name now clearly communicates that these are the template rendering context's reserved keys ("msg" for input data, "vars" for the variable namespace), not restrictions on user-provided variable names in general.
…idator, consolidate interface - Rename InvalidVariableName -> InvalidVariable error code (generic for all variable errors) - Extract VariableValidator utility class to decouple validation from processor - Rename ReservedVariableNames -> ReservedTemplateContextVariableNames - Make validation methods internal with InternalsVisibleTo for test access - Move variable-aware Convert overloads to IFhirConverter base interface - Add virtual NotImplementedException stubs in BaseProcessor - Override in FhirR4Processor with actual implementation - Delete IFhirConverterWithVariables (consolidated into IFhirConverter) - Normalize RaiseError null/empty message to resource string - Add case-insensitive duplicate variable name rejection - Add inline comment explaining Hash type conversion chain - Update all unit and functional tests (469+2410+324 = 3203 passing)
Summary
Implements Phase 1 of the Convert Engine Variable Support specification by adding a new FHIR R4-to-R4 conversion pathway with named string variables available at template render time.
The primary use case for this phase is Dragon Copilot MRN identifier injection: selecting a Patient identifier by
type.coding.codeand writing a new Dragon Copilot identifier with a configurable system URI.Scope Delivered
FhirR4Processorusing the existing JSON parsing and post-processing pipelineIFhirConverterWithVariablesvarsDragonPatientMrn.liquidChanges Included
New files
FhirR4Processor.cs— R4-to-R4 processor with variable supportIFhirConverterWithVariables.cs— interface extendingIFhirConverterwith variable-aware overloadsErrorFilters.cs—raise_errorLiquid filter for template-level business rule enforcementDragonPatientMrn.liquid— sample Dragon MRN transformation templatePassthrough.liquid— minimal template used by shared processor smoke testsmetadata.json— metadata for theFhirR4template folderFhirR4ProcessorTests.cs— unit coverage for processor behavior and edge casesFhirR4ConvertDataFunctionalTests.cs— end-to-end functional testsErrorFiltersTests.cs— unit tests forraise_errorPatientWithIdentifiers.json,PatientNoMR.json,PatientDuplicateMR.json,PatientMultiCodingMR.jsonDragonPatientMrn.jsonModified files
DataType.cs— addedFhirR4DefaultRootTemplateParentPath.cs— addedFhirR4FhirConverterErrorCode.cs— addedInvalidVariableName = 1314ConvertProcessorFactory.cs— addedFhirR4processor routingResources.resx— added variable validation error stringsProcessorTests.cs— added FhirR4 to shared processor coverageConvertProcessorFactoryTests.cs— added FhirR4 factory and interface assertionsDefaultTemplateCollectionProviderTests.cs— scoped assertions to currently packaged template setsTestConstants.cs— added FhirR4 template directory constantValidation
Focused FhirR4 coverage
Added 35 unit tests for:
dragonSystemAdded 5 functional tests for:
dragonSystemfailureFull solution verification
Build verification:
dotnet build --warnaserror→ 0 warnings, 0 errorsExplicitly Out of Scope for Phase 1
CLI tool integration
CLI integration was not added in this PR.
Reason:
--varargument model.FhirR4through the CLI without a variable input contract would not provide a usable end-to-end experience.This should be handled as a separate follow-up if CLI support is required.
Default template packaging / embedded template flows
Default template packaging was not added in this PR.
Reason:
templateCollectionReference.This should be addressed alongside API/default-template flow support in a later phase.
Adding variable overloads to
IFhirConverterThe base
IFhirConverterinterface was intentionally not changed.Reason:
IFhirConverterWithVariables : IFhirConverterkeeps the new capability isolated to processors that support it.Notes
This PR is intended to complete the Phase 1 library slice:
It does not attempt to complete later-phase integration surfaces.