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.

Sign up for GitHub

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

Jump to bottom

Improve error message when body size of a request is unknown #560

Closed
wants to merge 1 commit into from
Closed

Improve error message when body size of a request is unknown #560

wants to merge 1 commit into from

Conversation

savonarola
Copy link

@savonarola savonarola commented Aug 8, 2019 •
edited
Loading

Hello!
Recently I tried to send an Enumerable object as a body for a request:

http.put(url, body: enumerable_instance)

This is a valid body according to the code: https://github.com/httprb/http/blob/master/lib/http/request/body.rb#L76

def validate_source_type!
  return if @source.is_a?(String)
  return if @source.respond_to?(:read)
  return if @source.is_a?(Enumerable)
  return if @source.nil?

  raise RequestError, "body of wrong type: #{@source.class}"
end

But the request failed with the exception cannot determine size of body: ...

Indeed, although Enumerable is a valid option for body, there is no corresponding case in #size function: https://github.com/httprb/http/blob/master/lib/http/request/body.rb#L17

def size
  if @source.is_a?(String)
    @source.bytesize
  elsif @source.respond_to?(:read)
    raise RequestError, "IO object must respond to #size" unless @source.respond_to?(:size)

    @source.size
  elsif @source.nil?
    0 
  else
    raise RequestError, "cannot determine size of body: #{@source.inspect}"
  end
end

It seemed a bit vague for me and took some time to figure out that in this case one should set Content-Length header explicitly so that Body#size is not called: https://github.com/httprb/http/blob/master/lib/http/request/writer.rb#L50

def add_body_type_headers
  return if @headers[Headers::CONTENT_LENGTH] || chunked?

  @request_header << "#{Headers::CONTENT_LENGTH}: #{@body.size}"
end

It seemed for me that it would be useful to have some hint on it in the error message.

@ixti
Copy link
Member

ixti commented Aug 9, 2019

IMO this details should not be in exception details. It belongs to documentation/wiki.

@ixti
Copy link
Member

ixti commented Aug 9, 2019

@httprb/core WDYT?

@ixti
Copy link
Member

ixti commented Aug 9, 2019

I think for bodies that do not support #size we should use (force?) chunked encoding instead. Otherwise we might end up with user setting Content-Length that is invalid (e.g. smaller or bigger than the actual size is).

@janko
Copy link
Member

janko commented Aug 9, 2019

I think for bodies that do not support #size we should use (force?) chunked encoding instead.

👍The user should still be able to specify Content-Length, but if they haven't we should default to Transfer-Encoding: chunked.

On a related note, I also think that for IO-like objects we don't need to require #size.

http/lib/http/request/body.rb

Line 21 in 6240672

raise RequestError, "IO object must respond to #size" unless @source.respond_to?(:size)

@ixti
Copy link
Member

ixti commented Aug 9, 2019

@janko I agree.

@tarcieri tarcieri deleted the branch httprb:master September 10, 2021 15:21
@tarcieri tarcieri closed this Sep 10, 2021
@savonarola
Copy link
Author

Hello!

Just wanted to clarify, is this issue still worth being opened? As far as I can see, it was automatically closed because of the main branch rename.

@ixti
Copy link
Member

ixti commented Sep 14, 2021 via email

ixti added a commit that referenced this pull request Sep 14, 2021
@ixti
chore: Improve implicit Content-Length failures
00bc96a 
Suggest setting `Content-Length` header manually in case we can't guess
body size.

See-Also: #560
@ixti ixti mentioned this pull request Sep 14, 2021
Draft
@ixti
Copy link
Member

ixti commented Sep 14, 2021

Reopened as #689

ixti added a commit that referenced this pull request Sep 14, 2021
@ixti @savonarola
chore: Improve implicit Content-Length failures
c603521 
Suggest setting `Content-Length` header manually in case we can't guess
body size.

See-Also: #560
Co-Authored-By: @savonarola <[email protected]>
@tarcieri
Copy link
Member

Yep, sorry, pardon my dust

ixti added a commit that referenced this pull request Sep 14, 2021
@ixti @savonarola
chore: Improve implicit Content-Length failures
d4a4348 
Suggest setting `Content-Length` header manually in case we can't guess
body size.

See-Also: #560
Co-Authored-By: @savonarola <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@savonarola @ixti @janko @tarcieri