[TT-12698] Add linter to gateway to check for named scope conflicts

titpetric commented 1 month ago

PR Type

Enhancement, Tests

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

Description

Renamed imports for internal GraphQL and temporal model packages to avoid named scope conflicts.
Updated usage of renamed imports in gateway/handler_success.go and gateway/redis_signals.go.
Added a new linter task lint:gateway to the Taskfile.yml to run go-fsck lint in the gateway directory.
Updated lint:all task in Taskfile.yml to include lint:gateway as a dependency.

Changes walkthrough 📝

Relevant files

Enhancement

handler_success.go `Rename internal GraphQL package import and update usage` gateway/handler_success.go Renamed import for internal GraphQL package to `graphqlInternal`. Updated usage of `graphql` to `graphqlInternal` for `NewGraphStatsExtractor`.	+2/-2
redis_signals.go `Rename temporal model package import and update usage` gateway/redis_signals.go Renamed import for temporal model package to `temporalModel`. Updated usage of `model` to `temporalModel` for `Message` and `MessageTypeMessage`.	+3/-3

Tests

Taskfile.yml

Add linter task for gateway and update lint:all dependencies

Taskfile.yml

Added new task lint:gateway to run go-fsck lint in the gateway
directory.

Updated lint:all task to include lint:gateway as a dependency.

+6/-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-24 14:19:13.400406402 +0000
+++ current.txt 2024-07-24 14:19:10.308368454 +0000
@@ -1606,7 +1606,7 @@
    Tags            []string                   `json:"tags"`
    Health          map[string]HealthCheckItem `json:"health"`
    Stats           GWStats                    `json:"stats"`
-   HostDetails     model.HostDetails          `json:"host_details"`
+   HostDetails     internalmodel.HostDetails  `json:"host_details"`
 }

 type NotificationsManager struct {
@@ -2592,7 +2592,7 @@
        InputRequestContext,
    }
 )
-var ShouldOmit = reflect.IsEmpty
+var ShouldOmit = internalreflect.IsEmpty
     ShouldOmit is a compatibility alias. It may be removed in the future.

@@ -7789,7 +7789,7 @@
 func (a APIDefinitionLoader) ParseOAS(r io.Reader) (oas oas.OAS)

 type APIError struct {
-   Message htmlTemplate.HTML
+   Message htmltemplate.HTML
 }
     APIError is generic error object returned if there is something wrong with
     the request
@@ -9622,7 +9622,7 @@
 func (r *RPCStorageHandler) SetRollingWindow(keyName string, per int64, val string, pipeline bool) (int, []interface{})
     SetScrollingWindow is used in the rate limiter to handle rate limits fairly.

-func (r *RPCStorageHandler) StartPubSubHandler(_ string, _ func(*model.Message)) error
+func (r *RPCStorageHandler) StartPubSubHandler(_ string, _ func(*temporalmodel.Message)) error
     StartPubSubHandler will listen for a signal and run the callback with the
     message

@@ -10419,7 +10419,7 @@

 type TransformSpec struct {
    apidef.TemplateMeta
-   Template *textTemplate.Template
+   Template *texttemplate.Template
 }

 type TykGoPluginResponseHandler interface {
@@ -11752,7 +11752,7 @@

 func (c *Consul) Get(key string) (string, error)

-func (c *Consul) Store() *consulApi.KV
+func (c *Consul) Store() *consulapi.KV

 type Store interface {
    Get(key string) (string, error)
@@ -11770,7 +11770,7 @@
 }
     Vault is an implementation of a KV store which uses Consul as it's backend

-func (v *Vault) Client() *vaultApi.Client
+func (v *Vault) Client() *vaultapi.Client

 func (v *Vault) Get(key string) (string, error)

@@ -12133,7 +12133,7 @@
 func IsEnabled() bool
     IsEnabled returns true if the global trace manager is enabled.

-func Log(ctx context.Context, fields ...opentracingLog.Field)
+func Log(ctx context.Context, fields ...opentracinglog.Field)
     Log tries to check if there is a span in ctx and adds logs fields on the
     span.

@@ -12286,7 +12286,7 @@

 func (s Span) LogEventWithPayload(event string, payload interface{})

-func (s Span) LogFields(fields ...opentracingLog.Field)
+func (s Span) LogFields(fields ...opentracinglog.Field)

 func (s Span) LogKV(alternatingKeyValues ...interface{})

github-actions[bot] commented 1 month ago

PR Reviewer Guide 🔍

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

🧪 No relevant tests

🔒 No security concerns identified

⚡ No key issues to review

github-actions[bot] commented 1 month ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible issue	Add error handling right after the extractor object creation to catch initialization errors ___ Consider handling the error from `graphqlInternal.NewGraphStatsExtractor()` directly after its creation to ensure that any initialization errors are caught early, improving the robustness of error handling. [gateway/handler_success.go [156]](https://github.com/TykTechnologies/tyk/pull/6409/files#diff-45135957493eca37f2e3e9a81079577777133c53b27cf95ea2ff0329c05bd006R156-R156) ```diff -extractor := graphqlInternal.NewGraphStatsExtractor() +extractor, err := graphqlInternal.NewGraphStatsExtractor() +if err != nil { + logger.WithError(err).Error("failed to create graph stats extractor") + return +} ``` Suggestion importance[1-10]: 9 Why: This suggestion improves the robustness of the code by ensuring that any errors during the creation of the `GraphStatsExtractor` are caught early. This is crucial for maintaining the stability of the application.	9
Maintainability	Refactor type assertion and message type check into a separate function for clarity ___ Refactor the type assertion and message type check into a separate function to improve code readability and maintainability. [gateway/redis_signals.go [99-104]](https://github.com/TykTechnologies/tyk/pull/6409/files#diff-18cb136722c238e19b02741a85510dfd464343f85365482f0873aa60a37718afR99-R104) ```diff -message, ok := v.(temporalModel.Message) -if !ok { +if !isValidMessage(v) { return } -if message.Type() != temporalModel.MessageTypeMessage { - return +// Define the new function elsewhere in your codebase +func isValidMessage(v interface{}) bool { + message, ok := v.(temporalModel.Message) + if !ok \|\| message.Type() != temporalModel.MessageTypeMessage { + return false + } + return true } ``` Suggestion importance[1-10]: 7 Why: Refactoring the type assertion and message type check into a separate function improves code readability and maintainability. However, it is not critical for functionality.	7
Best practice	Add error handling for the gateway linting task ___ Ensure that the new lint task `lint:gateway` includes error handling to manage failures gracefully during the linting process. [Taskfile.yml [85-88]](https://github.com/TykTechnologies/tyk/pull/6409/files#diff-cd2d359855d0301ce190f1ec3b4c572ea690c83747f6df61c9340720e3d2425eR85-R88) ```diff lint:gateway: internal: true cmds: - - cd gateway && go-fsck lint + - cd gateway && go-fsck lint \|\| echo "Linting failed for gateway" ``` Suggestion importance[1-10]: 6 Why: Adding error handling to the linting task is a good practice to ensure that failures are managed gracefully. However, it is a minor improvement and not crucial for the core functionality.	6

titpetric commented 4 weeks ago

Sonarcloud reports existing issues and coverage as refactoring touched a lot of files; cannot address as the scope is too large/preexisting, so we're making a merge exception here due to the nature of the change (type safe import replacements).