From 96d0fd75ade6a6ca019ccc03cca6d876f0e12ecf Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Fri, 8 Jan 2016 16:18:24 -0800 Subject: [PATCH] Query for negative associations as subquery When a collection association (has_many, etc) is searched for negative conditions (NOT...), a JOIN will still include other rows that match. The implied meaning is that it should only select where *none* of the associations match, but the actual result still selects records where *any* of the joined associations match. This implementation removes joins that were added while building the conditions and moves them into a subquery if needed. --- lib/ransack/adapters/active_record/context.rb | 79 ++++++++++++++++++- .../adapters/active_record/ransack/context.rb | 2 + .../active_record/ransack/nodes/condition.rb | 38 ++++----- lib/ransack/nodes/attribute.rb | 4 + lib/ransack/nodes/condition.rb | 7 ++ .../adapters/active_record/base_spec.rb | 70 ++++++++++++++++ 6 files changed, 178 insertions(+), 22 deletions(-) diff --git a/lib/ransack/adapters/active_record/context.rb b/lib/ransack/adapters/active_record/context.rb index f1b0a3b..390664a 100644 --- a/lib/ransack/adapters/active_record/context.rb +++ b/lib/ransack/adapters/active_record/context.rb @@ -15,7 +15,6 @@ module Ransack def initialize(object, options = {}) super @arel_visitor = @engine.connection.visitor - @associations_pot = {} end def relation_for(object) @@ -139,6 +138,64 @@ module Ransack @join_dependency.alias_tracker end + def lock_association(association) + @lock_associations << association + end + + if ::ActiveRecord::VERSION::STRING >= Constants::RAILS_4_1 + def remove_association(association) + return if @lock_associations.include?(association) + @join_dependency.join_root.children.delete_if { |stashed| + stashed.eql?(association) + } + @object.joins_values.delete_if { |jd| + jd.join_root.children.map(&:object_id) == [association.object_id] + } + end + else + def remove_association(association) + return if @lock_associations.include?(association) + @join_dependency.join_parts.delete(association) + @object.joins_values.delete(association) + end + end + + # Build an Arel subquery that selects keys for the top query, + # drawn from the first join association's foreign_key. + # + # Example: for an Article that has_and_belongs_to_many Tags + # + # context = Article.search.context + # attribute = Attribute.new(context, "tags_name").tap do |a| + # context.bind(a, a.name) + # end + # context.build_correlated_subquery(attribute.parent).to_sql + # + # # SELECT "articles_tags"."article_id" FROM "articles_tags" + # # INNER JOIN "tags" ON "tags"."id" = "articles_tags"."tag_id" + # # WHERE "articles_tags"."article_id" = "articles"."id" + # + # The WHERE condition on this query makes it invalid by itself, + # because it is correlated to the primary key on the outer query. + # + def build_correlated_subquery(association) + join_constraints = extract_joins(association) + join_root = join_constraints.shift + join_table = join_root.left + correlated_key = join_root.right.expr.left + subquery = Arel::SelectManager.new(association.base_klass) + subquery.from(join_root.left) + subquery.project(correlated_key) + join_constraints.each do |j| + subquery.join_sources << Arel::Nodes::InnerJoin.new(j.left, j.right) + end + subquery.where(correlated_key.eq(primary_key)) + end + + def primary_key + @object.table[@object.primary_key] + end + private def database_table_exists? @@ -282,6 +339,21 @@ module Ransack found_association end + def extract_joins(association) + parent = @join_dependency.join_root + reflection = association.reflection + join_constraints = association.join_constraints( + parent.table, + parent.base_klass, + association, + Arel::Nodes::OuterJoin, + association.tables, + reflection.scope_chain, + reflection.chain + ) + join_constraints.to_a.flatten + end + else def build_association(name, parent = @base, klass = nil) @@ -297,6 +369,11 @@ module Ransack found_association end + def extract_joins(association) + query = Arel::SelectManager.new(association.base_klass, association.table) + association.join_to(query).join_sources + end + def find_association(name, parent = @base, klass = nil) found_association = @join_dependency.join_associations .detect do |assoc| diff --git a/lib/ransack/adapters/active_record/ransack/context.rb b/lib/ransack/adapters/active_record/ransack/context.rb index 12d75ae..708efea 100644 --- a/lib/ransack/adapters/active_record/ransack/context.rb +++ b/lib/ransack/adapters/active_record/ransack/context.rb @@ -27,6 +27,8 @@ module Ransack @join_dependency = join_dependency(@object) @join_type = options[:join_type] || Polyamorous::OuterJoin @search_key = options[:search_key] || Ransack.options[:search_key] + @associations_pot = {} + @lock_associations = [] if ::ActiveRecord::VERSION::STRING >= Constants::RAILS_4_1 @base = @join_dependency.join_root diff --git a/lib/ransack/adapters/active_record/ransack/nodes/condition.rb b/lib/ransack/adapters/active_record/ransack/nodes/condition.rb index b248bd3..e97ac5d 100644 --- a/lib/ransack/adapters/active_record/ransack/nodes/condition.rb +++ b/lib/ransack/adapters/active_record/ransack/nodes/condition.rb @@ -3,33 +3,29 @@ module Ransack class Condition def arel_predicate - if attributes.size > 1 - combinator_for_predicates - else - format_predicate - end + attributes.map { |attribute| + association = attribute.parent + if negative? && attribute.associated_collection? + query = context.build_correlated_subquery(association) + query.where(format_predicate(attribute).not) + context.remove_association(association) + Arel::Nodes::NotIn.new(context.primary_key, Arel.sql(query.to_sql)) + else + format_predicate(attribute) + end + }.reduce(combinator_method) end private - def arel_predicates - attributes.map do |a| - a.attr.send( - arel_predicate_for_attribute(a), formatted_values_for_attribute(a) - ) - end + def combinator_method + combinator === Constants::OR ? :or : :and end - def combinator_for_predicates - if combinator === Constants::AND - Arel::Nodes::Grouping.new(arel_predicates.inject(&:and)) - elsif combinator === Constants::OR - arel_predicates.inject(&:or) - end - end - - def format_predicate - predicate = arel_predicates.first + def format_predicate(attribute) + arel_pred = arel_predicate_for_attribute(attribute) + arel_values = formatted_values_for_attribute(attribute) + predicate = attribute.attr.public_send(arel_pred, arel_values) if casted_array_with_in_predicate?(predicate) predicate.right[0] = format_values_for(predicate.right[0]) end diff --git a/lib/ransack/nodes/attribute.rb b/lib/ransack/nodes/attribute.rb index 3183a3d..0eccd46 100644 --- a/lib/ransack/nodes/attribute.rb +++ b/lib/ransack/nodes/attribute.rb @@ -24,6 +24,10 @@ module Ransack .include?(attr_name.split('.').last) end + def associated_collection? + parent.respond_to?(:reflection) && parent.reflection.collection? + end + def type if ransacker return ransacker.type diff --git a/lib/ransack/nodes/condition.rb b/lib/ransack/nodes/condition.rb index 1fe3f28..09eb8c1 100644 --- a/lib/ransack/nodes/condition.rb +++ b/lib/ransack/nodes/condition.rb @@ -131,6 +131,9 @@ module Ransack Attribute.new(@context, name, ransacker_args).tap do |attribute| @context.bind(attribute, attribute.name) self.attributes << attribute if attribute.valid? + if predicate && !negative? + @context.lock_association(attribute.parent) + end end end @@ -182,6 +185,10 @@ module Ransack def predicate_name=(name) self.predicate = Predicate.named(name) + unless negative? + attributes.each { |a| context.lock_association(a.parent) } + end + @predicate end alias :p= :predicate_name= diff --git a/spec/ransack/adapters/active_record/base_spec.rb b/spec/ransack/adapters/active_record/base_spec.rb index c9d3e97..8220746 100644 --- a/spec/ransack/adapters/active_record/base_spec.rb +++ b/spec/ransack/adapters/active_record/base_spec.rb @@ -94,6 +94,76 @@ module Ransack end end + context 'negative conditions on HABTM associations' do + let(:medieval) { Tag.create!(name: 'Medieval') } + let(:fantasy) { Tag.create!(name: 'Fantasy') } + let(:arthur) { Article.create!(title: 'King Arthur') } + let(:marco) { Article.create!(title: 'Marco Polo') } + + before do + marco.tags << medieval + arthur.tags << medieval + arthur.tags << fantasy + end + + it 'removes redundant joins from top query' do + s = Article.ransack(tags_name_not_eq: "Fantasy") + sql = s.result.to_sql + + expect(sql).to_not include('LEFT OUTER JOIN') + end + + it 'handles != for single values' do + s = Article.ransack(tags_name_not_eq: "Fantasy") + articles = s.result.to_a + + expect(articles).to include marco + expect(articles).to_not include arthur + end + + it 'handles NOT IN for multiple attributes' do + s = Article.ransack(tags_name_not_in: ["Fantasy", "Scifi"]) + articles = s.result.to_a + + expect(articles).to include marco + expect(articles).to_not include arthur + end + end + + context 'negative conditions on self-referenced associations' do + let(:pop) { Person.create!(name: 'Grandpa') } + let(:dad) { Person.create!(name: 'Father') } + let(:mom) { Person.create!(name: 'Mother') } + let(:son) { Person.create!(name: 'Grandchild') } + + before do + son.parent = dad + dad.parent = pop + dad.children << son + mom.children << son + pop.children << dad + son.save! && dad.save! && mom.save! && pop.save! + end + + it 'handles multiple associations and aliases' do + s = Person.ransack( + c: { + '0' => { a: ['name'], p: 'not_eq', v: ['Father'] }, + '1' => { + a: ['children_name', 'parent_name'], + p: 'not_eq', v: ['Father'], m: 'or' + }, + '2' => { a: ['children_salary'], p: 'eq', v: [nil] } + }) + people = s.result + + expect(people.to_a).to include son + expect(people.to_a).to include mom + expect(people.to_a).to_not include dad # rule '0': 'name' + expect(people.to_a).to_not include pop # rule '1': 'children_name' + end + end + describe '#ransack_alias' do it 'translates an alias to the correct attributes' do p = Person.create!(name: 'Meatloaf', email: 'babies@example.com')