Toml webhook subscription parsing #3064

alexanderMontague · 2023-11-02T19:31:25Z

WHY are these changes introduced?

closes: https://github.com/Shopify/develop-app-management/issues/1500

Introduces schema changes to the app TOML that will provide the parsing and validation for declarative webhooks
Also somewhat consumes the beta, but not fully wired up to the mutation yet
The rest of the tidy up/wire up can come once we have mutations created to push declarative webhook data

WHAT is this pull request doing?

Adds new TOML fields, all are currently optional and validation is not checked until we turn it on. See the full property break down and content in which they refer to below
Validates the declarative webhook subscription heavily. Trying to give the best DX by surfacing errors early and often
Normalizes the TOML input into an easily consumable and consistent format we will ingest

How to test your changes?

spin up local branch, otherwise review the code
this is in a good spot but I expect this to change a bunch, so we can get this in and iterate

👀 New TOML Fields

[!NOTE]
These fields will exist on the schema but are not consumed until we enable the beta and remove static blocking flags

New Top Level Fields

[webhooks]
api_version = "2023-10" # existing 

# *NEW*
# subscription_endpoint_url - HTTPS endpoint
# pubsub_project & pubsub_topic - Google PubSub config
# arn - AWS Eventbridge ARN
# topics - String[] of top level topics

#  🛑 Only one of these destinations can be used at the top level

# Examples
subscription_endpoint_url = "https://my-app.com/webhooks"

pubsub_project = "absolute-feat-123"
pubsub_topic = "pub-sub-topic1"

arn = "arn:aws:events:us-west-2::event-source/aws.partner/shopify.com/123/SOMETHING"

topics = ['products/create', 'products/update', 'products/delete']

# existing
[webhooks.privacy_compliance]
customer_deletion_url = "https://example.com/privacy/customer_deletion"
customer_data_request_url = "https://example.com/privacy/customer_request"
shop_deletion_url = "https://example.com/privacy/deletion"

New subscriptions Level Fields

[webhooks]
api_version = "2023-10" # existing 

# Example top level config 
subscription_endpoint_url = "https://my-app.com/webhooks"
topics = ['products/create', 'products/update', 'products/delete']

# *NEW*
# topic - mandatory topic
# subscription_endpoint_url OR pubsub_project & pubsub_topic OR arn - The destination, that will override the top level destination if provided (optional if top level is defined)
# sub_topic - sub topic (optional)
# path - path to append to top level https URL or defined subscription_endpoint_url if defined (optional)
# include_fields - string[] (optional)
# metafield_namespaces - string[] (optional)
# format - 'json' | 'xml' (optional, defaults to json)

#  Examples

[[webhooks.subscriptions]]
topic = 'products/create'
path = '/my-neat-path'
subscription_endpoint_url = 'https://valid-url'

[[webhooks.subscriptions]]
topic = 'products/create'
sub_topic = 'something'
path = '/only-path'

[[webhooks.subscriptions]]
topic = 'products/create'
path = '/my-neat-path-new'
include_fields = ['variants', 'title']
metafield_namespaces = ['size']
subscription_endpoint_url = 'https://valid-url'

[[webhooks.subscriptions]]
topic = 'products/delete'
format = 'xml'
subscription_endpoint_url = 'https://valid-url'

[[webhooks.subscriptions]]
topic = 'products/delete'
format = 'json'
pubsub_project = "absolute-feat-123"
pubsub_topic = "pub-sub-topic1"

[[webhooks.subscriptions]]
subscription_endpoint_url = 'https://valid-url'
topic = 'products/delete'
format = 'xml'
path = "/absolute-feat-123"

[[webhooks.subscriptions]]
arn = "arn:aws:events:us-west-2::event-source/aws.partner/shopify.com/123/SOMETHING"
topic = 'products/delete'

[[webhooks.subscriptions]]
topic = "orders/delete"
path = "/absolute-feat-123"

👀 New CLI Content

