From 1417cb5bd18c801cd3f1d8505aa96946bc95bcb8 Mon Sep 17 00:00:00 2001
From: Brandur <brandur@mutelight.org>
Date: Thu, 27 Jul 2017 13:35:12 -0700
Subject: [PATCH] Allow empty strings in API invocation parameters

Currently, with a normal API resource, you can unset fields by
specifying a `nil` to that field's setter:

``` ruby
c = Charge.retrieve('ch_123')
c.customer = nil
c.save
```

This actually gets serialized as the form `customer=` (i.e. an empty
string), but we had to use the empty string to handle unsets because
form encoding has no concept of a `nil`/`null`.

To try and prevent usage errors, we actually prevent you from setting
fields with an empty string:

``` ruby
c = Charge.retrieve('ch_123')
c.customer = '' # error! use nil instead
```

When specifying parameters though, this doesn't work anywhere nearly as
well because usage patterns like this are very common in Ruby:

``` ruby
charge_opts = {
  params[:amount],
  params[:currency],
  params[:customer],
}
charge = Charge.create(charge_opts)
```

Each one of `params` above may or may not be `nil`, so we've
traditionally filtered those fields out during the invocation of
`Charge.create`.

Recently, I suggested to Slava that we may be able to change this
behavior, and we ended up putting in a patch as part of #557. Users
brought to my attention that this would be far too disruptive of a
change in #560 though, and having thought about it more carefully, I
agree. There's also an argument that filtered `nil` values are just a
better API, especially in Ruby where patterns like the one above are
frequently in effect.

So the best thing I can think of currently is to leave things as they
were before #557, and just require that users use an explicit empty
string when passes in parameter hashes:

``` ruby
Charge.update(customer: '') # will try to unset customer
```

Empty strings will continue to error for `StripeObject` fields like they
always have.

I don't think this is a perfect solution by any means (the different
between values on `StripeObject` versus using parameters is weird), but
it's the least disruptive thing that I can think of right now that gets
us the functionality that we need for endpoints like
`/v1/invoices/upcoming`.

Fixes #560.
---
 lib/stripe/stripe_client.rb       |  5 -----
 test/stripe/invoice_test.rb       | 13 +++++++++++++
 test/stripe/stripe_client_test.rb | 17 ++++++++++++-----
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/lib/stripe/stripe_client.rb b/lib/stripe/stripe_client.rb
index 213c64c8..4487c197 100644
--- a/lib/stripe/stripe_client.rb
+++ b/lib/stripe/stripe_client.rb
@@ -117,11 +117,6 @@ module Stripe
 
       check_api_key!(api_key)
 
-      # there's no concept of a nil in form encoding, so convert any explicitly
-      # provided nils to an empty string which the API will handle
-      # appropriately
-      params = params.merge(params) {|_, v| v.nil? ? '' : v}
-
       params = Util.objects_to_ids(params)
       url = api_url(url, api_base)
 
diff --git a/test/stripe/invoice_test.rb b/test/stripe/invoice_test.rb
index 4c176f55..d6f3f49f 100644
--- a/test/stripe/invoice_test.rb
+++ b/test/stripe/invoice_test.rb
@@ -74,6 +74,19 @@ module Stripe
           }
         assert invoice.kind_of?(Stripe::Invoice)
       end
+
+      should "be callable with an empty string" do
+        invoice = Stripe::Invoice.upcoming(
+          coupon: '',
+          customer: API_FIXTURES[:customer][:id]
+        )
+        assert_requested :get, "#{Stripe.api_base}/v1/invoices/upcoming",
+          query: {
+            coupon: '',
+            customer: API_FIXTURES[:customer][:id]
+          }
+        assert invoice.kind_of?(Stripe::Invoice)
+      end
     end
   end
 end
diff --git a/test/stripe/stripe_client_test.rb b/test/stripe/stripe_client_test.rb
index 46eff5b0..018b7456 100644
--- a/test/stripe/stripe_client_test.rb
+++ b/test/stripe/stripe_client_test.rb
@@ -493,17 +493,24 @@ module Stripe
       end
 
       context "params serialization" do
-        should 'convert nil params to empty string' do
+        should 'allows empty strings in params' do
           client = StripeClient.new
           client.execute_request(:get, '/v1/invoices/upcoming',
-                                 params: {customer: API_FIXTURES[:customer][:id],
-                                          subscription: API_FIXTURES[:subscription][:id],
-                                          coupon: nil})
+                                 params: { customer: API_FIXTURES[:customer][:id],
+                                           coupon: '' })
           assert_requested(:get, "#{Stripe.api_base}/v1/invoices/upcoming?",
                            query: { customer: API_FIXTURES[:customer][:id],
-                                    subscription: API_FIXTURES[:subscription][:id],
                                     coupon: '' })
         end
+
+        should 'filter nils in params' do
+          client = StripeClient.new
+          client.execute_request(:get, '/v1/invoices/upcoming',
+                                 params: { customer: API_FIXTURES[:customer][:id],
+                                           coupon: nil })
+          assert_requested(:get, "#{Stripe.api_base}/v1/invoices/upcoming?",
+                           query: { customer: API_FIXTURES[:customer][:id] })
+        end
       end
     end