From 97e8e8484e11c367ccfededecc2db327bc607bc8 Mon Sep 17 00:00:00 2001
From: Marcos Wright-Kuhns <mwrightkuhns@newrelic.com>
Date: Mon, 30 Nov 2020 07:26:10 -0800
Subject: [PATCH] Handle non-string values sent to set_required_query_params

---
 lib/oembed/provider.rb  | 21 ++++++++++++++++--
 lib/oembed/providers.rb |  2 +-
 spec/provider_spec.rb   | 47 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/lib/oembed/provider.rb b/lib/oembed/provider.rb
index fa02d84..b5f03a0 100644
--- a/lib/oembed/provider.rb
+++ b/lib/oembed/provider.rb
@@ -71,6 +71,7 @@ def initialize(endpoint, positional_format = OEmbed::Formatter.default, format:
         define_singleton_method("#{param}") { @required_query_params[param] } unless respond_to?("#{param}")
         define_singleton_method("#{param}=") { |val| set_required_query_params(param, val) } unless respond_to?("#{param}=")
       end
+      required_query_params_set?(reset_cache: true)
 
       @endpoint = endpoint
       @urls = []
@@ -96,10 +97,26 @@ def <<(url)
       @urls << url
     end
 
+    # Given the name of a required_query_param and a value
+    # store that value internally, so that it can be sent along
+    # with requests to this provider's endpoint.
+    # Raises an ArgumentError if the given param is not listed with required_query_params
+    # during instantiation.
     def set_required_query_params(param, val)
       raise ArgumentError.new("This provider does NOT have a required_query_param named #{param.inspect}") unless @required_query_params.has_key?(param)
 
-      @required_query_params[param] = ::CGI.escape(val)
+      @required_query_params[param] = val.nil? ? nil : ::CGI.escape(val.to_s)
+
+      required_query_params_set?(reset_cache: true)
+
+      @required_query_params[param]
+    end
+
+    # Returns true if all of this provider's required_query_params have a value
+    def required_query_params_set?(reset_cache: false)
+      return @all_required_query_params_set unless reset_cache || @all_required_query_params_set.nil?
+
+      @all_required_query_params_set = !@required_query_params.values.include?(nil)
     end
 
     # Send a request to the Provider endpoint to get information about the
@@ -121,7 +138,7 @@ def get(url, query = {})
     # against the Provider's URL schemes.
     # It will always return false of a provider has required_query_params that are not set.
     def include?(url)
-      return false if @required_query_params.values.include?(nil)
+      return false unless required_query_params_set?
       @urls.empty? || !!@urls.detect{ |u| u =~ url }
     end
 
diff --git a/lib/oembed/providers.rb b/lib/oembed/providers.rb
index 9388db2..24b71c2 100644
--- a/lib/oembed/providers.rb
+++ b/lib/oembed/providers.rb
@@ -81,7 +81,7 @@ def fallback
         @@fallback
       end
 
-      # Returns a Provider instance who's url scheme matches the given url.
+      # Returns a Provider instance whose url scheme matches the given url.
       def find(url)
         providers = @@urls[@@urls.keys.detect { |u| u =~ url }]
         Array(providers).first || nil
diff --git a/spec/provider_spec.rb b/spec/provider_spec.rb
index 3f5bead..7a1483d 100644
--- a/spec/provider_spec.rb
+++ b/spec/provider_spec.rb
@@ -474,4 +474,51 @@
       @flickr.get(example_url(:flickr), :timeout => 5)
     end
   end
+
+  describe "#set_required_query_params" do
+    let(:provider) { OEmbed::Provider.new("http://foo.com/oembed", required_query_params: { send_with_query: 'PROVIDER_ENV_VAR' }) }
+
+    around(:example) { |example|
+      orig_value = ENV['PROVIDER_ENV_VAR']
+      ENV['PROVIDER_ENV_VAR'] = 'a non-nil value'
+      example.run
+      ENV['PROVIDER_ENV_VAR'] = orig_value
+    }
+
+    it 'META: the around works as expected' do
+      expect(provider.send_with_query).to eq('a non-nil value')
+      expect(provider.required_query_params_set?).to be_truthy
+    end
+
+    [
+      [true, 'true'],
+      [false, 'false'],
+      ['one two', 'one+two'],
+      ['a@&?%25', 'a%40%26%3F%2525'],
+    ].each do |given_value, expected_value|
+      context "given #{given_value.inspect}" do
+        before(:each) { provider.send_with_query = given_value }
+
+        it "stringifies and escapes the value" do
+          expect(provider.send_with_query).to eq(expected_value)
+        end
+
+        it "satisfies required_query_params_set?" do
+          expect(provider.required_query_params_set?).to be_truthy
+        end
+      end
+    end
+
+    context "given nil" do
+      before(:each) { provider.send_with_query = nil }
+
+      it "nils the existing value" do
+        expect(provider.send_with_query).to be_nil
+      end
+
+      it "sets required_query_params_set? to falsey" do
+        expect(provider.required_query_params_set?).to be_falsey
+      end
+    end
+  end
 end