Skip to content

[TT-11966/TT-12064] handle edge case with empty event handlers#6267

Merged
jeffy-mathew merged 1 commit into
masterfrom
fix/TT-11966/TT-12064/empty-events-edge-cases
May 10, 2024
Merged

[TT-11966/TT-12064] handle edge case with empty event handlers#6267
jeffy-mathew merged 1 commit into
masterfrom
fix/TT-11966/TT-12064/empty-events-edge-cases

Conversation

@jeffy-mathew

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

Copy link
Copy Markdown
Contributor

User description

Description

handle missed edge case where event handlers are empty.

Related Issue

Parent task: https://tyktech.atlassian.net/browse/TT-11966
Sub task: https://tyktech.atlassian.net/browse/TT-12064

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, Tests


Description

  • Added checks and initializations in event.go to handle scenarios where event handlers are empty or undefined, preventing runtime errors.
  • Enhanced test coverage in event_test.go to include tests for nil and empty event handlers, ensuring robustness.

Changes walkthrough 📝

Relevant files
Bug fix
event.go
Handle edge cases and initialization for empty event handlers

apidef/oas/event.go

  • Added initialization of EventHandlers to handle cases where event
    handlers are empty.
  • Ensured api.EventHandlers.Events is initialized before use to prevent
    nil map assignments.
  • +6/-5     
    Tests
    event_test.go
    Extend unit tests for event handler edge cases                     

    apidef/oas/event_test.go

  • Added test cases for nil and empty event handlers.
  • Modified existing test cases to include scenarios with pre-existing
    event handlers.
  • +34/-4   

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

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    API Changes

    no api changes detected

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Description updated to latest commit (240c634)

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves changes in event handling logic which is crucial and sensitive, but the changes are not extensive. The logic added needs careful review to ensure it handles all edge cases correctly.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The logic in EventHandlers.Fill method might not handle all edge cases correctly. For instance, the initialization of events variable before checking the length might be unnecessary and could lead to unexpected behavior if not handled properly.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileapidef/oas/event.go
    suggestion      

    Consider initializing the events variable only when necessary to avoid unnecessary memory allocation. This can be done by moving the initialization inside the condition where it's actually needed. [important]

    relevant lineevents := EventHandlers{}

    relevant fileapidef/oas/event.go
    suggestion      

    To prevent potential issues with map access, ensure that api.EventHandlers.Events is not nil before the loop that processes events starts. This can be done by adding a nil check before the loop. [important]

    relevant lineif api.EventHandlers.Events == nil {

    relevant fileapidef/oas/event.go
    suggestion      

    Refactor the duplicated logic for checking and making the map api.EventHandlers.Events to a separate method to improve code readability and reduce redundancy. [medium]

    relevant lineapi.EventHandlers.Events = make(map[apidef.TykEvent][]apidef.EventHandlerTriggerConfig)

    relevant fileapidef/oas/event_test.go
    suggestion      

    Add more comprehensive tests for the new logic handling nil and empty event handlers to ensure all edge cases are covered and the behavior is as expected. [important]

    relevant linetitle: "nil event handlers",

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Simplify the assignment of an empty EventHandlers to *e.

    **Instead of initializing events to an empty EventHandlers and then assigning it to e, you
    can directly assign an empty EventHandlers to e to simplify the code.

    apidef/oas/event.go [123-126]

    -events := EventHandlers{}
     if len(api.EventHandlers.Events) == 0 {
    -    *e = events
    +    *e = EventHandlers{}
         return
     }
     
    Expand test coverage to include partially filled and invalid event handler configurations.

    Add test cases to cover scenarios where EventHandlers are partially filled or have invalid
    configurations to ensure robustness.

    apidef/oas/event_test.go [121-123]

     {
         title: "empty event handlers",
         input: EventHandlers{},
         expected: apidef.EventHandlerMetaConfig{
             Events: map[apidef.TykEvent][]apidef.EventHandlerTriggerConfig{},
         },
    +},
    +{
    +    title: "partially filled event handlers",
    +    input: EventHandlers{...},
    +    expected: apidef.EventHandlerMetaConfig{...},
    +},
    +{
    +    title: "invalid event handler configurations",
    +    input: EventHandlers{...},
    +    expected: apidef.EventHandlerMetaConfig{...},
     }
     
    Bug
    Initialize the Events map before checking if e is nil to prevent nil pointer dereference.

    Ensure that the Events map is initialized in the ExtractTo method before checking if e is
    nil to avoid potential nil pointer dereference when accessing Events.

    apidef/oas/event.go [170-172]

    +if api.EventHandlers.Events == nil {
    +    api.EventHandlers.Events = make(map[apidef.TykEvent][]apidef.EventHandlerTriggerConfig)
    +}
     if e == nil {
         return
     }
     
    Maintainability
    Improve test maintainability by extracting setup code into a helper function.

    Refactor the test setup by extracting repeated code into a helper function to improve
    maintainability and readability.

    apidef/oas/event_test.go [244-249]

    -var api apidef.APIDefinition
    -api.EventHandlers = tc.input
    -server := Server{
    -    EventHandlers: tc.existingHandlers,
    +setupTest := func(tc TestCase) Server {
    +    var api apidef.APIDefinition
    +    api.EventHandlers = tc.input
    +    server := Server{
    +        EventHandlers: tc.existingHandlers,
    +    }
    +    server.EventHandlers.Fill(api)
    +    return server
     }
    -server.EventHandlers.Fill(api)
    +server := setupTest(tc)
     

    @jeffy-mathew
    jeffy-mathew force-pushed the fix/TT-11966/TT-12064/empty-events-edge-cases branch from 240c634 to db041c6 Compare May 10, 2024 11:54
    @jeffy-mathew
    jeffy-mathew force-pushed the fix/TT-11966/TT-12064/empty-events-edge-cases branch from db041c6 to 74ecbe6 Compare May 10, 2024 12:00
    @jeffy-mathew
    jeffy-mathew enabled auto-merge (squash) May 10, 2024 12:13
    @sonarqubecloud

    Copy link
    Copy Markdown

    @jeffy-mathew
    jeffy-mathew merged commit 8fb9d2e into master May 10, 2024
    @jeffy-mathew
    jeffy-mathew deleted the fix/TT-11966/TT-12064/empty-events-edge-cases branch May 10, 2024 12:23
    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
    
    handle missed edge case where event handlers are empty.
    ## Related Issue
    Parent task: https://tyktech.atlassian.net/browse/TT-11966
    Sub task: https://tyktech.atlassian.net/browse/TT-12064
    
    ## 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, Tests
    
    
    ___
    
    ### **Description**
    - Added checks and initializations in `event.go` to handle scenarios
    where event handlers are empty or undefined, preventing runtime errors.
    - Enhanced test coverage in `event_test.go` to include tests for nil and
    empty event handlers, ensuring robustness.
    
    
    ___
    
    
    
    ### **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>event.go</strong><dd><code>Handle edge cases and
    initialization for empty event handlers</code></dd></summary>
    <hr>
    
    apidef/oas/event.go
    <li>Added initialization of <code>EventHandlers</code> to handle cases
    where event <br>handlers are empty.<br> <li> Ensured
    <code>api.EventHandlers.Events</code> is initialized before use to
    prevent <br>nil map assignments.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6267/files#diff-528a9f5b311ff21c0b3a9b273e61398209ca8b51550327e4d437bba81e49d577">+6/-5</a>&nbsp;
    &nbsp; &nbsp; </td>
    </tr>                    
    </table></td></tr><tr><td><strong>Tests
    </strong></td><td><table>
    <tr>
      <td>
        <details>
    <summary><strong>event_test.go</strong><dd><code>Extend unit tests for
    event handler edge cases</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
    &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
    <hr>
    
    apidef/oas/event_test.go
    <li>Added test cases for nil and empty event handlers.<br> <li> Modified
    existing test cases to include scenarios with pre-existing <br>event
    handlers.<br>
    
    
    </details>
        
    
      </td>
    <td><a
    href="https://github.com/TykTechnologies/tyk/pull/6267/files#diff-a1f19f96579a470e73c131a0e37895da0c21d378f6aa48067608274564e62da7">+34/-4</a>&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