Closed titpetric closed 15 hours ago
โฑ๏ธ Estimated effort to review [1-5] | 3 |
๐งช Relevant tests | Yes |
๐ Security concerns | No |
โก Key issues to review |
Logic Change: The logic for updating rate limits has been refactored with the introduction of shouldUpdateRateLimit function. Ensure that the new logic correctly implements the intended behavior, especially in edge cases where rate limits might be set to -1 (unlimited). |
Conditional Updates: The conditions under which session limits are updated have changed. It's crucial to verify that these conditions do not introduce regressions or unexpected behaviors in scenarios where API limits are not updated but session limits are expected to be. |
API Changes
no api changes detected
Category | Suggestion | Score |
Possible bug |
Add nil checks before updating properties to prevent runtime errors___ **Consider adding a check fornil values for apiLimits and sessionLimits before attempting to update their properties. This will prevent potential runtime panics due to dereferencing nil pointers.** [internal/policy/apply.go [460-476]](https://github.com/TykTechnologies/tyk/pull/6376/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R460-R476) ```diff -if t.shouldUpdateRateLimit(policyLimits, *apiLimits) { +if apiLimits != nil && t.shouldUpdateRateLimit(policyLimits, *apiLimits) { apiLimits.Rate = policyLimits.Rate apiLimits.Per = policyLimits.Per apiLimits.Smoothing = policyLimits.Smoothing ... } ``` Suggestion importance[1-10]: 10Why: This suggestion prevents potential runtime panics due to dereferencing nil pointers, which is critical for ensuring the stability of the code. | 10 |
Add a check for negative durations to handle unexpected values safely___ **Consider handling the case wherepolicyLimits.Duration() might be negative, which could be an unintended scenario. This will ensure that the function behaves correctly even if unexpected values are passed.** [internal/policy/apply.go [444-445]](https://github.com/TykTechnologies/tyk/pull/6376/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R444-R445) ```diff -if policyLimits.Duration() == 0 { +if policyLimits.Duration() <= 0 { return } ``` Suggestion importance[1-10]: 9Why: This suggestion addresses a potential bug by ensuring that negative durations are handled, which improves the robustness of the function. | 9 | |
Maintainability |
Refactor the rate limit updating logic into a separate method to enhance code maintainability___ **To improve readability and maintainability, consider extracting the logic inside theif block starting at line 460 into a separate method. This method would handle updating the rate limits and could be named updateRateLimits .**
[internal/policy/apply.go [460-476]](https://github.com/TykTechnologies/tyk/pull/6376/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R460-R476)
```diff
if t.shouldUpdateRateRateLimit(policyLimits, *apiLimits) {
- apiLimits.Rate = policyLimits.Rate
- apiLimits.Per = policyLimits.Per
- apiLimits.Smoothing = policyLimits.Smoothing
- ...
+ t.updateRateLimits(apiLimits, policyLimits)
}
```
Suggestion importance[1-10]: 7Why: Extracting the logic into a separate method improves readability and maintainability, but it is not crucial for functionality. | 7 |
Enhancement |
Simplify the logical operations in
___
**The comparison logic in | 6 |
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code
User description
This reverts to previous behaviour when updating session limits so they are only updated if the api limits are updated first. The session holds the GlobalRateLimit as default value.
https://tyktech.atlassian.net/browse/TT-12492
PR Type
Bug fix, Enhancement
Description
emptyRateLimit
function withshouldUpdateRateLimit
.TestApplyRateLimits_PolicyLimits
to reflect the new logic for session rate limits.Changes walkthrough ๐
apply.go
Refactor rate limit application logic and conditional updates
internal/policy/apply.go
emptyRateLimit
function withshouldUpdateRateLimit
.updated.
apply_test.go
Update tests for rate limit application logic
internal/policy/apply_test.go
TestApplyRateLimits_PolicyLimits
to reflect new sessionrate limit logic.