httpclient icon indicating copy to clipboard operation
httpclient copied to clipboard

Problems with base_url

Open vincentwoo opened this issue 9 years ago • 6 comments

Currently, the docs say that you can use base_url in this way: https://github.com/nahi/httpclient/blob/837d59f1ad19d5940c810e34e408d15897d5bacb/lib/httpclient.rb#L395

Reproduced here:

  # After you set :base_url, all resources you pass to get, post and other
  # methods are recognized to be prefixed with base_url. Say base_url is
  # 'https://api.example.com/v1, get('/users') is the same as
  # get('https://api.example.com/v1/users') internally.

However, this is actually untrue - the base url must end with a slash or it will be ignored:

[24] pry(main)> http.base_url = 'https://test.com/v1'
"https://test.com/v1"
[25] pry(main)> http.send :to_resource_url, 'test'
{
      :scheme => "https",
        :user => nil,
    :password => nil,
        :host => "test.com",
        :port => 443,
        :path => "/test",
       :query => nil,
    :fragment => nil
}
[26] pry(main)> http.send :to_resource_url, '/test'
{
      :scheme => "https",
        :user => nil,
    :password => nil,
        :host => "test.com",
        :port => 443,
        :path => "/test",
       :query => nil,
    :fragment => nil
}

This problem occurs on version 2.7.0.1 and may be related to https://github.com/nahi/httpclient/pull/269.

vincentwoo avatar Dec 08 '15 08:12 vincentwoo

However, note that if you set the trailing slash in base_url, everything seems to work more or less how you'd expect:

[27] pry(main)> http.base_url = 'https://test.com/v1/'
"https://test.com/v1/"
[28] pry(main)> http.send :to_resource_url, 'test'
{
      :scheme => "https",
        :user => nil,
    :password => nil,
        :host => "test.com",
        :port => 443,
        :path => "/v1/test",
       :query => nil,
    :fragment => nil
}
[29] pry(main)> http.send :to_resource_url, '/test'
{
      :scheme => "https",
        :user => nil,
    :password => nil,
        :host => "test.com",
        :port => 443,
        :path => "/test",
       :query => nil,
    :fragment => nil
}

vincentwoo avatar Dec 08 '15 08:12 vincentwoo

Note too that the appended resource cannot have a leading slash.

[24] pry(#<JSONClient>)> urify("https://localhost:8443/context/") + "/hi"
=> #<URI::HTTPS https://localhost:8443/hi>
[25] pry(#<JSONClient>)> urify("https://localhost:8443/context/") + "hi"
=> #<URI::HTTPS https://localhost:8443/context/hi>

This issue looks to be a regression since it was not present in version 2.5.2 which I had been using.

Looks like to_resource_url went from

urify(@base_url + uri)

to

urify(@base_url) + uri

awood avatar Dec 17 '15 21:12 awood

Thanks @vincentwoo and @awood, and sorry for the regression. Obviously the document is obviously untrue.

Expected behavior for me and OK/NG of 2.7.0.1 implementation.

1. http://foo/bar/baz + path -> http://foo/bar/bazpath => NG (http://foo/bar/path)
2. http://foo/bar/baz/ + path -> http://foo/bar/baz/path => OK
3. http://foo/bar/baz + ../path -> http://foo/path => OK
4. http://foo/bar/baz/ + ../path -> http://foo/bar/path => OK
5. http://foo/bar/baz + ./path -> http://foo/bar/path => OK
6. http://foo/bar/baz/ + ./path -> http://foo/bar/baz/path => OK
7. http://foo/bar/baz + /path -> http://foo/path => OK

I'm unsure if my above expectation No.1 is right or not. I'm now thinking that current implementation URI.join(@base_url, path) is acceptable if I write it clear at the document.

Anyway it was a breaking change and sorry for any inconvenience it caused.

nahi avatar Dec 20 '15 08:12 nahi

I fixed the document: https://github.com/nahi/httpclient/commit/f8a4680836dfa8ee23a2382360aee990c9280231 How do you think?

EDIT: oops, I introduced unnecessary change in the commit. I reverted and created a new commit here: https://github.com/nahi/httpclient/commit/357df32b87417714b3f1b6219d04e610ca1e9385

nahi avatar Dec 20 '15 08:12 nahi

I think the feature is now more confusing, but if you believe this to be the correct decision, it's your call. I think the docs do not cover what happens when:

http://foo/bar + path

Should you get

http://foo/bar/path or http://foo/barpath? Ambiguous from the examples.

vincentwoo avatar Dec 21 '15 10:12 vincentwoo

http://foo/bar + /path => http://foo/path

Is a severe regression against 2.6.1 which produced http:/foo/bar/path. Is there any intention maintain backwards compatibility?

fleipold avatar Jan 08 '16 00:01 fleipold