stefankroes / ancestry

Organise ActiveRecord model into a tree structure
MIT License
3.72k stars 458 forks source link

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

Open astyagun opened 5 years ago

astyagun commented 5 years ago

Given:

The problem occurs, for example, when:

Then:

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

kbrock commented 5 years ago

@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 commented 5 years ago

@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 commented 5 years ago
  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 commented 5 years ago

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 commented 5 years ago

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.

astyagun commented 5 years ago

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