Set default host in HTTP::Client#exec

[straight-shoota]

Fixes #6480

[asterite]

Just note that even though this is a good change, the original issue is not solved: the use case was to send a request to multiple hosts by reusing the request instance. Because the host is only set if it's not set already, that won't work.

Maybe we should remove the exec methods from being public. I don't think there's a sane way to reuse request objects, unless we dup/clone them in exec, but that's of course not ideal.

[straight-shoota]

Yeah, I figured as much. But the original issue can be solved by dup-ing the request when providing it to exec: client.exec(request.dup).

Obviously, this is not ideal, but it works fine for that use case. I'd suggest to merge this fix and continue with #6011 to redesign HTTP client.

[asterite]

I would recommend then documenting this behavior, mentioning that reusing an HTTP::Request shouldn't be done because the Host header is set once per request. That's why I would maybe prefer to hide the exec method, and so there's no way to misuse the API.

[straight-shoota]

An alternative solution would be to modify HTTP.serialize_headers_and_body to insert default host argument provided as argument if it is missing from headers.

[z64]

@asterite re: removing exec(request : HTTP::Request)

For what it's worth, I've made good use of this method in libs. It's been helpful to construct a namespace of HTTP::Request factory methods that my own Client implements in order to spec/unit test the requests nicely against API documentation without having to actually execute them, and where building a mock API service would be much more expensive (as in time consuming) task, and this is just as likely to ensure I've written working runtime code.

Basically:

# requests.cr
module Request
  extend self

  def get_resource(id : ID)
    HTTP::Request.new("GET", "/resource?id=#{id}")
  end
end

# client.cr
class Client
  @http_client : HTTP::Client

  def exec(request : HTTP::Request)
    @http_client.exec(request)
  end

  def get_resource(id : ID)
    response = Request.get_resource(id)
    Resource.from_json(response.body)
  end
end

# request_spec.cr
it "#get_resource" do
  request = Request.get_resource(1)
  request.method.should eq "GET"
  request.path.should eq "/resource"
  request.query.should eq "id=1"
end

I could replace this with my own types if removed with a slightly more complicated interface, but having exec(request : HTTP::Request) has been helpful in this regard, where things like rspecs expect(api_client).to receive(:get_resource).with(resource_id) doesn't exist in crystal

Maybe mock connections via #6011 would provide a equally simple solution, but that's still an abstract bullet point on the list, so I don't know

[newtonapple]

Correct me if I'm wrong, by removing HTTP::Client#exec(request : HTTP::Request) from the public API, it will effectively eliminate all public use cases / needs for HTTP::Request, correct? I personally think having a reusable HTTP::Request can be quite useful a number of scenarios.

[RX14]

A reusable HTTP::Request doesn't make sense because the body is an IO, which is consumed on use.