mirror of
https://github.com/rails/rails.git
synced 2022-11-09 12:12:34 -05:00
Correct the behavior of virtual attributes on models loaded from the db
Previously we had primarily tested the behavior of these attributes by calling `.new`, allowing this to slip through the cracks. There were a few ways in which they were behaving incorrectly. The biggest issue was that attempting to read the attribute would through a `MissingAttribute` error. We've corrected this by returning the default value when the attribute isn't backed by a database column. This is super special cased, but I don't see a way to avoid this conditional. I had considered handling this higher up in `define_default_attribute`, but we don't have the relevant information there as users can provide new defaults for database columns as well. Once I corrected this, I had noticed that the attributes were always being marked as changed. This is because the behavior of `define_default_attribute` was treating them as assigned from `Attribute::Null`. Finally, with our new implementation, `LazyAttributeHash` could no longer be marshalled, as it holds onto a proc. This has been corrected as well. I've not handled YAML in that class, as we do additional work higher up to avoid YAML dumping it at all. Fixes #25787 Close #25841
This commit is contained in:
parent
7ebef567ce
commit
f0ddf87e4b
6 changed files with 74 additions and 12 deletions
|
@ -152,7 +152,7 @@ module ActiveRecord
|
|||
end
|
||||
|
||||
def _original_value_for_database
|
||||
value_for_database
|
||||
type.serialize(original_value)
|
||||
end
|
||||
|
||||
class FromDatabase < Attribute # :nodoc:
|
||||
|
@ -180,7 +180,7 @@ module ActiveRecord
|
|||
value
|
||||
end
|
||||
|
||||
def changed_in_place_from?(old_value)
|
||||
def changed_in_place?
|
||||
false
|
||||
end
|
||||
end
|
||||
|
|
|
@ -3,7 +3,7 @@ require 'active_record/attribute_set/yaml_encoder'
|
|||
|
||||
module ActiveRecord
|
||||
class AttributeSet # :nodoc:
|
||||
delegate :each_value, to: :attributes
|
||||
delegate :each_value, :fetch, to: :attributes
|
||||
|
||||
def initialize(attributes)
|
||||
@attributes = attributes
|
||||
|
|
|
@ -3,11 +3,12 @@ require 'active_record/attribute'
|
|||
module ActiveRecord
|
||||
class AttributeSet # :nodoc:
|
||||
class Builder # :nodoc:
|
||||
attr_reader :types, :always_initialized
|
||||
attr_reader :types, :always_initialized, :default
|
||||
|
||||
def initialize(types, always_initialized = nil)
|
||||
def initialize(types, always_initialized = nil, &default)
|
||||
@types = types
|
||||
@always_initialized = always_initialized
|
||||
@default = default
|
||||
end
|
||||
|
||||
def build_from_database(values = {}, additional_types = {})
|
||||
|
@ -15,21 +16,22 @@ module ActiveRecord
|
|||
values[always_initialized] = nil
|
||||
end
|
||||
|
||||
attributes = LazyAttributeHash.new(types, values, additional_types)
|
||||
attributes = LazyAttributeHash.new(types, values, additional_types, &default)
|
||||
AttributeSet.new(attributes)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
class LazyAttributeHash # :nodoc:
|
||||
delegate :transform_values, :each_key, :each_value, to: :materialize
|
||||
delegate :transform_values, :each_key, :each_value, :fetch, to: :materialize
|
||||
|
||||
def initialize(types, values, additional_types)
|
||||
def initialize(types, values, additional_types, &default)
|
||||
@types = types
|
||||
@values = values
|
||||
@additional_types = additional_types
|
||||
@materialized = false
|
||||
@delegate_hash = {}
|
||||
@default = default || proc {}
|
||||
end
|
||||
|
||||
def key?(key)
|
||||
|
@ -76,9 +78,21 @@ module ActiveRecord
|
|||
end
|
||||
end
|
||||
|
||||
def marshal_dump
|
||||
materialize
|
||||
end
|
||||
|
||||
def marshal_load(delegate_hash)
|
||||
@delegate_hash = delegate_hash
|
||||
@types = {}
|
||||
@values = {}
|
||||
@additional_types = {}
|
||||
@materialized = true
|
||||
end
|
||||
|
||||
protected
|
||||
|
||||
attr_reader :types, :values, :additional_types, :delegate_hash
|
||||
attr_reader :types, :values, :additional_types, :delegate_hash, :default
|
||||
|
||||
def materialize
|
||||
unless @materialized
|
||||
|
@ -101,7 +115,7 @@ module ActiveRecord
|
|||
if value_present
|
||||
delegate_hash[name] = Attribute.from_database(name, value, type)
|
||||
elsif types.key?(name)
|
||||
delegate_hash[name] = Attribute.uninitialized(name, type)
|
||||
delegate_hash[name] = default.call(name) || Attribute.uninitialized(name, type)
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
|
@ -253,7 +253,7 @@ module ActiveRecord
|
|||
name,
|
||||
value,
|
||||
type,
|
||||
_default_attributes[name],
|
||||
_default_attributes.fetch(name.to_s) { nil },
|
||||
)
|
||||
else
|
||||
default_attribute = Attribute.from_database(name, value, type)
|
||||
|
|
|
@ -249,7 +249,11 @@ module ActiveRecord
|
|||
end
|
||||
|
||||
def attributes_builder # :nodoc:
|
||||
@attributes_builder ||= AttributeSet::Builder.new(attribute_types, primary_key)
|
||||
@attributes_builder ||= AttributeSet::Builder.new(attribute_types, primary_key) do |name|
|
||||
unless columns_hash.key?(name)
|
||||
_default_attributes[name].dup
|
||||
end
|
||||
end
|
||||
end
|
||||
|
||||
def columns_hash # :nodoc:
|
||||
|
|
|
@ -205,5 +205,49 @@ module ActiveRecord
|
|||
|
||||
assert_equal(:bar, child.new(foo: :bar).foo)
|
||||
end
|
||||
|
||||
test "attributes not backed by database columns are not dirty when unchanged" do
|
||||
refute OverloadedType.new.non_existent_decimal_changed?
|
||||
end
|
||||
|
||||
test "attributes not backed by database columns are always initialized" do
|
||||
OverloadedType.create!
|
||||
model = OverloadedType.first
|
||||
|
||||
assert_nil model.non_existent_decimal
|
||||
model.non_existent_decimal = "123"
|
||||
assert_equal 123, model.non_existent_decimal
|
||||
end
|
||||
|
||||
test "attributes not backed by database columns return the default on models loaded from database" do
|
||||
child = Class.new(OverloadedType) do
|
||||
attribute :non_existent_decimal, :decimal, default: 123
|
||||
end
|
||||
child.create!
|
||||
model = child.first
|
||||
|
||||
assert_equal 123, model.non_existent_decimal
|
||||
end
|
||||
|
||||
test "attributes not backed by database columns properly interact with mutation and dirty" do
|
||||
child = Class.new(ActiveRecord::Base) do
|
||||
self.table_name = "topics"
|
||||
attribute :foo, :string, default: "lol"
|
||||
end
|
||||
child.create!
|
||||
model = child.first
|
||||
|
||||
assert_equal "lol", model.foo
|
||||
|
||||
model.foo << "asdf"
|
||||
assert_equal "lolasdf", model.foo
|
||||
assert model.foo_changed?
|
||||
|
||||
model.reload
|
||||
assert_equal "lol", model.foo
|
||||
|
||||
model.foo = "lol"
|
||||
refute model.changed?
|
||||
end
|
||||
end
|
||||
end
|
||||
|
|
Loading…
Reference in a new issue