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

Revert "[TT-12893]: Adding first implementation of streams API" #6509

Closed titpetric closed 2 months ago

titpetric commented 2 months ago

User description

Reverts TykTechnologies/tyk#6496


PR Type

Bug fix, Enhancement, Dependencies


Description


Changes walkthrough ๐Ÿ“

Relevant files
Enhancement
5 files
api_loader.go
Remove streaming middleware and update spec handling         

gateway/api_loader.go
  • Removed StreamingMiddleware from middleware chain.
  • Renamed specsToUnload to specsToRelease.
  • Changed method from Unload to Release.
  • +5/-9     
    middleware.go
    Simplify middleware interface and creation process             

    gateway/middleware.go
  • Removed GetSpec and Unload methods from TykMiddleware interface.
  • Removed AddUnloadHook call in createMiddleware.
  • +1/-15   
    api_definition.go
    Refactor API spec resource management                                       

    gateway/api_definition.go
  • Removed unloadHooks from APISpec.
  • Renamed Unload method to Release.
  • +1/-15   
    linter_test.go
    Improve loop readability in linter test                                   

    apidef/oas/linter_test.go - Added explicit index variable in loop.
    +1/-1     
    graphql_request.go
    Enhance loop clarity in GraphQL request                                   

    internal/graphql/graphql_request.go - Added explicit index variable in loop.
    +1/-1     
    Bug fix
    2 files
    mw_context_vars.go
    Update context variable enabling condition                             

    gateway/mw_context_vars.go - Changed `EnabledForSpec` to check `EnableContextVars`.
    +1/-8     
    graphql_request_test.go
    Fix type declaration in GraphQL stats test                             

    internal/graphql/graphql_request_test.go - Updated map initialization to use explicit type declaration.
    +2/-2     
    Dependencies
    1 files
    go.mod
    Update dependencies and clean up go.mod                                   

    go.mod
  • Downgraded google.golang.org/grpc version.
  • Removed several indirect dependencies.
  • +23/-307
    Additional files (token-limit)
    1 files
    go.sum
    ...                                                                                                           

    go.sum ...
    +65/-1134

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

    API Changes

    --- prev.txt    2024-09-12 16:42:34.973536516 +0000
    +++ current.txt 2024-09-12 16:42:03.085363500 +0000
    @@ -5155,10 +5155,6 @@
            LivenessCheck: LivenessCheckConfig{
                CheckDuration: time.Second * 10,
            },
    -       Streaming: StreamingConfig{
    -           Enabled:     false,
    -           AllowUnsafe: []string{},
    -       },
        }
     )
     var Global func() Config
    @@ -5769,10 +5765,6 @@
    
        // OAS holds the configuration for various OpenAPI-specific functionalities
        OAS OASConfig `json:"oas_config"`
    -
    -   Streaming StreamingConfig `json:"streaming"`
    -
    -   Labs labsConfig `json:"labs"`
     }
         Config is the configuration object used by Tyk to set up various parameters.
    
    @@ -6371,12 +6363,6 @@
     func (config *StorageOptionsConf) HostAddrs() (addrs []string)
         HostAddrs returns a sanitized list of hosts to connect to.
    
    -type StreamingConfig struct {
    -   Enabled     bool     `json:"enabled"`
    -   AllowUnsafe []string `json:"allow_unsafe"`
    -}
    -    Add this new struct definition
    -
     type Tracer struct {
        // The name of the tracer to initialize. For instance appdash, to use appdash tracer
        Name string `json:"name"`
    @@ -7655,9 +7641,6 @@
         The name for event handlers as defined in the API Definition JSON/BSON
         format
    
    -const (
    -   ExtensionTykStreaming = "x-tyk-streaming"
    -)
     const ListDetailed = "detailed"
     const LoopScheme = "tyk"
     const OIDPREFIX = "openid"
    @@ -7921,9 +7904,6 @@
    
     func CloneAPI(a *APISpec) *APISpec
    
    -func (s *APISpec) AddUnloadHook(hook func())
    -    AddUnloadHook adds a function to be called when the API spec is unloaded
    -
     func (a *APISpec) CheckSpecMatchesStatus(r *http.Request, rxPaths []URLSpec, mode URLStatus) (bool, interface{})
         CheckSpecMatchesStatus checks if a URL spec has a specific status.
         Deprecated: The function doesn't follow go return conventions (T, ok);
    @@ -7945,6 +7925,9 @@
    
     func (a *APISpec) Init(authStore, sessionStore, healthStore, orgStore storage.Handler)
    
    +func (s *APISpec) Release()
    +    Release releases all resources associated with API spec
    +
     func (a *APISpec) RequestValid(r *http.Request) (bool, RequestStatus)
         RequestValid will check if an incoming request has valid version data and
         return a RequestStatus that describes the status of the request
    @@ -7964,9 +7947,6 @@
     func (a *APISpec) URLAllowedAndIgnored(r *http.Request, rxPaths []URLSpec, whiteListStatus bool) (RequestStatus, interface{})
         URLAllowedAndIgnored checks if a url is allowed and ignored.
    
    -func (s *APISpec) Unload()
    -    Release releases all resources associated with API spec
    -
     func (s *APISpec) Validate(oasConfig config.OASConfig) error
         Validate returns nil if s is a valid spec and an error stating why the spec
         is not valid.
    @@ -8059,8 +8039,6 @@
         FireEvent is added to the BaseMiddleware object so it is available across
         the entire stack
    
    -func (t *BaseMiddleware) GetSpec() *APISpec
    -
     func (t *BaseMiddleware) Init()
    
     func (t *BaseMiddleware) Logger() (logger *logrus.Entry)
    @@ -8075,8 +8053,6 @@
    
     func (t *BaseMiddleware) SetRequestLogger(r *http.Request)
    
    -func (t *BaseMiddleware) Unload()
    -
     func (t *BaseMiddleware) UpdateRequestSession(r *http.Request) bool
    
     type BaseTykResponseHandler struct {
    @@ -10329,29 +10305,6 @@
    
     type StatsDSinkSanitizationFunc func(*bytes.Buffer, string)
    
    -type StreamManager struct {
    -   // Has unexported fields.
    -}
    -
    -type StreamingMiddleware struct {
    -   *BaseMiddleware
    -
    -   // Has unexported fields.
    -}
    -    StreamingMiddleware is a middleware that handles streaming functionality
    -
    -func (s *StreamingMiddleware) EnabledForSpec() bool
    -
    -func (s *StreamingMiddleware) Init()
    -    Init initializes the middleware
    -
    -func (s *StreamingMiddleware) Name() string
    -
    -func (s *StreamingMiddleware) ProcessRequest(w http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
    -    ProcessRequest will handle the streaming functionality
    -
    -func (s *StreamingMiddleware) Unload()
    -
     type StripAuth struct {
        *BaseMiddleware
     }
    @@ -10582,10 +10535,6 @@
        ProcessRequest(w http.ResponseWriter, r *http.Request, conf interface{}) (error, int) // Handles request
        EnabledForSpec() bool
        Name() string
    -
    -   GetSpec() *APISpec
    -
    -   Unload()
     }
    
     type TykOsinServer struct {
    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

    Refactoring Impact
    The removal of `StreamingMiddleware` and changes in spec management could impact existing functionalities. Ensure that these changes do not affect other middleware integrations and that all scenarios are covered by tests. Interface Simplification
    The removal of `GetSpec` and `Unload` from the `TykMiddleware` interface simplifies the interface. However, it's crucial to verify that no existing functionalities that rely on these methods are adversely affected. Method Removal
    The removal of `AddUnloadHook` and changes to the `Release` method could lead to potential resource management issues if not handled correctly across all API specs. Conditional Logic Change
    The change in the condition for enabling context variables from a static `true` to `m.Spec.EnableContextVars` could alter the behavior in existing deployments. This needs thorough testing to ensure it behaves as expected under all configurations. Dependency Changes
    The modifications in dependencies, especially the downgrade of `google.golang.org/grpc` and changes in indirect dependencies, should be carefully reviewed to avoid compatibility issues with other libraries or unexpected behaviors.
    github-actions[bot] commented 2 months ago

    PR Code Suggestions โœจ

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Update to the latest version of google.golang.org/grpc ___ **Consider using the latest version of google.golang.org/grpc to ensure compatibility
    and security updates. The PR downgrades from v1.64.1 to v1.64.0.** [go.mod [71]](https://github.com/TykTechnologies/tyk/pull/6509/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R71-R71) ```diff -google.golang.org/grpc v1.64.0 +google.golang.org/grpc v1.64.1 ```
    Suggestion importance[1-10]: 9 Why: The suggestion correctly identifies a potential issue with downgrading a dependency version, which could lead to compatibility or security issues. It is important to use the latest stable version unless there is a specific reason not to.
    9
    Use a newer or more stable version of github.com/imdario/mergo ___ **Ensure that the version of github.com/imdario/mergo used is the latest or a more
    stable version, as the PR reverts to an older version v0.3.12 from v0.3.13.** [go.mod [155]](https://github.com/TykTechnologies/tyk/pull/6509/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R155-R155) ```diff -github.com/imdario/mergo v0.3.12 // indirect +github.com/imdario/mergo v0.3.13 // indirect ```
    Suggestion importance[1-10]: 8 Why: The suggestion highlights a reversion to an older version of a dependency, which could introduce bugs or instability. Using the latest stable version is generally advisable for maintaining security and compatibility.
    8
    Possible bug
    Initialize slice to avoid nil slice runtime errors ___ **Ensure that specsToRelease is not nil before appending to avoid potential runtime
    panics.** [gateway/api_loader.go [999-1005]](https://github.com/TykTechnologies/tyk/pull/6509/files#diff-cdf0b7f176c9d18e1a314b78ddefc2cb3a94b3de66f1f360174692c915734c68R999-R1005) ```diff -var specsToRelease []*APISpec +specsToRelease := make([]*APISpec, 0) ... specsToRelease = append(specsToRelease, curSpec) ```
    Suggestion importance[1-10]: 8 Why: Initializing the slice prevents potential runtime errors related to nil slices, which is a significant improvement in terms of code robustness and reliability.
    8
    Maintainability
    Improve variable naming in the loop for better readability ___ **Replace the blank identifier _ with a meaningful variable name in the loop to
    enhance code readability and maintainability.** [apidef/oas/linter_test.go [81-82]](https://github.com/TykTechnologies/tyk/pull/6509/files#diff-b92239afd81e77a829fe7fe8410044dfd4dfda525d17dbf5f8811714a9c986d3R81-R82) ```diff -for idx, _ := range settings.Middleware.Operations { +for idx, operation := range settings.Middleware.Operations { settings.Middleware.Operations[idx].CircuitBreaker.Threshold = 0.5 } ```
    Suggestion importance[1-10]: 7 Why: The suggestion improves code readability by replacing the blank identifier with a meaningful variable name, which is a good practice for maintainability, although it does not affect functionality.
    7
    Consider removing unnecessary indirect dependency github.com/xeipuuv/gojsonpointer ___ **Verify the necessity of adding github.com/xeipuuv/gojsonpointer as an indirect
    dependency, as it might not be required if JSON pointer functionalities are not
    utilized in the project.** [go.mod [194]](https://github.com/TykTechnologies/tyk/pull/6509/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R194-R194) ```diff -github.com/xeipuuv/gojsonpointer v0.0.0-20190809123943-df4f5c81cb3b // indirect +# If not used, consider removing: +# github.com/xeipuuv/gojsonpointer v0.0.0-20190809123943-df4f5c81cb3b // indirect ```
    Suggestion importance[1-10]: 6 Why: The suggestion to verify the necessity of an indirect dependency is good for maintainability. However, without specific context on its usage, the impact of this suggestion is moderate. Removing unused dependencies can reduce bloat and potential security risks.
    6
    Enhancement
    Update go.opentelemetry.io/otel and related packages to the latest stable version ___ **Consider using the latest stable version of go.opentelemetry.io/otel and related
    packages to ensure compatibility with other dependencies and the latest features.** [go.mod [95-96]](https://github.com/TykTechnologies/tyk/pull/6509/files#diff-33ef32bf6c23acb95f5902d7097b7a1d5128ca061167ec0716715b0b9eeaa5f6R95-R96) ```diff -go.opentelemetry.io/otel v1.19.0 -go.opentelemetry.io/otel/trace v1.19.0 +go.opentelemetry.io/otel v1.24.0 +go.opentelemetry.io/otel/trace v1.24.0 ```
    Suggestion importance[1-10]: 7 Why: While updating to the latest version can provide new features and improvements, it is not always crucial unless there are known issues with the current version. The suggestion is valid but not critical.
    7
    sonarcloud[bot] commented 2 months ago

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

    Measures
    0 Security Hotspots
    0.0% Coverage on New Code
    0.0% Duplication on New Code

    See analysis details on SonarCloud