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.

Sign up for GitHub

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

Adds attributes comparator and test case #1282

Merged
merged 12 commits into from
Feb 23, 2024
Merged

Adds attributes comparator and test case #1282

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a83923b
Adds attributes comparator and test case
sle-c Feb 22, 2024
9004239
Update some test cases to cover more bases
sle-c Feb 22, 2024
8ad3986
Removes byebug
sle-c Feb 22, 2024
0971d72
Moves hash_diff to comparator
sle-c Feb 22, 2024
bfbb2d4
Fixes resource base tests
sle-c Feb 22, 2024
910e33a
Adds changelog entry
sle-c Feb 22, 2024
c0621c7
Changes `unless` condition to `if` and add some comments to explain
sle-c Feb 23, 2024
327a819
Updates rest.md to explain how updating resource works
sle-c Feb 23, 2024
930e582
Minor content format
sle-c Feb 23, 2024
a09db36
Fixes typo
sle-c Feb 23, 2024
221f06e
Update docs/usage/rest.md
sle-c Feb 23, 2024
938aece
Updates rest.md to omit implementation details
sle-c Feb 23, 2024
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
Note: For changes to the API, see https://shopify.dev/changelog?filter=api

## Unreleased
- [#1282](https://github.com/Shopify/shopify-api-ruby/pull/1282) Fixes a bug where diffing attributes to update not take into account of Array changes and required ids.
- [#1254](https://github.com/Shopify/shopify-api-ruby/pull/1254) Introduce token exchange API for fetching access tokens. This feature is currently unstable and cannot be used yet.
- [#1268](https://github.com/Shopify/shopify-api-ruby/pull/1268) Add [new webhook handler interface](https://github.com/Shopify/shopify-api-ruby/blob/main/docs/usage/webhooks.md#create-a-webhook-handler) to provide `webhook_id ` and `api_version` information to webhook handlers.
- [#1274](https://github.com/Shopify/shopify-api-ruby/pull/1274) Update sorbet and rbi dependencies. Remove support for ruby 2.7.
Expand Down
67 changes: 55 additions & 12 deletions lib/shopify_api/rest/base.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
# frozen_string_literal: true

require "active_support/inflector"
require "hash_diff"

module ShopifyAPI
module Rest
Expand Down Expand Up @@ -187,6 +186,21 @@ def get_path(http_method:, operation:, entity: nil, ids: {})
custom_prefix ? "#{T.must(custom_prefix).sub(%r{\A/}, "")}/#{match}" : match
end

sig do
params(
http_method: Symbol,
operation: Symbol,
).returns(T.nilable(T::Array[Symbol]))
end
def get_path_ids(http_method:, operation:)
found_path = @paths.find do |path|
http_method == path[:http_method] && operation == path[:operation]
end
return nil if found_path.nil?

T.cast(found_path[:ids], T::Array[Symbol])
end

sig do
params(
http_method: Symbol,
Expand Down Expand Up @@ -355,9 +369,14 @@ def save!
sig { params(update_object: T::Boolean).void }
def save(update_object: false)
method = deduce_write_verb

body = {
self.class.json_body_name => attributes_to_update.merge(build_required_attributes(http_method: method)),
}

response = @client.public_send(
method,
body: { self.class.json_body_name => attributes_to_update },
body: body,
path: deduce_write_path(method),
)

Expand All @@ -374,22 +393,34 @@ def save(update_object: false)

sig { returns(T::Hash[String, String]) }
def attributes_to_update
original_state_for_update = original_state.reject do |attribute, _|
updatable_attributes = original_state.reject do |attribute, _|
self.class.read_only_attributes&.include?("@#{attribute}".to_sym)
end

diff = HashDiff::Comparison.new(
deep_stringify_keys(original_state_for_update),
deep_stringify_keys(to_hash(true)),
).left_diff
stringified_updatable_attributes = deep_stringify_keys(updatable_attributes)
stringified_new_attributes = deep_stringify_keys(to_hash(true))
ShopifyAPI::Utils::AttributesComparator.compare(
stringified_updatable_attributes,
stringified_new_attributes,
)
end

diff.each do |attribute, value|
if value.is_a?(Hash) && value[0] == HashDiff::NO_VALUE
diff[attribute] = send(attribute)
end
sig { params(http_method: Symbol).returns(T::Hash[String, T.untyped]) }
def build_required_attributes(http_method:)
required_attributes = {}

primary_key_value = send(self.class.primary_key)
unless primary_key_value.nil?
required_attributes[self.class.primary_key] = primary_key_value
end

diff
path_ids = deduce_path_ids(http_method)
path_ids&.each do |path_id|
path_id_value = send(path_id)
required_attributes[path_id.to_s] = path_id_value unless path_id_value.nil?
end

required_attributes
Comment on lines +409 to +423
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this makes the assumption that the primary key and anything required path params might also be required in the body payload

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds like a reasonable assumption to me.

end

sig { returns(Symbol) }
Expand All @@ -409,6 +440,18 @@ def deduce_write_path(method)
path
end

sig { params(method: Symbol).returns(T.nilable(T::Array[Symbol])) }
def deduce_path_ids(method)
path_ids = self.class.get_path_ids(http_method: method, operation: method)

if path_ids.nil?
method = method == :post ? :put : :post
path_ids = self.class.get_path_ids(http_method: method, operation: method)
end

path_ids
end

sig { params(hash: T::Hash[T.any(String, Symbol), T.untyped]).returns(T::Hash[String, String]) }
def deep_stringify_keys(hash)
hash.each_with_object({}) do |(key, value), result|
Expand Down
66 changes: 66 additions & 0 deletions lib/shopify_api/utils/attributes_comparator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
# typed: strict
# frozen_string_literal: true

require "hash_diff"

module ShopifyAPI
module Utils
module AttributesComparator
class << self
extend T::Sig

sig do
params(
original_attributes: T::Hash[String, T.untyped],
updated_attributes: T::Hash[String, T.untyped],
).returns(T::Hash[String, T.untyped])
end
def compare(original_attributes, updated_attributes)
attributes_diff = HashDiff::Comparison.new(
original_attributes,
updated_attributes,
).left_diff

update_value = build_update_value(
attributes_diff,
reference_values: updated_attributes,
)

update_value
end

sig do
params(
diff: T::Hash[String, T.untyped],
path: T::Array[String],
reference_values: T::Hash[String, T.untyped],
).returns(T::Hash[String, T.untyped])
end
def build_update_value(diff, path: [], reference_values: {})
new_hash = {}

diff.each do |key, value|
current_path = path + [key.to_s]

if value.is_a?(Hash)
has_numbered_key = value.keys.any? { |k| k.is_a?(Integer) }
ref_value = T.unsafe(reference_values).dig(*current_path)

if has_numbered_key && ref_value.is_a?(Array)
new_hash[key] = ref_value
Comment on lines +49 to +50
Copy link
Contributor Author

Choose a reason for hiding this comment

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

array diff will have the following format

original = { "a" => [] }
updated = { "a" => ["foo"] }

diff = { "a" => { 0 => "foo" } }

However, we don't want to submit the diff values, we want to submit the array so these 2 lines just take the updated value

has_numbered_key = true # because of `0` key
ref_value = ["foo"] # which is_a Array

resulting_hash = { "a" => ["foo"] }

Copy link
Contributor Author

@sle-c sle-c Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i don't dig deeper into this because when assigning a resource's array, we either pass in a completely new array, adding to it, modify an existing item in the array or removing an existing item. Then it's better to just use the entire updated array to send to Shopify.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed!

else
new_value = build_update_value(value, path: current_path, reference_values: reference_values)

new_hash[key] = new_value unless new_value.empty? && !ref_value.empty?
Copy link
Contributor Author

@sle-c sle-c Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

this is for cases where an object is explicitly emptied. For example

    draft_order = ShopifyAPI::DraftOrder.find(session: @test_session, id: 1143974756657)

    draft_order.shipping_address =  {}
    draft_order.save!

if there was a shipping address in the draft order, this should remove that shipping address from the draft order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in an opposite case, where we only update the address partially but accidentally use the same value for the same attribute, this helps clear out the entire diff because there's nothing to update. It's an edge case. For example

original = {
  shipping_address: {
    address1: "foo",
    address2: "bar"
  }
}

updated = {
  shipping_address: {
    address2: "bar"
  }
}

diff = {
  shipping_address: {
    address1: HashDiff::NO_VALUE # hash_diff thinks we want to remove `address1`
  } # address2 doesn't show up because it's not different
}

update_value = {} # should be nothing to update

sle-c marked this conversation as resolved.
Show resolved Hide resolved
end
elsif value != HashDiff::NO_VALUE
new_hash[key] = value
end
end

new_hash
end
end
end
end
end
14 changes: 8 additions & 6 deletions test/clients/base_rest_resource_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ def test_saves_removing_children
body = draft_order_data.dup
body["line_items"] = []
stubbed_request = stub_request(:put, "#{@prefix}/draft_orders/#{draft_order_data.dig("id")}.json")
.with(body: hash_including("draft_order": { line_items: [] }))
.with(body: hash_including("draft_order": { line_items: [], id: 1124601987358 }))
.to_return(status: 200)

draft_order.line_items = []
Expand Down Expand Up @@ -416,15 +416,17 @@ def test_put_requests_only_modify_changed_attributes
headers: { "X-Shopify-Access-Token" => "this_is_a_test_token", "Accept" => "application/json",
"Content-Type" => "application/json", },
body: { "product" => hash_including({ "metafields" => [{ "key" => "new", "value" => "newvalue",
"type" => "single_line_text_field", "namespace" => "global", }] }) },
"type" => "single_line_text_field", "namespace" => "global", }],
"id" => 632910392, }) },
)
.to_return(status: 200, body: JSON.generate({ "product" => { "id" => 632910392, "title" => "IPod Nano - 8GB",
"body_html" => "<p>It's the small iPod with one very big idea: Video. Now the world's most popular music player, available in 4GB and 8GB models, lets you enjoy TV shows, movies, video podcasts, and more. The larger, brighter display means amazing picture quality. In six eye-catching colors, iPod nano is stunning all around. And with models starting at just $149, little speaks volumes.</p>", "vendor" => "Apple", "product_type" => "Cult Products", "created_at" => "2023-02-02T09:09:49-05:00", "handle" => "ipod-nano", "updated_at" => "2023-02-02T09:28:31-05:00", "published_at" => "2007-12-31T19:00:00-05:00", "template_suffix" => nil, "status" => "active", "published_scope" => "web", "tags" => "Emotive, Flash Memory, MP3, Music", "admin_graphql_api_id" => "gid://shopify/Product/632910392", "variants" => [{ "id" => 808950810, "product_id" => 632910392, "title" => "Pink", "price" => "199.00", "sku" => "IPOD2008PINK", "position" => 1, "inventory_policy" => "continue", "compare_at_price" => nil, "fulfillment_service" => "manual", "inventory_management" => "shopify", "option1" => "Pink", "option2" => nil, "option3" => nil, "created_at" => "2023-02-02T09:09:49-05:00", "updated_at" => "2023-02-02T09:09:49-05:00", "taxable" => true, "barcode" => "1234_pink", "grams" => 567, "image_id" => 562641783, "weight" => 1.25, "weight_unit" => "lb", "inventory_item_id" => 808950810, "inventory_quantity" => 10, "old_inventory_quantity" => 10, "presentment_prices" => [{ "price" => { "amount" => "199.00", "currency_code" => "USD" }, "compare_at_price" => nil }], "requires_shipping" => true, "admin_graphql_api_id" => "gid://shopify/ProductVariant/808950810" }, { "id" => 49148385, "product_id" => 632910392, "title" => "Red", "price" => "199.00", "sku" => "IPOD2008RED", "position" => 2, "inventory_policy" => "continue", "compare_at_price" => nil, "fulfillment_service" => "manual", "inventory_management" => "shopify", "option1" => "Red", "option2" => nil, "option3" => nil, "created_at" => "2023-02-02T09:09:49-05:00", "updated_at" => "2023-02-02T09:09:49-05:00", "taxable" => true, "barcode" => "1234_red", "grams" => 567, "image_id" => nil, "weight" => 1.25, "weight_unit" => "lb", "inventory_item_id" => 49148385, "inventory_quantity" => 20, "old_inventory_quantity" => 20, "presentment_prices" => [{ "price" => { "amount" => "199.00", "currency_code" => "USD" }, "compare_at_price" => nil }], "requires_shipping" => true, "admin_graphql_api_id" => "gid://shopify/ProductVariant/49148385" }, { "id" => 39072856, "product_id" => 632910392, "title" => "Green", "price" => "199.00", "sku" => "IPOD2008GREEN", "position" => 3, "inventory_policy" => "continue", "compare_at_price" => nil, "fulfillment_service" => "manual", "inventory_management" => "shopify", "option1" => "Green", "option2" => nil, "option3" => nil, "created_at" => "2023-02-02T09:09:49-05:00", "updated_at" => "2023-02-02T09:09:49-05:00", "taxable" => true, "barcode" => "1234_green", "grams" => 567, "image_id" => nil, "weight" => 1.25, "weight_unit" => "lb", "inventory_item_id" => 39072856, "inventory_quantity" => 30, "old_inventory_quantity" => 30, "presentment_prices" => [{ "price" => { "amount" => "199.00", "currency_code" => "USD" }, "compare_at_price" => nil }], "requires_shipping" => true, "admin_graphql_api_id" => "gid://shopify/ProductVariant/39072856" }, { "id" => 457924702, "product_id" => 632910392, "title" => "Black", "price" => "199.00", "sku" => "IPOD2008BLACK", "position" => 4, "inventory_policy" => "continue", "compare_at_price" => nil, "fulfillment_service" => "manual", "inventory_management" => "shopify", "option1" => "Black", "option2" => nil, "option3" => nil, "created_at" => "2023-02-02T09:09:49-05:00", "updated_at" => "2023-02-02T09:09:49-05:00", "taxable" => true, "barcode" => "1234_black", "grams" => 567, "image_id" => nil, "weight" => 1.25, "weight_unit" => "lb", "inventory_item_id" => 457924702, "inventory_quantity" => 40, "old_inventory_quantity" => 40, "presentment_prices" => [{ "price" => { "amount" => "199.00", "currency_code" => "USD" }, "compare_at_price" => nil }], "requires_shipping" => true, "admin_graphql_api_id" => "gid://shopify/ProductVariant/457924702" }], "options" => [{ "id" => 594680422, "product_id" => 632910392, "name" => "Color", "position" => 1, "values" => ["Pink", "Red", "Green", "Black"] }], "images" => [{ "id" => 850703190, "product_id" => 632910392, "position" => 1, "created_at" => "2023-02-02T09:09:49-05:00", "updated_at" => "2023-02-02T09:09:49-05:00", "alt" => nil, "width" => 123, "height" => 456, "src" => "https://cdn.shopify.com/s/files/1/0005/4838/0009/products/ipod-nano.png?v=1675346989", "variant_ids" => [], "admin_graphql_api_id" => "gid://shopify/ProductImage/850703190" }, { "id" => 562641783, "product_id" => 632910392, "position" => 2, "created_at" => "2023-02-02T09:09:49-05:00", "updated_at" => "2023-02-02T09:09:49-05:00", "alt" => nil, "width" => 123, "height" => 456, "src" => "https://cdn.shopify.com/s/files/1/0005/4838/0009/products/ipod-nano-2.png?v=1675346989", "variant_ids" => [808950810], "admin_graphql_api_id" => "gid://shopify/ProductImage/562641783" }, { "id" => 378407906, "product_id" => 632910392, "position" => 3, "created_at" => "2023-02-02T09:09:49-05:00", "updated_at" => "2023-02-02T09:09:49-05:00", "alt" => nil, "width" => 123, "height" => 456, "src" => "https://cdn.shopify.com/s/files/1/0005/4838/0009/products/ipod-nano.png?v=1675346989", "variant_ids" => [], "admin_graphql_api_id" => "gid://shopify/ProductImage/378407906" }], "image" => { "id" => 850703190, "product_id" => 632910392, "position" => 1, "created_at" => "2023-02-02T09:09:49-05:00", "updated_at" => "2023-02-02T09:09:49-05:00", "alt" => nil, "width" => 123, "height" => 456, "src" => "https://cdn.shopify.com/s/files/1/0005/4838/0009/products/ipod-nano.png?v=1675346989", "variant_ids" => [], "admin_graphql_api_id" => "gid://shopify/ProductImage/850703190" }, } }), headers: {})

