Skip to content

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

Open
sdairs wants to merge 2 commits into
mainfrom
issue-237-extra-struct-fields-detector
Open

Detect struct fields removed from the spec but left in models.rs (#237)#240
sdairs wants to merge 2 commits into
mainfrom
issue-237-extra-struct-fields-detector

Conversation

@sdairs
Copy link
Copy Markdown
Collaborator

@sdairs sdairs commented Jun 5, 2026

Closes #237.

Problem

OpenAPI drift detection was one-directional for fields (spec → code). It caught fields added to the spec that are missing from models.rs and optionality flips, but not the reverse: a field removed from the live spec yet still present as a struct field. A model that is a superset of the spec passed every field check — exactly how the stale storageSize fields in #235/#236 slipped through.

Change

Adds the code→spec mirror of the existing missing-field check, matching the missing/extra split already used for client methods (assert_client_operation_coverage), and remediates the findings it surfaces so the model matches the vendored snapshot.

Detector

  • spec_coverage_test.rs: assert_no_extra_struct_fields + struct_fields_have_no_extras_vs_spec (snapshot) and struct_fields_have_no_extras_vs_live_spec (live, #[ignore]). Adds an EXTRA_FIELD_EXEMPTIONS allowlist — analogous to OPTIONALITY_EXEMPTIONS / NON_OPENAPI_CLIENT_METHODS — with stale-entry detection. Schemas with no/empty properties are skipped so composition/marker schemas don't flag every field.
  • check-openapi-drift.py: check_extra_fields reverse pass + parse_extra_field_exemptions (reads the same Rust constant), surfaced as a new "Extra Struct Fields" report section and summary row.
  • AGENTS.md: documents the now-bidirectional field coverage and the EXTRA_FIELD_EXEMPTIONS allowlist.

Remediation

The detector surfaced 5 stale fields against the snapshot; all are fixed, not exempted, so EXTRA_FIELD_EXEMPTIONS stays empty.

Schema Field(s) removed Spec allows
PostgresServicePatchRequest name, provider, region, postgresVersion size, haType, tags
ScimEnterpriseManager $ref displayName, value
  • PostgresServicePatchRequest is live CLI surface — same class of stale field as the storageSize cleanup in Remove storageSize from Postgres create/update requests #235/Remove storageSize from Postgres create/update requests #236. The postgres update command loses the matching --name/--region/--provider/--pg-version flags (keeps --size/--ha-type/--add-tag/--remove-tag); PostgresUpdateOptions, the postgres_update handler, the main.rs match arm, the parse/write-command tests, and the README update example are updated accordingly.
  • ScimEnterpriseManager.$ref was a code-only SCIM attribute absent from the ClickHouse spec, with no usages.
  • Library update_postgres_service mock test rewritten to patch size instead of name.

The live-spec-only findings (seekSnapshot on the two PubSub sources) are out of scope — they aren't in the vendored snapshot this PR aligns to; they'll be picked up by the live drift run / snapshot refresh.

Verification

  • struct_fields_have_no_extras_vs_spec now passes — all 9 snapshot spec-coverage tests green; the 8 live-spec tests stay #[ignore].
  • Allowlist proven both ways: a real entry is suppressed (NOTE: … exempted), a bogus entry fails as stale.
  • Drift script renders the new section/summary row; script and test agree on the same findings.
  • cargo build, cargo clippy --all-targets, and the full clickhousectl + clickhouse-cloud-api test suites pass; all integration targets compile.

🤖 Generated with Claude Code


Note

Medium Risk
Removes postgres update flags and patch request fields users may still rely on; changes are spec-aligned but are a breaking CLI/API-model surface for Postgres metadata updates.

Overview
Adds bidirectional OpenAPI field coverage so drift detection catches Rust struct fields that no longer exist in the spec, not only missing spec properties. spec_coverage_test.rs gains assert_no_extra_struct_fields with snapshot and live-spec tests plus an empty EXTRA_FIELD_EXEMPTIONS allowlist (stale-entry checks). check-openapi-drift.py mirrors this with check_extra_fields, exemption parsing, and an Extra Struct Fields report section.

Remediation removes five stale fields surfaced by the new check: PostgresServicePatchRequest drops name, provider, region, and postgresVersion; ScimEnterpriseManager drops $ref. The cloud postgres update CLI is narrowed to match the patch schema—--name, --region, --provider, and --pg-version are removed; updates use --size, --ha-type, and tag flags. README, library mock tests, and CLI parse/write tests follow the same shape.

AGENTS.md documents bidirectional field coverage and extra-field exemptions.

Reviewed by Cursor Bugbot for commit 676d5b9. Bugbot is set up for automated code reviews on this repo. Configure here.

OpenAPI drift detection was one-directional for fields: it caught spec
properties missing from models.rs and optionality flips, but not the
reverse — a field removed from the live spec yet still present as a
struct field. A model that is a superset of the spec passed every field
check (this is how the stale `storageSize` fields in #235/#236 slipped
through).

Add the code→spec mirror of the existing missing-field check, matching
the missing/extra split already used for client methods:

- spec_coverage_test.rs: `assert_no_extra_struct_fields` +
  `struct_fields_have_no_extras_vs_{spec,live_spec}` tests, plus an
  `EXTRA_FIELD_EXEMPTIONS` allowlist (analogous to OPTIONALITY_EXEMPTIONS
  / NON_OPENAPI_CLIENT_METHODS) with stale-entry detection. Schemas with
  no/empty `properties` are skipped so composition/marker schemas don't
  flag every field.
- check-openapi-drift.py: `check_extra_fields` reverse pass +
  `parse_extra_field_exemptions` (reads the same constant), surfaced as a
  new "Extra Struct Fields" report section and summary row.
- AGENTS.md: document the now-bidirectional field coverage and the
  EXTRA_FIELD_EXEMPTIONS allowlist.

Detection only — nothing is remediated and the allowlist is empty, so the
real findings it surfaces are reported rather than hidden.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sdairs sdairs temporarily deployed to cloud-integration June 5, 2026 12:37 — with GitHub Actions Inactive
Bring models.rs in line with the vendored OpenAPI snapshot, clearing the
5 findings surfaced by the new struct_fields_have_no_extras_vs_spec
detector (#237). EXTRA_FIELD_EXEMPTIONS stays empty — these are fixed,
not exempted.

- PostgresServicePatchRequest: drop name/provider/region/postgresVersion;
  the live spec only allows size/haType/tags. The `postgres update` CLI
  surface loses the matching --name/--region/--provider/--pg-version
  flags (same class of stale field as the storageSize cleanup in
  #235/#236).
- ScimEnterpriseManager: drop the code-only $ref attribute absent from
  the ClickHouse spec.

Updates the affected library and CLI tests plus the README update
example. struct_fields_have_no_extras_vs_spec now passes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@sdairs sdairs deployed to cloud-integration June 5, 2026 13:19 — with GitHub Actions Active
@sdairs sdairs marked this pull request as ready for review June 5, 2026 13:49
@sdairs sdairs requested a review from iskakaushik as a code owner June 5, 2026 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OpenAPI drift detection misses fields removed from the spec but still present in models.rs

1 participant