Position is set incorrectly when circular dependencies exist

[afn]

We saw some unusual behavior in our system, where items were being incorrectly reordered by acts_as_list. It seems that in order to reproduce the bug, you need a relatively complex set of associations, and the list needs to be scoped. See the failing test case below...

[afn]

With activerecord 4.1.2 .. 4.2.0, it fails like this:

$ rake
Run options: --seed 22446

# Running:

...................................................F.....................................................

Finished in 2.059680s, 50.9788 runs/s, 303.4452 assertions/s.

  1) Failure:
CircularAssociationsScopeTest#test_position [/Users/afn/acts_as_list/test/test_list.rb:636]:
Expected: [1, 2]
  Actual: [1, 1]

105 runs, 625 assertions, 1 failures, 0 errors, 0 skips
rake aborted!
Command failed with status (1): [ruby -I"lib:lib:test" -I"/Users/afn/.rvm/gems/ruby-2.1.1/gems/rake-10.4.2/lib" "/Users/afn/.rvm/gems/ruby-2.1.1/gems/rake-10.4.2/lib/rake/rake_test_loader.rb" "test/**/test_*.rb" ]

Tasks: TOP => default => test
(See full trace by running task with --trace)

With activerecord 4.1.1 or earlier, it passes.

[afn]

Here are the queries it runs with activerecord 4.2.0 (failing):

 (0.0ms)  begin transaction
SQL (0.1ms)  INSERT INTO "foos" DEFAULT VALUES
SQL (0.1ms)  UPDATE "cats" SET position = (position + 1) WHERE "cats"."foo_id" IS NULL AND (position >= 1)
SQL (0.1ms)  INSERT INTO "cats" ("position", "name") VALUES (?, ?)  [["position", 1], ["name", "1"]]
SQL (0.1ms)  INSERT INTO "bars" ("cat_id") VALUES (?)  [["cat_id", 1]]
SQL (0.0ms)  UPDATE "cats" SET position = (position + 1) WHERE "cats"."foo_id" IS NULL AND (position >= 2)
SQL (0.0ms)  INSERT INTO "cats" ("position", "name", "bar_id") VALUES (?, ?, ?)  [["position", 2], ["name", "2"], ["bar_id", 1]]
SQL (0.0ms)  INSERT INTO "bats" ("foo_id", "cat_id") VALUES (?, ?)  [["foo_id", 1], ["cat_id", 2]]
CircularAssociationsScopeTest::Cat Load (0.1ms)  SELECT  "cats".* FROM "cats" WHERE "cats"."foo_id" IS NULL AND (position > 1)  ORDER BY cats.position ASC LIMIT 1
SQL (0.1ms)  UPDATE "cats" SET position = (position - 1) WHERE "cats"."foo_id" IS NULL AND (position > 1)
SQL (0.0ms)  UPDATE "cats" SET position = (position + 1) WHERE "cats"."foo_id" = ? AND (position >= 1 AND id != 1)  [["foo_id", 1]]
SQL (0.0ms)  UPDATE "cats" SET "foo_id" = ? WHERE "cats"."id" = ?  [["foo_id", 1], ["id", 1]]
 (0.1ms)  SELECT COUNT(*) FROM "cats" WHERE "cats"."foo_id" = ? AND (position = 1)  [["foo_id", 1]]
CircularAssociationsScopeTest::Cat Load (0.1ms)  SELECT  "cats".* FROM "cats" WHERE "cats"."foo_id" IS NULL AND (position > 2)  ORDER BY cats.position ASC LIMIT 1
SQL (0.0ms)  UPDATE "cats" SET position = (position + 1) WHERE "cats"."foo_id" = ? AND (position >= 2 AND id != 2)  [["foo_id", 1]]
SQL (0.0ms)  UPDATE "cats" SET "foo_id" = ? WHERE "cats"."id" = ?  [["foo_id", 1], ["id", 2]]
 (0.1ms)  SELECT COUNT(*) FROM "cats" WHERE "cats"."foo_id" = ? AND (position = 2)  [["foo_id", 1]]
 (0.0ms)  commit transaction

And with 4.1.1 (passing):

 (0.0ms)  begin transaction
SQL (0.0ms)  INSERT INTO "foos" DEFAULT VALUES
SQL (0.1ms)  UPDATE "cats" SET position = (position + 1) WHERE "cats"."foo_id" = 1 AND (position >= 1)
SQL (0.1ms)  INSERT INTO "cats" ("foo_id", "name", "position") VALUES (?, ?, ?)  [["foo_id", 1], ["name", "1"], ["position", 1]]
SQL (0.0ms)  INSERT INTO "bars" ("cat_id") VALUES (?)  [["cat_id", 1]]
SQL (0.0ms)  UPDATE "cats" SET position = (position + 1) WHERE "cats"."foo_id" = 1 AND (position >= 2)
SQL (0.0ms)  INSERT INTO "cats" ("bar_id", "foo_id", "name", "position") VALUES (?, ?, ?, ?)  [["bar_id", 1], ["foo_id", 1], ["name", "2"], ["position", 2]]
SQL (0.1ms)  INSERT INTO "bats" ("cat_id", "foo_id") VALUES (?, ?)  [["cat_id", 2], ["foo_id", 1]]
 (0.1ms)  commit transaction

[afn]

This is starting to look more like an ActiveRecord bug rather than an acts_as_list issue. It appears that Cat is being updated (unnecessarily and incorrectly) immediately after being created. I'm working on reproducing the issue without using acts_as_list; if I manage to do so, I'll close this one.

[afn]

I believe the reason this bug surfaced in our application is related to rails/rails#18704, but there's still an issue with acts_as_list in the case where there's truly a circular dependency (and therefore the save call can't avoid UPDATEs). I've committed a test case which fails even on Rails < 4.1.2: https://github.com/amitree/acts_as_list/blob/7d4ff108728e6349e17a9e5825de7a7a4a33ba22/test/test_list.rb#l604

It fails with:

  1) Failure:
CircularAssociationsScopeTest#test_position [/Users/afn/acts_as_list/test/test_list.rb:619]:
Expected: [1, 2]
  Actual: [1, 3]

[afn]

I think the fundamental issue here is that when the caller inserts an element in a list at a specific position outside of the range [1..n+1], where n is the number of items currently in the list, then the shuffling logic doesn't quite do the right thing. In the above failing test case, what's happening is that first cat2 is being inserted at position 2, then cat1 is inserted at position 1, so the second insertion causes cat2 to be bumped down to position 3.

There are two possible ways to fix this:

1. Ensure that items are never inserted beyond the end of the list. So when we start by adding cat2 with position 2, after saving its position should be changed to 1.
2. When shuffling items around, try to fill any gaps. This seems like a much more difficult approach.

What do you think?

[swanandp]

Thanks for the detailed debugging, I'll go through this today.

[mhenrixon]

👍

[brendon]

@afn, could you rebase this to confirm that it's still a bug? Let's try to get this one solved :)

[afn]

Sure - I've rebased amitree/acts_as_list and it still fails.

[brendon]

Thanks for that. Could you do a pull request for this so we can look at the diff's?

[brendon]

@afn, can you introduce the failing tests as a PR and we can go from there.

[afn]

@brendon There's already a PR, #204. Thanks!

[brendon]

Thanks @afn :) Good to have it linked here :) Can you rebase your code to master and then we can go from there :)

[brendon]

Hi @afn, can we move this one along? I'm keen to close it off :)

[afn]

Sure! See #153 (comment) — I was waiting for some feedback on the preferred solution.

[brendon]

Ah righty :) There was a recent PR accepted that dealt with disparate position integers. Could you give master a whirl and see if it still causes this problem?

[afn]

Yep, I think that PR took care of it. Thanks!

[brendon]

Glad to hear it :)