From b5431069db5f11e1f38f633daf8a33e78ce8b59c Mon Sep 17 00:00:00 2001
From: Dirkjan Bussink <d.bussink@gmail.com>
Date: Tue, 27 Oct 2009 11:59:26 +0100
Subject: [PATCH] OrOperation should be valid if at least one branch is valid

OrOperations are a bit different from the other operations, since
they are valid if at least one of the branches it contains is valid.

A future refactor should also ensure that only the valid operations
go to the datastore, this can also eliminate OR branches of only
one valid branch is left.
---
 lib/dm-core/query/conditions/operation.rb | 11 +++++++++
 spec/semipublic/query/conditions_spec.rb  | 13 +++++++++++
 spec/semipublic/query_spec.rb             | 27 +++++++++++++++++++++++
 3 files changed, 51 insertions(+)

diff --git a/lib/dm-core/query/conditions/operation.rb b/lib/dm-core/query/conditions/operation.rb
index c9313f94d..e2868f24e 100644
--- a/lib/dm-core/query/conditions/operation.rb
+++ b/lib/dm-core/query/conditions/operation.rb
@@ -189,6 +189,17 @@ class OrOperation < AbstractOperation
         def matches?(record)
           @operands.any? { |operand| operand.matches?(record) }
         end
+
+        def valid?
+          @operands.any? do |operand|
+            if operand.respond_to?(:valid?)
+              operand.valid?
+            else
+              true
+            end
+          end
+        end
+
       end # class OrOperation
 
       class NotOperation < AbstractOperation
diff --git a/spec/semipublic/query/conditions_spec.rb b/spec/semipublic/query/conditions_spec.rb
index 3e4963a70..c4c54c9c0 100644
--- a/spec/semipublic/query/conditions_spec.rb
+++ b/spec/semipublic/query/conditions_spec.rb
@@ -73,6 +73,8 @@ class ::Heffalump
     before do
       @comp1 = Comparison.new(:eql, Heffalump.num_spots, 1)
       @comp2 = Comparison.new(:eql, Heffalump.color, 'green')
+      @comp3 = Comparison.new(:in,  Heffalump.mass, [])
+      @comp4 = Comparison.new(:in,  Heffalump.parent, [])
     end
 
     it 'should initialize an AbstractOperation object' do
@@ -141,6 +143,8 @@ class ::Heffalump
     describe 'OrOperation' do
       before do
         @op = Operation.new(:or, @comp1, @comp2)
+        @op1 = Operation.new(:or, @comp1, @comp3)
+        @op2 = Operation.new(:or, @comp3, @comp4)
       end
 
       it 'should match if any of the comparisons match' do
@@ -156,6 +160,15 @@ class ::Heffalump
 
         @op.should_not match(@heff3)
       end
+
+      it 'should be valid if at least one branch is valid' do
+        @op1.should be_valid
+      end
+
+      it 'should not be valid if there are no valid branches' do
+        @op2.should_not be_valid
+      end
+
     end
   end
 
diff --git a/spec/semipublic/query_spec.rb b/spec/semipublic/query_spec.rb
index 894aa121e..c3ea0163f 100644
--- a/spec/semipublic/query_spec.rb
+++ b/spec/semipublic/query_spec.rb
@@ -606,6 +606,33 @@ class ::Contact < User; end
           end
         end
 
+        describe 'with an Array with no entries' do
+          before :all do
+            @options[:conditions] = { :name => [] }
+            @return = DataMapper::Query.new(@repository, @model, @options.freeze)
+          end
+
+          it { @return.should be_kind_of(DataMapper::Query) }
+
+          it 'should set the conditions' do
+            pending do
+              @return.conditions.should ==
+                DataMapper::Query::Conditions::Operation.new(
+                  :and,
+                  DataMapper::Query::Conditions::Comparison.new(
+                    :eql,
+                    @model.properties[:name],
+                    'Dan Kubb'
+                  )
+                )
+            end
+          end
+
+          it 'should not be valid' do
+            @return.should_not be_valid
+          end
+        end
+
         describe 'with an Array with duplicate entries' do
           before :all do
             @options[:conditions] = { :name => [ 'John Doe', 'Dan Kubb', 'John Doe' ] }