Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor Relation and conditions to fix mutating scopes issues #268

Merged
merged 3 commits into from
Mar 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand All @@ -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:
```
Expand Down
2 changes: 2 additions & 0 deletions lib/active_hash.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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'
Expand Down
46 changes: 3 additions & 43 deletions lib/active_hash/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Comment on lines +166 to +168
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason to change this block?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the Relation could be initialized with the conditions directly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The initializer has:

self.conditions = Conditions.wrap(conditions || [])

I wondered if there was a reason to not use that.
The new code passes a nil (so it gets and empty array)
and the where! then adds to that conditions.

Is this to ensure we don't modify the options[:conditions]?

I kinda like the original way better but if this has a reason then 👍

end

delegate :where, :find, :find_by, :find_by!, :find_by_id, :count, :pluck, :ids, :pick, :first, :last, :order, to: :all
Expand Down
44 changes: 44 additions & 0 deletions lib/active_hash/condition.rb
Original file line number Diff line number Diff line change
@@ -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.cover?(value) if comparison.is_a?(Range)
return comparison.match?(value) if comparison.is_a?(Regexp)

normalize(value) == normalize(comparison)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you talk more on this?

The first thing I tested was:

(1.0).to_s == 1.to_s

But cases like this work great (which are probably more relevant since this will fix the very common find_by(id: "4"):

"4".to_s == "4".to_s

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes; this is to make ActiveHash behave similar to how ActiveRecord behaves for querying.

ActiveRecord (or rather the DB) does a similar typecasting when querying:

# AR
User.find("2").id # => 2
User.find_by(id: "2").id # => 2
User.find_by(username: :john).username # => "john"

The normalize with typecasting to string here makes ActiveHash behave similar:

# AH
City.find("2").id # => 2
City.find_by(id: "2").id # => 2
City.find_by(country_code: :gb).country_code # => "gb"

.. without this normalization, no records would be returned in the examples above for AH, while it would for AR.

end

def normalize(value)
value.respond_to?(:to_s) ? value.to_s : value
end
end
21 changes: 21 additions & 0 deletions lib/active_hash/conditions.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
class ActiveHash::Relation::Conditions
attr_reader :conditions

delegate :<<, :map, 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
Loading