fix(schema): add team/owner rate-limit fields to apikey JSON schema - #269
Conversation
PR #267 added team_id, team_rate_limit, owner_id, and owner_rate_limit to the ApiKey Rust struct but did not update the JSON schema validator. Since the schema uses additionalProperties: false, any payload containing these fields is rejected at schema validation time before serde even sees it. Also updates the unknown-field test to use a truly unknown field name instead of max_budget_usd (which was dropped in #259).
📝 WalkthroughWalkthroughThe PR extends the API key schema in ChangesAPI Key Schema Extension
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aisix-core/src/models/schema.rs (1)
201-204: ⚡ Quick winConsider adding
minLength: 1validation to ID fields.The
team_idandowner_idfields accept any string, including empty strings. Other identifier fields in this schema (e.g.,key_hashat line 195) and across other schemas (e.g.,display_name,model_name,provider_key_idat lines 111-114) enforceminLength: 1to prevent empty values. Empty identifiers are semantically invalid and could cause issues downstream.📏 Proposed validation addition
- "team_id": { "type": "string" }, + "team_id": { "type": "string", "minLength": 1 }, "team_rate_limit": { "$ref": "#/$defs/rate_limit" }, - "owner_id": { "type": "string" }, + "owner_id": { "type": "string", "minLength": 1 }, "owner_rate_limit": { "$ref": "#/$defs/rate_limit" }🤖 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/src/models/schema.rs` around lines 201 - 204, Add non-empty string validation to the ID fields by adding minLength: 1 to the "team_id" and "owner_id" entries in the schema so they cannot be empty; locate the definitions for "team_id" and "owner_id" in the struct/schema (the entries currently are "team_id": { "type": "string" } and "owner_id": { "type": "string" }) and change each to include "minLength": 1 consistent with other identifiers like "key_hash" and "display_name".
🤖 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.
Nitpick comments:
In `@crates/aisix-core/src/models/schema.rs`:
- Around line 201-204: Add non-empty string validation to the ID fields by
adding minLength: 1 to the "team_id" and "owner_id" entries in the schema so
they cannot be empty; locate the definitions for "team_id" and "owner_id" in the
struct/schema (the entries currently are "team_id": { "type": "string" } and
"owner_id": { "type": "string" }) and change each to include "minLength": 1
consistent with other identifiers like "key_hash" and "display_name".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d672ddb2-ff5c-4b31-a4ad-ba04d11ff37d
📒 Files selected for processing (1)
crates/aisix-core/src/models/schema.rs
There was a problem hiding this comment.
Pull request overview
Fixes ApiKey JSON schema validation in aisix-core to accept the team_id/owner_id and corresponding *_rate_limit fields introduced in #267. This is necessary because the apikey schema uses additionalProperties: false, so previously-valid payloads were being rejected before serde deserialization.
Changes:
- Extended the
apikeyJSON schema to includeteam_id,team_rate_limit,owner_id, andowner_rate_limit. - Added a unit test asserting apikey payloads with the new fields validate successfully.
- Updated the unknown-field schema test to use
bogus_fieldinstead ofmax_budget_usd(removed in #259).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Prevents empty-string IDs from creating shared limiter buckets.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aisix-core/src/models/schema.rs (1)
512-523: ⚡ Quick winAdd explicit regression tests for empty
team_id/owner_id.You added
minLength: 1constraints, but there’s no test that proves empty strings are rejected. Add one to lock in the limiter-bucket safety guarantee.✅ Suggested test addition
+ #[test] + fn apikey_empty_team_or_owner_id_rejected() { + let with_empty_team = json!({ + "key_hash":"9df37f5e7cbc3c391d872742b5f286c242e733a09add9eeaa4d26a599bd90b20", + "allowed_models":["gpt-4o"], + "team_id": "", + "owner_id": "member-uuid-1" + }); + assert!(validate_apikey(&with_empty_team).is_err()); + + let with_empty_owner = json!({ + "key_hash":"9df37f5e7cbc3c391d872742b5f286c242e733a09add9eeaa4d26a599bd90b20", + "allowed_models":["gpt-4o"], + "team_id": "team-uuid-1", + "owner_id": "" + }); + assert!(validate_apikey(&with_empty_owner).is_err()); + }🤖 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/src/models/schema.rs` around lines 512 - 523, Add regression tests that assert empty "team_id" and empty "owner_id" are rejected by the schema: create two new tests (e.g., apikey_with_empty_team_id_fails and apikey_with_empty_owner_id_fails) that build the same JSON shape used in apikey_with_team_and_owner_fields_passes but set "team_id": "" and "owner_id": "" respectively, then call validate_apikey(&v) and assert it returns an error (use unwrap_err() or assert!(validate_apikey(&v).is_err())). This ensures the minLength: 1 constraint enforced by validate_apikey fails for empty strings.
🤖 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.
Nitpick comments:
In `@crates/aisix-core/src/models/schema.rs`:
- Around line 512-523: Add regression tests that assert empty "team_id" and
empty "owner_id" are rejected by the schema: create two new tests (e.g.,
apikey_with_empty_team_id_fails and apikey_with_empty_owner_id_fails) that build
the same JSON shape used in apikey_with_team_and_owner_fields_passes but set
"team_id": "" and "owner_id": "" respectively, then call validate_apikey(&v) and
assert it returns an error (use unwrap_err() or
assert!(validate_apikey(&v).is_err())). This ensures the minLength: 1 constraint
enforced by validate_apikey fails for empty strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 966a762f-7166-42e4-b292-4d52be9c12a7
📒 Files selected for processing (1)
crates/aisix-core/src/models/schema.rs
PR #267 added
team_id,team_rate_limit,owner_id, andowner_rate_limitto theApiKeyRust struct but missed updating the JSON schema validator inschema.rs. Since the apikey schema usesadditionalProperties: false, any payload with these fields gets rejected at validation time before serde deserialization.This adds the four fields to the apikey JSON schema and updates the unknown-field test to use a truly unknown field name (
bogus_field) instead ofmax_budget_usd(dropped in #259).Blocks api7/AISIX-Cloud#271 (CP e2e fails because the dev image rejects
owner_id).Summary by CodeRabbit
New Features
Tests