Redesign webhook event parsing to avoid event/data conflicts

[weppos]

Closes #83

This is an alternate implementation of #84. It's a significant rewrite of the webhook parsing system. The key difference is how a webhook event is represented.

Current Implementation

In the current implementation, each event category (account event, domain event, etc) was represented by a specific type.

type AccountEvent struct {
	EventHeader		AbstractEvent
	Data    *AccountEvent     `json:"data"`
	Account *dnsimple.Account `json:"account"`
}

The type is composed by a fixed set of fields, called header, and a variable set of fields, defined directly in the type.

This design was largely inspired by the GO DNS library, where each RR type has exactly the same structure: a header and data.

All event types implemented a particular interface called Event. The interface was also used in method definitions to represent any event type. An example, the method Parse is defined as a method that takes a payload and returns a type that implements Event.

Type casting (and type assertion) was required to convert interface types to the concrete implementations.

You can see an example in strillone:

https://github.com/dnsimple/strillone/blob/7471dc6e58bffe6ae597219d63345df4387edc03/message.go#L11-L39

This solution worked, but it occasionally showed issues. For instance, parsing the data node was not trivial and required some hack. That's because the payload structure is not flatten, but the data is nested within the event:

{
  "data": {
    "account": {
      "id": 0,
      "email": "ops@example.com",
      "created_at": "2012-03-16T16:02:54Z",
      "updated_at": "2019-06-12T13:33:07Z",
      "plan_identifier": "monthly"
    },
    "account_invitation": {
      "id": 0,
      "email": "example@example.com",
      "token": "256bffc4",
      "account_id": 0,
      "created_at": "2019-06-13T07:45:29Z",
      "updated_at": "2019-06-13T07:45:29Z",
      "invitation_sent_at": "2019-06-13T07:45:29Z",
      "invitation_accepted_at": null
    }
  },
  "name": "account.user_invite",
  "actor": {
    "id": "0",
    "entity": "user",
    "pretty": "user@example.com"
  },
  "account": {
    "id": 0,
    "display": "Example",
    "identifier": "example"
  },
  "api_version": "v2",
  "request_identifier": "0e55bda3-8fdf-4ca2-8bcc-fce18417d40a"
}

The workaround was to assign the event type struct instance to its own Data field, effectively pointing the value of the Data field of a particular event struct, to the struct itself.

This is where #83 was discovered. In the case of an account event, there is both an Account node in the event payload (the same node is present in any event being delivered), and an account within the data payload. In theory, those two entities are the same, but they have slightly different structure as the account in the data is a full representation of the account, whereas the account in the event is just a reference.

I found a fix in #84, but it was a significant hack and I felt I wanted to try some alternative.

Proposed implementation

The proposed implementation is something I though when I originally created this lib. But I discarded this design years ago because I felt the need to type-case the data was overwhelming, and I was likely tricked by trying to inject an object-oriented inheritance approach into Go.

The truth is that, no matter what, in Go we'll have to type-case if we use event. And composition turned to not work in the way I originally though when it comes to referencing the sub-struct (the header struct in the case of the current implementation).

So this new design is the exact opposite of the current one. Rather than having one struct type per event category, we have a unique type Event (currently called EventContainer) and the Data of the event is the part that is strictly typed. In fact, the data is the one that vary depending on the event category.

This design allows to keep the data as well the payload internal to the event. It also doesn't require workaround in the parse, as this structure is not flat but reflects the event payload where Data is a node inside the event.

Comparison

The following represent todays implementation of strillone:

func Message(s MessagingService, e webhook.Event) (text string) {
	header := e.GetEventHeader()
	account := header.Account
	prefix := fmt.Sprintf("[%v] %v", s.FormatLink(account.Display, fmtURL("/a/%d/account", account.ID)), header.Actor.Pretty)

	switch event := e.(type) {
	case *webhook.CertificateEvent:
		certificate := event.Certificate
		certificateDisplay := certificate.CommonName
		certificateLink := s.FormatLink(certificateDisplay, fmtURL("/a/%d/domains/%d/certificates/%d", account.ID, certificate.DomainID, certificate.ID))
		switch event.Name {
		case "certificate.remove_private_key":
			text = fmt.Sprintf("%s deleted the private key for the certificate %s", prefix, certificateLink)
		default:
			text = fmt.Sprintf("%s performed %s", prefix, event.GetEventName())
		}

With the new code it will become:

func Message(s MessagingService, e webhook.Event) (text string) {
	account := e.Account
	prefix := fmt.Sprintf("[%v] %v", s.FormatLink(account.Display, fmtURL("/a/%d/account", account.ID)), header.Actor.Pretty)

	switch data := e.GetData().(type) {
	case *webhook.CertificateEventData:
		certificate := data.Certificate
		certificateDisplay := certificate.CommonName
		certificateLink := s.FormatLink(certificateDisplay, fmtURL("/a/%d/domains/%d/certificates/%d", account.ID, certificate.DomainID, certificate.ID))
		switch e.Name {
		case "certificate.remove_private_key":
			text = fmt.Sprintf("%s deleted the private key for the certificate %s", prefix, certificateLink)
		default:
			text = fmt.Sprintf("%s performed %s", prefix, e.Name)
		}

The use is quite similar, but both the external and the internal implementation are actually simpler.

Here's another side-by-side comparison:

package main

import (
	"fmt"

	"github.com/dnsimple/dnsimple-go/dnsimple/webhook"
)

func main() {
	payload := `{"data": {"account": {"id": 123, "email": "hello@example.com", "created_at": "2011-03-17T21:30:25Z", "updated_at": "2018-11-05T12:27:17Z", "plan_identifier": "dnsimple-personal-yearly"}}, "name": "account.update", "actor": {"id": "1120", "entity": "user", "pretty": "hello@example.com"}, "account": {"id": 123, "display": "Personal2", "identifier": "foobar"}, "api_version": "v2", "request_identifier": "692a46c5-1117-4995-8107-9dd1ad18fa20"}`

	// current        
	event1, _ := webhook.Parse([]byte(payload))
	fmt.Println(event1.GetEventName())
	event1t, _ := event1.(*webhook.AccountEvent)
	fmt.Println(event1t.Account)

	// proposed
	event2, _ := webhook.ParseEvent([]byte(payload))
	fmt.Println(event2.Name)
	event2t, _ := event2.GetData().(*webhook.AccountEventData)
	fmt.Println(event2t.Account)
}

This proposed alternative also makes it possible to delay the parsing of the event data, if needed. This allows to build an more efficient system especially in the case where you are listening to all events, but you are only interested in few of them.

You can use ParseEvent to get an Event with only the event headers (such as the Name), and call GetData() (to parse the data) only for the events categories the code needs to handle.

[aeden]

There is a typo that needs to be fixed.

[aeden]

Suggested change

	func unmashalEventData(data []byte, v interface{}) error {
	func unmarshalEventData(data []byte, v interface{}) error {

[aeden]

I have no objections to the change in general.