Refactor Relation and conditions to fix mutating scopes issues

README.md

@@ -211,11 +211,11 @@ Country#name=       # => sets the name
 The ActiveHash::Base.all method functions like an in-memory data store. You can save your records as ActiveHash::Relation object by using standard ActiveRecord create and save methods:
 ```ruby
 Country.all
-=> #<ActiveHash::Relation:0x00007f861e043bb0 @klass=Country, @all_records=[], @query_hash={}, @records_dirty=false>
+=> #<ActiveHash::Relation:0x00007f861e043bb0 @klass=Country, @all_records=[], @conditions=[..], @records_dirty=false>
 Country.create
 => #<Country:0x00007f861b7abce8 @attributes={:id=>1}>
 Country.all
-=> #<ActiveHash::Relation:0x00007f861b7b3628 @klass=Country, @all_records=[#<Country:0x00007f861b7abce8 @attributes={:id=>1}>], @query_hash={}, @records_dirty=false>
+=> #<ActiveHash::Relation:0x00007f861b7b3628 @klass=Country, @all_records=[#<Country:0x00007f861b7abce8 @attributes={:id=>1}>], @conditions=[..], @records_dirty=false>
 country = Country.new
 => #<Country:0x00007f861e059938 @attributes={}>
 country.new_record?
@@ -225,7 +225,7 @@ country.save
 country.new_record?
 # => false
 Country.all
-=> #<ActiveHash::Relation:0x00007f861e0ca610 @klass=Country, @all_records=[#<Country:0x00007f861b7abce8 @attributes={:id=>1}>, #<Country:0x00007f861e059938 @attributes={:id=>2}>], @query_hash={}, @records_dirty=false>
+=> #<ActiveHash::Relation:0x00007f861e0ca610 @klass=Country, @all_records=[#<Country:0x00007f861b7abce8 @attributes={:id=>1}>, #<Country:0x00007f861e059938 @attributes={:id=>2}>], @conditions=[..], @records_dirty=false>
 ```
 Notice that when adding records to the collection, it will auto-increment the id for you by default.  If you use string ids, it will not auto-increment the id.  Available methods are:
 ```

lib/active_hash.rb

@@ -13,6 +13,8 @@
 
 require 'active_hash/base'
 require 'active_hash/relation'
+require 'active_hash/condition'
+require 'active_hash/conditions'
 require 'active_file/multiple_files'
 require 'active_file/hash_and_array_files'
 require 'active_file/base'

lib/active_hash/base.rb

@@ -21,50 +21,8 @@ class FileTypeMismatchError < StandardError
   end
 
   class Base
-
     class_attribute :_data, :dirty, :default_attributes, :scopes
 
-    class WhereChain
-      def initialize(scope)
-        @scope = scope
-        @records = @scope.all
-      end
-
-      def not(options)
-        return @scope if options.blank?
-
-        # use index if searching by id
-        if options.key?(:id) || options.key?("id")
-          ids = @scope.pluck(:id) - Array.wrap(options.delete(:id) || options.delete("id"))
-          candidates = ids.map { |id| @scope.find_by_id(id) }.compact
-        end
-
-        filtered_records = (candidates || @records || []).reject do |record|
-          options.present? && match_options?(record, options)
-        end
-
-        ActiveHash::Relation.new(@scope.klass, filtered_records, {})
-      end
-
-      def match_options?(record, options)
-        options.all? do |col, match|
-          if match.kind_of?(Array)
-            match.any? { |v| normalize(v) == normalize(record[col]) }
-          else
-            normalize(record[col]) == normalize(match)
-          end
-        end
-      end
-
-      private :match_options?
-
-      def normalize(v)
-        v.respond_to?(:to_sym) ? v.to_sym : v
-      end
-
-      private :normalize
-    end
-
     if Object.const_defined?(:ActiveModel)
       extend ActiveModel::Naming
       include ActiveModel::Conversion
@@ -205,7 +163,9 @@ def create!(attributes = {})
       end
 
       def all(options = {})
-        ActiveHash::Relation.new(self, @records || [], options[:conditions] || {})
+        relation = ActiveHash::Relation.new(self, @records || [])
+        relation = relation.where!(options[:conditions]) if options[:conditions]
+        relation
       end
 
       delegate :where, :find, :find_by, :find_by!, :find_by_id, :count, :pluck, :ids, :pick, :first, :last, :order, to: :all

lib/active_hash/condition.rb

@@ -0,0 +1,44 @@
+class ActiveHash::Relation::Condition
+  attr_reader :constraints, :inverted
+
+  def initialize(constraints)
+    @constraints = constraints
+    @inverted = false
+  end
+
+  def invert!
+    @inverted = !inverted
+
+    self
+  end
+
+  def matches?(record)
+    match = begin
+      return true unless constraints
+
+      expectation_method = inverted ? :any? : :all?
+
+      constraints.send(expectation_method) do |attribute, expected|
+        value = record.public_send(attribute)
+
+        matches_value?(value, expected)
+      end
+    end
+
+    inverted ? !match : match
+  end
+
+  private
+
+  def matches_value?(value, comparison)
+    return comparison.any? { |v| matches_value?(value, v) } if comparison.is_a?(Array)
+    return comparison.include?(value) if comparison.is_a?(Range)
+    return comparison.match?(value) if comparison.is_a?(Regexp)
+
+    normalize(value) == normalize(comparison)
+  end
+
+  def normalize(value)
+    value.respond_to?(:to_s) ? value.to_s : value
+  end
+end

lib/active_hash/conditions.rb

@@ -0,0 +1,21 @@
+class ActiveHash::Relation::Conditions
+  attr_reader :conditions
+
+  delegate_missing_to :conditions
+
+  def initialize(conditions = [])
+    @conditions = conditions
+  end
+
+  def matches?(record)
+    conditions.all? do |condition|
+      condition.matches?(record)
+    end
+  end
+
+  def self.wrap(conditions)
+    return conditions if conditions.is_a?(self)
+
+    new(conditions)
+  end
+end

lib/active_hash/relation.rb

@@ -7,24 +7,80 @@ class Relation
     delegate :empty?, :length, :first, :second, :third, :last, to: :records
     delegate :sample, to: :records
 
-    def initialize(klass, all_records, query_hash = nil)
+    attr_reader :conditions, :order_values, :klass, :all_records
+
+    def initialize(klass, all_records, conditions = nil, order_values = nil)
       self.klass = klass
       self.all_records = all_records
-      self.query_hash = query_hash
-      self.records_dirty = false
+      self.conditions = Conditions.wrap(conditions || [])
+      self.order_values = order_values || []
+    end
+
+    def where(conditions_hash = :chain)
+      return WhereChain.new(self) if conditions_hash == :chain
+
+      spawn.where!(conditions_hash)
+    end
+
+    class WhereChain
+      attr_reader :relation
+
+      def initialize(relation)
+        @relation = relation
+      end
+
+      def not(conditions_hash)
+        relation.conditions << Condition.new(conditions_hash).invert!
+        relation
+      end
+    end
+
+    def order(*options)
+      spawn.order!(*options)
+    end
+
+    def reorder(*options)
+      spawn.reorder!(*options)
+    end
+
+    def where!(conditions_hash, inverted = false)
+      self.conditions << Condition.new(conditions_hash)
       self
     end
 
-    def where(query_hash = :chain)
-      return ActiveHash::Base::WhereChain.new(self) if query_hash == :chain
+    def spawn
+      self.class.new(klass, all_records, conditions, order_values)
+    end
+
+    def order!(*options)
+      check_if_method_has_arguments!(:order, options)
+      self.order_values += preprocess_order_args(options)
+      self
+    end
+
+    def reorder!(*options)
+      check_if_method_has_arguments!(:order, options)
+
+      self.order_values = preprocess_order_args(options)
+      @records = apply_order_values(records, order_values)
 
-      self.records_dirty = true unless query_hash.nil? || query_hash.keys.empty?
-      self.query_hash.merge!(query_hash || {})
+      self
+    end
+
+    def records
+      @records ||= begin
+        filtered_records = apply_conditions(all_records, conditions)
+        ordered_records = apply_order_values(filtered_records, order_values) # rubocop:disable Lint/UselessAssignment
+      end
+    end
+
+    def reload
+      @records = nil # Reset records
       self
     end
 
     def all(options = {})
-      if options.has_key?(:conditions)
+      if options.key?(:conditions)
         where(options[:conditions])
       else
         where({})
@@ -58,10 +114,11 @@ def find(id = nil, *args, &block)
     end
 
     def find_by_id(id)
-      return where(id: id).first if query_hash.present?
-
       index = klass.send(:record_index)[id.to_s] # TODO: Make index in Base publicly readable instead of using send?
-      index and records[index]
+      return unless index
+
+      record = all_records[index]
+      record if conditions.matches?(record)
     end
 
     def count
@@ -84,86 +141,32 @@ def pick(*column_names)
       pluck(*column_names).first
     end
 
-    def reload
-      @records = filter_all_records_by_query_hash
-    end
-
-    def order(*options)
-      check_if_method_has_arguments!(:order, options)
-      relation = where({})
-      return relation if options.blank?
-
-      processed_args = preprocess_order_args(options)
-      candidates = relation.dup
-
-      order_by_args!(candidates, processed_args)
-
-      candidates
-    end
-
     def to_ary
       records.dup
     end
 
     def method_missing(method_name, *args)
-      return super unless self.klass.scopes.key?(method_name)
+      return super unless klass.scopes.key?(method_name)
 
-      instance_exec(*args, &self.klass.scopes[method_name])
+      instance_exec(*args, &klass.scopes[method_name])
     end
 
-    attr_reader :query_hash, :klass, :all_records, :records_dirty
-
-    private
-
-    attr_writer :query_hash, :klass, :all_records, :records_dirty
-
-    def records
-      if !defined?(@records) || @records.nil? || records_dirty
-        reload
-      else
-        @records
-      end
+    def respond_to_missing?(method_name, include_private = false)
+      klass.scopes.key?(method_name) || super
     end
 
-    def filter_all_records_by_query_hash
-      self.records_dirty = false
-      return all_records if query_hash.blank?
-
-      # use index if searching by id
-      if query_hash.key?(:id) || query_hash.key?("id")
-        ids = (query_hash.delete(:id) || query_hash.delete("id"))
-        ids = range_to_array(ids) if ids.is_a?(Range)
-        candidates = Array.wrap(ids).map { |id| klass.find_by_id(id) }.compact
-      end
+    private
 
-      return candidates if query_hash.blank?
+    attr_writer :conditions, :order_values, :klass, :all_records
 
-      (candidates || all_records || []).select do |record|
-        match_options?(record, query_hash)
-      end
-    end
+    def apply_conditions(records, conditions)
+      return records if conditions.blank?
 
-    def match_options?(record, options)
-      options.all? do |col, match|
-        if match.kind_of?(Array)
-          match.any? { |v| normalize(v) == normalize(record[col]) }
-        else
-          normalize(match) === normalize(record[col])
-        end
+      records.select do |record|
+        conditions.matches?(record)
       end
     end
 
-    def normalize(v)
-      v.respond_to?(:to_sym) ? v.to_sym : v
-    end
-
-    def range_to_array(range)
-      return range.to_a unless range.end.nil?
-
-      e = records.last[:id]
-      (range.begin..e).to_a
-    end
-
     def check_if_method_has_arguments!(method_name, args)
       return unless args.blank?
 
@@ -179,7 +182,9 @@ def preprocess_order_args(order_args)
       ary.map! { |e| e.split(/\W+/) }.reverse!
     end
 
-    def order_by_args!(candidates, args)
+    def apply_order_values(records, args)
+      ordered_records = records.dup
+
       args.each do |arg|
         field, dir = if arg.is_a?(Hash)
                        arg.to_a.flatten.map(&:to_sym)
@@ -189,14 +194,16 @@ def order_by_args!(candidates, args)
                        arg.to_sym
                      end
 
-        candidates.sort! do |a, b|
+        ordered_records.sort! do |a, b|
           if dir.present? && dir.to_sym.upcase.equal?(:DESC)
             b[field] <=> a[field]
           else
             a[field] <=> b[field]
           end
         end
       end
+
+      ordered_records
     end
   end
 end