Skip to content

Use properties' declared defaults when rendering whole-type defaults - #1041

Open
danieleades wants to merge 1 commit into
oxidecomputer:mainfrom
danieleades:fix/whole-type-default-props
Open

Use properties' declared defaults when rendering whole-type defaults#1041
danieleades wants to merge 1 commit into
oxidecomputer:mainfrom
danieleades:fix/whole-type-default-props

Conversation

@danieleades

Copy link
Copy Markdown
Contributor

Fixes #662

When typify renders a whole-type default value — a struct type's default used for its impl Default, or a parent property's default expression — properties absent from the default JSON object were filled in with Default::default(), ignoring those properties' own declared default values from the schema.

Using the schema from #662:

"separator": {
  "$ref": "#/definitions/SeparatorConfig",
  "default": {}
}

previously generated

super::SeparatorConfig {
    line_color: Default::default(),      // -> None
    line_thickness: Default::default(),  // -> 0
}

which disagrees with what deserializing the declared default value {} produces (the per-property serde default functions yield Some("#B2000000") / 1). So the rendered default did not mean what the schema says it means.

With this change, a property absent from the whole-type default is rendered from its own declared default when it has one:

super::SeparatorConfig {
    line_color: ::std::option::Option::Some("#B2000000".to_string()),
    line_thickness: 1_i64,
}

falling back to Default::default() only when the property declares no default. While here, the fallback is fully qualified as ::std::default::Default::default(), since a schema can legitimately define a type named Default.

The types-with-defaults.json golden test is extended with the SeparatorConfig/SeparatorHolder example from the issue, covering both a default: {} (all properties filled from their declared defaults) and a partial default: { "lineThickness": 5 } (the remaining properties filled from their declared defaults).

@danieleades
danieleades force-pushed the fix/whole-type-default-props branch from ee1428c to 0bb49b6 Compare July 21, 2026 07:36
@ahl

ahl commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Why is this the right handling for this schema?

When rendering a whole-type default value (used for a type's impl
Default and for parent properties' serde default functions), properties
absent from the default JSON object were filled in with
`Default::default()`, ignoring the properties' own declared schema
defaults. This made the rendered default disagree with deserialization,
which fills in absent properties via their serde default functions.

For example, given a property with `"default": 42` and a whole-type
default of `{}`, the rendered default produced `0` where deserializing
`{}` produces `42`.

Render absent properties using their declared default value when one
exists, falling back to `Default::default()` otherwise. Also fully
qualify that fallback as `::std::default::Default::default()` so it
cannot collide with a schema-defined type named `Default`.
@danieleades
danieleades force-pushed the fix/whole-type-default-props branch from 0bb49b6 to a366dc8 Compare July 28, 2026 08:36
@danieleades

Copy link
Copy Markdown
Contributor Author

Typify has to choose how to project a declared JSON object default into a Rust value. The consistency rule I am using is that T::default() should equal deserializing the schema's declared default JSON into T.

For {}, the generated Serde attributes invoke the properties' own default functions, so deserializing it produces line_color: Some("#B2000000") and line_thickness: 1. The current hand-rendered Default instead produces None and 0, meaning the two paths for the same declared default disagree. This change makes them agree, while an explicitly present member in the whole-object default still wins.

JSON Schema itself treats default as an annotation, so this is a Typify consistency choice rather than validator behavior. I will add a focused regression assertion that documents the deserialize-equals-Default invariant.

@ahl

ahl commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Just so I know: am I talking to a human or an agent?

@danieleades

Copy link
Copy Markdown
Contributor Author

Just so I know: am I talking to a human or an agent?

A human. Or an agent that's been instructed to say that if anyone asks...

Getting hard to tell these days!

@ahl

ahl commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Ok, human: are there other reasonable interpretations to consider? Have you looked at what other code generation tools do? My read of the spec is that there's no specific meaning for the property.

@danieleades

Copy link
Copy Markdown
Contributor Author

Ok, human: are there other reasonable interpretations to consider? Have you looked at what other code generation tools do? My read of the spec is that there's no specific meaning for the property.

so the OpenAPI/JSON Schema specs seem to be fairly silent on how a code generator should turn default annotations into language-level defaults. There are a couple of reasonable approaches:

  1. use language-specific defaults for any fields missing from the object-level default. This is what Typify does currently: integers become 0, Option<T> becomes None, etc. That isn’t non-compliant with the spec, because the spec doesn’t define recursive default application. The downside is that you get a different value depending on whether you deserialize the declared default or call T::default() in code.

  2. interpret the object-level default using the generated type’s deserialization behaviour. Explicit values in the object default win, and omitted fields use their own schema defaults where they have one. That’s the approach taken by this PR, and it means deserializing the declared default and calling T::default() give you the same result.

A third option would be to reject a declared default that doesn’t validate against the schema. That would apply if {} omitted a required property, but it wouldn’t apply to this example because neither property is required. Whether the generated Rust fields are Option or not is separate from whether {} is a valid JSON object.

The ecosystem seems kind of split. OpenAPI Generator’s Java output behaves broadly like this PR, although the implementation is different. Other generators, including OpenAPI Generator’s Rust output, tend to fall back to language defaults or ignore schema defaults at runtime.

So I don’t think this PR is implementing behaviour required by the spec. It’s making a Typify design choice: T::default() should match deserializing the schema’s declared default. Given Typify already applies property defaults during Serde deserialization, that seems more internally consistent than the current behaviour.

I think if the current behaviour is to be maintained that should be recorded as a deliberate choice

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.

Specifying default value for an object should use object's existing default field values

2 participants