fix(validator): type Element.id-derived ids as string, not id (#424)

crates/fhir-validator/src/converter/mod.rs

@@ -107,9 +107,51 @@ pub(crate) struct Ed {
     pub rest: serde_json::Map<String, Value>,
 }
 
+impl Ed {
+    /// The value-domain type an element's own type resolution must yield when the
+    /// FHIR type model — not the (inconsistent) `structuredefinition-fhir-type`
+    /// extension — is authoritative. Returns `Some("string")` for any element
+    /// deriving from `Element.id`.
+    ///
+    /// # Why (issue #424)
+    ///
+    /// FHIR defines `Element.id` as type `string` ("any string value that does
+    /// not contain spaces"), whereas `Resource.id` is the constrained `id` token
+    /// (`[A-Za-z0-9\-\.]{1,64}`). The spec's own `type[].code` for both is the
+    /// FHIRPath `System.String`; the concrete FHIR type is carried only as a
+    /// `structuredefinition-fhir-type` extension hint — which HL7 populates
+    /// inconsistently across versions: R4B stamps `id` on *every* `.id`
+    /// (including `Element.id`), R5 stamps it on `ElementDefinition.id`, while
+    /// R4/R6 say `string`. Trusting that hint makes the `id` regex reject valid
+    /// element ids that contain `[x]` (choice elements), `:` (named slices), or
+    /// exceed 64 characters — i.e. it rejects StructureDefinitions the spec
+    /// itself publishes.
+    ///
+    /// HL7's own `fhir.schema.json` types every `Element.id` (and
+    /// `ElementDefinition.id`) as `string` in all four versions, confirming the
+    /// extension is the outlier. Keying on `base.path == "Element.id"` fixes the
+    /// whole class — `ElementDefinition.id`, `Element.id`, and every datatype
+    /// `.id` — without relaxing genuine resource-id validation
+    /// (`base.path == "Resource.id"` is left to honor the extension, correctly
+    /// yielding `id`) and without disturbing `Extension.url` (`uri`). It is a
+    /// no-op for the already-correct R4 and R6 packs.
+    pub(crate) fn value_type_override(&self) -> Option<&'static str> {
+        match self.base.as_ref().and_then(|b| b.path.as_deref()) {
+            Some("Element.id") => Some("string"),
+            _ => None,
+        }
+    }
+}
+
 #[derive(Debug, Deserialize)]
 pub(crate) struct EdBase {
     pub max: Option<String>,
+    /// The path of the base element this ED derives from (e.g. `Element.id`,
+    /// `Resource.id`, `Extension.url`). Used to resolve the value-domain type of
+    /// FHIRPath `System.*`-coded elements structurally rather than trusting the
+    /// (inconsistent) `structuredefinition-fhir-type` extension — see
+    /// [`Ed::value_type_override`].
+    pub path: Option<String>,
 }
 
 #[derive(Debug, Deserialize)]

crates/fhir-validator/src/converter/tree.rs

@@ -240,7 +240,13 @@ fn apply_element_content(element: &mut Node, ed: &Ed, warnings: &mut Vec<String>
         }
         1 => {
             let t = &ed.types[0];
-            element.schema.type_ = Some(t.effective_code());
+            // `Element.id`-derived elements are `string`, not the `id` the
+            // (inconsistent) fhir-type extension may claim (#424).
+            element.schema.type_ = Some(
+                ed.value_type_override()
+                    .map(str::to_string)
+                    .unwrap_or_else(|| t.effective_code()),
+            );
             if !t.target_profile.is_empty() {
                 element.schema.refers = Some(t.target_profile.clone());
             }
@@ -251,7 +257,11 @@ fn apply_element_content(element: &mut Node, ed: &Ed, warnings: &mut Vec<String>
                 "{}: multiple types without [x]; using the first",
                 ed.path
             ));
-            element.schema.type_ = Some(ed.types[0].effective_code());
+            element.schema.type_ = Some(
+                ed.value_type_override()
+                    .map(str::to_string)
+                    .unwrap_or_else(|| ed.types[0].effective_code()),
+            );
         }
     }
     apply_value_keywords(&mut element.schema, ed, warnings);

crates/fhir-validator/tests/converter_tests.rs

