add event handler class

[xavdid-stripe]

Why?

We've been designing a streamlined approach to handling incoming events that is easy to get right and hard to get wrong. This PR has the initial implementation of this new system.

NOTE: final names for classes / methods are still being gaveled, so don't worry about those for now. Weigh in on this doc if you have strong feelings there!

The only other pending item is to add a method to allow handling a webhook without verifying the signature. This is good for testing and for Event Bridge, which doesn't use the signature-based verification. Otherwise, this is ready for review.

What?

• add EventHandler class
• add event handler constructor on StripeClient
• add tests
• add method for changing the stripe context on a client

Example usage

[Route("api/[controller]")]
[ApiController]
public class EventNotificationWebhookHandler : ControllerBase
{
    private readonly StripeClient client;
    private readonly StripeEventRouter eventRouter;

    public EventNotificationWebhookHandler()
    {
        client = new StripeClient(Environment.GetEnvironmentVariable("STRIPE_API_KEY"));
        eventRouter = client.Router(Environment.GetEnvironmentVariable("WEBHOOK_SECRET") ?? string.Empty, HandleUnhandledError);

        // register handlers
        eventRouter.V1BillingMeterErrorReportTriggeredEventNotification += HandleBillingMeterErrorReportTriggeredEventNotification;
    }

    private void HandleBillingMeterErrorReportTriggeredEventNotification(object sender, Stripe.StripeEventNotificationEventArgs<Stripe.Events.V1BillingMeterErrorReportTriggeredEventNotification> e)
    {
        var meter = e.EventNotification.FetchRelatedObject();
        Console.WriteLine($"Meter {meter.DisplayName} had an error");
    }

    private void HandleUnhandledError(object sender, Stripe.StripeUnhandledEventNotificationEventArgs e)
    {
        Console.WriteLine($"Received unhandled event notification type: {e.EventNotification.Type}");
    }

    [HttpPost]
    public async Task<IActionResult> Index()
    {
        var json = await new StreamReader(HttpContext.Request.Body).ReadToEndAsync();
        eventRouter.Handle(json, Request.Headers["Stripe-Signature"]);
        return null;
    }
}

See Also

• DEVSDK-2822
• ⚠️ Add strongly typed EventNotifications #3168

[xavdid-stripe]

Since unhandledEventHandler i'm not convinced it actually needs to be an event handler? but i'm not sure if that's what users expect anyway

[jar-stripe]

I dont think I've seen any classes take an event handler as a constructor arg, but also I don't think I've seen too many patterns where an event is expected to have a handler when it is fired or else its an error. I think the shape of this is fine, but if you wanted to make it a little more "this is just a function"-like, you could do:

public StripeEventRouter(StripeClient client, string webhookSecret, Action<object, StripeUnhandledEventNotificationEventArgs> unhandledEventHandler)
{
    this.client = client ?? throw new ArgumentNullException(nameof(client));
    this.webhookSecret = webhookSecret ?? throw new ArgumentNullException(nameof(webhookSecret));
    this.UnhandledEventHandler += new EventHandler<StripeUnhandledEventNotificationEventArgs>(unhandledEventHandler);
}

This has the added benefit of making it easy to pass anonymous functions in; but they also could do the new EventHandler... adapter call if you wanted to keep the argument the way it is.

Absent user feedback, I don't think one way is necessarily better than the other.

[xavdid-stripe]

Env isn't shared between recipes, so this doesn't work if you don't have the root set

[jar-stripe]

we may want to fail if DOTNET_ROOT isnt set here; iirc a lot of stuff doesnt work right if that env var isn't already set.

[jar-stripe]

Not sure if the comment should go here or in your doc, but HandleUnhandledError seems like the wrong name in your example. That's just HandleUnhandledEventNotification, right?

[jar-stripe]

will this ever get constructed outside of our SDK code? If not, I would consider changing this constructor to be internal

[xavdid-stripe]

Wouldn't users need it for tests? Like, if they write their function and want to test it in isolation, that function takes these args and they'd nee to be able to construct it?

[jar-stripe]

Took a pass thru, it looks largely good (the names are pretty long and I left a comment in your naming doc), but I have concerns about StripeClient/ApiRequestor reuse when you swap in and out the StripeContext

[jar-stripe]

its a small nit, but can you pull the null check and throw into a separate line/conditional? I find that easier to read

[jar-stripe]

I dont think I've seen any classes take an event handler as a constructor arg, but also I don't think I've seen too many patterns where an event is expected to have a handler when it is fired or else its an error. I think the shape of this is fine, but if you wanted to make it a little more "this is just a function"-like, you could do:

public StripeEventRouter(StripeClient client, string webhookSecret, Action<object, StripeUnhandledEventNotificationEventArgs> unhandledEventHandler)
{
    this.client = client ?? throw new ArgumentNullException(nameof(client));
    this.webhookSecret = webhookSecret ?? throw new ArgumentNullException(nameof(webhookSecret));
    this.UnhandledEventHandler += new EventHandler<StripeUnhandledEventNotificationEventArgs>(unhandledEventHandler);
}

This has the added benefit of making it easy to pass anonymous functions in; but they also could do the new EventHandler... adapter call if you wanted to keep the argument the way it is.

Absent user feedback, I don't think one way is necessarily better than the other.

[jar-stripe]

we may want to fail if DOTNET_ROOT isnt set here; iirc a lot of stuff doesnt work right if that env var isn't already set.

[jar-stripe]

I don't think you can safely swap out and back the StripeContext in an environment with concurrent API calls, if this.client represents the StripeClient that created the router. The EventRouter should hold its own StripeClient and requestor, if it is expected to make API calls with different context than e.g. a user's "normal" StripeClient instance.

[jar-stripe]

We should probably check this in the other SDKs too; we're not always consistent about which request objects are reused vs recreated in different scenarios.

[jar-stripe]

this message needs some love 😆

[jar-stripe]

but actually, in all seriousness could this just fall thru to the UnhandledEventHandler? at least then the user might see it and have context in which to raise an issue back to us and/or recover on their side if we have to fix something.

[jar-stripe]

Also, we should consider adding the example from the PR description as an actual example in the project, so folks can see how to use this from code they can easily find.