[WIP/RFC] Begin custom unmarshalling for events, do not use maps

[EVODelavega]

I'm in the process of implementing webhooks, and in doing so noticed that all events are essentially unmarshalled into a map[string]interface{} type. This makes code messy, error-prone, and due to the insane amount of type assertions might tempt people to simply check the event type, and proceed to do something like:

var customer stripe.Customer
json.Unmarshal([]byte(event.Data.Raw), &customer)

Essentially unmarshalling the same payload twice (I know I briefly considered this). Rather than adding all the listed events right now, I thought I'd open this PR as an RFC, and see if this would be a welcome change. If it isn't, then I'd rather not waste too much time going through the entire list before being told it's not worth the effort.

[brandur-stripe]

Yeah, this would definitely be a nice shortcut most of the time — the one consideration here though is that it's possible to receive both webhooks and events that are of a different version than the one that stripe-go is pegged to, and when that happens, unmarshaling may fail. If we brought something like this in, we'd have to be really careful to specify that it's a "best effort" approach and that unmarshaling is not guaranteed to succeed.

That makes this addition a bit of a trade off unfortunately. It will mostly provide better usability, but will also be more fragile.

If you didn't see it already, you should also take a look at the provided GetObjectValue and GetPreviousValue helpers, which are designed to make inspecting values inside an object a little easier.

With regards to the patch: in the custom UnmarshalJSON, you should reach into the object instead of looking at the event's type in order to figure out what to unmarshal. i.e., Instead of looking at account.updated, you should just look at account. This approach will be much more sustainable in the long run.

@remi-stripe @ob-stripe Do either of you have any feelings around whether we should pursue this or not?

[EVODelavega]

@brahn-stripe WRT the last bit (just looking at account): I did initially plan to do just that, but there are a number of events where that doesn't seem to be the case. I've identified events where an event type starts with account, but the payload is of type Application, and likewise events starting with Account where the payload can be either a Card or a BankAccount, hence my choosing to be explicit.

Another reason why I chose to be explicit is so I'd have a default case to fall back on, which would behave in the same way as the current event unmarshal stuff. Perhaps that would also address the concerns WRT versioning etc...?

[brandur-stripe]

WRT the last bit (just looking at account): I did initially plan to do just that, but there are a number of events where that doesn't seem to be the case. I've identified events where an event type starts with account, but the payload is of type Application, and likewise events starting with Account where the payload can be either a Card or a BankAccount, hence my choosing to be explicit.

But the object field is always going to correspond to what the object it contains actually is though, and we always want to deserialize to the right thing, right? Trusting the payload to flag itself as a card or a bank account (for example) seems to be the right move.

Another reason why I chose to be explicit is so I'd have a default case to fall back on, which would behave in the same way as the current event unmarshal stuff. Perhaps that would also address the concerns WRT versioning etc...?

Yeah, having the map as a backup would help with versioning et all. I'd move that out of the default case though — we should always deserialize to the map in addition to any typed object for backwards compatibility.

[EVODelavega]

@brandur-stripe If we should always unmarshal into a map and additionally try to unmarshal into a typed object, then there's very little point to me pursuing this further, really... the Data field contains the map and raw JSON, and I can decide to unmarshal the raw message manually should I need to already. Performance-wise, that seems to be the best way to go, in that case. If I add the automatically typed stuff here, then there's always going to be 2 unmarshal calls regardless, even if I'm not interested in the event payload itself, or I simply need to access an ID or something. I'm leaving this PR open in case this is something worth adding via a second type (giving people the choice to unmarshal the payload into a typed event vs untyped), but I'll probably handle this by manually unmarshalling the data a second time.

[oralordos]

Maybe some helper functions to get the type and make the unmarshalling a bit easier and an example for the godoc documentation on how to use them? That way each user will not have to come up with their own way of doing it.

[brandur-stripe]

Performance-wise, that seems to be the best way to go, in that case. If I add the automatically typed stuff here, then there's always going to be 2 unmarshal calls regardless, even if I'm not interested in the event payload itself, or I simply need to access an ID or something.

Personally, I wouldn't worry about the performance aspects of the approach too much ... recall that unmarshaling JSON is really, really fast compared to the network call you just made. So much so that it's practically negligible by comparison.

