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

Filter properties in the database instead of in memory #5

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from 5 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
9 changes: 0 additions & 9 deletions lib/rails-properties.rb
Original file line number Diff line number Diff line change
@@ -1,12 +1,3 @@
module RailsProperties
# In Rails 3, attributes can be protected by `attr_accessible` and `attr_protected`
# In Rails 4, attributes can be protected by using the gem `protected_attributes`
# In Rails 5, protecting attributes is obsolete (there are `StrongParameters` only)
def self.can_protect_attributes?
(ActiveRecord::VERSION::MAJOR == 3) || defined?(ProtectedAttributes)
end
end

require 'rails-properties/property_object'
require 'rails-properties/configuration'
require 'rails-properties/base'
Expand Down
12 changes: 7 additions & 5 deletions lib/rails-properties/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,7 @@ def properties(var)
raise ArgumentError unless var.is_a?(Symbol)
raise ArgumentError.new("Unknown key: #{var}") unless self.class.default_properties[var]

if RailsProperties.can_protect_attributes?
property_objects.detect { |s| s.var == var.to_s } || property_objects.build({ :var => var.to_s }, :without_protection => true)
else
property_objects.detect { |s| s.var == var.to_s } || property_objects.build(:var => var.to_s, :target => self)
end
cached_properties[var] ||= property_objects.find_or_initialize_by(var: var)
Copy link
Author

@alimi alimi May 30, 2024

Choose a reason for hiding this comment

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

This memoization is being done to support new property objects. If a property object isn't in the database, property_objects.find_or_initialize_by(var: var) returns an new object in memory. Since we can write multiple values to a new property like

account.properties(:portal).premium = true
account.properties(:portal).fee = 42.5

#properties needs to return the same instance of the property object. Otherwise, we'll have two different property objects for the same var (:portal in the above example), and that's not allowed.

This memoization was happening implicitly before when everything was being loading into memory.

end

def properties=(value)
Expand All @@ -42,6 +38,12 @@ def to_properties_hash
end
properties_hash
end

private

def cached_properties
@cached_properties ||= {}
end
end
end
end
Expand Down
14 changes: 0 additions & 14 deletions lib/rails-properties/property_object.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,6 @@ class PropertyObject < ActiveRecord::Base

serialize :value, Hash

if RailsProperties.can_protect_attributes?
# attr_protected can not be used here because it touches the database which is not connected yet.
# So allow no attributes and override <tt>#sanitize_for_mass_assignment</tt>
attr_accessible
end

REGEX_SETTER = /\A([a-z]\w+)=\Z/i
REGEX_GETTER = /\A([a-z]\w+)\Z/i

Expand All @@ -44,14 +38,6 @@ def method_missing(method_name, *args, &block)
end
end

protected
if RailsProperties.can_protect_attributes?
# Simulate attr_protected by removing all regular attributes
def sanitize_for_mass_assignment(attributes, role = nil)
attributes.except('id', 'var', 'value', 'target_id', 'target_type', 'created_at', 'updated_at')
end
end

private
def _get_value(name)
if value[name].nil?
Expand Down
2 changes: 1 addition & 1 deletion lib/rails-properties/version.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module RailsProperties
VERSION = '3.4.3'
VERSION = '4.0.0'
Copy link
Author

Choose a reason for hiding this comment

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

This feels like a big enough change to warrant a major version bump.

end
4 changes: 2 additions & 2 deletions rails-properties.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ Gem::Specification.new do |gem|
gem.test_files = gem.files.grep(%r{^(test|spec|features)/})
gem.require_paths = ['lib']

gem.add_dependency 'activerecord', '>= 3.1'
gem.add_dependency 'activerecord', '>= 6.0'

gem.add_development_dependency 'rake'
gem.add_development_dependency 'sqlite3'
gem.add_development_dependency 'sqlite3', "~> 1.4"
gem.add_development_dependency 'rspec'
gem.add_development_dependency 'coveralls'
gem.add_development_dependency 'simplecov', RUBY_VERSION < '2' ? '~> 0.11.2' : '>= 0.11.2'
Expand Down
4 changes: 2 additions & 2 deletions spec/properties_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@
end

it "should add properties" do
user.properties(:dashboard).update_attributes! :smart => true
user.properties(:dashboard).update! :smart => true

user.reload
expect(user.properties(:dashboard).smart).to eq(true)
Expand Down Expand Up @@ -179,7 +179,7 @@
end

it "should update properties" do
user.properties(:dashboard).update_attributes! :smart => true
user.properties(:dashboard).update! :smart => true
user.reload

expect(user.properties(:dashboard).smart).to eq(true)
Expand Down
22 changes: 4 additions & 18 deletions spec/property_object_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,8 @@
describe RailsProperties::PropertyObject do
let(:user) { User.create! :name => 'Mr. Pink' }

if RailsProperties.can_protect_attributes?
let(:new_property_object) { user.property_objects.build({ :var => 'dashboard'}, :without_protection => true) }
let(:saved_property_object) { user.property_objects.create!({ :var => 'dashboard', :value => { 'theme' => 'pink', 'filter' => false}}, :without_protection => true) }
else
let(:new_property_object) { user.property_objects.build({ :var => 'dashboard'}) }
let(:saved_property_object) { user.property_objects.create!({ :var => 'dashboard', :value => { 'theme' => 'pink', 'filter' => false}}) }
end
let(:new_property_object) { user.property_objects.build({ :var => 'dashboard'}) }
let(:saved_property_object) { user.property_objects.create!({ :var => 'dashboard', :value => { 'theme' => 'pink', 'filter' => false}}) }

describe "serialization" do
it "should have a hash default" do
Expand Down Expand Up @@ -105,7 +100,7 @@

describe "update_attributes" do
it 'should save' do
expect(new_property_object.update_attributes(:foo => 42, :bar => 'string')).to be_truthy
expect(new_property_object.update(:foo => 42, :bar => 'string')).to be_truthy
new_property_object.reload

expect(new_property_object.foo).to eq(42)
Expand All @@ -115,16 +110,7 @@
end

it 'should not save blank hash' do
expect(new_property_object.update_attributes({})).to be_truthy
end

if RailsProperties.can_protect_attributes?
it 'should not allow changing protected attributes' do
new_property_object.update_attributes!(:var => 'calendar', :foo => 42)

expect(new_property_object.var).to eq('dashboard')
expect(new_property_object.foo).to eq(42)
end
expect(new_property_object.update({})).to be_truthy
end
end

Expand Down
4 changes: 2 additions & 2 deletions spec/queries_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
user.properties(:dashboard).bar = 'string'
user.properties(:calendar).bar = 'string'
user.save!
}.to perform_queries(3)
}.to perform_queries(4)
Copy link
Author

@alimi alimi May 30, 2024

Choose a reason for hiding this comment

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

The number of queries increases because we're not loading all of the target's property objects into memory anymore. So we have one query to get the user's :dashboard property object and another to get the user's :calendar property object.

end
end

Expand Down Expand Up @@ -94,7 +94,7 @@

it "should update properties by one SQL query" do
expect {
user.properties(:dashboard).update_attributes! :foo => 'bar'
user.properties(:dashboard).update! :foo => 'bar'
}.to perform_queries(1)
end
end
Expand Down
2 changes: 1 addition & 1 deletion spec/serialize_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@

describe 'updated properties' do
it 'should be serialized' do
user.properties(:dashboard).update_attributes! :smart => true
user.properties(:dashboard).update! :smart => true

dashboard_properties = user.property_objects.where(:var => 'dashboard').first
calendar_properties = user.property_objects.where(:var => 'calendar').first
Expand Down