Skip to content

TypeSchema/TS: emit validateRequired() for inherited base-required fields#166

Merged
ryukzak merged 7 commits into
atomic-ehr:mainfrom
cognovis:fix/profile-validate-inherited-required
Jun 1, 2026
Merged

TypeSchema/TS: emit validateRequired() for inherited base-required fields#166
ryukzak merged 7 commits into
atomic-ehr:mainfrom
cognovis:fix/profile-validate-inherited-required

Conversation

@sussdorff
Copy link
Copy Markdown
Contributor

Summary

  • Profile snapshots silently dropped any base-resource cardinality the profile differential did not re-state. Generated `validate()` methods therefore skipped inherited required fields (e.g. `Provenance.target`, `Provenance.recorded`), so resources missing those fields passed TypeScript-side validation only to be rejected by the FHIR server at write time.
  • `SnapshotProfileTypeSchema` now carries an optional `inheritedRequiredFields: string[]` listing top-level base fields the profile chain leaves untouched. `buildProfileSnapshot` populates it; the TS writer appends one `validateRequired()` call per entry.
  • Why a separate list instead of merging base `fields` into the snapshot: a full merge would also pull every base choice variant (`valueString`, `valueRange`, …) into the profile's getter/setter surface — even on profiles that explicitly narrowed `value[x]` to one variant. Keeping `fields` as the differential-merged view and `inheritedRequiredFields` as the additive validation hook avoids that bloat.
  • `flatProfile` merge loop also handles the relaxation case: if any profile in the chain sets `min: 0` on a field, the merged field's `required` is reset to false. Without this, the field-builder's `isRequired` walk would still return true (because the base ancestor requires it) and `validate()` would emit `validateRequired()` for a field the profile intentionally relaxed.

Generated code (Provenance profile that does NOT re-state target/recorded)

Before:
```typescript
validate(): { errors: string[]; warnings: string[] } {
const profileName = "PraxisProposalProvenance"
return {
errors: [
...validateRequired(res, profileName, "activity"),
...validateRequired(res, profileName, "agent"),
],
warnings: [],
}
}
```

After:
```typescript
validate(): { errors: string[]; warnings: string[] } {
const profileName = "PraxisProposalProvenance"
return {
errors: [
...validateRequired(res, profileName, "activity"),
...validateRequired(res, profileName, "agent"),
...validateRequired(res, profileName, "target"),
...validateRequired(res, profileName, "recorded"),
],
warnings: [],
}
}
```

Tests

  • Unit (`test/unit/typeschema/utils.test.ts`): three cases covering inherited-required collection, `min: 0` relaxation, and the all-restated path that yields `undefined`.
  • Snapshot (`local-package.test.ts.snap`): `ExampleTypedBundle` regenerated — `Bundle.type` is base-R4 required and was previously missed.
  • Existing R4 and US Core example snapshots are unchanged (those profiles already restate every base-required field).

sussdorff added 4 commits May 24, 2026 17:52
Profile snapshots silently dropped any base-resource cardinality the
profile differential did not re-state. Downstream consumers (generated
validate() methods, schema diff tooling) had no way to know that, e.g.,
a Provenance profile still requires `target` and `recorded` from R4
core. This commit fixes the snapshot itself; the TS writer change lands
in a separate commit so each layer can be reviewed independently.

- types.ts: SnapshotProfileTypeSchema gains an optional
  `inheritedRequiredFields: string[]` listing top-level fields the base
  resource marks required and the profile chain leaves untouched.
- utils.ts (buildProfileSnapshot): walks the base resource's `fields`
  after flatProfile() and collects every entry with `required: true`
  whose name is absent from the merged differential fields. Listed
  separately from `fields` so this fix only enables additional
  validation — it does not inflate the profile's getter/setter surface
  with unused base metadata (e.g. value[x] variants the profile bans).
- utils.ts (flatProfile merge loop): when a profile in the chain sets
  `min: 0` on a field, the merged field's `required` is reset to false.
  Required for the relaxation case so a profile that intentionally
  drops a base requirement doesn't accidentally still trigger
  validateRequired() emission.
