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

Prevent app to receive webhook for event this app has triggered #12538

Open
lkostrowski opened this issue Apr 13, 2023 · 2 comments
Open

Prevent app to receive webhook for event this app has triggered #12538

lkostrowski opened this issue Apr 13, 2023 · 2 comments
Labels
apps Issues related to apps webhooks

Comments

@lkostrowski
Copy link
Member

lkostrowski commented Apr 13, 2023

Problem

There is an App, likely integrated with external systems, like CMS.

This app performs the following flow:

  1. Listen on PRODUCT_UPDATED or PRODUCT_VARIANT_UPDATED (or created)
  2. Save a reference to Saleor Product/Variant ID in the external system (CMS)
  3. Receive ID from external system and save it in Product/Variant metadata/external ID

So, the app ensures the product is linked between systems

The problem is that app, saving metadata/external ID of the product, is triggering PRODUCT_UPDATED event, so it's called again. This leads to 3 problems:

  1. Unnecessary traffic on Saleor side
  2. Unnecessary traffic on the App side
  3. App must write a code to ignore the second webhook, possibly must fetch more data and compare the diff

Real example of webhook payload for PRODUCT_VARIANT_UPDATED

{
  "__typename": "ProductVariantUpdated",
  "issuedAt": "2023-04-13T12:58:54.469111+00:00",
  "issuingPrincipal": {
    "__typename": "App",
    "id": "QXBwOjY5Nw=="
  },
  "recipient": {
    "id": "QXBwOjY5Nw=="
  }
}

Proposed solution

Introduce a new field to webhookCreate and manifest webhook creation that can be named like omitForSelfEvents (probably something better). With default value false (current behavior), and with Saleor v4 - true.

There should be decision if this flag should be "true" or "false" by default

@lkostrowski lkostrowski changed the title Prevent app to receive webhook for event this app created Prevent app to receive webhook for event this app has triggered Apr 13, 2023
@korycins
Copy link
Member

Introduce a new field to webhookCreate and manifest webhook creation that can be named like omitForSelfEvents (probably something better). With default value false (current behavior), and with Saleor v4 - true.

@lkostrowski
Question: Why do you want to change the behavior of this flag between 3.x and 4.0?

I think that some apps may expect to receive the payload triggered by themselves. For example, an app that finalizes the checkout by calling "orderCreateFromCheckout" may also expect to receive the "order-fully-paid" webhook.

I believe that there is no reason to switch the value between Saleor versions. If you, as an app developer, do not expect to receive self-events, then you can set this flag to true. Otherwise, you will need to add additional implementation to handle this case.


A helpful change we could make in Saleor 4.0 is the behavior of product_variant_updated. Currently, it is triggered whenever there is a change in the metadata of a variant. However, we already have a separate webhook for this action: PRODUCT_VARIANT_METADATA_UPDATED. We could change product_variant_updated to not be triggered for metadata changes and assume that everyone who expects to receive metadata changes would be subscribed to PRODUCT_VARIANT_METADATA_UPDATED.


Another way to simplify the validation of current events that are sent to the app would be to include an identifier in the App type. This way, you could fetch it in issuingPrincipal.


If we agree to provide omitForSelfEvents, we would also need to add it to webhookUpdate and to the manifest flow installation.

@korycins korycins added webhooks apps Issues related to apps labels Apr 14, 2023
@lkostrowski
Copy link
Member Author

Question: Why do you want to change the behavior of this flag between 3.x and 4.0?

I think that some apps may expect to receive the payload triggered by themselves. For example, an app that finalizes the >checkout by calling "orderCreateFromCheckout" may also expect to receive the "order-fully-paid" webhook.

My initial thought was that it should be the default behaviour (hence the change in v4 to avoid breaking change). But if it makes sense to be the default - that's fine, I'm editing the original description. If we document it well, I think both options are fine.

Another way to simplify the validation of current events that are sent to the app would be to include an identifier in the App >type. This way, you could fetch it in issuingPrincipal.

I think its not a problem for app to detect this event, rather optimize unnecessary traffic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apps Issues related to apps webhooks
Projects
None yet
Development

No branches or pull requests

2 participants