content	TOML scenario
"Only https urls are allowed"	`subscription_endpoint_url` is not https
"URL can’t end with a forward slash"	`subscription_endpoint_url` ends with a forward slash (could conflict with path) ex. `https://example.ca/`
"Path must start with a forward slash and be longer than 1 character"	`path` does not start with a forward slash or is only 1 character (only the `/`)
"You must declare both pubsub_project and pubsub_topic if you wish to use a top-level pub sub destination"	Only one of pubsub_project or pubsub_topic is defined at the top level
"You must declare both pubsub_project and pubsub_topic if you wish to use a pub sub destination"	Only one of pubsub_project or pubsub_topic is defined at an inner subscription
"You are only allowed to declare one (1) of subscription_endpoint_url, pubsub_project & pubsub_topic, or arn at the top level"	Multiple top level destinations are defined at the top level
"You are only allowed to declare one (1) of subscription_endpoint_url, pubsub_project & pubsub_topic, or arn per subscription"	Multiple top level destinations are defined at an inner subscription
"To use a top-level destination, you must also provide a `topics` array or `subscriptions` configuration"	Top level destination is provided but no top level topics array or inner subscriptions
"To use top-level topics, you must also provide a top-level destination of either subscription_endpoint_url, pubsub_project & pubsub_topic, or arn"	Top level topics array is provided but no top level destination
"You can’t have duplicate subscriptions with the exact same topic and destination"	There are duplicate subscription definitions. This could be an identical topic/destination at the top level, an identical topic/destination at an inner subscription, or identical topic/destination defined through a combination of both
"You must declare either a top-level destination or a destination per subscription"	Inner subscriptions are defined, but there is no top level or sub level destination defined
"You must declare a subscription_endpoint_url if you wish to use a relative path"	No top level or sub level https URL is defined but path was included
"You can’t define a path when using arn or pubsub"	Path is defined at an inner level but so is an arn or pub sub configuration

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes
I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

github-actions · 2023-11-02T19:31:45Z

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

Themes: @shopify/advanced-edits
UI extensions: @shopify/ui-extensions-cli
- Checkout UI extensions: @shopify/checkout-ui-extensions-api-stewardship
Hydrogen: @shopify/hydrogen
Other: @shopify/cli-foundations

karenxie

didn't look at the tests yet but great work so far Alex! my comments are mostly for my own understanding and minor things that you may address as you work on this

packages/app/src/cli/models/app/app.ts

github-actions · 2023-11-08T21:51:44Z

