Permalink
Browse files

+ in query parameters is treated as +, not as a space.

  • Loading branch information...
1 parent f3abca6 commit fe1af0c76d353cf3213fe8787ff2c9fc0bc8c208 @bblimke committed Feb 28, 2013
View
2 lib/webmock/http_lib_adapters/excon_adapter.rb
@@ -31,7 +31,7 @@ def self.to_query(hash)
string << key.to_s << '&'
else
for value in [*values]
- string << key.to_s << '=' << CGI.escape(value.to_s) << '&'
+ string << key.to_s << '=' << URI.escape(value.to_s) << '&'
end
end
end
View
2 lib/webmock/util/query_mapper.rb
@@ -66,7 +66,7 @@ def self.query_to_values(query, options={})
value = true if value.nil?
key = Addressable::URI.unencode_component(key)
if value != true
- value = Addressable::URI.unencode_component(value.gsub(/\+/, " "))
+ value = Addressable::URI.unencode_component(value)
@sferik
sferik Mar 1, 2013

Specifically, this change causes the backward incompatibility.

end
if options[:notation] == :flat
if accumulator[key]
View
2 spec/acceptance/excon/excon_spec_helper.rb
@@ -5,7 +5,7 @@ module ExconSpecHelper
def http_request(method, uri, options = {}, &block)
Excon.defaults[:ssl_verify_peer] = false
uri = Addressable::URI.heuristic_parse(uri)
- uri = uri.omit(:userinfo).to_s.gsub(' ', '+')
+ uri = uri.omit(:userinfo).to_s.gsub(' ', '%20')
options = options.merge(:method => method, :nonblock => false) # Dup and merge
response = Excon.new(uri).request(options, &block)
View
2 spec/acceptance/patron/patron_spec_helper.rb
@@ -12,7 +12,7 @@ def http_request(method, uri, options = {}, &block)
sess.timeout = 30
sess.max_redirects = 0
uri = "#{uri.path}#{uri.query ? '?' : ''}#{uri.query}"
- uri.gsub!(' ','+')
+ uri.gsub!(' ','%20')
response = sess.request(method, uri, options[:headers] || {}, {
:data => options[:body]
})
View
9 spec/acceptance/shared/stubbing_requests.rb
@@ -15,6 +15,15 @@
stub_request(:get, /.*x=ab c.*/).to_return(:body => "abc")
http_request(:get, "http://www.example.com/hello/?#{ESCAPED_PARAMS}").body.should == "abc"
end
+
+ it "should raise error specifying stubbing instructions with escaped characters in params if there is no matching stub" do
+ begin
+ http_request(:get, "http://www.example.com/hello/?#{NOT_ESCAPED_PARAMS}")
+ rescue WebMock::NetConnectNotAllowedError => e
+ e.message.should match /Unregistered request: GET http:\/\/www\.example\.com\/hello\/\?x=ab%20c&z='Stop!'%20said%20Fred\+m/m
+ e.message.should match /stub_request\(:get, "http:\/\/www\.example\.com\/hello\/\?x=ab%20c&z='Stop!'%20said%20Fred\+m"\)/m
+ end
+ end
end
describe "based on query params" do
View
4 spec/acceptance/webmock_shared.rb
@@ -10,8 +10,8 @@
unless defined? SAMPLE_HEADERS
SAMPLE_HEADERS = { "Content-Length" => "8888", "Accept" => "application/json" }
- ESCAPED_PARAMS = "x=ab%20c&z=%27Stop%21%27%20said%20Fred"
- NOT_ESCAPED_PARAMS = "z='Stop!' said Fred&x=ab c"
+ ESCAPED_PARAMS = "x=ab%20c&z=%27Stop%21%27%20said%20Fred%2Bm"
+ NOT_ESCAPED_PARAMS = "z='Stop!' said Fred+m&x=ab c"
end
shared_examples "with WebMock" do |*adapter_info|

5 comments on commit fe1af0c

@sferik

👎 😦 💢 🔴 👊 💥 This change is causing my tests to fail. If you want to make this change, please release it in a new major version (i.e. 2.0.0)—in compliance with Semantic Versioning—so I can stay locked to ~> 1.0 until I get a chance to upgrade.

If this breakage was unintentional, please revert it and release 1.10.1 or 1.11.0.

Thanks!

@craiglittle

👍 This is also causing my tests to break.

@bblimke
Owner

I was wondering if I'm going to cause a storm with this change, but I didn't expect as many emoticons ;)

This is indeed breaking change, but it's actually a bug fix.
This change fixes incorrect behaviour that existed in all previous versions of WebMock from beginning.

Here is the pull request/issue which led to this change #214

What does Semantic Versioning say about incompatible bug fixes? :)

@bblimke
Owner

Unless there is a bug introduced with this commit and that's why your tests fail. An example would be useful.

@sferik

@bblimke I've added an example to the issue I created about this commit (#259). It probably makes more sense to continue this discussion over there. For what it's worth, I'm feeling much less emotional now than I was earlier.

Please sign in to comment.