@@ -231,6 +231,121 @@ fn carries_informational_mirrors_and_short_labels() {
     assert_eq!(note["mustSupport"], Value::Null);
 }
 
+// ── issue #424: Element.id-derived ids are `string`, not `id` ──────────────
+//
+// The FHIRPath `System.String` code carries a `structuredefinition-fhir-type`
+// extension that HL7 populates inconsistently (R4B stamps `id` on every `.id`,
+// R5 on `ElementDefinition.id`). Taken literally, the `id` regex
+// `[A-Za-z0-9\-\.]{1,64}` rejects element ids with `[x]`, `:`, or > 64 chars —
+// i.e. StructureDefinitions the spec itself publishes. The converter now keys
+// the value-domain type on the base element: `Element.id` → `string`, while
+// `Resource.id` and `Extension.url` keep honoring the extension.
+
+const FHIR_TYPE_EXT: &str = "http://hl7.org/fhir/StructureDefinition/structuredefinition-fhir-type";
+const SYSTEM_STRING: &str = "http://hl7.org/fhirpath/System.String";
+
+/// Converts a one-field complex-type SD and returns that field's schema `type`.
+/// The field derives from `base_path` and its single `type[]` carries `code`
+/// plus a `structuredefinition-fhir-type` extension of `ext_value`.
+fn field_type(field: &str, base_path: &str, code: &str, ext_value: &str) -> Value {
+    let sd = json!({
+        "resourceType": "StructureDefinition",
+        "url": "http://example.org/StructureDefinition/T",
+        "name": "T", "kind": "complex-type", "derivation": "specialization", "type": "T",
+        "snapshot": { "element": [
+            { "path": "T", "min": 0, "max": "*" },
+            {
+                "path": format!("T.{field}"),
+                "min": 0, "max": "1",
+                "base": { "path": base_path, "min": 0, "max": "1" },
+                "type": [{
+                    "extension": [{ "url": FHIR_TYPE_EXT, "valueUrl": ext_value }],
+                    "code": code
+                }]
+            }
+        ]}
+    });
+    let conversion = convert(&sd).expect("conversion");
+    serde_json::to_value(&conversion.schema).unwrap()["elements"][field]["type"].clone()
+}
+
+#[test]
+fn element_id_is_string_despite_id_type_extension() {
+    // The bug: R4B/R5 stamp `valueUrl: "id"` on Element.id. Must resolve to
+    // `string` so the restrictive `id` regex is never applied to element ids.
+    assert_eq!(
+        field_type("id", "Element.id", SYSTEM_STRING, "id"),
+        json!("string")
+    );
+}
+
+#[test]
+fn resource_id_keeps_id_type() {
+    // Resource ids are the genuine constrained `id` token; the fix must NOT
+    // relax them. A `Resource.id`-based field keeps honoring the extension.
+    assert_eq!(
+        field_type("id", "Resource.id", SYSTEM_STRING, "id"),
+        json!("id")
+    );
+}
+
+#[test]
+fn extension_url_keeps_uri_type() {
+    // Extension.url is `uri` in every version's fhir.schema.json; the fix must
+    // not demote it to `string`.
+    assert_eq!(
+        field_type("url", "Extension.url", SYSTEM_STRING, "uri"),
+        json!("uri")
+    );
+}
+
+#[test]
+fn element_id_override_is_independent_of_the_extension_value() {
+    // Whatever the (inconsistent) extension claims, an Element.id-derived field
+    // is `string`: `id` (the R4B/R5 bug) and `string` (R4/R6) both land there.
+    assert_eq!(
+        field_type("id", "Element.id", SYSTEM_STRING, "string"),
+        json!("string")
+    );
+    assert_eq!(
+        field_type("id", "Element.id", SYSTEM_STRING, "id"),
+        json!("string")
+    );
+}
+
+#[test]
+fn multiple_types_without_choice_take_the_first_and_warn() {
+    // A non-`[x]` element with more than one type is malformed but tolerated:
+    // the converter warns and uses the first type. This also exercises the
+    // `value_type_override` fall-through on the multi-type arm — the field is
+    // not `Element.id`-derived, so the override yields None and the first
+    // type's effective code (`string`) is used.
+    let sd = json!({
+        "resourceType": "StructureDefinition",
+        "url": "http://example.org/StructureDefinition/T",
+        "name": "T", "kind": "complex-type", "derivation": "specialization", "type": "T",
+        "snapshot": { "element": [
+            { "path": "T", "min": 0, "max": "*" },
+            {
+                "path": "T.weird",
+                "min": 0, "max": "1",
+                "type": [{ "code": "string" }, { "code": "integer" }]
+            }
+        ]}
+    });
+    let conversion = convert(&sd).expect("conversion");
+    let value = serde_json::to_value(&conversion.schema).unwrap();
+    assert_eq!(value["elements"]["weird"]["type"], json!("string"));
+    assert!(
+        conversion
+            .warnings
+            .iter()
+            .any(|w| w.contains("multiple types without [x]")),
+        "expected a multiple-types warning, got: {:?}",
+        conversion.warnings
+    );
+}
+
 /// `ordered: true` slicing carries each slice's declaration ordinal as
 /// `order` — without it the engine's ordered check has nothing to compare and
 /// silently passes (`engine/slicing.rs`).

