Skip to content

Commit 451f983

Browse files
andy-liuuandy-liuu
authoredNov 5, 2024
Fix: ShopifyAPI::Webhooks::Registry doesn't update webhooks if metafield_namespaces has changed (Shopify#1344)
* remove dead code in test * make the do registration test helper more granular, update how it's called * fill out all test scenarios, temp set check to true * remove unused comments and temporary bypass that made unit tests pass * the dumb solution * add no registration done tests for http, pubsub, eventbridge * make parse type untyped and possibly sort arrays when checking for equality * invert order of attribute arrays for testing equality test * changelog * make type conversions more explicit, add unit test for edgecase of comparing nil and empty array * remove removing duplicate code * initial unit test refactoring: put everything in a loop * rename to registry test queries * pull out adding and registring webhooks into seperate fn * allow more addresses in the loop, remove hardcoding 2 other tests * rename constant * make test names shorter for rubocop * bring in http/pubsub/eventbridge registration error into loop * rename test * refactor away no registration test helper function because it's only used once * rename test again * remove only instance of check error test to abstract less * consolidate adding new webhook test, reduces redundancy * helper function to setup tests and expected queries * totally refactor away do registration test, and use setup queries helper for the other tests * rename test
1 parent 39eda8b commit 451f983

File tree

8 files changed

+488
-294
lines changed

8 files changed

+488
-294
lines changed
 

‎CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
Note: For changes to the API, see https://shopify.dev/changelog?filter=api
44
## Unreleased
55

