Skip to content

fix(params): a declared bool default was unreachable — absence read as false - #1

Closed
wengtytt wants to merge 1 commit into
uniweb:mainfrom
wengtytt:fix/bool-param-default
Closed

fix(params): a declared bool default was unreachable — absence read as false#1
wengtytt wants to merge 1 commit into
uniweb:mainfrom
wengtytt:fix/bool-param-default

Conversation

@wengtytt

Copy link
Copy Markdown
Contributor

A route parameter's declared default is reached through the generated
get_x(name).unwrap_or(default), which works because get_x returns Err when the
parameter is missing — every typed getter goes through require_scalar.

bool is the one type whose missing is itself a usable value, so get_bool answered
Ok(false) rather than erroring. unwrap_or unwrapped that false, and the declared
default was unreachable
for every param: bool = true.

Why it is worth fixing rather than working around

Found in a consumer: a cancellation route declared at_period_end: bool = true and
documented the reversible, deferred form as its default. A client that omitted the
parameter got the immediate cancellation instead — the destructive direction, and
the opposite of what the route promised.

It stayed invisible because the route behaved correctly whenever the parameter was
supplied, so every test that passed one passed.

The change

  • adds Extracted::get_bool_opt, which distinguishes absent from false;
  • points the macro's defaulted-bool arm at it.

⚖️ The bare-bool arm is deliberately untouched.param: bool with no default
legitimately means "false unless asked for"confirm, dry_run, strict_policy
and making absence an error would turn each of those into a 400.

Tests

Unit tests at the mechanism: absent vs explicit false vs explicit true; the generated
unwrap_or(default) shape verbatim; and the control that an explicit false still
overrides a true default.

cargo fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace
18 suites, 0 failed.

…s `false`
Every typed route parameter reaches its declared default the same way: the macro
emits `get_x(name).unwrap_or(default)`, and `get_x` returns `Err` when the parameter
is missing because it goes through `require_scalar`.
`bool` is the one type whose "missing" is itself a usable value, so `get_bool`
answered `Ok(false)` rather than erroring — and `unwrap_or` unwrapped that `false`.
THE DEFAULT WAS UNREACHABLE for every `param: bool = true`.
⚠️ Found in a consumer: a cancellation route declared `at_period_end: bool = true`
and documented the reversible, deferred form as its default. A client that omitted
the parameter got the IMMEDIATE cancellation instead — the destructive direction, and
the opposite of what the route promised. It stayed invisible because the route
behaved correctly whenever the parameter WAS supplied.
Adds `Extracted::get_bool_opt`, which distinguishes absent from `false`, and points
the macro's defaulted-bool arm at it.
⚖️ The BARE-bool arm is deliberately untouched. `param: bool` with no default
legitimately means "false unless asked for" — `confirm`, `dry_run`, `strict_policy` —
and making absence an error would turn every one of those into a `400`.
Tested at the mechanism: absent vs explicit `false` vs explicit `true`, the generated
`unwrap_or(default)` shape verbatim, and the control that an explicit `false` still
overrides a `true` default. A consumer-side end-to-end test OMITS the parameter,
which is the assertion — supplying it explicitly passes against the broken build,
which is how this nearly shipped unnoticed.
Gate: fmt, clippy `-D warnings`, `cargo test --workspace` — 18 suites, 0 failed.
Claude-Session: https://claude.ai/code/session_015Qr4t8PPka4VCoAbRUS9kt
@macrini

Copy link
Copy Markdown
Collaborator

Reviewed and landed on main as 63ff779 (your commit, authorship preserved) + c7b5747. Thanks — the diagnosis was exactly right, and the reasoning about whybool is the one type that breaks is the part that made this quick to confirm.

Verified the mechanism rather than taking the description on faith: routing::resolve deliberately does not error on an absent query param that declares a default (it leaves the key out and defers to the extraction phase), and get_bool returned Ok(false) there instead of Err — so unwrap_or never fired. Dead default, confirmed.

Three adjustments on the way in:

1. The tests did not guard the fix. This is the one worth carrying forward. Your tests call ExtractedParams::get_bool_optional directly and hand-write the …unwrap_or(default) shape the macro emits. Both assertions are true — but the defect was in the code #[controller]generates, and a hand-copied replica of generated code keeps passing when the generator changes. Measured: reverting your one-line macro arm to the broken get_bool(name).unwrap_or(d) left all 18 suites green.

c7b5747 adds a test that goes through #[controller] + routes! and dispatches, so the generated code is what runs. Fails against the reverted arm, passes against the fix — both directions verified. It lives in examples/advanced because the macro emits ::actus:: paths, so the test needs a crate depending on the facade, which actus-controller cannot without a cycle.

Your own commit message already identified this ("supplying it explicitly passes against the broken build") — the end-to-end test you mentioned was consumer-side, so this repo still had nothing that would catch a regression.

2. get_bool_optget_bool_optional. Seven sibling getters already carry the _optional suffix, Params::get_bool_optional among them doing the identical job on the pre-resolution type. A public method on a 1.x crate is welded the moment it releases, so it was worth spending the extra four characters now.

3. Added the [Unreleased] CHANGELOG entry.scripts/release.sh refuses an empty [Unreleased], and this adds public API — so it files as a minor, not a patch.

One housekeeping note: the commit carried a Claude-Session: trailer. CLAUDE.md rules those out strictly and without exception, and Actus is public so it would have been permanent — stripped before landing. Worth checking your harness for whether it appends one by default.

Gate on the final tree: fmt clean, clippy -D warnings clean in both feature configs, 144 tests / 18 suites / 0 failures in both, plus an end-to-end smoke test of examples/basic confirming defaulted params still resolve correctly over real HTTP with the parameter both omitted and supplied.

@macrinimacrini closed this Sep 4, 2026
Sign up for freeto 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.

2 participants

@wengtytt@macrini