Merge pull request #352 from Dynamoid/add-warning-about-skipped-conditions

Add warning about skipped conditions

Author: andrykonchin

lib/dynamoid/criteria/chain.rb

@@ -1,9 +1,11 @@
 # frozen_string_literal: true
 
 require_relative 'key_fields_detector'
+require_relative 'ignored_conditions_detector'
+require_relative 'overwritten_conditions_detector'
 require_relative 'nonexistent_fields_detector'
 
-module Dynamoid #:nodoc:
+module Dynamoid
   module Criteria
     # The criteria chain is equivalent to an ActiveRecord relation (and realistically I should change the name from
     # chain to relation). It is a chainable object that builds up a query and eventually executes it by a Query or Scan.
@@ -42,20 +44,23 @@ def initialize(source)
       #
       # @since 0.2.0
       def where(args)
-        query.update(args.symbolize_keys)
-
-        nonexistent_fields = NonexistentFieldsDetector.new(args, @source).fields
+        detector = IgnoredConditionsDetector.new(args)
+        if detector.found?
+          Dynamoid.logger.warn(detector.warning_message)
+        end
 
-        if nonexistent_fields.present?
-          fields_list = nonexistent_fields.map { |s| "`#{s}`" }.join(', ')
-          fields_count = nonexistent_fields.size
+        detector = OverwrittenConditionsDetector.new(@query, args)
+        if detector.found?
+          Dynamoid.logger.warn(detector.warning_message)
+        end
 
-          Dynamoid.logger.warn(
-            "where conditions contain nonexistent" \
-            " field #{ 'name'.pluralize(fields_count) } #{ fields_list }"
-          )
+        detector = NonexistentFieldsDetector.new(args, @source)
+        if detector.found?
+          Dynamoid.logger.warn(detector.warning_message)
         end
 
+        query.update(args.symbolize_keys)
+
         # we should re-initialize keys detector every time we change query
         @key_fields_detector = KeyFieldsDetector.new(@query, @source)
 

lib/dynamoid/criteria/ignored_conditions_detector.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+module Dynamoid
+  module Criteria
+    class IgnoredConditionsDetector
+      def initialize(conditions)
+        @conditions = conditions
+        @ignored_keys = ignored_keys
+      end
+
+      def found?
+        @ignored_keys.present?
+      end
+
+      def warning_message
+        return unless found?
+
+        'Where conditions may contain only one condition for an attribute. ' \
+          "Following conditions are ignored: #{ignored_conditions}"
+      end
+
+      private
+
+      def ignored_keys
+        @conditions.keys
+          .group_by(&method(:key_to_field))
+          .select { |field, ary| ary.size > 1 }
+          .flat_map { |field, ary| ary[0 .. -2] }
+      end
+
+      def key_to_field(key)
+        key.to_s.split('.')[0]
+      end
+
+      def ignored_conditions
+        @conditions.slice(*@ignored_keys)
+      end
+    end
+  end
+end
+

lib/dynamoid/criteria/nonexistent_fields_detector.rb

@@ -6,19 +6,31 @@ class NonexistentFieldsDetector
       def initialize(conditions, source)
         @conditions = conditions
         @source = source
+        @nonexistent_fields = nonexistent_fields
       end
 
-      def fields
-        fields_from_conditions - fields_existent
+      def found?
+        @nonexistent_fields.present?
+      end
+
+      def warning_message
+        return unless found?
+
+        fields_list = @nonexistent_fields.map { |s| "`#{s}`" }.join(', ')
+        count = @nonexistent_fields.size
+
+        "where conditions contain nonexistent" \
+          " field #{ 'name'.pluralize(count) } #{ fields_list }"
       end
 
       private
 
+      def nonexistent_fields
+        fields_from_conditions - fields_existent
+      end
+
       def fields_from_conditions
-        @conditions.keys.map do |s|
-          name, _ = s.to_s.split('.')
-          name
-        end.map(&:to_sym)
+        @conditions.keys.map { |s| s.to_s.split('.')[0].to_sym }
       end
 
       def fields_existent

lib/dynamoid/criteria/overwritten_conditions_detector.rb

@@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+module Dynamoid
+  module Criteria
+    class OverwrittenConditionsDetector
+      def initialize(conditions, conditions_new)
+        @conditions = conditions
+        @new_conditions = conditions_new
+        @overwritten_keys = overwritten_keys
+      end
+
+      def found?
+        @overwritten_keys.present?
+      end
+
+      def warning_message
+        return unless found?
+
+        'Where conditions may contain only one condition for an attribute. ' \
+          "Following conditions are ignored: #{ignored_conditions}"
+      end
+
+      private
+
+      def overwritten_keys
+        new_fields = @new_conditions.keys.map(&method(:key_to_field))
+        @conditions.keys.select { |key| key_to_field(key).in?(new_fields) }
+      end
+
+      def key_to_field(key)
+        key.to_s.split('.')[0]
+      end
+
+      def ignored_conditions
+        @conditions.slice(*@overwritten_keys.map(&:to_sym))
+      end
+    end
+  end
+end
+

spec/dynamoid/criteria/chain_spec.rb

@@ -970,6 +970,61 @@ def request_params
         model.where(town: 'New York', street1: 'Allen Street')
       end
     end
+
+    context 'passed several conditions for the same attribute' do
+      let(:model) do
+        new_class do
+          field :age, :integer
+          field :name
+        end
+      end
+
+      before do
+        model.create_table
+      end
+
+      it 'ignores conditions except the last one' do
+        (1 .. 5).each { |i| model.create(age: i) }
+
+        models = model.where('age.gt': 2, 'age.lt': 4).to_a
+        expect(models.map(&:age)).to contain_exactly(1, 2, 3)
+
+        models = model.where('age.lt': 4, 'age.gt': 2).to_a
+        expect(models.map(&:age)).to contain_exactly(3, 4, 5)
+      end
+
+      describe 'warning' do
+        it 'writes warning message' do
+          expect(Dynamoid.logger).to receive(:warn)
+            .with(
+              'Where conditions may contain only one condition for an attribute. ' \
+              'Following conditions are ignored: {:"age.gt"=>2}'
+            )
+
+          model.where('age.gt': 2, 'age.lt': 4)
+        end
+
+        it 'writes warning message when conditions are build with several calls of `where`' do
+          expect(Dynamoid.logger).to receive(:warn)
+            .with(
+              'Where conditions may contain only one condition for an attribute. ' \
+              'Following conditions are ignored: {:"age.gt"=>2}'
+            )
+
+          model.where('age.gt': 2).where('age.lt': 4)
+        end
+
+        it 'writes warning message when there are ignored conditions for several attributes' do
+          expect(Dynamoid.logger).to receive(:warn)
+            .with(
+              'Where conditions may contain only one condition for an attribute. ' \
+              'Following conditions are ignored: {:age=>2, :name=>"Alex"}'
+            )
+
+          model.where(age: 2, name: 'Alex').where('age.gt': 3, name: 'David')
+        end
+      end
+    end
   end
 
   describe '#find_by_pages' do