Add missing methods param to resource-wise rate limits#1220
Open
RakhithaRR wants to merge 2 commits intowso2:mainfrom
Open
Add missing methods param to resource-wise rate limits#1220RakhithaRR wants to merge 2 commits intowso2:mainfrom
RakhithaRR wants to merge 2 commits intowso2:mainfrom
Conversation
Contributor
WalkthroughAdded per-HTTP-method support to resource-level rate limiting by introducing a Methods field and associated enum in the API types, propagating that field through internal models, service mappers, deployment YAML generation, and the OpenAPI schema. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@platform-api/src/api/generated.go`:
- Around line 1938-1943: The struct field Methods (type
RateLimitingResourceLimitMethods) on the rate-limiting config has been annotated
with binding:"required", which makes omission of the methods key a breaking
change; either remove the binding:"required" tag from Methods in the generated
struct (or change validation to allow a missing/empty slice) to keep backwards
compatibility, or—if the intent is to enforce this change—add explicit release
notes and migration guidance calling out the breaking change for the Methods
field on the RateLimiting resource and update API docs and client examples;
locate the Methods field on the RateLimitingResource (or RateLimiting* generated
struct) to apply the change.
- Around line 269-279: Tests still reference the old unqualified constants
(api.GET, api.POST, api.DELETE, etc.) which were renamed to prefixed
identifiers; update every test occurrence to the new names (e.g., replace
api.GET -> RouteExceptionMethodsGET, api.POST -> RouteExceptionMethodsPOST,
api.DELETE -> RouteExceptionMethodsDELETE, and similarly for
HEAD/OPTIONS/PATCH/PUT/* using RouteExceptionMethodsHEAD,
RouteExceptionMethodsOPTIONS, RouteExceptionMethodsPATCH,
RouteExceptionMethodsPUT, RouteExceptionMethodsAsterisk) so the test code
imports and uses the newly generated constants (search for usages of
api.GET/api.POST/api.DELETE in the test files and perform the replacements).
In `@platform-api/src/internal/service/llm_deployment.go`:
- Around line 712-721: The code builds policyMethods from r.Methods but leaves
it empty when r.Methods is nil/empty, causing emitted methods: [] and breaking
legacy rate-limits; update the logic in the blocks that construct policyMethods
(the one using r.Methods and the similar block around lines 738-747) so that if
len(r.Methods) == 0 you set policyMethods to a single-entry slice containing the
wildcard method (convert "*" to api.LLMPolicyPathMethods), then pass that slice
into addOrAppendPolicyPath (the call that uses tokenBasedRateLimitPolicyName,
rateLimitPolicyVersion and api.LLMPolicyPath) so the resulting api.LLMPolicyPath
always has Methods ["*"] when no methods are specified.
In `@platform-api/src/internal/service/llm.go`:
- Around line 1524-1528: When converting model rate-limit entries to API
resources in the block that builds methods (the loop over r.Methods producing
methods []api.RateLimitingResourceLimitMethods and appending
api.RateLimitingResourceLimit{Resource: r.Resource, Methods: methods, ...}),
ensure you preserve prior semantics for records that lack a "methods" key by
falling back to ["*"] when r.Methods is nil or empty: if len(r.Methods) == 0
initialize methods to a single entry representing "*" before creating the
api.RateLimitingResourceLimit; this guarantees policyMethods downstream in
llm_deployment.go receives ["*"] rather than an empty slice and maintains
backward compatibility with pre-existing configs.
In `@platform-api/src/resources/openapi.yaml`:
- Around line 5294-5304: The schema currently requires the property "methods"
which breaks legacy clients; update the OpenAPI object schema so "methods" is
optional (remove it from required) and add a default of ["*"] plus validation
"minItems: 1" under the "methods" property, keeping the existing enum values;
also update any examples or the LLMRateLimitingConfig examples to either include
explicit methods arrays or reflect the new default ["*"] so examples remain
valid.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platform-api/src/internal/service/llm_deployment.go (1)
712-731:⚠️ Potential issue | 🟠 Major
addOrAppendPolicyPathdeduplicates by path only — same-path/different-method entries are silently dropped.The inner dedup guard in
addOrAppendPolicyPath(line 885) compares onlyexistingPath.Path. Now thatMethodscarries real per-resource semantics, two resources sharing the same path but with different method lists — e.g.:resources: - resource: /chat methods: [GET] limit: { token: { ... } } - resource: /chat methods: [POST] limit: { token: { ... } }will cause the second call to
addOrAppendPolicyPathto hit thereturnearly, silently discarding thePOSTrate-limit entry. The// TODO: Temporarycomment at line 883 predates per-method support; this gap is now directly user-visible.The dedup key should be
(path, methods)at minimum:🐛 Proposed fix in
addOrAppendPolicyPathfunc addOrAppendPolicyPath(policies *[]api.LLMPolicy, name, version string, path api.LLMPolicyPath) { for i := range *policies { if (*policies)[i].Name == name && (*policies)[i].Version == version { - // TODO: Temporary for _, existingPath := range (*policies)[i].Paths { - if existingPath.Path == path.Path { - // Keep first occurrence and avoid duplicates. + if existingPath.Path == path.Path && methodsEqual(existingPath.Methods, path.Methods) { + // Keep first occurrence and avoid duplicates. return } }Where
methodsEqualcompares two[]LLMPolicyPathMethodsslices (e.g., via a sorted-set comparison or a small helper).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-api/src/internal/service/llm_deployment.go` around lines 712 - 731, The dedup logic in addOrAppendPolicyPath currently ignores Methods and returns early for same Path, dropping different-method entries; update addOrAppendPolicyPath to treat the dedup key as (Path, Methods) instead of Path only by adding a helper (e.g., methodsEqual) that compares two []api.LLMPolicyPathMethods slices (either by sorting and comparing or by set membership) and use it alongside existingPath.Path when deciding whether to merge/return so that entries with the same path but different Methods are preserved (merge or append a new path entry when methods differ).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@platform-api/src/internal/service/llm_deployment.go`:
- Around line 712-731: The dedup logic in addOrAppendPolicyPath currently
ignores Methods and returns early for same Path, dropping different-method
entries; update addOrAppendPolicyPath to treat the dedup key as (Path, Methods)
instead of Path only by adding a helper (e.g., methodsEqual) that compares two
[]api.LLMPolicyPathMethods slices (either by sorting and comparing or by set
membership) and use it alongside existingPath.Path when deciding whether to
merge/return so that entries with the same path but different Methods are
preserved (merge or append a new path entry when methods differ).
---
Duplicate comments:
In `@platform-api/src/internal/service/llm_deployment.go`:
- Around line 739-763: The advanced rate-limit block correctly defaults
r.Methods to a wildcard but still needs the same deduplication fix as the
token-based block: when building policyMethods for addOrAppendPolicyPath (used
with advancedRateLimitPolicyName and rateLimitPolicyVersion and the
api.LLMPolicyPath entry), ensure you normalize and deduplicate method entries
(convert r.Methods into a set before appending, and only use ["*"] when
r.Methods is empty) so duplicate method strings are not added to policyMethods
or when merging into existing policy paths by addOrAppendPolicyPath.
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.
Purpose
$subject
Issue
https://github.com/wso2-enterprise/apim-saas/issues/2089
Summary by CodeRabbit
New Features
API Changes