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

Disallow entity for 205 status code #2686

Merged
merged 3 commits into from Sep 10, 2019

Conversation

@sunghyunzz
Copy link
Contributor

commented Sep 6, 2019

Purpose

Disallow entity for 205 status code as response with this status code should contain empty content.

References

References #2685

Changes

  • Now you cannot pass HTTP entity to response with 205 status code

Background Context

Set allowsEntity as false would be the simplest way of following RFC spec.

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 6, 2019

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

Copy link
Member

left a comment

While I agree this would have been better, I'm not sure I think it is worth risking breaking applications that already (admittedly incorrectly) use a ResetContent status code with an entity.

(sorry, did not read the linked issue yet)

Copy link
Member

left a comment

I agree with the change, given that without this setting we automatically generate a body even though the RFC explicitly forbids it.

This does mean any applications that (admittedly incorrectly) relied on sending 205 responses with entity bodies would now no longer work.

That sounds unlikely to be common, but I do think it would be good to mention this in the release notes. Could you add such a mention to this PR?

@sunghyunzz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 8, 2019

@raboof Thanks for comments. To mention breaking changes, should I create 10.2.x release notes or create 10.1.10 section in existing 10.1.x release notes?

@jrudolph

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Add it to existing 10.1.x release notes. It's a breaking change but since it fixes a (currently) uncommonly used status code, we think it would be fine to be included in 10.1.x.

@raboof

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

OK TO TEST

@akka-ci akka-ci added validating tested and removed validating labels Sep 9, 2019
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 9, 2019

Test PASSed.

@sunghyunzz

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

@raboof @jrudolph I mentioned about the change on release note, but I'm not sure if it is written correctly since english is not my first language 😅please suggest me any changes. Thanks :)

@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

Test PASSed.

Copy link
Member

left a comment

I think your addition to the release notes is great. I think it would be good to highlight this change in a 'migration notes' section - see above for a suggestion on the text.

Co-Authored-By: Arnout Engelen <github@bzzt.net>
@akka-ci

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2019

Test PASSed.

Copy link
Member

left a comment

Great, thanks you @sunghyunzz. LGTM

@jrudolph jrudolph merged commit f26f76b into akka:master Sep 10, 2019
4 checks passed
4 checks passed
Jenkins PR Auto-Formatter Successful
Details
Jenkins PR Validation Test PASSed. 4170 tests run, 1074 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details
@sunghyunzz sunghyunzz deleted the Rainist:fix-205-status-code branch Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.