FailFast if Content-Length header value is greater than configured max request size #59
Conversation
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
============================================
+ Coverage 90.2% 90.39% +0.18%
- Complexity 1947 1955 +8
============================================
Files 141 141
Lines 5790 5797 +7
Branches 765 767 +2
============================================
+ Hits 5223 5240 +17
+ Misses 387 378 -9
+ Partials 180 179 -1
Continue to review full report at Codecov.
|
|
|
||
| maxRequestSizeInBytes = 10; | ||
| httpHeaders.set(CONTENT_LENGTH, 100); | ||
| httpHeaders.set(CONTENT_TYPE, APPLICATION_JSON); |
There was a problem hiding this comment.
Is the content-type header necessary?
There was a problem hiding this comment.
no, i'll remove them as they are not necessary for the testing of this functionality. i added them just for typical use case even though they are ignored.
| // given | ||
| doReturn(null).when(endpointMock).maxRequestSizeInBytesOverride(); | ||
|
|
||
| maxRequestSizeInBytes = 10; |
There was a problem hiding this comment.
Do you need to specify maxRequestSizeInBytes and create your own handlerSpy? Isn't this how handlerSpy is already setup?
Edit: I guess you're probably doing this for clarity vs. the other similar methods. That's fine.
There was a problem hiding this comment.
your edit is spot on. i kept it there for clarity. if you think of a way that is more clear let me know and i'll change it.
| // given | ||
| doReturn(100).when(endpointMock).maxRequestSizeInBytesOverride(); | ||
|
|
||
| maxRequestSizeInBytes = 99; |
There was a problem hiding this comment.
Should probably test when the global is above and below the endpoint override (to prove that the override is used no matter what). I'd recommend using a @DataProvider to set maxRequestSizeInBytes to 99 and 101 - the rest of the test should still work as-is I think.
There was a problem hiding this comment.
good call. i need to start using @dataProvider more often, i see the benefit.
done!
| } | ||
|
|
||
| @Test | ||
| public void doChannelRead_HttpRequest_transfer_encoding_chunked_processed() { |
There was a problem hiding this comment.
Confusing test name. Recommend: doChannelRead_HttpRequest_does_not_throw_TooLongFrameException_if_content_length_header_is_missing(). I'd also suggest making it a @DataProvider test with a boolean isChunkedTransferEncoding argument to test when transfer-encoding is chunked or when all relevant headers are just missing.
There was a problem hiding this comment.
renamed and made suggested updates.
| } | ||
|
|
||
| @Test | ||
| public void doChannelRead_HttpRequest_request_size_validation_endpoint_overridden_processed_message() { |
There was a problem hiding this comment.
Another confusing test name. Recommend: doChannelRead_HttpRequest_does_not_throw_TooLongFrameException_if_content_length_is_less_than_endpoint_overridden_value(). Also recommend making this a @DataProvider test with content-length set below the endpoint override as well as exactly the endpoint override value.
There was a problem hiding this comment.
thanks for the names. renamed and updated with dataprovider
There was a problem hiding this comment.
Yeah I tend to just mix & match the given/when/then bits to come up with the test names. [target_method/code]_[does_thing_i_want/expect]_[given_some_scenario]. Thanks for humoring me.
rabeyta
force-pushed
the
failFastRequestTooBig
branch
from
b1fd254
to
bc69ded
Compare
|
The last thing I think this PR needs is some adjustments to I'm not sure how to get RestAssured to not automatically put in content-length header for the latter tests - if you can't figure out a way with RestAssured (does RestAssured let you do chunked transfer encoding? Maybe with the |
…ength header is set and greater than the configured max request size once we know the endpoint for the request
rabeyta
force-pushed
the
failFastRequestTooBig
branch
from
bc69ded
to
ed19ec4
Compare
|
@nicmunroe That is a good call. I took those tests and duplicated them using the Netty client and transfer-encoding header instead of content-length like RestAssured does due to the underlining Apache client. This should then cover both of the scenarios we care about and enable a nice regression in case this logic needs to change. If you had other ideas on the implementation let me know and i'll make the necessary updates. |
|
Looks great, thank you! |
Per #37 , Configured validation in RoutingHandler to fail fast if the content-length header is set and greater than the configured max request size once we know the endpoint for the request
This was put into the RoutingHandler instead of RequestInfoSetterHandler as we do not know the endpoint yet until after the RoutingHandler figures it out, which is necessary to process an endpoint's override of the global limit.
I also refactored a couple of the requestSize methods into HttpUtils to enable both handlers to share the code of determining the max size and if it is disabled.