Skip to content

fix: enforce dependentRequired in generated schemas - #80

Merged
damaz91 merged 1 commit into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/dependent-required-injection
Sep 24, 2026
Merged

damaz91 merged 1 commit into
Universal-Commerce-Protocol:mainfrom
vishkaty:fix/dependent-required-injection

Conversation

@vishkaty

Copy link
Copy Markdown
Contributor

Description

dependentRequired (JSON Schema 2020-12) is dropped by the typescript-zod
target of quicktype. common/types/time_interval.json has no required list
and allows an empty fragment, but declares

"dependentRequired": { "opens": ["closes"], "closes": ["opens"] }

so opens and closes must appear together. The generated
TimeIntervalSchema accepted { opens: "09:00" }, and the objects composing
it through allOf (ExceptionHourElementSchema, DailyHourElementSchema)
inherited the gap: { valid_from, valid_through, opens } and { day, opens }
both passed on main.

What changes

scripts/inject-schema-constraints.mjs records the keyword, keyed by the
resolved property set like the other object-level rules and following $ref
and allOf the way properties are resolved, and renders it as an
object-level superRefine. Every occurrence of a property set must agree on
the rule list, so a coincidental shape clash injects nothing; a rule naming a
field the property set does not carry (a request projection omitted it) is
skipped rather than approximated. Idempotent on re-run.

Vacuous rules are not rendered

A rule whose dependents the generated object already requires can never
change a verdict: z.object rejects their absence first. Rendering it anyway
would wrap the exported ZodObject in a ZodEffects, which loses .shape,
.extend, .pick, .omit and .merge at the type level for no behavioural
gain. Such rules are dropped per subject before rendering, and the injector
reports them (N vacuous dependentRequired rule(s) skipped).

Vacuity is judged on the generated property chain (.optional() and its
relatives, undefined-accepting bases, unions with such a member, references
followed to their top-level declaration), not on the schema required list,
for two reasons found while measuring:

  • An update and a response variant of one type share a property set and
    differ only in optionality, and the property-set index cannot tell them
    apart. A schema-side check would record the rule for both, or through the
    ambiguity guard for neither.
  • quicktype sometimes emits a required property as optional.
    daily_hour.json lists day, opens and closes as required, yet on
    main DailyHourElementSchema accepts {}. On that object the pair rule is
    live, and a schema-side check would have dropped it and left { day, opens }
    passing.

Evidence, measured at runtime on the compiled zod schemas by comparing the
constructor of every *Schema export on main (99572ca) with this branch:

  • 318 exports on both; none removed, none added.
  • Exactly 5 exports change class, all ZodObject to ZodEffects, and each
    is a live rule that now rejects a payload main accepted: TimeIntervalSchema,
    ExceptionHourElementSchema, DailyHourElementSchema, and the aliases
    ExceptionHourSchema and DailyHourSchema.
  • Without the skip the same measurement showed 8. The extra three were
    FulfillmentMethodCreateRequestSchema and its aliases
    FulfillmentMethodSchema and MethodElementSchema, wrapped for the
    destinations needs type rule of fulfillment_method.json, which is
    vacuous there because type is required on every fulfillment object main
    generates. They keep .shape now.

Relation to #77 and the fulfillment request projection PR

fulfillment_method.json declares
dependentRequired: { destinations: ["type"] }. On main every fulfillment
object requires type, so the rule is vacuous and nothing in that family
changes here. The companion PR that projects the fulfillment family per
request variant leaves one gap open on purpose: its update variant carries
type optional, so { line_item_ids, destinations } without type is
accepted there. With both applied (verified by stacking this commit on that
branch and regenerating), FulfillmentMethodUpdateRequestSchema is the one
fulfillment object that gains the rule, the create and response variants stay
plain ZodObject (reported as 2 vacuous skips), and the test that PR dropped
(update request rejects destinations without type) passes: 233 tests, 0
failing.

Verification

  • ./generate_models.sh against ucp d3ccb55c, twice: byte-identical both
    times.
  • npm test: 158 passing, 0 failing (main: 147; this PR adds 11). Every added
    test was seen failing first: the injector tests against the unchanged
    script, the generated-schema tests against the schemas compiled from main.
  • npm run build and npm run build:noEmit: clean. Prettier clean on every
    file the pre-commit hook covers.
  • Kill test: removing the allOf walk from the dependentRequired collector
    regenerates with only TimeIntervalSchema constrained and turns 3 tests
    red (the allOf injector test, ExceptionHourElementSchema,
    DailyHourElementSchema).