Maybe some helper functions to get the type and make the unmarshalling a bit easier and an example for the godoc documentation on how to use them? That way each user will not have to come up with their own way of doing it.

I'm in fully support of this idea — basically leave the default as an untyped map, but have a utility function that will try to transform it into a typed structure, and documented carefully in such a way that it indicates there's some possibility of failure in case of divergent version representations, etc.

[EVODelavega]

But the object field is always going to correspond to what the object it contains actually is though, and we always want to deserialize to the right thing, right? Trusting the payload to flag itself as a card or a bank account (for example) seems to be the right move.

With regards to the patch: in the custom UnmarshalJSON, you should reach into the object instead of looking at the event's type in order to figure out what to unmarshal. i.e., Instead of looking at account.updated, you should just look at account. This approach will be much more sustainable in the long run.

I'm looking into handling certain events again (after finishing off some other stuff that came up). There seems to be 2 slight misunderstandings at play here:

1. Letting the payload flag itself isn't quite possible. I'd either have to check for an id field, and check the id prefix and map that onto the type in question (e.g. data["id"][:4] == "cus_" for customers etc...). Some types allow you to manually specify an ID (for example: creating a product). That would break the unmarshalling if the package doesn't use the event type.

2. Looking at the event type is another tricky one. I'd love to be able to write something like this, and was originally planning to:

parts := strings.Split(evet.Type, ".")
eventType := parts[0]

Sadly, there are events like account.external_account.created and account.application.deauthorized. Not a big problem, I thought, I'll just change the code to:

eventType := parts[len(parts)-2] // next to last substring

This would indeed work, but the problem with the external_account event types in particular is that the payload can be either a card, or a bank account (as per API documentation). Having something like a switch on something like parts[len(parts)-2] also looked a bit hacky and feels quite fragile to me. The event unmarshalling, though, looks to me like a prime candidate for a template for code generation, where the massive switch-case is just generated from the API spec files, and each case can just unmarshal to the corresponding type. if there are several possible types, either try them sequentially, or generate a new type (embedding all possible types), and have a function that updates the main Event type, like so:

type cardOrBankAccount struct {
    *Card
    *BankAccount
    // whatever other types are possible
}

switch event.Type {
case "some.external_account.action", "another.external_account.action":
    var typed cardOrBankAccount
    if err := json.Unmarshal([]byte(event.Raw), &typed); err != nil {
        return err
    }
    event.setExternalAccount(typed)
}

func (e *Event) setExternalAccount(data cardOrBankAccount) {
    if data.Card != nil {
        e.Card = data.Card
        return
    }
    e.BankAccount = data.BankAccount
}

This PR was clearly not in a state where my intentions were clear to everyone. Hopefully this cleared up any confusion, in which case, I'd like to know if this is still something worth pursuing.

[brandur-stripe]

Hey @EVODelavega, I'd still say that decoding object type based on object field alone is very definitively the way to go, and if you're running into trouble with that, we should try to examine what that is and dig into it.

If you reference our Stripe implementation for Ruby or for PHP, you'll see that they already have a function that does this exact thing of converting an object identifier to a certain resource type.

[EVODelavega]

@brahn-stripe I've not run into problems so far. In fact, yesterday I've used the API doc page to extract some JSON to map various events onto specific types, and wrote a little generator to generate a typed event wrapper. It basically unmarshals everything into the existing stripe.Event type, whereas my event hook handler is unmarshalling the body in this wrapped type. Due to the custom unmarshaller, I can unmarshal the raw body into the specific type. That's how I'm handing things now, and I'm not experiencing any issues. I can change this generator somewhat, and use the API specs as a source I think (although I've not checked yet whether or not the events are in there).

Here's the gist of the generator I hacked together: https://gist.github.com/EVODelavega/288a109d8ed9e908c99175b75cfb70a8

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

Elias Van Ootegem seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[pakrym-stripe]

Hi, I'm sorry it's been so long since you've opened the pull request without us getting back.

We are looking at improving event handling experience across languages but don't have a full design for it yet.

I'm going to close this PR because of it's age. My apologies once again.