Fix OpenAPI drift: ClickStack expansion, quotas, and model updates

[sdairs]

Closes #308

Remediates the OpenAPI drift between the live ClickHouse Cloud spec and clickhouse-cloud-api. The drift had grown since the issue was filed; this PR remediates today's live document (findings at remediation time: 25 missing client methods, 29 missing model types, 27 missing struct fields, 5 optionality mismatches, 5 missing enum values, 25 newly-beta operations).

What's added

• ClickStack sources: ClickStackPromqlSource (new union member), get/create/update/delete endpoints, and restructured source schemas — new section/disabled/knownColumnsListExpression/useTextIndexForImplicitColumn fields (the last is a string enum auto/enabled/disabled per the live spec, not a boolean).
• ClickStack connections, roles, saved searches: full models + CRUD endpoints (roles carry free-form CASL permissions with serde_json::Value conditions).
• ClickStack webhooks: ClickStackWebhookInput + create/update/delete (spec has no get-by-id); responses reuse the existing ClickStackWebhook union.
• Dashboard validation: click_stack_validate_dashboard + response models.
• Organization quotas: OrganizationQuota + list/get endpoints.
• Chart models: shared 13-value ClickStackChartColor palette enum, background charts, number-tile color conditions (numeric/between/equality union), categorical-bar and event-patterns tile configs wired into ClickStackTileConfig, ClickStackOnClickExternal wired into ClickStackOnClick.
• Existing models: alert numConsecutiveWindows, filter appliesToSourceIds, line fitYAxisToData, pie limit/orderBy, ClickPipes partitionByExpr (required on ClickPipePostgresPipeTableMapping, nullable on the patch-remove mapping), 4 new ActivityType values, alert state PENDING.
• Snapshot refreshed to the live document; BETA_OPERATIONS regenerated (+25 entries).

Breaking changes (library consumers, pre-1.0)

• Source structs' id is now Option<String> (spec: server-generated, no longer required).
• ClickStackTraceSource::default_table_select_expression is now required String.
• ClickStackSource, ClickStackTileConfig, and ClickStackOnClick untagged unions gained variants.
• ClickPipePostgresPipeTableMapping gained a required partition_by_expr: String field (#[serde(default)], so deserialization of old payloads is unaffected).

No CLI surface changes — ClickStack is not exposed in the CLI; exposure is a separate follow-up decision.

Notes for reviewers

• Union-dispatch ordering in ClickStackTileConfig is deliberate and commented: the new strict-displayType variants (bar, event_patterns) are placed where the existing catch-all builder variants can't swallow them. A pre-existing quirk (a plain displayType:"table" tile parses as ClickStackLineChartConfig) exists on main too and is out of scope here.
• Verified: cargo test --workspace, workspace clippy -D warnings, cargo fmt --all --check, python3 -m unittest discover -s scripts/tests, and python3 scripts/check-openapi-drift.py --dry-run → Actionable drift: 0. Library fully covers the live spec.

🤖 Generated with Claude Code

[sdairs]

bugbot review

[sdairs]

ClickStackTileConfig misdispatches legacy builder tiles

While reviewing this branch we confirmed a dispatch bug that predates this PR (reproduces on main)

Mechanism. The spec models tile configs as a oneOf with no discriminator, so ClickStackTileConfig is a #[serde(untagged)] enum — serde tries variants in declaration order and takes the first that parses. The five legacy builder variants (Line, Bar, Table, Number, Pie) share identical required fields (displayType, sourceId, select) and their displayType enums each carry an Unknown(String) catch-all (e.g. ClickStackTableBuilderChartConfigDisplaytype), so they accept any display type. Result: {"displayType": "table", ...} successfully parses as the first-listed variant — ClickStackLineChartConfig with display_type = Unknown("table") — and never reaches the Table variant. Verified empirically during review.

Impact. click_stack_get_dashboard/list_dashboards consumers matching on the tile variant get the wrong type for every table/bar/number/pie tile, and variant-specific fields are silently dropped. This is also the mechanism that was hiding the ClickStackOnClickExternal payload loss fixed in this PR.

Why it's visible now. This PR introduces the counter-convention: the new CategoricalBar and EventPatterns variants use strict single-value displayType enums (no catch-all), which is exactly why they dispatch correctly and why they had to be ordered ahead of the greedy legacy variants (see the comment on the union in models.rs). The file now has both conventions side by side.

Proposed fix

1. Make the five legacy builder displayType enums strict (drop their Unknown(String) catch-alls) so untagged dispatch becomes value-driven and order-independent.
2. As part of the same change, replace the union's Unknown(String) catch-all (which only matches JSON strings) with an object-tolerant fallback such as Unknown(serde_json::Value) — otherwise a genuinely new display type would go from "mis-parses as Line" to "hard deserialize error on the whole dashboard".
3. Add dispatch tests per display type through the union, mirroring the ones this PR added for bar/event_patterns/stacked_bar.

Trade-off to weigh: the per-enum catch-alls exist for forward compatibility, but the drift analyzer already flags new enum values, and step 2 preserves graceful degradation at the union level.

🤖 Generated with Claude Code

[cursor]

✅ 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 0709f62. Configure here.}

[Copilot]

Pull request overview

Remediates ClickHouse Cloud OpenAPI drift in the clickhouse-cloud-api crate by adding newly missing ClickStack + organization quota endpoints, expanding/adjusting ClickStack models/enums, and extending unit/integration-style tests to lock in the updated wire shapes.

Changes:

• Added new Client methods for ClickStack (sources, connections, roles, saved searches, webhooks, dashboard validation) and organization quota endpoints.
• Expanded/adjusted model types and enums to match the live spec (new fields, new enum values, and new ClickStack union variants).
• Added/extended tests validating serialization/deserialization and client request/response shapes for the new endpoints/models.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
crates/clickhouse-cloud-api/src/client.rs	Adds new API operations (ClickStack CRUD + dashboard validation; organization quotas).
crates/clickhouse-cloud-api/src/models.rs	Updates/extends models and enums for ClickStack + quotas; adds new union members and supporting types.
crates/clickhouse-cloud-api/src/meta.rs	Updates BETA_OPERATIONS list to include newly beta-classified endpoints.
crates/clickhouse-cloud-api/tests/client_test.rs	Adds wiremock-backed tests for the new client methods/endpoints.
crates/clickhouse-cloud-api/tests/models_test.rs	Adds serialization/deserialization regression tests for new/changed models and union dispatch ordering.

Comments suppressed due to low confidence (6)

crates/clickhouse-cloud-api/src/models.rs:7926

• After changing Unknown to serde_json::Value, the Display impl should format that value (otherwise this won’t compile).

            Self::ClickStackCategoricalBarRawSqlChartConfig(_) => {
                write!(f, "ClickStackCategoricalBarRawSqlChartConfig")
            }
            Self::Unknown(s) => write!(f, "{s}"),
        }

crates/clickhouse-cloud-api/src/models.rs:8028

• After changing Unknown to serde_json::Value, update the Display match arm accordingly.

            Self::ClickStackEqualityColorCondition(_) => {
                write!(f, "ClickStackEqualityColorCondition")
            }
            Self::Unknown(s) => write!(f, "{s}"),
        }

crates/clickhouse-cloud-api/src/models.rs:8134

• After changing ClickStackSource::Unknown to serde_json::Value, update the Display impl match arm to print the JSON value.

            Self::ClickStackSessionSource(_) => write!(f, "ClickStackSessionSource"),
            Self::ClickStackPromqlSource(_) => write!(f, "ClickStackPromqlSource"),
            Self::Unknown(s) => write!(f, "{s}"),
        }

crates/clickhouse-cloud-api/src/models.rs:8186

• ClickStackTileConfig is an untagged enum over object variants, but Unknown(String) only matches JSON strings. If the API adds a new tile config object, deserialization will error rather than landing in Unknown. Use Unknown(serde_json::Value) to make the catch-all effective.

