Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix issue where empty query key causes a `TypeError` #352

Merged
merged 2 commits into from

2 participants

@JonRowe

As seen in the real world (tm), sometimes queries have values but no keys which
causes webmock to blow up with a TypeError:

lib/webmock/util/query_mapper.rb:187:in `dup': can't dup NilClass (TypeError)
  from ./gems/webmock-1.15.2/lib/webmock/util/query_mapper.rb:187:in `block in values_to_query'
  from ./gems/webmock-1.15.2/lib/webmock/util/query_mapper.rb:185:in `each'
  from ./gems/webmock-1.15.2/lib/webmock/util/query_mapper.rb:185:in `values_to_query'
  from ./gems/webmock-1.15.2/lib/webmock/util/uri.rb:18:in `block in <class:URI>'
  from ./gems/webmock-1.15.2/lib/webmock/util/uri.rb:33:in `yield'
  from ./gems/webmock-1.15.2/lib/webmock/util/uri.rb:33:in `normalize_uri'
  from ./gems/webmock-1.15.2/lib/webmock/request_signature.rb:10:in `initialize'
  from ./gems/webmock-1.15.2/lib/webmock/http_lib_adapters/net_http.rb:290:in `new'
  from ./gems/webmock-1.15.2/lib/webmock/http_lib_adapters/net_http.rb:290:in `request_signature_from_request'
  from ./gems/webmock-1.15.2/lib/webmock/http_lib_adapters/net_http.rb:75:in `request'
  from ./gems/net-http-persistent-2.9/lib/net/http/persistent.rb:986:in `request'
  from ./bundler/gems/mechanize-3c0a3714926e/lib/mechanize/http/agent.rb:259:in `fetch'
  from ./bundler/gems/mechanize-3c0a3714926e/lib/mechanize.rb:440:in `get'

This resolves that by casting nil to a string as with Symbol.

@bblimke
Owner

Is it allowed to have blank query keys in the url according to the spec?

@JonRowe

A query string is any character between ? and # citation with the only further thing said "often used to carry identifying information in the form of 'key=value' pair" however, URI.parse handles it, and it's seen in the wild, and as a testing framework I think this should be handled gracefully, and this is the best effort at that.

An alternative would be to validate the format of the query string before attempting to split it into key value pairs, as the query string could then just be left alone.

@bblimke
Owner

Ok, looks good. Cheers.

@bblimke bblimke merged commit 3c670aa into bblimke:master

1 check failed

Details default The Travis CI build could not complete due to an error
@JonRowe JonRowe deleted the JonRowe:fixup_query_mapper branch
@JonRowe

Thanks :)

@bblimke
Owner

Released as 1.17.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
4 lib/webmock/util/query_mapper.rb
@@ -131,8 +131,8 @@ def self.values_to_query(new_query_values)
end
new_query_values = new_query_values.to_hash
new_query_values = new_query_values.map do |key, value|
- key = key.to_s if key.kind_of?(Symbol)
- [key, value]
+ key = key.to_s if key.kind_of?(Symbol) || key.nil?
+ [key.to_s, value]
end
# Useful default for OAuth and caching.
# Only to be used for non-Array inputs. Arrays should preserve order.
View
30 spec/unit/util/query_mapper_spec.rb
@@ -0,0 +1,30 @@
+require 'spec_helper'
+
+describe WebMock::Util::QueryMapper do
+ let(:query_mapper) { described_class }
+
+ it "converts query to values" do
+ query = "key=value&other_key=other_value"
+ values = { 'key' => 'value', 'other_key' => 'other_value' }
+ expect(query_mapper.query_to_values query).to eq values
+ end
+
+ it 'converts values to a query string' do
+ query = "key=value&other_key=other_value"
+ values = [['key','value'],['other_key','other_value']]
+ expect(query_mapper.values_to_query values).to eq query
+ end
+
+ it 'converts values with missing keys to a query string' do
+ query = "=value"
+ values = { '' => 'value' }
+ expect(query_mapper.values_to_query values).to eq query
+ end
+
+ it 'converts values with nil keys to a query string' do
+ query = "=value"
+ values = { nil => 'value' }
+ expect(query_mapper.values_to_query values).to eq query
+ end
+
+end
Something went wrong with that request. Please try again.