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

Adjust JSON limit to 2MB and report on sizes #2162

Merged
merged 7 commits into from
Jun 16, 2021

Conversation

peter-formlogic
Copy link
Contributor

@peter-formlogic peter-formlogic commented Apr 14, 2021

PR Type

Other

PR Checklist

  • Tests for the changes have been added / updated.
  • Documentation comments have been added / updated.
  • A changelog entry has been made for the appropriate packages.
  • Format code with the latest stable rustfmt.
  • (Team) Label with affected crates and semver status.

Overview

This adjusts the default limit of JSON payloads to 2mb from 32kb and adjusts the error to include what the limits are when they're hit. As this changes the error enum & has possibly knock on effects to memory usage by allowing more through this is probably considered a breaking change.

To justify the increase, I have looked at other defaults in other software for what their limits are:

(Honestly, I'd much prefer having no limit at all by default as these things seem to have a way of coming back to bite at unrealistic times, like 2am on a saturday morning!)

@fakeshadow fakeshadow added A-web project: actix-web B-semver-major breaking change requiring a major version bump labels Apr 15, 2021
@fakeshadow
Copy link
Contributor

I think it's fine to relax the limit in general. We need a changelog for limit and error changes

@peter-formlogic
Copy link
Contributor Author

Done!

src/error.rs Outdated Show resolved Hide resolved
@robjtede robjtede added this to the actix-web v4 milestone Apr 22, 2021
@thalesfragoso
Copy link
Member

Thanks for the PR, is this just waiting for the conflicts to be resolved ?

@peter-formlogic
Copy link
Contributor Author

@thalesfragoso sorry didn't notice that it was blocked on conflicts. I've merged master into this PR

@robjtede robjtede merged commit fb2b362 into actix:master Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-web project: actix-web B-semver-major breaking change requiring a major version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants