refactor(models): converge provider_key/mcp/a2a field names to the canonical resource model - #757
Conversation
…anonical resource model
Per CLAUDE.md "The Resource Model Is Canonical in cp-admin.yaml", this
repo converges to the control plane's field names when the two disagree.
Three fields and one route spelling drifted; this converges them:
- `ProviderKey.secret` -> `api_key`
- `McpServer.display_name` -> `name`
- `A2aAgent.display_name` -> `name`
- `/admin/v1/apikeys*` -> canonical `/admin/v1/api_keys*` added;
the former spelling stays routed to the same handlers.
Acceptance of the former names is dual-layer, because the etcd loader
validates documents against the generated JSON Schema BEFORE serde:
- serde: the renamed fields carry `#[serde(alias = "...")]`, so both
spellings deserialize onto the same field (composes with
`deny_unknown_fields`).
- JSON Schema: `schemars` does not emit serde aliases, so the schema
producers apply an `accept_renamed_field` transform — the former name
is declared as a property with the canonical shape and requiredness
becomes `anyOf: [{required:[new]}, {required:[old]}]`, with
`additionalProperties: false` intact. Without this, every stored
old-name document would be silently dropped at the loader's schema
gate.
A document carrying BOTH spellings passes the schema and is rejected by
serde's duplicate-field check, so the ambiguity never loads with one
value silently winning; pinned by unit and loader tests.
Re-serialization (admin GET responses, admin-written documents) now
emits only the canonical names. Regenerated `schemas/resources/` and
the admin OpenAPI accordingly; the former API-key paths are mirrored
into the OpenAPI document programmatically so the published reference
matches the router without a hand-maintained duplicate.
e2e: existing fixtures keep the former spellings as acceptance proof; a
differential case seeds the same logical provider_key under each
spelling and asserts identical serve behavior plus canonical `api_key`
emission for both; a few fixtures migrate to the canonical spellings so
both stay exercised long-term.
BREAKING CHANGE: Admin API GET responses and admin-written documents
now emit `api_key` (provider_keys) and `name` (mcp_servers, a2a_agents)
instead of `secret` / `display_name`. Requests and stored documents are
accepted under both spellings, and `/admin/v1/apikeys*` keeps working
unchanged, so writers and stored data are unaffected; only consumers
that read the former field names out of Admin API responses need
updating.
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (36)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
moonming
changed the title
The renamed-field acceptance moved requiredness into a document-root `anyOf`, and the JSON-Schema library's anyOf failure message interpolates the entire failing instance — so a document missing both name spellings but carrying an upstream credential would echo that credential into loader warnings, the rejection detail surfaced on the health path, and Admin API 400 bodies. Mask instance values in `validate()`; messages keep their structure and JSON pointer, and the pre-existing property-level value echoes stop too. Flagged by the independent post-push audit of #757.
1 participant
Motivation
CLAUDE.md, "The Resource Model Is Canonical in cp-admin.yaml": when this repo and the control plane disagree about a resource field's name, the control plane's spec wins and this repo converges to it, with
#[serde(alias = "…")]so stored documents and existing callers keep loading, and regeneratedschemas/resources/afterwards. Three fields and one route spelling had drifted; this PR converges them:provider_keyssecretapi_keymcp_serversdisplay_namenamea2a_agentsdisplay_namename/admin/v1/apikeys*/admin/v1/api_keys*(former spelling still served, same handlers)House precedent for
!renames with explicit compatibility notes: #657, #755.Dual-layer acceptance (the load-bearing part)
The etcd snapshot loader validates every document against the generated JSON Schema before serde (
crates/aisix-etcd/src/loader.rs), and a schema-rejected row is silently skipped. Stored etcd data and current control-plane writes still carry the former names, so acceptance must hold at both layers:#[serde(alias = "secret")]/#[serde(alias = "display_name")]. Alias names are known fields to serde, so this composes withdeny_unknown_fields.schemarsdoes not emit serde aliases — a naively regenerated schema would list only the new name and, withadditionalProperties: false, reject every old-name document at the loader's schema gate. A newaccept_renamed_fieldtransform incrates/aisix-core/src/models/schema.rs(applied inside the same*_root_schema()producers that feed both the runtime validators anddump-schema, so published schema == enforced schema by construction) declares the former name as a property with the canonical property's shape (minLengthetc. keep applying) and rewrites requiredness asanyOf: [{required:["api_key"]}, {required:["secret"]}], keepingadditionalProperties: false.schemas/resources/{provider_key,mcp_server,a2a_agent}.schema.jsonare regenerated viacargo run -p aisix-core --bin dump-schema; the admin OpenAPI (which embeds these files) is regenerated/verified viacargo run -p aisix-admin --bin dump-openapi. Both are idempotent, matching the CI drift checks.Both-spellings corner
A document carrying both the canonical and the former spelling passes the schema (
anyOfadmits it) and is then rejected by serde's duplicate-field error — both names map to the same field — so the ambiguous document never loads with one value silently winning. In the loader this surfaces as aParseFailedrejection (row skipped, batch continues, rejection reported on the health surface); on the Admin API write path it is a 400. Pinned by unit tests in each model, schema tests, and loader tests.Emission is the visible change
Re-serialization now emits only the canonical names:
api_key/name.Requests and stored documents are accepted under both spellings, and
/admin/v1/apikeys*keeps working unchanged.For standalone operators: nothing to migrate. Existing etcd data keeps loading as-is; scripts that POST/PUT the former field names or call
/admin/v1/apikeys*keep working. Only tooling that readssecret/display_nameout of Admin API responses for these three resources needs to readapi_key/nameinstead.Mixed-version fleets sharing one etcd: a gateway replica on an older version schema-rejects canonical-spelling documents (its schema requires the former names and closes
additionalProperties) and silently drops them from its snapshot. In multi-replica deployments sharing one configuration store, upgrade every replica to this version before writing resources through the Admin API for these kinds (this version's admin writes store the canonical spellings) — and before any writer flips its emission to the canonical names.For the control plane: its writes carry the former names today and keep validating and loading through the alias + schema
anyOf; it can move its own emission to the canonical names independently once the data planes it writes for are on this version — nothing else in this PR forces timing on that.Admin API surface
/admin/v1/api_keys,/admin/v1/api_keys/{id},/admin/v1/api_keys/{id}/rotatealongside the former spelling (same handlers).document_former_apikeys_pathsstep mirrors each onto the former path (summary suffixed "(alternate path)", description prefixed with the equivalence note) so the published reference matches the router exactly without a second hand-maintained copy.name must not contain …).Tests
ParseFailedpath./admin/v1/api_keysis readable/rotatable/deletable via/admin/v1/apikeysand vice versa; OpenAPI tests assert the exact path set including both spellings.seed-vs-admin-characterization-e2e.test.tsseeds the same logical provider_key once per spelling and asserts identical serve behavior and that admin GET emitsapi_key(neversecret) for admin-written and both seeded documents. A small number of fixtures migrate to the canonical spellings (auth-baseline,canary-routing, oneapikey-lifecyclerotation via the canonical route, one mcp integration fixture) so both spellings stay exercised long-term.Audit follow-up
The independent post-push audit flagged that the renamed-field
anyOfsits at the document root, and the JSON-Schema library'sanyOffailure message interpolates the entire failing instance — so a document missing both name spellings but carrying a live upstream credential would have that credential echoed into loader warnings, the rejection detail surfaced on the health/heartbeat path, and Admin API 400 bodies. Fixed by masking instance values invalidate()(err.masked()), which also stops the pre-existing property-level value echoes; messages keep their structure and JSON pointer. Pinned byschema_error_for_missing_name_does_not_echo_the_document.Verification
cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace— green.aisix-adminetcd_integration,aisix-etcdwatch_integration) — green against a real etcd.tests/e2e, real etcd + redis) — green twice.dump-schemaanddump-openapiidempotent (CI mirror).