Split request/response model types; tolerant responses via all-Option fields - #316
Split request/response model types; tolerant responses via all-Option fields#316sdairs wants to merge 26 commits into
Conversation
Convert the union from derived untagged Deserialize to explicit dispatch on
the `type` field ("email"/"webhook"), so unrecognized types land in Unknown
instead of being shape-matched. Serialization is unchanged (still untagged).
With hard dispatch in place, the variant structs' discriminating fields can
be tolerant: add serde(default) to ClickStackAlertChannelEmail's
emailRecipients/type and ClickStackAlertChannelWebhook's type/webhookId so a
server-dropped field degrades to a default rather than failing the response.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Give discriminated_union! an optional trailing `none` arm that routes an absent (or non-string) discriminator to a named variant instead of Unknown, and use it for the six ClickStack chart-config sub-unions: `configType: "sql"` selects the raw-SQL variant, key absence selects the builder variant. Without a `none` arm the expansion is unchanged. Serialization is untouched: the enums keep Serialize plus serde(untagged) and only drop the derived Deserialize. With hard dispatch in place the 12 variant structs' fields can be tolerant: add serde(default) to every strict field so a server-dropped field degrades to a default. That makes each builder variant total, so its arm always succeeds and the sub-union Unknown is now reachable only via an unrecognized string configType — pinned by new tests alongside the non-string and key-absent dispatch cases. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Carries the union-semantics fixes from #313's 6d1eb1b, which the all-Option response split makes more necessary rather than less: an all-Option variant struct matches any JSON object, so explicit discriminator dispatch is the only thing left doing real work. The discriminated_union! arms propagated the selected variant's error, so a recognized discriminator whose payload no longer fits (an array field that became a string) hard-failed the whole response where the untagged derive had degraded to Unknown. Arms now route through serde_helpers::deserialize_or_raw and fall back to the lossless Unknown(Value) catch-all. The `none` arm of the six chart-config unions became total once the cherry-pick of d57839e defaulted every builder field, so a raw-SQL chart config whose spec-required configType the server drops was silently retyped as an empty builder config, discarding connectionId/sqlTemplate. The arm is now `none unless "connectionId" | "sqlTemplate"`, which sends such a payload to Unknown intact. ClickStackAlertChannelEmailType defaulted to Webhook (the spec gives both alert-channel variants the same enum), so ClickStackAlertChannel::default() — reachable from a response that drops `channel` — deserialized back as the webhook variant and could be PUT back as a webhook channel with no webhookId. #[default] moves to Email, pinned both by a dedicated test and by a cross-union invariant test asserting every discriminated_union! enum's Default is a fixed point of its own Deserialize. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ue 314)
Classify every spec schema by usage position: request (reachable from a
request body or operation parameter) and/or response (reachable from an
operation response), via transitive closure over the schema $ref graph.
Requiredness/optionality findings now apply only in request position;
in response position every field is Option<T> by policy, so optionality
findings are suppressed by design while field-presence (missing/extra)
and enum-value checks keep working in both directions. Schemas reachable
from neither direction keep the historical strict request-side checks.
A schema used in both directions maps to two Rust types: {Name} for the
request variant and {Name}Response for the response variant when that
type exists, guarded against Rust types that model a spec schema
literally named {Name}Response; until the split lands, both directions
collapse onto {Name}, keeping the current unsplit models.rs green.
Deprecated-field expectations expand the same way, so a split variant
carrying a spec-deprecated field doubles the DEPRECATED_FIELDS
bookkeeping and is enforced per variant.
Expose response-tree membership as a public API for the later policy
enforcement test: response_tree(client_rs, models_rs) returns the model
types transitively reachable from Client method return types (through
struct fields, enum variant payloads, and type aliases) plus the
(type, wire field) pairs that are not yet Option<T>. Client method
return types were not previously inventoried; MethodInfo now records
every named type in the return position.
Config audit: no optionality_exemptions entry is removed — every entry
keys a request-position struct (ServicePostRequest, ClickPipe*, PgConfig
via the bidirectional pgConfig schema, etc.), so all remain live under
suppression. Both partial_required_schemas entries (Service,
ServiceScalingPatchResponse) sit in response position and are now inert;
they are retained with documentation that partial_required_schemas is
request-position-only semantics.
No report schema_version bump: no new FindingKind, no new report
fields, and no Python rendering changes — the direction rules only
narrow where existing finding kinds fire.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Every Postgres-family response type is now all-`Option<T>` with
`skip_serializing_if` on each field, so a field the API drops or sends as
`null` deserializes to `None` instead of failing the response, and absent
fields are omitted from `--json` output rather than emitted as `null`.
`postgresInstanceConfig`, `pgConfig` and `pgBouncerConfig` are used in both
directions, so each splits into a strict request variant keeping the schema's
name and a tolerant `{Name}Response` variant; `postgres_instance_config_get`
now returns `PostgresInstanceConfigResponse`. Writing a fetched configuration
back goes through the new `crate::convert` module, whose
`TryFrom<PostgresInstanceConfigResponse> for PostgresInstanceConfig` names the
missing required wire fields instead of fabricating empty objects. All
`#[serde(default)]` is gone from the Postgres slice, request side included:
`postgres config replace/patch --file` keeps accepting a document that omits a
section, but the handler now resolves the omission explicitly.
CLI reads of Postgres response fields render absence as `-` (list table,
detail view) and no longer match a filter on an absent field.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e 314)
Every response type in the services, organizations, keys, members,
invitations, roles, backups, scaling, upgrade-window, query-endpoint, usage
and ClickHouse-settings families is now all-`Option<T>` with
`skip_serializing_if` on each field, so a field the API drops or sends as
`null` deserializes to `None` instead of failing the response, and absent
fields are omitted from `--json` output rather than emitted as `null`. All
`#[serde(default)]` is gone from these families, request side included.
`ipAccessListEntry` and `resourceTagsV1` are sent as well as returned, so each
splits into a strict request variant keeping the schema's name and a tolerant
`{Name}Response` variant; `Service`, `ServiceScalingPatchResponse`, `ApiKey`
and the Postgres responses (via the new `PgTagsResponse` alias) now reference
the response variants. Writing a fetched value back goes through
`crate::convert`, which gains `TryFrom` conversions for tags, scaling-schedule
entries and upgrade windows that name the missing required wire fields instead
of fabricating values.
`BackupBucket::default()` now names its variant's own `bucketProvider`, because
an all-`Option` variant would otherwise serialize to `{}` and dispatch back
through `discriminated_union!` as `Unknown`.
CLI reads of these response fields render absence as `-` through the shared
`ABSENT`/`or_absent` helpers (moved to `cloud::output` and reused by the
Postgres slice), no longer `unwrap()` a response field, and treat an absent
field as non-matching when filtering. Query-endpoint provisioning fails loudly
when the API omits a credential rather than persisting an empty key pair.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The last non-ClickPipes, non-ClickStack response types are now all-`Option<T>` with `skip_serializing_if` on every field: `OrganizationQuota` and `ReversePrivateEndpoint` no longer fail a response when the API drops a field or sends it as `null`, and absent fields are omitted from `--json` output rather than emitted as `null`. The `ApiResponse` envelope omits an absent `result` for the same reason. `customPrivateDnsMapping` and `RBACPolicyTags` are sent as well as returned, so each splits: the strict request variant keeps the schema's name for `CreateReversePrivateEndpoint`/`UpdateReversePrivateEndpoint` and `RBACPolicyCreateRequest`, while `ReversePrivateEndpoint` and `RBACPolicy` point at the new tolerant `CustomPrivateDnsMappingResponse` and `RBACPolicyTagsResponse`. The `BackupBucket` unions needed no split: the `AwsBackupBucket`/`GcpBackupBucket`/`AzureBackupBucket` variants are response-only, and the request unions have their own `*PostRequestV1`/ `*PatchRequestV1` variant structs. `#[serde(default)]` is gone from every remaining infrastructure and miscellaneous model — reverse private endpoints, quotas, RBAC policy tags, the `ApiResponse` envelope, the SCIM family, and the unreferenced `License`, `AzureEventHub`, `MutualTLS`, `PLAIN`, `MskIamUser` and `ServiceAccount` schemas. Those last ones and the SCIM types are request-position (orphan or request-only), so they stay strict: a payload missing a required field is now rejected instead of silently defaulted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The SCIM family needs no request/response split: the spec defines 40 `Scim*` schemas but declares no SCIM path, so no `Client` method sends or returns one and the family is reachable from neither a request root nor a response root. Verified empirically that the analyzer resolves such operation-unreferenced schemas in request position — making a SCIM list/response envelope all-`Option` reports `FieldOptionalityMismatch` drift while protecting no actual response — so the family stays strict. Record that determination structurally instead of by assumption: a new `scim_models_are_outside_the_response_tree` test asserts SCIM stays out of the analyzer's response tree, so adding a SCIM operation to `client.rs` forces the split first, and `scim_request_models_stay_strict` pins the request-side behaviour. `OrganizationQuota` (the slice's only response-tree type) is already all-`Option`; add the missing explicit-`null` and omit-on-serialize coverage for it. No CLI change: neither SCIM nor quotas are exposed in `clickhousectl`. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(issue 314)
The ClickPipes response tree is now tolerant by type: every field of every
ClickPipe response struct is `Option<T>` with `skip_serializing_if`, so a key
the API drops — or sends as an explicit `null` — deserializes to `None`
instead of failing the response, and absence stays absent in `--json` output.
The spec's own top-level split (`ClickPipePost*`/`Patch*`/`Mutate*` vs the
response shapes) is preserved. The 15 nested schemas used in both directions
split into a strict request variant that keeps the schema's name and an
all-`Option` `{Name}Response`: the pipe settings and table mappings for
BigQuery/MongoDB/MySQL/Postgres, the destination column/table
definition/table engine shapes, the field mapping, the Kafka offset, plus
`ClickPipeSettings` and `ClickPipeScaling`. `click_pipe_settings_get`/`_update`
now return `ClickPipeSettingsResponse`.
`ClickPipeScalingResponse.concurrency` carries the spec's deprecation marker,
so `DEPRECATED_FIELDS` gains the response-variant entry; the regeneration
script derives names from the spec alone and both it and `meta.rs` now say so.
`#[serde(default)]` is gone from the whole family, including the request-only
structs — no request wire shape changes, because `default` only ever affected
deserialization. `serde_helpers::null_to_empty` goes with it: `Option<Vec<T>>`
absorbs `null` natively on the response side, and on a request struct the
helper only ever worked in combination with the `default` this policy bans.
CLI reads of pipe response fields go through the shared `or_absent`, and the
ClickPipes integration suites read ids and states through two new support
helpers rather than unwrapping.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e 314)
ClickStack sources are the last bidirectional top-level type: the same
schema is a create/update body and a GET response. Split it, and every
type in its tree, so response tolerance lives in the type system.
- `ClickStackSourceResponse` joins `ClickStackSource`, over all-`Option`
`*Response` variant structs, with `kind` dispatch and the lossless
`Unknown(Value)` catch-all preserved. It deliberately has no `Default`:
an all-`Option` variant's default serializes to `{}`, which carries no
discriminator and would not round-trip to the same variant.
- 20 nested structs split (sources, `SourceFrom`, `MetricTables`,
materialized views, filter settings, query settings, `ClickStackFilter`,
saved-search filters, `CASLPermission`); connections, roles and saved
searches are response-only and become all-`Option` in place.
- `TryFrom<ClickStackSourceResponse> for ClickStackSource` in `convert.rs`,
with the nested conversions it needs, so a get/edit/write-back names the
fields the response omitted instead of fabricating them. An unknown
`kind` passes through the request union's `Unknown` arm verbatim.
- `#[serde(default)]` is gone from every struct in this slice; the
remaining occurrences are alerts, dashboards, charts and webhooks.
ClickStack is not exposed in the CLI, so there are no CLI changes.
Analyzer `response_tree()` non-`Option` fields: 235 -> 149.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…response variants (issue 314)
Split the 33 bidirectional dashboard-tree structs (chart configs, select
items, number formats, color conditions, on-click targets, dashboard
containers, alert channels) into strict request types and all-Option
{Name}Response types, and duplicate their 11 discriminated unions as
{Name}Response unions with identical dispatch semantics: configType
presence still selects raw-SQL vs builder, the `none unless
"connectionId" | "sqlTemplate"` guard still keeps a dropped
discriminator from silently retyping a raw-SQL payload as the now-total
builder variant, and a known discriminator over a payload that no
longer fits falls to the lossless Unknown(Value). The response unions
deliberately have no Default; ClickStackWebhook (response-only, so not
split) keeps its Default by naming its own `service` wire value now
that its variants are all-Option.
Response-only models (alerts, dashboards, tiles, validation results,
webhooks) become all-Option in place, and the last `#[serde(default)]`
attributes leave models.rs: request bodies are strict again and
response tolerance lives entirely in the type system. The analyzer's
response tree now reports zero non-Option fields.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add three policy tests to spec_coverage_test.rs, all backed by the shared analyzer so no source parsing leaks into the library crate: - every_response_tree_field_is_option: every field of every response-reachable type is Option<T>, with an empty exception list and a vacuity guard on the tree size. - every_response_tree_option_field_omits_none_when_serialized: pins the serialization decision — absent response fields are omitted from JSON output, never emitted as null. The analyzer's ResponseTree now reports Option fields lacking skip_serializing_if to back this. - models_carry_no_serde_default: zero #[serde(default)] anywhere in models.rs. The inventory records the attribute in bare, `default = path` and container-level forms (a container default marks every field, so the ban cannot be dodged by moving the attribute up). Close the one serialization gap earlier slices left: ClickStackValidateDashboardResponse.normalized now carries skip_serializing_if, and its test asserts explicit null deserializes to None and is omitted on the way out instead of pinning the old null echo. Deprecated-field bookkeeping needed no change: regenerating from the spec reproduces meta.rs exactly, ClickPipeScaling is the only split schema with a deprecated field, and both its variants already carry the entry and the cfg marker; cargo check --workspace --all-features is green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The policy record still argued for the superseded serde(default) sweep:
AGENTS.md told contributors to "retain #[serde(default)] on model fields"
and pointed at an enforcement test that no longer exists. Replace it with
the split-type policy the code now implements.
AGENTS.md gains a "Request and response models" section covering the
strict-request/all-Option-response split, the {Name}Response naming
convention that makes the analyzer's dual mapping mechanical, union and
enum catch-alls being unchanged by the split, the deliberate
skip_serializing_if omission decision, convert.rs write-back conversions
replacing the old GET-write-back caveat, the honest residual (a present
key whose type changed still fails a plain struct field), and the four
spec_coverage_test.rs tests that pin it. Drift remediation, deprecated
field hiding, and the cloud-command checklist now reflect the convention;
the direction-aware optionality text says why response-side optionality
drift is invisible by design.
Crate docs get the same policy for published-crate consumers, with a
compiled example of an explicit write-back through TryFrom, and README
documents that an absent field renders as "-" and is omitted from --json.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The CLI's DeleteResponse was the one remaining --json path that fabricated
values the server did not send ("status": 0.0, "requestId": ""); its fields
are now Option + skip_serializing_if like every library response type, and
the six CloudClient delete wrappers pass the library fields through.
first_endpoint dropped a partially-present endpoint (host without port
rendered as "-"); it now keeps whichever half the API sent.
The Default round-trip invariant's covered-union list was enforced only by a
doc-comment convention. The analyzer now exposes
model_types_with_manual_default_impl, and the test requires its covered list
to equal the set of hand-written `impl Default for` blocks in models.rs, so
a new union gaining a Default cannot silently escape the invariant.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
|
bugbot review |
There was a problem hiding this comment.
Pull request overview
This PR implements the “split request/response model types” policy across clickhouse-cloud-api and updates both the OpenAPI drift analyzer and clickhousectl to treat API response fields as tolerant (Option<T>) without fabricating defaults. This aligns the library with the goal of preventing deserialization outages when the Cloud API drops fields or returns null, while keeping request bodies strict.
Changes:
- Introduces direction-aware drift checking and a computed Rust “response tree” to enforce that all response-reachable fields are
Option<T>and omitNoneduring serialization. - Updates
clickhousectlcloud commands to avoidunwrap/fabrication on response fields and to render absent values consistently (-) in human/table outputs while omitting absent keys in--json. - Updates integration/unit tests and metadata tooling/docs to reflect the request/response split and deprecated-field bookkeeping for
*Responsevariants.
Reviewed changes
Copilot reviewed 30 out of 32 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/regenerate-deprecated-fields.py | Documents manual follow-up needed for {Name}Response deprecated-field entries after regeneration. |
| README.md | Adds user-facing documentation explaining how absent API fields are rendered and serialized. |
| crates/clickhousectl/src/cloud/types.rs | Makes delete payload fields optional and omits absent fields from JSON serialization. |
| crates/clickhousectl/src/cloud/types_test.rs | Updates serialization/deserialization tests for optional delete fields and Option-based response enums. |
| crates/clickhousectl/src/cloud/service_query.rs | Adds explicit “required field” extraction to avoid persisting unusable credentials when responses omit fields. |
| crates/clickhousectl/src/cloud/postgres.rs | Adapts Postgres handlers/output/filtering to Option-based response models; adds strict request-body construction helper. |
| crates/clickhousectl/src/cloud/output.rs | Adds ABSENT/or_absent helpers to standardize rendering of missing response fields. |
| crates/clickhousectl/src/cloud/commands.rs | Updates multiple cloud commands to print/format Option response fields safely; adds endpoint/list helpers. |
| crates/clickhousectl/src/cloud/client.rs | Stops fabricating delete-response fields; updates method return types to response variants where split. |
| crates/clickhouse-openapi-analyzer/src/rust_inventory.rs | Enhances Rust inventory with type-name reachability, serde attribute tracking, and response-tree reachability walking. |
| crates/clickhouse-openapi-analyzer/src/openapi.rs | Classifies schemas into request/response positions via transitive closure; tracks rust schema names for split mapping safety. |
| crates/clickhouse-openapi-analyzer/src/lib.rs | Exposes response_tree() and policy helpers (serde default detection, manual Default impl listing). |
| crates/clickhouse-openapi-analyzer/src/config.rs | Updates config semantics/docs for request-position-only optionality/requiredness logic. |
| crates/clickhouse-openapi-analyzer/src/compare.rs | Implements direction-aware field checks; maps response position to {Name}Response where applicable; doubles deprecated-field expectations. |
| crates/clickhouse-cloud-api/tests/spec_coverage_test.rs | Adds enforced tests for all-Option response tree, omit-None serialization, and banning #[serde(default)]. |
| crates/clickhouse-cloud-api/tests/integration_test.rs | Updates ClickHouse lifecycle integration to Option response fields with explicit required-field extraction. |
| crates/clickhouse-cloud-api/tests/integration_postgres_test.rs | Updates Postgres lifecycle integration for Option response fields and response tag handling. |
| crates/clickhouse-cloud-api/tests/integration_org_test.rs | Updates org lifecycle integration to avoid unwraps and to use Option-safe helpers. |
| crates/clickhouse-cloud-api/tests/common/support.rs | Adds shared helpers (require_field, field_string, service_state, https_endpoint) for Option response handling. |
| crates/clickhouse-cloud-api/tests/client_test.rs | Updates mocked-client tests to assert against Option response fields. |
| crates/clickhouse-cloud-api/tests/clickpipes/support.rs | Adds ClickPipe-specific accessors for Option response fields used across clickpipes tests. |
| crates/clickhouse-cloud-api/tests/clickpipes/stages/mod.rs | Updates ClickPipe stage logic to tolerate Option state/id and avoid direct unwraps. |
| crates/clickhouse-cloud-api/tests/clickpipes/smoke_test.rs | Updates smoke test cleanup/logging to handle Option response id/state. |
| crates/clickhouse-cloud-api/tests/clickpipes/postgres_cdc_test.rs | Updates CDC E2E test to avoid relying on fields that may be omitted and to use Option-safe helpers. |
| crates/clickhouse-cloud-api/src/serde_helpers.rs | Replaces null-to-empty helper with deserialize_or_raw to preserve payloads when variant parsing fails. |
| crates/clickhouse-cloud-api/src/meta.rs | Documents and enforces deprecated-field duplication for split {Name}Response types; adds an example entry. |
| crates/clickhouse-cloud-api/src/lib.rs | Adds crate-level docs describing strict requests, tolerant responses, split naming, and explicit write-back conversions. |
| crates/clickhouse-cloud-api/src/client.rs | Updates client method return types to use *Response variants for response-position types. |
| AGENTS.md | Updates contributor guidance to reflect the new request/response split, response Option policy, and enforcement tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0a6cddf. Configure here.
…view) An omitted `tags` in a Postgres GET response is indistinguishable from a field the API dropped, and the PATCH replaces the tag set wholesale, so coercing absence to an empty set silently deleted every existing tag not named by --add-tag. Resolve absence explicitly instead: the new `merge_response_tags` helper refuses the merge with an error saying the response omitted the field. A genuine empty list still merges. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
`service delete --force` polled the stop with `or_absent(state)`, so an absent state matched neither the success nor the failure set and the timeout-less loop spun forever; `classify_stop_poll_state` now fails on an absent state instead. `key create` decided "no generated key material returned" from the response alone, so a generated-key request whose response omitted `keyId`/`keySecret` printed success plus a false explanation while the one-time secret was lost; `resolve_key_create_material` reads the mode from the request and fails, naming the key that may still exist. `service create` rendered its query hint with `--id -` when the response omitted the service id; the hint is now dropped, leaving the rest of the output (including the one-time password) intact. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A scalar, array or null root made every `doc.get(key)` lookup miss, so both
sections resolved to `{}` and the CLI sent an empty body that reset the
service configuration instead of rejecting the malformed file.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…316 review) `ensure_service_query_setup` only cleaned up the API key it created when the query-endpoint binding failed. An absent `keyId`/`keySecret` on the create response, or an absent `id` on the endpoint upsert response, returned straight out and left an orphaned key behind on every retry. Resolve the key's resource UUID first (the only absence with no cleanup available, since it names the key to delete), then route each subsequent failure through a best-effort delete. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Position classification only followed `#/components/schemas/...` refs, so an operation naming a reusable `#/components/responses/...`, `#/components/requestBodies/...` or `#/components/parameters/...` component left every schema behind it unclassified — defaulting it to request position and inviting spurious FieldOptionalityMismatch findings. The reference walk now resolves non-schema component refs against the spec document and continues through them, with a visited set so a component reference cycle terminates. The vendored snapshot uses no such components, so the drift report is unchanged; this is robustness against future spec drift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 33 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
crates/clickhousectl/src/cloud/types_test.rs:74
- These calls pass
Option<Enum>by value intoenum_wire_value, moving fields out ofact. After changingenum_wire_valueto acceptOption<&T>, pass.as_ref()here to avoid moves.
crates/clickhousectl/src/cloud/types_test.rs:96 - Same move issue as above: pass
.as_ref()intoenum_wire_valuesoactcan still be borrowed afterward.
crates/clickhousectl/src/cloud/types_test.rs:108 - Pass
.as_ref()intoenum_wire_valueto avoid movingact.r#type.
crates/clickhousectl/src/cloud/types_test.rs:174 - In this loop,
act.r#typeis moved intoenum_wire_value; pass.as_ref()to match the updated helper signature and avoid moves.
crates/clickhousectl/src/cloud/types_test.rs:185 - Pass
.as_ref()intoenum_wire_valueto avoid movingact.actor_type.
crates/clickhousectl/src/cloud/types_test.rs:212 - Pass
.as_ref()intoenum_wire_valueto avoid movingact.key_update_type.
|
Deep review of the full stack (19 commits, reviewed against Verdict: approve. The core design — tolerance in the type system rather than serde attributes — is strictly better than the #313 What impressed me most:
Minor findings, none blocking:
Things I specifically checked that are NOT problems: no fabrication anywhere on the response path ( The main risk — 88 hand-maintained split pairs — is inherent to the design and mitigated as well as it can be by the direction-aware analyzer. Ship it (after retargeting to |
|
Deep review of the current head ( Findings
VerificationI ran:
The worktree remained clean. The current Copilot comment about Verdict: request changes. The remaining issues can revoke existing credentials or lose one-time secrets under the exact partial-response conditions this PR is intended to make safe. |
… 316 review) A 200 `instance_query_endpoint_get` whose `result` or `openApiKeys` was absent was coerced to an empty list, and the upsert replaces the binding wholesale, so provisioning silently revoked every key already bound to the endpoint. Only a 404 now starts from an empty list; an incomplete 200 fails naming the omitted field, and the caller's existing cleanup discards the provisioned key. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…316 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…16 review) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…view) `generation_requested` treated a body carrying only `newDoubleSha1Hash` as a hash update, but the spec says that key "will be ignored and the generated password will be used" when `newPasswordHash` is absent, and that the response carries a password "only if there was no 'newPasswordHash' in the request". So `--new-double-sha1-hash` on its own rotates the password to a freshly generated one, and reporting "password hash updated; no plaintext password returned" discarded the only copy of it. Key the mode on `newPasswordHash` alone. Also stop telling a Postgres create to reset a password that is not lost: the spec says `connectionString` embeds the service password, so when the response omitted `password` but carried a connection string, the recovery advice would rotate a working credential. Point at the connection string instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…eview) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All five findings were valid and are now fixed, one commit each, with tests confirmed to fail against the pre-fix code:
Also in this push: 29beb7d narrows the Gates on the final head: |
Closes #314
Stacked on #311 — base is
issue-308-clickstack-drift; retarget tomainafter #311 merges. Replaces #313 (closed), whoseserde(default)sweep fabricated values indistinguishable from server-sent ones.What and why
A server-dropped or
nullresponse field must never fail deserialization — each strict response field is a latent outage point. Tolerance now lives in the type system instead of serde attributes:T, optional areOption<T>+skip_serializing_if. The compiler enforces "strict in what we send".Option. Every field of every type reachable from aClientreturn type (339 types) isOption<T>. Missing key and explicitnullboth land asNone— nothing is fabricated, so "server sent0" and "server dropped the field" stay distinguishable.Unknowncatch-alls absorb unrecognized values and (new in this PR) malformed payloads under a recognized discriminator.How
{Name}(request) +{Name}Response— 88 pairs, more than the 75+2 the issue measured (agents verified reachability fromclient.rsrather than trusting the list). Unions whose variant structs split are duplicated (discriminated_union!invocation per direction).--json/print_humanoutput, never emitted asnull(skip_serializing_ifon every response field, test-enforced).convert.rswithTryFrom<{Name}Response>/Fromconversions (top-level:PostgresInstanceConfig,ClickStackSource); a fallible conversion returnsMissingRequiredFieldsnaming the missing wire fields. This closes Adopt tolerant-response deserialization: sweep serde(default) and codify the policy #313's GET→write-back caveat instead of documenting it.serde(default)is banned inmodels.rs, enforced by test via the analyzer.{Name}Responsewhen it exists. Newresponse_tree()API backs the enforcement tests.none unless "connectionId" | "sqlTemplate"guards on the chart-config unions (mandatory now — all-Optionvariants are total, so a dropped discriminator would otherwise silently retype a RawSql payload); known-discriminator malformed payloads degrade to losslessUnknown(Value); every unionDefaultround-trips to the same variant, with the covered list structurally required to equal the manualimpl Defaultblocks.unwrap/expecton response fields; absence renders as-(or_absent);postgres config setbuilds the request variant; delete--jsonno longer fabricatesstatus/requestId.Residual (documented, intentional)
A key that is present with a changed type still fails a plain struct field —
Option<T>absorbs absence andnull, not shape changes. Enums/unions absorb those via catch-alls. Spec conformance stays the drift job's responsibility.Breaking changes (library consumers, pre-1.0)
Option<T>; response-position types returned byClientmethods are the{Name}Responsevariants where split.discriminated_union!grammar: barenone =>arms no longer parse; absence-discriminated unions must name disqualifying keys.DeleteResponsefields areOption.Verification
Every commit is independently green (per-commit verifier agents re-ran all gates):
cargo test --workspace, clippy-D warningsworkspace-wide,cargo fmt --all --check,cargo check --workspace --all-features,python3 -m unittest discover -s scripts/tests, andpython3 scripts/check-openapi-drift.py --dry-run→ 0 actionable findings against the live spec. A fresh-context adversarial review of the full stack found no high/medium defects; its three low-severity findings are fixed in the final commit. Enforcement tests were mutation-tested (strictening a response field / removing a covered union fails the corresponding test with the exact offender named).🤖 Generated with Claude Code