product = ShopifyAPI::Product.find(id: 632910392, session: @session)

product.client.expects(:put).with(
body: { "product" => { "metafields" => [{ "key" => "new", "value" => "newvalue", "type" => "single_line_text_field",
"namespace" => "global", }] } },
"namespace" => "global", }], "id" => 632910392, } },
path: "products/632910392.json",
)
product.metafields = [
Expand All @@ -438,7 +440,7 @@ def test_put_requests_only_modify_changed_attributes
product.save
end

def test_put_request_for_has_one_associaiton_works
def test_put_request_for_has_one_association_works
stub_request(:get, "https://test-shop.myshopify.com/admin/api/#{ShopifyAPI::Context.api_version}/customers/207119551.json")
.to_return(status: 200, body: JSON.generate({ "customer" => { "id" => 207119551,
"email" => "[email protected]", "accepts_marketing" => false, "created_at" => "2023-02-02T09:42:27-05:00", "updated_at" => "2023-02-02T09:42:27-05:00", "first_name" => "Bob", "last_name" => "Norman", "orders_count" => 1, "state" => "disabled", "total_spent" => "199.65", "last_order_id" => 450789469, "note" => nil, "verified_email" => true, "multipass_identifier" => nil, "tax_exempt" => false, "tags" => "L\u00E9on, No\u00EBl", "last_order_name" => "#1001", "currency" => "USD", "phone" => "+16136120707", "addresses" => [{ "id" => 207119551, "customer_id" => 207119551, "first_name" => nil, "last_name" => nil, "company" => nil, "address1" => "Chestnut Street 92", "address2" => "", "city" => "Louisville", "province" => "Kentucky", "country" => "United States", "zip" => "40202", "phone" => "555-625-1199", "name" => "", "province_code" => "KY", "country_code" => "US", "country_name" => "United States", "default" => true }], "accepts_marketing_updated_at" => "2005-06-12T11:57:11-04:00", "marketing_opt_in_level" => nil, "tax_exemptions" => [], "email_marketing_consent" => { "state" => "not_subscribed", "opt_in_level" => nil, "consent_updated_at" => "2004-06-13T11:57:11-04:00" }, "sms_marketing_consent" => { "state" => "not_subscribed", "opt_in_level" => "single_opt_in", "consent_updated_at" => "2023-02-02T09:42:27-05:00", "consent_collected_from" => "OTHER" }, "admin_graphql_api_id" => "gid://shopify/Customer/207119551", "default_address" => { "id" => 207119551, "customer_id" => 207119551, "first_name" => nil, "last_name" => nil, "company" => nil, "address1" => "Chestnut Street 92", "address2" => "", "city" => "Louisville", "province" => "Kentucky", "country" => "United States", "zip" => "40202", "phone" => "555-625-1199", "name" => "", "province_code" => "KY", "country_code" => "US", "country_name" => "United States", "default" => true }, } }), headers: {})
Expand All @@ -448,7 +450,7 @@ def test_put_request_for_has_one_associaiton_works
session: @session,
)
customer.client.expects(:put).with(
body: { "customer" => { "tags" => "New Customer, Repeat Customer" } },
body: { "customer" => { "tags" => "New Customer, Repeat Customer", "id" => 207119551 } },
path: "customers/207119551.json",
)
customer.tags = "New Customer, Repeat Customer"
Expand Down Expand Up @@ -496,7 +498,7 @@ def test_put_requests_for_resource_with_read_only_attributes

variant = ShopifyAPI::Variant.find(id: 169, session: @session)
variant.client.expects(:put).with(
body: { "variant" => { "barcode" => "1234" } },
body: { "variant" => { "barcode" => "1234", "id" => 169 } },
path: "variants/169.json",
)
variant.barcode = "1234"
Expand Down
Loading
Loading