This issue also appears on line 8204 of the same file.

    ClickStackEventPatternsChartConfig(ClickStackEventPatternsChartConfig),
    ClickStackMarkdownChartConfig(ClickStackMarkdownChartConfig),
    /// Catch-all for unknown or newly-added values.
    Unknown(String),
}

crates/clickhouse-cloud-api/src/models.rs:8206

• After changing ClickStackTileConfig::Unknown to serde_json::Value, update the Display impl match arm accordingly.

            Self::ClickStackMarkdownChartConfig(_) => write!(f, "ClickStackMarkdownChartConfig"),
            Self::Unknown(s) => write!(f, "{s}"),
        }

crates/clickhouse-cloud-api/src/models.rs:8057

• If ClickStackOnClick::Unknown becomes serde_json::Value, the Default + Display impls should be updated accordingly (they currently assume String).

            Self::ClickStackOnClickDashboard(_) => write!(f, "ClickStackOnClickDashboard"),
            Self::ClickStackOnClickExternal(_) => write!(f, "ClickStackOnClickExternal"),
            Self::Unknown(s) => write!(f, "{s}"),
        }
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sdairs]

Two valid-response deserialization bugs remain in the untagged ClickStack unions. The current tests pass because the trace test bypasses ClickStackSource, while the webhook endpoint tests exercise only the first (Slack) union arm.

[sdairs]

Re: the tile-config follow-up above — this is now fixed in this PR (3cea4c3) rather than deferred.

Instead of strictening the legacy displayType enums (step 1 of the proposal), ClickStackTileConfig deserialization is dispatched explicitly on displayType by a hand-written Deserialize keyed on the spec's single-value enums (line, stacked_bar, bar, table, number, pie, heatmap, search, event_patterns, markdown) — the same BackupBucket-style pattern now used for ClickStackSource, ClickStackWebhook, ClickStackOnClick, and ClickStackNumberTileColorCondition. This makes dispatch value-driven and order-independent, and step 2 of the proposal (object-tolerant fallback) is implemented as Unknown(serde_json::Value) on the union and its Builder/RawSql sub-unions, so a genuinely new display type degrades gracefully and round-trips. The two counter-convention strict enums this branch introduced were restored to the standard catch-all form, so the file has a single convention again. Dispatch tests through the union now cover all 10 display types.

One residual instance of the same defect class was confirmed during a final audit and is deliberately not fixed here: ClickStackDashboardChartSeries captures valid type:"table" payloads as the first-listed Time variant (identical required fields, catch-all type enums) and drops the table-only sortOrder on round-trip. The remaining unions (ClickStackAlertChannel, ClickStackOnClickTarget, the Builder/RawSql sub-unions) were audited and dispatch correctly for spec-valid payloads via disjoint required fields.

🤖 Generated with Claude Code

[sdairs]

Review follow-up: 4 items to address in this PR

Deep review of the remediation commits (1842d75, 001cd95, 3cea4c3, 8874b03) against the long-term model conventions. The discriminator-dispatch pattern is correct and well-tested; the four items below should land here rather than as follow-ups.

1. BackupBucket* catch-all is now the inconsistent (and lossy) one

The four pre-existing unions (BackupBucket, BackupBucketPatchRequest, BackupBucketPostRequest, BackupBucketProperties — models.rs:7682–7835) still use Unknown(value.to_string()), which stringifies the JSON object and then re-serializes as a JSON string on round-trip — so Unknown is lossy in a way the new ClickStack unions are not. The file now has two catch-all conventions side by side: Unknown(serde_json::Value) (round-trips faithfully) on the 5 ClickStack unions and Unknown(String) (does not) on the 4 BackupBucket ones. Migrate the BackupBucket quartet to Unknown(serde_json::Value) + value (not value.to_string()) in the fallback arm, update the Display impls, and add round-trip tests mirroring the ClickStack unknown-variant tests.

2. ClickStackDashboardChartSeries — same defect class, fix here

