Resolve multiple profiling and slicing bugs in resource factory #337
Merged
Resolve multiple profiling and slicing bugs in resource factory #337
Conversation
Polymorphic elements such as Observation.value[x] carry more than one type code (e.g. Quantity, CodeableConcept, string, ...). The previous implementation rejected any call with len(datatypes) != 1, which caused DefinitionResolutionError whenever a differential profile constrained a child of such an element. Changes: - _build_type_node now iterates over all candidate types instead of requiring exactly one. FHIRPath system types and non-complex types are silently skipped; the first complex type whose snapshot contains a sub-element matching the requested local_id is used. - An empty datatypes list still raises immediately. - The final raise includes all attempted types to aid debugging. - Updated the docstring accordingly. Tests added in test_fhir_resources_factory_resolver.py: - raises on empty datatypes list - returns correct node when multiple types given (first match wins) - skips a non-matching first type in favour of a matching second type - raises when all types are FHIRPath system types - raises when all types are FHIR primitives (no complex sub-elements) - skips FHIRPath entries in a mixed FHIRPath + complex list - raises when no type in the list has the requested sub-element - _build_intermediate_node succeeds for a polymorphic value[x] parent with [Quantity, CodeableConcept] type codes
…-choice slices and improve slice filtering
#331 Fixes a bug where FHIRModelFactory.build() would silently change the Python field type (List[T] vs T) when a differential StructureDefinition altered the cardinality of an element relative to its base resource. This broke type inheritance, allowing profiled models to accept payloads that the base class correctly rejects (and vice versa). Root cause ---------- build_field_information() derived the List[T]-vs-T annotation purely from the merged node's `max` value, which was overwritten by the differential. The base element's cardinality was never consulted. Changes ------- fhircraft/fhir/resources/factory/element_node.py - Drop `from __future__ import annotations`; import `PydanticUndefined` as a sentinel (`_Unset`) for "cardinality not specified". - `max_cardinality` / `is_array` now return `_Unset` / `None` instead of raising ValueError when `max` is absent on a partial differential element, making them safe to call during merging. - New computed properties `base_min_cardinality`, `base_max_cardinality`, and `base_is_array` read the FHIR `ElementDefinition.base` sub-element (populated during differential merging) to expose the base resource's original cardinality intent. fhircraft/fhir/resources/factory/resolver.py - `_merge_node_with_base()` now populates `definition.base` on the merged element (using the differential's existing `.base` if present, otherwise synthesising it from `base_node.min` / `base_node.max`), so that `base_is_array` is available on every merged node. - After merging, raises `DefinitionResolutionError` when a differential attempts to widen cardinality from scalar (max=1) to array (max>1), which is prohibited by FHIR profiling rules. fhircraft/fhir/resources/factory/builders/base.py - `build_field_information()` computes `effective_is_array` from `node.base_is_array` when available, falling back to `node.is_array` for snapshot / root nodes. All `List[T]` wrapping, default coercion, and min/max_length constraints are driven by `effective_is_array`, ensuring the Python type matches the base resource regardless of differential cardinality overrides. - Remove leftover `_type = type` alias. Tests ----- test/test_fhir_resources_factory_node.py - Unit tests for new `base_is_array`, `base_min_cardinality`, and `base_max_cardinality` properties on ElementNode. test/test_fhir_resources_factory_builders_base.py - Parametrised unit tests for `build_field_information()` covering narrowing (base array → scalar) and widening (base scalar → array) combinations of `base_is_array` / `is_array`. test/test_fhir_resources_factory_resolver.py - Unit tests for the updated `_merge_node_with_base()` logic: verifies that `definition.base` is correctly synthesised and that widening raises `DefinitionResolutionError`. test/test_fhir_resources_regression.py - `test_regression_narrowing_cardinality_preserves_list_type`: narrowing Observation.category 1..* → 1..1 keeps the field as List[T] and rejects a bare-dict payload the same as the base class. - `test_regression_widening_cardinality_preserves_scalar_type`: widening Observation.code 1..1 → 1..* (invalid per FHIR rules) now raises DefinitionResolutionError at build time. Additional test fixes test/test_fhir_resources_factory.py test/test_fhir_resources_factory_builders_backbone.py test/test_fhir_resources_factory_builders_simple.py test/test_fhir_resources_factory_builders_type_choice.py - Minor updates to account for `is_array` returning `None` (instead of raising) for nodes with no `max` specified.
…ixes #332 SlicedFieldBuilder unconditionally passed `alias=node.name` to `build_field_information()`, producing a redundant serialization alias even when the Python-safe name was identical to the FHIR element name (e.g. `extension` → `alias='extension'`). An alias is only meaningful when the name was modified to avoid a Python keyword conflict. Fix: only pass `alias` when `safe_name != node.name`. Also replace the test `test_build__field_alias_is_node_raw_name`, which was asserting the buggy behavior, with two focused tests: - `test_build__non_keyword_field_has_no_alias` — non-keyword fields must not receive an alias. - `test_build__python_keyword_field_gets_alias` — keyword-renamed fields must receive an alias equal to the original FHIR name.
…-alphanumeric content
…hot. Fixes #334 When a profile's `baseDefinition` points to a differential-only custom profile (one without a snapshot), the resolver was building its `base_index` solely from that profile's partial merged index. Any FHIR element untouched by the intermediate profile was therefore missing from the base, causing a `TypeResolutionError` / `AssemblerError` whenever the child profile tried to constrain it. Fix: add `SnapshotResolver._build_full_ancestor_index()` which walks the `baseDefinition` chain until it finds an ancestor with a real snapshot (guaranteed for built-in FHIR types), returns a `DefinitionIndex` from that snapshot, and lets the caller layer partial differential constraints on top via `update()`. A root-id guard preserves the pre-existing fall-back path for the non-standard case where intermediate profiles use a different root element name. Tests added: - `test_fhir_resources_regression.py`: two failing regression tests that reproduce the reported bug (chained differential-only profiles) and the BackboneElement type-resolution bug (separate, not yet fixed). - `test_fhir_resources_factory_resolver.py`: seven unit tests covering `_build_full_ancestor_index` (happy path, multi-level chain, exhausted chain) and the updated `resolve()` no-snapshot branch (ancestor snapshot used, type info preserved, mismatched-root fallback).
…children. Fixes #333 When a backbone element appears in the differential with no child elements (e.g. a cardinality-only constraint), BackboneFieldBuilder.build() previously called ModelAssembler to create an empty subclass (e.g. ExampleProfileReferenceRange with only `pass`), yielding a new type instead of the original specific type (e.g. ObservationReferenceRange). Fix: short-circuit build() when get_children() returns an empty list — use the resolved backbone_base directly as the field type and skip assembler invocation. The can_handle() extension from the prior commit ensures the builder is still selected for these childless backbone nodes. Also extend test_fhir_resources_factory_builders_backbone.py with 7 tests: - can_handle() returns True/False for the backbone-type-code + base-model branch - build() short-circuit: uses base type directly, skips assembler, correct field name Fixes #333
…and related tests. Fixes #335
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
1 participant
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.
This PR addresses a series of bugs in the FHIR resource factory pipeline (
SnapshotResolver,ElementNode,Builder, and related classes) that caused incorrect model generation when processing profiled StructureDefinitions.Added
SnapshotResolver._build_full_ancestor_index()which walks thebaseDefinitionchain until a real snapshot is found, enabling resolution of profiles that chain through differential-only intermediate profiles.ElementNodecomputed propertiesbase_min_cardinality,base_max_cardinality, andbase_is_arrayto expose the FHIRElementDefinition.basecardinality independently from the differential.ElementNodeandDefinitionIndexto handle type-choice slices (e.g.Observation.value[x]:valueStringindependently of conventional slices and improved slice filtering.Changed
build_field_information()to computeeffective_is_arrayfromnode.base_is_array(falling back tonode.is_array) so the Python field type always matches the base resource, regardless of differential overrides._merge_node_with_base()to populatedefinition.baseon the merged element and raiseDefinitionResolutionErrorwhen a differential attempts a prohibited cardinality widening (scalar → array).ElementNode.max_cardinality/is_arrayto return a sentinel (_Unset/None) instead of raising whenmaxis absent on a partial differential node.Fixed
FHIRModelFactorybreaks type inheritance when differential changes element cardinality #331SlicedFieldBuilderfrom emitting a redundant serialization alias when the field name was not renamed. FixesSlicedFieldBuildersets redundantaliason non-keyword field names #332build()and return the original specific backbone type directly when the differential has no children, instead of generating an empty subclass. Fixes Profile differential with backbone element overrides resolves field type toBackboneElementinstead of specific backbone type #333SnapshotResolverto correctly resolve all base elements when an intermediate profile in thebaseDefinitionchain has no snapshot. FixesAssemblerErrorwhen building a profile whose base is itself a differential-only profile #3340..0) to be set asNone-typed fields in the builder.ElementNode.documentationretrieval to skip non-alphanumeric-only content in theshort,definitionandcomment.max_cardinalitytype annotation to allowNonein generated source code