[TT-12618] Refactor/checkspec findspec

titpetric commented 1 month ago

User description

Adds a type safe function to get an *URL spec for the request;

https://tyktech.atlassian.net/browse/TT-12618

PR Type

Enhancement, Other

Description

Removed the CheckSpecMatchesStatus function from api_definition.go.
Added a new CheckSpecMatchesStatus function with a deprecation notice in model_apispec.go.
Introduced the FindSpecMatchesStatus function to replace the deprecated function.
Added helper functions getMatchPathAndMethod, modeSpecificSpec, and matchesMethod to modularize the code.

Changes walkthrough 📝

Relevant files

Enhancement

api_definition.go Remove `CheckSpecMatchesStatus` function from `APISpec` gateway/api_definition.go - Removed the `CheckSpecMatchesStatus` function.	+0/-118
model_apispec.go Add `FindSpecMatchesStatus` and helper functions gateway/model_apispec.go Added `CheckSpecMatchesStatus` function with deprecation notice. Added `FindSpecMatchesStatus` function. Added `getMatchPathAndMethod` helper function.	+77/-0
model_urlspec.go `Add helper functions for URLSpec status matching` gateway/model_urlspec.go Added `modeSpecificSpec` function. Added `matchesMethod` function.	+100/-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] commented 1 month ago

API Changes

--- prev.txt    2024-07-15 11:18:15.798292404 +0000
+++ current.txt 2024-07-15 11:18:12.766230365 +0000
@@ -7795,10 +7795,16 @@
 func CloneAPI(a *APISpec) *APISpec

 func (a *APISpec) CheckSpecMatchesStatus(r *http.Request, rxPaths []URLSpec, mode URLStatus) (bool, interface{})
-    CheckSpecMatchesStatus checks if a url spec has a specific status
+    CheckSpecMatchesStatus checks if a URL spec has a specific status.
+    Deprecated: The function doesn't follow go return conventions (T, ok);
+    use FindSpecMatchesStatus;

 func (a *APISpec) Expired() bool

+func (a *APISpec) FindSpecMatchesStatus(r *http.Request, rxPaths []URLSpec, mode URLStatus) (*URLSpec, bool)
+    FindSpecMatchesStatus checks if a URL spec has a specific status and returns
+    the URLSpec for it.
+
 func (s *APISpec) FireEvent(name apidef.TykEvent, meta interface{})

 func (a *APISpec) GetSessionLifetimeRespectsKeyExpiration() bool

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review

Deprecated Usage
The function `CheckSpecMatchesStatus` is marked as deprecated but is still being modified and used. It's generally a good practice to avoid making changes to deprecated functions unless it's part of a phase-out strategy. Deprecated Usage
The function `modeSpecificSpec` is marked as deprecated. Consider refactoring the existing code to eliminate the need for this function or replace it with a non-deprecated alternative.

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a default return value to prevent potential nil dereference ___ Consider checking if `vName` is not `nil` before dereferencing it to avoid potential runtime panics. [gateway/api_definition.go [1576-1577]](https://github.com/TykTechnologies/tyk/pull/6415/files#diff-0cf80174bbafb36f6d4f4308ebbd971b2833b76a936bad568220aa1a4ba0ee8bR1576-R1577) ```diff if vName := ctxGetVersionName(r); vName != nil { return *vName +} +return "" ``` Suggestion importance[1-10]: 10 Why: This suggestion addresses a potential runtime panic by ensuring a default return value is provided if `vName` is `nil`. This is a crucial fix for preventing unexpected crashes.	10
Enhancement	Replace deprecated function with the recommended alternative ___ Instead of using a deprecated function, refactor the code to use the recommended `FindSpecMatchesStatus` function. [gateway/model_apispec.go [9-10]](https://github.com/TykTechnologies/tyk/pull/6415/files#diff-80c49b9bdb411a3d5a4706ec3ff138ef44154d0306040c19eba1cb5559f199d6R9-R10) ```diff -// Deprecated: The function doesn't follow go return conventions (T, ok); use FindSpecMatchesStatus; -func (a APISpec) CheckSpecMatchesStatus(r http.Request, rxPaths []URLSpec, mode URLStatus) (bool, interface{}) { +// Use the recommended function with proper Go conventions +func (a APISpec) CheckSpecMatchesStatus(r http.Request, rxPaths []URLSpec, mode URLStatus) (*URLSpec, bool) { ``` Suggestion importance[1-10]: 8 Why: Refactoring to use the recommended `FindSpecMatchesStatus` function improves code quality and future-proofs the code by avoiding deprecated functions.	8
Maintainability	Extract repeated condition checks into a helper method to enhance code clarity ___ To improve readability and maintainability, consider extracting the repeated condition checks into a separate method. [gateway/model_apispec.go [14-22]](https://github.com/TykTechnologies/tyk/pull/6415/files#diff-80c49b9bdb411a3d5a4706ec3ff138ef44154d0306040c19eba1cb5559f199d6R14-R22) ```diff -if rxPaths[i].Status != mode { - continue -} -if !rxPaths[i].Spec.MatchString(matchPath) { - continue -} -if !rxPaths[i].matchesMethod(method) { +if !isValidPath(rxPaths[i], mode, matchPath, method) { continue } +// New helper method +func isValidPath(spec URLSpec, mode URLStatus, path, method string) bool { + return spec.Status == mode && spec.Spec.MatchString(path) && spec.matchesMethod(method) +} + ``` Suggestion importance[1-10]: 7 Why: This suggestion improves readability and maintainability by reducing code duplication, making the code easier to understand and maintain.	7
Performance	Use a map to handle mode-specific specifications for cleaner and more efficient code ___ Refactor the switch-case to use a map for cleaner and more efficient code, especially when dealing with many cases. [gateway/model_urlspec.go [6-50]](https://github.com/TykTechnologies/tyk/pull/6415/files#diff-ed766dd1814a557a1943cd3483c6ef511ad1b8febc7bd59982792d0ab7e23cffR6-R50) ```diff -switch mode { -case Ignored, BlackList, WhiteList: - return nil, true -case Cached: - return &u.CacheConfig, true -... +specMap := map[URLStatus]func() (interface{}, bool){ + Ignored: func() (interface{}, bool) { return nil, true }, + BlackList: func() (interface{}, bool) { return nil, true }, + WhiteList: func() (interface{}, bool) { return nil, true }, + Cached: func() (interface{}, bool) { return &u.CacheConfig, true }, + ... +} +if specFunc, ok := specMap[mode]; ok { + return specFunc() +} +return nil, false ``` Suggestion importance[1-10]: 6 Why: Using a map for mode-specific specifications can make the code cleaner and potentially more efficient, though the performance gain might be marginal. It mainly enhances code readability.	6

sonarcloud[bot] commented 1 month ago

Quality Gate failed

Failed conditions
67.1% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

TykTechnologies / tyk