Skip to content

[TT-12064]Update Cast function signature#6261

Merged
jeffy-mathew merged 5 commits into
masterfrom
fix/TT-12064/update-cast-func-signature
May 8, 2024
Merged

[TT-12064]Update Cast function signature#6261
jeffy-mathew merged 5 commits into
masterfrom
fix/TT-12064/update-cast-func-signature

Conversation

@jeffy-mathew

@jeffy-mathew jeffy-mathew commented May 8, 2024

Copy link
Copy Markdown
Contributor

User description

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

PR Type

Bug fix, Enhancement


Description

  • Updated Cast function in reflect.go to return pointers, improving error handling and consistency.
  • Modified JSON marshaling in server.go to handle maps through dereferenced values, enhancing data handling.
  • Fixed pointer dereference in api_definitions.go to ensure correct data assignment.
  • Adjusted unit tests in reflect_test.go to align with the new pointer return types from Cast function.

Changes walkthrough 📝

Relevant files
Bug fix
api_definitions.go
Fix pointer dereference in WebHookHandlerConf                       

apidef/api_definitions.go

  • Fixed pointer dereference to ensure proper assignment in
    WebHookHandlerConf's Scan method.
  • +1/-1     
    Enhancement
    server.go
    Update JSON marshaling and event extraction                           

    apidef/oas/server.go

  • Modified MarshalJSON to correctly marshal the dereferenced map.
  • Updated ExtractTo method to dereference handlerMeta before assignment.

  • +6/-5     
    reflect.go
    Refactor Cast function to return pointers                               

    internal/reflect/reflect.go

  • Changed Cast function to return a pointer to the type parameter T.
  • Added error handling to return nil on marshaling or unmarshaling
    errors.
  • +6/-3     
    Tests
    reflect_test.go
    Update tests for Cast function changes                                     

    internal/reflect/reflect_test.go

  • Updated unit tests to assert dereferenced pointers after changes to
    Cast function.
  • +5/-5     

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

    @jeffy-mathew jeffy-mathew changed the title [TT-12064]Update Cast function signature. [TT-12064]Update Cast function signature May 8, 2024
    @github-actions

    github-actions Bot commented May 8, 2024

    Copy link
    Copy Markdown
    Contributor

    API Changes

    --- prev.txt	2024-05-08 12:48:15.342021718 +0000
    +++ current.txt	2024-05-08 12:48:12.494008056 +0000
    @@ -3012,10 +3012,8 @@
     }
         Event holds information about individual event to be configured on the API.
     
    -func (e *Event) GetWebhookConf() (map[string]interface{}, error)
    -    GetWebhookConf converts Event.WebhookEvent to map[string]interface{}
    -    with apidef.WebHookHandlerConf structure for classic API definition
    -    compatibility.
    +func (e *Event) GetWebhookConf() apidef.WebHookHandlerConf
    +    GetWebhookConf converts Event.WebhookEvent apidef.WebHookHandlerConf.
     
     func (e *Event) MarshalJSON() ([]byte, error)
         MarshalJSON marshals Event as per Tyk OAS API definition contract.

    @github-actions

    github-actions Bot commented May 8, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Description updated to latest commit (2b805aa)

    @github-actions

    github-actions Bot commented May 8, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves changes across multiple files with modifications to function signatures and data handling which requires a careful review to ensure that the changes do not introduce new bugs or regressions.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: Dereferencing nil pointers could occur if handlerMeta is nil before dereferencing in internal/reflect/reflect.go.

    Data Integrity: Changing function signatures to return pointers instead of values might lead to unintended side effects if not handled properly throughout the codebase.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileinternal/reflect/reflect.go
    suggestion      

    Consider adding a nil check before dereferencing handlerMeta to prevent potential runtime panics. [important]

    relevant lineHandlerMeta: *handlerMeta,

    relevant fileinternal/reflect/reflect.go
    suggestion      

    Ensure error handling is robust especially after changing the function signature to return a pointer which could be nil. This is crucial to prevent runtime errors in case of unsuccessful casts. [important]

    relevant linereturn nil, err

    relevant fileapidef/oas/server.go
    suggestion      

    Verify that outMap is not nil before dereferencing to outMapVal to avoid potential nil pointer dereference. [important]

    relevant lineoutMapVal := *outMap

    relevant fileinternal/reflect/reflect.go
    suggestion      

    Consider handling cases where JSON marshaling does not support the type of src, such as channels or functions, to avoid runtime errors. [medium]

    relevant lineb, err := json.Marshal(src)

    @github-actions

    github-actions Bot commented May 8, 2024

    Copy link
    Copy Markdown
    Contributor

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    @github-actions

    github-actions Bot commented May 8, 2024

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Possible issue
    Add nil check before dereferencing to prevent runtime panic.

    *Instead of directly dereferencing conf and assigning it to w, consider checking if conf
    is nil to avoid potential runtime panic.

    apidef/api_definitions.go [1443]

    -*w = *conf
    +if conf != nil {
    +    *w = *conf
    +}
     
    Check for nil before dereferencing to prevent runtime panic.

    Ensure webhookMap is not nil before dereferencing it in the loop to avoid runtime panic.

    apidef/oas/server.go [324-326]

    -for k, v := range *webhookMap {
    -    outMapVal[k] = v
    +if webhookMap != nil {
    +    for k, v := range *webhookMap {
    +        outMapVal[k] = v
    +    }
     }
     
    Enhancement
    Improve error handling by providing more context in the error message.

    Consider handling the case where the type T cannot be marshaled into JSON, which would
    cause json.Marshal to return an error. This is especially important for types that do not
    have a straightforward JSON representation.

    internal/reflect/reflect.go [71-73]

     b, err := json.Marshal(src)
     if err != nil {
    -    return nil, err
    +    return nil, fmt.Errorf("failed to marshal src: %w", err)
     }
     
    Add a check to ensure unmarshaled data is not nil.

    To enhance the robustness of the Cast function, add a check to ensure that dst is not nil
    after unmarshaling, before returning it.

    internal/reflect/reflect.go [76-80]

     err = json.Unmarshal(b, &dst)
     if err != nil {
         return nil, err
     }
    +if &dst == nil {
    +    return nil, fmt.Errorf("unmarshal resulted in nil")
    +}
     return &dst, nil
     

    Comment thread apidef/oas/server.go
    func (e *Event) GetWebhookConf() (map[string]interface{}, error) {
    webhookConf := apidef.WebHookHandlerConf{
    // GetWebhookConf converts Event.WebhookEvent apidef.WebHookHandlerConf.
    func (e *Event) GetWebhookConf() apidef.WebHookHandlerConf {

    Copy link
    Copy Markdown
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    <3

    @sonarqubecloud

    sonarqubecloud Bot commented May 8, 2024

    Copy link
    Copy Markdown

    Please retry analysis of this Pull-Request directly on SonarCloud

    @jeffy-mathew
    jeffy-mathew force-pushed the fix/TT-12064/update-cast-func-signature branch from c90bbbe to 890e2c3 Compare May 8, 2024 10:08
    @jeffy-mathew
    jeffy-mathew enabled auto-merge (squash) May 8, 2024 10:08
    @sonarqubecloud

    sonarqubecloud Bot commented May 8, 2024

    Copy link
    Copy Markdown

    @jeffy-mathew
    jeffy-mathew force-pushed the fix/TT-12064/update-cast-func-signature branch from 890e2c3 to 75781dd Compare May 8, 2024 12:23
    @github-actions

    github-actions Bot commented May 8, 2024

    Copy link
    Copy Markdown
    Contributor

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    @sonarqubecloud

    sonarqubecloud Bot commented May 8, 2024

    Copy link
    Copy Markdown

    Quality Gate Failed Quality Gate failed

    Failed conditions
    65.4% Coverage on New Code (required ≥ 80%)

    See analysis details on SonarCloud

    @jeffy-mathew
    jeffy-mathew merged commit 29569ae into master May 8, 2024
    @jeffy-mathew
    jeffy-mathew deleted the fix/TT-12064/update-cast-func-signature branch May 8, 2024 13:09
    nerdydread pushed a commit that referenced this pull request Sep 6, 2024
    ### **User description**
    <!-- Provide a general summary of your changes in the Title above -->
    
    ## Description
    
    <!-- Describe your changes in detail -->
    
    ## Related Issue
    
    <!-- This project only accepts pull requests related to open issues. -->
    <!-- If suggesting a new feature or change, please discuss it in an
    issue first. -->
    <!-- If fixing a bug, there should be an issue describing it with steps
    to reproduce. -->
    <!-- OSS: Please link to the issue here. Tyk: please create/link the
    JIRA ticket. -->
    
    ## Motivation and Context
    
    <!-- Why is this change required? What problem does it solve? -->
    
    ## How This Has Been Tested
    
    <!-- Please describe in detail how you tested your changes -->
    <!-- Include details of your testing environment, and the tests -->
    <!-- you ran to see how your change affects other areas of the code,
    etc. -->
    <!-- This information is helpful for reviewers and QA. -->
    
    ## Screenshots (if appropriate)
    
    ## Types of changes
    
    <!-- What types of changes does your code introduce? Put an `x` in all
    the boxes that apply: -->
    
    - [x] Bug fix (non-breaking change which fixes an issue)
    - [ ] New feature (non-breaking change which adds functionality)
    - [ ] Breaking change (fix or feature that would cause existing
    functionality to change)
    - [ ] Refactoring or add test (improvements in base code or adds test
    coverage to functionality)
    
    ## Checklist
    
    <!-- Go over all the following points, and put an `x` in all the boxes
    that apply -->
    <!-- If there are no documentation updates required, mark the item as
    checked. -->
    <!-- Raise up any additional concerns not covered by the checklist. -->
    
    - [ ] I ensured that the documentation is up to date
    - [ ] I explained why this PR updates go.mod in detail with reasoning
    why it's required
    - [ ] I would like a code coverage CI quality gate exception and have
    explained why
    
    
    ___
    
    ### **PR Type**
    Bug fix, Enhancement
    
    
    ___
    
    ### **Description**
    - Updated `Cast` function in `reflect.go` to return pointers, improving
    error handling and consistency.
    - Modified JSON marshaling in `server.go` to handle maps through
    dereferenced values, enhancing data handling.
    - Fixed pointer dereference in `api_definitions.go` to ensure correct
    data assignment.
    - Adjusted unit tests in `reflect_test.go` to align with the new pointer
    return types from `Cast` function.
    
    
    ___
    
    
    
    ### **Changes walkthrough** 📝
    <table><thead><tr><th></th><th align="left">Relevant
    files</th></tr></thead><tbody><tr><td><strong>Bug fix
    </strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>api_definitions.go</strong><dd><code>Fix pointer
    dereference in WebHookHandlerConf</code>&nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
    <hr>
    
    apidef/api_definitions.go
    <li>Fixed pointer dereference to ensure proper assignment in
    <br><code>WebHookHandlerConf</code>'s <code>Scan</code> method.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6261/files#diff-9961ccc89a48d32db5b47ba3006315ef52f6e5007fb4b09f8c5d6d299c669d67">+1/-1</a>&nbsp;
    &nbsp; &nbsp; </td>
    </tr>                    
    </table></td></tr><tr><td><strong>Enhancement
    </strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>server.go</strong><dd><code>Update JSON marshaling and
    event extraction</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
    <hr>
    
    apidef/oas/server.go
    <li>Modified <code>MarshalJSON</code> to correctly marshal the
    dereferenced map.<br> <li> Updated <code>ExtractTo</code> method to
    dereference <code>handlerMeta</code> before assignment.<br> <br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6261/files#diff-21857c42e8659f7980014e277c3c758703f29e9e5c0c40553f2584cddb870808">+6/-5</a>&nbsp;
    &nbsp; &nbsp; </td>
    </tr>                    
    
    <tr>
      <td>
        <details>
    <summary><strong>reflect.go</strong><dd><code>Refactor Cast function to
    return pointers</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    </dd></summary>
    <hr>
    
    internal/reflect/reflect.go
    <li>Changed <code>Cast</code> function to return a pointer to the type
    parameter <code>T</code>.<br> <li> Added error handling to return nil on
    marshaling or unmarshaling <br>errors.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6261/files#diff-e8975fcdc2226608fd31ef31fea201f98c70d9f575a35ab7bda3d6bac0303af9">+6/-3</a>&nbsp;
    &nbsp; &nbsp; </td>
    </tr>                    
    </table></td></tr><tr><td><strong>Tests
    </strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>reflect_test.go</strong><dd><code>Update tests for Cast
    function changes</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; </dd></summary>
    <hr>
    
    internal/reflect/reflect_test.go
    <li>Updated unit tests to assert dereferenced pointers after changes to
    <br><code>Cast</code> function.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6261/files#diff-7d968b0c22e2d7a7ab217ecbb1602d9e84765a7d489abeba81a1bb36d0e162b6">+5/-5</a>&nbsp;
    &nbsp; &nbsp; </td>
    </tr>                    
    </table></td></tr></tr></tbody></table>
    
    ___
    
    > 💡 **PR-Agent usage**:
    >Comment `/help` on the PR to get a list of all available PR-Agent tools
    and their descriptions
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants