Skip to content

Commit

Permalink
Make dup method clone URI to prevent unwanted mutation
Browse files Browse the repository at this point in the history
  • Loading branch information
dstotz committed Jul 20, 2018
1 parent 794a872 commit f74b0e0
Show file tree
Hide file tree
Showing 3 changed files with 85 additions and 6 deletions.
10 changes: 9 additions & 1 deletion lib/springboard/client/uri.rb
Expand Up @@ -10,7 +10,7 @@ class URI
#
# @return [URI]
def self.parse(value)
return value if value.is_a?(self)
return value.dup if value.is_a?(self)
new(::Addressable::URI.parse(value))
end

Expand All @@ -30,6 +30,14 @@ def initialize(uri)
@uri = uri
end

##
# Clones the URI object
#
# @return [URI]
def dup
self.class.new(@uri.dup)
end

##
# Returns a new URI with the given subpath appended to it. Ensures a single
# forward slash between the URI's path and the given subpath.
Expand Down
45 changes: 44 additions & 1 deletion spec/springboard/client/resource_spec.rb
Expand Up @@ -48,6 +48,12 @@
it "should add bracket notation for array parameters" do
expect(resource.__send__(method, :somearray => [1, 2, 3]).uri.to_s).to eq("/some/path?somearray[]=1&somearray[]=2&somearray[]=3")
end

it "should return a new resource without modifying the existing URI" do
new_resource = resource.query(per_page: 1)
expect(new_resource.uri.to_s).to eq("/some/path?per_page=1")
expect(resource.uri.to_s).to eq("/some/path")
end
end

describe "when called without arguments" do
Expand Down Expand Up @@ -107,7 +113,7 @@
end
end

%w{count each each_page}.each do |method|
%w{each each_page}.each do |method|
describe method do
it "should call the client's #{method} method with the resource's URI" do
expect(client).to receive(method).with(resource.uri)
Expand All @@ -134,6 +140,37 @@
end
end

describe "count" do
let(:response_data) {
{
:status => 200,
:body => {total: 123}.to_json
}
}

it "should call the client's count method with the resource's URI" do
expect(client).to receive('count').with(resource.uri)
resource.__send__('count')
end

it "should set the per_page query string param to 1" do
request_stub = stub_request(:get, "#{base_url}/some/path?page=1&per_page=1").to_return(response_data)
resource.count
expect(request_stub).to have_been_requested
end

it "should return the resource count" do
request_stub = stub_request(:get, "#{base_url}/some/path?page=1&per_page=1").to_return(response_data)
expect(resource.count).to eq(123)
end

it "should not modify the original resource URI" do
request_stub = stub_request(:get, "#{base_url}/some/path?page=1&per_page=1").to_return(response_data)
resource.count
expect(resource.uri.to_s).to eq("/some/path")
end
end

describe "first" do
let(:response_data) {
{
Expand All @@ -152,6 +189,12 @@
request_stub = stub_request(:get, "#{base_url}/some/path?page=1&per_page=1").to_return(response_data)
expect(resource.first).to eq({"id" => "Me first!"})
end

it "should not modify the original resource URI" do
request_stub = stub_request(:get, "#{base_url}/some/path?page=1&per_page=1").to_return(response_data)
resource.first
expect(resource.uri.to_s).to eq("/some/path")
end
end

describe "embed" do
Expand Down
36 changes: 32 additions & 4 deletions spec/springboard/client/uri_spec.rb
Expand Up @@ -12,10 +12,38 @@
end

describe "parse" do
it "should return a URI based on the given string" do
uri = described_class.parse('/some_path')
expect(uri).to be_a(described_class)
expect(uri.to_s).to eq('/some_path')
describe "when called with a URI object" do
it "should return a cloned URI object" do
parsed_uri = described_class.parse(uri)
expect(uri.class).to eq(parsed_uri.class)
expect(uri.object_id).not_to eq(parsed_uri.object_id)
end
end

describe "when called with a URI string" do
it "should return a URI based on the given string" do
uri = described_class.parse('/some_path')
expect(uri).to be_a(described_class)
expect(uri.to_s).to eq('/some_path')
end
end
end

describe "dup" do
it "should return a duplicate URI object" do
dup_uri = uri.dup
expect(uri.class).to eq(dup_uri.class)
expect(uri.to_s).to eq(dup_uri.to_s)
expect(uri.object_id).not_to eq(dup_uri.object_id)
end

describe "when mutating the copy" do
it "should not affect the original" do
dup_uri = uri.dup
dup_uri.query_values = {per_page: 1}
expect(uri.to_s).to eq('/relative/path')
expect(dup_uri.to_s).to eq('/relative/path?per_page=1')
end
end
end

Expand Down

0 comments on commit f74b0e0

Please sign in to comment.