Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TT-9864] Optimize the creation/usage of AST documents #6345

Merged

Conversation

buraksezer
Copy link
Contributor

@buraksezer buraksezer commented Jun 13, 2024

User description

PR for https://tyktech.atlassian.net/browse/TT-9864. See TykTechnologies/graphql-go-tools#431 for details.


PR Type

Enhancement, Dependencies


Description

  • Added cleanup for allocated resources in the HandleReverseProxy method in internal/graphengine/engine_v2.go.
  • Updated graphql-go-tools dependency version in go.mod.
  • Updated checksum for graphql-go-tools dependency in go.sum.

Changes walkthrough 📝

Relevant files
Enhancement
engine_v2.go
Add resource cleanup in `HandleReverseProxy` method.         

internal/graphengine/engine_v2.go

  • Added cleanup for allocated resources in HandleReverseProxy method.
  • +2/-0     
    Dependencies
    go.mod
    Update `graphql-go-tools` dependency version.                       

    go.mod

    • Updated graphql-go-tools dependency version.
    +1/-1     
    go.sum
    Update checksum for `graphql-go-tools` dependency.             

    go.sum

    • Updated checksum for graphql-go-tools dependency.
    +2/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @github-actions github-actions bot added enhancement dependencies Pull requests that update a dependency file Review effort [1-5]: 2 labels Jun 13, 2024
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    2

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Resource Cleanup Verification:
    Ensure that the defer gqlRequest.Cleanup() call effectively cleans up all necessary resources without side effects or errors.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Prevent potential nil pointer dereference by checking if gqlRequest is not nil before cleanup

    Ensure that gqlRequest is not nil before calling Cleanup() to prevent potential nil
    pointer dereference.

    internal/graphengine/engine_v2.go [243]

    -defer gqlRequest.Cleanup()
    +if gqlRequest != nil {
    +    defer gqlRequest.Cleanup()
    +}
     
    
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug where gqlRequest could be nil, leading to a nil pointer dereference when Cleanup() is called. Adding a nil check improves the robustness and stability of the code.

    9

    Copy link
    Contributor

    API Changes

    no api changes detected

    @buraksezer buraksezer changed the title [TT-9864] Try to cleanup allocated resources for a GQL query. [TT-9864] Optimize the creation/usage of AST documents Jun 14, 2024
    Copy link

    sonarcloud bot commented Jun 14, 2024

    @buraksezer buraksezer merged commit 37efc33 into master Jun 17, 2024
    36 checks passed
    @buraksezer buraksezer deleted the feat/TT-9864/Optimize-the-creation-usage-of-AST-documents branch June 17, 2024 07:58
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    dependencies Pull requests that update a dependency file enhancement Review effort [1-5]: 2
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    2 participants