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

Tell why a request is bad for a 400 Bad Request response #1575

Merged
merged 1 commit into from Feb 11, 2019

Conversation

trustin
Copy link
Collaborator

@trustin trustin commented Feb 8, 2019

Motivation:

It is sometimes hard for a client to know why a request is bad.

Modifications:

  • Added short error messages to 400 Bad Request responses
  • Used ProtocolViolationException instead of IllegalArgumentException for bad requests.
  • Removed unnecessary validation of HTTP method in HttpServerHandler.
  • Cleaned up response generation in HttpServerHandler.

Result:

  • User friendliness.

Motivation:

It is sometimes hard for a client to know why a request is bad.

Modifications:

- Added short error messages to 400 Bad Request responses
- Used `ProtocolViolationException` instead of `IllegalArgumentException` for bad requests.
- Removed unnecessary validation of HTTP method in `HttpServerHandler`.

Result:

- User friendliness.
@trustin trustin added this to the 0.80.0 milestone Feb 8, 2019
@trustin
Copy link
Collaborator Author

trustin commented Feb 8, 2019

/cc @jwills

@codecov
Copy link

codecov bot commented Feb 8, 2019

Codecov Report

Merging #1575 into master will increase coverage by <.01%.
The diff coverage is 67.02%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1575      +/-   ##
============================================
+ Coverage     72.77%   72.77%   +<.01%     
- Complexity     7495     7499       +4     
============================================
  Files           695      695              
  Lines         30171    30188      +17     
  Branches       3683     3687       +4     
============================================
+ Hits          21956    21970      +14     
- Misses         6310     6317       +7     
+ Partials       1905     1901       -4
Impacted Files Coverage Δ Complexity Δ
...spring/web/reactive/ArmeriaHttpHandlerAdapter.java 75% <0%> (-3.95%) 5 <0> (ø)
...m/linecorp/armeria/server/Http2RequestDecoder.java 59.68% <50%> (+2.54%) 24 <1> (+1) ⬆️
.../linecorp/armeria/server/tomcat/TomcatService.java 63.82% <62.5%> (+0.09%) 24 <0> (ø) ⬇️
...m/linecorp/armeria/server/Http1RequestDecoder.java 76.76% <66.66%> (+1.76%) 27 <0> (+1) ⬆️
...com/linecorp/armeria/server/HttpServerHandler.java 78.7% <79.54%> (+1.52%) 79 <11> (+5) ⬆️
...rp/armeria/server/tomcat/ManagedTomcatService.java 50.84% <0%> (-11.87%) 6% <0%> (-1%)
...armeria/server/tomcat/Tomcat90ProtocolHandler.java 48.14% <0%> (-7.41%) 11% <0%> (-2%)
...lient/circuitbreaker/CircuitBreakerHttpClient.java 69.69% <0%> (-3.04%) 10% <0%> (-1%)
.../linecorp/armeria/client/Http1ResponseDecoder.java 58.18% <0%> (-1.82%) 22% <0%> (-1%)
...om/linecorp/armeria/client/HttpSessionHandler.java 57.75% <0%> (-1.73%) 27% <0%> (-1%)
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70f781f...7c39307. Read the comment docs.

@line line deleted a comment from codecov bot Feb 8, 2019
Copy link
Member

@minwoox minwoox left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@trustin trustin removed the request for review from hyangtack February 11, 2019 10:56
@trustin trustin merged commit 82bc639 into line:master Feb 11, 2019
@trustin trustin deleted the 400_with_message branch February 11, 2019 10:56
@trustin
Copy link
Collaborator Author

trustin commented Feb 11, 2019

Thanks for reviewing.

fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
Motivation:

It is sometimes hard for a client to know why a request is bad.

Modifications:

- Added short error messages to 400 Bad Request responses
- Used `ProtocolViolationException` instead of `IllegalArgumentException` for bad requests.
- Removed unnecessary validation of HTTP method in `HttpServerHandler`.

Result:

- User friendliness.
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.

None yet

2 participants