Adopt tolerant-response deserialization: sweep #[serde(default)] across model fields and codify the policy in CLAUDE.md

[sdairs]

Problem

The library is inconsistent about #[serde(default)] on required (non-Option) model fields, and PR #311's review remediation pulled it in the wrong direction. Current state of crates/clickhouse-cloud-api/src/models.rs:

• 668 required fields carry #[serde(default)] (tolerant: missing field on deserialize → Default::default())
• 382 required fields do not (strict: missing field fails the whole response deserialization)
• 12 Option fields lack #[serde(default)] (harmless — serde treats missing Option as None — but inconsistent)
• ~50 of the tolerant fields are in ClickStack models; commit a768db6 (PR Fix OpenAPI drift: ClickStack expansion, quotas, and model updates #311 remediation, prompted by a Bugbot finding) stripped serde(default) from ClickStackTraceSource.default_table_select_expression and added strictness regression tests, establishing a "strict" convention that contradicts the majority pattern and the CLAUDE.md line "Retain #[serde(default)] on model fields". (The Fix OpenAPI drift: ClickStack expansion, quotas, and model updates #311 branch has since reverted that commit in dcdb3aa, inverting the trace-source test into a missing-field round-trip; the pre-existing strict fields, including ClickStackLogSource's, remain.)

Multiple teams evolve the Cloud API independently and can remove response fields without notice. With strict deserialization, a dropped response field breaks the CLI for users even though the API call itself succeeds — and via discriminated_union! (which propagates payload errors for known discriminators rather than falling back to Unknown), one missing field on one trace source fails an entire list sources response. The 382 strict fields being "fine" so far is survivorship, not safety: each is a latent outage point whose field simply hasn't been dropped yet.

Why tolerant-response is the right policy

#[serde(default)] only affects deserialization — it never changes what we serialize. On a required T field it yields exactly "strict in what we send, liberal in what we accept" (Postel's law) with a single shared model:

• Requests stay strict via the type system: a required field is T, so it must be populated to construct the request and is always serialized. No serde attribute weakens this.
• Responses degrade gracefully: a dropped field becomes ""/0/false instead of a hard error. For a CLI, an empty cell beats a dead command.
• Detection is CI's job, not runtime's: the analyzer derives requiredness purely from T vs Option<T> (rust_inventory.rs reads only rename/untagged/other), so tolerance costs zero drift-detection fidelity. The daily drift workflow turns spec drift into a GitHub issue instead of a user-facing outage.
• Internal consistency: the library already ignores unknown fields (no deny_unknown_fields), gives every string enum an Unknown(String) catch-all, and (as of Fix OpenAPI drift: ClickStack expansion, quotas, and model updates #311) lands unknown union payloads in Unknown(serde_json::Value). Strict struct fields are the one place the library still hard-fails on server evolution.
• Ecosystem precedent: proto3/gRPC (missing scalar → default value, clients must tolerate), AWS Smithy SDKs (lenient outputs, input requiredness enforced at build time), Kubernetes API conventions / kube-rs (Option + default pervasively), Fowler's Tolerant Reader. openapi-generator's strict Rust output is the cautionary tale, not the idiom.

Alternative considered and rejected: blanket Option<T> on response fields

171 model types are transitively shared between request and response positions (computed from client.rs signatures), so blanket-Option would (a) lose type-level enforcement of required request fields, (b) destroy the T vs Option<T> code↔spec requiredness mapping the analyzer verifies bidirectionally, and (c) push unwrap_or_default() sentinels to every call site instead of declaring the tolerance once in the model.

Proposed policy (to codify in CLAUDE.md)

Optionality and deserialization tolerance. Requiredness is expressed only in the type: required non-nullable fields are T, optional/nullable fields are Option<T> + skip_serializing_if. We are strict in what we send (the type system enforces required request fields) and liberal in what we accept: every model field carries #[serde(default)], unknown fields are ignored (never deny_unknown_fields), and enums/unions carry Unknown catch-alls. Deserialization of a response must never fail because the API added, removed, or changed a field — spec conformance is enforced by the analyzer and the daily drift job, not by runtime errors. Exception: a field that structurally discriminates an untagged union must stay strict (prefer converting the union to discriminated_union! instead). If defaulting a specific field would be misleading, make it Option<T> with an optionality_exemptions entry, not strict.

This replaces the current unexplained "Retain #[serde(default)] on model fields" line, whose lack of rationale is exactly what let a review bot ratchet PR #311 toward strictness.

Scope of work

This lands as one PR delivering the complete end state: union conversions, an enforcement test, the CLAUDE.md policy, and the full sweep. There is no interim policy/code gap to manage — internal sequencing is left to the implementer, with one exception called out below. Baseline is main after PR #311 merges: ClickStackSource, ClickStackTileConfig, ClickStackOnClickTarget, and the webhook/alert-channel-adjacent unions are already discriminated_union!-dispatched there.

Union conversions — the one hard ordering constraint

Convert the remaining structurally-matched untagged sub-unions to discriminator dispatch before (or together with) defaulting their distinguishing fields: the Builder-vs-RawSql chart configs (Line/Bar/CategoricalBar/Table/Number/Pie) and ClickStackAlertChannel. These are the one place required-field strictness is load-bearing: untagged matching picks the first variant whose required fields are present, so defaulting the distinguishing fields (configType, sql, …) while the unions are still untagged would make Builder swallow RawSql payloads.

RawSql payloads carry configType: "rawSql"; Builder payloads lack the key, so this needs a key-absence arm in discriminated_union! (None => Variant) — the current macro dispatches only on a key's string value. Two semantics to pin down with explicit tests, not accidents:

• None conflates "key absent" and "key present but not a string" — decide deliberately (treating both as Builder is probably right) and test it.
• Once the sweep lands, Builder variants become total (every field defaultable), so the absence arm always succeeds and the sub-union's Unknown is reachable only via unrecognized configType values. This matches spec semantics (payloads reaching the sub-union already passed the parent's displayType dispatch, and "no configType" is the builder discriminator), but it changes what Unknown means for these unions — assert the new behavior in tests.

Enforcement test

Nothing currently enforces the policy: the analyzer ignores serde(default), so a future agent (or bot-prompted remediation) could strip it again silently. Make the policy self-enforcing:

• Add default: bool to SerdeOptions in rust_inventory.rs (small; the parser already handles serde attrs).
• Add a plain test in clickhouse-cloud-api (reusing the analyzer inventory as the existing dev-dependency — no new FindingKind, no report schema_version bump, no Python rendering; this is an internal convention, not spec drift) asserting every T field carries #[serde(default)].
• Since the sweep lands in the same PR, the test ships with an empty exception list — or, if any structural-discriminator field genuinely cannot be converted (ideally none), a permanent, commented entry.

CLAUDE.md policy

Land the policy text above, replacing "Retain #[serde(default)] on model fields". Include the rationale so future agents can cite it when review bots re-file "serde(default) masks required field" findings.

The sweep

Add #[serde(default)] to every remaining strict T field — ClickStack and all legacy domains (Activity, ApiKey, Backup, ClickPipes, Postgres, SCIM, …) — adding Default impls where a field type lacks one, plus the 12 bare Option fields for uniformity. Replace any remaining strictness regression tests with missing-field-degrades-to-default round-trip tests (the trace-source one was already inverted in dcdb3aa on the #311 branch).

Documentation

Document the round-trip caveat in the crate docs: a consumer that GETs an object and writes it back could persist a defaulted "" for a field the server stopped sending. The CLI doesn't round-trip (update commands build requests from flags; PATCH types are separate and all-optional), and exposure is bounded by the daily drift job.

The sweep is invisible to the analyzer's drift comparison, so drift reports are unaffected.

Acknowledged tradeoff

Sentinel masking: a dropped required response field renders as ""/0/false rather than erroring. This is the intended failure mode for a CLI; per-field exceptions where a sentinel would mislead (e.g. cost figures) use Option<T> + optionality_exemptions.

🤖 Generated with Claude Code