Skip to content

Commit

Permalink
fix: avoid double slashes in urls used for API calls.
Browse files Browse the repository at this point in the history
- Added relevant unit tests.
- Note: this commit changes behaviour if you have configured a
  base url ending with a single slash.
  If you still want to connect to endpoints with two slashes at the
  end of the baseurl (for whatever reason), then you need to
  terminate the url parameter with two slashes.
  • Loading branch information
rarruda committed Oct 4, 2021
1 parent b777c56 commit a10d07c
Show file tree
Hide file tree
Showing 7 changed files with 60 additions and 40 deletions.
2 changes: 1 addition & 1 deletion lib/unleash/client.rb
Expand Up @@ -150,7 +150,7 @@ def register

# Send the request, if possible
begin
response = Unleash::Util::Http.post(Unleash.configuration.client_register_url, info.to_json)
response = Unleash::Util::Http.post(Unleash.configuration.client_register_uri, info.to_json)
rescue StandardError => e
Unleash.logger.error "unable to register client with unleash server due to exception #{e.class}:'#{e}'."
Unleash.logger.error "stacktrace: #{e.backtrace}"
Expand Down
18 changes: 11 additions & 7 deletions lib/unleash/configuration.rb
Expand Up @@ -52,18 +52,22 @@ def http_headers
}.merge(custom_http_headers.dup)
end

def fetch_toggles_url
project_param = (self.project_name.nil? ? "" : "?project=#{self.project_name}")
def fetch_toggles_uri
uri = URI("#{self.url_stripped_of_slash}/client/features")
uri.query = "project=#{self.project_name}" unless self.project_name.nil?
uri
end

"#{self.url}/client/features#{project_param}"
def client_metrics_uri
URI("#{self.url_stripped_of_slash}/client/metrics")
end

def client_metrics_url
"#{self.url}/client/metrics"
def client_register_uri
URI("#{self.url_stripped_of_slash}/client/register")
end

def client_register_url
"#{self.url}/client/register"
def url_stripped_of_slash
self.url.delete_suffix '/'
end

private
Expand Down
2 changes: 1 addition & 1 deletion lib/unleash/metrics_reporter.rb
Expand Up @@ -36,7 +36,7 @@ def generate_report
def send
Unleash.logger.debug "send() Report"

response = Unleash::Util::Http.post(Unleash.configuration.client_metrics_url, self.generate_report.to_json)
response = Unleash::Util::Http.post(Unleash.configuration.client_metrics_uri, self.generate_report.to_json)

if ['200', '202'].include? response.code
Unleash.logger.debug "Report sent to unleash server sucessfully. Server responded with http code #{response.code}"
Expand Down
2 changes: 1 addition & 1 deletion lib/unleash/toggle_fetcher.rb
Expand Up @@ -36,7 +36,7 @@ def toggles
# rename to refresh_from_server! ??
def fetch
Unleash.logger.debug "fetch()"
response = Unleash::Util::Http.get(Unleash.configuration.fetch_toggles_url, etag)
response = Unleash::Util::Http.get(Unleash.configuration.fetch_toggles_uri, etag)

if response.code == '304'
Unleash.logger.debug "No changes according to the unleash server, nothing to do."
Expand Down
6 changes: 2 additions & 4 deletions lib/unleash/util/http.rb
Expand Up @@ -4,17 +4,15 @@
module Unleash
module Util
module Http
def self.get(url, etag = nil)
uri = URI(url)
def self.get(uri, etag = nil)
http = http_connection(uri)

request = Net::HTTP::Get.new(uri.request_uri, http_headers(etag))

http.request(request)
end

def self.post(url, body)
uri = URI(url)
def self.post(uri, body)
http = http_connection(uri)

request = Net::HTTP::Post.new(uri.request_uri, http_headers)
Expand Down
42 changes: 21 additions & 21 deletions spec/unleash/client_spec.rb
Expand Up @@ -2,7 +2,7 @@

RSpec.describe Unleash::Client do
it "Uses custom http headers when initializing client" do
WebMock.stub_request(:post, "http://test-url//client/register")
WebMock.stub_request(:post, "http://test-url/client/register")
.with(
headers: {
'Accept' => '*/*',
Expand All @@ -13,7 +13,7 @@
}
)
.to_return(status: 200, body: "", headers: {})
WebMock.stub_request(:post, "http://test-url//client/metrics")
WebMock.stub_request(:post, "http://test-url/client/metrics")
.with(
headers: {
'Accept' => '*/*',
Expand All @@ -23,7 +23,7 @@
}
)
.to_return(status: 200, body: "", headers: {})
WebMock.stub_request(:get, "http://test-url//client/features")
WebMock.stub_request(:get, "http://test-url/client/features")
.with(
headers: {
'Accept' => '*/*',
Expand Down Expand Up @@ -54,15 +54,15 @@
expect(unleash_client).to be_a(Unleash::Client)

expect(
a_request(:post, "http://test-url//client/register")
a_request(:post, "http://test-url/client/register")
.with(headers: { 'Content-Type': 'application/json' })
.with(headers: { 'X-API-KEY': '123', 'Content-Type': 'application/json' })
.with(headers: { 'UNLEASH-APPNAME': 'my-test-app' })
.with(headers: { 'UNLEASH-INSTANCEID': 'rspec/test' })
).to have_been_made.once

expect(
a_request(:get, "http://test-url//client/features")
a_request(:get, "http://test-url/client/features")
.with(headers: { 'X-API-KEY': '123' })
.with(headers: { 'UNLEASH-APPNAME': 'my-test-app' })
.with(headers: { 'UNLEASH-INSTANCEID': 'rspec/test' })
Expand All @@ -71,7 +71,7 @@
# Test now sending of metrics
Unleash.reporter.send
expect(
a_request(:post, "http://test-url//client/metrics")
a_request(:post, "http://test-url/client/metrics")
.with(headers: { 'Content-Type': 'application/json' })
.with(headers: { 'X-API-KEY': '123', 'Content-Type': 'application/json' })
.with(headers: { 'UNLEASH-APPNAME': 'my-test-app' })
Expand All @@ -80,7 +80,7 @@
end

it "should load/use correct variants from the unleash server" do
WebMock.stub_request(:post, "http://test-url//client/register")
WebMock.stub_request(:post, "http://test-url/client/register")
.with(
headers: {
'Accept' => '*/*',
Expand Down Expand Up @@ -121,7 +121,7 @@
]
}'

WebMock.stub_request(:get, "http://test-url//client/features")
WebMock.stub_request(:get, "http://test-url/client/features")
.with(
headers: {
'Accept' => '*/*',
Expand Down Expand Up @@ -156,12 +156,12 @@
).to eq(true)

expect(WebMock).not_to have_requested(:get, 'http://test-url/')
expect(WebMock).to have_requested(:post, 'http://test-url//client/register')
expect(WebMock).to have_requested(:get, 'http://test-url//client/features')
expect(WebMock).to have_requested(:post, 'http://test-url/client/register')
expect(WebMock).to have_requested(:get, 'http://test-url/client/features')
end

it "should not fail if we are provided no toggles from the unleash server" do
WebMock.stub_request(:post, "http://test-url//client/register")
WebMock.stub_request(:post, "http://test-url/client/register")
.with(
headers: {
'Accept' => '*/*',
Expand All @@ -173,7 +173,7 @@
)
.to_return(status: 200, body: "", headers: {})

WebMock.stub_request(:get, "http://test-url//client/features")
WebMock.stub_request(:get, "http://test-url/client/features")
.with(
headers: {
'Accept' => '*/*',
Expand Down Expand Up @@ -207,9 +207,9 @@
).to eq(true)

expect(WebMock).not_to have_requested(:get, 'http://test-url/')
expect(WebMock).to have_requested(:get, 'http://test-url//client/features')
expect(WebMock).to have_requested(:post, 'http://test-url//client/register')
expect(WebMock).not_to have_requested(:post, 'http://test-url//client/metrics')
expect(WebMock).to have_requested(:get, 'http://test-url/client/features')
expect(WebMock).to have_requested(:post, 'http://test-url/client/register')
expect(WebMock).not_to have_requested(:post, 'http://test-url/client/metrics')
end

it "should return default results via block or param if running with disable_client" do
Expand Down Expand Up @@ -353,10 +353,10 @@
).to eq(true)

expect(WebMock).not_to have_requested(:get, 'http://test-url/')
expect(WebMock).not_to have_requested(:get, 'http://test-url//client/features')
expect(WebMock).not_to have_requested(:post, 'http://test-url//client/features')
expect(WebMock).not_to have_requested(:post, 'http://test-url//client/register')
expect(WebMock).not_to have_requested(:post, 'http://test-url//client/metrics')
expect(WebMock).not_to have_requested(:get, 'http://test-url/client/features')
expect(WebMock).not_to have_requested(:post, 'http://test-url/client/features')
expect(WebMock).not_to have_requested(:post, 'http://test-url/client/register')
expect(WebMock).not_to have_requested(:post, 'http://test-url/client/metrics')
end

it "should yield correctly to block when using if_enabled" do
Expand Down Expand Up @@ -400,7 +400,7 @@
}

before do
WebMock.stub_request(:post, "http://test-url//client/register")
WebMock.stub_request(:post, "http://test-url/client/register")
.with(
headers: {
'Accept' => '*/*',
Expand All @@ -412,7 +412,7 @@
)
.to_return(status: 200, body: '', headers: {})

WebMock.stub_request(:get, "http://test-url//client/features")
WebMock.stub_request(:get, "http://test-url/client/features")
.with(
headers: {
'Accept' => '*/*',
Expand Down
28 changes: 23 additions & 5 deletions spec/unleash/configuration_spec.rb
Expand Up @@ -51,20 +51,38 @@
expect{ Unleash.configuration.validate! }.not_to raise_error
expect(Unleash.configuration.url).to eq('http://test-url/')
expect(Unleash.configuration.app_name).to eq('my-test-app')
expect(Unleash.configuration.fetch_toggles_url).to eq('http://test-url//client/features')
expect(Unleash.configuration.fetch_toggles_uri.to_s).to eq('http://test-url/client/features')
expect(Unleash.configuration.client_metrics_uri.to_s).to eq('http://test-url/client/metrics')
expect(Unleash.configuration.client_register_uri.to_s).to eq('http://test-url/client/register')
end

it "should build the correct unleash endpoints from the base url" do
config = Unleash::Configuration.new(url: 'https://testurl/api', app_name: 'test-app')
expect(config.url).to eq('https://testurl/api')
expect(config.fetch_toggles_url).to eq('https://testurl/api/client/features')
expect(config.client_metrics_url).to eq('https://testurl/api/client/metrics')
expect(config.client_register_url).to eq('https://testurl/api/client/register')
expect(config.fetch_toggles_uri.to_s).to eq('https://testurl/api/client/features')
expect(config.client_metrics_uri.to_s).to eq('https://testurl/api/client/metrics')
expect(config.client_register_uri.to_s).to eq('https://testurl/api/client/register')
end

it "should build the correct unleash endpoints from a base url ending with slash" do
config = Unleash::Configuration.new(url: 'https://testurl/api/', app_name: 'test-app')
expect(config.url).to eq('https://testurl/api/')
expect(config.fetch_toggles_uri.to_s).to eq('https://testurl/api/client/features')
expect(config.client_metrics_uri.to_s).to eq('https://testurl/api/client/metrics')
expect(config.client_register_uri.to_s).to eq('https://testurl/api/client/register')
end

it "should build the correct unleash endpoints from a base url ending with double slashes" do
config = Unleash::Configuration.new(url: 'https://testurl/api//', app_name: 'test-app')
expect(config.url).to eq('https://testurl/api//')
expect(config.fetch_toggles_uri.to_s).to eq('https://testurl/api//client/features')
expect(config.client_metrics_uri.to_s).to eq('https://testurl/api//client/metrics')
expect(config.client_register_uri.to_s).to eq('https://testurl/api//client/register')
end

it "should build the correct unleash features endpoint when project_name is used" do
config = Unleash::Configuration.new(url: 'https://testurl/api', app_name: 'test-app', project_name: 'test-project')
expect(config.fetch_toggles_url).to eq('https://testurl/api/client/features?project=test-project')
expect(config.fetch_toggles_uri.to_s).to eq('https://testurl/api/client/features?project=test-project')
end

it "should allow hashes for custom_http_headers via yield" do
Expand Down

0 comments on commit a10d07c

Please sign in to comment.