From 235eae3b825e16a6a02e37423b2d24f6cda97da5 Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Sat, 2 Jan 2016 10:27:52 -0800 Subject: [PATCH 01/11] Refactor: condition handles binding of its attribute --- .../adapters/active_record/ransack/context.rb | 2 +- lib/ransack/context.rb | 1 + lib/ransack/nodes/attribute.rb | 1 - lib/ransack/nodes/condition.rb | 15 +++++++-------- lib/ransack/nodes/sort.rb | 2 +- 5 files changed, 10 insertions(+), 11 deletions(-) diff --git a/lib/ransack/adapters/active_record/ransack/context.rb b/lib/ransack/adapters/active_record/ransack/context.rb index eba9539ab..12d75aead 100644 --- a/lib/ransack/adapters/active_record/ransack/context.rb +++ b/lib/ransack/adapters/active_record/ransack/context.rb @@ -40,7 +40,7 @@ def initialize(object, options = {}) @base.table_name, as: @base.aliased_table_name, type_caster: self ) @bind_pairs = Hash.new do |hash, key| - parent, attr_name = get_parent_and_attribute_name(key.to_s) + parent, attr_name = get_parent_and_attribute_name(key) if parent && attr_name hash[key] = [parent, attr_name] end diff --git a/lib/ransack/context.rb b/lib/ransack/context.rb index 8be64d627..5f012bfba 100644 --- a/lib/ransack/context.rb +++ b/lib/ransack/context.rb @@ -58,6 +58,7 @@ def scope_arity(scope) end def bind(object, str) + return nil unless str object.parent, object.attr_name = @bind_pairs[str] end diff --git a/lib/ransack/nodes/attribute.rb b/lib/ransack/nodes/attribute.rb index 0b2fc3b1f..3183a3dd0 100644 --- a/lib/ransack/nodes/attribute.rb +++ b/lib/ransack/nodes/attribute.rb @@ -16,7 +16,6 @@ def initialize(context, name = nil, ransacker_args = []) def name=(name) @name = name - context.bind(self, name) unless name.blank? end def valid? diff --git a/lib/ransack/nodes/condition.rb b/lib/ransack/nodes/condition.rb index 5bd3982af..ab09ecf4a 100644 --- a/lib/ransack/nodes/condition.rb +++ b/lib/ransack/nodes/condition.rb @@ -79,14 +79,12 @@ def attributes def attributes=(args) case args when Array - args.each do |attr| - attr = Attribute.new(@context, attr) - self.attributes << attr if attr.valid? + args.each do |name| + build_attribute(name) end when Hash args.each do |index, attrs| - attr = Attribute.new(@context, attrs[:name], attrs[:ransacker_args]) - self.attributes << attr if attr.valid? + build_attribute(attrs[:name], attrs[:ransacker_args]) end else raise ArgumentError, @@ -129,9 +127,10 @@ def combinator=(val) alias :m= :combinator= alias :m :combinator - def build_attribute(name = nil) - Attribute.new(@context, name).tap do |attribute| - self.attributes << attribute + def build_attribute(name = nil, ransacker_args = []) + Attribute.new(@context, name, ransacker_args).tap do |attribute| + @context.bind(attribute, attribute.name) + self.attributes << attribute if attribute.valid? end end diff --git a/lib/ransack/nodes/sort.rb b/lib/ransack/nodes/sort.rb index 0a700df86..244aeec74 100644 --- a/lib/ransack/nodes/sort.rb +++ b/lib/ransack/nodes/sort.rb @@ -32,7 +32,7 @@ def valid? def name=(name) @name = name - context.bind(self, name) unless name.blank? + context.bind(self, name) end def dir=(dir) From 3df134e08e798d077155e24005138d4b6d1bdfd5 Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Sat, 2 Jan 2016 10:49:13 -0800 Subject: [PATCH 02/11] Tell if a condition is negative --- lib/ransack/nodes/condition.rb | 4 ++++ lib/ransack/predicate.rb | 4 ++++ spec/ransack/nodes/condition_spec.rb | 9 +++++++++ 3 files changed, 17 insertions(+) diff --git a/lib/ransack/nodes/condition.rb b/lib/ransack/nodes/condition.rb index ab09ecf4a..1fe3f288c 100644 --- a/lib/ransack/nodes/condition.rb +++ b/lib/ransack/nodes/condition.rb @@ -247,6 +247,10 @@ def inspect "Condition <#{data}>" end + def negative? + predicate.negative? + end + private def valid_combinator? diff --git a/lib/ransack/predicate.rb b/lib/ransack/predicate.rb index 2b5bd5da5..935729469 100644 --- a/lib/ransack/predicate.rb +++ b/lib/ransack/predicate.rb @@ -74,5 +74,9 @@ def validate(vals, type = @type) vals.any? { |v| validator.call(type ? v.cast(type) : v.value) } end + def negative? + @name.include?("not_".freeze) + end + end end diff --git a/spec/ransack/nodes/condition_spec.rb b/spec/ransack/nodes/condition_spec.rb index 6cb728ddc..8520fd882 100644 --- a/spec/ransack/nodes/condition_spec.rb +++ b/spec/ransack/nodes/condition_spec.rb @@ -29,6 +29,15 @@ module Nodes specify { expect(subject.values.size).to eq(2) } end + describe '#negative?' do + let(:context) { Context.for(Person) } + let(:eq) { Condition.extract(context, 'name_eq', 'A') } + let(:not_eq) { Condition.extract(context, 'name_not_eq', 'A') } + + specify { expect(not_eq.negative?).to be true } + specify { expect(eq.negative?).to be false } + end + context 'with an invalid predicate' do subject { Condition.extract( From 17742fd0bf952e01be7345bf7105afd61082e5e0 Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Fri, 8 Jan 2016 15:31:54 -0800 Subject: [PATCH 03/11] Refactor build_or_find_association into separate methods --- lib/ransack/adapters/active_record/context.rb | 72 ++++++++++--------- 1 file changed, 39 insertions(+), 33 deletions(-) diff --git a/lib/ransack/adapters/active_record/context.rb b/lib/ransack/adapters/active_record/context.rb index 4f68d6502..3eb03160f 100644 --- a/lib/ransack/adapters/active_record/context.rb +++ b/lib/ransack/adapters/active_record/context.rb @@ -242,37 +242,41 @@ def convert_join_strings_to_ast(table, joins) .map { |join| table.create_string_join(Arel.sql(join)) } end + def build_or_find_association(name, parent = @base, klass = nil) + find_association(name, parent, klass) or build_association(name, parent, klass) + end + if ::ActiveRecord::VERSION::STRING >= Constants::RAILS_4_1 - def build_or_find_association(name, parent = @base, klass = nil) - found_association = @join_dependency.join_root.children - .detect do |assoc| + def find_association(name, parent = @base, klass = nil) + @join_dependency.join_root.children.detect do |assoc| assoc.reflection.name == name && (@associations_pot.nil? || @associations_pot[assoc] == parent) && (!klass || assoc.reflection.klass == klass) end + end - unless found_association - jd = JoinDependency.new( - parent.base_klass, - Polyamorous::Join.new(name, @join_type, klass), - [] + def build_association(name, parent = @base, klass = nil) + jd = JoinDependency.new( + parent.base_klass, + Polyamorous::Join.new(name, @join_type, klass), + [] + ) + found_association = jd.join_root.children.last + associations found_association, parent + + # TODO maybe we dont need to push associations here, we could loop + # through the @associations_pot instead + @join_dependency.join_root.children.push found_association + + # Builds the arel nodes properly for this association + @join_dependency.send( + :construct_tables!, jd.join_root, found_association ) - found_association = jd.join_root.children.last - associations found_association, parent - # TODO maybe we dont need to push associations here, we could loop - # through the @associations_pot instead - @join_dependency.join_root.children.push found_association + # Leverage the stashed association functionality in AR + @object = @object.joins(jd) - # Builds the arel nodes properly for this association - @join_dependency.send( - :construct_tables!, jd.join_root, found_association - ) - - # Leverage the stashed association functionality in AR - @object = @object.joins(jd) - end found_association end @@ -283,24 +287,26 @@ def associations(assoc, parent) else - def build_or_find_association(name, parent = @base, klass = nil) + def build_association(name, parent = @base, klass = nil) + @join_dependency.send( + :build, + Polyamorous::Join.new(name, @join_type, klass), + parent + ) + found_association = @join_dependency.join_associations.last + # Leverage the stashed association functionality in AR + @object = @object.joins(found_association) + + found_association + end + + def find_association(name, parent = @base, klass = nil) found_association = @join_dependency.join_associations .detect do |assoc| assoc.reflection.name == name && assoc.parent == parent && (!klass || assoc.reflection.klass == klass) end - unless found_association - @join_dependency.send( - :build, - Polyamorous::Join.new(name, @join_type, klass), - parent - ) - found_association = @join_dependency.join_associations.last - # Leverage the stashed association functionality in AR - @object = @object.joins(found_association) - end - found_association end end From 4b1544898cc2f4d15b69f964f09ececa71fd6e92 Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Fri, 8 Jan 2016 16:43:14 -0800 Subject: [PATCH 04/11] Clean out unused constant references --- lib/ransack/adapters/active_record/3.1/context.rb | 1 - lib/ransack/adapters/active_record/context.rb | 1 - lib/ransack/adapters/mongoid/context.rb | 4 ---- 3 files changed, 6 deletions(-) diff --git a/lib/ransack/adapters/active_record/3.1/context.rb b/lib/ransack/adapters/active_record/3.1/context.rb index 2a718d2e8..e736bb493 100644 --- a/lib/ransack/adapters/active_record/3.1/context.rb +++ b/lib/ransack/adapters/active_record/3.1/context.rb @@ -8,7 +8,6 @@ module ActiveRecord class Context < ::Ransack::Context # Because the AR::Associations namespace is insane JoinDependency = ::ActiveRecord::Associations::JoinDependency - JoinPart = JoinDependency::JoinPart # Redefine a few things for ActiveRecord 3.1. diff --git a/lib/ransack/adapters/active_record/context.rb b/lib/ransack/adapters/active_record/context.rb index 3eb03160f..4e10f3dfc 100644 --- a/lib/ransack/adapters/active_record/context.rb +++ b/lib/ransack/adapters/active_record/context.rb @@ -9,7 +9,6 @@ class Context < ::Ransack::Context # Because the AR::Associations namespace is insane JoinDependency = ::ActiveRecord::Associations::JoinDependency - JoinPart = JoinDependency::JoinPart def initialize(object, options = {}) super diff --git a/lib/ransack/adapters/mongoid/context.rb b/lib/ransack/adapters/mongoid/context.rb index 1847f5d84..b36b24461 100644 --- a/lib/ransack/adapters/mongoid/context.rb +++ b/lib/ransack/adapters/mongoid/context.rb @@ -6,10 +6,6 @@ module Adapters module Mongoid class Context < ::Ransack::Context - # Because the AR::Associations namespace is insane - # JoinDependency = ::Mongoid::Associations::JoinDependency - # JoinPart = JoinDependency::JoinPart - def initialize(object, options = {}) super # @arel_visitor = @engine.connection.visitor From 00631f969d615ccf48f89cf33dadd841780100b3 Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Thu, 11 Feb 2016 18:26:11 -0800 Subject: [PATCH 05/11] Refactor: inline private method --- .../active_record/ransack/nodes/condition.rb | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/lib/ransack/adapters/active_record/ransack/nodes/condition.rb b/lib/ransack/adapters/active_record/ransack/nodes/condition.rb index a45eba7f0..da39c5f4b 100644 --- a/lib/ransack/adapters/active_record/ransack/nodes/condition.rb +++ b/lib/ransack/adapters/active_record/ransack/nodes/condition.rb @@ -3,7 +3,11 @@ module Nodes class Condition def arel_predicate - arel_predicate_for(attributes_array) + if attributes.size > 1 + combinator_for(attributes_array) + else + format_predicate(attributes_array.first) + end end private @@ -16,14 +20,6 @@ def attributes_array end end - def arel_predicate_for(predicates) - if predicates.size > 1 - combinator_for(predicates) - else - format_predicate(predicates.first) - end - end - def combinator_for(predicates) if combinator === Constants::AND Arel::Nodes::Grouping.new(Arel::Nodes::And.new(predicates)) From 55e56acd0adb613545e453d85a064aae0f955693 Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Thu, 11 Feb 2016 18:29:24 -0800 Subject: [PATCH 06/11] Confusing private arguments and method names --- .../active_record/ransack/nodes/condition.rb | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/lib/ransack/adapters/active_record/ransack/nodes/condition.rb b/lib/ransack/adapters/active_record/ransack/nodes/condition.rb index da39c5f4b..b248bd315 100644 --- a/lib/ransack/adapters/active_record/ransack/nodes/condition.rb +++ b/lib/ransack/adapters/active_record/ransack/nodes/condition.rb @@ -4,15 +4,15 @@ class Condition def arel_predicate if attributes.size > 1 - combinator_for(attributes_array) + combinator_for_predicates else - format_predicate(attributes_array.first) + format_predicate end end private - def attributes_array + def arel_predicates attributes.map do |a| a.attr.send( arel_predicate_for_attribute(a), formatted_values_for_attribute(a) @@ -20,20 +20,20 @@ def attributes_array end end - def combinator_for(predicates) + def combinator_for_predicates if combinator === Constants::AND - Arel::Nodes::Grouping.new(Arel::Nodes::And.new(predicates)) + Arel::Nodes::Grouping.new(arel_predicates.inject(&:and)) elsif combinator === Constants::OR - predicates.inject(&:or) + arel_predicates.inject(&:or) end end - def format_predicate(predicate) - predicate.tap do - if casted_array_with_in_predicate?(predicate) - predicate.right[0] = format_values_for(predicate.right[0]) - end + def format_predicate + predicate = arel_predicates.first + if casted_array_with_in_predicate?(predicate) + predicate.right[0] = format_values_for(predicate.right[0]) end + predicate end def casted_array_with_in_predicate?(predicate) From a873c63db0e762ad9c744cf9d5d7916b5a7d4caf Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Fri, 12 Feb 2016 15:13:45 -0800 Subject: [PATCH 07/11] Load older versions as overrides to share logic --- lib/ransack/adapters/active_record.rb | 4 ++-- lib/ransack/adapters/active_record/3.0/context.rb | 6 ++++-- lib/ransack/adapters/active_record/3.1/context.rb | 2 -- lib/ransack/adapters/active_record/context.rb | 4 +++- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/ransack/adapters/active_record.rb b/lib/ransack/adapters/active_record.rb index 1a4369d59..0ffbf303c 100644 --- a/lib/ransack/adapters/active_record.rb +++ b/lib/ransack/adapters/active_record.rb @@ -1,6 +1,8 @@ require 'ransack/adapters/active_record/base' ActiveRecord::Base.extend Ransack::Adapters::ActiveRecord::Base +require 'ransack/adapters/active_record/context' + case ActiveRecord::VERSION::STRING when /^3\.0\./ require 'ransack/adapters/active_record/3.0/context' @@ -8,6 +10,4 @@ require 'ransack/adapters/active_record/3.1/context' when /^3\.2\./ require 'ransack/adapters/active_record/3.2/context' -else - require 'ransack/adapters/active_record/context' end diff --git a/lib/ransack/adapters/active_record/3.0/context.rb b/lib/ransack/adapters/active_record/3.0/context.rb index f9ad0f726..e725a2cdf 100644 --- a/lib/ransack/adapters/active_record/3.0/context.rb +++ b/lib/ransack/adapters/active_record/3.0/context.rb @@ -7,9 +7,11 @@ module Ransack module Adapters module ActiveRecord class Context < ::Ransack::Context + # Because the AR::Associations namespace is insane - JoinDependency = ::ActiveRecord::Associations::ClassMethods::JoinDependency - JoinBase = JoinDependency::JoinBase + if defined? ::ActiveRecord::Associations::ClassMethods::JoinDependency + JoinDependency = ::ActiveRecord::Associations::ClassMethods::JoinDependency + end # Redefine a few things for ActiveRecord 3.0. diff --git a/lib/ransack/adapters/active_record/3.1/context.rb b/lib/ransack/adapters/active_record/3.1/context.rb index e736bb493..8a9cbdc07 100644 --- a/lib/ransack/adapters/active_record/3.1/context.rb +++ b/lib/ransack/adapters/active_record/3.1/context.rb @@ -6,8 +6,6 @@ module Ransack module Adapters module ActiveRecord class Context < ::Ransack::Context - # Because the AR::Associations namespace is insane - JoinDependency = ::ActiveRecord::Associations::JoinDependency # Redefine a few things for ActiveRecord 3.1. diff --git a/lib/ransack/adapters/active_record/context.rb b/lib/ransack/adapters/active_record/context.rb index 4e10f3dfc..381a69c81 100644 --- a/lib/ransack/adapters/active_record/context.rb +++ b/lib/ransack/adapters/active_record/context.rb @@ -8,7 +8,9 @@ module ActiveRecord class Context < ::Ransack::Context # Because the AR::Associations namespace is insane - JoinDependency = ::ActiveRecord::Associations::JoinDependency + if defined? ::ActiveRecord::Associations::JoinDependency + JoinDependency = ::ActiveRecord::Associations::JoinDependency + end def initialize(object, options = {}) super From 43cf9ec33ebbf38c580cf9f5bff0c3f211811a8b Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Tue, 16 Feb 2016 01:02:18 -0800 Subject: [PATCH 08/11] Inline internal method --- lib/ransack/adapters/active_record/context.rb | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/lib/ransack/adapters/active_record/context.rb b/lib/ransack/adapters/active_record/context.rb index 381a69c81..f1b0a3bf2 100644 --- a/lib/ransack/adapters/active_record/context.rb +++ b/lib/ransack/adapters/active_record/context.rb @@ -15,6 +15,7 @@ class Context < ::Ransack::Context def initialize(object, options = {}) super @arel_visitor = @engine.connection.visitor + @associations_pot = {} end def relation_for(object) @@ -252,7 +253,7 @@ def build_or_find_association(name, parent = @base, klass = nil) def find_association(name, parent = @base, klass = nil) @join_dependency.join_root.children.detect do |assoc| assoc.reflection.name == name && - (@associations_pot.nil? || @associations_pot[assoc] == parent) && + (@associations_pot.empty? || @associations_pot[assoc] == parent) && (!klass || assoc.reflection.klass == klass) end end @@ -264,7 +265,7 @@ def build_association(name, parent = @base, klass = nil) [] ) found_association = jd.join_root.children.last - associations found_association, parent + @associations_pot[found_association] = parent # TODO maybe we dont need to push associations here, we could loop # through the @associations_pot instead @@ -281,11 +282,6 @@ def build_association(name, parent = @base, klass = nil) found_association end - def associations(assoc, parent) - @associations_pot ||= {} - @associations_pot[assoc] = parent - end - else def build_association(name, parent = @base, klass = nil) From eb8257092a60dc64ef391de1347eed0284d1651a Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Mon, 15 Feb 2016 11:02:09 -0800 Subject: [PATCH 09/11] Silence deprecation warning --- spec/ransack/helpers/form_helper_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/ransack/helpers/form_helper_spec.rb b/spec/ransack/helpers/form_helper_spec.rb index 7dc28d112..c0af3a0cb 100644 --- a/spec/ransack/helpers/form_helper_spec.rb +++ b/spec/ransack/helpers/form_helper_spec.rb @@ -312,7 +312,7 @@ module Helpers context 'view has existing parameters' do before do - @controller.view_context.params.merge!({ exist: 'existing' }) + @controller.view_context.params[:exist] = 'existing' end describe '#sort_link should not remove existing params' do subject { @controller.view_context From 96d0fd75ade6a6ca019ccc03cca6d876f0e12ecf Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Fri, 8 Jan 2016 16:18:24 -0800 Subject: [PATCH 10/11] 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 f1b0a3bf2..390664a4d 100644 --- a/lib/ransack/adapters/active_record/context.rb +++ b/lib/ransack/adapters/active_record/context.rb @@ -15,7 +15,6 @@ class Context < ::Ransack::Context def initialize(object, options = {}) super @arel_visitor = @engine.connection.visitor - @associations_pot = {} end def relation_for(object) @@ -139,6 +138,64 @@ def alias_tracker @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 @@ def build_association(name, parent = @base, klass = nil) 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 @@ def build_association(name, parent = @base, klass = nil) 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 12d75aead..708efeaa2 100644 --- a/lib/ransack/adapters/active_record/ransack/context.rb +++ b/lib/ransack/adapters/active_record/ransack/context.rb @@ -27,6 +27,8 @@ def initialize(object, options = {}) @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 b248bd315..e97ac5def 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 Nodes 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 - 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 + def combinator_method + combinator === Constants::OR ? :or : :and 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 3183a3dd0..0eccd46a9 100644 --- a/lib/ransack/nodes/attribute.rb +++ b/lib/ransack/nodes/attribute.rb @@ -24,6 +24,10 @@ def valid? .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 1fe3f288c..09eb8c198 100644 --- a/lib/ransack/nodes/condition.rb +++ b/lib/ransack/nodes/condition.rb @@ -131,6 +131,9 @@ def build_attribute(name = nil, ransacker_args = []) 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 @@ def hash 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 c9d3e97c7..8220746f6 100644 --- a/spec/ransack/adapters/active_record/base_spec.rb +++ b/spec/ransack/adapters/active_record/base_spec.rb @@ -94,6 +94,76 @@ module ActiveRecord 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') From 7f1235adb5a7c0ee3ac01913d076ffc805f66ec0 Mon Sep 17 00:00:00 2001 From: Andrew Vit Date: Tue, 16 Feb 2016 12:30:28 -0800 Subject: [PATCH 11/11] Add new method stubs for mongoid --- lib/ransack/adapters/mongoid/context.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/lib/ransack/adapters/mongoid/context.rb b/lib/ransack/adapters/mongoid/context.rb index b36b24461..f1a1db41f 100644 --- a/lib/ransack/adapters/mongoid/context.rb +++ b/lib/ransack/adapters/mongoid/context.rb @@ -96,6 +96,14 @@ def klassify(obj) end end + def lock_association(association) + warn "lock_association is not implemented for Ransack mongoid adapter" if $DEBUG + end + + def remove_association(association) + warn "remove_association is not implemented for Ransack mongoid adapter" if $DEBUG + end + private def get_parent_and_attribute_name(str, parent = @base)