Skip to content

Conversation

@plehatron
Copy link
Contributor

@plehatron plehatron commented Oct 7, 2025

Reason for This PR

When max_request_size is set and a client sends a payload that exceeds this limit, return a 413 Request Entity Too Large response instead of a generic 500 Internal Server Error. A 413 status code correctly indicates a client-side error, whereas a 500 status code implies a server-side issue.

Description of Changes

  • Modify the ServeHTTP to return 413 when http.MaxBytesError occurs.

License Acceptance

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any user-facing changes are mentioned in CHANGELOG.md.
  • All added/changed functionality is tested.

Summary by CodeRabbit

  • Bug Fixes
    • Oversized HTTP requests now return 413 Payload Too Large instead of 500 Internal Server Error, aligning with HTTP standards.
    • Applies to requests exceeding maximum body size, including URL-encoded payloads, ensuring consistent responses across endpoints.
    • Improves client feedback, making it easier to detect and handle payload limits.

When `max_request_size` is set and a client sends a payload that exceeds this limit,
return a 413 Request Entity Too Large response instead of a generic 500 Internal Server Error.
A 413 status code correctly indicates a client-side error, while 500 implies a server-side issue.
@coderabbitai
Copy link

coderabbitai bot commented Oct 7, 2025

Walkthrough

Dynamic error-based HTTP status handling added in handler ServeHTTP: returns 413 for MaxBytesError, otherwise 500. Corresponding tests updated to expect 413 for oversized requests across multiple cases. No public APIs changed.

Changes

Cohort / File(s) Summary
HTTP handler error mapping
handler/handler.go
ServeHTTP now maps MaxBytesError to 413 Request Entity Too Large; all other errors remain 500. No exported symbols changed.
Tests expecting 413 for oversized requests
tests/http_plugin_test.go, tests/http_plugin2_test.go, tests/http_plugin3_test.go
Updated assertions to expect http.StatusRequestEntityTooLarge (413) instead of 500 for oversized request bodies/URL-encoded payloads.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant C as Client
    participant H as ServeHTTP Handler
    participant B as Body Reader/Parser

    C->>H: HTTP Request (potentially large body)
    H->>B: Read/parse request body with size limit
    alt Body exceeds limit
        B-->>H: MaxBytesError
        Note over H: Error type check
        H-->>C: 413 Request Entity Too Large
    else Other error occurs
        B-->>H: Generic error
        H-->>C: 500 Internal Server Error
    end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

Thump-thump—my paws on the wire,
Bytes too big? I raise the choir: 4-1-3!
No longer five-oh-oh in fright,
I nibble errors, set them right.
Carrot logs filed, tails held high—
Small change, swift hop, we comply.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly and accurately describes the primary change by stating that a 413 status will be returned when a MaxBytesError occurs, which directly reflects the core modification in the ServeHTTP error handling logic. It is concise, specific, and clearly conveys the intent without extraneous detail.
Description Check ✅ Passed The pull request description follows the repository template by providing a clear “Reason for This PR” section, a detailed “Description of Changes,” the required license acceptance statement, and the completed PR checklist, ensuring all mandatory sections are present and filled out.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b7a3915 and 21a6d38.

📒 Files selected for processing (4)
  • handler/handler.go (1 hunks)
  • tests/http_plugin2_test.go (1 hunks)
  • tests/http_plugin3_test.go (1 hunks)
  • tests/http_plugin_test.go (1 hunks)
🔇 Additional comments (4)
handler/handler.go (1)

140-145: LGTM! Correctly returns 413 for oversized requests.

The implementation properly distinguishes between client errors (oversized payload → 413) and server errors (other issues → 500). Using errors.As correctly handles wrapped errors, and the status code selection aligns with RFC 7231.

tests/http_plugin_test.go (1)

1789-1789: Test expectation correctly updated.

The test now expects HTTP 413 for oversized requests, consistent with the handler changes.

tests/http_plugin3_test.go (1)

1155-1155: Test expectation correctly updated.

The test now expects HTTP 413 for oversized URL-encoded requests, consistent with the handler changes.

tests/http_plugin2_test.go (1)

608-608: Test expectation correctly updated.

The test now expects HTTP 413 for oversized POST requests, consistent with the handler changes.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@rustatian rustatian requested review from Copilot and rustatian October 7, 2025 08:53
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR improves HTTP error handling by returning the appropriate HTTP status code (413 Request Entity Too Large) when clients send requests that exceed the configured maximum request size limit, instead of the generic 500 Internal Server Error.

  • Modifies the HTTP handler to detect http.MaxBytesError and return HTTP 413 status
  • Updates test assertions to expect the correct 413 status code instead of 500

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
handler/handler.go Adds logic to detect MaxBytesError and return 413 status code
tests/http_plugin_test.go Updates test to expect 413 status code for oversized requests
tests/http_plugin2_test.go Updates test to expect 413 status code for oversized requests
tests/http_plugin3_test.go Updates test to expect 413 status code for oversized URL-encoded requests

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@rustatian
Copy link
Member

Hey @plehatron 👋🏻
Thank you for the PR, Good job 👍🏻

@rustatian rustatian added the enhancement New feature or request label Oct 7, 2025
@codecov
Copy link

codecov bot commented Oct 7, 2025

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@rustatian rustatian merged commit b372b65 into roadrunner-server:master Oct 7, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants