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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dirty attribute changes on update #1149

Merged
merged 7 commits into from
May 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,6 @@ Layout/EmptyLineAfterGuardClause:
Enabled: true
Style/GlobalStdStream:
Enabled: true
Layout/LineLength:
Exclude:
- "test/clients/*"
60 changes: 47 additions & 13 deletions lib/shopify_api/rest/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -234,16 +234,20 @@ def create_instance(data:, session:, instance: nil)
attr_sym = attribute.to_sym

if has_many?(attr_sym) && value
instance.original_state[attr_sym] = []
attr_list = []
value.each do |element|
attr_list << T.unsafe(@has_many[attr_sym]).create_instance(data: element, session: session)
child = T.unsafe(@has_many[attr_sym]).create_instance(data: element, session: session)
attr_list << child
instance.original_state[attr_sym] << child.to_hash(true)
end
instance.public_send("#{attribute}=", attr_list)
elsif has_one?(attr_sym) && value
# force a hash if core returns values that instantiate objects like "USD"
data_hash = value.is_a?(Hash) ? value : { attribute.to_s => value }
instance.public_send("#{attribute}=",
T.unsafe(@has_one[attr_sym]).create_instance(data: data_hash, session: session))
child = T.unsafe(@has_one[attr_sym]).create_instance(data: data_hash, session: session)
instance.public_send("#{attribute}=", child)
instance.original_state[attr_sym] = child.to_hash(true)
Copy link

@DuncanUszkay1 DuncanUszkay1 Jun 6, 2023

Choose a reason for hiding this comment

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

This was a breaking change for our application that went unreported in the changelog. We were creating an instance and immediately calling save on it, which with this change resulted in an empty hash being sent to Shopify (since the original state matched the current state after instantiation)

I.E. ShopifyAPI::ObjectName.build(attrs).save! <- this fails now AFAICT

Copy link

@DuncanUszkay1 DuncanUszkay1 Jun 6, 2023

Choose a reason for hiding this comment

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

Not sure if we were using the library improperly since I didn't see mention of this is the docs, but if not I can go update the changelog for others who might run into this. cc @nelsonwittwer

else
instance.public_send("#{attribute}=", value)
instance.original_state[attr_sym] = value
Expand Down Expand Up @@ -335,16 +339,12 @@ def save!

sig { params(update_object: T::Boolean).void }
def save(update_object: false)
hash = HashDiff::Comparison.new(original_state, to_hash(true)).left_diff
method = hash[self.class.primary_key] ? :put : :post

path = self.class.get_path(http_method: method, operation: method, entity: self)
if path.nil?
method = method == :post ? :put : :post
path = self.class.get_path(http_method: method, operation: method, entity: self)
end

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

if update_object
self.class.create_instance(
Expand All @@ -359,6 +359,40 @@ def save(update_object: false)

private

sig { returns(T::Hash[String, String]) }
def attributes_to_update
HashDiff::Comparison.new(
deep_stringify_keys(original_state),
deep_stringify_keys(to_hash(true)),
).left_diff
end

sig { returns(Symbol) }
def deduce_write_verb
send(self.class.primary_key) ? :put : :post
end

sig { params(method: Symbol).returns(T.nilable(String)) }
def deduce_write_path(method)
path = self.class.get_path(http_method: method, operation: method, entity: self)

if path.nil?
method = method == :post ? :put : :post
path = self.class.get_path(http_method: method, operation: method, entity: self)
end

path
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|
new_key = key.to_s
new_value = value.is_a?(Hash) ? deep_stringify_keys(value) : value
result[new_key] = new_value
end
end

sig { params(key: T.any(String, Symbol), val: T.untyped).void }
def set_property(key, val)
# Some API fields contain invalid characters, like `?`, which causes issues when setting them as instance
Expand Down
Loading
Loading