Issue when reordering within scopes

[fabn]

I'm using this gem in conjunction with ancestry gem in the following model:

class Category < ActiveRecord::Base
  has_ancestry orphan_strategy: :adopt, cache_depth: true
  acts_as_list scope: [:ancestry]
end

However I found an edge case with this usage (suggested by ancestry wiki)

Given that every list is scoped to the ancestry column (i.e. siblings nodes are in the same list), when I move a node with children, children list is reordered in a wrong way. Here is a sample tree

Category 1 (id: 1, position: 1, depth: 0, ancestry: )
├── Category 2 (id: 2, position: 1, depth: 1, ancestry: 1)
│   ├── Category 3 (id: 3, position: 1, depth: 2, ancestry: 1/2)
│   └── Category 4 (id: 4, position: 2, depth: 2, ancestry: 1/2)
└── Category 5 (id: 5, position: 2, depth: 1, ancestry: 1)
    ├── Category 6 (id: 6, position: 1, depth: 2, ancestry: 1/5)
    └── Category 7 (id: 7, position: 2, depth: 2, ancestry: 1/5)

When I move Category 6 to root level their children assumes a wrong position

Category 1 (id: 1, position: 1, depth: 0, ancestry: )
└── Category 2 (id: 2, position: 1, depth: 1, ancestry: 1)
    ├── Category 3 (id: 3, position: 1, depth: 2, ancestry: 1/2)
    └── Category 4 (id: 4, position: 2, depth: 2, ancestry: 1/2)
Category 5 (id: 5, position: 2, depth: 0, ancestry: )
├── Category 6 (id: 6, position: 1, depth: 1, ancestry: 5)
└── Category 7 (id: 7, position: 1, depth: 1, ancestry: 5)

As you can see both 6 and 7 have position set to 1 after the transaction

Here is another example with two errors after transaction (only the final tree)

Category 1 (id: 1, position: 1, depth: 0, ancestry: )
├── Category 2 (id: 2, position: 1, depth: 1, ancestry: 1)
│   ├── Category 3 (id: 3, position: 1, depth: 2, ancestry: 1/2)
│   ├── Category 4 (id: 4, position: 2, depth: 2, ancestry: 1/2)
│   └── Category 5 (id: 5, position: 3, depth: 2, ancestry: 1/2)
└── Category 6 (id: 6, position: 2, depth: 1, ancestry: 1)
    ├── Category 7 (id: 7, position: 1, depth: 2, ancestry: 1/6)
    ├── Category 8 (id: 8, position: 2, depth: 2, ancestry: 1/6)
    └── Category 9 (id: 9, position: 3, depth: 2, ancestry: 1/6)
Category 10 (id: 10, position: 3, depth: 0, ancestry: )
├── Category 11 (id: 11, position: 1, depth: 1, ancestry: 10)
├── Category 12 (id: 12, position: 1, depth: 1, ancestry: 10)
└── Category 13 (id: 13, position: 2, depth: 1, ancestry: 10)

Here node 11 and 12 have the same position and node 10 (the node I moved) has position set to 3.

I think this behavior is due to the fact that ancestry gem triggers updates for the ancestry column for all children nodes and when they execute acts_as_list callbacks, but since their position is not changed they fail to update lists.

Query log

Spec code is (more or less) the following:

  # Default tree has 1 + 2 + 4 nodes, tree is also the root node
  let!(:tree) { create(:category_tree, breadth: breadth) }

  it 'should keep order consistent on rearrange' do
    node_to_move = tree.children.last
    with_query_logging { node_to_move.update_attributes! parent: nil }
    expect(node_to_move.reload.children.map(&:position)).to eq([1,2])
  end

and here is the most relevant and commented output (full output is here):

...
"Executing check_scope for 6"
# Here callback for 6 updates node 7 position which becomes 1
SQL (0.1ms)  UPDATE "categories" SET position = (position - 1) WHERE ("categories"."ancestry" = '1/5' AND position > 1)
...
"Executing check_scope for 7"
# Here node 7 in its callback still uses the old position (i.e. 2) while in database the new position is 1, updated by previous update
Category Load (0.1ms)  SELECT  "categories".* FROM "categories"  WHERE ("categories"."ancestry" = '1/5' AND position > 2)  ORDER BY categories.position ASC LIMIT 1
# Here is the same
SQL (0.0ms)  UPDATE "categories" SET position = (position + 1) WHERE ("categories"."ancestry" = '5' AND position >= 2)
(0.1ms)  SELECT COUNT(*) FROM "categories"  WHERE ("categories"."ancestry" = '5' AND position = 2)

Demo app

I've uploaded a demo app to reproduce the problem, you can find it here: https://github.com/fabn/category.

You can execute specs with a couple of env variables to control output and size of the generated tree, for instance

DEBUG=1 BREADTH=3 rspec

will generate the latest tree in the example above.

[fabn]

I'm investigating this issue and I found out that the issue is triggered by check_scope method.

I've changed the method in this way to debug the problem

          def check_scope
            if scope_changed?
              position = send(position_column)
              db_position = self.class.where(id: id).select(position_column).first[position_column]
              warn "For record #{id} db_position is different #{position} != #{db_position}" if db_position != position
              # self[position_column] = db_position
              swap_changed_attributes
              send('decrement_positions_on_lower_items') if lower_item
              swap_changed_attributes
              send("add_to_list_#{add_new_at}")
            end
          end

and in the current codebase the warning is triggered more than once, so there must be something wrong in method implementation.

If I enable the self[position_column] = db_position some tests fail but in my use case described in the issue report I got rid of duplicate positions, while still having an inconsistent order.

I'll try to create a test case for this particolar issue, @swanandp is a problem if I add ancestry gem in Gemfile to reproduce the issue?

[fabn]

Hi @swanandp could you please send me a feedback on this issue (and related PR). Thanks.

[burnzoire]

I can confirm this issue exists. I've applied @fabn 's fix as a temp monkey patch and can confirm it resolves the issue. Waiting for PR to be resolved.

[brendon]

Hi @fabn, you're still working on this one by the looks?

[fabn]

@brendon I'm not working on this in this moment. I rebased my original pr couple of weeks ago but currently it's not mergeable anymore.

[brendon]

Ok, let's leave it open. I use ancestry too. I haven't noticed problems but I actually override the following to disable the scope detection, then have a few methods to move items around instead:

  acts_as_tree :cache_depth => true

  # MySQL sorts NULL's first anyway, so we override ancestry's scopes
  scope :ordered_by_ancestry, lambda { reorder("#{table_name}.#{ancestry_column}") }
  scope :ordered_by_ancestry_and, lambda { |order| reorder("#{table_name}.#{ancestry_column}, #{order}") }

  acts_as_list

  # There is a bug (as of acts_as_list 0.7.2) that causes some reordering issues when moving tree nodes.
  # Setting the scope manually, and setting scope_changed? to false avoids the scope changing logic, and
  # avoids the bug. We handle moving nodes around the tree via the `move` method.
  def scope_condition
    { :ancestry => read_attribute(:ancestry) }
  end

  def scope_changed?
    false
  end

Let me know if you want to see those methods.

[brendon]

Hi @fabn, I'd be interested to see if the work on @scope_changed fixed this bug. Could you let me know?

[fabn]

@brendon rebased again the PR and the fix introduced in 1cf515d9fccc473409fc729ae394493aebe1ba86 is still necessary. Without that commit tests will fail.

[brendon]

Hi @fabn, given that the Rails 5 compatible attribute swapper is now in master, let's try to close this one off with a solution around that existing code. I think there's also no PR for this yet. Can you open one and that'll give us a working point. :)

[brendon]

Hi @fabn, just reviewing my code, I've been able to remove my scope overrides and am now about to just do this:

acts_as_list :scope => [:ancestry]

