You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#312 adopted tolerant response deserialization by sweeping #[serde(default)] across every model field (implemented in #313). Review of that PR surfaced a structural flaw in the mechanism: #[serde(default)] on a required T field fabricates a value (""/0/false) that is indistinguishable from a genuine server-sent value.
Write-back hazard.clickhouse-cloud-api is published to crates.io, and some types flow in both directions — PostgresInstanceConfig is literally the same type in the postgres_instance_config_get response and the postgres_instance_config_post/patch body. A consumer doing get → tweak → post silently persists fabricated values for any field the server dropped. Adopt tolerant-response deserialization: sweep serde(default) and codify the policy #313 documents this as the "GET→write-back caveat" rather than closing it.
Semantic loss. "Server sent 0" and "server dropped the field" become the same value — in --json output, in print_human, and in any consumer logic.
The goal of #312 stands unchanged: a server-dropped response field must not fail the response — multiple teams evolve the Cloud API independently, and each strict response field is a latent outage point. Only the mechanism changes.
Decision
Split request and response model types, and express response tolerance in the type system instead of via serde attributes:
Request types: unchanged. Required fields stay T (the type system enforces "strict in what we send"), optional/nullable fields stay Option<T> + skip_serializing_if. No #[serde(default)] anywhere.
Response types: every field is Option<T>. A missing key deserializes to None natively (no serde attribute needed), and — unlike serde(default), which only fills a missing key — an explicit JSON null also lands as None, closing a residual failure mode Adopt tolerant-response deserialization: sweep serde(default) and codify the policy #313 explicitly left open (e.g. null on a Vec field). Nothing is fabricated; absence is explicit and each caller decides what absence means at the point of use.
Enums and unions: unchanged. String enums keep their Unknown(String) catch-alls; object unions stay on discriminated_union! with lossless Unknown(Value).
Current shape of the code (measured, as of the #311 branch)
The operation layer is already ~split: 50 request-body types vs 49 response types, and only 2 top-level types cross directions (ClickStackSource, PostgresInstanceConfig).
75 nested structs are transitively shared between the request tree and the response tree. These are the types that need splitting into a request variant (strict) and a response variant (all-Option). Concentrated in ClickStack (chart configs, sources, dashboard containers, on-click targets), ClickPipes (pipe settings, table mappings, destination columns, scaling), and Postgres (PostgresInstanceConfig, PgConfig, PgBouncerConfig), plus a few shared leaves (IpAccessListEntry, ResourceTagsV1, RBACPolicyTags, CustomPrivateDnsMapping).
The full response tree is ~330 types — the all-Option conversion touches all of them, not just the 75 shared ones.
The spec itself already splits in places, so there is precedent to follow: ClickPipes ClickPipePost*/ClickPipePatch*/ClickPipeMutate* vs the response shapes, and ClickStack *Input vs *Output/*Response.
(Shared enums — ~94 of them — do not need splitting: value enums with Unknown catch-alls are direction-safe as-is.)
What needs to be done
Models (models.rs)
Split the 2 top-level bidirectional types and the 75 shared nested structs into request/response variants. Pick one naming convention up front and apply it uniformly (the spec's own precedents are *Input/*Output and Mutate*; whatever is chosen must be encodable in the analyzer's mapping, see below).
Convert every field of every response-tree type (~330 types) to Option<T>. Remove all #[serde(default)] (it becomes meaningless on Option fields and is banned on request fields by the write-back argument).
Response types must keep Serialize — the CLI's --json output and print_human both serialize response models. Decide deliberately whether absent fields serialize as null or are omitted (skip_serializing_if); omission is probably right so --json output distinguishes "absent" without inventing null noise, but it should be an explicit decision with a test pinning it.
Consider From<FooResponse> for FooRequest-style conversion helpers (or TryFrom where required request fields may be absent in the response) for consumers who legitimately do get → modify → write. The point of the split is that this conversion is explicit — absent fields must be consciously resolved — not that it's impossible.
Analyzer (clickhouse-openapi-analyzer)
Requiredness resolution becomes direction-aware: for request-position schemas, required[]/PATCH-all-optional/description-heuristic rules apply as today; for response-position schemas, every field is expected to be Option<T> and optionality findings are suppressed (they'd otherwise fire on every response field). Field presence checks (missing/extra fields) remain the drift signal in both directions and must keep working.
A spec schema used in both positions now maps to two Rust types. The schema↔type mapping in compare.rs/rust_inventory.rs must resolve both, each checked under its direction's rule. The naming convention chosen for the split is what makes this mechanical — encode it, don't hand-map.
Exemption keys (optionality_exemptions, extra_field_exemptions, deprecated_field_exemptions) are keyed by RustStructName — audit existing entries for structs being split and re-key to the correct variant(s). Same for partial_required_schemas semantics (now request-side only).
Replace Adopt tolerant-response deserialization: sweep serde(default) and codify the policy #313's every_model_field_carries_serde_default test with the equivalent structural enforcement for the new policy: every field of every response-reachable type is Option<T>, empty exception list. This likely needs the analyzer to expose request/response tree membership (it already inventories client method signatures, which define the roots).
Deprecated-field hiding and meta
meta.rs::DEPRECATED_FIELDS and #[cfg(feature = "deprecated-fields")] markers are keyed per struct — split structs double the bookkeeping. Regenerate with scripts/regenerate-deprecated-fields.py and verify both feature configurations compile (cargo check --workspace --all-features).
CLI (crates/clickhousectl)
Every read of a response field must handle None: print_human paths, tabled list views, and any logic branching on response values. Display code already handles Option widely (~69 sites in cloud/commands.rs), so this is mechanical — render absence as -/empty, never unwrap()/expect() on a response field.
postgres.rs constructs PostgresInstanceConfig from user input for config set — it must move to the request variant.
Tests and docs
models_test.rs round-trips, client_test.rs, integration tests, and CLI parsing/builder tests all touch response field access — expect broad mechanical churn. Add targeted tests: missing-key → None, explicit-null → None, and the serialization (null vs omit) decision.
Cherry-pick, don't rewrite — these are needed under this approach too, and all-Option makes them more necessary: an all-Option variant matches any JSON object, so untagged shape-matching degenerates completely and explicit discriminator dispatch is the only thing that works.
125ad7a — ClickStackAlertChannel → discriminated_union! dispatch on type.
d57839e — six chart-config Builder/RawSql unions dispatched on configType presence, including the none unless disqualifying-key guard.
The union-semantics parts of 6d1eb1b — malformed-payload-for-known-discriminator → lossless Unknown(Value) fallback, and the Default-round-trips-to-same-variant invariant test. This commit mixes those fixes with sweep-dependent changes, so it needs splitting rather than a clean cherry-pick.
The discriminated_union! macro itself lives in #311 and merges independently.
Keep in mind
What this still does not cover: a plain struct field whose type changes server-side (key present, wrong shape) still fails deserialization — Option<T> doesn't absorb that any more than serde(default) did. Unions absorb it via the Unknown fallback; enums via their catch-alls. Document this residual honestly, as Adopt tolerant-response deserialization: sweep serde(default) and codify the policy #313 did.
Discriminator strictness survives the sweep by construction:discriminated_union! dispatches on the raw JSON value, not on struct fields, so all-Option variant structs don't weaken dispatch. The Adopt tolerant-response deserialization: sweep serde(default) and codify the policy #313 adversarial-review findings (RawSql-without-configType misroute, Default round-trip mismatch) remain the reference for the failure modes to test.
Response-side optionality drift becomes invisible to the analyzer by design (everything is Option). That signal was low-value; presence/absence drift is what matters and is retained. Say so in the analyzer docs rather than leaving it as an apparent gap.
VALUES consts, beta lists, and enum coverage checking are unaffected.
Sequencing and commit slicing are deliberately out of scope here — to be planned by whoever picks this up.
Problem
#312 adopted tolerant response deserialization by sweeping
#[serde(default)]across every model field (implemented in #313). Review of that PR surfaced a structural flaw in the mechanism:#[serde(default)]on a requiredTfield fabricates a value (""/0/false) that is indistinguishable from a genuine server-sent value.clickhouse-cloud-apiis published to crates.io, and some types flow in both directions —PostgresInstanceConfigis literally the same type in thepostgres_instance_config_getresponse and thepostgres_instance_config_post/patchbody. A consumer doing get → tweak → post silently persists fabricated values for any field the server dropped. Adopt tolerant-response deserialization: sweep serde(default) and codify the policy #313 documents this as the "GET→write-back caveat" rather than closing it.0" and "server dropped the field" become the same value — in--jsonoutput, inprint_human, and in any consumer logic.The goal of #312 stands unchanged: a server-dropped response field must not fail the response — multiple teams evolve the Cloud API independently, and each strict response field is a latent outage point. Only the mechanism changes.
Decision
Split request and response model types, and express response tolerance in the type system instead of via serde attributes:
T(the type system enforces "strict in what we send"), optional/nullable fields stayOption<T>+skip_serializing_if. No#[serde(default)]anywhere.Option<T>. A missing key deserializes toNonenatively (no serde attribute needed), and — unlikeserde(default), which only fills a missing key — an explicit JSONnullalso lands asNone, closing a residual failure mode Adopt tolerant-response deserialization: sweep serde(default) and codify the policy #313 explicitly left open (e.g.nullon aVecfield). Nothing is fabricated; absence is explicit and each caller decides what absence means at the point of use.Unknown(String)catch-alls; object unions stay ondiscriminated_union!with losslessUnknown(Value).Current shape of the code (measured, as of the #311 branch)
ClickStackSource,PostgresInstanceConfig).Option). Concentrated in ClickStack (chart configs, sources, dashboard containers, on-click targets), ClickPipes (pipe settings, table mappings, destination columns, scaling), and Postgres (PostgresInstanceConfig,PgConfig,PgBouncerConfig), plus a few shared leaves (IpAccessListEntry,ResourceTagsV1,RBACPolicyTags,CustomPrivateDnsMapping).Optionconversion touches all of them, not just the 75 shared ones.ClickPipePost*/ClickPipePatch*/ClickPipeMutate*vs the response shapes, and ClickStack*Inputvs*Output/*Response.The 75 shared structs
ClickPipeBigQueryPipeSettings, ClickPipeBigQueryPipeTableMapping, ClickPipeDestinationColumn, ClickPipeDestinationTableDefinition, ClickPipeDestinationTableEngine, ClickPipeFieldMapping, ClickPipeKafkaOffset, ClickPipeMongoDBPipeSettings, ClickPipeMongoDBPipeTableMapping, ClickPipeMySQLPipeSettings, ClickPipeMySQLPipeTableMapping, ClickPipePostgresPipeSettings, ClickPipePostgresPipeTableMapping, ClickPipeScaling, ClickPipeSettings, ClickStackAggregatedColumn, ClickStackAlertChannelEmail, ClickStackAlertChannelWebhook, ClickStackBackgroundChart, ClickStackBarBuilderChartConfig, ClickStackBarRawSqlChartConfig, ClickStackBetweenColorCondition, ClickStackCASLPermission, ClickStackCategoricalBarBuilderChartConfig, ClickStackCategoricalBarRawSqlChartConfig, ClickStackDashboardContainer, ClickStackDashboardContainerTab, ClickStackEqualityColorCondition, ClickStackEventPatternsChartConfig, ClickStackFilter, ClickStackFilterSettingsColumn, ClickStackHeatmapChartConfig, ClickStackHeatmapSelectItem, ClickStackHighlightedAttributeExpression, ClickStackLineBuilderChartConfig, ClickStackLineRawSqlChartConfig, ClickStackLogSource, ClickStackLogSourceMetadataMaterializedViews, ClickStackMarkdownChartConfig, ClickStackMaterializedView, ClickStackMetricSource, ClickStackMetricSourceFrom, ClickStackMetricTables, ClickStackNumberBuilderChartConfig, ClickStackNumberFormat, ClickStackNumberRawSqlChartConfig, ClickStackNumericColorCondition, ClickStackOnClickDashboard, ClickStackOnClickExternal, ClickStackOnClickFilterTemplate, ClickStackOnClickSearch, ClickStackOnClickTargetIdVariant, ClickStackOnClickTargetTemplateVariant, ClickStackPieBuilderChartConfig, ClickStackPieRawSqlChartConfig, ClickStackPromqlSource, ClickStackQuerySetting, ClickStackSavedFilterValue, ClickStackSavedSearchFilter, ClickStackSearchChartConfig, ClickStackSelectItem, ClickStackSessionSource, ClickStackSourceFilterSettings, ClickStackSourceFrom, ClickStackTableBuilderChartConfig, ClickStackTableRawSqlChartConfig, ClickStackTraceSource, ClickStackTraceSourceMetadataMaterializedViews, CustomPrivateDnsMapping, IpAccessListEntry, PgBouncerConfig, PgConfig, PostgresInstanceConfig, RBACPolicyTags, ResourceTagsV1
(Shared enums — ~94 of them — do not need splitting: value enums with
Unknowncatch-alls are direction-safe as-is.)What needs to be done
Models (
models.rs)*Input/*OutputandMutate*; whatever is chosen must be encodable in the analyzer's mapping, see below).Option<T>. Remove all#[serde(default)](it becomes meaningless onOptionfields and is banned on request fields by the write-back argument).Serialize— the CLI's--jsonoutput andprint_humanboth serialize response models. Decide deliberately whether absent fields serialize asnullor are omitted (skip_serializing_if); omission is probably right so--jsonoutput distinguishes "absent" without inventingnullnoise, but it should be an explicit decision with a test pinning it.From<FooResponse> for FooRequest-style conversion helpers (orTryFromwhere required request fields may be absent in the response) for consumers who legitimately do get → modify → write. The point of the split is that this conversion is explicit — absent fields must be consciously resolved — not that it's impossible.Analyzer (
clickhouse-openapi-analyzer)required[]/PATCH-all-optional/description-heuristic rules apply as today; for response-position schemas, every field is expected to beOption<T>and optionality findings are suppressed (they'd otherwise fire on every response field). Field presence checks (missing/extra fields) remain the drift signal in both directions and must keep working.compare.rs/rust_inventory.rsmust resolve both, each checked under its direction's rule. The naming convention chosen for the split is what makes this mechanical — encode it, don't hand-map.optionality_exemptions,extra_field_exemptions,deprecated_field_exemptions) are keyed byRustStructName— audit existing entries for structs being split and re-key to the correct variant(s). Same forpartial_required_schemassemantics (now request-side only).serde(default)inventory tracking andmodel_fields_missing_serde_default()if that commit was carried; it enforces the superseded policy.Policy enforcement
every_model_field_carries_serde_defaulttest with the equivalent structural enforcement for the new policy: every field of every response-reachable type isOption<T>, empty exception list. This likely needs the analyzer to expose request/response tree membership (it already inventories client method signatures, which define the roots).Deprecated-field hiding and meta
meta.rs::DEPRECATED_FIELDSand#[cfg(feature = "deprecated-fields")]markers are keyed per struct — split structs double the bookkeeping. Regenerate withscripts/regenerate-deprecated-fields.pyand verify both feature configurations compile (cargo check --workspace --all-features).CLI (
crates/clickhousectl)None:print_humanpaths,tabledlist views, and any logic branching on response values. Display code already handlesOptionwidely (~69 sites incloud/commands.rs), so this is mechanical — render absence as-/empty, neverunwrap()/expect()on a response field.postgres.rsconstructsPostgresInstanceConfigfrom user input forconfig set— it must move to the request variant.Tests and docs
models_test.rsround-trips,client_test.rs, integration tests, and CLI parsing/builder tests all touch response field access — expect broad mechanical churn. Add targeted tests: missing-key →None, explicit-null→None, and the serialization (nullvs omit) decision.serde(default)policy and must not survive as the policy record, for the same reason this issue supersedes Adopt tolerant-response deserialization: sweep #[serde(default)] across model fields and codify the policy in CLAUDE.md #312.What carries over from #313 (being closed)
Cherry-pick, don't rewrite — these are needed under this approach too, and all-
Optionmakes them more necessary: an all-Optionvariant matches any JSON object, so untagged shape-matching degenerates completely and explicit discriminator dispatch is the only thing that works.125ad7a—ClickStackAlertChannel→discriminated_union!dispatch ontype.d57839e— six chart-config Builder/RawSql unions dispatched onconfigTypepresence, including thenone unlessdisqualifying-key guard.6d1eb1b— malformed-payload-for-known-discriminator → losslessUnknown(Value)fallback, and theDefault-round-trips-to-same-variant invariant test. This commit mixes those fixes with sweep-dependent changes, so it needs splitting rather than a clean cherry-pick.The
discriminated_union!macro itself lives in #311 and merges independently.Keep in mind
Option<T>doesn't absorb that any more thanserde(default)did. Unions absorb it via theUnknownfallback; enums via their catch-alls. Document this residual honestly, as Adopt tolerant-response deserialization: sweep serde(default) and codify the policy #313 did.discriminated_union!dispatches on the raw JSON value, not on struct fields, so all-Optionvariant structs don't weaken dispatch. The Adopt tolerant-response deserialization: sweep serde(default) and codify the policy #313 adversarial-review findings (RawSql-without-configTypemisroute,Defaultround-trip mismatch) remain the reference for the failure modes to test.Option). That signal was low-value; presence/absence drift is what matters and is retained. Say so in the analyzer docs rather than leaving it as an apparent gap.VALUESconsts, beta lists, and enum coverage checking are unaffected.Supersedes #312. Replaces the approach in #313.