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

fix bug with transfer-encoding: chunked #220

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix bug with transfer-encoding: chunked #220

wants to merge 1 commit into from

Conversation

d-t-w
Copy link

@d-t-w d-t-w commented Feb 21, 2018

Doc for yada.interceptors/process-request-body says:

"Therefore we process the request body if the request contains a Content-Length or Transfer-Encoding header, regardless of the method semantics."

Current implementation does not process the request body if there is a Transfer-Encoding header (indicating chunked) and no Content-Length header, valid per HTTP 1.1 (see: conditional on line 174).

This PR causes yada.request-body/process-request-body to be called if the request contains either header, in line with the doc-string.

If the consumer in the original impl was intended to deal with chunked encoding, be aware that as long as the underlying Netty pipeline includes the HttpObjectAggregator then Netty will take care of that chunking aggregation for you.

@d-t-w
Copy link
Author

d-t-w commented Feb 21, 2018

I should add I found this issue when attempting to receive data via a PUT with chunked encoding, e.g.

curl -v -X PUT --data-binary "some-data" "host:port/resource" -H "Transfer-Encoding: chunked"

And noting the request-body/process-request-body defmethod defined for the content type was not being called.

@malcolmsparks
Copy link
Contributor

The reason for the :consumer was where yada users wanted to get access to the stream itself. There are reasons for keeping it. Let me leave this open to consider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants