integrallis / stripe_event

Stripe webhook integration for Rails applications.
https://rubygems.org/gems/stripe_event
MIT License
840 stars 107 forks source link

Documentation: Warning against memoization #122

Open m5rk opened 5 years ago

m5rk commented 5 years ago

There's only one subscriber instantiated from this configure block:

StripeEvent.configure do |events|
  events.subscribe 'invoice.', StripeInvoiceEvent.new
end

And that means that you can't have methods in StripeInvoiceEvent that memoize:

def account
  @account || = Account.find_by(stripe_customer_id: event.data.object.id)
end
rmm5t commented 5 years ago

A documentation PR is welcome, though it should probably be more specific about the fact that event subscriptions are singletons that just respond to call.

With that said, and/or alternatively, and/or optimally, I suppose there's also room to adjust the StripeEvent::NotificationAdapter#call to instantiate a new instance if the passed subscriber is a Class as opposed to an instance of something that responds to #call itself. I'd be willing to review a PR (with specs) that does that.

yannis commented 4 years ago

First, I'd like to thank you for the great gem which helped us a lot while integrating Stripe in our app.

Still, we were victim of the memoization issue, and I'd like to insist on the fact that the use of a Singleton, instantiated in an initializer can lead to a lot of issues. Indeed, any setting of an instance variable in the recorder is a mistake since it can change anytime, even in the middle of a method call. For example, we had something like this (code simplified for clarity):

class StripeInvoiceRecorder
  attr_reader :action, event

  def initialize(action:)
    @action = action
  end

  def call(event)
    @event = event
     send(action)
  end

  private def payment_succeeded
    object = event["data"]["object"]
    SubscriptionJob1.perform_later(subscription_stripe_id: object["subscription"])
    SubscriptionJob2.perform_later(stripe_invoice_id: object["id"])
  end
end

Here, instantiating @event = event in #call is a mistake as any other call to #call during the execution of #payment_succeeded will change the value of @event changing object mid-flight.

So to prevent this issue, we changed this code to:

class StripeInvoiceRecorder
  attr_reader :action

  def initialize(action:)
    @action = action
  end

  def call(event)
     send(action, event)
  end

  private def payment_succeeded(event)
    object = event["data"]["object"]
    SubscriptionJob1.perform_later(subscription_stripe_id: object["subscription"])
    SubscriptionJob2.perform_later(stripe_invoice_id: object["id"])
  end
end

I hope it will help other having strange issues with their recorders. And indeed, switching from a Singleton to instance variables would prevent this kind of issues.

vakrolme commented 4 years ago

+1, and I would expand this to code reloading also, since I've just spent some hours figuring out why code reloading doesn't work with a service object that I pass into the above mentioned initializer (thought it to be a Zeitwerk issue or something).

In hindsight it all makes sense. But it isn't obvious if you just mindlessly follow the setup guide.

rromanchuk commented 4 years ago

BTW Using StripeEvent.configure do |events| in a config/initializers/stripe.rb (as shown in the README) and assigning classes that live in /app is going to cause problems in rails6+

kirylrb commented 3 years ago

@rromanchuk What kind of problems you mean?

christianrolle commented 3 years ago

@kirylpl related to the subscribed class, like:

StripeEvent.configure do |events|
  events.subscribe 'charge.failed', Stripe::ChargeFailedInteractor
end

and

module Stripe
  class FailedChargeInteractor
    include Interactor

    def call(event)
      Rails.logger.error("Stripe charge failed: #{params}")
    end
  end
end

and then running the server with rails s:

uninitialized constant Stripe::ChargeFailedInteractor (NameError)