Closed buger closed 2 months ago
β±οΈ Estimated effort to review: 3 π΅π΅π΅βͺβͺ |
π§ͺ No relevant tests |
π No security concerns identified |
β‘ Key issues to review Code Redundancy The function `extractPaths` is defined within `GetHTTPPaths` and contains redundant checks for string type conversion that could be simplified or extracted to a utility function to enhance code reusability and readability. Error Handling The function `extractPaths` uses default paths when the expected keys are missing or values are not strings, which could potentially hide configuration errors. Consider adding logging or error handling to notify when defaults are used. |
Category | Suggestion | Score |
Maintainability |
Introduce a generic helper function to handle type assertions___ **Replace the repeated checks for type assertion with a helper function to reduceredundancy and improve maintainability.** [gateway/mw_streaming.go [209-220]](https://github.com/TykTechnologies/tyk/pull/6535/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R209-R220) ```diff -brokerList, ok := brokerConfig.([]interface{}) +// Helper function to perform type assertion +func assertType[T any](item interface{}) (T, bool) { + result, ok := item.(T) + return result, ok +} +... +brokerList, ok := assertType[[]interface{}](brokerConfig) if !ok { continue } ... -brokerItemMap, ok := brokerItem.(map[string]interface{}) +brokerItemMap, ok := assertType[map[string]interface{}](brokerItem) if !ok { continue } ... -httpConfigMap, ok := httpServerConfig.(map[string]interface{}) +httpConfigMap, ok := assertType[map[string]interface{}](httpServerConfig) if !ok { continue } ``` Suggestion importance[1-10]: 8Why: Introducing a generic helper function for type assertions reduces code redundancy and improves maintainability, making the code cleaner and easier to manage. | 8 |
Improve code clarity by refactoring nested loops and conditionals into separate functions___ **Refactor the nested loops and conditionals for clarity and reduced complexity byextracting parts into separate functions.** [gateway/mw_streaming.go [194-229]](https://github.com/TykTechnologies/tyk/pull/6535/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R194-R229) ```diff for _, component := range components { + processComponent(component, streamConfig) +} +... +func processComponent(component string, streamConfig map[string]interface{}) { componentConfig, exists := streamConfig[component] if !exists { - continue + return } ... - if brokerConfig, exists := componentMap["broker"]; exists { + processBrokerOrHttpServer(componentConfig) +} +... +func processBrokerOrHttpServer(componentConfig map[string]interface{}) { + if brokerConfig, exists := componentConfig["broker"]; exists { ... - } else if httpServerConfig, exists := componentMap["http_server"]; exists { + } else if httpServerConfig, exists := componentConfig["http_server"]; exists { ... } } ``` Suggestion importance[1-10]: 6Why: Refactoring nested loops and conditionals into separate functions can improve code clarity and maintainability, but it is not as crucial as addressing redundancy or potential errors. | 6 | |
Enhancement |
Ensure uniqueness of paths by using a set___ **Use a set (implemented via a map) to ensure that the paths added are unique,avoiding potential duplicates in the output.** [gateway/mw_streaming.go [184-186]](https://github.com/TykTechnologies/tyk/pull/6535/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R184-R186) ```diff -paths = append(paths, defaultPaths[key]) +if _, exists := uniquePaths[defaultPaths[key]]; !exists { + paths = append(paths, defaultPaths[key]) + uniquePaths[defaultPaths[key]] = struct{}{} +} ... -paths = append(paths, pathStr) +if _, exists := uniquePaths[pathStr]; !exists { + paths = append(paths, pathStr) + uniquePaths[pathStr] = struct{}{} +} ``` Suggestion importance[1-10]: 7Why: Ensuring uniqueness of paths by using a set is a good enhancement that prevents potential duplicates, improving the correctness of the output. | 7 |
Best practice |
Add logging for type assertion failures to aid in debugging___ **Consider handling errors or unexpected types more robustly instead of silentlycontinuing, which could help in debugging and maintaining.** [gateway/mw_streaming.go [209-220]](https://github.com/TykTechnologies/tyk/pull/6535/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R209-R220) ```diff if !ok { + log.Printf("Error: expected type %T, got %T", expectedType, actualType) continue } ``` Suggestion importance[1-10]: 5Why: Adding logging for type assertion failures aids in debugging, but it is not critical for functionality and mainly improves the development process. | 5 |
/describe
@buger Sorry, but I do not understand!
PR Description updated to latest commit (https://github.com/TykTechnologies/tyk/commit/3bab1e319a0d5a1917c9c047a4862b116a36d44f)
User description
Ensure that it will work with broker input/ouput, e.g. :
Also remove hasHTTP, it does not needed because we can check if paths non empty. Additionally instead of just checking if path contains, brought back muxer.Match, by initialize defaultManager.muxer object.
PR Type
enhancement
Description
GetHTTPPaths
function to enhance support for broker input/output configurations.path
,ws_path
, andstream_path
when they are missing or not strings.Changes walkthrough π
mw_streaming.go
Refactor GetHTTPPaths to support broker configurations and default
paths
gateway/mw_streaming.go
GetHTTPPaths
function to handle broker configurations.configurations.