Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

[WIP] [Feature] Pass webhook id to handlers #606

Merged
merged 4 commits into from
Dec 2, 2022
Merged

[WIP] [Feature] Pass webhook id to handlers #606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4ee5e39
Pass webhook id to handlers
jbwyme Nov 21, 2022
cfe86d8
remove unnecessary new line
jbwyme Nov 21, 2022
4abd265
cleanup + tests + changelog
jbwyme Nov 30, 2022
e0817f6
fix tests
jbwyme Dec 1, 2022
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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ and adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

## Unreleased

- [Minor] Pass webhook id to handlers

## [5.2.0] - 2022-10-04

- Added support for the `2022-10` API version [#535](https://github.com/Shopify/shopify-api-node/pull/535)
Expand Down
1 change: 1 addition & 0 deletions src/base-types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,5 @@ export enum ShopifyHeader {
Hmac = 'X-Shopify-Hmac-Sha256',
Topic = 'X-Shopify-Topic',
Domain = 'X-Shopify-Shop-Domain',
WebhookId = 'X-Shopify-Webhook-Id',
}
8 changes: 7 additions & 1 deletion src/webhooks/__tests__/registry.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,9 @@ async function genericWebhookHandler(
topic: string,
shopDomain: string,
body: string,
webhookId?: string,
): Promise<void> {
if (!topic || !shopDomain || !body) {
if (!topic || !shopDomain || !body || !webhookId) {
throw new Error('Missing webhook parameters');
}
}
Expand Down Expand Up @@ -702,11 +703,13 @@ function headers({
hmac = 'fake',
topic = 'products',
domain = 'shop1.myshopify.io',
webhookId = 'b54557e4-bdd9-4b37-8a5f-bf7d70bcd043',
lowercase = false,
}: {
hmac?: string;
topic?: string;
domain?: string;
webhookId?: string;
lowercase?: boolean;
} = {}) {
return {
Expand All @@ -715,6 +718,9 @@ function headers({
topic,
[lowercase ? ShopifyHeader.Domain.toLowerCase() : ShopifyHeader.Domain]:
domain,
[lowercase
? ShopifyHeader.WebhookId.toLowerCase()
: ShopifyHeader.WebhookId]: webhookId,
};
}

Expand Down
8 changes: 8 additions & 0 deletions src/webhooks/registry.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,6 +360,7 @@ const WebhooksRegistry: RegistryInterface = {
let hmac: string | string[] | undefined;
let topic: string | string[] | undefined;
let domain: string | string[] | undefined;
let webhookId: string | string[] | undefined;
Object.entries(request.headers).map(([header, value]) => {
switch (header.toLowerCase()) {
case ShopifyHeader.Hmac.toLowerCase():
Expand All @@ -371,6 +372,9 @@ const WebhooksRegistry: RegistryInterface = {
case ShopifyHeader.Domain.toLowerCase():
domain = value;
break;
case ShopifyHeader.WebhookId.toLowerCase():
webhookId = value;
break;
}
});

Expand All @@ -384,6 +388,9 @@ const WebhooksRegistry: RegistryInterface = {
if (!domain) {
missingHeaders.push(ShopifyHeader.Domain);
}
if (!webhookId) {
missingHeaders.push(ShopifyHeader.WebhookId);
}
Comment on lines +391 to +393
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this header isn't strictly necessary for validation, I wonder if we want to fail on it missing. That being said, we should always have the header so I suppose there's no harm in checking 🙂.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's optional in the WebhookHandlerFunction type, so it probably shouldn't be mandatory here. The existing tests aren't passing because of this check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My 2c: It's optional in the type for backwards compatibility but the expected behavior is the header always exists. Therefore, I'd argue this is more correct. However, it's a minor nit and it probably doesn't matter much either way. Happy to go either direction!


if (missingHeaders.length) {
response.writeHead(StatusCode.BadRequest);
Expand Down Expand Up @@ -417,6 +424,7 @@ const WebhooksRegistry: RegistryInterface = {
graphqlTopic,
domain as string,
reqBody,
webhookId as string,
);
statusCode = StatusCode.Ok;
} catch (error) {
Expand Down
1 change: 1 addition & 0 deletions src/webhooks/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ type WebhookHandlerFunction = (
topic: string,
shop_domain: string,
body: string,
webhookId?: string | undefined,
) => Promise<void>;

export interface RegisterOptions {
Expand Down