Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create AEP-501: Webhook payloads #209

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 124 additions & 0 deletions aep/webhooks/501/aep.md.j2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why 501? considering the number of the AEP shouldn't matter too much in the future, I'd rather we just start grabbing the next unused integer. What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just picked a number, happy to renumber it. I think 6 is the next available number right?

Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
# Webhook payloads

Webhooks are an event-based API style in which the API server calls the client,
rather than the other way around. API consumers register callback URIs for
specific events. When an event occurs, the server calls the consumer's
registered callback URI with information related to the event.

The AEPs in this section provide guidance for both the behavior of webhook APIs
rofrankel marked this conversation as resolved.
Show resolved Hide resolved
and the structure of the request made to a client when an event occurs.

## Guidance

Webhook APis **must** pass callback URIs an
rofrankel marked this conversation as resolved.
Show resolved Hide resolved
[`aep.api.webhooks.Notification`][webhook-proto]. Information specific to the
event type **must** be represented in the `payload` field.

### Payloads

Every event type **must** have a registered payload. For event types that will
_never_ contain any event type-specific information, `google.protobuf.Empty`
**may** be used. However, event types which may add event type-specific
information in the future **should** define an empty payload message to which
fields can be added as needed.
Comment on lines +21 to +23
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like an empty payload would basically be a backwards-incompatible choice. I think we should always have a custom payload for the event.

our goal is to help people from painting themselves into a corner - requiring a custom payload would ensure that.


#### Events triggered by service API methods on resources
rofrankel marked this conversation as resolved.
Show resolved Hide resolved

If the event was triggered by a service API method on a resource, the name of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If the event was triggered by a service API method on a resource, the name of
If the event was triggered by a method on a resource, the name of

similar to above, I don't think "service API" adds much.

the event type **should** be the resource name followed by the past participle
of the method's verb. For example, the standard `CreateBook` method would
trigger a `BookCreated` event; the custom `ArchiveBook` method would trigger a
`BookArchived` event.
Comment on lines +28 to +31
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering how complex english conjugation is, this pattern wouldn't be machine-readable. So if you wanted to, for example, link a callback to the RPC that called it, you'd have to have an annotation on it anyway. Is that ok?

Comment on lines +28 to +31
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

considering how complex english conjugation is, this pattern wouldn't be machine-readable. So if you wanted to, for example, link a callback to the RPC that it's eventing off of, you'd have to have an annotation on it anyway. Is that ok?


**Note:** When this would result in an ungrammatical event type, which may
occur with multi-word verbs, an equivalent grammatically correct form
**should** be used. For example, for a `GenerateBookSynopsis` custom method
with a verb form like `book:generateSynopsis`, the payload should be named
`BookSynopsisGenerated`.

The first field of the payload **must** be a
[resource reference](./association) to the resource in question.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[resource reference](./association) to the resource in question.
[resource reference](/association) to the resource in question.

absolute paths allow us to move aeps without having dead links.


{% tab proto %}

```proto
// Payload for the BookArchived event, which fires after the `ArchiveBook`
// method is successfully executed.
message BookArchived {
option (aep.api.webhook_payload) = {
event_type: "BookArchived"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this also be prefixed with the API name, similar to resource type? e.g. bookstore.example.com/BookArchived?

}

// The path of the book.
// Format: publishers/{publisher}/books/{book}
string book = 1 [
(google.api.resource_reference) = {
type: "apis.example.com/library/Book"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type: "apis.example.com/library/Book"
type: "apis.example.com/Book"

resource types don't typically have parents... that's more of a unique identifier of a collection.

}];
}
```

- The payload message **must** be annotated with the
[`aep.api.webhook_payload`][webhook-proto] option, which **must** include the
`event_type` field with the name of the event type.

{% tab oas %}

**Note:** OAS example not yet written.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should have OAS examples for new AEPs - there was justification when we were porting but I think we should think about the ramifications of new patterns in both proto and HTTP JSON context.


{% endtabs %}

