Detect struct fields removed from the spec but left in models.rs (#237)

AGENTS.md

@@ -82,15 +82,24 @@ In `models.rs`, required non-nullable fields use bare types (`T`), optional/null
 **Tooling:**
 
 - `scripts/resolve-field-requirements.py` — resolves required/optional for every schema field, outputs a JSON manifest. Handles both conventions + PATCH + nullable.
-- `scripts/check-openapi-drift.py` — daily CI drift check; reports missing/extra methods, missing schemas/fields, and field-level optionality mismatches against the live spec.
+- `scripts/check-openapi-drift.py` — daily CI drift check; reports missing/extra methods, missing/extra struct fields, missing schemas, and field-level optionality mismatches against the live spec.
 - `spec_coverage_test.rs::field_optionality_matches_spec` — asserts every field's `Option<T>` vs `T` matches the snapshot.
 
+Field coverage is **bidirectional**, mirroring the missing/extra split used for client methods:
+
+- `struct_fields_cover_every_spec_property` (spec → code) — every spec property has a matching struct field; catches fields *added* to the spec.
+- `struct_fields_have_no_extras_vs_spec` (code → spec) — every struct field maps to a spec property; catches fields *removed* from the spec but left behind in `models.rs` (a superset model would otherwise pass every other field check). Schemas with no/empty `properties` are skipped, so composition/marker schemas don't flag every field. The drift script's "Extra Struct Fields" section reports the same finding.
+
 Field optionality is maintained by hand. When the drift check or test flags a mismatch, edit `models.rs` directly to flip the field (`T` ↔ `Option<T>`) and adjust the `#[serde(skip_serializing_if = "Option::is_none")]` attribute to match.
 
 **Optionality exemptions:**
 
 Sometimes the spec marks a field as required but the API rejects empty/default values, meaning the field is effectively optional. These fields are kept as `Option<T>` in `models.rs` and listed in the `OPTIONALITY_EXEMPTIONS` constant in `spec_coverage_test.rs`. The test logs each exemption and fails if any become stale (spec was fixed upstream). When adding a new exemption, add a `("RustStructName", "specFieldName")` entry with a comment explaining the API behavior.
 
+**Extra-field exemptions:**
+
+A struct field that intentionally has no spec property (a code-only/computed field, or a standard attribute the upstream spec omits) goes in the `EXTRA_FIELD_EXEMPTIONS` constant in `spec_coverage_test.rs`, analogous to `OPTIONALITY_EXEMPTIONS` and to `NON_OPENAPI_CLIENT_METHODS` for methods. `struct_fields_have_no_extras_vs_spec` fails on a stale entry (one that no longer corresponds to an actual extra field), and `check-openapi-drift.py` parses the same list so the report and test stay in sync. The list is empty by default — only add an entry for a *deliberate* addition, not to silence a field that should be removed.
+
 ##### Deprecated field hiding
 
 Fields the spec marks `deprecated: true` — on both response schemas (e.g. `Service.tier`, `ApiKey.roles`) and request schemas (e.g. `ServicePostRequest.tier`, `InvitationPostRequest.role`) — are removed from the struct entirely so consumers, including the CLI, can't even reference a field the API has deprecated. Each carries `#[cfg(feature = "deprecated-fields")]` in `models.rs`: absent from the struct by default, present only when the `deprecated-fields` Cargo feature is on. On a **response** struct that means reading it is a compile error and it never appears in output (deserializing a payload that still contains it just ignores the extra key — no schema uses `deny_unknown_fields`). On a **request** struct it means callers can't set it and `skip_serializing_if` keeps it off the wire entirely.

README.md

@@ -499,7 +499,6 @@ clickhousectl cloud postgres create \
 
 # Update metadata (all flags optional)
 clickhousectl cloud postgres update <pg-id> \
-  --name renamed \
   --size m7i.4xlarge \
   --add-tag env=prod --remove-tag legacy
 

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

@@ -10521,14 +10521,6 @@ pub struct PostgresServicePatchRequest {
     #[serde(rename = "haType", skip_serializing_if = "Option::is_none", default)]
     pub ha_type: Option<PgHaType>,
     #[serde(skip_serializing_if = "Option::is_none", default)]
-    pub name: Option<PgNameProperty>,
-    #[serde(rename = "postgresVersion", skip_serializing_if = "Option::is_none", default)]
-    pub postgres_version: Option<PgVersion>,
-    #[serde(skip_serializing_if = "Option::is_none", default)]
-    pub provider: Option<PgProvider>,
-    #[serde(skip_serializing_if = "Option::is_none", default)]
-    pub region: Option<PgRegion>,
-    #[serde(skip_serializing_if = "Option::is_none", default)]
     pub size: Option<PgSize>,
     #[serde(skip_serializing_if = "Option::is_none", default)]
     pub tags: Option<PgTags>,
@@ -10818,8 +10810,6 @@ pub struct ScalingSchedulePostRequest {
 /// `ScimEnterpriseManager` from the ClickHouse Cloud API.
 #[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)]
 pub struct ScimEnterpriseManager {
-    #[serde(rename = "$ref", default)]
-    pub r#ref: String,
     #[serde(rename = "displayName", default)]
     pub display_name: String,
     #[serde(default)]

crates/clickhouse-cloud-api/tests/client_test.rs

@@ -2052,24 +2052,24 @@ async fn update_postgres_service() {
 
     Mock::given(method("PATCH"))
         .and(path("/v1/organizations/org-1/postgres/pg-1"))
-        .and(body_partial_json(serde_json::json!({"name": "pg-renamed"})))
+        .and(body_partial_json(serde_json::json!({"size": "c6gd.medium"})))
         .respond_with(ok_json(serde_json::json!({
             "id": "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee",
-            "name": "pg-renamed"
+            "name": "pg-1"
         })))
         .mount(&s)
         .await;
 
     let body = PostgresServicePatchRequest {
-        name: Some("pg-renamed".to_string()),
+        size: Some(PgSize::C6gd_medium),
         ..Default::default()
     };
     let resp = c
         .postgres_service_patch("org-1", "pg-1", &body)
         .await
         .unwrap();
     let pg = resp.result.unwrap();
-    assert_eq!(pg.name, "pg-renamed");
+    assert_eq!(pg.name, "pg-1");
 }
 
 #[tokio::test]

crates/clickhouse-cloud-api/tests/spec_coverage_test.rs

@@ -281,6 +281,18 @@ async fn struct_fields_cover_every_live_spec_property() {
     assert_field_coverage(&spec);
 }
 
+#[test]
+fn struct_fields_have_no_extras_vs_spec() {
+    assert_no_extra_struct_fields(&serde_json::from_str(SPEC_JSON).unwrap());
+}
+
+#[tokio::test]
+#[ignore = "hits the live published ClickHouse OpenAPI spec"]
+async fn struct_fields_have_no_extras_vs_live_spec() {
+    let spec = load_live_spec().await;
+    assert_no_extra_struct_fields(&spec);
+}
+
 /// Fields where our `Option<T>` vs `T` intentionally disagrees with the spec.
 ///
 /// The spec sometimes marks fields as required when the API actually treats them
@@ -541,6 +553,98 @@ fn assert_field_coverage(spec: &Value) {
     );
 }
 
+/// Struct fields we deliberately keep in `models.rs` even though the mapped
+/// spec schema has no such property. Analogous to `NON_OPENAPI_CLIENT_METHODS`
+/// (intentional client methods with no spec operation): a code-only field we
+/// add on purpose — a response-only/computed field, or a standard attribute the
+/// upstream spec omits. Each entry is `("RustStructName", "specFieldName")`.
+///
+/// Empty by design. Every extra field the detector surfaces today is a real
+/// drift finding (a field removed upstream but left in `models.rs`), not an
+/// intentional addition — so nothing is exempted. The
+/// `struct_fields_have_no_extras_vs_spec` test fails on a stale entry (one that
+/// no longer corresponds to an actual extra field) so this list can't rot.
+const EXTRA_FIELD_EXEMPTIONS: &[(&str, &str)] = &[];
+
+/// Assert that no field in a Rust struct is absent from its mapped OpenAPI
+/// schema. The mirror of `assert_field_coverage`: that catches spec properties
+/// missing from structs (spec → code); this catches struct fields missing from
+/// the spec (code → spec), e.g. a field removed from the schema upstream but
+/// left behind in `models.rs`. Intentional code-only fields are listed in
+/// `EXTRA_FIELD_EXEMPTIONS`.
+///
+/// Schemas with no `properties` (or an empty `properties` object) are skipped,
+/// matching `assert_field_coverage` — composition/marker schemas carry their
+/// fields elsewhere and would otherwise flag every struct field as extra.
+fn assert_no_extra_struct_fields(spec: &Value) {
+    let schemas = spec["components"]["schemas"].as_object().unwrap();
+    let model_fields = parse_model_fields(MODELS_RS);
+
+    let exemptions: BTreeSet<(&str, &str)> = EXTRA_FIELD_EXEMPTIONS.iter().copied().collect();
+    let mut exemptions_hit: BTreeSet<(&str, &str)> = BTreeSet::new();
+    let mut extras = Vec::new();
+
+    for (spec_name, schema) in schemas {
+        let rust_name = pascalize_identifier(spec_name);
+        let fields = match model_fields.get(&rust_name) {
+            Some(f) => f,
+            None => continue, // Schema not in models — covered by other tests
+        };
+
+        let props = match schema.get("properties").and_then(Value::as_object) {
+            Some(p) if !p.is_empty() => p,
+            _ => continue,
+        };
+
+        for spec_field in fields.keys() {
+            if props.contains_key(spec_field.as_str()) {
+                continue;
+            }
+
+            if exemptions
+                .iter()
+                .any(|(s, f)| *s == rust_name && *f == spec_field.as_str())
+            {
+                exemptions_hit.insert(
+                    *exemptions
+                        .iter()
+                        .find(|(s, f)| *s == rust_name && *f == spec_field.as_str())
+                        .unwrap(),
+                );
+                eprintln!(
+                    "NOTE: {}.{} extra-field exempted — see EXTRA_FIELD_EXEMPTIONS",
+                    rust_name, spec_field
+                );
+                continue;
+            }
+
+            extras.push(format!("{}.{}", rust_name, spec_field));
+        }
+    }
+
+    // Detect stale exemptions — entries that no longer correspond to an extra.
+    let stale: Vec<_> = exemptions
+        .difference(&exemptions_hit)
+        .map(|(s, f)| format!("({}, {})", s, f))
+        .collect();
+    assert!(
+        stale.is_empty(),
+        "Stale EXTRA_FIELD_EXEMPTIONS (struct field now matches the spec or was removed):\n{}",
+        stale.join("\n")
+    );
+
+    extras.sort();
+    assert!(
+        extras.is_empty(),
+        "Struct fields with no matching spec property ({} total):\n{}\n\
+         A field listed here was removed from (or never existed in) its OpenAPI \
+         schema but still lives in models.rs. Remove it, or — if it's an \
+         intentional code-only field — add it to EXTRA_FIELD_EXEMPTIONS.",
+        extras.len(),
+        extras.join("\n")
+    );
+}
+
 /// Schemas where the spec's `required` array lists only newly-added fields;
 /// older fields on the same schema still rely on the description heuristic.
 /// For these we union `required[]` with the description heuristic instead of

crates/clickhousectl/src/cloud/cli.rs

@@ -2673,7 +2673,7 @@ mod tests {
 
         // Postgres writes
         assert_write(&["clickhousectl", "cloud", "postgres", "create", "--name", "pg", "--region", "us-east-1", "--size", "m7i.2xlarge"], true);
-        assert_write(&["clickhousectl", "cloud", "postgres", "update", "pg-1", "--name", "renamed"], true);
+        assert_write(&["clickhousectl", "cloud", "postgres", "update", "pg-1", "--size", "c6gd.large"], true);
         assert_write(&["clickhousectl", "cloud", "postgres", "delete", "pg-1"], true);
         assert_write(&["clickhousectl", "cloud", "postgres", "config", "replace", "pg-1", "--file", "/tmp/c.json"], true);
         assert_write(&["clickhousectl", "cloud", "postgres", "config", "patch", "pg-1", "--set", "max_connections=500"], true);

crates/clickhousectl/src/cloud/postgres.rs

@@ -72,15 +72,7 @@ pub enum PostgresCommands {
     Update {
         postgres_id: String,
         #[arg(long)]
-        name: Option<String>,
-        #[arg(long)]
-        region: Option<String>,
-        #[arg(long)]
         size: Option<String>,
-        #[arg(long)]
-        provider: Option<String>,
-        #[arg(long, value_parser = clap::builder::PossibleValuesParser::new(KNOWN_PG_VERSIONS))]
-        pg_version: Option<String>,
         #[arg(long, value_parser = clap::builder::PossibleValuesParser::new(KNOWN_PG_HA_TYPES))]
         ha_type: Option<String>,
         /// Add a tag (repeatable), e.g. --add-tag env=prod
@@ -436,11 +428,7 @@ pub struct PostgresCreateOptions<'a> {
 }
 
 pub struct PostgresUpdateOptions<'a> {
-    pub name: Option<&'a str>,
-    pub region: Option<&'a str>,
     pub size: Option<&'a str>,
-    pub provider: Option<&'a str>,
-    pub pg_version: Option<&'a str>,
     pub ha_type: Option<&'a str>,
     pub add_tag: &'a [String],
     pub remove_tag: &'a [String],
@@ -630,15 +618,7 @@ pub async fn postgres_update(
 ) -> Result<(), Box<dyn std::error::Error>> {
     let org_id = resolve_org_id(client, opts.org_id).await?;
 
-    let provider = opts
-        .provider
-        .map(|v| parse_serde_enum::<PgProvider>(v, "provider", KNOWN_PG_PROVIDERS))
-        .transpose()?;
     let size = opts.size.map(parse_pg_size).transpose()?;
-    let pg_version = opts
-        .pg_version
-        .map(|v| parse_serde_enum::<PgVersion>(v, "pg-version", KNOWN_PG_VERSIONS))
-        .transpose()?;
     let ha_type = opts
         .ha_type
         .map(|v| parse_serde_enum::<PgHaType>(v, "ha-type", KNOWN_PG_HA_TYPES))
@@ -659,11 +639,7 @@ pub async fn postgres_update(
     };
 
     let req = PostgresServicePatchRequest {
-        name: opts.name.map(|s| s.to_string()),
-        provider,
-        region: opts.region.map(|s| s.to_string()),
         size,
-        postgres_version: pg_version,
         ha_type,
         tags,
     };
@@ -1135,31 +1111,31 @@ mod tests {
     fn parses_postgres_update_tag_diff_flags() {
         let cmd = parse_postgres(&[
             "clickhousectl", "cloud", "postgres", "update", "pg-1",
-            "--name", "renamed",
+            "--size", "c6gd.large",
             "--add-tag", "env=prod",
             "--add-tag", "team=data",
             "--remove-tag", "old",
         ]);
         let PostgresCommands::Update {
-            postgres_id, name, add_tag, remove_tag, ..
+            postgres_id, size, add_tag, remove_tag, ..
         } = cmd
         else {
             panic!("expected update");
         };
         assert_eq!(postgres_id, "pg-1");
-        assert_eq!(name.as_deref(), Some("renamed"));
+        assert_eq!(size.as_deref(), Some("c6gd.large"));
         assert_eq!(add_tag, vec!["env=prod", "team=data"]);
         assert_eq!(remove_tag, vec!["old"]);
     }
 
     #[test]
     fn parses_postgres_update_no_fields() {
         let cmd = parse_postgres(&["clickhousectl", "cloud", "postgres", "update", "pg-1"]);
-        let PostgresCommands::Update { postgres_id, name, .. } = cmd else {
+        let PostgresCommands::Update { postgres_id, size, .. } = cmd else {
             panic!("expected update");
         };
         assert_eq!(postgres_id, "pg-1");
-        assert!(name.is_none());
+        assert!(size.is_none());
     }
 
     #[test]

crates/clickhousectl/src/main.rs

@@ -1066,22 +1066,14 @@ async fn run_postgres(
         }
         PostgresCommands::Update {
             postgres_id,
-            name,
-            region,
             size,
-            provider,
-            pg_version,
             ha_type,
             add_tag,
             remove_tag,
             org_id,
         } => {
             let opts = PostgresUpdateOptions {
-                name: name.as_deref(),
-                region: region.as_deref(),
                 size: size.as_deref(),
-                provider: provider.as_deref(),
-                pg_version: pg_version.as_deref(),
                 ha_type: ha_type.as_deref(),
                 add_tag: &add_tag,
                 remove_tag: &remove_tag,