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] A struct is needed to resemble the info section inside of … #6519

Closed buraksezer closed 2 months ago

buraksezer commented 2 months ago

User description

PR for https://tyktech.atlassian.net/browse/TT-13036

Adding a version for streams config It is expected that the configuration will evolve heavily in the feature when gathering feedback from users. Therefore a version will be important to allow breaking changes if needed while keeping old configurations possible.

For MVP the version can live inside an info block in the streams config section.

{
  "x-tyk-streaming": {
    "info": {
      "version": "1.0"
    },
    "streams": {
    }
  },
}

PR Type

enhancement


Description


Changes walkthrough 📝

Relevant files
Enhancement
mw_streaming.go
Refactor streaming configuration with new StreamsConfig struct

gateway/mw_streaming.go
  • Introduced a new StreamsConfig struct to encapsulate streaming
    configuration.
  • Modified the initStreams method to use the new StreamsConfig struct.
  • Updated the getStreamsConfig method to return a StreamsConfig pointer.
  • Improved logging and error handling for stream configuration
    processing.
  • +60/-44 

    💡 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: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    The error handling in `getStreamsConfig` method might lead to silent failures which can be hard to debug. Specifically, if JSON marshaling or unmarshaling fails, the method returns nil without logging the error. Consider adding error logs before returning nil in cases of failures. Code Duplication
    The logging for stream configuration changes appears to be duplicated in the `getStreamsConfig` method. This could be refactored to reduce redundancy and improve maintainability.
    github-actions[bot] commented 2 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Add type assertion for tykStreamingConfig to ensure it is a map before marshaling ___ **Use a type assertion to ensure that tykStreamingConfig is a map before marshaling
    it. This adds a layer of type safety, preventing potential panics if the type is not
    as expected.** [gateway/mw_streaming.go [211-216]](https://github.com/TykTechnologies/tyk/pull/6519/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R211-R216) ```diff -tykStreamingConfig, ok := s.Spec.OAS.T.Extensions[ExtensionTykStreaming] +tykStreamingConfig, ok := s.Spec.OAS.T.Extensions[ExtensionTykStreaming].(map[string]interface{}) if !ok { return nil } data, err := json.Marshal(tykStreamingConfig) ```
    Suggestion importance[1-10]: 9 Why: Adding a type assertion ensures type safety and prevents potential panics, which is a best practice for handling dynamic types in Go.
    9
    Possible bug
    Add a check for the 'streams' key in the tykStreamingConfig to prevent potential runtime errors ___ **Consider checking for the presence of the 'streams' key in the tykStreamingConfig
    before attempting to marshal it. This will prevent potential runtime errors if the
    'streams' key is missing.** [gateway/mw_streaming.go [211-216]](https://github.com/TykTechnologies/tyk/pull/6519/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R211-R216) ```diff tykStreamingConfig, ok := s.Spec.OAS.T.Extensions[ExtensionTykStreaming] -if !ok { +if !ok || tykStreamingConfig["streams"] == nil { return nil } data, err := json.Marshal(tykStreamingConfig) ```
    Suggestion importance[1-10]: 8 Why: This suggestion adds a necessary check for the presence of the 'streams' key, which can prevent runtime errors if the key is missing. It addresses a potential bug in the code.
    8
    Enhancement
    Log errors from JSON operations to aid in debugging ___ **To improve error handling, consider logging the error from json.Marshal and
    json.Unmarshal operations before returning nil. This will help in debugging and
    understanding why the marshaling or unmarshaling failed.** [gateway/mw_streaming.go [216-224]](https://github.com/TykTechnologies/tyk/pull/6519/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R216-R224) ```diff data, err := json.Marshal(tykStreamingConfig) if err != nil { + s.Logger().Errorf("Failed to marshal tykStreamingConfig: %v", err) return nil } streamsConfig := &StreamsConfig{} err = json.Unmarshal(data, streamsConfig) if err != nil { + s.Logger().Errorf("Failed to unmarshal into StreamsConfig: %v", err) return nil } ```
    Suggestion importance[1-10]: 7 Why: Logging errors from JSON operations improves error handling and aids in debugging, making it easier to identify issues during marshaling and unmarshaling.
    7
    Performance
    Optimize the check for r != nil by moving it outside the loop ___ **Instead of checking for r != nil inside the loop for each stream, perform this check
    once before the loop. This will make the code cleaner and more efficient by reducing
    unnecessary checks.** [gateway/mw_streaming.go [227-253]](https://github.com/TykTechnologies/tyk/pull/6519/files#diff-6f565750150d990575c808f1ca8f38483160dc6edf05f1534cd0bedb27c2e6c8R227-R253) ```diff -for streamID, stream := range streamsConfig.Streams { - if r != nil { +if r != nil { + for streamID, stream := range streamsConfig.Streams { s.Logger().Debugf("Stream config for %s: %v", streamID, stream) ... - } else { - s.Logger().Debugf("No request available to replace variables in stream config for %s", streamID) } +} else { + s.Logger().Debugf("No request available to replace variables in stream config") } ```
    Suggestion importance[1-10]: 6 Why: Moving the check for `r != nil` outside the loop improves code efficiency and readability by reducing unnecessary checks within the loop.
    6
    buger commented 2 months ago

    I believe for CI/CD use case, this field can be optional, right?

    buraksezer commented 2 months ago

    This PR should be reworked due to conflicts.