The `book` field may be also be an
[embedded resource reference](./association#embedded-resources):
Comment on lines +71 to +72
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this universally applicable to all resource references? why is this guidance unique to webhooks?


{% tab proto %}

```proto
// Payload for the BookArchived event, which fires after the `ArchiveBook`
// method is successfully executed.
message BookArchived {
option (aep.api.webhook_payload) = {
event_type: "BookArchived"
}

// The book.
Book book = 1 [
(google.api.resource_reference) = {
type: "apis.example.com/library/Book"
}];
}
```
Comment on lines +74 to +90
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of having multiple examples of slightly variations in patterns - it's very expensive to find all the examples and update them.

The pattern of embedded resources is well spelled out in http://localhost:4001/124#embedded-resources - why duplicate?


{% tab oas %}

**Note:** OAS example not yet written.

{% endtabs %}

### Versioning

Webhook payloads **must** be versioned, and API consumers **must** register
callback URIs for a specific version of a payload.
Comment on lines +100 to +101
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this different from a regular resource?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Webhook payloads aren't resources at all, so I don't think we can treat guidance about resources as applying to them.


Webhook payloads with resource references **must** be versioned with the API
containing those resources. Webhook payloads with resource references to
resources in multiple APIs must be versioned with one of those APIs, and **must
not** change which API is used to version the payload.

Webhook payloads without resource references, and without any other
relationship to a service API, **must** be versioned.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you mean by "service API"? do you mean "API service"? https://aep.dev/3#api-service.

If service API is intended to be a new term that differs, it would be good to add it to the Glossary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By "service API" I mean an API that accepts requests, as distinct from a webhook API where the client accepts requests from the API. Happy to add to the glossary if you agree this makes sense.


Breaking changes to a webhook payload **must not** be made within a major
version.
Comment on lines +111 to +112
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this any different from aip.dev/180? (pointing to AIPs since we haven't imported it yet).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well...I don't think it's obvious to everyone (or at least it wasn't within Roblox) which rules of service APIs would also apply to webhooks. Thus a general pattern of restating stuff a bit (while staying as DRY as possible).


### Additional metadata

Payloads **must** include only information specific to the event that triggered
the webhook callback. Any additional metadata not pertaining to the event
should be sent in a side channel and **must not** be included in the payload.
Comment on lines +116 to +118
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this guidance is actionable. If I wanted to include something in the webhook payload, it's probably because some user needed it, and that's enough of a reason arguably to say it's information specific to the event.

Also if I include non-event related data, it wouldn't break any clients or doc generators. So is it really a must? If this was the only thing that an API did that wasn't AEP compliant, would we be ok with them not adopting the AEPs for this reason?

Copy link
Collaborator Author

@rofrankel rofrankel Aug 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I agree - this is probably a specific reaction to a question that came up at Roblox, but it was an odd question and likely wouldn't come up elsewhere.

Do you think we should change must to should here or just remove the guidance?

This includes standard headers such as "retry-count", "signature", and
"request-origin".
Comment on lines +119 to +120
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this standard guidance regardless of webhooks? I feel like guidance around what goes in a side-channel vs not could be it's own AEP.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be...this guidance might overfit specific questions that arose within Roblox. Happy to file a ticket for that. Think we should leave this here in the meantime?


<!-- prettier-ignore-start -->
[webhook-proto]: https://github.com/aep-dev/aep/blob/main/proto/aep-api/aep/api/webhook.proto
<!-- prettier-ignore-end -->
8 changes: 8 additions & 0 deletions aep/webhooks/501/aep.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
id: 5001
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

501 or 5001?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack but will let the other comment get resolved first.

state: approved
slug: webhook-payloads
created: 2024-08-16
placement:
category: webhooks
order: 510
6 changes: 6 additions & 0 deletions aep/webhooks/scope.yaml
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need a new directory for webhooks? this feels like a regular design pattern to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have three webhook-specific AIPs internally at Roblox (this is the most interesting one). If we were to have multiple webhook-specific AEPs would you still suggest not having a folder? (No strong feelings on my end either way.)

Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
title: Webhooks
order: 500
categories:
- code: webhooks
title: Webhooks
Loading