Create AEP-501: Webhook payloads #209
Conversation
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I just picked a number, happy to renumber it. I think 6 is the next available number right?
aep/webhooks/scope.yaml
Outdated
There was a problem hiding this comment.
why do we need a new directory for webhooks? this feels like a regular design pattern to me.
There was a problem hiding this comment.
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.)
| @@ -0,0 +1,8 @@ | |||
| --- | |||
| id: 5001 | |||
There was a problem hiding this comment.
Ack but will let the other comment get resolved first.
| Webhook payloads **must** be versioned, and API consumers **must** register | ||
| callback URIs for a specific version of a payload. |
There was a problem hiding this comment.
is this different from a regular resource?
There was a problem hiding this comment.
Webhook payloads aren't resources at all, so I don't think we can treat guidance about resources as applying to them.
| 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
Is this any different from aip.dev/180? (pointing to AIPs since we haven't imported it yet).
There was a problem hiding this comment.
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).
| This includes standard headers such as "retry-count", "signature", and | ||
| "request-origin". |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| 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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
|
High level question: have you looked at other standards like https://www.standardwebhooks.com/? That guidance has some pros:
I imagine you're motivated by providing webhook guidance around protobuf, but it might be good to base it on an HTTP+JSON standard. |
Co-authored-by: Yusuke Tsutsumi <[email protected]>
Co-authored-by: Yusuke Tsutsumi <[email protected]>
Co-authored-by: Yusuke Tsutsumi <[email protected]>
Adds guidance for webhook payloads.
The referenced common components are added in aep-dev/aep#17.
🍱 Types of changes
What types of changes does your code introduce to AEP? Put an
xin the boxesthat apply
📋 Your checklist for this pull request
Please review the AEP Style and Guidance for
contributing to this repository.
General
references AEPs
correctly.
(usually
prettier -w .)Additional checklist for a new AEP
guidelines are met.
Document structure
guidelines are met.
💝 Thank you!