Adds attributes comparator and test case #1282
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sle-c
commented
Feb 22, 2024
Comment on lines
+409
to
+423
| 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 |
There was a problem hiding this comment.
this makes the assumption that the primary key and anything required path params might also be required in the body payload
There was a problem hiding this comment.
That sounds like a reasonable assumption to me.
|
Checking against the 2024-01 Open API spec files all the put/patch operations list their url params as required param in the body payload |
sle-c
commented
Feb 22, 2024
Comment on lines
+49
to
+50
| if has_numbered_key && ref_value.is_a?(Array) | ||
| new_hash[key] = ref_value |
There was a problem hiding this comment.
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"] }There was a problem hiding this comment.
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.
sle-c
commented
Feb 22, 2024
| 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? |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
paulomarg
reviewed
Feb 22, 2024
Comment on lines
+409
to
+423
| 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 |
There was a problem hiding this comment.
That sounds like a reasonable assumption to me.
paulomarg
approved these changes
Feb 23, 2024
lizkenyon
approved these changes
Feb 23, 2024
There was a problem hiding this comment.
Thank you for tackling this bug! The community will appreciate this fix! 🎉
I was thinking we could add a bit of explicit information into the docs on the usage of REST resources about this.
Explaining that only changed attributes + required URL params are sent when saving.
We could add a information about explicitly setting to null values.
This could help devs debug in the future if there are some cases this solution doesn't cover perfectly.
|
Doc updated 
|
paulomarg
reviewed
Feb 23, 2024
Co-authored-by: Paulo Margarido <64600052+paulomarg@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #1203
Fixes #1164
How has this been tested?
Checklist: