Closed straight-shoota closed 3 years ago
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?
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.
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).
HTTP works by requesting URLs from a server
No:
GET /docs/index.html HTTP/1.1
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.
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.
@asterite /docs/index.html
is the URL being requested.
@straight-shoota it ain't URL, it's a path, there's a difference.
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:
path
and query
it is used as-isabsolute-form
fragment
should always be ignored.
@straight-shoota my bad, fair 'nuff!
So, any other language that accepts an URI
object as a client's instance get
method?
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.
@straight-shoota That's a class method, not an instance method, as far as I can see.
Ah, no, it's an instance method.
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
@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
That works, but you can't configure timeouts and other things.
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?
This should be covered by #6011
As it has come up in #4947 methods like
HTTP::Client#get
require the path parameter to be aString
. If working with anURI
, it should be possible to use that directly instead of having to allURI#full_path
manually. All path arguments inHTTP::Client
actually don't have a type restriction, so this could easily be accomplished by adding an additional constructor toHTTP::Request
(which is invoked byClient#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.