Generated profile validate() methods only emitted validateRequired()
for fields the profile differential mentioned. Inherited cardinality
from the base R4 resource (e.g. Provenance.target, .recorded) was
silently skipped, so a downstream resource could pass .validate() in
TypeScript and then be rejected by the FHIR server for missing the
base requirement.

After the snapshot now carries `inheritedRequiredFields`, the writer
just appends one validateRequired() per entry at the end of the errors
array — the regular field loop still handles fields the profile
restates (so AC-3 — profile relaxes with min:0 — falls out naturally
because the field stays in `fields` with required:false).

Before:
    return {
        errors: [
            ...validateRequired(res, profileName, "agent"),
            ...validateRequired(res, profileName, "activity"),
        ],
        warnings: [],
    }

After (Provenance profile that leaves target/recorded untouched):
    return {
        errors: [
            ...validateRequired(res, profileName, "agent"),
            ...validateRequired(res, profileName, "activity"),
            ...validateRequired(res, profileName, "target"),
            ...validateRequired(res, profileName, "recorded"),
        ],
        warnings: [],
    }
…ation

- AC-2: profile leaving base-required fields untouched lists them in
  inheritedRequiredFields and keeps explicitly-restated fields out of
  it (mirrors the Provenance.target/.recorded/.agent pattern).
- AC-3: profile lowering min to 0 on a base-required field marks the
  merged field required:false and excludes it from
  inheritedRequiredFields, so validate() emits nothing for it.
- inheritedRequiredFields is undefined when the profile restates every
  base requirement (no spurious empty arrays).
ExampleTypedBundle profile slices entry[] but does not re-state
Bundle.type in its differential. With the inherited-required fix in
place, validate() now correctly emits validateRequired() for type.
sussdorff added a commit to cognovis/codegen that referenced this pull request May 24, 2026
Replants cognovis/mira-adapters onto cognovis/next + committed dist/.

Includes:
- Consolidated canonical-resolver fix (codegen-vrq + codegen-pkt)
- Inherited-required validate() emission (codegen-8iw, upstream PR atomic-ehr#166)
- Preserve fixed CodeableConcept codings (codegen-vwd, upstream PR atomic-ehr#165)
- COGNOVIS strategy doc + bd init infrastructure
- KBV newer-deps repro script
sussdorff added 3 commits May 24, 2026 18:26
The first cut filtered base-required fields out as soon as the
profile mentioned them — but profile snapshots frequently inline
inherited elements without restating the required flag (FHIRSchema
copies the snapshot expansion verbatim). The field-builder then
emits target/recorded with required:false even though the base
requires them, and the regular validate loop skipped them too.

Re-derive: a base-required field is "inherited" unless the profile
either restated required:true (already covered by the regular loop)
or explicitly relaxed min to 0. Adds AC-2b to lock the snapshot-
inlined case to the same behaviour as a profile that omits the field
entirely.
…ission

End-to-end coverage via localStructureDefinitions: a Provenance profile
whose differential touches only activity/agent/entity must still emit
validateRequired() for the inherited base-R4 target and recorded fields.

Fixture mirrors the real de.cognovis.fhir.praxis PraxisProposalProvenance
differential — the profile that originally surfaced codegen-8iw.
… validated

The profile-helpers import guard keyed solely on snapshot.fields being
non-empty. A profile whose only validation comes from an inherited
base-required field (e.g. an Extension profile relying on the base
Extension.url) emitted validateRequired() without importing it →
tsc TS2304 "Cannot find name 'validateRequired'".

Surfaced by the upstream Test SDKs CI (on-the-fly norge-r4 example,
Extension_NoBasisMiddlename). Extend the guard to also fire when
inheritedRequiredFields is non-empty, and cover it with a minimal
Extension fixture that states no differential fields.
@ryukzak ryukzak merged commit 4bee4a2 into atomic-ehr:main Jun 1, 2026
16 checks passed
@ryukzak
Copy link
Copy Markdown
Collaborator

ryukzak commented Jun 1, 2026

Follow-ups now that this is merged:

sussdorff added a commit to cognovis/codegen that referenced this pull request Jun 2, 2026
…nherited-required choice-skip + atomic-ehr#166 inherited-required validation (f027e0f)
@sussdorff
Copy link
Copy Markdown
Contributor Author

Thank you! highly appreciated. So far it did not find any issues with the new code generation on our end, so this looks good to us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants