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.

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

Problems with base_url #285

Open
vincentwoo opened this issue Dec 8, 2015 · 6 comments
Open

Problems with base_url #285

vincentwoo opened this issue Dec 8, 2015 · 6 comments
Labels

Comments

@vincentwoo
Copy link
Contributor

Currently, the docs say that you can use base_url in this way:

# methods are recognized to be prefixed with base_url. Say base_url is

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 #269.

@vincentwoo
Copy link
Contributor Author

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
}

@awood
Copy link

awood commented Dec 17, 2015

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 added a commit to candlepin/candlepin that referenced this issue Dec 17, 2015
@nahi nahi added the BUG label Dec 20, 2015
@nahi
Copy link
Owner

nahi commented Dec 20, 2015

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 nahi removed the BUG label Dec 20, 2015
@nahi
Copy link
Owner

nahi commented Dec 20, 2015

I fixed the document: f8a4680
How do you think?

EDIT: oops, I introduced unnecessary change in the commit. I reverted and created a new commit here: 357df32

@nahi nahi added the Feedback label Dec 20, 2015
@vincentwoo
Copy link
Contributor Author

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.

@fleipold
Copy link

fleipold commented Jan 8, 2016

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?

awood added a commit to candlepin/candlepin that referenced this issue Jan 15, 2016
awood added a commit to candlepin/candlepin that referenced this issue Feb 16, 2016
awood added a commit to candlepin/candlepin that referenced this issue Feb 29, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants