Rails 4.0 - delete_if() doesn't work as expected on has_many associations

[georgiev]

delete_if() doesn't remove objects from has_many association test case: https://gist.github.com/georgiev/6446656

[gouravtiwari]

This worked for me, I ran the test and it passed, can you elaborate more?

[georgiev]

Ok - here's what I have starting with blank rvm installation:

$ ruby --version ruby 1.9.3p194 (2012-04-20 revision 35410) [x86_64-linux]

$ gem list

* LOCAL GEMS *

bundler (1.1.3) rake (0.9.2.2) rubygems-bundler (1.0.2) rvm (1.11.3.3)

$ gem install rails $ gem install sqlite3

$ gem list

* LOCAL GEMS *

actionmailer (4.0.0) actionpack (4.0.0) activemodel (4.0.0) activerecord (4.0.0) activerecord-deprecated_finders (1.0.3) activesupport (4.0.0) arel (4.0.0) atomic (1.1.13) builder (3.1.4) bundler (1.3.5, 1.1.3) erubis (2.7.0) hike (1.2.3) i18n (0.6.5) mail (2.5.4) mime-types (1.25) minitest (4.7.5) multi_json (1.8.0) polyglot (0.3.3) rack (1.5.2) rack-test (0.6.2) rails (4.0.0) railties (4.0.0) rake (0.9.2.2) rubygems-bundler (1.0.2) rvm (1.11.3.3) sprockets (2.10.0) sprockets-rails (2.0.0) sqlite3 (1.3.8) thor (0.18.1) thread_safe (0.1.2) tilt (1.4.1) treetop (1.4.15) tzinfo (0.3.37)

$ testrb has_many_delete_if_test.rb Finished tests in 0.035786s, 27.9442 tests/s, 111.7769 assertions/s.

1) Failure: test_has_many_delete_if(HasManyDeleteIfTest) [/home/tangra/projects/r4_test/has_many_delete_if_test.rb:38]: Expected: true Actual: false

1 tests, 4 assertions, 1 failures, 0 errors, 0 skips

[robin850]

I can reproduce this as well on my machine. It works on 3.2.14. Actually, the problem is that post.comments returns an Array on 3.2.14 while Rails 4.0.0 returns ActiveRecord::Associations::CollectionProxy::ActiveRecord_Associations_CollectionProxy_Comment which seems to inherit from ActiveRecord::Relation. I have no workaround but following code:

class ActiveRecord::Relation
  include Enumerable
end

Make the following assertion:

assert_equal true, post.comments.empty?

green but the record isn't removed from the database :

assert_equal 0, post.comments.size # Fails

I'm not comfortable with ActiveRecord so I'm pinging @senny.

[gouravtiwari]

I am able to reproduce this too

[georgiev]

I also think that there's inconsistency with has_many.include?(), which may be related, but I'll do more tests tomorrow.

[gouravtiwari]

Further more, it's a bug in ActiveRecord, where relation doesn't respond_to delete_if in rails 4.0.0

# In Rails 4.0.0
print post.comments.respond_to?(:delete_if)
> false
print post.comments.class
> ActiveRecord::Associations::CollectionProxy::ActiveRecord_Associations_CollectionProxy_Comment
# As@robin850 mentioned

# In Rails 3.2.14
print post.comments.respond_to?(:delete_if)
> true
print post.comments.class
> Array

Looking further to see if we need to really include Enumerable or something we missed from rails-3.2.14

[georgiev]