This is my move method:

  # Expects an array of sibling_ids (strings), and a parent component_instance
  # This method relies on scope_changed? in acts_as_list to do the magic of positioning
  def move(parent, sibling_ids = [], scope = parent.children.current)
    self.parent = parent

    if sibling_ids.present?
      raise ArgumentError.new('self.id must exist in sibling_ids') unless sibling_ids.include?(id.to_s)

      if sibling_ids.size == 1
        self.position = 1
      elsif sibling_ids.size > 1
        position_among_siblings = sibling_ids.index(id.to_s)
        reference = scope.offset(position_among_siblings).first

        self.position = reference.position if reference
      end
    end

    save
  end

[aadill77]

Hi all, I am also expecting similar issue with acts_as_paranoid and acts_as_list.
I have already asked a question on Stack Overflow on the matter. Would be glad if I can get some help Here. Thanks.
http://stackoverflow.com/questions/43788773/acts-as-list-with-paranoia-gem

[patrickdavey]

I have just experienced this issue using ancestry 3.0 and acts_as_list 0.9.5 (on a rails 3.2 codebase).

I have no experience of the internals of either of these gems, but, getting this bug fixed is pretty important to me. If I take a copy of the PR and try to fix the merge conflicts would someone be available to review? (not sure that I'll be up to it, but I can try!)

Alternatively, @brendon I didn't quite follow in your comment above are you saying that you are able to use acts_as_list :scope => [:ancestry] as long as you are using the move method you defined above? That is, the only change required to fix is to use that move method and then no fix required?

Thanks for the great gems!

[brendon]

Hi @patrickdavey, I'm happy for you to have a go at getting the patch working with master. acts_as_list is a fairly simple codebase. The tests are super easy to run. We use appraisal to run across multiple rails versions but just running bundle then rake should install the required gems and execute the tests provided you're on a decent ruby version.

Correct, in that I get by with just the code mentioned above. I had a look and I also use this:

    # MySQL sorts NULL's first anyway, so we override ancestry's scopes
    scope :ordered_by_ancestry, -> { reorder("component_instances.ancestry") }
    scope :ordered_by_ancestry_and, -> (order) { reorder("component_instances.ancestry, #{order}") }

That might help you too, but it's not got much to do with the main problem. component_instances is just my model table.

Keep me posted on your progress :)

[patrickdavey]

@brendon I have had a play with this, but, I haven't got very far. We then went with closure_tree as that handles the ordering and nesting etc. (it has its own performance issues, but, that's life).

If I get some more time I'll try and fix it.

[brendon]

All good @patrickdavey. I remember looking at closure_tree for a new project but got freaked out from memory. I can't remember why. I ended up with ancestry and ranked_model though unfortunately ranked_model seems to be unmaintained at the moment. Perhaps I shouldn't have cheated on acts_as_list! ;)

[patrickdavey]

Well @brendon I ran into a nasty bug which brought production to, while not a halt, a bad place ;) So.. now I'm thinking I shouldn't have cheated on ancestry and acts_as_list ;)

[brendon]

Lol! Come back home man!

[cipater]

Having the same issue as OP with acts_as_list + ancestry.

Taking the scope_changed? suggestion above I came up with a quick'n'dirty hack that seems to be working well for me:

def scope_changed?
  return false unless ancestry_changed?
  return true unless ancestry_change.all? # return true if either new or old value is nil
  # 'scope' is the immediate parent (the last position of the ancestry key), 
  # so let's compare the previous value to see if it has changed
  ancestry_change[0].split('/')[-1] != ancestry_change[1].split('/')[-1]
end

[brendon]

Hi @cipater, can you come up with a failing test for acts_as_list? We can then look at fixing this. I use ancestry with acts_as_list as I mentioned further up the comments and don't have any problems just using the array type scope definition. Are you on the latest version?

[brendon]

@fabn, not sure what to do with this one. Should we close it for now?

[brendon]

Closing this for now.