refactor(core): make resource structs the single source of truth for config schemas - #638
Merged
Merged
Conversation
Pins the exact accept/reject behavior of validate_model across every constraint the hand-written model_schema() enforces (oneOf mutual exclusion, provider pattern/length, nested object bounds, additional- Properties). This is the guardrail for collapsing the runtime validator onto the Model struct + schemars so the refactor can prove it did not widen or narrow the config contract. The rate_limit.rps/rph cases encode today's (buggy) rejection and will flip to accept in the refactor.
…e source) The model resource had two independently hand-maintained schema representations in this crate: the runtime validator (a json! literal in models/schema.rs) and the schemars-derived struct (published to schemas/resources/ and vendored downstream). Nothing gated them against each other, so they drifted — most visibly, the runtime validator's $defs/rate_limit omitted rps/rph while the RateLimit struct and the rate limiter (store/local.rs, store/redis.rs) support them, so a model carrying rate_limit.rps was accepted by the published schema but silently dropped at the DP loader. Make the Model struct the single source: port every per-field constraint (provider pattern/length, minLength, minItems, numeric ranges, status-code item bounds) onto the struct as schemars attributes, and express the one cross-field invariant schemars cannot derive (direct/routing/ensemble mutual exclusion) as model_one_of(), injected by the new producer model_root_schema(). Both the runtime validator and dump-schema call that producer, so published == enforced by construction. option_add_null_type is disabled for the model generation so optional fields stay plain-but-absent (matching the wire shape) rather than nullable. Behavior is preserved exactly (40-case characterization corpus) except the intended fix: rps/rph are now accepted on both model and apikey rate_limit. Regenerated model/ensemble/routing schemas reflect the constraints now carried on the structs. Other resources still use hand-written validators; migrating them to the same producer pattern is follow-up work.
Continues the single-source migration (after model). Both runtime validators now build from their structs via the shared struct_root_schema producer; dump-schema emits the same object, so published == enforced. - apikey: keeps nullable Option representation (team_id/user_id accept explicit null, which cp-api sends to clear team/owner); minLength(1) on key_hash/team_id/user_id ported as schemars attrs. Shared RateLimit now exposes all 7 dims incl rps/rph. - cache_policy: minLength/maxLength on name/applies_to and the ttl_seconds 1..=604800 range ported as attrs. Struct has no deny_unknown_fields, so additionalProperties stays open (forward-compat). Characterization corpora (resource_schema_characterization.rs) lock the accept/reject behavior for both.
Adds PolicyScope/PolicyWindow Rust enums (closed sets that were enforced only by the hand-written schema's enum constraint), so scope/window are now struct-derived and rejected at deserialize. The one cross-field rule schemars can't express — at least one of max_requests/max_tokens — is injected as a top-level anyOf by rate_limit_policy_root_schema(). name/ scope_ref minLength and max_requests/max_tokens range(min=1) ported as schemars attrs. aisix-proxy/quota.rs now matches the enums exhaustively (drops the dead String fall-through arms). Behavior change (intended): an unknown scope/window is now rejected at deserialize instead of silently ignored. Characterization corpus added for rate_limit_policy.
provider_key runtime validator now builds from the ProviderKey struct. telemetry_tags.kind becomes a closed TelemetryKind enum (catalog|byo), so the closed set is struct-derived and rejected at deserialize; the usage-event provider_kind emission (chat/messages) maps it via as_str(). display_name/secret gain minLength(1). Nullable Option representation kept (true) so cp-api's explicit null on telemetry labels still passes.
observability_exporter runtime validator now builds from the ObservabilityExporter struct. schemars renders the internally-tagged ExporterKind as a native top-level oneOf; the producer post-processes it to (1) re-close each branch with additionalProperties:false — schemars drops deny_unknown_fields in tagged-enum branches and serde doesn't enforce it there, so this restores plaintext-secret rejection — copying the shared name/enabled into each closed branch, and (2) inject the object_store cloud-identity cross-field rule (if/then/else) schemars can't derive. Per-field endpoint/site regex, project/logstore/bucket/etc minLength, content_max_bytes caps ported as schemars attrs. Intended tightening: cross-kind field leakage (e.g. a datadog exporter carrying an otlp 'project') is now rejected; no valid config mixes kinds. Characterization corpus added for observability_exporter.
…om structs Final resources in the single-source migration. Both runtime validators now build from their structs via *_root_schema() producers used by dump-schema too (published == enforced). guardrail: schemars renders the internally-tagged GuardrailKind as a native oneOf; the top level + branches stay open (unknown inner fields are caught by serde at deserialize, matching the old schema). The producer (1) re-closes the tagged sub-enums KeywordPattern/ BedrockAWSCredentials/BedrockLatencyMode with additionalProperties:false (schemars drops their deny_unknown_fields), (2) injects the closed enums for the stringly-typed moderation fields (output_type/text_source/ stream_processing_mode/on_buffer_exceeded/risk_level_threshold/categories) — kept as String since their values flow through aisix-guardrails as strings; converting to Rust enums would churn that crate — and (3) republishes created_at's date-time format. Per-field length/range attrs (name, bedrock id/version/region, endpoints, api_key, severity_threshold 0..7, window_size, max_buffer_bytes, timeout_ms u32 cap) ported to the structs. guardrail_attachment: struct-derived (nullable scope_id), now also published (it had no schemas/resources file before). Characterization corpora added for both.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR replaces hand-written JSON schema constants with schemars-derived ChangesSchema generation and typed policy enums
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
crates/aisix-core/tests/resource_schema_characterization.rs (1)
543-770: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd guardrail regression cases for the tightened/conditional fields
Please add corpus entries to explicitly pin:
created_at: nullbehavior inguardrail_corpus(accept/reject, whichever is intended contract), and- non-
envattachments with missing/nullscope_idinguardrail_attachment_corpus.This will keep these high-impact contract edges from drifting again.
🤖 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 `@crates/aisix-core/tests/resource_schema_characterization.rs` around lines 543 - 770, Add regression test cases to the guardrail_corpus function to explicitly test created_at with a null value and document whether it should pass or fail validation. Additionally, add test cases to the guardrail_attachment_corpus function for non-env scope types (such as model, team, and api_key) with missing or null scope_id values to ensure these high-impact contract edges are properly validated and prevent future drift. These edge cases should be documented with descriptive test names that clearly indicate the expected validation behavior.crates/aisix-core/tests/model_schema_characterization.rs (1)
253-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin explicit
nullrejection for non-null optional fields.The schema refactor removes nullable unions for many optional model fields; add a small reject test so the golden corpus catches accidental re-widening later.
Example test
+#[test] +fn reject_explicit_null_optional_fields() { + for (label, value) in [ + ( + "timeout null", + json!({"display_name": "m", "provider": "openai", "model_name": "g", "provider_key_id": "pk-1", "timeout": null}), + ), + ( + "rate_limit null", + json!({"display_name": "m", "provider": "openai", "model_name": "g", "provider_key_id": "pk-1", "rate_limit": null}), + ), + ( + "routing strategy null", + json!({"display_name": "r", "routing": {"targets": [{"model": "x"}], "strategy": null}}), + ), + ] { + reject(label, value); + } +} +🤖 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 `@crates/aisix-core/tests/model_schema_characterization.rs` around lines 253 - 280, Add a new reject test function following the pattern of reject_negative_timeout and reject_non_integer_timeout that explicitly tests rejecting null values for optional non-null fields. Create a test that calls the reject helper function with a JSON object where one of the optional fields (such as timeout or allowed_cidrs) is set to null, providing a description like "null timeout" or similar. This ensures that if someone accidentally re-widens the schema to allow nullable unions, the golden corpus test will catch it.
🤖 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 `@crates/aisix-core/src/models/model.rs`:
- Around line 168-176: The documentation comment for the `provider` field
contains issue-tracker context (`#417` bug class reference) that is not
appropriate for public API reference documentation. Remove the phrase about
re-creating the `#417` bug class from the comment for the `provider` field, and
keep only the neutral, functional explanation of why the dot character is
accepted in the vendor identity pattern. This ensures the documentation remains
focused on API usage rather than internal implementation history.
In `@crates/aisix-core/src/models/schema.rs`:
- Around line 356-368: The JSON schema validation rule for `object_store` when
`is_object_store` is true does not fully enforce the documented contract. The
"then" branch currently only constrains the provider field to ["s3", "gcs"] when
auth_mode is "cloud_identity", but it does not forbid the credential_ref field
as stated in the documentation. Update the "then" branch object in the allOf
array to also add a constraint that forbids credential_ref (e.g., using "not": {
"required": ["credential_ref"] }) so that the schema properly rejects payloads
containing both auth_mode: "cloud_identity" and credential_ref together,
matching the documented behavior that cloud_identity must not have
credential_ref.
In `@schemas/resources/guardrail_attachment.schema.json`:
- Around line 32-52: The schema currently allows scope_id to be null or omitted
for all scope_type values, but the description indicates it should only be null
when scope_type is Env. To fix this, modify the guardrail_attachment schema to
enforce that scope_id is required and non-null for non-env scope types (model,
api_key, team). Use JSON Schema conditional constraints (such as if-then
patterns based on the scope_type value) or oneOf/anyOf patterns to define
separate validation rules: one allowing nullable scope_id when scope_type is
Env, and another requiring scope_id as a non-null string when scope_type is
model, api_key, or team.
In `@schemas/resources/guardrail.schema.json`:
- Around line 518-522: The `created_at` field in the guardrail schema currently
defines `"type": "string"` which breaks backward compatibility by rejecting null
values that existed in prior payloads. Update the type definition to accept both
string and null values by changing `"type": "string"` to `"type": ["string",
"null"]` to maintain backward compatibility while keeping the RFC3339 date-time
format validation for non-null values.
In `@schemas/resources/model.schema.json`:
- Around line 519-522: Remove the internal implementation detail reference to
Resource::name() from the display_name field description in the source model
definition (not the generated schema file), keeping only the external behavior
that is relevant to API consumers. Update the description to only document what
users observe when using the display_name field through the public API
endpoints, then regenerate the schemas/resources/model.schema.json file from the
updated source model documentation.
In `@schemas/resources/observability_exporter.schema.json`:
- Around line 261-264: The credential_ref field in the
observability_exporter.schema.json schema accepts empty strings, which will fail
during credential resolution. Add a minLength constraint of 1 to the
credential_ref field definition to reject empty string values, ensuring
consistency with how SLS and Datadog credential fields handle this validation
requirement.
In `@schemas/resources/provider_key.schema.json`:
- Around line 43-70: The descriptions for default_body_fields, default_headers,
param_constraints, and param_renames properties expose internal implementation
details and helper function names that should not be in public API
documentation. Remove references to internal function names like
apply_default_body_fields, apply_default_headers, and apply_param_renames, as
well as implementation details like serde_json::Map and etcd round-trip from
these descriptions. Update the source model documentation (where this schema is
generated from) to use clear public API reference text that describes what these
fields do from a user perspective, then regenerate this schema file.
In `@schemas/resources/rate_limit_policy.schema.json`:
- Around line 17-62: The JSON schema for rate_limit_policy has two issues: the
enum descriptions for PolicyScope and PolicyWindow contain Rustdoc link syntax
(backtick-bracket notation like [`RateLimitPolicy`]) that should not appear in
public API documentation, and the main properties (max_requests, max_tokens,
name, scope, scope_ref, and window) lack descriptions entirely. Update the
source model documentation to remove the Rustdoc syntax from the PolicyScope and
PolicyWindow enum descriptions and add clear, user-facing descriptions for each
property that explain their purpose in the rate limit policy. After updating the
source model, regenerate the JSON schema from the model to ensure the schema
consumers receive proper API reference documentation.
---
Nitpick comments:
In `@crates/aisix-core/tests/model_schema_characterization.rs`:
- Around line 253-280: Add a new reject test function following the pattern of
reject_negative_timeout and reject_non_integer_timeout that explicitly tests
rejecting null values for optional non-null fields. Create a test that calls the
reject helper function with a JSON object where one of the optional fields (such
as timeout or allowed_cidrs) is set to null, providing a description like "null
timeout" or similar. This ensures that if someone accidentally re-widens the
schema to allow nullable unions, the golden corpus test will catch it.
In `@crates/aisix-core/tests/resource_schema_characterization.rs`:
- Around line 543-770: Add regression test cases to the guardrail_corpus
function to explicitly test created_at with a null value and document whether it
should pass or fail validation. Additionally, add test cases to the
guardrail_attachment_corpus function for non-env scope types (such as model,
team, and api_key) with missing or null scope_id values to ensure these
high-impact contract edges are properly validated and prevent future drift.
These edge cases should be documented with descriptive test names that clearly
indicate the expected validation behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4cfdbc41-c639-44cf-8ea5-34655037608e
📒 Files selected for processing (30)
crates/aisix-admin/src/openapi.rscrates/aisix-core/src/bin/dump-schema.rscrates/aisix-core/src/lib.rscrates/aisix-core/src/models/apikey.rscrates/aisix-core/src/models/cache_policy.rscrates/aisix-core/src/models/ensemble.rscrates/aisix-core/src/models/guardrail.rscrates/aisix-core/src/models/mod.rscrates/aisix-core/src/models/model.rscrates/aisix-core/src/models/observability_exporter.rscrates/aisix-core/src/models/provider_key.rscrates/aisix-core/src/models/rate_limit_policy.rscrates/aisix-core/src/models/routing.rscrates/aisix-core/src/models/schema.rscrates/aisix-core/tests/model_schema_characterization.rscrates/aisix-core/tests/resource_schema_characterization.rscrates/aisix-etcd/src/loader.rscrates/aisix-proxy/src/chat.rscrates/aisix-proxy/src/messages.rscrates/aisix-proxy/src/quota.rsschemas/resources/api_key.schema.jsonschemas/resources/cache_policy.schema.jsonschemas/resources/ensemble.schema.jsonschemas/resources/guardrail.schema.jsonschemas/resources/guardrail_attachment.schema.jsonschemas/resources/model.schema.jsonschemas/resources/observability_exporter.schema.jsonschemas/resources/provider_key.schema.jsonschemas/resources/rate_limit_policy.schema.jsonschemas/resources/routing.schema.json
…t edges Addresses review on #638: - ObjectStoreConfig.credential_ref regained minLength(1) (the hand-written obs union enforced it on the shared credential_ref; SLS/Datadog already had it, object_store was missed) — an empty credential_ref is rejected again instead of failing later at credential resolution. - Characterization corpus pins three contract edges raised in review: object_store empty credential_ref (reject), guardrail created_at:null (reject — the runtime validator always enforced non-null; cp-api omits it, never sends null), and non-env guardrail_attachment with null scope_id (accept — the validator never conditionally required scope_id).
This was referenced Jun 24, 2026
4 tasks
This was referenced Jul 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Each config resource had two independently hand-maintained schema representations in
aisix-core: the runtime validator (a hand-writtenjson!inmodels/schema.rs) and theschemars-derived struct (published toschemas/resources/*.jsonand vendored downstream). Nothing gated them against each other, so they drifted. The clearest symptom: the runtimerate_limitvalidator's$defslisted only 5 dimensions while theRateLimitstruct and the rate limiter (aisix-ratelimit) support 7 — so a model carryingrate_limit.rps/rphwas accepted by the published schema but silently dropped at the DP loader.Change
The struct is now the single source of truth for all 8 validated resources (model, api_key, provider_key, guardrail, guardrail_attachment, cache_policy, observability_exporter, rate_limit_policy). Each resource's runtime validator AND
dump-schemabuild from one*_root_schema()producer, so published == enforced by construction — the drift class is gone.schemarsattributes (length/range/regex/inner).schemarscan't derive are injected by the producer: model's direct/routing/ensembleoneOf, rate_limit_policy's at-least-one-ofanyOf, object_store's cloud-identityif/then/else.Stringfields became Rust enums where the consumer surface is small (PolicyScope/PolicyWindow,TelemetryKind). Guardrail's 6 moderation enums stayStringwith a producer-injectedenum(15+aisix-guardrailsread sites make conversion churn-heavy); converting them is a possible follow-up.schemarsdropsdeny_unknown_fieldsinside internally-taggedoneOfbranches, and serde doesn't enforce it there either — so the observability producer re-closes each branch withadditionalProperties:false(restoring plaintext-secret rejection) and the guardrail producer re-closes its 3 tagged sub-defs.Behavior changes (intended, documented per commit)
rate_limit.rps/rphnow accepted on model + api_key (previously advertised but silently dropped).datadogexporter carrying an otlpproject) now rejected; no valid config mixes kinds.scope/windownow rejected at deserialize instead of silently ignored.schemas/resources/guardrail_attachment.schema.json).The client-facing proxy data API is unchanged.
Tests
A characterization (golden-corpus) test was added per resource, written against the old validators (green before the refactor) and kept green after, proving the config contract is preserved except the flips above.
crates/aisix-core/tests/{model_schema,resource_schema}_characterization.rs.Compatibility / follow-ups
api7/AISIX-Cloudneed a re-sync (separate change).rps/rphalso needs a paired CP + dashboard change (CP's write-time validator still rejects them).Refs api7/AISIX-Cloud#788 (P0-1: configuration contract single source of truth).
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Fixes api7/AISIX-Cloud#788