crates/fhir-validator/tests/pack_smoke.rs

@@ -248,3 +248,69 @@ fn r4_pack_validates_known_good_and_bad_resources() {
         serde_json::to_string_pretty(&outcome.errors).unwrap()
     );
 }
+
+/// Regression for issue #424: a StructureDefinition whose `element.id`s carry a
+/// choice suffix (`[x]`) or a slice qualifier (`:`) must NOT trip a
+/// primitive-value error. Before the converter fix, R4B/R5 typed
+/// `ElementDefinition.id` as the constrained `id` primitive, whose regex
+/// `[A-Za-z0-9\-\.]{1,64}` rejects exactly these ids — so the spec's own
+/// profiles failed validation. R4 was always clean (typed `string`), so it
+/// serves as the control that the assertion is meaningful.
+///
+/// Version-gated behind R4B/R5 (that is where the bug lived), so it runs in
+/// CI's `cargo test --workspace --all-features` but not in the coverage job,
+/// which builds default features (R4 only).
+#[cfg(any(feature = "R4B", feature = "R5"))]
+#[test]
+fn structuredefinition_element_ids_with_choice_or_slice_are_not_rejected() {
+    // A trimmed but structurally-valid StructureDefinition whose element ids
+    // include a choice base (`Observation.value[x]`) and a named slice
+    // (`Observation.category:vital-signs`). These are the exact shapes the `id`
+    // regex rejected.
+    let sd = json!({
+        "resourceType": "StructureDefinition",
+        "url": "http://example.org/StructureDefinition/choice-and-slice",
+        "name": "ChoiceAndSlice",
+        "status": "active",
+        "kind": "resource",
+        "abstract": false,
+        "type": "Observation",
+        "baseDefinition": "http://hl7.org/fhir/StructureDefinition/Observation",
+        "derivation": "constraint",
+        "differential": { "element": [
+            { "id": "Observation.value[x]", "path": "Observation.value[x]", "min": 0, "max": "1" },
+            {
+                "id": "Observation.category:vital-signs",
+                "path": "Observation.category",
+                "sliceName": "vital-signs",
+                "min": 0, "max": "1"
+            }
+        ]}
+    });
+
+    let versions = [
+        #[cfg(feature = "R4B")]
+        FhirVersion::R4B,
+        #[cfg(feature = "R5")]
+        FhirVersion::R5,
+    ];
+    for version in versions {
+        let validator = Validator::new(core_registry(version));
+        let outcome = validator.validate_sync(&sd, &ValidationOptions::default());
+
+        // The precise #424 failure: a primitive-value error on an element id.
+        let id_value_errors: Vec<_> = outcome
+            .errors
+            .iter()
+            .filter(|e| {
+                e.kind == helios_fhir_validator::ErrorKind::PrimitiveValue
+                    && e.path.ends_with(".id")
+            })
+            .collect();
+        assert!(
+            id_value_errors.is_empty(),
+            "{version:?}: element ids with [x]/: must not fail primitive-value validation, got: {}",
+            serde_json::to_string_pretty(&id_value_errors).unwrap()
+        );
+    }
+}