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

@@ -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'

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<Conditions::AbstractOperation, Conditions::AbstractComparison, Hash, 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<Symbol>] 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

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