TykTechnologies / tyk

Tyk Open Source API Gateway written in Go, supporting REST, GraphQL, TCP and gRPC protocols
Other
9.76k stars 1.09k forks source link

[TT-13036] Fix bad streams config #6562

Closed buraksezer closed 2 months ago

buraksezer commented 2 months ago

User description

This PR fixes bad handling of stream configuration if *http.Request passed as nil in getStreamsConfig.

I also decided to return an initialized *StreamsConfig instead of nil to eliminate nil checks in other methods.


PR Type

Bug fix, Enhancement


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
mw_streaming.go
Improve stream configuration handling and initialization 

gateway/mw_streaming.go
  • Return initialized *StreamsConfig instead of nil.
  • Handle nil request by logging and continuing processing.
  • Ensure stream configuration is updated and unmarshaled correctly.
  • +22/-23 

    ๐Ÿ’ก 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 2 months ago

    PR Reviewer Guide ๐Ÿ”

    โฑ๏ธ Estimated effort to review: 2 ๐Ÿ”ต๐Ÿ”ตโšชโšชโšช
    ๐Ÿงช No relevant tests
    ๐Ÿ”’ No security concerns identified
    โšก Key issues to review

    Error Handling
    The error handling for JSON marshaling and unmarshaling in the `getStreamsConfig` method might be improved by adding specific actions or fallbacks when an error occurs, rather than just logging and continuing. Redundant Logging
    Consider reducing redundant logging within the same conditional block for stream configuration changes to improve performance and readability.
    github-actions[bot] commented 2 months ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a continue statement to skip processing when the request is nil ___ **Consider handling the case where r is nil before attempting to replace variables in
    the stream configuration. This will prevent potential nil pointer dereferences later
    in the code.** [gateway/mw_streaming.go [244]](https://github.com/TykTechnologies/tyk/pull/6562/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R244-R244) ```diff if r == nil { s.Logger().Debugf("No request available to replace variables in stream config for %s", streamID) -} else { - s.Logger().Debugf("Stream config for %s: %v", streamID, stream) - marshaledStream, err := json.Marshal(stream) - ... + continue } +s.Logger().Debugf("Stream config for %s: %v", streamID, stream) +marshaledStream, err := json.Marshal(stream) +... ```
    Suggestion importance[1-10]: 9 Why: The suggestion correctly identifies a potential bug where a nil request could lead to a nil pointer dereference. Adding a continue statement when `r` is nil is a crucial fix to prevent runtime errors.
    9
    Enhancement
    Include stream ID in error logs for better traceability ___ **To improve error handling, consider logging the stream ID along with the error
    message when marshaling or unmarshaling fails. This will help in identifying which
    stream caused the error more quickly.** [gateway/mw_streaming.go [250-264]](https://github.com/TykTechnologies/tyk/pull/6562/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R250-R264) ```diff if err != nil { - s.Logger().Errorf("Failed to marshal stream config: %v", err) + s.Logger().Errorf("Failed to marshal stream config for %s: %v", streamID, err) continue } ... if err != nil { - s.Logger().Errorf("Failed to unmarshal replaced stream config: %v", err) + s.Logger().Errorf("Failed to unmarshal replaced stream config for %s: %v", streamID, err) continue } ```
    Suggestion importance[1-10]: 8 Why: Including the stream ID in error logs enhances traceability and debugging, making it easier to identify which stream caused the error. This is a valuable improvement for maintainability and debugging.
    8
    Maintainability
    Use a boolean variable to store the comparison result for cleaner code ___ **Instead of checking replacedStream != string(marshaledStream) twice, store the
    result in a boolean variable to make the code cleaner and potentially improve
    performance by avoiding repeated conversions and comparisons.** [gateway/mw_streaming.go [255-258]](https://github.com/TykTechnologies/tyk/pull/6562/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R255-R258) ```diff -if replacedStream != string(marshaledStream) { +streamChanged := replacedStream != string(marshaledStream) +if streamChanged { s.Logger().Debugf("Stream config changed for %s: %s", streamID, replacedStream) } else { s.Logger().Debugf("Stream config has not changed for %s: %s", streamID, replacedStream) } ```
    Suggestion importance[1-10]: 7 Why: The suggestion improves code readability and maintainability by reducing redundancy. While not critical, it is a good practice to simplify the logic and avoid repeated operations.
    7
    Extract stream processing logic into a separate function for clarity ___ **Refactor the nested conditionals for better readability and maintainability by
    extracting the logic into a separate method or function.** [gateway/mw_streaming.go [241-272]](https://github.com/TykTechnologies/tyk/pull/6562/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R241-R272) ```diff if streamsMap, ok := extension.(map[string]any); ok { + processStreams(streamsMap) +} + +func processStreams(streamsMap map[string]any) { if streams, ok := streamsMap["streams"].(map[string]any); ok { for streamID, stream := range streams { ... } } } ```
    Suggestion importance[1-10]: 6 Why: Extracting the nested logic into a separate function can improve code readability and maintainability. However, the impact is moderate as it primarily addresses code organization rather than functionality.
    6