Merge pull request #363 from Dynamoid/refactor-key-fields-detector

Refactor KeyFieldsDetector

Author: andrykonchin

CHANGELOG.md

@@ -8,6 +8,7 @@
 
 ## Fixes
 * Fix: [#357](https://github.com/Dynamoid/dynamoid/pull/357) Fix synchronous table creation issue
+* Fix: [#362](https://github.com/Dynamoid/dynamoid/pull/362) Fix issue with selecting Global Secondary Index (@atyndall)
 
 ---
 

lib/dynamoid/criteria/key_fields_detector.rb

@@ -3,55 +3,126 @@
 module Dynamoid #:nodoc:
   module Criteria
     class KeyFieldsDetector
-      attr_reader :hash_key, :range_key, :index_name
+
+      class Query
+        def initialize(query_hash)
+          @query_hash = query_hash
+          @fields_with_operator = query_hash.keys.map(&:to_s)
+          @fields = query_hash.keys.map(&:to_s).map { |s| s.split('.').first }
+        end
+
+        def contain?(field_name)
+          @fields.include?(field_name.to_s)
+        end
+
+        def contain_with_eq_operator?(field_name)
+          @fields_with_operator.include?(field_name.to_s)
+        end
+      end
 
       def initialize(query, source)
         @query = query
         @source = source
-
-        detect_keys
+        @query = Query.new(query)
+        @result = find_keys_in_query
       end
 
       def key_present?
-        @hash_key.present?
+        @result.present?
+      end
+
+      def hash_key
+        @result && @result[:hash_key]
+      end
+
+      def range_key
+        @result && @result[:range_key]
+      end
+
+      def index_name
+        @result && @result[:index_name]
       end
 
       private
 
-      def detect_keys
-        query_keys = @query.keys.collect { |k| k.to_s.split('.').first }
+      def find_keys_in_query
+        match_table_and_sort_key ||
+          match_local_secondary_index ||
+          match_global_secondary_index_and_sort_key ||
+          match_table ||
+          match_global_secondary_index
+      end
 
-        # See if querying based on table hash key
-        if @query.keys.map(&:to_s).include?(@source.hash_key.to_s)
-          @hash_key = @source.hash_key
+      # Use table's default range key
+      def match_table_and_sort_key
+        return unless @query.contain_with_eq_operator?(@source.hash_key)
+        return unless @source.range_key
 
-          # Use table's default range key
-          if query_keys.include?(@source.range_key.to_s)
-            @range_key = @source.range_key
-            return
-          end
+        if @query.contain?(@source.range_key)
+          {
+            hash_key: @source.hash_key,
+            range_key: @source.range_key
+          }
+        end
+      end
+
+      # See if can use any local secondary index range key
+      # Chooses the first LSI found that can be utilized for the query
+      def match_local_secondary_index
+        return unless @query.contain_with_eq_operator?(@source.hash_key)
+
+        lsi = @source.local_secondary_indexes.values.find do |lsi|
+          @query.contain?(lsi.range_key)
+        end
+
+        if lsi.present?
+          {
+            hash_key: @source.hash_key,
+            range_key: lsi.range_key,
+            index_name: lsi.name,
+          }
+        end
+      end
 
-          # See if can use any local secondary index range key
-          # Chooses the first LSI found that can be utilized for the query
-          @source.local_secondary_indexes.each do |_, lsi|
-            next unless query_keys.include?(lsi.range_key.to_s)
+      # See if can use any global secondary index
+      # Chooses the first GSI found that can be utilized for the query
+      # GSI with range key involved into query conditions has higher priority
+      # But only do so if projects ALL attributes otherwise we won't
+      # get back full data
+      def match_global_secondary_index_and_sort_key
+        gsi = @source.global_secondary_indexes.values.find do |gsi|
+          @query.contain_with_eq_operator?(gsi.hash_key) && gsi.projected_attributes == :all &&
+            @query.contain?(gsi.range_key)
+        end
 
-            @range_key = lsi.range_key
-            @index_name = lsi.name
-          end
+        if gsi.present?
+          {
+            hash_key: gsi.hash_key,
+            range_key: gsi.range_key,
+            index_name: gsi.name,
+          }
         end
+      end
+
+      def match_table
+        return unless @query.contain_with_eq_operator?(@source.hash_key)
 
-        # See if can use any global secondary index
-        # Chooses the first GSI found that can be utilized for the query
-        # But only do so if projects ALL attributes otherwise we won't
-        # get back full data
-        @source.global_secondary_indexes.each do |_, gsi|
-          next unless @query.keys.map(&:to_s).include?(gsi.hash_key.to_s) && gsi.projected_attributes == :all
-          next if @hash_key.present? && !query_keys.include?(gsi.range_key.to_s)
+        {
+          hash_key: @source.hash_key,
+        }
+      end
+
+      def match_global_secondary_index
+        gsi = @source.global_secondary_indexes.values.find do |gsi|
+          @query.contain_with_eq_operator?(gsi.hash_key) && gsi.projected_attributes == :all
+        end
 
-          @hash_key = gsi.hash_key
-          @range_key = gsi.range_key
-          @index_name = gsi.name
+        if gsi.present?
+          {
+            hash_key: gsi.hash_key,
+            range_key: gsi.range_key,
+            index_name: gsi.name,
+          }
         end
       end
     end

spec/dynamoid/criteria/chain_spec.rb

@@ -1109,7 +1109,7 @@ def request_params
           expect(model.where('name': nil).to_a).to eq [@johndoe]
         end
 
-        it 'supports "in [nil]" check', log_level: :debug do
+        it 'supports "in [nil]" check' do
           pending 'because of temporary bug with nil type casting'
           expect(model.where('name.in': [nil]).to_a).to eq [@johndoe]
         end