Yep :( has_many.include?() doesn't return Boolean as expected. test case: https://gist.github.com/georgiev/6485097

[gouravtiwari]

Commenting out the dup method actually makes it work, which is correct in case of delete_if as delete_if need to be executed on the array object not the duplicate.

      # active_record/associations/collection_proxy.rb
      def to_ary
        load_target#.dup
      end

This could be a work around, but may not be the right approach as it would break some other methods. Finding root cause further.

[al2o3cr]

Just tried this on a Rails 3.2 app, and while the records are removed from the in-memory array they aren't unlinked in the DB. Perhaps the method was removed in 4.0 because it doesn't do what users (well, what at least THIS user) would have expected?

[georgiev]

Let me clear this - I'm not expecting records to be updated in the DB without calling save on the master of the association - the test case exposes exactly what I'm expecting. It's another question that I'll expect them to be updated when I call save on the Post.

[hitendrasingh]

I am able to reproduce this bug too with Active Record '4.0.0'. and mysql2 '0.3.13'. But @gouravtiwari as you said that respond_to_delete_if is returning false. Its not happening in my case :

# In Active Record 4.0.0
p post.comments.respond_to?(:delete_if)
> true
p post.comments.class
> ActiveRecord::Associations::CollectionProxy::ActiveRecord_Associations_CollectionProxy_Comment
p post.comments.delete_if{true}
> [ ]
p post.comments
> #<ActiveRecord::Associations::CollectionProxy [#<Comment id: 1, post_id: 1>]>

[gouravtiwari]

Not sure, may be I was debugging and found that in some weird scenario. Now cleaning up does show as what you said @hitendrasingh, apologize for incorrect info.

[rafaelfranca]

This should be not supported. What an association return is not an Array. If you want do array operation on it you should call to_a.

[georgiev]

And why is it supported then !? This is soooo wrong - if the method is not supported it should not allow me to call it !!

[hitendrasingh]

+1 for @georgiev's comment...

[jeremy]

 if the method is not supported it should not allow me to call it !!

Yes. It should raise a NoMethodError if it's not delegated.

[dguti]

I think the method should be supported. In rails 3.0.11 I used delete_if extensively in this case;

• I built several nested forms for several model instances created by invoking build on the parent association in the following way: parent.association_name.build
• After sending the form, when updating the parent model, all children that had no data are removed
• The removal operation is performed in the following way: parent.association_name.delete_if { |c| c.is_empty?}

I think that was the expected way delete_if had to work, but it doesn't work anymore!

[aaron]

@dguti I was working on a similar issue and found that I could replace the old delete_if code with accepts_nested_attributes_for :reject_if option.

# creates avatar_attributes=
accepts_nested_attributes_for :avatar, reject_if: proc { |attributes| attributes['name'].blank? }
# creates avatar_attributes=
accepts_nested_attributes_for :avatar, reject_if: :all_blank

[dguti]

Thank you for your suggestion, Aaron, I'll probably make use of it in the near future.

An alternative way of implementing "delete_if" could be "parent_model.has_many_assoc.select{|c| ...condition...}.each{|c| c.delete}

In my opinion, the rails way should make possible to manage arrays and associations as similarly as possible, and that includes the implementation of these wrapper methods.

2013/9/25 Aaron Baldwin notifications@github.com

@dguti https://github.com/dguti I was working on a similar issue and found that I could replace the old delete_if code with accepts_nested_attributes_for :reject_if option.

creates avatar_attributes=accepts_nested_attributes_for :avatar, reject_if: proc { |attributes| attributes['name'].blank? }# creates avatar_attributes=accepts_nested_attributes_for :avatar, reject_if: :all_blank

— Reply to this email directly or view it on GitHubhttps://github.com/rails/rails/issues/12140#issuecomment-25117988 .

d.gutierrez.perez@gmail.com

[jaredbeck]

@jeremy said:

It should raise a NoMethodError

It looks like rails 4.1.0.beta1 now raises NoMethodError, though the release notes do not currently mention this. Can you confirm this / update the release notes?

[carlosantoniodasilva]

I think it is not in the release notes, but in the changelog only:

Relation no longer has mutator methods like #map! and #delete_if. Convert to an Array by calling #to_a before using these methods.

It intends to prevent odd bugs and confusion in code that call mutator methods directly on the Relation.

[senny]

Added the change to the Rails 4.1 release notes: abe648452f04be52ee3f95031f0ea01f98fdcf40

[jaredbeck]

Thanks @carlosantoniodasilva and @senny! Good to know about map! too.