Skip to content

fix: honour required contract on serialize for fields with intrinsic Rust defaults#4

Merged
ndreno merged 1 commit into
mainfrom
fix/required-fields-with-intrinsic-defaults
May 13, 2026
Merged

fix: honour required contract on serialize for fields with intrinsic Rust defaults#4
ndreno merged 1 commit into
mainfrom
fix/required-fields-with-intrinsic-defaults

Conversation

@ndreno
Copy link
Copy Markdown

@ndreno ndreno commented May 13, 2026

Summary

When a schema marks a field `required` but its Rust type has an intrinsic default (`Vec`, `HashMap`, `Option`), the previous behaviour (from #918) emitted both `#[serde(default)]` and `skip_serializing_if`. Deserialize was lenient (`{}` parsed), but serialize silently dropped the field when it was empty — violating the schema's wire contract that a required field must always be present.

This PR introduces `StructPropertyState::RequiredWithDefault`, which splits the two halves: emit `#[serde(default)]` alone, keeping deserialize lenient while serialize always renders the field.

Before / after

For schema `{ required: ["tags"], properties: { tags: { type: "array" } } }` and value `Foo { tags: vec![] }`:

Before After
Deserialize `{}` OK OK
Serialize `Foo { tags: vec![] }` `{}` `{"tags":[]}`
Round-trip lossy (loses `tags` key) lossless

Wire-format change

This is a breaking change for downstream JSON consumers. Any code that ingests serialized output from typify-generated types — diff tooling, snapshot tests, mock services, contract tests — will see required fields appear where they were previously omitted (`[]`, `{}`, or `null`). Payload size grows modestly for schemas with many required nullable/array fields (the GitHub OpenAPI fixture moves 908 lines for this reason).

Documented under Bug fixes in `CHANGELOG.adoc`.

Tests

  • Codegen fixture `typify/tests/schemas/required-with-implicit-defaults.json` — three schemas covering required Vec / Map (with and without explicit `default: []`/`{}`) and a mixed required-and-optional struct. `expectorate` verifies the generated output.
  • Five runtime tests in `typify-test/src/main.rs`:
    • `test_required_implicit_defaults_deserialize_empty` — `from_str("{}")` succeeds, fields use intrinsic empties.
    • `test_required_implicit_defaults_deserialize_partial` — partial JSON fills the gap.
    • `test_required_implicit_defaults_serialize_emits_required_fields` — regression-critical: required empty Vec/Map still appear in serialized output.
    • `test_mixed_required_and_optional_serialization` — required emits, optional empty still skipped.
    • `test_required_implicit_defaults_roundtrip` — content survives serialize → deserialize.
  • Existing fixtures regenerated (intentional): `github.out` (-908), `vega.out` (-22), `draft-2019-09.rs`, `draft-2020-12.rs`, `property-pattern.rs`, `rust-collisions.rs`, `untyped-enum-with-null.rs`. Each diff replaces `#[serde(default, skip_serializing_if = "...")]` with `#[serde(default)]` exactly where the field is in the schema's required array.

Test plan

  • `cargo fmt --all -- --check`
  • `cargo clippy --workspace --all-targets --locked --exclude typify-test` (only pre-existing dead-code warnings, addressed in PR ci: cover fmt, clippy, tests, doc, advisories, and MSRV #3)
  • `cargo test --workspace --locked` — all suites pass, including the 5 new runtime tests
  • `cargo deny check advisories` (no new advisories)

…red fields with intrinsic Rust defaults

When a schema marks a field `required` but its Rust type has an
intrinsic default (`Vec`, `HashMap`, `Option<T>`), the previous code
went through PR oxidecomputer#918's path and emitted both `#[serde(default)]` and
`skip_serializing_if`. That made deserialize lenient (`{}` parses) but
also silently dropped the field from serialize output when empty —
which violates the schema's wire contract: a `required` field must
always be present.

Introduces a new `StructPropertyState::RequiredWithDefault` that
splits the two halves: emit `#[serde(default)]` alone, so deserialize
stays lenient while serialize always renders the field. The promotion
from `Optional` happens in `convert_struct_property` only when the
field is in the schema's required array.

Tests:
- New fixture `typify/tests/schemas/required-with-implicit-defaults.json`
  covering required Vec / Map (with and without explicit empty default)
  and mixed required/optional structs. Verifies via expectorate that
  required fields emit `#[serde(default)]` only and optional ones keep
  `skip_serializing_if`.
- Five runtime tests in `typify-test/src/main.rs` proving:
    - `from_str("{}")` succeeds (lenient deserialize)
    - serializing the default emits required fields with empty values
    - optional empty fields are still skipped
    - serialize → deserialize round-trip preserves content

Fixture diffs in `github.out`, `vega.out`, and four workspace fixtures
are intentional ripple effects: every required Vec/Map/Option field in
the corpus loses its `skip_serializing_if` clause.

Wire-format change documented in CHANGELOG: payloads that previously
omitted empty required fields will now render them as `[]`, `{}`, or
`null`.
@ndreno ndreno merged commit 1a999ac into main May 13, 2026
8 checks passed
@ndreno ndreno mentioned this pull request May 13, 2026
4 tasks
@ndreno ndreno deleted the fix/required-fields-with-intrinsic-defaults branch May 13, 2026 09:56
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.

1 participant