We detected some changes at either packages/*/src or packages/cli-kit/assets/cli-ruby/** and there are no updates in the .changeset.
If the changes are user-facing, run "pnpm changeset add" to track your changes and include them in the next release CHANGELOG.

gonzaloriestra

Awesome work! Very nice validations and test suite 👌

I've fixed a couple of failing tests.

packages/app/src/cli/models/app/app.ts

gonzaloriestra · 2023-11-09T11:46:04Z

packages/app/src/cli/models/app/app.ts

+    .optional(),
+  pubsub_project: PubSubProjectValidation,
+  pubsub_topic: PubSubTopicValidation,
+  arn: ArnValidation,


What about using callback_url for everything instead of subscription_endpoint_url, pubsub_project, pubsub_topic and arn? As we do in the server now.

I was going to float the question about this, it would definitely simplify things! I think the biggest question was around the pub sub config. @karenxie @jenstanicak what do we think?

We call them endpoint types in the docs, could that work?

Well, the endpoint types are those 3 different options. But here we want to refer to the URI of each one where the event is going to be delivered:

URL for HTTPS

ARN for EventBridge

Project::Topic for Pub/Sub

My suggestion was to use the same field name for all of them, to simplify the TOML structure, because we won't allow multiple endpoint types at the same time. Something like callback_uri, destination_uri or endpoint_uri.

Note
As @alexanderMontague commented recently, to be precise, we should use URI instead of URL. URI is a general resource identifier, while URL is a specific type of URI for Web (the ones starting with http, unlike the ARNs).

I'm on board with the single URI identifier if that fits in with our UX/DX. It will simplify the error checking, code and validation content. The only main thing is for pub sub you won't have both the project and topic level overrides, which I dont think is a huge deal as you'll save a toml line and you can just type out the whole pub sub URI if you want something different.

For an update here, I've gone ahead and replaced the individual "destinations/endpoints" with a singular endpoint key. Example of a full TOML would be:

[webhooks] api_version = "2023-10" endpoint = "https://my-app.com/webhooks" topics = ['products/create', 'products/update', 'products/delete'] [[webhooks.subscriptions]] topic = 'products/create' path = '/my-neat-path' endpoint = 'https://valid-url' [[webhooks.subscriptions]] topic = 'products/create' sub_topic = 'something' path = '/only-path' [[webhooks.subscriptions]] topic = 'products/create' path = '/my-neat-path-new' include_fields = ['variants', 'title'] metafield_namespaces = ['size'] endpoint = 'https://valid-url' [[webhooks.subscriptions]] topic = 'products/delete' format = 'xml' endpoint = 'https://valid-url' [[webhooks.subscriptions]] topic = 'products/delete' format = 'json' endpoint = "pubsub://absolute-feat-123:pub-sub-topic1" [[webhooks.subscriptions]] endpoint = 'https://valid-url' topic = 'products/delete' format = 'xml' path = "/absolute-feat-123" [[webhooks.subscriptions]] endpoint = "arn:aws:events:us-west-2::event-source/aws.partner/shopify.com/123/SOMETHING" topic = 'products/delete' [[webhooks.subscriptions]] topic = "orders/delete" path = "/absolute-feat-123"

Regarding the name, let me add some extra confusion:

In the REST API we call it address: Destination URI to which the webhook subscription should send the POST request when an event occurs.

In the GraphQL API there's a property called endpoint, which can contain:

callbackUrl

arn

pubSubProject/pubSubTopic

In the database we store it as address.

So if we want to be consistent with the current APIs, maybe the most accurate term would be address, which matches what we have here (the URI for any kind of endpoint type). What do you think?

packages/app/src/cli/models/app/app.ts

github-actions · 2023-11-09T13:31:05Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	73.07% (+0.19% 🔼)	6172/8447
🟡	Branches	70.38% (+0.27% 🔼)	3027/4301
🟡	Functions	71.57% (-0.25% 🔻)	1571/2195
🟡	Lines	74.27% (+0.26% 🔼)	5862/7893

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / webhooks.ts	94.44%	100%	60%	94.12%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / extension-instance.ts	78.16% (-1.84% 🔻)	70.31% (-3.22% 🔻)	87.1%	80.49% (-1.87% 🔻)
🟢	`...` / context.ts	89.71% (-0.15% 🔻)	87.79%	87.1%	90.1% (-0.15% 🔻)
🟢	`...` / generate-schema.ts	96.43% (-0.12% 🔻)	92.86%	100%	96.43% (-0.12% 🔻)
🟢	`...` / push.ts	82.98% (-0.35% 🔻)	85.71% (+1.27% 🔼)	66.67%	86.05% (-1.71% 🔻)
🟢	`...` / fetch.ts	78.72% (-4.33% 🔻)	66.67% (-8.33% 🔻)	80% (-4.62% 🔻)	81.4% (-3.22% 🔻)
🟢	`...` / bundler.ts	91.18% (+0.61% 🔼)	93.33% (-1.67% 🔻)	75% (+1.32% 🔼)	91.04% (+0.66% 🔼)
🟢	`...` / utilities.ts	97.37% (-0.25% 🔻)	100%	80%	97.37% (-0.25% 🔻)
🔴	`...` / server.ts	0% (-1.52% 🔻)	0%	0%	0% (-1.54% 🔻)
🔴	`...` / draftable-extension.ts	28.57% (-18.49% 🔻)	10% (-15% 🔻)	33.33% (-16.67% 🔻)	29.17% (-17.5% 🔻)
🔴	`...` / graphiql.ts	50% (+10% 🔼)	50% (-50% 🔻)	33.33%	50% (+10% 🔼)
🟢	`...` / Dev.tsx	93.33% (-0.09% 🔻)	86.05% (+1.6% 🔼)	89.47%	94.29% (-0.08% 🔻)
🟡	`...` / error.ts	66% (-2.63% 🔻)	59.38% (-3.13% 🔻)	72.73% (-9.09% 🔻)	65.31% (-2.69% 🔻)
🔴	`...` / ui.tsx	16.22% (-18.31% 🔻)	20.69% (-15.42% 🔻)	22.22% (-18.4% 🔻)	17.91% (-16.66% 🔻)
🟢	`...` / admin.ts	80% (-2.76% 🔻)	37.5%	87.5% (-2.5% 🔻)	83.33% (-2.38% 🔻)

Test suite run success

1480 tests passing in 666 suites.

Report generated by 🧪jest coverage report action from 14fb416

packages/app/src/cli/services/app/write-app-configuration-file.ts

packages/app/src/cli/services/deploy.ts

alexanderMontague · 2023-11-09T16:44:29Z

packages/app/src/cli/services/deploy.ts

@@ -119,6 +125,66 @@ export async function deploy(options: DeployOptions) {
        },
      ]

+      if (partnersApp.betas?.declarativeWebhooks) {


@gonzaloriestra thoughts on keeping this as a separate task? I think the code should definitely be separate, but how does this play into the deploy task especially once we start consuming other declarative app config. I think they somehow have to be tied together as this will ultimately be part of the new deployed app version

Yeah, better to separate the code (I'd even extract the task to another function). Regarding the tasks, from a UX perspective, I also see two concepts here: deploying extensions and configuration.

But I'm wondering if it will happen too quickly to read the messages, so maybe it would be better to have a single task. It depends on the timing. We can decide later, once we can check it properly.

I think it will definitely be too quick to read the messages, so agree it makes sense to keep under the main deploy task. We'll pull the code out eventually though once things are finalized. The versioned config getting update is still an atomic action of an app deploy

Are you finally using a single task?

finalize toml parser and validations, begin on finishing tests finish tests

finalize webhook config normalization task and tests

refactor and split out validations rebase and fix imports Fix tests fix type circular dependencies

jenstanicak · 2023-11-09T21:41:14Z

Thanks for pulling the content into the table, Alex! So helpful for me.

Couple of persistent things:

instead of declare, what about include? Soft opinion, but wondering if declare matches how devs think of what they're doing?
you can drop if you wish throughout, like You must declare a subscription_endpoint_url to use a relative path

And then a few specific callouts (as long as these changes are accurate!):

Current	Change
`Only https urls are allowed`	`URLs must start with https`
`You must declare both pubsub_project and pubsub_topic if you wish to use a top-level pub sub destination`	`You need to include both pubsub_project and pubsub_topic` Do we need to specify the destination or could we just use the same message for both?
`You are only allowed to declare one (1) of subscription_endpoint_url, pubsub_project & pubsub_topic, or arn at the top level`	`You can only use 1 endpoint type in [webhooks]. To use different endpoint types, configure subscriptions individually` Pending if endpoint type is the right word here

alexanderMontague · 2023-11-14T21:15:18Z

packages/app/src/cli/utilities/app/config/webhooks.ts

+      message: 'To use a top-level `endpoint`, you must also provide a `topics` array or `[[webhooks.subscriptions]]`',
+      fatal: true,
+    }
+  }
+
+  if (!hasEndpoint && hasTopics) {
+    return {
+      code: zod.ZodIssueCode.custom,
+      message: 'To use top-level topics, you must also provide a top-level `endpoint`',


@jenstanicak the content / error messages for these scenarios changed slightly with the endpoint rename

gonzaloriestra

👏 👏 👏

packages/app/src/cli/models/app/app.ts

gonzaloriestra · 2023-11-15T10:45:08Z

packages/app/src/cli/services/deploy.ts

@@ -119,6 +125,66 @@ export async function deploy(options: DeployOptions) {
        },
      ]

+      if (partnersApp.betas?.declarativeWebhooks) {


Are you finally using a single task?

alexanderMontague · 2023-11-15T19:29:35Z

Are you finally using a single task?

@gonzaloriestra This is still technically a separate task, once we wire up the mutation/connectivity I will combine it with the deploy task. I also want to sync with Phong on the best way to do this such that it scales with all the other config as well

karenxie

fantastic work here Alex! 👏 All makes sense to me - my comments are non-blocking

packages/app/src/cli/services/deploy.test.ts

packages/app/src/cli/models/app/loader.test.ts

packages/app/src/cli/utilities/app/config/webhooks.ts

karenxie reviewed Nov 3, 2023

View reviewed changes

alexanderMontague force-pushed the toml-webhook-subscription-parsing branch 2 times, most recently from d0ba034 to 833bbcd Compare November 8, 2023 21:51

alexanderMontague marked this pull request as ready for review November 8, 2023 21:51

alexanderMontague force-pushed the toml-webhook-subscription-parsing branch from 833bbcd to 1289360 Compare November 8, 2023 21:56

gonzaloriestra reviewed Nov 9, 2023

View reviewed changes

alexanderMontague commented Nov 9, 2023

View reviewed changes

packages/app/src/cli/services/app/write-app-configuration-file.ts Show resolved Hide resolved

alexanderMontague commented Nov 9, 2023

View reviewed changes

packages/app/src/cli/services/deploy.ts Outdated Show resolved Hide resolved

alexanderMontague commented Nov 9, 2023

View reviewed changes

alexanderMontague added 5 commits November 9, 2023 14:57

add declarative webhook TOML parsing and validation

4fd0a47

finalize toml parser and validations, begin on finishing tests finish tests

add topic and destination uniqueness + tests

16c3b02

finish normalization and serializing of webhook config

4374598

finalize webhook config normalization task and tests

make reusable error messages getter functions for better visibility

4192c0d

split up webhook schema validation

f0c6e87

refactor and split out validations rebase and fix imports Fix tests fix type circular dependencies

alexanderMontague force-pushed the toml-webhook-subscription-parsing branch from 36a7a2a to f0c6e87 Compare November 9, 2023 19:57

alexanderMontague force-pushed the toml-webhook-subscription-parsing branch from 9ecbcaf to f110b5d Compare November 14, 2023 21:09

alexanderMontague commented Nov 14, 2023

View reviewed changes

gonzaloriestra approved these changes Nov 15, 2023

View reviewed changes

alexanderMontague force-pushed the toml-webhook-subscription-parsing branch from f110b5d to eaee6d2 Compare November 15, 2023 19:30

alexanderMontague requested review from jenstanicak and karenxie November 15, 2023 19:30

karenxie approved these changes Nov 15, 2023

View reviewed changes

change toml content and use singular enpoint concept

14fb416

alexanderMontague force-pushed the toml-webhook-subscription-parsing branch from eaee6d2 to 14fb416 Compare November 16, 2023 17:09

alexanderMontague added this pull request to the merge queue Nov 16, 2023

Merged via the queue into main with commit 6ca39b7 Nov 16, 2023

alexanderMontague deleted the toml-webhook-subscription-parsing branch November 16, 2023 18:36

shopify-shipit bot temporarily deployed to nightly November 17, 2023 15:33 Inactive

shopify-shipit bot had a problem deploying to experimental November 20, 2023 10:30 Failure

shopify-shipit bot temporarily deployed to experimental November 20, 2023 10:58 Inactive

shopify-shipit bot temporarily deployed to production December 6, 2023 13:21 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toml webhook subscription parsing #3064

Toml webhook subscription parsing #3064

alexanderMontague commented Nov 2, 2023 •

edited

Loading

github-actions bot commented Nov 2, 2023

karenxie left a comment

github-actions bot commented Nov 8, 2023

gonzaloriestra left a comment

gonzaloriestra Nov 9, 2023

alexanderMontague Nov 9, 2023

jenstanicak Nov 9, 2023

gonzaloriestra Nov 10, 2023

alexanderMontague Nov 10, 2023 •

edited

Loading

alexanderMontague Nov 14, 2023

gonzaloriestra Nov 15, 2023

github-actions bot commented Nov 9, 2023 •

edited

Loading

alexanderMontague Nov 9, 2023

gonzaloriestra Nov 9, 2023

alexanderMontague Nov 9, 2023

gonzaloriestra Nov 15, 2023

jenstanicak commented Nov 9, 2023

alexanderMontague Nov 14, 2023

gonzaloriestra left a comment

gonzaloriestra Nov 15, 2023

alexanderMontague commented Nov 15, 2023

karenxie left a comment

Toml webhook subscription parsing #3064

Toml webhook subscription parsing #3064

Conversation

alexanderMontague commented Nov 2, 2023 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

👀 New TOML Fields

👀 New CLI Content

Checklist

github-actions bot commented Nov 2, 2023

karenxie left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 8, 2023

gonzaloriestra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderMontague Nov 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 9, 2023 • edited Loading

Coverage report

Test suite run success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenstanicak commented Nov 9, 2023

Choose a reason for hiding this comment

gonzaloriestra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderMontague commented Nov 15, 2023

karenxie left a comment

Choose a reason for hiding this comment

alexanderMontague commented Nov 2, 2023 •

edited

Loading

alexanderMontague Nov 10, 2023 •

edited

Loading

github-actions bot commented Nov 9, 2023 •

edited

Loading