Add a setting to specify that all string columns should be immutable
In Rails 4.2 we introduced mutation detection, to remove the need to call `attribute_will_change!` after modifying a field. One side effect of that change was that we needed to enforce that the `_before_type_cast` form of a value is a different object than the post type cast value, if the value is mutable. That contract is really only relevant for strings, but it meant we needed to dup them. In Rails 5 we added the `ImmutableString` type, to allow people to opt out of this duping in places where the memory usage was causing problems, and they don't need to mutate that field. This takes that a step further, and adds a class-level setting to specify whether strings should be frozen by default or not. The default value of this setting is `false` (strings are mutable), and I do not plan on changing that. While I think that immutable strings by default would be a reasonable default for new applications, I do not think that we would be able to document the value of this setting in a place that people will be looking when they can't figure out why some string is frozen. Realistically, the number of applications where this setting is relevant is fairly small, so I don't think it would make sense to ever enable it by default.
This commit is contained in:
parent
7118c43599
commit
332c3364b6
@ -11,6 +11,14 @@ def changed_in_place?(raw_old_value, new_value)
|
||||
end
|
||||
end
|
||||
|
||||
def to_immutable_string
|
||||
ImmutableString.new(
|
||||
limit: limit,
|
||||
precision: precision,
|
||||
scale: scale,
|
||||
)
|
||||
end
|
||||
|
||||
private
|
||||
def cast_value(value)
|
||||
case value
|
||||
|
@ -1,3 +1,10 @@
|
||||
* Added the setting `ActiveRecord::Base.immutable_strings_by_default`, which
|
||||
allows you to specify that all string columns should be frozen unless
|
||||
otherwise specified. This will reduce memory pressure for applications which
|
||||
do not generally mutate string properties of Active Record objects.
|
||||
|
||||
*Sean Griffin*
|
||||
|
||||
* Deprecate `map!` and `collect!` on `ActiveRecord::Result`.
|
||||
|
||||
*Ryuta Kamizono*
|
||||
|
@ -117,6 +117,15 @@ module ModelSchema
|
||||
# during an ordered finder call. Useful when the primary key is not an
|
||||
# auto-incrementing integer, for example when it's a UUID. Records are subsorted
|
||||
# by the primary key if it exists to ensure deterministic results.
|
||||
|
||||
##
|
||||
# :singleton-method: immutable_strings_by_default=
|
||||
# :call-seq: immutable_strings_by_default=(bool)
|
||||
#
|
||||
# Determines whether columns should infer their type as `:string` or
|
||||
# `:immutable_string`. This setting does not affect the behavior of
|
||||
# `attribute :foo, :string`. Defaults to false.
|
||||
|
||||
included do
|
||||
mattr_accessor :primary_key_prefix_type, instance_writer: false
|
||||
|
||||
@ -126,6 +135,7 @@ module ModelSchema
|
||||
class_attribute :internal_metadata_table_name, instance_accessor: false, default: "ar_internal_metadata"
|
||||
class_attribute :pluralize_table_names, instance_writer: false, default: true
|
||||
class_attribute :implicit_order_column, instance_accessor: false
|
||||
class_attribute :immutable_strings_by_default, instance_accessor: false
|
||||
|
||||
self.protected_environments = ["production"]
|
||||
self.inheritance_column = "type"
|
||||
@ -495,9 +505,11 @@ def load_schema!
|
||||
columns_hash = columns_hash.except(*ignored_columns) unless ignored_columns.empty?
|
||||
@columns_hash = columns_hash.freeze
|
||||
@columns_hash.each do |name, column|
|
||||
type = connection.lookup_cast_type_from_column(column)
|
||||
type = _convert_type_from_options(type)
|
||||
define_attribute(
|
||||
name,
|
||||
connection.lookup_cast_type_from_column(column),
|
||||
type,
|
||||
default: column.default,
|
||||
user_provided_default: false
|
||||
)
|
||||
@ -546,6 +558,14 @@ def compute_table_name
|
||||
base_class.table_name
|
||||
end
|
||||
end
|
||||
|
||||
def _convert_type_from_options(type)
|
||||
if immutable_strings_by_default && type.respond_to?(:to_immutable_string)
|
||||
type.to_immutable_string
|
||||
else
|
||||
type
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -63,6 +63,7 @@ def current_adapter_name
|
||||
Decimal = ActiveModel::Type::Decimal
|
||||
Float = ActiveModel::Type::Float
|
||||
Integer = ActiveModel::Type::Integer
|
||||
ImmutableString = ActiveModel::Type::ImmutableString
|
||||
String = ActiveModel::Type::String
|
||||
Value = ActiveModel::Type::Value
|
||||
|
||||
@ -74,6 +75,7 @@ def current_adapter_name
|
||||
register(:decimal, Type::Decimal, override: false)
|
||||
register(:float, Type::Float, override: false)
|
||||
register(:integer, Type::Integer, override: false)
|
||||
register(:immutable_string, Type::ImmutableString, override: false)
|
||||
register(:json, Type::Json, override: false)
|
||||
register(:string, Type::String, override: false)
|
||||
register(:text, Type::Text, override: false)
|
||||
|
@ -111,22 +111,24 @@ class CustomPropertiesTest < ActiveRecord::TestCase
|
||||
|
||||
test "overloading properties does not attribute method order" do
|
||||
attribute_names = OverloadedType.attribute_names
|
||||
assert_equal %w(id overloaded_float unoverloaded_float overloaded_string_with_limit string_with_default non_existent_decimal), attribute_names
|
||||
expected = OverloadedType.column_names + ["non_existent_decimal"]
|
||||
assert_equal expected, attribute_names
|
||||
end
|
||||
|
||||
test "caches are cleared" do
|
||||
klass = Class.new(OverloadedType)
|
||||
column_count = klass.columns.length
|
||||
|
||||
assert_equal 6, klass.attribute_types.length
|
||||
assert_equal 6, klass.column_defaults.length
|
||||
assert_equal 6, klass.attribute_names.length
|
||||
assert_equal column_count + 1, klass.attribute_types.length
|
||||
assert_equal column_count + 1, klass.column_defaults.length
|
||||
assert_equal column_count + 1, klass.attribute_names.length
|
||||
assert_not klass.attribute_types.include?("wibble")
|
||||
|
||||
klass.attribute :wibble, Type::Value.new
|
||||
|
||||
assert_equal 7, klass.attribute_types.length
|
||||
assert_equal 7, klass.column_defaults.length
|
||||
assert_equal 7, klass.attribute_names.length
|
||||
assert_equal column_count + 2, klass.attribute_types.length
|
||||
assert_equal column_count + 2, klass.column_defaults.length
|
||||
assert_equal column_count + 2, klass.attribute_names.length
|
||||
assert_includes klass.attribute_types, "wibble"
|
||||
end
|
||||
|
||||
@ -292,5 +294,37 @@ def deserialize(*)
|
||||
assert_equal 1, klass.new(no_type: 1).no_type
|
||||
assert_equal "foo", klass.new(no_type: "foo").no_type
|
||||
end
|
||||
|
||||
test "immutable_strings_by_default changes schema inference for string columns" do
|
||||
with_immutable_strings do
|
||||
OverloadedType.reset_column_information
|
||||
immutable_string_type = Type.lookup(:immutable_string).class
|
||||
assert_instance_of immutable_string_type, OverloadedType.type_for_attribute("inferred_string")
|
||||
end
|
||||
end
|
||||
|
||||
test "immutable_strings_by_default retains limit information" do
|
||||
with_immutable_strings do
|
||||
OverloadedType.reset_column_information
|
||||
assert_equal 255, OverloadedType.type_for_attribute("inferred_string").limit
|
||||
end
|
||||
end
|
||||
|
||||
test "immutable_strings_by_default does not affect `attribute :foo, :string`" do
|
||||
with_immutable_strings do
|
||||
OverloadedType.reset_column_information
|
||||
default_string_type = Type.lookup(:string).class
|
||||
assert_instance_of default_string_type, OverloadedType.type_for_attribute("string_with_default")
|
||||
end
|
||||
end
|
||||
|
||||
private
|
||||
def with_immutable_strings
|
||||
old_value = ActiveRecord::Base.immutable_strings_by_default
|
||||
ActiveRecord::Base.immutable_strings_by_default = true
|
||||
yield
|
||||
ensure
|
||||
ActiveRecord::Base.immutable_strings_by_default = old_value
|
||||
end
|
||||
end
|
||||
end
|
||||
|
@ -1125,6 +1125,7 @@
|
||||
t.float :unoverloaded_float
|
||||
t.string :overloaded_string_with_limit, limit: 255
|
||||
t.string :string_with_default, default: "the original default"
|
||||
t.string :inferred_string, limit: 255
|
||||
end
|
||||
|
||||
create_table :users, force: true do |t|
|
||||
|
Loading…
Reference in New Issue
Block a user