add type safety for rate limit namespace and scope identifiers#1943
add type safety for rate limit namespace and scope identifiers#1943nXtCyberNet wants to merge 1 commit into
Conversation
Signed-off-by: nXtCyberNet <rohantech2005@gmail.com>
📝 WalkthroughWalkthroughThis PR introduces type-safe wrappers ( ChangesType Safety for Rate Limit Identifiers
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
tests/istio/extension_reconciler_test.go (1)
462-462:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winSix
Equalassertions comparestringagainstLimitNamespace— all will fail at runtime.The
Scopefield onwasm.Actionis typestring. Now thatLimitsNamespaceFromRoutereturnsLimitNamespace(a distinct named type), Gomega'sEqualusesreflect.DeepEqual, which returnsfalsewhenever the types differ — even if the underlying values match.Every struct-literal assignment was correctly updated with
string(...), but these sixExpectcalls in the "Full featured RLP targeting HTTPRoute" test were missed.🐛 Proposed fix (apply at all six sites)
- Expect(actionSet.Actions[1].Scope).To(Equal(controllers.LimitsNamespaceFromRoute(httpRoute))) + Expect(actionSet.Actions[1].Scope).To(Equal(string(controllers.LimitsNamespaceFromRoute(httpRoute))))Also applies to: 520-520, 578-578, 635-635, 693-693, 751-751
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/istio/extension_reconciler_test.go` at line 462, The test is comparing a string-typed Scope (actionSet.Actions[1].Scope) against a value of the named type controllers.LimitNamespace, causing Gomega Equal to fail due to type mismatch; update each failing Expect call in the "Full featured RLP targeting HTTPRoute" test (and the five other sites listed) to convert the named type to string by calling string(controllers.LimitsNamespaceFromRoute(httpRoute)) so the assertion compares identical string types (i.e., change Equal(controllers.LimitsNamespaceFromRoute(...)) to Equal(string(controllers.LimitsNamespaceFromRoute(...))) for the Expect checks referencing actionSet.Actions[n].Scope).
🧹 Nitpick comments (3)
internal/controller/ratelimit_workflow_helpers.go (2)
34-36: ⚡ Quick win
ToActionScope()conversion method stated in PR objectives is absent.The PR description explicitly lists: "Provide a conversion method
ToActionScope()onLimitNamespacefor explicit conversion toActionScope." The implementation instead uses ad-hoc direct casting (ActionScope(limitsNamespace)) at every call site. Adding the method would centralise the conversion, make the intent self-documenting, and match the documented API contract.💡 Suggested addition
type LimitNamespace string type ActionScope string + +// ToActionScope converts a LimitNamespace to an ActionScope for use in WASM action construction. +func (n LimitNamespace) ToActionScope() ActionScope { + return ActionScope(n) +}Call sites in
buildWasmActionsForRateLimitandbuildWasmActionsForTokenRateLimitcan then uselimitsNamespace.ToActionScope()instead ofActionScope(limitsNamespace).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/ratelimit_workflow_helpers.go` around lines 34 - 36, Add a conversion method on LimitNamespace named ToActionScope that returns ActionScope; replace ad-hoc casts ActionScope(limitsNamespace) with limitsNamespace.ToActionScope() in buildWasmActionsForRateLimit and buildWasmActionsForTokenRateLimit so callers use the explicit conversion method (declare func (n LimitNamespace) ToActionScope() ActionScope and update the two buildWasm... functions to call it).
375-422: ⚡ Quick win
buildWasmActionsForAnyRateLimitsilently erases theActionScopetype in itsactionFuncsignature.Line 381 keeps the
actionFuncscope parameter as a plainstring:actionFunc func(interface{}, string, string, string, kuadrantv1.WhenPredicates) wasm.Action,Line 417 converts
LimitNamespace → stringbefore passing it in, and then the caller's closure at Line 320 immediately re-wraps it asActionScope(scope). TheLimitNamespace → string → ActionScoperound-trip within the same logical call chain undermines the type safety this PR is intended to establish. Changing theactionFuncscope parameter toActionScopewould make the boundary explicit and eliminate the re-wrapping closure.♻️ Proposed refactor
func buildWasmActionsForAnyRateLimit( path []machinery.Targetable, rules map[string]kuadrantv1.MergeableRule, topLevelPredicatesKey string, policyPredicate func(machinery.Policy) bool, identifierFunc func(k8stypes.NamespacedName, string) string, - actionFunc func(interface{}, string, string, string, kuadrantv1.WhenPredicates) wasm.Action, + actionFunc func(interface{}, string, ActionScope, string, kuadrantv1.WhenPredicates) wasm.Action, ) []wasm.Action {And on line 417:
- scope := string(limitsNamespace) + scope := ActionScope(limitsNamespace)The
buildWasmActionsForRateLimitclosure can then remove theActionScope(scope)re-wrap:func(spec interface{}, limitIdentifier string, scope, sourcePolicyLocator string, predicates kuadrantv1.WhenPredicates) wasm.Action { limit := spec.(*kuadrantv1.Limit) - return wasmActionFromLimit(limit, limitIdentifier, ActionScope(scope), sourcePolicyLocator, predicates) + return wasmActionFromLimit(limit, limitIdentifier, scope, sourcePolicyLocator, predicates) },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/ratelimit_workflow_helpers.go` around lines 375 - 422, buildWasmActionsForAnyRateLimit is losing the ActionScope type by declaring actionFunc's scope parameter as string and converting LimitsNamespace to string before calling it; change the actionFunc signature to take kuadrantv1.ActionScope (or the ActionScope type you introduced) instead of string, keep limitsNamespace as an ActionScope (or cast it once from LimitsNamespaceFromRoute(parsed.GetRoute()) to ActionScope), pass that ActionScope directly into actionFunc when calling it, and update the caller closures (the buildWasmActionsForRateLimit closure that currently re-wraps ActionScope(scope)) to remove the redundant ActionScope(scope) re-wrap so the scope type boundary is explicit and preserved.internal/controller/ratelimit_workflow_helpers_test.go (1)
22-25: 💤 Low value
BeAssignableToTypeOfassertions are trivially true and add no safety signal.A value already declared as
LimitNamespaceorActionScopeis always assignable to its own type. These tests will pass even if the type system is broken. They should either be removed or replaced with tests that actually exercise the conversion boundary — e.g., verifyingToActionScope()(the explicit conversion method described in the PR objectives) returns the correct type and value.Also applies to: 31-34, 58-61
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/controller/ratelimit_workflow_helpers_test.go` around lines 22 - 25, Replace the meaningless BeAssignableToTypeOf assertions in the tests (e.g., the spec creating ns := LimitNamespace("mynamespace/myroute") and similar cases for ActionScope) with real conversion/value checks: call the explicit conversion method ToActionScope() or the relevant conversion function and Assert the returned type/value equals the expected ActionScope string/struct (use Expect(converted).To(Equal(expected)) or Expect(converted).To(BeAssignableToTypeOf(ActionScope(""))) plus an equality check); remove any redundant tests that only assert a value is assignable to its own declared type and apply the same replacement to the other occurrences referenced in the comment (the tests at the other two blocks).
🤖 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 `@tests/istio/extension_reconciler_test.go`:
- Line 462: The test is comparing a string-typed Scope
(actionSet.Actions[1].Scope) against a value of the named type
controllers.LimitNamespace, causing Gomega Equal to fail due to type mismatch;
update each failing Expect call in the "Full featured RLP targeting HTTPRoute"
test (and the five other sites listed) to convert the named type to string by
calling string(controllers.LimitsNamespaceFromRoute(httpRoute)) so the assertion
compares identical string types (i.e., change
Equal(controllers.LimitsNamespaceFromRoute(...)) to
Equal(string(controllers.LimitsNamespaceFromRoute(...))) for the Expect checks
referencing actionSet.Actions[n].Scope).
---
Nitpick comments:
In `@internal/controller/ratelimit_workflow_helpers_test.go`:
- Around line 22-25: Replace the meaningless BeAssignableToTypeOf assertions in
the tests (e.g., the spec creating ns := LimitNamespace("mynamespace/myroute")
and similar cases for ActionScope) with real conversion/value checks: call the
explicit conversion method ToActionScope() or the relevant conversion function
and Assert the returned type/value equals the expected ActionScope string/struct
(use Expect(converted).To(Equal(expected)) or
Expect(converted).To(BeAssignableToTypeOf(ActionScope(""))) plus an equality
check); remove any redundant tests that only assert a value is assignable to its
own declared type and apply the same replacement to the other occurrences
referenced in the comment (the tests at the other two blocks).
In `@internal/controller/ratelimit_workflow_helpers.go`:
- Around line 34-36: Add a conversion method on LimitNamespace named
ToActionScope that returns ActionScope; replace ad-hoc casts
ActionScope(limitsNamespace) with limitsNamespace.ToActionScope() in
buildWasmActionsForRateLimit and buildWasmActionsForTokenRateLimit so callers
use the explicit conversion method (declare func (n LimitNamespace)
ToActionScope() ActionScope and update the two buildWasm... functions to call
it).
- Around line 375-422: buildWasmActionsForAnyRateLimit is losing the ActionScope
type by declaring actionFunc's scope parameter as string and converting
LimitsNamespace to string before calling it; change the actionFunc signature to
take kuadrantv1.ActionScope (or the ActionScope type you introduced) instead of
string, keep limitsNamespace as an ActionScope (or cast it once from
LimitsNamespaceFromRoute(parsed.GetRoute()) to ActionScope), pass that
ActionScope directly into actionFunc when calling it, and update the caller
closures (the buildWasmActionsForRateLimit closure that currently re-wraps
ActionScope(scope)) to remove the redundant ActionScope(scope) re-wrap so the
scope type boundary is explicit and preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1eb15134-0e05-4a74-9121-a6867de81c13
📒 Files selected for processing (9)
internal/controller/limitador_limits_reconciler.gointernal/controller/ratelimit_workflow_helpers.gointernal/controller/ratelimit_workflow_helpers_test.gointernal/controller/ratelimit_workflow_test.gointernal/controller/tokenratelimit_workflow_test.gotests/common/ratelimitpolicy/ratelimitpolicy_controller_test.gotests/common/ratelimitpolicy/suite_test.gotests/envoygateway/extension_reconciler_test.gotests/istio/extension_reconciler_test.go
1 participant
feat: add type safety for rate limit namespace and scope identifiers
Define LimitNamespace and ActionScope as distinct named types wrapping
string to prevent accidental mixing of Limitador limit namespaces and
WASM action scopes.
Changes:
Closes #1916
Summary by CodeRabbit
Bug Fixes
Improvements