brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.04k stars 355 forks source link

Add byebug gem and failing specs for bulk update with nested attributes #404

Closed singhdurgesh closed 3 months ago

singhdurgesh commented 2 years ago

Adding the failing specs for bulk update with nested attributes

brendon commented 2 years ago

Hi @singhdurgesh, can you give me a quick rundown on the issue? :)

singhdurgesh commented 2 years ago

Hi,

When we use acts as list with the scope of a foreign key and we have added the uniqueness constraint on position and scoped class id.

Envoirnment: Database: Postgres

Let's say, we have a Page model. and it has many items. And to create/update/destroy, nested attributes are being used.

class Page < ActiveRecord::Base
  attr_accessor :label
  has_many :items, dependent: destroy

  accepts_nested_attributes_for :items
end

class Item < ActiveRecord::Base
  attr_accessor :detail, :page_id, :position
  belongs_to :page

  acts_as_list scope: :page
end

We have unique constraint on the items table -> [page_id, position], to avoid the duplication issue.

We create a page with few items.

page = Page.create(label: "ABC Poster", items_attributes: [{detail: 'Sec 1', position: 1}, {'detail: 'Sec 2', position:  2}, {detail: 'Sec 3', position: 3}]
item_position1 = page.items.find_by(position: 1)
item_position2 = page.items.find_by(position: 2)
item_position3 = page.items.find_by(position: 3)

Here, we try to shuffle the position of items, error is being raised.

page.update(items_attributes: [{id: item_position1.id, position: 2}, {id: item_position2.id, position: 3}, {id: item_position3.id, position: 1}]

Exception:
ScopedWithUserDefinedUniqueConstraint#test_scope_with_user_defined_unique_constraint:
ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "scoped_parent_id_position_unique"
DETAIL:  Key (scoped_parent_id, "position")=(1, 2) already exists.

    /Users/bluesapling05/.rvm/gems/ruby-3.1.2/gems/activerecord-7.0.3.1/lib/active_record/connection_adapters/postgresql_adapter.rb:768:in `exec_params'
    /Users/bluesapling05/.rvm/gems/ruby-3.1.2/gems/activerecord-7.0.3.1/lib/active_record/connection_adapters/postgresql_adapter.rb:768:in `block (2 levels) in exec_no_cache'
    /Users/bluesapling05/.rvm/gems/ruby-3.1.2/gems/activesupport-7.0.3.1/lib/active_support/concurrency/share_lock.rb:187:in `yield_shares'
    /Users/bluesapling05/.rvm/gems/ruby-3.1.2/gems/activesupport-7.0.3.1/lib/active_support/dependencies/interlock.rb:41:in `permit_concurrent_loads'
    /Users/bluesapling05/.rvm/gems/ruby-3.1.2/gems/activerecord-7.0.3.1/lib/active_record/connection_adapters/postgresql_adapter.rb:767:in `block in exec_no_cache'
    /Users/bluesapling05/.rvm/gems/ruby-3.1.2/gems/activesupport-7.0.3.1/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'
    /Users/bluesapling05/.rvm/gems/ruby-3.1.2/gems/activesupport-7.0.3.1/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'
    /Users/bluesapling05/.rvm/gems/ruby-3.1.2/gems/activesupport-7.0.3.1/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'
    /Users/bluesapling05/.rvm/gems/ruby-3.1.2/gems/activesupport-7.0.3.1/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'
    /Users/bluesapling05/.rvm/gems/ruby-3.1.2/gems/activerecord-7.0.3.1/lib/active_record/connection_adapters/abstract_adapter.rb:765:in `block in log'
    /Users/bluesapling05/.rvm/gems/ruby-3.1.2/gems/activesupport-7.0.3.1/lib/active_support/notifications/instrumenter.rb:24:in `instrument'
brendon commented 2 years ago

Thanks @singhdurgesh, check out sequential_updates: https://github.com/brendon/acts_as_list#more-options

Does this help? :)

singhdurgesh commented 2 years ago

No, sequential_updates doesn't help in this case.

brendon commented 2 years ago

Hi @singhdurgesh, does it work if you update only one item record at a time with nested attributes? The gem isn't really built to cope with multiple repositioning all at once. When I do this (say just rewriting all positions in a scope for a list) I just use update_all which avoids any interaction with acts_as_list. acts_as_list is supposed to shuffle things for you when you move something but it looks like you're just trying to set all positions again which means you probably don't need acts_as_list in that sense. What do you think?

singhdurgesh commented 2 years ago

Hi @brendon, with nested attributes, by default Rails saves the nested objects one by one. That is why there is duplication in the database and raising exception. Also, in the following case exception is being raised


class ListObject < ActiveRecord
  attr_accessor :position
end

# position has uniquness constraint on the database.

list_object1 = ListObject.create(position: 1)
list_object2 = ListObject.create(position: 2)

list_object1.update(position: 2)
raises ActiveRecord::RecordNotUnique: PG::UniqueViolation: ERROR

I looked up into this issue and I think the solution would be around a before_save callback where we will be repositioning the other objects if position is already taken with the current scope.

brendon commented 2 years ago

I've actually implemented a simple concern (well it started out that way) that implements a lot of what acts_as_list does. It's not a gem yet but the gist is here: https://gist.github.com/brendon/d6cfd60cb5e70dc77a15a2476f04d279

I wanted to have a uniqueness constraint too and also have complex scopes. I've not tried nested attributes with it. Would be interested to see how it works for you?

singhdurgesh commented 2 years ago

Thanks a lot @brendon!. I'll check it out. :)

singhdurgesh commented 3 months ago

Closing the PR as it was open for very long time.

brendon commented 3 months ago

Hi @singhdurgesh, in the intervening years I've since released the positioning gem based off of that concern: https://github.com/brendon/positioning

Check it out and let me know if it performs better in your situation :)