Merging to release-5.4.0: [TT-12454] Extract ApplyPolicies into internal/policy scope (#6367)

[buger]

User description

TT-12454 Extract ApplyPolicies into internal/policy scope (#6367)

User description

This extracts a large problematic ApplyPolicies function into it's own package scope. It does this by:

• defining an interface for policy access and deleting inline mutex use
• updated *Gateway to implement storage interface
• implement storage interface in policy.Store as well (used inline)

On top of that:

• Fixes a bug in the Duration/Less() functions on user.APILimit, fixes tests
• Adds a test particular to ApplyRateLimits (decrease coginitive complexity)

The duration was calculated as rate/per, however, the correct way was per/rate; This fixes it so duration is calculated correctly, fixing the Less function comparison.

---

PR Type

Enhancement, Bug fix

---

Description

• Refactored handleGetPolicy to use the new PolicyByID method.
• Introduced a Repository interface and added methods PolicyIDs, PolicyByID, and PolicyCount to the Gateway struct.
• Refactored ApplyPolicies in BaseMiddleware to use the new policy package.
• Updated buildNodeInfo to use PolicyCount instead of policiesByIDLen.
• Removed redundant methods getPolicy and policiesByIDLen from Gateway.

---

Changes walkthrough 📝

Relevant files

Enhancement

api.go Refactor policy retrieval in `handleGetPolicy`.                    gateway/api.go - Replaced `getPolicy` with `PolicyByID` in `handleGetPolicy`. +1/-1      gateway.go Add policy-related methods and interface to Gateway.          gateway/gateway.go Introduced Repository interface. Added methods PolicyIDs, PolicyByID, and PolicyCount. +38/-0    middleware.go Refactor ApplyPolicies to use new policy store.                    gateway/middleware.go Removed clearSession method. Refactored ApplyPolicies to use policy.New and store.Apply. +3/-411  rpc_storage_handler.go Update policy count retrieval in buildNodeInfo.                    gateway/rpc_storage_handler.go - Replaced `policiesByIDLen` with `PolicyCount` in `buildNodeInfo`. +1/-1      server.go Remove redundant policy methods from Gateway.                        gateway/server.go - Removed `getPolicy` and `policiesByIDLen` methods. +0/-13

---

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

---

Co-authored-by: Tit Petric tit@tyk.io

---

PR Type

Enhancement, Bug fix, Tests

---

Description

• Refactored handleGetPolicy to use the new PolicyByID method.
• Introduced a Repository interface and added methods PolicyIDs, PolicyByID, and PolicyCount to the Gateway struct.
• Refactored ApplyPolicies in BaseMiddleware to use the new policy package.
• Updated buildNodeInfo to use PolicyCount instead of policiesByIDLen.
• Removed redundant methods getPolicy and policiesByIDLen from Gateway.
• Fixed the Duration method in APILimit and removed the Less method.
• Added tests for the new policy package and updated existing tests.
• Added a Taskfile for running tests and checking coverage.

---

Changes walkthrough 📝

	Relevant files
Enhancement	10 files api.go Refactor `handleGetPolicy` to use `PolicyByID`                      gateway/api.go - Refactored `handleGetPolicy` to use `PolicyByID` method. +1/-1      gateway.go Introduce `Repository` interface and methods in `Gateway` gateway/gateway.go Introduced Repository interface. Added PolicyIDs, PolicyByID, and PolicyCount methods to Gateway. +38/-0    middleware.go Refactor `ApplyPolicies` to use new `policy` package          gateway/middleware.go Refactored ApplyPolicies to use the new policy package. Removed clearSession and inlined its logic into the new package. +7/-411  mw_rate_limiting.go Update error message in `handleRateLimitFailure`                  gateway/mw_rate_limiting.go - Updated error message in `handleRateLimitFailure`. +1/-1      rpc_storage_handler.go Refactor `buildNodeInfo` to use `PolicyCount`                        gateway/rpc_storage_handler.go - Refactored `buildNodeInfo` to use `PolicyCount` method. +1/-1      server.go Remove redundant methods from `Gateway`                                    gateway/server.go - Removed `getPolicy` and `policiesByIDLen` methods. +0/-13    event.go Remove `RateLimitExceeded` event from `eventMap`                  internal/event/event.go - Removed `RateLimitExceeded` event from `eventMap`. +0/-1      apply.go Introduce `Service` struct and refactor policy application internal/policy/apply.go Introduced Service struct for policy application. Moved ApplyPolicies and ClearSession logic to Service. Added ApplyRateLimits method. +478/-0  store.go Introduce `Store` struct for in-memory policy storage        internal/policy/store.go - Introduced `Store` struct for in-memory policy storage. +35/-0    util.go Add utility functions for policy application                          internal/policy/util.go - Added utility functions for policy application. +129/-0
Tests	5 files policy_test.go Update tests to use `PolicyByID` method                                    gateway/policy_test.go Updated tests to use PolicyByID method. Added assertions for PolicyByID. +17/-4    server_test.go Update tests to use `PolicyCount` method                                  gateway/server_test.go - Updated tests to use `PolicyCount` method. +1/-1      event_test.go Update test to use `RateLimitSmoothingUp` event                    internal/event/event_test.go - Updated test to use `RateLimitSmoothingUp` event. +1/-1      apply_test.go Add tests for `ApplyRateLimits` and `ApplyPolicies`            internal/policy/apply_test.go - Added tests for `ApplyRateLimits` and `ApplyPolicies`. +124/-0  session_test.go Update tests for `Duration` method and remove `Less` method tests user/session_test.go Updated tests for Duration method. Removed tests for Less method. +1/-39
Bug fix	1 files session.go Fix `Duration` method and remove `Less` method in `APILimit` user/session.go Fixed Duration method in APILimit. Removed Less method from APILimit. +1/-6
Configuration changes	1 files Taskfile.yml Add Taskfile for running tests and coverage                            internal/policy/Taskfile.yml - Added Taskfile for running tests and coverage. +40/-0

---

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[github-actions[bot]]

API Changes

--- prev.txt    2024-06-26 09:19:53.557602332 +0000
+++ current.txt 2024-06-26 09:19:50.317566233 +0000
@@ -8234,6 +8234,12 @@

 func (gw *Gateway) NotifyCurrentServerStatus()

+func (gw *Gateway) PolicyByID(polID string) (user.Policy, bool)
+
+func (gw *Gateway) PolicyCount() int
+
+func (gw *Gateway) PolicyIDs() []string
+
 func (gw *Gateway) ProcessOauthClientsOps(clients map[string]string)
     ProcessOauthClientsOps performs the appropriate action for the received
     clients it can be any of the Create,Update and Delete operations
@@ -9522,6 +9528,10 @@
     TickOk triggers a reload and ensures a queue happened and a reload cycle
     happens. This will block until all the cases are met.

+type Repository interface {
+   policy.Repository
+}
+
 type RequestDefinition struct {
    Method      string            `json:"method"`
    Headers     map[string]string `json:"headers"`
@@ -12032,10 +12042,6 @@

 func (limit APILimit) IsEmpty() bool

-func (g *APILimit) Less(in APILimit) bool
-    Less will return true if the receiver has a smaller duration between
-    requests than `in`.
-
 type AccessDefinition struct {
    APIName              string                  `json:"api_name" msg:"api_name"`
    APIID                string                  `json:"api_id" msg:"api_id"`

[github-actions[bot]]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	4
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The refactoring of the ApplyPolicies function into the new policy package should be carefully reviewed to ensure that all previous functionalities are preserved and correctly implemented. The changes are extensive and involve multiple components, which increases the risk of introducing bugs or regressions.
	Performance Concern: The new implementation introduces additional layers of abstraction and interface calls (e.g., Repository and Service in the policy package). It's important to assess the impact on performance, especially in high-load environments.
	Code Complexity: The new policy package introduces a significant amount of new code and logic. Reviewers should ensure that the code is maintainable and well-documented, particularly the parts handling policy application logic and session modifications.

[github-actions[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Enhancement	Improve error message specificity for better debugging ___ **Consider using a more specific error message in fmt.Errorf("policy not found: %s", polID) by including additional context about the error, such as the function or scenario in which it occurred.** [internal/policy/apply.go [44]](https://github.com/TykTechnologies/tyk/pull/6371/files#diff-59b92e9d31f142f1d99b746eb3ff7db4e26bf6c3044c9b87b58034a947ee04d1R44-R44) ```diff -return fmt.Errorf("policy not found: %s", polID) +return fmt.Errorf("in ClearSession, policy not found: %s", polID) ``` Suggestion importance[1-10]: 9 Why: Providing more specific error messages greatly aids in debugging and understanding the context of the error, which is crucial for maintaining and troubleshooting the code.	9
	Simplify the handling of orgID by using a direct string assignment ___ **Instead of using a pointer for orgID, consider using a default value or handling the nil case explicitly to simplify the logic and avoid indirect references.** [gateway/middleware.go [353-355]](https://github.com/TykTechnologies/tyk/pull/6371/files#diff-703054910891a4db633eca0f42ed779d6b4fa75cd9b3aa4c503e681364201c1bR353-R355) ```diff -var orgID *string +orgID := "" if t.Spec != nil { - orgID = &t.Spec.OrgID + orgID = t.Spec.OrgID } ``` Suggestion importance[1-10]: 7 Why: This suggestion simplifies the code and avoids the complexity of indirect references, improving readability and maintainability.	7
Error handling	Enhance error handling by checking the return value of the Apply method ___ **To improve error handling, consider adding a check for the success of the Apply method and handle the error appropriately.** [gateway/middleware.go [357-358]](https://github.com/TykTechnologies/tyk/pull/6371/files#diff-703054910891a4db633eca0f42ed779d6b4fa75cd9b3aa4c503e681364201c1bR357-R358) ```diff store := policy.New(orgID, t.Gw, log) -return store.Apply(session) +err := store.Apply(session) +if err != nil { + return err +} +return nil ``` Suggestion importance[1-10]: 9 Why: This suggestion enhances error handling by ensuring that any errors from the `Apply` method are properly checked and handled, which is important for robustness.	9
Possible bug	Add error handling for nil t.Spec to prevent runtime errors ___ **Consider handling the case where t.Spec is nil before using it to avoid potential nil pointer dereferences.** [gateway/middleware.go [354-355]](https://github.com/TykTechnologies/tyk/pull/6371/files#diff-703054910891a4db633eca0f42ed779d6b4fa75cd9b3aa4c503e681364201c1bR354-R355) ```diff -if t.Spec != nil { - orgID = &t.Spec.OrgID +if t.Spec == nil { + return errors.New("specification is undefined"), http.StatusBadRequest } +orgID = &t.Spec.OrgID ``` Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential runtime error by adding error handling for a nil `t.Spec`, which is crucial for preventing crashes.	8
Maintainability	Maintain method naming consistency and usage pattern ___ **Replace the call to r.Gw.PolicyCount() with r.Gw.policiesByIDLen() to maintain consistency with the existing method naming and usage pattern in the codebase.** [gateway/rpc_storage_handler.go [174]](https://github.com/TykTechnologies/tyk/pull/6371/files#diff-8875f75b602664c44b62b67a4da41d748124ad270573a44db4ec977ee5d68021R174-R174) ```diff -PoliciesCount: r.Gw.PolicyCount(), +PoliciesCount: r.Gw.policiesByIDLen(), ``` Suggestion importance[1-10]: 8 Why: The suggestion ensures consistency in method naming and usage patterns, which is important for maintainability and readability of the codebase.	8
	Ensure method naming consistency across the codebase ___ **Replace the call to ts.Gw.PolicyCount() with ts.Gw.policiesByIDLen() to ensure consistency with the existing method naming and usage pattern in the codebase.** [gateway/server_test.go [156]](https://github.com/TykTechnologies/tyk/pull/6371/files#diff-d9f006370c9748c09affd99d0a4edeb8f3419057703a67fd70838a764a485696R156-R156) ```diff -actual := ts.Gw.PolicyCount() +actual := ts.Gw.policiesByIDLen() ``` Suggestion importance[1-10]: 8 Why: This suggestion promotes consistency in method naming, which helps in maintaining a uniform codebase and reduces potential confusion.	8
	Refactor test setup into a separate helper function ___ **It's a good practice to separate the creation of test data from the test logic. Consider refactoring the test setup (creation of session, apiLimits, and policy) into a separate helper function to improve test readability and reusability.** [internal/policy/apply_test.go [16-24]](https://github.com/TykTechnologies/tyk/pull/6371/files#diff-5af7e299a6b0ce11e22f8aa4a01854b1151f4b54dccc68f0cd1cbedee5aed7c8R16-R24) ```diff -session := &user.SessionState{ - Rate: 5, - Per: 10, -} -apiLimits := user.APILimit{ - Rate: 10, - Per: 10, -} -policy := user.Policy{} +session, apiLimits, policy := setupTestData(5, 10, 10, 10) ``` Suggestion importance[1-10]: 6 Why: Refactoring the test setup into a helper function improves code readability and reusability, making the tests easier to maintain and understand.	6
Best practice	Add error handling check for ApplyRateLimits function calls ___ **Consider adding a check for the return value of ApplyRateLimits in your test cases to ensure that the function behaves as expected not only in terms of output but also in terms of its success or failure state.** [internal/policy/apply_test.go [26]](https://github.com/TykTechnologies/tyk/pull/6371/files#diff-5af7e299a6b0ce11e22f8aa4a01854b1151f4b54dccc68f0cd1cbedee5aed7c8R26-R26) ```diff -svc.ApplyRateLimits(session, policy, &apiLimits) +err := svc.ApplyRateLimits(session, policy, &apiLimits) +assert.NoError(t, err) ``` Suggestion importance[1-10]: 8 Why: Adding error handling checks ensures that the function not only produces the expected output but also operates without errors, which is crucial for robust testing.	8
Possible issue	Maintain original functionality unless change is justified ___ **Replace the call to String(RateLimitSmoothingUp) with String(RateLimitExceeded) to maintain the original functionality unless there is a specific reason for the change.** [internal/event/event_test.go [17]](https://github.com/TykTechnologies/tyk/pull/6371/files#diff-441a34fa81121d95f8fad2cbaddca97199411a8fad7b3e2f3df5be69acf1a94aR17-R17) ```diff -s := String(RateLimitSmoothingUp) +s := String(RateLimitExceeded) ``` Suggestion importance[1-10]: 7 Why: The suggestion aims to maintain the original functionality, which is generally a good practice unless there is a specific reason for the change. However, the impact is relatively minor.	7
	Verify that the Per field remains unchanged after applying rate limits ___ **To ensure that the ApplyRateLimits method does not modify the Per field unexpectedly, add assertions to check the Per value remains unchanged after method execution.** [internal/policy/apply_test.go [26]](https://github.com/TykTechnologies/tyk/pull/6371/files#diff-5af7e299a6b0ce11e22f8aa4a01854b1151f4b54dccc68f0cd1cbedee5aed7c8R26-R26) ```diff svc.ApplyRateLimits(session, policy, &apiLimits) +assert.Equal(t, 10, int(apiLimits.Per)) +assert.Equal(t, 10, int(session.Per)) ``` Suggestion importance[1-10]: 7 Why: Ensuring that the `Per` field remains unchanged helps verify the integrity of the function's behavior, which is important for maintaining expected functionality.	7
Performance	Optimize PolicyIDs method by preallocating slice space ___ **To enhance the performance of PolicyIDs, consider using a slice preallocated with the exact size of s.policies instead of appending to an empty slice, which can cause multiple memory allocations if the map is large.** [internal/policy/store.go [21-24]](https://github.com/TykTechnologies/tyk/pull/6371/files#diff-13dec7bc453c9ff99550c83d2f86a017bbf7fb863584dc30603af15d29ef9d3dR21-R24) ```diff -policyIDs := make([]string, 0, len(s.policies)) +policyIDs := make([]string, len(s.policies)) +i := 0 for _, val := range s.policies { - policyIDs = append(policyIDs, val.ID) + policyIDs[i] = val.ID + i++ } ``` Suggestion importance[1-10]: 5 Why: Preallocating the slice space can enhance performance by reducing the number of memory allocations, although the performance gain might be minor unless dealing with a large map.	5

[sonarcloud[bot]]

Quality Gate failed

Failed conditions
30.0% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint