fix: honour required contract on serialize for fields with intrinsic Rust defaults#4
Merged
Merged
Conversation
…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`.
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
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.
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![] }`:
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
Test plan