test(contract): assert pkg/types request shapes against backend swagger#101
Conversation
Closes #97. Adds a unit-test-only contract layer that validates every stackctl request struct against the backend's published OpenAPI (Swagger 2.0) schema. Catches the class of bug shipped four times this week (#95, #98, k8s-sm#264, the BulkOperationResult shape) where unit / integration / e2e tests all stub the backend and decode requests into stackctl's OWN types — so a json-tag drift between stackctl and the backend decodes cleanly in tests but 400s the moment a real backend reads the bytes. What ships: cli/test/contract/testdata/swagger.json Vendored copy of omattsson/k8s-stack-manager:backend/docs/swagger.json. Pinned (not auto-fetched) so backend-side wire changes show up as explicit diffs in this repo. cli/test/contract/refresh-swagger.sh One-shot refresh: `./refresh-swagger.sh [ref]`. Validates the payload is JSON, sha256-compares before overwriting, and tells you which test to re-run. Pinned to a ref so partial backend rollouts don't break stackctl CI. cli/test/contract/contract_test.go Embeds swagger.json via go:embed, walks 9 request structs via reflection, and asserts bidirectionally: 1. Every Go json tag exists as a property in swagger. Catches stale or typoed Go-side fields. 2. Every swagger "required" field has a Go json tag. Catches missing-required-field bugs. 3. Field types align (string/boolean/integer/number/array/object). Covers: CreateClusterRequest, UpdateClusterRequest, BulkInstancesRequest, BulkTemplatesRequest, RegisterRequest, LoginRequest, CreateAPIKeyRequest, ResetPasswordRequest, CreateCleanupPolicyRequest. Plus TestSwaggerVendorIntegrity — sanity floor on the vendored copy so a truncated swagger.json doesn't manifest as a pile of confusing per-case failures. Failure-mode validated with a synthetic drift (matches the pre-fix/bulk-wire-contract bug exactly): Error: Go struct field Ids (json:"ids") has no matching property in swagger schema — typo, or backend doesn't accept this field Error: swagger schema requires field "instance_ids" but the Go struct has no field with that json tag — request will fail validation V1 deliberately skips: - Response types (issue calls out polymorphism for GET endpoints; a separate iteration). - CreateTemplateRequest / UpdateTemplateRequest — backend's swag annotation uses an unexported struct (createTemplateRequest) so no schema is published. Worth a tiny upstream PR to make the type exported + annotated. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a contract test suite ( ChangesContract Test Suite
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cli/test/contract/refresh-swagger.sh (1)
32-32: ⚡ Quick winConsider stdin redirection for safer quoting.
The current approach embeds
$tmpin the Python code string. While mktemp output is typically safe, using stdin redirection avoids potential quoting issues entirely (e.g., if TMPDIR contains spaces).♻️ Proposed refactor
-if ! python3 -c "import json,sys; json.load(open('$tmp'))" 2>/dev/null; then +if ! python3 -c "import json,sys; json.load(sys.stdin)" < "$tmp" 2>/dev/null; then echo "ERROR: fetched file is not valid JSON, refusing to overwrite" >&2 exit 1 fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/test/contract/refresh-swagger.sh` at line 32, Replace the embedded filename usage in the inline Python call to avoid quoting issues by reading from stdin: change the Python snippet that currently calls json.load(open('$tmp')) to read from sys.stdin (json.load(sys.stdin)) and invoke python3 -c with input redirected from the shell variable $tmp (use shell stdin redirection with "$tmp"); update the invocation around the existing python3 -c string and the tmp variable accordingly so the script validates JSON via stdin rather than embedding the filename.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cli/test/contract/refresh-swagger.sh`:
- Line 17: Update the top comment that lists prerequisites so it includes
python3: modify the existing "# Requires: curl, shasum." line to also mention
python3, since the script invokes python3 for JSON validation (see the python3
usage around the JSON validation call). Ensure the wording matches the existing
style, e.g., add ", python3" to the requirements list.
---
Nitpick comments:
In `@cli/test/contract/refresh-swagger.sh`:
- Line 32: Replace the embedded filename usage in the inline Python call to
avoid quoting issues by reading from stdin: change the Python snippet that
currently calls json.load(open('$tmp')) to read from sys.stdin
(json.load(sys.stdin)) and invoke python3 -c with input redirected from the
shell variable $tmp (use shell stdin redirection with "$tmp"); update the
invocation around the existing python3 -c string and the tmp variable
accordingly so the script validates JSON via stdin rather than embedding the
filename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 966ebaf8-5aeb-4b0b-ae86-ad9605dbef89
⛔ Files ignored due to path filters (1)
cli/test/contract/testdata/swagger.jsonis excluded by!**/testdata/**
📒 Files selected for processing (2)
cli/test/contract/contract_test.gocli/test/contract/refresh-swagger.sh
…ia stdin Both address CodeRabbit on PR #101: the script invokes python3 for JSON validation but the requirements comment listed only curl and shasum, and the validation embedded the temp filename in the inline Python string (mktemp output is normally safe, but stdin redirection is cleaner regardless and removes any quoting concern). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Actionable comments posted: 0 |
Summary
Closes #97.
Adds a unit-test-only contract layer that validates every stackctl request struct against the backend's published OpenAPI (Swagger 2.0) schema. Catches the class of bug shipped four times this week (#95, #98, k8s-sm#264, the
BulkOperationResultshape) where every other test layer stubs the backend and decodes requests into stackctl's own types — so a json-tag drift between stackctl and the backend decodes cleanly in tests but 400s the moment a real backend reads the bytes.What's in the PR
cli/test/contract/testdata/swagger.jsonomattsson/k8s-stack-manager:backend/docs/swagger.json. Pinned (not auto-fetched) so backend-side wire changes surface as explicit diffs.cli/test/contract/refresh-swagger.sh./refresh-swagger.sh [ref]. Validates the payload is JSON, sha256-compares before overwriting, tells you which test to re-run.cli/test/contract/contract_test.gogo:embed-loads swagger.json, walks 9 request structs via reflection, asserts bidirectionally.Coverage (V1)
Nine request types, each pinned to its
handlers.*(ormodels.*) swagger definition:CreateClusterRequest,UpdateClusterRequest,BulkInstancesRequest,BulkTemplatesRequest,RegisterRequest,LoginRequest,CreateAPIKeyRequest,ResetPasswordRequest,CreateCleanupPolicyRequest.Validation rules
jsontag must exist as a property in the swagger definition — catches stale or typoed Go-side fields.requiredfield must have a matching Gojsontag — catches missing-required-field bugs.reflect.Kind→ swaggertype(string/boolean/integer/number/array/object), with pointer unwrap and slice-element checks.Failure mode (synthetic drift, mirrors the pre-fix/bulk-wire-contract bug)
V1 non-goals (deferred)
TemplateDetailResponsevsStackTemplate); needs special handling, separate iteration.createTemplateRequest) so no schema is published. Worth a tiny upstream PR.Test plan
go test ./test/contract/...passes against current vendored schemago vet ./test/contract/...clean🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Chores