Shopify / Shopify/shopify-api-ruby

Hashdiff does not see change if nested object is updated in place due to same reference value in resource object and original_state

Open
#1,331 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
Ruby
Stars
1.1k
Forks
484
PR merge metrics
No merged PRs in 30d

Description

Issue summary

HashDiff does not see change if nested resource object is updated in place. When resource object is created with #create_instance method, it adds value to both resource and to the original_state. If you update the object in place, the change will be reflected in original_state aswell which is wrong.

Expected behavior

All values in original_state should be duplicates, not references.

Steps to reproduce the problem

  1. Install gems gem install shopify_api rspec
  2. Create ruby script with the content below
  3. Execute script with rspec bundle exec rspec ${script_file_name}
it "should reproduce issue with reference value in original_state" do
    resource_hash = {
        name: "#111",
        line_items: [
          { title: "One", price: 100, quantity: 1 },
        ],
        shipping_address: {
          first_name: "John",
          last_name: "Doe",
          address_1: "Freedom street",
        },
    }.values_as_hash

    draft_order = ShopifyAPI::DraftOrder.new(from_hash: resource_hash)
    draft_order.save!
    expect(draft_order.shipping_address["first_name"]).to eq("John")

    # This changes original_state which is wrong
    draft_order.shipping_address["first_name"] = "Tom"
    draft_order.save!

    expect(draft_order.shipping_address["first_name"]).to eq("Tom")

    # It is only possible to update it by setting whole object like this
    updated_address = draft_order.shipping_address.values_as_hash
    updated_address[:first_name] = "Tom"
    draft_order.shipping_address = updated_address
    draft_order.save!

    expect(draft_order.shipping_address["first_name"]).to eq("Tom")
end

This is our current workaround to fix this issue.

module ShopifyAPI
  module Rest
    module BaseExtension

      def create_instance(data:, session:, instance: nil)
        result = super
        result.original_state = result.original_state.deep_dup

        result
      end

    end
  end
end

ShopifyAPI::Rest::Base.singleton_class.prepend(ShopifyAPI::Rest::BaseExtension)

This is where the actual issue is happening:
https://github.com/Shopify/shopify-api-ruby/blob/6a74e1e2a1ec454d700e3b53d91cae00c236fada/lib/shopify_api/rest/base.rb#L293-L296

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at lib/shopify_api/rest/base.rb lines 293-296, where create_instance builds the resource and original_state. Run the reproduction with bundle exec rspec, then verify that mutating a nested value in place does not alter original_state and is detected on save.

Written by the indexing model from the issue text.

Assessment

Tech stack
ruby
Domain
api, backend
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.