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.

Sign up for GitHub

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

Jump to bottom

Merging to release-5.3: [TT-12775] Add request size limit test for POST, PUT and PATCH methods. (#6751) #6755

Merged
merged 1 commit into from
Dec 10, 2024
Merged

Merging to release-5.3: [TT-12775] Add request size limit test for POST, PUT and PATCH methods. (#6751) #6755

merged 1 commit into from
Dec 10, 2024

Conversation

buger
Copy link
Member

@buger buger commented Dec 10, 2024 •
edited by github-actions bot
Loading

User description

TT-12775 Add request size limit test for POST, PUT and PATCH methods. (#6751)

User description

TT-12775
Summary Request size limit breaks GET and DELETE requests
Type Bug Bug
Status In Code Review
Points N/A
Labels '24Bugsmash, customer_bug, jira_escalated

Follow up PR for #6734

This PR adds a negative test case for POST, PUT and PATCH methods. The
test is more complex than the existing ones because Golang http package
automatically adds Content-Length to request if the method is POST,
PUT or PATCH.

PR Type

Tests, Bug fix

Description

  • Added a new test case to ensure that requests without Content-Length
    headers for POST, PUT, and PATCH methods are correctly handled by the
    middleware.
  • Verified that the middleware returns StatusLengthRequired and logs
    an appropriate error message when the Content-Length header is
    missing.
  • Improved test coverage for request size limit functionality.

Changes walkthrough 📝

Relevant files
Tests
mw_request_size_limit_test.go
Add test case for missing Content-Length in POST, PUT, PATCH methods

gateway/mw_request_size_limit_test.go

  • Added a new test case to validate behavior when Content-Length is
    missing for POST, PUT, and PATCH methods.
  • Introduced a logger and middleware setup for the new test case.
  • Verified that the middleware returns StatusLengthRequired and an
    appropriate error message when Content-Length is missing.
    		•  +33/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull
    request to receive relevant information

    PR Type

    Tests, Bug fix

    Description

    • Added a new test case to ensure that requests without Content-Length headers for POST, PUT, and PATCH methods are correctly handled by the middleware.
    • Verified that the middleware returns StatusLengthRequired and logs an appropriate error message when the Content-Length header is missing.
    • Improved test coverage for request size limit functionality.

    Changes walkthrough 📝

    Relevant files
    Tests
    mw_request_size_limit_test.go
    Add test case for missing Content-Length in POST, PUT, PATCH methods

    gateway/mw_request_size_limit_test.go

  • Added a new test case to validate behavior when Content-Length is
    missing for POST, PUT, and PATCH methods.
  • Introduced a logger and middleware setup for the new test case.
  • Verified that the middleware returns StatusLengthRequired and logs an
    appropriate error message when Content-Length is missing.
    		•  +32/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @buraksezer
    [TT-12775] Add request size limit test for POST, PUT and PATCH method…
    97dd9c6 
    …s. (#6751)

### **User description**
<details open>
<summary><a href="https://tyktech.atlassian.net/browse/TT-12775"
title="TT-12775" target="_blank">TT-12775</a></summary>
  <br />
  <table>
    <tr>
      <th>Summary</th>
      <td>Request size limit breaks GET and DELETE requests</td>
    </tr>
    <tr>
      <th>Type</th>
      <td>
<img alt="Bug"
src="https://tyktech.atlassian.net/rest/api/2/universal_avatar/view/type/issuetype/avatar/10303?size=medium"
/>
        Bug
      </td>
    </tr>
    <tr>
      <th>Status</th>
      <td>In Code Review</td>
    </tr>
    <tr>
      <th>Points</th>
      <td>N/A</td>
    </tr>
    <tr>
      <th>Labels</th>
<td><a
href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20'24Bugsmash%20ORDER%20BY%20created%20DESC"
title="'24Bugsmash">'24Bugsmash</a>, <a
href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20customer_bug%20ORDER%20BY%20created%20DESC"
title="customer_bug">customer_bug</a>, <a
href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20jira_escalated%20ORDER%20BY%20created%20DESC"
title="jira_escalated">jira_escalated</a></td>
    </tr>
  </table>
</details>
<!--
  do not remove this marker as it will break jira-lint's functionality.
  added_by_jira_lint
-->

---

Follow up PR for #6734 

This PR adds a negative test case for POST, PUT and PATCH methods. The
test is more complex than the existing ones because Golang http package
automatically adds `Content-Length` to request if the method is POST,
PUT or PATCH.


___

### **PR Type**
Tests, Bug fix


___

### **Description**
- Added a new test case to ensure that requests without `Content-Length`
headers for POST, PUT, and PATCH methods are correctly handled by the
middleware.
- Verified that the middleware returns `StatusLengthRequired` and logs
an appropriate error message when the `Content-Length` header is
missing.
- Improved test coverage for request size limit functionality.



___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Tests</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>mw_request_size_limit_test.go</strong><dd><code>Add
test case for missing Content-Length in POST, PUT, PATCH
methods</code></dd></summary>
<hr>

gateway/mw_request_size_limit_test.go

<li>Added a new test case to validate behavior when
<code>Content-Length</code> is <br>missing for POST, PUT, and PATCH
methods.<br> <li> Introduced a logger and middleware setup for the new
test case.<br> <li> Verified that the middleware returns
<code>StatusLengthRequired</code> and an <br>appropriate error message
when <code>Content-Length</code> is missing.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6751/files#diff-107317fefc06776e7acf5e35daac311b025a92c6721432272dbd7c7dcdd854f8">+33/-0</a>&nbsp;
&nbsp; </td>

</tr>
</table></td></tr></tr></tbody></table>

___

> 💡 **PR-Agent usage**: Comment `/help "your question"` on any pull
request to receive relevant information

(cherry picked from commit 9c5a43b)
    @buger buger enabled auto-merge (squash) December 10, 2024 09:07
    @github-actions GitHub Actions
    Copy link
    Contributor

    API Changes

    no api changes detected

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    6751 - Fully compliant

    Fully compliant requirements:

    • Add a negative test case for POST, PUT, and PATCH methods to ensure requests without Content-Length headers are handled correctly.
    • Ensure the middleware returns StatusLengthRequired and logs an appropriate error message when the Content-Length header is missing.
    • Improve test coverage for request size limit functionality.
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    The error message "Content length is required for this request" should be verified in the test to ensure it's being logged as expected.

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Correct the usage of require.Errorf to properly format the error message

    Ensure that the error message in require.Errorf matches the expected format. The
    function require.Errorf expects the first argument to be the error, the second to be
    the format string, and the subsequent arguments to be values used in the format
    string.

    gateway/mw_request_size_limit_test.go [92]

    -require.Errorf(t, err, "Content length is required for this request")
+require.Errorf(t, err, "Content length is required for this request: %v", err)
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies and fixes an issue with the misuse of require.Errorf in Go, ensuring that the error message is formatted correctly with the error variable included. This enhances the error reporting quality in test cases.

    		8

    @sonarqubecloud SonarQubeCloud
    Copy link

    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 SonarQube Cloud

    @buger buger merged commit c35cb9f into release-5.3 Dec 10, 2024
    33 of 38 checks passed
    @buger buger deleted the merge/release-5.3/9c5a43bcafb2ca81b38592e4f57d002f55f889d7 branch December 10, 2024 09:53
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @buger @buraksezer