ClickStackDashboardChartSeries (models.rs:7925) is still derived #[serde(untagged)] with ClickStackTimeChartSeries first and ClickStackTimeChartSeriesType::Unknown(String) catching any type, so type:"table" / type:"number" payloads greedily resolve to Time and drop variant-specific fields (e.g. sortOrder on the table series). This is the same defect class as the 5 unions already remediated and the pattern transfers directly: hand-write Deserialize keyed on type (time/table/number/search/markdown), change the catch-all to Unknown(serde_json::Value), and add per-variant dispatch tests through the union plus an unknown round-trip test. Per AGENTS.md this otherwise needs a tracking issue — easier to fix it now while the pattern is loaded.

3. Extract a macro_rules! discriminator-dispatch helper

There are now 9 hand-written Deserialize impls (4 BackupBucket + 5 ClickStack) that are structurally identical: serde_json::Value::deserialize → match value.get("discriminator").and_then(|v| v.as_str()) → serde_json::from_value(value).map(Variant).map_err(serde::de::Error::custom) per arm → Unknown(value) fallback. A macro_rules! (e.g. discriminated_union!) would collapse each impl to a one-liner listing discriminator→variant pairs and make the convention self-documenting. Adding it now, while the pattern is new and there are 9 instantiations to migrate, sets the long-term shape and stops the boilerplate from growing with every future ClickStack union.

4. Display for Unknown(serde_json::Value)

The new catch-alls print the raw Value via {s} (compact JSON) while every named variant prints its type name. This matches the existing BackupBucket convention (which prints the stringified JSON the same way), so it is not a regression — but as part of unifying the catch-all convention (item 1), decide deliberately whether Display on Unknown should emit the JSON payload, a stable token like unknown, or the discriminator value. Right now the behaviour is inherited by accident rather than chosen.

[sdairs]

Re: the 4-item follow-up — all four are addressed on this branch (d85bb9d..23c7e46), plus two more instances of the same defect class that a final review sweep caught.

1. BackupBucket catch-alls → Unknown(serde_json::Value) — d85bb9d

All four unions (BackupBucket, BackupBucketPatchRequest, BackupBucketPostRequest, BackupBucketProperties) now store the raw Value (fallback arm is Unknown(value), not value.to_string()), so unknown payloads round-trip as JSON objects. Each union gained an unknown-provider round-trip test mirroring the ClickStack ones, and the file is back to a single catch-all convention.

2. ClickStackDashboardChartSeries dispatch — abab0f5

Hand-written Deserialize keyed on type (time/table/number/search/markdown), catch-all changed to Unknown(serde_json::Value). Per-variant dispatch tests cover all five values — including the called-out regression: a type:"table" payload now lands in ClickStackTableChartSeries with sortOrder preserved (that test fails against the pre-fix code) — plus an unknown-type object round-trip test.

3. discriminated_union! macro — 5b53194

All 10 hand-written impls collapsed to one-liner invocations listing discriminator→variant pairs, including the multi-value arms on ClickStackNumberTileColorCondition (gt|gte|lt|lte, eq|neq). Two notes:

• The macro generates only the Deserialize impl; enum declarations and Display impls stay literal source so the syn-based analyzer keeps inventorying every union — spec_coverage_test passes unchanged.
• ClickStackWebhook actually discriminates on service (the comment listed the other four keys); verified from source and encoded accordingly.

Behaviour is byte-identical, including the error path: a known discriminator with an invalid body still errors rather than falling through to Unknown. No dispatch/round-trip test was modified.

4. Display on Unknown — decided in d85bb9d

Chosen deliberately: Display emits the raw compact-JSON payload. serde_json::Value's Display is byte-identical to the previous stringified-String output, so nothing user-visible changes, and the payload is the most useful thing to show for an unrecognized variant. The choice is now stated in the doc comment on every union catch-all, with a Display test.

Follow-up: two more lossy object unions — 23c7e46

A final sweep for the item-1 defect class found ClickStackAlertChannel and ClickStackOnClickTarget still on Unknown(String) — and these were worse than lossy: an unknown-shape object hard-fails to deserialize under derived untagged, because an object can't coerce to String, so there was zero forward-compatibility. Both now carry Unknown(serde_json::Value) (derived untagged kept — their variants are distinguishable by required fields, so no discriminator dispatch is needed), ClickStackOnClickTarget::default() updated to match, with unknown-shape round-trip tests proven to fail pre-fix.

Full gate green on every commit: cargo fmt --all --check, cargo test -p clickhouse-cloud-api -p clickhouse-openapi-analyzer, cargo clippy --workspace --all-targets -- -D warnings.

🤖 Generated with Claude Code

[sdairs]

Review of the remediation work (all threads + follow-ups)

Independently verified the follow-up commits (1842d75..23c7e46) against the review threads, the vendored OpenAPI snapshot, and an exhaustive sweep of every #[serde(untagged)] union in models.rs. Overall: the work is structural, not comment-appeasing — all 10 dispatch tables match the spec exactly (including the counterintuitive-but-correct "stacked_bar" → BarChartConfig / "bar" → CategoricalBarChartConfig mapping), zero residual instances of the misdispatch class remain across all 265 untagged usages, the 8 unions that kept derived Deserialize are genuinely disjoint on required non-discriminator fields, and every claimed regression test exists and asserts meaningfully through the union type. Gates green at HEAD.

One item was marked resolved without being fixed, plus some minors.

Should fix before merge

1. Bugbot's "Trace table expression omits deserialize" is unaddressed (models.rs:12142)

ClickStackTraceSource.default_table_select_expression still carries #[serde(default)] — a leftover from 31d2e71 when the field was converted from Option<String> to String. The spec lists defaultTableSelectExpression in required[] for both log and trace sources, and ClickStackLogSource correctly has no default, so the two structs are inconsistent. Confirmed empirically: a kind:"trace" payload omitting the field deserializes with "" while the equivalent log payload correctly errors. The test named deserialize_clickstack_trace_source_requires_default_table_select_expression doesn't test omission, and the analyzer can't catch this (it checks Option vs T, not stray default on required scalars).

Fix: drop , default on that line and add a negative test asserting omission is rejected. The thread got no reply, unlike every other one — looks swept up in bulk-resolution.

Minor (non-blocking)

2. Multi-value dispatch arms are only sampled. ClickStackNumberTileColorCondition dispatch tests cover gt and eq but not gte/lt/lte/neq — a typo in those literals in the macro invocation would silently route valid payloads to Unknown with no test failure. A small table-driven test over all seven operator values closes this cheaply.

3. Raw-SQL sub-variants under-tested. Only the categorical-bar sub-union exercises its RawSql variant through the union; line/stacked-bar/table/number/pie RawSql variants and five of six sub-union Unknown(Value) catch-alls are untested. Low risk (uniform derive-untagged shape), but the builder-vs-rawsql disambiguation is per-enum.

4. Doc-comment drift surface. Each dispatched union's doc comment duplicates the discriminator wire values that also live in the macro invocation directly below it. Naming just the key in the doc would remove the duplication.

5. Test organization. Dispatch tests are grouped by drift-issue era rather than by type (the heatmap tile test sits ~800 lines from the other nine tile tests), naming mixes deserialize_clickstack_* and clickstack_* prefixes for the same kind of test, and the 5-line unknown-round-trip idiom repeats ~8 times where a shared helper would do.

6. Consistency option. ClickStackOnClickTarget could move to discriminated_union! on "mode" ("id"/"template" are single-valued per variant); the derived form is correct today, so purely a convention choice. The Builder/RawSql pairs and ClickStackAlertChannel genuinely cannot use the macro (no single key present-and-distinct across variants), so their derived form is right as-is.

🤖 Generated with Claude Code

[cursor]

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit dcdb3aa. Configure here.}

[cursor]

Empty partitionByExpr on serialize

Medium Severity

ClickPipePostgresPipeTableMapping maps missing partitionByExpr to an empty string on deserialize but always serializes that field, so a mapping loaded from API JSON without partitionByExpr re-emits "partitionByExpr":"" on update instead of omitting it. That can change ClickPipe table configuration on get-modify-put flows.

 

^{Reviewed by Cursor Bugbot for commit dcdb3aa. Configure here.}

[rndD]

Any chance we split tests by domains?

[rndD]

and maybe it would be cool to split client methods by domain