From a458292449e7d8d8bc51007f2695e2baab383f7a Mon Sep 17 00:00:00 2001 From: Dan Kubb Date: Thu, 8 Oct 2009 13:03:50 -0700 Subject: [PATCH] Refactored Query#update so that raw conditions can be merged * Reduced amount of duplicate work than Query#update needs to perform to merge another query in. * Refactored Query#initialize so conditions merging is handled by a separate method, shared with #update * Simplified Query#relative to use Query#merge rather than creating a new Query using options. [#1084 state:resolved] --- lib/dm-core/adapters/data_objects_adapter.rb | 4 +- lib/dm-core/query.rb | 94 +++++++++++++------- spec/semipublic/query_spec.rb | 32 +++++-- 3 files changed, 88 insertions(+), 42 deletions(-) diff --git a/lib/dm-core/adapters/data_objects_adapter.rb b/lib/dm-core/adapters/data_objects_adapter.rb index 31755e61..14dc482f 100644 --- a/lib/dm-core/adapters/data_objects_adapter.rb +++ b/lib/dm-core/adapters/data_objects_adapter.rb @@ -476,7 +476,7 @@ def conditions_statement(conditions, qualify = false) when Query::Conditions::AbstractComparison then comparison_statement(conditions, qualify) when Array statement, bind_values = conditions # handle raw conditions - [ "(#{statement})", bind_values ] + [ "(#{statement})", bind_values ].compact end end @@ -516,7 +516,7 @@ def operation_statement(operation, qualify) operation.each do |operand| statement, values = conditions_statement(operand, qualify) statements << statement - bind_values.concat(values) + bind_values.concat(values) unless values.nil? end join_with = operation.kind_of?(@negated ? Query::Conditions::OrOperation : Query::Conditions::AndOperation) ? 'AND' : 'OR' diff --git a/lib/dm-core/query.rb b/lib/dm-core/query.rb index dbd1dcc1..56cac9b6 100644 --- a/lib/dm-core/query.rb +++ b/lib/dm-core/query.rb @@ -325,24 +325,26 @@ def reverse! def update(other) assert_kind_of 'other', other, self.class, Hash - other_options = if other.kind_of? self.class - if self.eql?(other) - return self - end + other_options = if other.kind_of?(self.class) + return self if self.eql?(other) assert_valid_other(other) other.options else + return self if other.empty? other end - unless other_options.empty? - options = @options.merge(other_options) - if @options[:conditions] and other_options[:conditions] - options[:conditions] = @options[:conditions].dup << other_options[:conditions] - end - initialize(repository, model, options) + @options = @options.merge(other_options).freeze + assert_valid_options(@options) + + normalize = other_options.only(*OPTIONS - [ :conditions ]).map do |attribute, value| + instance_variable_set("@#{attribute}", value.try_dup) + attribute end + merge_conditions([ other_options.except(*OPTIONS), other_options[:conditions] ]) + normalize_options(normalize) + self end @@ -381,9 +383,9 @@ def relative(options) offset = options.delete(:offset) limit = options.delete(:limit) || self.limit - offset - self.class.new(repository, model, @options.merge(options)).slice!(offset, limit) + merge(options).slice!(offset, limit) else - self.class.new(repository, model, @options.merge(options)) + merge(options) end end @@ -602,26 +604,8 @@ def initialize(repository, model, options = {}) @links = @links.dup - # treat all non-options as conditions - @options.except(*OPTIONS).each { |kv| append_condition(*kv) } - - # parse @options[:conditions] differently - case conditions = @options[:conditions] - when Conditions::AbstractOperation, Conditions::AbstractComparison - add_condition(conditions) - - when Hash - conditions.each { |kv| append_condition(*kv) } - - when Array - statement, *bind_values = *conditions - add_condition([ statement, bind_values ]) - @raw = true - end - - normalize_order - normalize_fields - normalize_links + merge_conditions([ @options.except(*OPTIONS), @options[:conditions] ]) + normalize_options end # Copying contructor, called for Query#dup @@ -861,9 +845,51 @@ def assert_valid_other(other) end end - # Normalize order elements to Query::Direction instances + # Handle all the conditions options provided # - # TODO: needs example + # @param [Array] + # a list of conditions + # + # @return [undefined] + # + # @api private + def merge_conditions(conditions) + @conditions = Conditions::Operation.new(:and) << @conditions unless @conditions.nil? + + conditions.compact! + conditions.each do |condition| + case condition + when Conditions::AbstractOperation, Conditions::AbstractComparison + add_condition(condition) + + when Hash + condition.each { |kv| append_condition(*kv) } + + when Array + statement, *bind_values = *condition + raw_condition = [ statement ] + raw_condition << bind_values if bind_values.size > 0 + add_condition(raw_condition) + @raw = true + end + end + end + + # Normalize options + # + # @param [Array] options + # the options to normalize + # + # @return [undefined] + # + # @api private + def normalize_options(options = OPTIONS) + (options & [ :order, :fields, :links ]).each do |option| + send("normalize_#{option}") + end + end + + # Normalize order elements to Query::Direction instances # # @api private def normalize_order diff --git a/spec/semipublic/query_spec.rb b/spec/semipublic/query_spec.rb index c91be698..756a50d5 100644 --- a/spec/semipublic/query_spec.rb +++ b/spec/semipublic/query_spec.rb @@ -2592,13 +2592,33 @@ class ::Clone it 'should update the conditions' do @return.conditions.should == DataMapper::Query::Conditions::Operation.new( - :and, - DataMapper::Query::Conditions::Comparison.new(:eql, - @model.properties[:name], - @options[:name] - ) + :and, + DataMapper::Query::Conditions::Comparison.new( + :eql, + @model.properties[:name], + @options[:name] + ) + ) + end + end + + describe 'using raw conditions' do + before :all do + @query.update(:conditions => [ 'name IS NOT NULL' ]) + + @return = @query.update(:conditions => [ 'name = ?', 'Dan Kubb' ]) + end + + it { @return.should be_kind_of(DataMapper::Query) } + + it { @return.should equal(@original) } + + it 'should update the conditions' do + @return.conditions.should == DataMapper::Query::Conditions::Operation.new( + :and, + [ 'name IS NOT NULL' ], + [ 'name = ?', [ 'Dan Kubb' ] ] ) - #@return.conditions.should == [ [ :eql, @model.properties[:name], @options[:name] ] ] end end end