-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Normalize webhook payloads #19110
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
Normalize webhook payloads #19110
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've been busy! Looks good to me so far. I've left a few, minor comments. Can move on to testing once you've raised as a "ready for review" PR.
Meantime a couple of general points to make sure we've considered.
- If someone wants to amend the webhook response for a particular type and format (e.g. the content published for extended format), can they? Would be a nice extension point if it were possible.
- Some third party packages uses webhooks - at least Forms, Deploy and Commerce. They are presumably happy with the formats they have, but may want to use this new option for "minimal" and "extended". Do we have a good story there - so that they don't have to change if they don't want to when upgrading to 16, but if they did, they could hook into these same format options?
src/Umbraco.Core/Webhooks/WebhookEventCollectionBuilderCmsUserExtensions.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/LegacyEvents/User/LegacyUserSavedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/Events/User/UserPasswordResetWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/Events/User/UserPasswordChangedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/Events/User/UserForgotPasswordRequestedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/Events/ContentType/DocumentTypeChangedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/Events/ContentType/MediaTypeChangedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/Events/ContentType/MemberTypeChangedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/Events/Package/ImportedPackageWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/Events/User/UserPasswordChangedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/Events/User/UserUnlockedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/ExtendedEvents/Content/ExtendedContentPublishedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/ExtendedEvents/Content/ExtendedContentPublishedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/ExtendedEvents/Content/ExtendedContentSavedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
src/Umbraco.Core/Webhooks/ExtendedEvents/Content/ExtendedContentSavedWebhookEvent.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Kenn Jacobsen <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The refactorings look good to me 👍 I'm all for merging it if it tests out good.
Could someone just address my two comments from above please? Even if it's just "don't worry, we've thought about it, and we'll address in the documentation"! |
Sorry, i Thought i had already replied to this.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tested with various webhooks and settings and all looks to be working as expected - backward compatible but with these new slim and extended options. Nice work @Migaroez.
I made a couple of small updates:
- Fixed the failing unit tests.
- Created a constant for the default payload type so we only define this in one place.
I'll leave you to merge if that's OK, partly just so you can check you are happy with the small changes I made, and partly as we'll want this in release/16.0
and would like you to be sure when it's cherry-picked across it doesn't bring anything else with it.
Co-authored-by: Andy Butland <[email protected]> Co-authored-by: Kenn Jacobsen <[email protected]>
General
Since the introduction of the webhooks (almost) every event has been covered by a webhook either by HQ or the community. Since then we have noticed that most of the payloads do not align with what we see as a good standard payload to ship with the framework.
In essence, we believe that a webhook is an external extension of the internal domain events. Since the consumption of the webhook payload is no longer part of the internal domain, we believe none of the internal information should be exposed.
There are of course exceptions to this rule, mainly when dealing with published (public) information. For this reason we also added an extended set of payloads that aims at passing along all relevant public information about the concerned entity.
Minimal payloads
We aim to supply only the ID of the entity the webhook relates to and information that is not obtainable after the webhook has been fired, for example in the case of the UserUnlockedNotification we also send the ID of the user who did the unlocking which can differ from the user being unlocked. This makes it that by default the webhook payloads do not leak unwanted information as the API user used to retrieve the full entity will only be able to fetch what it is allowed trough its configured permissions.
Extended payloads
As mentioned above, these currently only exist for
These webhooks will use the management api content builders to construct their payload similar to how the legacy webhooks did this, with one improvement. In a multi culture setup, the result will return all available culture representations of the content.
Legacy payloads
These contain all the payloads as they were before this PR. These will be removed starting from v18, see the next section for more details.
Switching payloads
A new setting was introduced:
Umbraco:CMS:Webhook:PayloadType
to switch between the 3 payload types discussed above. For now the default isLegacy
, this will change tominimal
starting from v17.When switching to extended payloads, any notification that is not covered by an extended webhook will get a minimal assigned instead.
Customizing payloads
Just like before, you can remove any webhook loaded into the DI container by default and replace it with your custom webhook and thus payload as you see fit.
Concerns
@kjac Because of the inner workings of nested content, I had to update the variationContextAccessor.VariationContext mid request a few times to be able to fetch all relevant published/preview data. For safety sake I hold a copy of the original and set it back before the webhook finishes. If you have a better idea, please chip in.
Testing
This is a humongous PR with a lot of things to test. I would advice to at least make sure we can swap between the different payload types and that the extended ones function as desired. I have done my best to test every single webhook I added, but I have worked in batches so might have forgotten one here or there.