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

Closed
georgiev opened this Issue Sep 5, 2013 · 23 comments

Projects

None yet
@georgiev
georgiev commented Sep 5, 2013

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

@gouravtiwari
Contributor

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

@georgiev
georgiev commented Sep 8, 2013

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
Member
robin850 commented Sep 8, 2013

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
Contributor

I am able to reproduce this too

@georgiev
georgiev commented Sep 8, 2013

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

@gouravtiwari
Contributor

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
georgiev commented Sep 8, 2013

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

@gouravtiwari
Contributor

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
Contributor
al2o3cr commented Sep 8, 2013

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
georgiev commented Sep 9, 2013

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
Contributor

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
Contributor

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
Member

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
Contributor

+1 for @georgiev's comment...

@jeremy
Member
jeremy commented Sep 24, 2013
 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
dguti commented Sep 25, 2013

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
aaron commented Sep 25, 2013

@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
dguti commented Sep 27, 2013

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
Contributor

@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
Member
senny commented Jan 27, 2014

Added the change to the Rails 4.1 release notes: abe6484

@jaredbeck
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment