Skip to content

Loading…

find_or_create_ fails on many-to-many association #5325

Closed
yjchen opened this Issue · 20 comments

10 participants

@yjchen

I have a model Photo and Face with many-to-many association like this:

class Photo < ActiveRecord::Base
  belongs_to :user
  has_many :ownerships
  has_many :faces, :through => :ownerships
end
class Face < ActiveRecord::Base
  has_many :ownerships
  has_many :photos, :through => :ownerships
end
class Ownership < ActiveRecord::Base
  belongs_to :face
  belongs_to :photo
end

Use find_or_create create associated object twice:

1.9.3-p125 :003 > photo.faces.find_or_create_by_name('Tim')
  Face Load (1.1ms)  SELECT "faces".* FROM "faces" INNER JOIN "ownerships" ON "faces"."id" = "ownerships"."face_id" WHERE "ownerships"."photo_id" = 1 AND "faces"."name" = 'Tim' LIMIT 1
   (0.1ms)  begin transaction
  SQL (42.5ms)  INSERT INTO "faces" ("created_at", "name", "updated_at") VALUES (?, ?, ?)  [["created_at", Wed, 07 Mar 2012 14:01:41 UTC +00:00], ["name", "Tim"], ["updated_at", Wed, 07 Mar 2012 14:01:41 UTC +00:00]]
   (3.0ms)  commit transaction
  Face Load (0.4ms)  SELECT "faces".* FROM "faces" INNER JOIN "ownerships" ON "faces"."id" = "ownerships"."face_id" WHERE "ownerships"."photo_id" = 1 AND "faces"."name" = 'Tim' LIMIT 1
   (0.1ms)  begin transaction
  SQL (0.6ms)  INSERT INTO "faces" ("created_at", "name", "updated_at") VALUES (?, ?, ?)  [["created_at", Wed, 07 Mar 2012 14:01:41 UTC +00:00], ["name", "Tim"], ["updated_at", Wed, 07 Mar 2012 14:01:41 UTC +00:00]]
   (2.7ms)  commit transaction
 => #<Face id: 5, name: "Tim", created_at: "2012-03-07 14:01:41", updated_at: "2012-03-07 14:01:41"> 

This runs on Rails 3.2.1.

@ecoffey

I can reproduce this against v3.2.1, but not v3.2.2, so it looks like a simple version bump should fix this for you.

@kennyj

Hi @yjchen

Could you test this issue with rails 3.2.2 ? :-)

@yjchen

I check with 3.2.2 and it is flxed. Thanx for this information.

@yjchen yjchen closed this
@yjchen yjchen reopened this
@yjchen

Sorry to reopen this bug, but I found something more weird.

Now, photo.faces.find_or_create_by_name('Tim') indeed creates a new face to photo, and use photo.faces indeed find the correct faces, but the association is not saved.

To reproduce this bug, try this:

1.9.3-p125 :022 > Ownership.destory_all #clean data
1.9.3-p125 :022 > Face.destroy_all #clean data
1.9.3-p125 :022 > photo = Photo.first
  Photo Load (0.4ms)  SELECT "photos".* FROM "photos" LIMIT 1 => #<Photo id: 1, title: "ocean", user_id: 1, created_at: "2012-03-07 09:59:44", updated_at: "2012-03-07 09:59:44"> 
1.9.3-p125 :022 > f = photo.faces.find_or_create_by_name('Tim')
  Face Load (0.4ms)  SELECT "faces".* FROM "faces" INNER JOIN "ownerships" ON "faces"."id" = "ownerships"."face_id" WHERE "ownerships"."photo_id" = 1 AND "faces"."name" = 'Tim' LIMIT 1
   (0.1ms)  begin transaction
  SQL (0.7ms)  INSERT INTO "faces" ("created_at", "name", "updated_at") VALUES (?, ?, ?)  [["created_at", Tue, 13 Mar 2012 06:24:38 UTC +00:00], ["name", "Tim"], ["updated_at", Tue, 13 Mar 2012 06:24:38 UTC +00:00]]
   (2.0ms)  commit transaction
