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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting #parent_id with nested attributes can result in corrupted #ancestry #423

Open
astyagun opened this issue Nov 27, 2018 · 6 comments
Assignees

Comments

@astyagun
Copy link

Given:

  • Tree model
  • Node model
  • Tree has_many :nodes
  • Tree accepts_nested_attributes_for :nodes
  • Node has_ancestry
  • Nested attributes for nodes modify nodes' ancestry using #parent_id attributes

The problem occurs, for example, when:

  • A tree has 3 sibling nodes (let's assume, that they have #id attributes respectively '1', '2' and '3')
  • And user nests node two under node one and node three under node two resulting in two-level nesting

Then:

  • Each node is assigned a new #parent_id attribute
  • When assigning to child node, parent node is loaded from DB and its #ancestry attribute is used to assign #ancestry of a child node.
    • Node two gets assigned #ancestry of #id of node one, '1', which is correct
    • But node three gets assigned #ancestry of #id of node two only, '2'. Which is incorrect as it should have IDs of all chain of parent nodes in its #ancestry attribute. What it should be assigned is '1/2'.
  • All nodes are saved to DB

And this results in corrupted #ancestry attribute of node three and it disappears from the nodes tree.

@kbrock
Copy link
Collaborator

kbrock commented Nov 27, 2018

@astyagun Have you created a test for this?

node1.parent_id = node2.id
node2.parent_id = node3.id
node1.save
node2.save
  1. Using parent instead of parent_id fixes this.
  2. Save order fixes this. i.e.: node2.save ; node1.save

Wonder if there is a way to enforce a good save order.

@astyagun
Copy link
Author

@kbrock To reproduce the problem a test like this could be used:

  describe '#update nodes using nested attributes' do
    subject(:update) { instance.update nodes_attributes: nodes_attributes }

    let(:instance) { create :tree }

    context 'when nesting nodes several levels deep' do
      let!(:node_one) { create :node, tree: instance }
      let!(:node_two) { create :node, tree: instance }
      let!(:node_three) { create :node, tree: instance }
      let :nodes_attributes do
        # Order of hashes is significant
        [
          { id: node_two.id, parent_id: node_one.id },
          { id: node_three.id, parent_id: node_two.id }
        ]
      end

      it 'saves #ancestry of the most nested node correctly' do
        update
        expect(node_three.reload.ancestry).to eq "#{node_one.id}/#{node_two.id}"
      end
    end
  end

@astyagun
Copy link
Author

  1. Using parent instead of parent_id fixes this.
  2. Save order fixes this. i.e.: node2.save ; node1.save

Reordering works indeed, I've tried it in the test above. But using parent instead of parent_id didn't work.

@astyagun
Copy link
Author

More about #parent attribute. To ensure, that instances, that receive attributes, are the same as instances, that are being assigned to #parent attributes, changed the test this way:

describe '#update nodes using nested attributes' do
  subject(:update) { instance.update nodes_attributes: nodes_attributes }

  let(:instance) { create :tree }

  context 'when nesting nodes several levels deep' do
    let!(:node_one) { create :node, tree: instance }
    let!(:node_two) { create :node, tree: instance }
    let!(:node_three) { create :node, tree: instance }
    let(:tree_nodes_association) { instance.association :nodes }
    let(:tree_nodes) { tree_nodes_association.target }
    let(:node_one_from_association) { tree_nodes.detect { |node| node.id == node_one.id } }
    let(:node_two_from_association) { tree_nodes.detect { |node| node.id == node_two.id } }
    let :nodes_attributes do
      # Order of hashes is significant
      [
        { id: node_two.id, parent: node_one_from_association },
        { id: node_three.id, parent: node_two_from_association }
      ]
    end

    before { tree_nodes_association.load_target }

    it 'saves ancestry of the most nested node correctly' do
      update
      expect(node_three.reload.ancestry).to eq "#{node_one.id}/#{node_two.id}"
    end
  end
end

No luck, still fails. This is not the way to get same instances everywhere, probably.

@kbrock
Copy link
Collaborator

kbrock commented Dec 17, 2018

As I'm sure you already know,

The problem is when you set a parent_id into the nodes, it needs to lookup the parent record so it can properly determine the path.

There are then 2 copies of the record in memory, and when you update one, the other is wrong / stale.

A working algorithm would be to set the parent nodes instead of the parent_id nodes.
Nested attributes will be a problem with this.

There may be a way to change ancestry to delay the lookup of the parent record from the parent_id but I can't think of how to do this. And I don't even know if this is a good solution.

@kbrock kbrock self-assigned this Dec 17, 2018
@astyagun
Copy link
Author

astyagun commented Jan 3, 2019

Another option is to sort nodes before they are submitted into update as nested attributes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants