Cannot add position uniqueness index

[bzwei]

In order to fight against the duplicated position issue the document suggests to add position uniqueness index. However this constraint forbids the position being updated. Positions are adjusted in the after_update method updated_positions. Specifically it fixes duplicates when they are found in https://github.com/brendon/acts_as_list/blob/master/lib/acts_as_list/active_record/acts/list.rb#L418

In my test this updated_positions has no chance to be called because the uniqueness constraints already raises ActiveRecord::RecordNotUnique Exception: PG::UniqueViolation

[brendon]

Hi @bzwei, I'm not sure I understand you. Are you able to raise a PR with a failing test case to show the problem? :)

[bzwei]

@brendon If I add a migration in my app like

  def change
    add_index :workflows, [:position, :tenant_id], unique: true
  end

In the model class I force sequential updates

class Workflow < ApplicationRecord
  acts_as_list :scope => [:tenant_id],  :sequential_updates => true
end

I can no longer update the position, getting error ActiveRecord::RecordNotUnique Exception: PG::UniqueViolation.

[brendon]

Thanks @bzwei, can you show me the code you use to update the position?

[bzwei]

I have a rspec test

    around(:each) do |example|
      ActsAsTenant.with_tenant(tenant) { example.run }
    end

    before { create_list(:workflow, 5) }
    it 'moves up a workflow position' do
      old_ids = Workflow.pluck(:id)
      Workflow.find(old_ids[3]).update(:position => Workflow.find(old_ids[1]).position)
      expect(Workflow.pluck(:id)).to eq([old_ids[0], old_ids[3], old_ids[1], old_ids[2], old_ids[4]])
    end

    it 'moves down a workflow position' do
      old_ids = Workflow.pluck(:id)
      Workflow.find(old_ids[1]).set_list_position(Workflow.find(old_ids[3]).position)
      expect(Workflow.pluck(:id)).to eq([old_ids[0], old_ids[2], old_ids[3], old_ids[1], old_ids[4]])
    end

Before the test I create 5 workflows with consecutive id and position. Then I use either update or set_list_position method to change the position, get the same error message.

[brendon]

I see your point. Unfortunately I'm not in much of a position to fix this quickly but am definitely happy to help you by reviewing a PR if you'd like to take a stab at fixing this yourself?

Start with a failing test in the PR and let's see where we get to :D

[ericpowell03]

I'm running into this issue with uniqueness indices configured on Postgres, regardless of the :sequential_updates setting. I removed the indices for now. A call to item.move_higher will fail when the index is present. It works once I have removed it.

class ChecklistItemDefinition< ApplicationRecord
  belongs_to :checklist_definition
  acts_as_list scope: :checklist_definition
end

class ChecklistDefinition< ApplicationRecord
  has_many :checklist_item_definitions, -> { order(position: :asc) }
end

#The Migration
add_index :checklist_item_definitions, [:checklist_definition_id, :position], unique: true, name: :checklist_item_definition_position_unique

As @bzwei noted, I think this is due to the after_update callback. The record can't save because it violates the unique constraint before the other position values are updated. I'd have to dig into the code a bit more, but using a before_save callback could work, but could also have unintended consequences. For now, perhaps a note on the documentation would help.

[brendon]

Hi @ericpowell03, you're probably right there. I'd be happy to merge a documentation PR for you. Changing when the re-arranging happens would be a bigger (probably breaking) change. To be honest, the re-arranging of the list is the central problem of this gem. It also causes deadlocks in some instances. I maintain ranked-model and that also has some issues around re-arranging. Sometimes I think it might be time to come up with a better strategy and spin off a brand new gem :)

[ericpowell03]

Hey @brendon -- yeah, I'll work on putting something together. I figured moving the callback would probably be too big of a change. It works as-is, though I do think it would be good to be able to enforce the uniqueness constraint at the DB level.

[martinstreicher]

I ran into this today with a model in development. I have a uniqueness constraint in the DB akin to %I[position project_id user_id] to allow each user to maintain a unique order for projects. A move_higher fails because the first save of any project below the project being moved runs afoul of the constraint.

[brendon]

Hi @martinstreicher, sadly I'm not sure there's an easy answer to having a unique constraint on the position column. There is a mode you can go into called sequential_updates. With that on, changes are made one at a time which could get around your issue.

[kamilpogo]

This should help: 1) Create an constraint that is DEFERRABLE

ALTER TABLE table_name
ADD CONSTRAINT index_name UNIQUE (project_id, position, user_id) DEFERRABLE INITIALLY IMMEDIATE;

2) Execute move_lower / move_higher in a transaction and set the constraint of your index to DEFERRED

ActiveRecord::Base.transaction do
  ActiveRecord::Base.connection.execute('SET CONSTRAINTS index_name DEFERRED') 
  item.move_lower
end

https://www.postgresql.org/docs/current/sql-set-constraints.html

[brendon]

I don't think the DEFERRABLE trick works on MySQL last I checked? Could be wrong.

I recently rolled my own list maintenance concern that you can see here: https://gist.github.com/brendon/d6cfd60cb5e70dc77a15a2476f04d279

I solved the reordering problem by moving the moved item to position 0 (with straight SQL to avoid any callbacks). I then shuffle things around and the moved items new position is set at the conclusion of the save process.

Implementing this process in acts_as_list would probably be a breaking change as some people use 0. Could use -1 I guess but some people have a positive integer constraint too.

[moveson]

1. Create an constraint that is DEFERRABLE

ALTER TABLE table_name
ADD CONSTRAINT index_name UNIQUE (project_id, position, user_id) DEFERRABLE INITIALLY IMMEDIATE;

@kamilpogo Any reason not to create this constraint as DEFERRABLE INITIALLY DEFERRED ? Wouldn't that eliminate the need to set the constraint as DEFERRED later on?

[kamilpogo]

1. Create an constraint that is DEFERRABLE

ALTER TABLE table_name
ADD CONSTRAINT index_name UNIQUE (project_id, position, user_id) DEFERRABLE INITIALLY IMMEDIATE;

@kamilpogo Any reason not to create this constraint as DEFERRABLE INITIALLY DEFERRED ? Wouldn't that eliminate the need to set the constraint as DEFERRED later on?

There might be some implications: Rails automatically wraps any test case in a database transaction that is rolled back after the test completes (see https://guides.rubyonrails.org/testing.html#testing-parallel-transactions ). So your test might behave differently than your production code (constraint won't be checked in tests while it is checked in development/production) if you don't use transactions. In tests you could use self.use_transactional_tests = false and later clean up your test database if your constraint is initially deferred.

But I didn't have a look if this is an issue here. Maybe everything is wrapped in a transaction. Then there shouldn't be a problem

[sedubois]

So now with Rails 7.1.0 available, for Postgresql DBs it should be doable as shown below, see https://github.com/rails/rails/pull/46192, https://github.com/rails/rails/pull/49383, https://github.com/rails/rails/pull/49393:

class ArticleUniquePosition < ActiveRecord::Migration[7.1]
  def change
    add_index :articles, :position, unique: true
    add_unique_constraint :articles, [:position], deferrable: :deferred, name: "unique_article_position"
  end
end

Then just use article.move_lower.

-- or --

class ArticleUniquePosition < ActiveRecord::Migration[7.1]
  def change
    add_index :articles, :position, unique: true
    add_unique_constraint :articles, [:position], deferrable: :immediate, name: "unique_article_position"
  end
end

Then something like:

Article.transaction do
  Article.connection.execute('SET CONSTRAINTS index_name DEFERRED') 
  article.move_lower
end

I haven't tested yet as I first need to upgrade to Rails 7.1.

[brendon]

Happy for you to try some things out to see how you get on :)

[brendon]

Closing this for now. Check out https://github.com/brendon/positioning as it supports the unique constraint out of the box.