Allow HTTP::Client methods to receive a URI object

[straight-shoota]

As it has come up in #4947 methods like HTTP::Client#get require the path parameter to be a String. If working with an URI, it should be possible to use that directly instead of having to all URI#full_path manually.
All path arguments in HTTP::Client actually don't have a type restriction, so this could easily be accomplished by adding an additional constructor to HTTP::Request (which is invoked by Client#new_request) to accept an URI. Internally, the request resource is stored as an URI anyway, so it could theoretically be used as instance variable directly. But it will probably need some cleanup to remove host and port which are not part of the request URI.

[asterite]

I appreciate the effort to try to improve things, but #4947 was closed with a perfectly valid solution. You just pass uri.full_path. If we allow passing a URI, then what happens with host and scheme? Silently ignored? Super bad. In fact, I think creating an HTTP::Client from an URI is already a smell, except that this might be in a configuration so creating a client from that as a "base" URI is fine.

Please, I'd like to ask you to stop creating issues for every small detail, specially if you are not the one "suffering" these problems. There's already #4947 to discuss that, why open yet another issue to discuss the same thing?

[asterite]

Or maybe wait a bit more before opening these issues. Only one user "complained" about this, and very recently, but a solution was given. I see you opened other issues, like #4613, which reference many issues around a single source of problems, and that's good as a tracking issue. But in this case it was super minor and with a solution.

[straight-shoota]

It requires extra effort to understand you have to use url.full_path (instead of maybe url.to_s which might suggest itself) on the URI and I'd find it logical to use a URI as resource argument for a HTTP request. HTTP works by requesting URLs from a server, therefore I'd expect to pass a URI object to the request method (this is already working in the class methods, but not for the instance methods).

A valid URI (or in this case, more precisely a URL) doesn't need to be absolute, so it could only be containing values that would be represented in full_path. In this case there would be no problem whatsoever. But the path argument to Client#get etc. is expected to be a URL so why shouldn't it allow an object of type URI ?
It would need to be decided what happens if there are e.g. a host or port specified. It could throw an exception instead of silently ignoring the values. It would make sense though to allow values that match the current connection of the client.

About the proceedings: Yes, only one user complained about it. But it is likely, that this kind of problem will emerge again. And I recognized this as what I believe is a design error. I think your solution is more a workaround and it should be possible to use a URI directly. That's why I created an issue for this, because I figured this shouldn't be continued in the original issue which was about finding a currently working solution (and should have been posted on SO of course).

[asterite]

HTTP works by requesting URLs from a server

No:

GET /docs/index.html HTTP/1.1

[Thyra]

If you decide against it, I think it would be wise to include an example with URI#full_path in the HTTP::Client documentation. Because I read all docs and still would probably never have come up with that solution myself.

[asterite]

StackOverflow is a perfect website for this. It's a huge questions and answers, and examples, collection. I don't think it's possible to document every use case in the docs. And even if we do so, StackOverflow is more organic and there's no need for us to change docs and release stuff.

[straight-shoota]

@asterite /docs/index.html is the URL being requested.

[Sija]

@straight-shoota it ain't URL, it's a path, there's a difference.

[straight-shoota]

No, it's not just a path. It is - in terms of the HTTP specification - a Request Target that identifies the requested resource. It usually consists of an absolute path and optional query.

In Crystal, this can be represented as a plain string or as a URI.

According to section 5.3.2 of RFC 7230 clients MUST use absolute URIs (i.e. including host etc.) when making a request to a proxy. So depending on the connection, a full URI might actually be what's required as path argument. This even supports my point of allowing to receiving a URI. With the suggested behaviour:

• If it is a relative URL containing only path and query it is used as-is
• If it is an absolute URL and the authority part and scheme matches the current connection, then only the local part is used (path + query)
• Otherwise the absolute URL is used as-is to allow proxy requests using absolute-form

fragment should always be ignored.

[Sija]

@straight-shoota my bad, fair 'nuff!

[asterite]

So, any other language that accepts an URI object as a client's instance get method?

[straight-shoota]

Don't need to look any further than Crystal's big brother.

Net::HTTP#get: Retrieves data from path on the connected-to host which may be an absolute path String or a URI to extract the path from.

The URI handling part is in Net::HTTPGenericRequest.

Btw. in Ruby's URI class, path + query is called request_uri instead of full_path in Crystal. I don't think any of these names are really good, but full_path is worse because it's just not a only path.

[asterite]

@straight-shoota That's a class method, not an instance method, as far as I can see.

[asterite]

Ah, no, it's an instance method.

[asterite]

Because I closed #1821 in favor of this I'd like to include the current solution:

require "http/client"

uri = URI.parse("http://api.icndb.com/jokes/random?firstName=John&lastName=Doe")

HTTP::Client.new(uri) do |client|
  response = client.get uri.full_path
  puts response.body
end

[Sija]

@asterite what about this:

require "http/client"

uri = URI.parse("http://api.icndb.com/jokes/random?firstName=John&lastName=Doe")

HTTP::Client.get(uri) do |response|
  puts response.body
end

[asterite]

That works, but you can't configure timeouts and other things.

[miketheman]

#codetriage
Reading through the history here, it feels like there's some confusion as to how HTTP::Client#get ought to work.

There appear to be a few approaches, all of which seem like they ought to work the qay the reader wants them to.

require "http/client"
require "json"

## working:
response = HTTP::Client.get uri
body = JSON.parse(response.body)
puts body["value"]["joke"]

## working, but feels repetitive to re-use `uri` and have to use the `.full_path` method
HTTP::Client.new(uri) do |client|
  response = client.get uri.full_path
  puts response.body
end

# non-working, as the response is `body_io` within a block
HTTP::Client.get(uri) do |response|
  puts response.body
end

## working
HTTP::Client.get(uri) do |response|
  puts response.body_io.gets
end

So the methods involved appear to accept a URI and behave correctly.

What's the remaining open questions to be answered to close out this issue?

[straight-shoota]

This should be covered by #6011