Before/after:

TimeIntervalSchema          {}                                    accepted on main, accepted now
TimeIntervalSchema          { opens }                             accepted on main, rejected now
TimeIntervalSchema          { closes }                            accepted on main, rejected now
ExceptionHourElementSchema  { valid_from, valid_through, opens }  accepted on main, rejected now
DailyHourElementSchema      { day, opens }                        accepted on main, rejected now
FulfillmentMethodSchema     any payload                           unchanged, still a ZodObject with .shape

Category (Required)

  • Core Protocol: Changes to the base communication layer, global context, or breaking refactors. (Requires Technical Council approval)
  • Governance/Contributing: Updates to GOVERNANCE.md, CONTRIBUTING.md, or CODEOWNERS. (Requires Governance Council approval)
  • Capability: New schemas (Discovery, Cart, etc.) or extensions. (Requires Maintainer approval)
  • Documentation: Updates to README, or documentations regarding schema or capabilities. (Requires Maintainer approval)
  • Infrastructure: CI/CD, Linters, or build scripts. (Requires DevOps Maintainer approval)
  • Maintenance: Version bumps, lockfile updates, or minor bug fixes. (Requires DevOps Maintainer approval)
  • SDK: Language-specific SDK updates and releases. (Requires DevOps Maintainer approval)
  • Samples / Conformance: Maintaining samples and the conformance suite. (Requires Maintainer approval)
  • UCP Schema: Changes to the ucp-schema tool (resolver, linter, validator). (Requires Maintainer approval)
  • Community Health (.github): Updates to templates, workflows, or org-level configs. (Requires DevOps Maintainer approval)

Related Issues

Related to #77: together with the fulfillment request projection PR this
closes its update-side gap. No issue is filed for the time_interval gap
itself.

Checklist

  • I have followed the Contributing Guide (including Conventional Commits title requirements and ! for breaking changes).
  • I have updated the documentation (if applicable).
  • My changes pass all local linting and formatting checks.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • (For Core/Capability) I have included/updated the relevant JSON schemas.
  • I have regenerated Python Pydantic models by running generate_models.sh under python_sdk.

Screenshots / Logs (if applicable)

N/A. The before/after table and the runtime class comparison above are the
repro.

@damaz91 damaz91 added status:needs-triage Signal that the PR is ready for human triage status:under-review and removed status:needs-triage Signal that the PR is ready for human triage labels Sep 18, 2026
The typescript-zod target of quicktype drops `dependentRequired`, so the
generated TimeInterval accepted `opens` without `closes` (and the
reverse), and the objects composing it through allOf (DailyHour,
ExceptionHour) inherited the gap. Record the keyword in the constraint
injector, keyed by the resolved property set like the other object-level
rules and following $ref/allOf the way properties are resolved, and
render it as an object-level superRefine. Every occurrence of a property
set must agree on the rule list, so a coincidental shape clash injects
nothing; a rule naming a field the property set does not carry (a
request projection omitted it) is skipped rather than approximated.
Idempotent on re-run.

A rule whose dependents the generated object already requires can never
change a verdict, because z.object rejects their absence first. Such a
rule is not rendered: the superRefine wrapper turns an exported
ZodObject into a ZodEffects and drops .shape, .extend, .pick, .omit and
.merge at the type level for no behavioural gain. Vacuity is judged on
the generated property chain, not on the schema `required` list. An
update and a response variant of one type share a property set and
differ only in optionality, so the property-set index cannot tell them
apart; and quicktype sometimes emits a required property as optional
(DailyHour: all three fields are optional in the generated object, a
separate pre-existing loss), in which case the rule is live and kept.

Effect on the generated file: TimeIntervalSchema,
ExceptionHourElementSchema and DailyHourElementSchema (with the aliases
ExceptionHourSchema and DailyHourSchema) gain the rule and become
ZodEffects; each now rejects a payload main accepted. The destinations
needs type rule of fulfillment_method.json is vacuous on every
fulfillment object main generates, since `type` is required on all of
them, so no fulfillment export changes class. Once the fulfillment
family is projected per request variant (js-sdk#77), the update variant
carries `type` optional and is the one object that gains the rule.

Tests: main has 147, this change adds 11 (158 total).
@damaz91
damaz91 force-pushed the fix/dependent-required-injection branch from 795de24 to c572b0c Compare September 23, 2026 16:24
@damaz91
damaz91 merged commit 06ae497 into Universal-Commerce-Protocol:main Sep 24, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants