Skip to content

Commit

Permalink
Improve use of INNER/OUTER joins in AR Table backend
Browse files Browse the repository at this point in the history
  • Loading branch information
shioyama committed Apr 23, 2018
1 parent e166558 commit 2282934
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 19 deletions.
30 changes: 11 additions & 19 deletions lib/mobility/backends/active_record/table/query_methods.rb
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ def extended(relation)

def define_join_method(association_name, translation_class, foreign_key: nil, table_name: nil, **)
define_method :"join_#{association_name}" do |**options|
return self if joins_values.any? { |v| v.is_a?(Arel::Nodes::Join) && (v.left.name == table_name.to_s) }
if join = joins_values.find { |v| (Arel::Nodes::Join === v) && (v.left.name == table_name.to_s) }
return self if (options[:outer_join] || Arel::Nodes::InnerJoin === join)
self.joins_values = joins_values - [join]
end
t = translation_class.arel_table
m = arel_table
join_type = options[:outer_join] ? Arel::Nodes::OuterJoin : Arel::Nodes::InnerJoin
Expand All @@ -67,26 +70,15 @@ def define_query_methods(association_name, translation_class, **)
# When deciding whether to use an outer or inner join, array-valued
# conditions are treated as nil if they have any values.
#
# Article.where(title: nil, content: ["foo", nil]) #=> OUTER JOIN (all nil or array with nil)
# Article.where(title: "foo", content: ["foo", nil]) #=> INNER JOIN (one non-nil)
# Article.where(title: ["foo", "bar"], content: ["foo", nil]) #=> INNER JOIN (one non-nil array)
#
# Note that if you call `where` multiple times, you may end up with an
# outer join when a (faster) inner join would have worked fine:
#
# Article.where(title: nil).where(content: "foo") #=> OUTER JOIN
# Article.where(title: [nil, "foo"]).where(content: "foo") #=> OUTER JOIN
#
# In this case, we are searching for a match on the article_translations table
# which has a NULL title and a content equal to "foo". Since we need a positive
# match for content, there must be an English translation on the article, thus
# we can use an inner join. However, Mobility will use an outer join since we don't
# want to modify the existing relation which has already been joined.
# Article.where(title: nil, content: ["foo", nil]) #=> OUTER JOIN (all nil or array with nil)
# Article.where(title: "foo", content: ["foo", nil]) #=> INNER JOIN (one non-nil)
# Article.where(title: ["foo", "bar"], content: ["foo", nil]) #=> INNER JOIN (one non-nil array)
#
# To avoid this problem, simply make sure to either order your queries to place nil
# values last, or include all queried attributes in a single `where`:
# The logic also applies when a query has more than one where clause.
#
# Article.where(title: nil, content: "foo") #=> INNER JOIN
# Article.where(title: nil).where(content: nil) #=> OUTER JOIN (all nils)
# Article.where(title: nil).where(content: "foo") #=> INNER JOIN (one non-nil)
# Article.where(title: "foo").where(content: nil) #=> INNER JOIN (one non-nil)
#
define_method :where! do |opts, *rest|
if i18n_keys = q.extract_attributes(opts)
Expand Down
4 changes: 4 additions & 0 deletions spec/mobility/backends/active_record/table_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,11 @@
describe "joins" do
it "uses inner join for WHERE queries if query has at least one non-null attribute" do
expect(Article.i18n.where(title: "foo", content: nil).to_sql).not_to match(/OUTER/)
expect(Article.i18n.where(title: "foo").where(content: nil).to_sql).not_to match(/OUTER/)
expect(Article.i18n.where(content: nil).where(title: "foo").to_sql).not_to match(/OUTER/)
expect(Article.i18n.where(title: "foo", content: [nil, "bar"]).to_sql).not_to match(/OUTER/)
expect(Article.i18n.where(title: "foo").where(content: [nil, "bar"]).to_sql).not_to match(/OUTER/)
expect(Article.i18n.where(content: [nil, "bar"]).where(title: "foo").to_sql).not_to match(/OUTER/)
end

it "does not use OUTER JOIN with .not" do
Expand Down

0 comments on commit 2282934

Please sign in to comment.