1.9.3-p125 :022 > photo.faces
  Face Load (0.4ms)  SELECT "faces".* FROM "faces" INNER JOIN "ownerships" ON "faces"."id" = "ownerships"."face_id" WHERE "ownerships"."photo_id" = 1
 => [#<Face id: 8, name: "Tim", created_at: "2012-03-13 06:24:38", updated_at: "2012-03-13 06:24:38">] 
1.9.3-p125 :022 > f.persisted?
 => true 
1.9.3-p125 :023 > Ownership.all
  Ownership Load (0.4ms)  SELECT "ownerships".* FROM "ownerships" 
 => [] 

In another word, the association is not saved in the Ownership even though photo.faces returned correct records. Is it a correct behavior for many-to-many association ?

@jeremyf

@yjchen With Rails 3.2.3 is this still a problem? Would you be able to write a test case in the activerecord test suite that fails? I might be able to help you get it working then.

(@jeremyf)

@yjchen

It still exists on Rails 3.2.3. I create a rails application for demonstration at https://github.com/yjchen/rails_bug. The test suit is at test/unit/ownership_test.rb.

@rafaelfranca
Ruby on Rails member

@yjchen could you check this with first_or_create?

where(:name => 'Tim').first_or_create
@yjchen

where(:name => 'Tim').first_or_create still doesn't work. It is tested against 3.2.5 now. Here is the result from rails console:

f = photo.faces.where(:name => 'Tim').first_or_create
  Face Load (0.3ms)  SELECT "faces".* FROM "faces" INNER JOIN "ownerships" ON "faces"."id" = "ownerships"."face_id" WHERE "ownerships"."photo_id" = 1 AND "faces"."name" = 'Tim' LIMIT 1
   (0.1ms)  begin transaction
  SQL (11.2ms)  INSERT INTO "faces" ("created_at", "name", "updated_at") VALUES (?, ?, ?)  [["created_at", Thu, 21 Jun 2012 00:56:14 UTC +00:00], ["name", "Tim"], ["updated_at", Thu, 21 Jun 2012 00:56:14 UTC +00:00]]
   (11.5ms)  commit transaction
 => #<Face id: 2, name: "Tim", created_at: "2012-06-21 00:56:14", updated_at: "2012-06-21 00:56:14"> 
@senny
Ruby on Rails member

just verified and this issue is also present on rails/master.

@parndt

Has this previously worked on any version of Rails? Does anybody know?

@al2o3cr

This looks like another variant of #6161 - can you try adding the :inverse_of option to the associations as discussed there?

@yjchen

Nope. Here is the code I used to test:

      class Photo < ActiveRecord::Base
        attr_accessible :name
        has_many :ownerships, :inverse_of => :photo
        has_many :faces, :through => :ownerships
      end
      class Face < ActiveRecord::Base
        attr_accessible :name
        has_many :ownerships, :inverse_of => :face
        has_many :photos, :through => :ownerships
      end
      class Ownership < ActiveRecord::Base
        belongs_to :face, :inverse_of => :ownerships
        belongs_to :photo, :inverse_of => :ownerships
        # attr_accessible :title, :body
      end

And the result is the same:

1.9.3p125 :001 > photo = Photo.first
  Photo Load (0.1ms)  SELECT "photos".* FROM "photos" LIMIT 1
 => #<Photo id: 2, name: "Bob", created_at: "2012-12-06 01:14:38", updated_at: "2012-12-06 01:14:38"> 
1.9.3p125 :002 > f = photo.faces.find_or_create_by_name('Tim')
  Face Load (0.1ms)  SELECT "faces".* FROM "faces" INNER JOIN "ownerships" ON "faces"."id" = "ownerships"."face_id" WHERE "ownerships"."photo_id" = 2 AND "faces"."name" = 'Tim' LIMIT 1
   (0.1ms)  begin transaction
  SQL (3.4ms)  INSERT INTO "faces" ("created_at", "name", "updated_at") VALUES (?, ?, ?)  [["created_at", Thu, 06 Dec 2012 01:25:46 UTC +00:00], ["name", "Tim"], ["updated_at", Thu, 06 Dec 2012 01:25:46 UTC +00:00]]
   (56.7ms)  commit transaction
 => #<Face id: 8, name: "Tim", created_at: "2012-12-06 01:25:46", updated_at: "2012-12-06 01:25:46"> 
1.9.3p125 :003 > f.persisted?
 => true 
1.9.3p125 :004 > Ownership.all
  Ownership Load (0.3ms)  SELECT "ownerships".* FROM "ownerships" 
 => [] 

And I notice that after creating the face by find_or_create, photo.faces.first return nil. But after running photo.faces, photo.faces.first return the correct record:

1.9.3p125 :001 > photo = Photo.first
  Photo Load (0.1ms)  SELECT "photos".* FROM "photos" LIMIT 1
 => #<Photo id: 2, name: "Bob", created_at: "2012-12-06 01:14:38", updated_at: "2012-12-06 01:14:38"> 
1.9.3p125 :002 > f = photo.faces.find_or_create_by_name('Tim')
  Face Load (0.1ms)  SELECT "faces".* FROM "faces" INNER JOIN "ownerships" ON "faces"."id" = "ownerships"."face_id" WHERE "ownerships"."photo_id" = 2 AND "faces"."name" = 'Tim' LIMIT 1
   (0.1ms)  begin transaction
  SQL (3.5ms)  INSERT INTO "faces" ("created_at", "name", "updated_at") VALUES (?, ?, ?)  [["created_at", Thu, 06 Dec 2012 01:30:14 UTC +00:00], ["name", "Tim"], ["updated_at", Thu, 06 Dec 2012 01:30:14 UTC +00:00]]
   (52.7ms)  commit transaction
 => #<Face id: 10, name: "Tim", created_at: "2012-12-06 01:30:14", updated_at: "2012-12-06 01:30:14"> 
1.9.3p125 :003 > photo.faces.first
  Face Load (0.4ms)  SELECT "faces".* FROM "faces" INNER JOIN "ownerships" ON "faces"."id" = "ownerships"."face_id" WHERE "ownerships"."photo_id" = 2 LIMIT 1
 => nil 
1.9.3p125 :004 > photo.faces
  Face Load (0.2ms)  SELECT "faces".* FROM "faces" INNER JOIN "ownerships" ON "faces"."id" = "ownerships"."face_id" WHERE "ownerships"."photo_id" = 2
 => [#<Face id: 10, name: "Tim", created_at: "2012-12-06 01:30:14", updated_at: "2012-12-06 01:30:14">] 
1.9.3p125 :005 > photo.faces.first
 => #<Face id: 10, name: "Tim", created_at: "2012-12-06 01:30:14", updated_at: "2012-12-06 01:30:14"> 

I guess something is cached in memory and is not retrieved correctly.

@senny
Ruby on Rails member

I created a failing test-case:

diff --git a/activerecord/test/cases/relations_test.rb b/activerecord/test/cases/relations_test.rb
index 8298d75..d954e91 100644
--- a/activerecord/test/cases/relations_test.rb
+++ b/activerecord/test/cases/relations_test.rb
@@ -5,6 +5,8 @@ require 'models/post'
 require 'models/topic'
 require 'models/comment'
 require 'models/author'
+require 'models/category'
+require 'models/categorization'
 require 'models/entrant'
 require 'models/developer'
 require 'models/reply'
@@ -1052,6 +1054,17 @@ class RelationTest < ActiveRecord::TestCase
     assert_raises(ActiveRecord::RecordInvalid) { Bird.where(:color => 'green').first_or_create!([ {:name => 'parrot'}, {:pirate_id => 1} ]) }
   end

+  def test_first_or_create_on_has_many_through_also_creates_join_model
+    category = authors(:david).categories.where(name: 'IT').first_or_create
+
+    categories = Category.all.map(&:name)
+    davids_categories = authors(:david).categories.map(&:name)
+
+    assert category.persisted?
+    assert categories.include?('IT'), "expected #{davids_categories} to include 'IT' (missing model)"
+    assert davids_categories.include?('IT'), "expected #{davids_categories} to include 'IT' (missing join model)"
+  end
+
   def test_first_or_initialize
     parrot = Bird.where(:color => 'green').first_or_initialize(:name => 'parrot')
     assert_kind_of Bird, parrot

I'm not aware of a good way to fix this problem. As the first_or_create methods only create the target objects but no join models. This could be a situation that we need to resolve with documentation rather than supporting the feature. If someone has a good idea just submit a PR though ;)

@carlosantoniodasilva @rafaelfranca what do you think?

@neerajdotname
Ruby on Rails member

This is duplicate of #9994 . I sent a PR with the fix.

Just want to clarify that since the issue was opened the style that is causing problem is now deprecated.

f = photo.faces.find_or_create_by_name('Tim') # deprecated and is buggy. PR is to fix this
f = photo.faces.find_or_create_by(name: 'Tim') # already works in master
@rafaelfranca
Ruby on Rails member

This was fixed at master.

@williscool

I'm having this issue with first_or_initialize in the rails 4 beta

song = @musician.songs.where(name: song_name).first_or_initialize

if you save the song it wont be assocaited with the musician

on the other hand

song = @musician.songs.where(name: song_name).first_or_initialize
song.musicians << @musician

works as expected

@neerajdotname
Ruby on Rails member

@williscool if it is a has_many relationship using :through then please try it with rails master. There was an issue similar to you mentioned and the fix might be there in master. If not I'll take a look.

If the problem persists then please post a gist as per the section 1.2 of Rails guide http://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#create-a-self-contained-gist-for-active-record-issues?

@williscool

didnt work on master but being on master broke a whole lot of other things lol.

I'm on a deadline for this thing right now so I'll just use the workaround for now

but I'll make the test gist once I get some time soon

@neerajdotname
Ruby on Rails member

@williscool I just confirmed that this is still an issue in master. I'm working on a fix. Thank you.

@williscool

Awesome! and to clarify it is a has many through.

class Musician < ActiveRecord::Base
  has_many :song_musicians 
  has_many :songs, :through => :song_musicians 
end

class SongMusician < ActiveRecord::Base
  belongs_to :song
  belongs_to :musician
end

class Song < ActiveRecord::Base
  has_many :song_musicians 
  has_many :musicians , :through => :song_musicians 
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.