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

Fix attribute change detection on latest Rails #95

Merged
merged 1 commit into from
Nov 3, 2022
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
2 changes: 1 addition & 1 deletion activerecord-typedstore.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ Gem::Specification.new do |spec|
spec.add_dependency 'activerecord', '>= 6.1'

spec.add_development_dependency 'bundler'
spec.add_development_dependency 'rake', '~> 10'
spec.add_development_dependency 'rake'
spec.add_development_dependency 'rspec', '~> 3'
spec.add_development_dependency 'sqlite3', '~> 1'
spec.add_development_dependency 'database_cleaner', '~> 1'
Expand Down
19 changes: 19 additions & 0 deletions lib/active_record/typed_store/behavior.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,5 +71,24 @@ def attribute?(attr_name)
super
end
end

private

def attribute_names_for_partial_inserts
# Contrary to all vanilla Rails types, typedstore attribute have an inherent default
# value that doesn't match the database column default.
# As such we need to insert them on partial inserts even if they weren't changed.
super | self.class.typed_stores.keys.map(&:to_s)
end

def attribute_names_for_partial_updates
# On partial updates we shouldn't need to force stores to be persisted. However since
# we weren't persisting them for a while on insertion, we now need to gracefully deal
# with existing records that may have been persisted with a `NULL` store
# We use `blank?` as an heuristic to detect these.
super | self.class.typed_stores.keys.map(&:to_s).select do |store|
@attributes.key?(store) && @attributes[store].value_before_type_cast.blank?
end
end
end
end
7 changes: 1 addition & 6 deletions lib/active_record/typed_store/type.rb
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,7 @@ def default_value?(value)

def changed_in_place?(raw_old_value, value)
return false if value.nil?
if ActiveRecord.version.segments.first >= 5
raw_new_value = serialize(value)
else
# 4.2 capability
raw_new_value = type_cast_for_database(value)
end
raw_new_value = serialize(value)
raw_old_value.nil? != raw_new_value.nil? || raw_old_value != raw_new_value
end
end
Expand Down
20 changes: 20 additions & 0 deletions spec/active_record/typed_store_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -954,3 +954,23 @@
expect(model.settings[:new_attribute]).to be == '42'
end
end

describe DirtyTrackingModel do
it 'stores the default on creation' do
model = DirtyTrackingModel.create!
expect(model.settings_before_type_cast).to_not be_blank
end

it 'handles loaded records having uninitialized defaults' do
model = DirtyTrackingModel.create!
DirtyTrackingModel.update_all("settings = NULL") # bypass validation
model = DirtyTrackingModel.find(model.id)
expect(model.settings_changed?).to be false
expect(model.changes).to be_empty

model.update!(title: "Hello")

expect(model.settings_changed?).to be false
expect(model.changes).to be_empty
end
end
33 changes: 28 additions & 5 deletions spec/support/models.rb
Original file line number Diff line number Diff line change
Expand Up @@ -81,15 +81,21 @@ def define_stores_with_prefix_and_suffix(**options)
typed_store(:custom_suffixed_settings, suffix: :custom, **options) { |t| t.any :language }
end

MigrationClass = ActiveRecord::Migration["5.0"]
MigrationClass = ActiveRecord::Migration["#{ActiveRecord::VERSION::MAJOR}.#{ActiveRecord::VERSION::MINOR}"]
class CreateAllTables < MigrationClass

def self.up
ActiveRecord::Base.establish_connection(ActiveRecord::Base.configurations.configs_for(env_name: "test", name: :test_sqlite3))
create_table(:sqlite3_regular_ar_models, force: true) { |t| define_columns(t); t.text :untyped_settings }
create_table(:yaml_typed_store_models, force: true) { |t| %i[settings explicit_settings partial_settings untyped_settings prefixed_settings suffixed_settings custom_prefixed_settings custom_suffixed_settings].each { |column| t.text column} }
create_table(:json_typed_store_models, force: true) { |t| %i[settings explicit_settings partial_settings untyped_settings prefixed_settings suffixed_settings custom_prefixed_settings custom_suffixed_settings].each { |column| t.text column} }
create_table(:marshal_typed_store_models, force: true) { |t| %i[settings explicit_settings partial_settings untyped_settings prefixed_settings suffixed_settings custom_prefixed_settings custom_suffixed_settings].each { |column| t.text column} }
create_table(:yaml_typed_store_models, force: true) { |t| %i[settings explicit_settings partial_settings untyped_settings prefixed_settings suffixed_settings custom_prefixed_settings custom_suffixed_settings].each { |column| t.text column}; t.string :regular_column }
create_table(:json_typed_store_models, force: true) { |t| %i[settings explicit_settings partial_settings untyped_settings prefixed_settings suffixed_settings custom_prefixed_settings custom_suffixed_settings].each { |column| t.text column}; t.string :regular_column }
create_table(:marshal_typed_store_models, force: true) { |t| %i[settings explicit_settings partial_settings untyped_settings prefixed_settings suffixed_settings custom_prefixed_settings custom_suffixed_settings].each { |column| t.text column}; t.string :regular_column }

create_table(:dirty_tracking_models, force: true) do |t|
t.string :title
t.text :settings

t.timestamps
end
end
end
ActiveRecord::Migration.verbose = true
Expand Down Expand Up @@ -123,6 +129,11 @@ class YamlTypedStoreModel < ActiveRecord::Base
establish_connection :test_sqlite3
store :untyped_settings, accessors: [:title]

after_update :read_active
def read_active
enabled
end

define_store_with_attributes
define_store_with_no_attributes
define_store_with_partial_attributes
Expand Down Expand Up @@ -177,3 +188,15 @@ class MarshalTypedStoreModel < ActiveRecord::Base
JsonTypedStoreModel,
MarshalTypedStoreModel
]

class DirtyTrackingModel < ActiveRecord::Base
after_update :read_active

typed_store(:settings) do |f|
f.boolean :active, default: false, null: false
end

def read_active
active
end
end