Through association condition clobbers join condition #1797

Merged
merged 1 commit into from Jul 11, 2011

Projects

None yet

4 participants

@kuahyeow
Contributor

For a through association with a condition, e.g.

has_many :misc_tags, :through => :taggings, :source => :tag, :conditions => "tags.name = 'Misc'"
Post.joins(:misc_tags)
Post.all(:joins => :misc_tags)

It's outputting the following SQL

SELECT "posts".* FROM "posts" INNER JOIN "taggings" ON "posts"."id" = "taggings"."taggable_id" AND "taggings"."taggable_type" = 'Post' INNER JOIN "tags" ON tags.name = 'Misc' WHERE "posts"."id" = 1 AND (taggings.tag_id != tags.id)

when I expect

SELECT "posts".* FROM "posts" INNER JOIN "taggings" ON "posts"."id" = "taggings"."taggable_id" AND "taggings"."taggable_type" = 'Post' INNER JOIN "tags"  ON "tags"."id" = "taggings"."tag_id" AND tags.name = 'Misc' WHERE "posts"."id" = 1 AND (taggings.tag_id != tags.id)

i.e. @ ON "tags"."id" = "taggings"."tag_id" @ is missing. See failing test too.

It looks like a regression from 2.x. I've tried the test on master though and it seems to be working there

@dmathieu dmathieu and 1 other commented on an outdated diff Jun 21, 2011
...es/associations/has_many_through_associations_test.rb
@@ -482,6 +482,13 @@ class HasManyThroughAssociationsTest < ActiveRecord::TestCase
assert_equal [posts(:eager_other)], posts
end
+ def test_join_on_has_many_association_collection_with_conditions
+ posts(:welcome).tags.create!(:name => 'Misc')
+ posts = Post.joins(:misc_tags).where('posts.id' => posts(:welcome).id).where('taggings.tag_id != tags.id')
+ puts posts.to_sql
@dmathieu
dmathieu Jun 21, 2011 Contributor

This should be removed.

@kuahyeow
kuahyeow Jun 21, 2011 Contributor

thanks, removed

@spastorino
Member

Can you please squash the commits in just one?, thanks.

@kuahyeow
Contributor

Ah, nice, I wasn't sure Github could handle that

@jake3030 jake3030 pushed a commit to jake3030/rails that referenced this pull request Jun 28, 2011
@dubek @NZKoz dubek + NZKoz Mysql#reconnect is set according to the 'reconnect' key in the connec…
…tion spec.

The 'reconenct' boolean option is read from the connection specification
and is used to set the reconnect attribute of Mysql.  The default is
false in order not to change existing application behaviour.

Also, reconnect is set AFTER real_connect is called, so its value sticks
(the mysql gem sets reconnect to false inside real_connect).

Signed-off-by: Michael Koziarski <michael@koziarski.com>
[#1797 state:committed]
5fe6635
@kuahyeow
Contributor

I have updated my patch so that building of the join conditions are dealt with in each sub-case - as each different sub-case really expected different array sizes to return.

Tests are running successfully. Please let me know if the patch is ok.

@spastorino
Member
@jonleighton jonleighton and 1 other commented on an outdated diff Jul 11, 2011
activerecord/lib/active_record/associations.rb
@@ -2151,23 +2151,27 @@ module ActiveRecord
join_table = Arel::Table.new(options[:join_table], :as => aliased_join_table_name, :engine => arel_engine)
fk = options[:foreign_key] || reflection.active_record.to_s.foreign_key
klass_fk = options[:association_foreign_key] || klass.to_s.foreign_key
+ as_conditions = reflection.options[:conditions] && process_conditions(reflection.options[:conditions], aliased_table_name)
@jonleighton
jonleighton Jul 11, 2011 Member

Why is this repeated in several places? Can you not just set the variable outside of the case statement? (Similar comment for jt_as_conditions outside the if/else.)

@kuahyeow
kuahyeow Jul 11, 2011 Contributor

True, that can be dryed up. I have done so in the new patch below.

@kuahyeow kuahyeow Fix for 3-0-stable - Conditions specified on through association shou…
…ldn't clobber asssociation join condition.

This fix refactors processing of association join conditions so that both the join condition and the custom condition will be used when called by query_methods.rb, which expects a 1 or 2-sized array (depending on the type of association). Previously, a custom condition specified would create a 2 or 3-sized array which will clobber the association join condition.
caec639
@jonleighton jonleighton merged commit fc4bce1 into rails:3-0-stable Jul 11, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment