Stripe::Customer.delete_discount corrupts Stripe::Customer object

[matthewbjones]

Introduced in version 5.0.0, worked in prior releases

Bug conditions

1. Have an existing Stripe customer that has an existing discount applied
2. Delete the existing discount via delete_discount method
3. Upon save, a Stripe::InvalidRequestError ((Status 404) (Request req_0Hk4gAhb1KNnl3) No such customer: 'di_xxx') will occur

Expected
To succeed, because deleting of a discount shouldn't prevent future operations on the Stripe::Customer object. The discount does actually get deleted, it's just future calls on the Stripe::Customer object, like save, will error with an incorrect customer ID.

Gem version
Broke in 5.0.0 (August 20, 2019), previous release 4.24.0 (August 13, 2019) works and earlier versions

# Setup below
require 'stripe'
Stripe.api_key = ENV['STRIPE_SECRET_KEY']
Stripe.api_version = '2020-08-27'

# Create the test customer
test_customer = Stripe::Customer.create(email: "[email protected]")
customer_stripe_id = test_customer.id

# Create a discount to have existing on customer
existing_coupon_id = "FEEDBACK_#{Time.now.to_i}"
Stripe::Coupon.create({
  id: existing_coupon_id,
  percent_off: 20,
  duration: 'forever'
})
test_customer.coupon = existing_coupon_id
test_customer.save

# Actual code
# Reproduce error
stripe_user = Stripe::Customer.retrieve(customer_stripe_id)
stripe_user.delete_discount
# The line below will raise a Stripe::InvalidRequestError
# with an error message of something like ((Status 404) (Request req_0Hk4gAhb1KNnl3) No such customer: 'di_xxx')
# Note: The customer ID prefix is now 'di_' and not the original 'cust_'
stripe_user.save

[matthewbjones]

I believe the issue stems from: https://github.com/stripe/stripe-ruby/blob/master/lib/stripe/resources/customer.rb#L33

Here's a monkey patch that fixes the broken code above:

module Stripe
  class Customer < APIResource
    def delete_discount
      resp, opts = execute_resource_request(:delete, resource_url + "/discount")
      Stripe::Discount.construct_from(resp.data, opts)
    end
  end
end

Since resp.data is a JSON payload about a Discount, we do not want to call initialize_from within Stripe::Customer, but rather return a Discount from the delete_discount method.

I don't know if the Stripe::Customer is stale and needs to refresh or not.

[remi-stripe]

@matthewbjones Thank you so much for the detailed reproduction steps and finding the line of code that is causing issues. I think the bug was introduced by this change here. We used to remove the discount on the Customer explicitly and then would return the Customer but incorrectly switched to overriding the Customer with a Discount instead.

I could confirm it quickly by modifying the test for it to this

    context "#delete_discount" do
      should "delete a discount" do
        customer = Stripe::Customer.retrieve("cus_123")
        customer = customer.delete_discount
        assert_requested :delete, "#{Stripe.api_base}/v1/customers/cus_123/discount"
        assert customer.is_a?(Stripe::Customer)
        assert customer.object == "customer"
      end
    end

it fails because object is now discount event though the class' instance is still a Customer.

In a way it's a simple fix, but the bug has been here for long enough that it's always problematic to make subtle changes like this in a patch release and it might need a major release for it.

I'll discuss this with the other maintainers next week to decide what is the best plan of action. We might also need to add that test logic to our test suite to ensure that you can't get a an instance of class A but object matching class B.

In the meantime, I would recommend moving away from .save entirely. While the library supports this (and we definitely shouldn't have broken this!) we consider it mostly deprecated and all our docs have been updated to move to the update method approach instead a few years ago. So instead of saving the customer that way, we recommend always being explicit with what you want to change or mutate and use Stripe::Customer.update('cus_123', options) approach instead.

[matthewbjones]

@remi-stripe Thanks for the speedy and detailed reply. For context, we were previously using version 2.12.0 prior to this upgrade with an API version of 2013-02-13. We've been on this stack for years and years throughout all of Stripe's updates and improvements and the Ruby gem + API held strong without issue. We only just got around to updating the gem to the latest version because we're finally looking to implement some of the "newer" Stripe features (Apple Pay, etc.) and we had to finally update the SDKs.

We wanted to make as little changes to our codebase as possible, given the 7+ years of stability and were a bit fearful of introducing a new bug considering just how long it's been and how much of the API and Ruby code changed over the years. The migration was more straightforward than feared so that's good, mostly replacing .all with .list on the various objects and then dealing with delete_subscription no longer being a method on Stripe::Customer, which was a rather easy fix given that our business only allows one subscription at a time (which I believe was the functionality way back in 2013). The only break in functionality we've seen in production so far is this delete_discount glitch.

Thanks for the information on what the Stripe team recommends in terms of how to best use the Ruby gem these days. We have a whole test suite around billing + Stripe with VCR cassettes, etc. that instills a great level of confidence that our Stripe integration works as intended the way it was written years ago. This is something we will slowly migrate to over time to make sure we don't have any breaks in functionality.

Kudos to the Stripe team for allowing us to use 2013 API versions with a 2017 gem for all this time!

[brandur-stripe]

Thanks for reporting and bearing with us! We've released a fix in 5.29.1.