6+
- [#1344](https://github.com/Shopify/shopify-api-ruby/pull/1344) Allow ShopifyAPI::Webhooks::Registry to update a webhook when fields or metafield_namespaces are changed.
67
- [#1343](https://github.com/Shopify/shopify-api-ruby/pull/1343) Make ShopifyAPI::Context::scope parameter optional. `scope` defaults to empty list `[]`.
78

89
## 14.6.0

‎lib/shopify_api/webhooks/registration.rb

+8-1
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,14 @@ def mutation_name(webhook_id); end
4848
sig { abstract.returns(String) }
4949
def build_check_query; end
5050

51-
sig { abstract.params(body: T::Hash[String, T.untyped]).returns(T::Hash[Symbol, String]) }
51+
sig do
52+
abstract.params(body: T::Hash[String, T.untyped]).returns({
53+
webhook_id: T.nilable(String),
54+
current_address: T.nilable(String),
55+
fields: T::Array[String],
56+
metafield_namespaces: T::Array[String],
57+
})
58+
end
5259
def parse_check_result(body); end
5360

5461
sig { params(webhook_id: T.nilable(String)).returns(String) }

‎lib/shopify_api/webhooks/registrations/event_bridge.rb

+16-2
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ def build_check_query
3030
edges {
3131
node {
3232
id
33+
includeFields
34+
metafieldNamespaces
3335
endpoint {
3436
__typename
3537
... on WebhookEventBridgeEndpoint {
@@ -43,17 +45,29 @@ def build_check_query
4345
QUERY
4446
end
4547

46-
sig { override.params(body: T::Hash[String, T.untyped]).returns(T::Hash[Symbol, String]) }
48+
sig do
49+
override.params(body: T::Hash[String, T.untyped]).returns({
50+
webhook_id: T.nilable(String),
51+
current_address: T.nilable(String),
52+
fields: T::Array[String],
53+
metafield_namespaces: T::Array[String],
54+
})
55+
end
4756
def parse_check_result(body)
4857
edges = body.dig("data", "webhookSubscriptions", "edges") || {}
4958
webhook_id = nil
59+
fields = []
60+
metafield_namespaces = []
5061
current_address = nil
5162
unless edges.empty?
5263
node = edges[0]["node"]
5364
webhook_id = node["id"].to_s
5465
current_address = node["endpoint"]["arn"].to_s
66+
fields = node["includeFields"] || []
67+
metafield_namespaces = node["metafieldNamespaces"] || []
5568
end
56-
{ webhook_id: webhook_id, current_address: current_address }
69+
{ webhook_id: webhook_id, current_address: current_address, fields: fields,
70+
metafield_namespaces: metafield_namespaces, }
5771
end
5872
end
5973
end

‎lib/shopify_api/webhooks/registrations/http.rb

+16-2
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ def build_check_query
3636
edges {
3737
node {
3838
id
39+
includeFields
40+
metafieldNamespaces
3941
endpoint {
4042
__typename
4143
... on WebhookHttpEndpoint {
@@ -49,10 +51,19 @@ def build_check_query
4951
QUERY
5052
end
5153

52-
sig { override.params(body: T::Hash[String, T.untyped]).returns(T::Hash[Symbol, String]) }
54+
sig do
55+
override.params(body: T::Hash[String, T.untyped]).returns({
56+
webhook_id: T.nilable(String),
57+
current_address: T.nilable(String),
58+
fields: T::Array[String],
59+
metafield_namespaces: T::Array[String],
60+
})
61+
end
5362
def parse_check_result(body)
5463
edges = body.dig("data", "webhookSubscriptions", "edges") || {}
5564
webhook_id = nil
65+
fields = []
66+
metafield_namespaces = []
5667
current_address = nil
5768
unless edges.empty?
5869
node = edges[0]["node"]
@@ -63,8 +74,11 @@ def parse_check_result(body)
6374
else
6475
node["callbackUrl"].to_s
6576
end
77+
fields = node["includeFields"] || []
78+
metafield_namespaces = node["metafieldNamespaces"] || []
6679
end
67-
{ webhook_id: webhook_id, current_address: current_address }
80+
{ webhook_id: webhook_id, current_address: current_address, fields: fields,
81+
metafield_namespaces: metafield_namespaces, }
6882
end
6983
end
7084
end

‎lib/shopify_api/webhooks/registrations/pub_sub.rb

+18-3
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ def build_check_query
3434
edges {
3535
node {
3636
id
37+
includeFields
38+
metafieldNamespaces
3739
endpoint {
3840
__typename
3941
... on WebhookPubSubEndpoint {
@@ -48,17 +50,30 @@ def build_check_query
4850
QUERY
4951
end
5052

51-
sig { override.params(body: T::Hash[String, T.untyped]).returns(T::Hash[Symbol, String]) }
53+
sig do
54+
override.params(body: T::Hash[String, T.untyped]).returns({
55+
webhook_id: T.nilable(String),
56+
current_address: T.nilable(String),
57+
fields: T::Array[String],
58+
metafield_namespaces: T::Array[String],
59+
})
60+
end
5261
def parse_check_result(body)
5362
edges = body.dig("data", "webhookSubscriptions", "edges") || {}
5463
webhook_id = nil
64+
fields = []
65+
metafield_namespaces = []
5566
current_address = nil
5667
unless edges.empty?
5768
node = edges[0]["node"]
5869
webhook_id = node["id"].to_s
59-
current_address = "pubsub://#{node["endpoint"]["pubSubProject"]}:#{node["endpoint"]["pubSubTopic"]}"
70+
current_address =
71+
"pubsub://#{node["endpoint"]["pubSubProject"]}:#{node["endpoint"]["pubSubTopic"]}"
72+
fields = node["includeFields"] || []
73+
metafield_namespaces = node["metafieldNamespaces"] || []
6074
end
61-
{ webhook_id: webhook_id, current_address: current_address }
75+
{ webhook_id: webhook_id, current_address: current_address, fields: fields,
76+
metafield_namespaces: metafield_namespaces, }
6277
end
6378
end
6479
end

‎lib/shopify_api/webhooks/registry.rb

+7-1
Original file line numberDiff line numberDiff line change
@@ -219,8 +219,14 @@ def webhook_registration_needed?(client, registration)
219219
check_response = client.query(query: registration.build_check_query, response_as_struct: false)
220220
raise Errors::WebhookRegistrationError,
221221
"Failed to check if webhook was already registered" unless check_response.ok?
222+
222223
parsed_check_result = registration.parse_check_result(T.cast(check_response.body, T::Hash[String, T.untyped]))
223-
must_register = parsed_check_result[:current_address] != registration.callback_address
224+
registration_fields = registration.fields || []
225+
registration_metafield_namespaces = registration.metafield_namespaces || []
226+
227+
must_register = parsed_check_result[:current_address] != registration.callback_address ||
228+
parsed_check_result[:fields].sort != registration_fields.sort ||
229+
parsed_check_result[:metafield_namespaces].sort != registration_metafield_namespaces.sort
224230

225231
{ webhook_id: parsed_check_result[:webhook_id], must_register: must_register }
226232
end

‎test/webhooks/registry_test.rb

+182-279
Large diffs are not rendered by default.

‎test/webhooks/webhook_registration_queries.rb ‎test/webhooks/registry_test_queries.rb

+240-6
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@ def queries
1414
edges {
1515
node {
1616
id
17+
includeFields
18+
metafieldNamespaces
1719
endpoint {
1820
__typename
1921
... on WebhookHttpEndpoint {
@@ -36,13 +38,15 @@ def queries
3638
register_add_query:
3739
<<~QUERY,
3840
mutation webhookSubscription {
39-
webhookSubscriptionCreate(topic: SOME_TOPIC, webhookSubscription: {callbackUrl: "https://app-address.com/test-webhooks"}) {
41+
webhookSubscriptionCreate(topic: SOME_TOPIC, webhookSubscription: {callbackUrl: "https://app-address.com/test-webhooks", includeFields: ["field1", "field2"], metafieldNamespaces: ["namespace1", "namespace2"]}) {
4042
userErrors {
4143
field
4244
message
4345
}
4446
webhookSubscription {
4547
id
48+
includeFields
49+
metafieldNamespaces
4650
}
4751
}
4852
}
@@ -82,7 +86,11 @@ def queries
8286
"data" => {
8387
"webhookSubscriptionCreate" => {
8488
"userErrors" => [],
85-
"webhookSubscription" => { "id" => "gid://shopify/WebhookSubscription/12345" },
89+
"webhookSubscription" => {
90+
"id" => "gid://shopify/WebhookSubscription/12345",
91+
"includeFields" => ["field1", "field2"],
92+
"metafieldNamespaces" => ["namespace1", "namespace2"],
93+
},
8694
},
8795
},
8896
},
@@ -123,6 +131,23 @@ def queries
123131
},
124132
},
125133
},
134+
check_existing_response_with_attributes: {
135+
"data" => {
136+
"webhookSubscriptions" => {
137+
"edges" => [
138+
"node" => {
139+
"id" => "gid://shopify/WebhookSubscription/12345",
140+
"includeFields" => ["field1", "field2"],
141+
"metafieldNamespaces" => ["namespace1", "namespace2"],
142+
"endpoint" => {
143+
"typename" => "WebhookHttpEndpoint",
144+
"callbackUrl" => "https://app-address.com/test-webhooks",
145+
},
146+
},
147+
],
148+
},
149+
},
150+
},
126151
register_update_query:
127152
<<~QUERY,
128153
mutation webhookSubscription {
@@ -137,6 +162,36 @@ def queries
137162
}
138163
}
139164
QUERY
165+
register_update_query_with_fields:
166+
<<~QUERY,
167+
mutation webhookSubscription {
168+
webhookSubscriptionUpdate(id: "gid://shopify/WebhookSubscription/12345", webhookSubscription: {callbackUrl: "https://app-address.com/test-webhooks", includeFields: ["field1", "field2", "field3"]}) {
169+
userErrors {
170+
field
171+
message
172+
}
173+
webhookSubscription {
174+
id
175+
includeFields
176+
}
177+
}
178+
}
179+
QUERY
180+
register_update_query_with_metafield_namespaces:
181+
<<~QUERY,
182+
mutation webhookSubscription {
183+
webhookSubscriptionUpdate(id: "gid://shopify/WebhookSubscription/12345", webhookSubscription: {callbackUrl: "https://app-address.com/test-webhooks", metafieldNamespaces: ["namespace1", "namespace2", "namespace3"]}) {
184+
userErrors {
185+
field
186+
message
187+
}
188+
webhookSubscription {
189+
id
190+
metafieldNamespaces
191+
}
192+
}
193+
}
194+
QUERY
140195
register_update_response: {
141196
"data" => {
142197
"webhookSubscriptionUpdate" => {
@@ -145,6 +200,29 @@ def queries
145200
},
146201
},
147202
},
203+
204+
register_update_with_fields_response: {
205+
"data" => {
206+
"webhookSubscriptionUpdate" => {
207+
"userErrors" => [],
208+
"webhookSubscription" => {
209+
"id" => "gid://shopify/WebhookSubscription/12345",
210+
"includeFields" => ["field1", "field2", "field3"],
211+
},
212+
},
213+
},
214+
},
215+
register_update_with_metafield_namespaces_response: {
216+
"data" => {
217+
"webhookSubscriptionUpdate" => {
218+
"userErrors" => [],
219+
"webhookSubscription" => {
220+
"id" => "gid://shopify/WebhookSubscription/12345",
221+
"metafieldNamespaces" => ["namespace1", "namespace2", "namespace3"],
222+
},
223+
},
224+
},
225+
},
148226
},
149227
event_bridge: {
150228
check_query:
@@ -154,6 +232,8 @@ def queries
154232
edges {
155233
node {
156234
id
235+
includeFields
236+
metafieldNamespaces
157237
endpoint {
158238
__typename
159239
... on WebhookEventBridgeEndpoint {
@@ -175,13 +255,15 @@ def queries
175255
register_add_query:
176256
<<~QUERY,
177257
mutation webhookSubscription {
178-
eventBridgeWebhookSubscriptionCreate(topic: SOME_TOPIC, webhookSubscription: {arn: "test-webhooks"}) {
258+
eventBridgeWebhookSubscriptionCreate(topic: SOME_TOPIC, webhookSubscription: {arn: "test-webhooks", includeFields: ["field1", "field2"], metafieldNamespaces: ["namespace1", "namespace2"]}) {
179259
userErrors {
180260
field
181261
message
182262
}
183263
webhookSubscription {
184264
id
265+
includeFields
266+
metafieldNamespaces
185267
}
186268
}
187269
}
@@ -220,7 +302,11 @@ def queries
220302
"data" => {
221303
"eventBridgeWebhookSubscriptionCreate" => {
222304
"userErrors" => [],
223-
"webhookSubscription" => { "id" => "gid://shopify/WebhookSubscription/12345" },
305+
"webhookSubscription" => {
306+
"id" => "gid://shopify/WebhookSubscription/12345",
307+
"includeFields" => ["field1", "field2"],
308+
"metafieldNamespaces" => ["namespace1", "namespace2"],
309+
},
224310
},
225311
},
226312
},
@@ -261,6 +347,23 @@ def queries
261347
},
262348
},
263349
},
350+
check_existing_response_with_attributes: {
351+
"data" => {
352+
"webhookSubscriptions" => {
353+
"edges" => [
354+
"node" => {
355+
"id" => "gid://shopify/WebhookSubscription/12345",
356+
"endpoint" => {
357+
"typename" => "WebhookEventBridgeEndpoint",
358+
"arn" => "test-webhooks",
359+
},
360+
"includeFields" => ["field2", "field1"],
361+
"metafieldNamespaces" => ["namespace2", "namespace1"],
362+
},
363+
],
364+
},
365+
},
366+
},
264367
register_update_query:
265368
<<~QUERY,
266369
mutation webhookSubscription {
@@ -275,6 +378,36 @@ def queries
275378
}
276379
}
277380
QUERY
381+
register_update_query_with_fields:
382+
<<~QUERY,
383+
mutation webhookSubscription {
384+
eventBridgeWebhookSubscriptionUpdate(id: "gid://shopify/WebhookSubscription/12345", webhookSubscription: {arn: "test-webhooks", includeFields: ["field1", "field2", "field3"]}) {
385+
userErrors {
386+
field
387+
message
388+
}
389+
webhookSubscription {
390+
id
391+
includeFields
392+
}
393+
}
394+
}
395+
QUERY
396+
register_update_query_with_metafield_namespaces:
397+
<<~QUERY,
398+
mutation webhookSubscription {
399+
eventBridgeWebhookSubscriptionUpdate(id: "gid://shopify/WebhookSubscription/12345", webhookSubscription: {arn: "test-webhooks", metafieldNamespaces: ["namespace1", "namespace2", "namespace3"]}) {
400+
userErrors {
401+
field
402+
message
403+
}
404+
webhookSubscription {
405+
id
406+
metafieldNamespaces
407+
}
408+
}
409+
}
410+
QUERY
278411
register_update_response: {
279412
"data" => {
280413
"eventBridgeWebhookSubscriptionUpdate" => {
@@ -283,6 +416,28 @@ def queries
283416
},
284417
},
285418
},
419+
register_update_with_fields_response: {
420+
"data" => {
421+
"eventBridgeWebhookSubscriptionUpdate" => {
422+
"userErrors" => [],
423+
"webhookSubscription" => {
424+
"id" => "gid://shopify/WebhookSubscription/12345",
425+
"includeFields" => ["field1", "field2", "field3"],
426+
},
427+
},
428+
},
429+
},
430+
register_update_with_metafield_namespaces_response: {
431+
"data" => {
432+
"eventBridgeWebhookSubscriptionUpdate" => {
433+
"userErrors" => [],
434+
"webhookSubscription" => {
435+
"id" => "gid://shopify/WebhookSubscription/12345",
436+
"metafieldNamespaces" => ["namespace1", "namespace2", "namespace3"],
437+
},
438+
},
439+
},
440+
},
286441
},
287442
pub_sub: {
288443
check_query:
@@ -292,6 +447,8 @@ def queries
292447
edges {
293448
node {
294449
id
450+
includeFields
451+
metafieldNamespaces
295452
endpoint {
296453
__typename
297454
... on WebhookPubSubEndpoint {
@@ -314,13 +471,15 @@ def queries
314471
register_add_query:
315472
<<~QUERY,
316473
mutation webhookSubscription {
317-
pubSubWebhookSubscriptionCreate(topic: SOME_TOPIC, webhookSubscription: {pubSubProject: "my-project-id", pubSubTopic: "my-topic-id"}) {
474+
pubSubWebhookSubscriptionCreate(topic: SOME_TOPIC, webhookSubscription: {pubSubProject: "my-project-id", pubSubTopic: "my-topic-id", includeFields: ["field1", "field2"], metafieldNamespaces: ["namespace1", "namespace2"]}) {
318475
userErrors {
319476
field
320477
message
321478
}
322479
webhookSubscription {
323480
id
481+
includeFields
482+
metafieldNamespaces
324483
}
325484
}
326485
}
@@ -359,7 +518,11 @@ def queries
359518
"data" => {
360519
"pubSubWebhookSubscriptionCreate" => {
361520
"userErrors" => [],
362-
"webhookSubscription" => { "id" => "gid://shopify/WebhookSubscription/12345" },
521+
"webhookSubscription" => {
522+
"id" => "gid://shopify/WebhookSubscription/12345",
523+
"includeFields" => ["field1", "field2"],
524+
"metafieldNamespaces" => ["namespace1", "namespace2"],
525+
},
363526
},
364527
},
365528
},
@@ -401,6 +564,24 @@ def queries
401564
},
402565
},
403566
},
567+
check_existing_response_with_attributes: {
568+
"data" => {
569+
"webhookSubscriptions" => {
570+
"edges" => [
571+
"node" => {
572+
"id" => "gid://shopify/WebhookSubscription/12345",
573+
"endpoint" => {
574+
"typename" => "WebhookPubSubEndpoint",
575+
"pubSubProject" => "my-project-id",
576+
"pubSubTopic" => "my-topic-id",
577+
},
578+
"includeFields" => ["field1", "field2"],
579+
"metafieldNamespaces" => ["namespace1", "namespace2"],
580+
},
581+
],
582+
},
583+
},
584+
},
404585
register_update_query:
405586
<<~QUERY,
406587
mutation webhookSubscription {
@@ -415,6 +596,37 @@ def queries
415596
}
416597
}
417598
QUERY
599+
register_update_query_with_fields:
600+
<<~QUERY,
601+
mutation webhookSubscription {
602+
pubSubWebhookSubscriptionUpdate(id: "gid://shopify/WebhookSubscription/12345", webhookSubscription: {pubSubProject: "my-project-id", pubSubTopic: "my-topic-id", includeFields: ["field1", "field2", "field3"]}) {
603+
userErrors {
604+
field
605+
message
606+
}
607+
webhookSubscription {
608+
id
609+
includeFields
610+
}
611+
}
612+
}
613+
QUERY
614+
register_update_query_with_metafield_namespaces:
615+
<<~QUERY,
616+
mutation webhookSubscription {
617+
pubSubWebhookSubscriptionUpdate(id: "gid://shopify/WebhookSubscription/12345", webhookSubscription: {pubSubProject: "my-project-id", pubSubTopic: "my-topic-id", metafieldNamespaces: ["namespace1", "namespace2", "namespace3"]}) {
618+
userErrors {
619+
field
620+
message
621+
}
622+
webhookSubscription {
623+
id
624+
metafieldNamespaces
625+
}
626+
}
627+
}
628+
QUERY
629+
418630
register_update_response: {
419631
"data" => {
420632
"pubSubWebhookSubscriptionUpdate" => {
@@ -423,6 +635,28 @@ def queries
423635
},
424636
},
425637
},
638+
register_update_with_fields_response: {
639+
"data" => {
640+
"pubSubWebhookSubscriptionUpdate" => {
641+
"userErrors" => [],
642+
"webhookSubscription" => {
643+
"id" => "gid://shopify/WebhookSubscription/12345",
644+
"includeFields" => ["field1", "field2", "field3"],
645+
},
646+
},
647+
},
648+
},
649+
register_update_with_metafield_namespaces_response: {
650+
"data" => {
651+
"pubSubWebhookSubscriptionUpdate" => {
652+
"userErrors" => [],
653+
"webhookSubscription" => {
654+
"id" => "gid://shopify/WebhookSubscription/12345",
655+
"metafieldNamespaces" => ["namespace1", "namespace2", "namespace3"],
656+
},
657+
},
658+
},
659+
},
426660
},
427661
fetch_id_query:
428662
<<~QUERY,

0 commit comments

Comments
 (0)
Please sign in to comment.