Initial Support for v3 Webhook Payloads

[ChezCrawford]

Adds some initial support for processing V3 Webhook Payloads.

Includes the types for the OutboundEvent and the WebhookPayload wrapper. Also includes a generic method of accessing specific fields inside of event.data. This allows consumers to access all fields in event.data for any event type.

The groundwork is also set for code that handles polymorphic unmarshalling of event.data in future updates.

[ChezCrawford]

One case I am still considering how we should handle is when this method is called using a set of keys that does not terminate in a string value. For example if the object was:

{
      "title": "A little bump in the road",
      "service": {
        "html_url": "https://acme.pagerduty.com/services/PF9KMXH",
        "id": "PF9KMXH",
        "self": "https://api.pagerduty.com/services/PF9KMXH",
        "summary": "API Service",
        "type": "service_reference"
      }
}

and this function was called with GetEventDataValue("service") you would get the stringified version of the service object which isn't super useful.

My gut feeling is to return an error for this case as well.

[theckman]

Per the io.Reader interface contract, you should try processing the data in b before you handle the io.ReadAll error. I think this block of code should be something like:

	orb := r.Body
	defer func() { _ = orb.Close() }()

	b, err := ioutil.ReadAll(io.LimitReader(r.Body, webhookBodyReaderLimit))
	r.Body = ioutil.NopCloser(bytes.NewReader(b))

	if len(b) == 0 {
		if err != nil {
			return nil, fmt.Errorf("failed to read response body: %w", err)
		}

		return nil, ErrMalformedBody
	}

This version does a bit more to ensure the *http.Request.Body will contain the request body we received, even if we only got a partial read of the data. That way consumers can read that partial body themselves and inspect it.

The one case this does not handle is when the io.LimitReader is violated. In that case they will get the .Body that we read so far, and the remaining bytes will be lost meaning the caller couldn't choose to fully read it even if they wanted to. That could be accomplished by using io.MultiReader(), but would require some reworking of my snippet above.

[ChezCrawford]

Per the io.Reader interface contract, you should try processing the data in b before you handle the io.ReadAll error.
@theckman : Where are you seeing that? I don't see it mentioned in io.ReadAll or in the example provided there...

(IIRC, we got this code from the comments you provided in the previous PR.)

[theckman]

Those snippets were things I probably hammered out on my mobile phone while responding back to the comment(s). In hindsight, I should have done a better job at communicating that I hacked them together pretty rapidly as an example. I'm sorry about that.

[ChezCrawford]

No worries but I would still like to get clarity on what is the "perfect" reusable HTTP body reader.

This is one of the reasons I was pushing for more focused functions that verify signatures and construct events in #326. I would like to focus my effort on providing solid support for V3 Webhooks rather than the details of how folks implement that functionality.

[theckman]

I gotta admit I'm not digging this approach on my first read through. I think I understand why this was the path you took, but it's immediately got a few things that concern me about the UX of an API like this.

The first minor nit is that I believe that this should be a function that accepts an OutboundEvent (no pointer), as it's really acting on the oe as an input and not mutating its internal state.

The second larger thing is that I think, overall, this might "violate" one of the Go Proverbs, as it seems to be trying to be pretty clever:

Clear is better than clever.

It took me far too long to figure out how it's supposed to be used, and what the expected outcome of the call is when I pass in two values. It seems like a pretty confusing API, that I admit could be cleared up with a comment above the method.

The other is that it's a bit of a leaky abstraction, as consumers of this package still need to be intimately familiar with the JSON document structure of the webhook event payload. One of my goals in the suggested approach in #367 (comment) was to avoid consumers needing to be super familiar with the actual JSON, by offering a way to determine which type of payload it is and then have struct types that clearly enumerate the possible values within that payload.

This would ultimately make this project more self-documenting, without the need for a consumer to also browse the PagerDuty docs.

I could maybe be convinced of this approach, and would love to hear your thoughts on the tradeoffs you see versus the other.

[ChezCrawford]

The first minor nit is that I believe that this should be a function that accepts an OutboundEvent (no pointer), as it's really acting on the oe as an input and not mutating its internal state.

This was an oversight. It has been corrected.

[ChezCrawford]

Ah so I guess I should have provided a bit more color than I did in the PR description. A few items of background:

• V3 Webhooks are still in the early stages of life at PagerDuty and we are routinely adding new event types / event data types.
• The event data types delivered in Outbound Events are not exactly equal to their corresponding REST API representation. We are trying our best for them to be a subset of these resources.
• Historically, PagerDuty has not always provided concrete static types in all of our client libraries.

This method is not meant to be mutually exclusive with a set of methods that describe concrete types as described in #367 (comment). [I have actually started a bit of work on that as well.] Instead, this method is meant to allow consumers to work with V3 Webhooks in a generic way, particularly in situations where the full structured types may not have been implemented quite yet.

While there are examples of how to use this function in examples/webhooks/webhook_server.go as well as in the tests, this whole file was missing some godoc and that has been added. Have a look and let me know what you think...

[theckman]

Okay, that makes it a bit more clear to me. Although I do have one question:

Historically, PagerDuty has not always provided concrete static types in all of our client libraries.

How many of your client libraries are for statically typed languages? If they've been more focused towards the Perl, Ruby, Python, and JS communities, it doesn't surprise me that there haven't been static types defined.

With the cost of doing a major version bump in Go, especially because we're already at v1, we should be extra thoughtful before increasing the API surface area because we're probably going to be stuck with it for quite awhile. I say this because I don't know that I'm aligned with expanding the API surface area in this way, unrelated to maintenance cost of adding this to the public API.

Instead of expanding our API in this way, in cases where we've yet to define a struct type, I'd instead advocate for consumers to define their own struct (bonus points if they contribute it back to us). The reason I'm leaning towards this path, is that whether they want to define their own struct, or use GetEventDataValue, they will need to inspect the API documentation to parse the data.

Further compounding this, GetEventDataValue only returns string values, but there are webhook fields that are non-string values. So not only will they need to know which field(s) to extract from the payload, but they will also need to know the value's type and handle any conversions themselves.

Finally, if they want to use multiple values from within the webhook, they will need to extract those values individually, possibly convert some from a string to a more appropriate type (int, bool, etc.), and then put them inside of some container that allows them to be passed around. I suspect most folks would define a struct type for this container, if one wasn't already present in package webhookv3.

So considering all of these factors that would drive a consumer to define a struct type anyway, I am not personally in favor of this API being added to the webhookv3 package. Instead, I'd like to see us implement the most common types, commit to adding new types and fields as they are documented, and finally in cases where we have not defined the type yet encourage consumers to define their own struct types as needed and contribute them back as it makes sense.

[ChezCrawford]

While I am not going to attempt to unpack all of this here, I would be happy to Zoom or exchange a few emails if you would like to discuss further.

We would really like to keep this method in place and would appreciate any feedback you might have around how to make it better.

That being said, see what you think about ChezCrawford#2 for also adding support for structured events into the mix at the same time.

[theckman]

I'd still personally like to see this become func GetEventDataValue(e OutboundEvent, keys ...string) (string, error) since it's treating the public fields of the OutboundEvent as input, and is not modifying internal state of the value.