A generated DTO's Default disagrees with the boolean a client's omission now gives #232

New issue

Closed

opened 2026-09-16 01:50:23 +02:00 by Claude · 1 comment

Claude commented

2026-09-16 01:50:23 +02:00

Collaborator

#226 gave 65 generated booleans the value upstream's constructor starts them at when a client omits them. Default was left alone, so the two now disagree.

The disagreement

T::default() still yields false for every one of those fields, while serde_json::from_str::<T>("{}") yields true. Any ..Default::default() construction therefore serves false where upstream serves true.

Nothing is broken today: editing.rs::shown() sets the six LibraryOptions flags by hand, and configuration.rs::described() sets the ServerConfiguration ones. The trap is the next boolean upstream adds to a constructor: the generator picks it up on the wire side and silently not on the construct side.

The shape of the fix

Have gen_dto emit an impl Default beside a struct whenever any of its fields uses crate::wire::yes, so the two cannot drift. The check that belongs with it — T::default() == from_str::<T>("{}") for every generated type — fails for all twenty schemas until that impl exists, so it has to come second.

Two latent limits in the initialiser reader

xtask/src/initializers.rs, neither of which bites at the pin:

constructor_body takes the first public {Name}(…). ProfileCondition has three, and the parameterless one happens to come first. A class declaring it second would be read from the wrong body. Its [^)]* also breaks on a parameter containing ).
The assignment pattern would read a conditionally assigned X = true; inside an if as an unconditional default. Every constructor it reads at the pin is flat.

Found reviewing #226.

#226 gave 65 generated booleans the value upstream's constructor starts them at when a client omits them. `Default` was left alone, so the two now disagree. ## The disagreement `T::default()` still yields `false` for every one of those fields, while `serde_json::from_str::<T>("{}")` yields `true`. Any `..Default::default()` construction therefore serves `false` where upstream serves `true`. Nothing is broken today: `editing.rs::shown()` sets the six `LibraryOptions` flags by hand, and `configuration.rs::described()` sets the `ServerConfiguration` ones. The trap is the next boolean upstream adds to a constructor: the generator picks it up on the wire side and silently not on the construct side. ## The shape of the fix Have `gen_dto` emit an `impl Default` beside a struct whenever any of its fields uses `crate::wire::yes`, so the two cannot drift. The check that belongs with it — `T::default() == from_str::<T>("{}")` for every generated type — fails for all twenty schemas until that impl exists, so it has to come second. ## Two latent limits in the initialiser reader `xtask/src/initializers.rs`, neither of which bites at the pin: - `constructor_body` takes the **first** `public {Name}(…)`. `ProfileCondition` has three, and the parameterless one happens to come first. A class declaring it second would be read from the wrong body. Its `[^)]*` also breaks on a parameter containing `)`. - The assignment pattern would read a conditionally assigned `X = true;` inside an `if` as an unconditional default. Every constructor it reads at the pin is flat. Found reviewing #226.

Claude referenced this issue

2026-09-16 02:25:50 +02:00

A LibraryOptions boolean a client omits is false, where upstream's constructor makes it true #226

Claude commented

2026-09-16 07:21:14 +02:00

Author

Collaborator

Fixed at the generator, and landed on main at ec3a4ee8.

gen_dto emits an impl Default beside any struct carrying a field that starts at true, so a type's own default and the value a client's omission gives are one and the same — twenty structs, sixty-eight booleans. It also emits a test asserting T::default() == from_str("{}") for all 259 generated structs, which fails on a single mutated field and names it.

The two limits in the reader were measured rather than assumed: only ProfileCondition and TranscodingProfile have overloaded constructors and both declare the parameterless one first, and no owning class assigns a boolean inside a branch. Both are fixed anyway — the constructor is now matched explicitly and only assignments at brace depth zero count — and the generated output is unchanged by either.

The corrected default exposed a real bug. Upstream's CreatePlaylistDto.IsPublic starts true, but the controller forwards it as a nullable and the manager settles OpenAccess = request.Public ?? false. So a playlist made with no body is private, and one whose body omits the flag is public. Ours collapsed both. All three cases are pinned by tests now, including the explicit false that is the leak direction.

Two more call sites moved to upstream's own value rather than false: three ServerConfiguration flags, and EnableAudioVbrEncoding on a transcoding profile, which reaches the streaming URL.

Fixed at the generator, and landed on `main` at `ec3a4ee8`. `gen_dto` emits an `impl Default` beside any struct carrying a field that starts at `true`, so a type's own default and the value a client's omission gives are one and the same — twenty structs, sixty-eight booleans. It also emits a test asserting `T::default() == from_str("{}")` for all 259 generated structs, which fails on a single mutated field and names it. The two limits in the reader were measured rather than assumed: only `ProfileCondition` and `TranscodingProfile` have overloaded constructors and both declare the parameterless one first, and no owning class assigns a boolean inside a branch. Both are fixed anyway — the constructor is now matched explicitly and only assignments at brace depth zero count — and the generated output is unchanged by either. **The corrected default exposed a real bug.** Upstream's `CreatePlaylistDto.IsPublic` starts `true`, but the controller forwards it as a nullable and the manager settles `OpenAccess = request.Public ?? false`. So a playlist made with **no body** is private, and one whose body omits the flag is public. Ours collapsed both. All three cases are pinned by tests now, including the explicit `false` that is the leak direction. Two more call sites moved to upstream's own value rather than `false`: three `ServerConfiguration` flags, and `EnableAudioVbrEncoding` on a transcoding profile, which reaches the streaming URL.

Claude closed this issue

2026-09-16 07:21:15 +02:00