building a has_many :through with :conditions #161

Closed
the8472 opened this Issue Aug 30, 2012 · 7 comments

Comments

Projects
None yet
3 participants
@the8472

the8472 commented Aug 30, 2012

I think i've run into a bug:

class A < ActiveRecord::Base
  has_many :a_b_mappings
  has_many :b_assoc, :through => :a_b_mappings, :conditions => {:context => "foo"}
end


puts A.last.b_assoc.build
#<B id: nil, context: nil>

context should be "foo".

After stepping through the code I found that find_equality_predicates in the 3.1/relation-extensions.rb only checks for arel nodes but the nodes array which is passed as argument actually contains 2 equality nodes and a raw sql string: "b"."context" = 'foo', which obviously is the condition we actually want.

@ernie

This comment has been minimized.

Show comment Hide comment
@ernie

ernie Aug 30, 2012

Member

Hm. What version of Rails/Squeel? Have you tried against Squeel master?

Does stock AR behave "correctly" in this situation? It seems odd to me, since creating a b_assoc devoid of its a_b_mapping context would be unable to correctly populate other essential values, like a b_id to relate back to a_b_mappings.

Member

ernie commented Aug 30, 2012

Hm. What version of Rails/Squeel? Have you tried against Squeel master?

Does stock AR behave "correctly" in this situation? It seems odd to me, since creating a b_assoc devoid of its a_b_mapping context would be unable to correctly populate other essential values, like a b_id to relate back to a_b_mappings.

@the8472

This comment has been minimized.

Show comment Hide comment
@the8472

the8472 Aug 30, 2012

  • rails 3.2.8
  • ruby 1.9.3
  • squeel 1.0.9
  • haven't tried master.

As for the other essential values: :inverse_of in the join table relations on B and ABMappings allows the association ids to be populated, which already works. Rails does some magic by traversing the chain of through associations to fetch all the conditions there, so it should work in vanilla rails. See http://stackoverflow.com/a/12159307 for an example. Although I have yet have to test it myself.

the8472 commented Aug 30, 2012

  • rails 3.2.8
  • ruby 1.9.3
  • squeel 1.0.9
  • haven't tried master.

As for the other essential values: :inverse_of in the join table relations on B and ABMappings allows the association ids to be populated, which already works. Rails does some magic by traversing the chain of through associations to fetch all the conditions there, so it should work in vanilla rails. See http://stackoverflow.com/a/12159307 for an example. Although I have yet have to test it myself.

@ernie

This comment has been minimized.

Show comment Hide comment
@ernie

ernie Aug 30, 2012

Member

Maybe. I'm suspicious that it won't work, after reading the Rails source, which works very similarly to my implementation, minus the lazy evaluation of hashes. That being said, I haven't set up a test to try.

Member

ernie commented Aug 30, 2012

Maybe. I'm suspicious that it won't work, after reading the Rails source, which works very similarly to my implementation, minus the lazy evaluation of hashes. That being said, I haven't set up a test to try.

@the8472

This comment has been minimized.

Show comment Hide comment
@the8472

the8472 Aug 30, 2012

Seems like you're right, with vanilla rails 3.2.8:

class A < ActiveRecord::Base
  has_many :a_b_mappings, :inverse_of => :a
  has_many :b, :through => :a_b_mappings, :source => :b, :conditions => {:context => "foo"}
end

class B < ActiveRecord::Base
  has_many :a_b_mappings, :inverse_of => :b
end

class ABMapping < ActiveRecord::Base
  self.table_name = :a_b_mappings
  belongs_to :a, :inverse_of => :a_b_mappings
  belongs_to :b, :inverse_of => :a_b_mappings
end


puts A.first.b.create.tap{|b| puts b.inspect}.a_b_mappings.inspect
# #<B id: 3, context: nil> 
# [#<ABMapping id: 2, a_id: 1, b_id: 2>]

It creates the through-association but does not set the defaults. Meh. And I bet this will never get fixed in rails.

the8472 commented Aug 30, 2012

Seems like you're right, with vanilla rails 3.2.8:

class A < ActiveRecord::Base
  has_many :a_b_mappings, :inverse_of => :a
  has_many :b, :through => :a_b_mappings, :source => :b, :conditions => {:context => "foo"}
end

class B < ActiveRecord::Base
  has_many :a_b_mappings, :inverse_of => :b
end

class ABMapping < ActiveRecord::Base
  self.table_name = :a_b_mappings
  belongs_to :a, :inverse_of => :a_b_mappings
  belongs_to :b, :inverse_of => :a_b_mappings
end


puts A.first.b.create.tap{|b| puts b.inspect}.a_b_mappings.inspect
# #<B id: 3, context: nil> 
# [#<ABMapping id: 2, a_id: 1, b_id: 2>]

It creates the through-association but does not set the defaults. Meh. And I bet this will never get fixed in rails.

@alan-andrade

This comment has been minimized.

Show comment Hide comment
@alan-andrade

alan-andrade May 22, 2013

This means I can't use sifters in the condition value ?

This means I can't use sifters in the condition value ?

@the8472

This comment has been minimized.

Show comment Hide comment
@the8472

the8472 May 23, 2013

Conditions should only be simple hashes to allow rails to build the join models

the8472 commented May 23, 2013

Conditions should only be simple hashes to allow rails to build the join models

@alan-andrade

This comment has been minimized.

Show comment Hide comment
@alan-andrade

alan-andrade May 23, 2013

👍 thank you guys! I consider squeel a majestic gem. Kudooosss!

👍 thank you guys! I consider squeel a majestic gem. Kudooosss!

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