Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP
Browse files

While merging relations preserve context for joins

Fixes #3002. Also see #5494.

```
class Comment < ActiveRecord::Base
  belongs_to :post
end

class Author < ActiveRecord::Base
  has_many :posts
end

class Post < ActiveRecord::Base
  belongs_to :author
  has_many :comments
end
```

`Comment.joins(:post).merge(Post.joins(:author).merge(Author.where(:name => "Joe Blogs"))).all` would
fail with `ActiveRecord::ConfigurationError: Association named 'author' was not found on Comment`.

It is failing because `all` is being called on relation which looks like this after all the merging:
`{:joins=>[:post, :author], :where=>[#<Arel::Nodes::Equality: ....}`. In this relation all the context that
`Post` was joined with `Author` is lost and hence the error that `author` was not found on `Comment`.

Ths solution is to build JoinAssociation when two relations with join information are being merged. And later
while building the arel use the  previously built `JoinAssociation` record in `JoinDependency#graft` to
build the right from clause.

Thanks to Jared Armstrong (https://github.com/armstrjare) for most of the work. I ported it to make it
compatible with new code base.
  • Loading branch information...
commit dc764fcc348d562376add26ff8ef5173946b575b 1 parent bb61d2d
@neerajdotname neerajdotname authored
View
32 activerecord/CHANGELOG.md
@@ -1,5 +1,37 @@
## Rails 4.0.0 (unreleased) ##
+* Preserve context while merging relations with join information.
+
+ class Comment < ActiveRecord::Base
+ belongs_to :post
+ end
+
+ class Author < ActiveRecord::Base
+ has_many :posts
+ end
+
+ class Post < ActiveRecord::Base
+ belongs_to :author
+ has_many :comments
+ end
+
+ `Comment.joins(:post).merge(Post.joins(:author).merge(Author.where(:name => "Joe Blogs"))).all`
+ would fail with
+ `ActiveRecord::ConfigurationError: Association named 'author' was not found on Comment`.
+
+ It is failing because `all` is being called on relation which looks like this after all
+ the merging: `{:joins=>[:post, :author], :where=>[#<Arel::Nodes::Equality: ....}`. In this
+ relation all the context that `Post` was joined with `Author` is lost and hence the error
+ that `author` was not found on `Comment`.
+
+ Ths solution is to build JoinAssociation when two relations with join information are being
+ merged. And later while building the arel use the previously built `JoinAssociation` record
+ in `JoinDependency#graft` to build the right from clause.
+
+ Fixes #3002.
+
+ *Jared Armstrong and Neeraj Singh*
+
* `default_scopes?` is deprecated. Check for `default_scopes.empty?` instead.
*Agis Anastasopoulos*
View
7 activerecord/lib/active_record/associations/join_dependency/join_association.rb
@@ -55,7 +55,12 @@ def ==(other)
def find_parent_in(other_join_dependency)
other_join_dependency.join_parts.detect do |join_part|
- parent == join_part
+ case parent
+ when JoinBase
+ parent.base_klass == join_part.base_klass
+ else
+ parent == join_part
+ end
end
end
View
32 activerecord/lib/active_record/relation/merger.rb
@@ -39,7 +39,7 @@ def other
end
class Merger # :nodoc:
- attr_reader :relation, :values
+ attr_reader :relation, :values, :other
def initialize(relation, other)
if other.default_scoped? && other.klass != relation.klass
@@ -48,11 +48,12 @@ def initialize(relation, other)
@relation = relation
@values = other.values
+ @other = other
end
NORMAL_VALUES = Relation::SINGLE_VALUE_METHODS +
Relation::MULTI_VALUE_METHODS -
- [:where, :order, :bind, :reverse_order, :lock, :create_with, :reordering, :from] # :nodoc:
+ [:joins, :where, :order, :bind, :reverse_order, :lock, :create_with, :reordering, :from] # :nodoc:
def normal_values
NORMAL_VALUES
@@ -66,12 +67,39 @@ def merge
merge_multi_values
merge_single_values
+ merge_joins
relation
end
private
+ def merge_joins
+ return if values[:joins].blank?
+
+ if other.klass == relation.klass
+ relation.joins! *values[:joins]
+ else
+ joins_dependency, rest = values[:joins].partition do |join|
+ case join
+ when Hash, Symbol, Array
+ true
+ else
+ false
+ end
+ end
+
+ join_dependency = ActiveRecord::Associations::JoinDependency.new(other.klass,
+ joins_dependency,
+ [])
+ relation.joins! rest
+
+ join_dependency.join_associations.each do |association|
+ @relation = association.join_relation(relation)
+ end
+ end
+ end
+
def merge_multi_values
relation.where_values = merged_wheres
relation.bind_values = merged_binds
View
11 activerecord/test/cases/relation_test.rb
@@ -1,10 +1,12 @@
require "cases/helper"
require 'models/post'
require 'models/comment'
+require 'models/author'
+require 'models/rating'
module ActiveRecord
class RelationTest < ActiveRecord::TestCase
- fixtures :posts, :comments
+ fixtures :posts, :comments, :authors
class FakeKlass < Struct.new(:table_name, :name)
end
@@ -176,6 +178,13 @@ def test_references_values_dont_duplicate
relation.merge!(where: ['foo = ?', 'bar'])
assert_equal ['foo = bar'], relation.where_values
end
+
+ def test_relation_merging_with_merged_joins
+ special_comments_with_ratings = SpecialComment.joins(:ratings)
+ posts_with_special_comments_with_ratings = Post.group("posts.id").joins(:special_comments).merge(special_comments_with_ratings)
+ assert_equal 3, authors(:david).posts.merge(posts_with_special_comments_with_ratings).to_a.length
+ end
+
end
class RelationMutationTest < ActiveSupport::TestCase
Please sign in to comment.
Something went wrong with that request. Please try again.