From 101c19f55f5f1d86d35574b805278f11e9a1a48e Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Fri, 6 Feb 2015 11:05:38 -0700 Subject: [PATCH] Allow a symbol to be passed to `attribute`, in place of a type object The same is not true of `define_attribute`, which is meant to be the low level no-magic API that sits underneath. The differences between the two APIs are: - `attribute` - Lazy (the attribute will be defined after the schema has loaded) - Allows either a type object or a symbol - `define_attribute` - Runs immediately (might get trampled by schema loading) - Requires a type object This was the last blocker in terms of public interface requirements originally discussed for this feature back in May. All the implementation blockers have been cleared, so this feature is probably ready for release (pending one more look-over by me). --- activerecord/lib/active_record/attributes.rb | 13 ++++-- .../connection_adapters/abstract/quoting.rb | 21 ++++++++++ .../abstract_mysql_adapter.rb | 4 ++ .../postgresql/oid/array.rb | 6 +++ .../postgresql/oid/range.rb | 8 +++- .../connection_adapters/postgresql/quoting.rb | 41 ++++++++++++++++++ .../connection_adapters/sqlite3_adapter.rb | 4 ++ .../test/cases/attribute_decorators_test.rb | 6 +-- activerecord/test/cases/attributes_test.rb | 42 ++++++++++++++++--- activerecord/test/cases/base_test.rb | 4 +- activerecord/test/cases/calculations_test.rb | 6 +-- activerecord/test/cases/dirty_test.rb | 2 +- activerecord/test/cases/migration_test.rb | 4 +- activerecord/test/cases/schema_dumper_test.rb | 5 +++ activerecord/test/cases/type/integer_test.rb | 2 +- activerecord/test/cases/validations_test.rb | 2 +- 16 files changed, 148 insertions(+), 22 deletions(-) diff --git a/activerecord/lib/active_record/attributes.rb b/activerecord/lib/active_record/attributes.rb index 7cb6b075a0..f34e6cf912 100644 --- a/activerecord/lib/active_record/attributes.rb +++ b/activerecord/lib/active_record/attributes.rb @@ -44,7 +44,7 @@ module ClassMethods # :nodoc: # store_listing.price_in_cents # => BigDecimal.new(10.1) # # class StoreListing < ActiveRecord::Base - # attribute :price_in_cents, Type::Integer.new + # attribute :price_in_cents, :integer # end # # # after @@ -77,7 +77,10 @@ def attribute(name, cast_type, **options) name = name.to_s reload_schema_from_cache - self.attributes_to_define_after_schema_loads = attributes_to_define_after_schema_loads.merge(name => [cast_type, options]) + self.attributes_to_define_after_schema_loads = + attributes_to_define_after_schema_loads.merge( + name => [cast_type, options] + ) end def define_attribute( @@ -93,7 +96,11 @@ def define_attribute( def load_schema! super attributes_to_define_after_schema_loads.each do |name, (type, options)| - define_attribute(name, type, **options) + if type.is_a?(Symbol) + type = connection.type_for_attribute_options(type, **options.except(:default)) + end + + define_attribute(name, type, **options.slice(:default)) end end diff --git a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb index 2c013a074a..55d3360070 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract/quoting.rb @@ -134,8 +134,29 @@ def prepare_binds_for_database(binds) # :nodoc: binds.map(&:value_for_database) end + def type_for_attribute_options(type_name, **options) + klass = type_classes_with_standard_constructor.fetch(type_name, Type::Value) + klass.new(**options) + end + private + def type_classes_with_standard_constructor + { + big_integer: Type::BigInteger, + binary: Type::Binary, + boolean: Type::Boolean, + date: Type::Date, + date_time: Type::DateTime, + decimal: Type::Decimal, + float: Type::Float, + integer: Type::Integer, + string: Type::String, + text: Type::Text, + time: Type::Time, + } + end + def types_which_need_no_typecasting [nil, Numeric, String] end diff --git a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb index 5c8c4b883a..61bac6741f 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -947,6 +947,10 @@ def cast_value(value) end end end + + def type_classes_with_standard_constructor + super.merge(string: MysqlString, date_time: MysqlDateTime) + end end end end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid/array.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid/array.rb index e45a2f59d9..0a0a7fdbb3 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/oid/array.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid/array.rb @@ -48,6 +48,12 @@ def type_cast_for_database(value) end end + def ==(other) + other.is_a?(Array) && + subtype == other.subtype && + delimiter == other.delimiter + end + private def type_cast_array(value, method) diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid/range.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid/range.rb index 3adfb8b9d8..2a5a59fbc6 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/oid/range.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid/range.rb @@ -7,7 +7,7 @@ module OID # :nodoc: class Range < Type::Value # :nodoc: attr_reader :subtype, :type - def initialize(subtype, type) + def initialize(subtype, type = :range) @subtype = subtype @type = type end @@ -40,6 +40,12 @@ def type_cast_for_database(value) end end + def ==(other) + other.is_a?(Range) && + other.subtype == subtype && + other.type == type + end + private def type_cast_single(value) diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb index 464adb4e23..11114f32fe 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb @@ -65,6 +65,23 @@ def lookup_cast_type_from_column(column) # :nodoc: type_map.lookup(column.oid, column.fmod, column.sql_type) end + def type_for_attribute_options( + type_name, + array: false, + range: false, + **options + ) + if array + subtype = type_for_attribute_options(type_name, **options) + OID::Array.new(subtype) + elsif range + subtype = type_for_attribute_options(type_name, **options) + OID::Range.new(subtype) + else + super(type_name, **options) + end + end + private def _quote(value) @@ -103,6 +120,30 @@ def _type_cast(value) super end end + + def type_classes_with_standard_constructor + super.merge( + bit: OID::Bit, + bit_varying: OID::BitVarying, + binary: OID::Bytea, + cidr: OID::Cidr, + date: OID::Date, + date_time: OID::DateTime, + decimal: OID::Decimal, + enum: OID::Enum, + float: OID::Float, + hstore: OID::Hstore, + inet: OID::Inet, + json: OID::Json, + jsonb: OID::Jsonb, + money: OID::Money, + point: OID::Point, + time: OID::Time, + uuid: OID::Uuid, + vector: OID::Vector, + xml: OID::Xml, + ) + end end end end diff --git a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb index c06213a7bf..edd060248f 100644 --- a/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb @@ -240,6 +240,10 @@ def _type_cast(value) # :nodoc: end end + def type_classes_with_standard_constructor + super.merge(binary: SQLite3Binary) + end + def quote_string(s) #:nodoc: @connection.class.quote(s) end diff --git a/activerecord/test/cases/attribute_decorators_test.rb b/activerecord/test/cases/attribute_decorators_test.rb index 9ad02ffae8..0b96319cbd 100644 --- a/activerecord/test/cases/attribute_decorators_test.rb +++ b/activerecord/test/cases/attribute_decorators_test.rb @@ -51,7 +51,7 @@ def type_cast_from_user(value) end test "undecorated columns are not touched" do - Model.attribute :another_string, Type::String.new, default: 'something or other' + Model.attribute :another_string, :string, default: 'something or other' Model.decorate_attribute_type(:a_string, :test) { |t| StringDecorator.new(t) } assert_equal 'something or other', Model.new.another_string @@ -86,7 +86,7 @@ def type_cast_from_user(value) end test "decorating attributes does not modify parent classes" do - Model.attribute :another_string, Type::String.new, default: 'whatever' + Model.attribute :another_string, :string, default: 'whatever' Model.decorate_attribute_type(:a_string, :test) { |t| StringDecorator.new(t) } child_class = Class.new(Model) child_class.decorate_attribute_type(:another_string, :test) { |t| StringDecorator.new(t) } @@ -110,7 +110,7 @@ def type_cast_from_user(value) end test "decorating with a proc" do - Model.attribute :an_int, Type::Integer.new + Model.attribute :an_int, :integer type_is_integer = proc { |_, type| type.type == :integer } Model.decorate_matching_attribute_types type_is_integer, :multiplier do |type| Multiplier.new(type) diff --git a/activerecord/test/cases/attributes_test.rb b/activerecord/test/cases/attributes_test.rb index 6f331c5985..a753e8b74e 100644 --- a/activerecord/test/cases/attributes_test.rb +++ b/activerecord/test/cases/attributes_test.rb @@ -1,17 +1,17 @@ require 'cases/helper' class OverloadedType < ActiveRecord::Base - attribute :overloaded_float, Type::Integer.new - attribute :overloaded_string_with_limit, Type::String.new(limit: 50) - attribute :non_existent_decimal, Type::Decimal.new - attribute :string_with_default, Type::String.new, default: 'the overloaded default' + attribute :overloaded_float, :integer + attribute :overloaded_string_with_limit, :string, limit: 50 + attribute :non_existent_decimal, :decimal + attribute :string_with_default, :string, default: 'the overloaded default' end class ChildOfOverloadedType < OverloadedType end class GrandchildOfOverloadedType < ChildOfOverloadedType - attribute :overloaded_float, Type::Float.new + attribute :overloaded_float, :float end class UnoverloadedType < ActiveRecord::Base @@ -124,5 +124,37 @@ def type_cast_from_database(*) assert_equal "from user", model.wibble end + + if current_adapter?(:PostgreSQLAdapter) + test "arrays types can be specified" do + klass = Class.new(OverloadedType) do + attribute :my_array, :string, limit: 50, array: true + attribute :my_int_array, :integer, array: true + end + + string_array = ConnectionAdapters::PostgreSQL::OID::Array.new( + Type::String.new(limit: 50)) + int_array = ConnectionAdapters::PostgreSQL::OID::Array.new( + Type::Integer.new) + refute_equal string_array, int_array + assert_equal string_array, klass.type_for_attribute("my_array") + assert_equal int_array, klass.type_for_attribute("my_int_array") + end + + test "range types can be specified" do + klass = Class.new(OverloadedType) do + attribute :my_range, :string, limit: 50, range: true + attribute :my_int_range, :integer, range: true + end + + string_range = ConnectionAdapters::PostgreSQL::OID::Range.new( + Type::String.new(limit: 50)) + int_range = ConnectionAdapters::PostgreSQL::OID::Range.new( + Type::Integer.new) + refute_equal string_range, int_range + assert_equal string_range, klass.type_for_attribute("my_range") + assert_equal int_range, klass.type_for_attribute("my_int_range") + end + end end end diff --git a/activerecord/test/cases/base_test.rb b/activerecord/test/cases/base_test.rb index 7c5939fc47..ef1173a2ba 100644 --- a/activerecord/test/cases/base_test.rb +++ b/activerecord/test/cases/base_test.rb @@ -903,8 +903,8 @@ def test_default class NumericData < ActiveRecord::Base self.table_name = 'numeric_data' - attribute :my_house_population, Type::Integer.new - attribute :atoms_in_universe, Type::Integer.new + attribute :my_house_population, :integer + attribute :atoms_in_universe, :integer end def test_big_decimal_conditions diff --git a/activerecord/test/cases/calculations_test.rb b/activerecord/test/cases/calculations_test.rb index f47568f2f5..f0393aa6b1 100644 --- a/activerecord/test/cases/calculations_test.rb +++ b/activerecord/test/cases/calculations_test.rb @@ -15,9 +15,9 @@ class NumericData < ActiveRecord::Base self.table_name = 'numeric_data' - attribute :world_population, Type::Integer.new - attribute :my_house_population, Type::Integer.new - attribute :atoms_in_universe, Type::Integer.new + attribute :world_population, :integer + attribute :my_house_population, :integer + attribute :atoms_in_universe, :integer end class CalculationsTest < ActiveRecord::TestCase diff --git a/activerecord/test/cases/dirty_test.rb b/activerecord/test/cases/dirty_test.rb index 192ba6f7cd..c2573ac72b 100644 --- a/activerecord/test/cases/dirty_test.rb +++ b/activerecord/test/cases/dirty_test.rb @@ -711,7 +711,7 @@ def type_cast_for_database(value) test "attribute_will_change! doesn't try to save non-persistable attributes" do klass = Class.new(ActiveRecord::Base) do self.table_name = 'people' - attribute :non_persisted_attribute, ActiveRecord::Type::String.new + attribute :non_persisted_attribute, :string end record = klass.new(first_name: "Sean") diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index d969345361..51b0034755 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -15,9 +15,9 @@ class BigNumber < ActiveRecord::Base unless current_adapter?(:PostgreSQLAdapter, :SQLite3Adapter) - attribute :value_of_e, Type::Integer.new + attribute :value_of_e, :integer end - attribute :my_house_population, Type::Integer.new + attribute :my_house_population, :integer end class Reminder < ActiveRecord::Base; end diff --git a/activerecord/test/cases/schema_dumper_test.rb b/activerecord/test/cases/schema_dumper_test.rb index bafc9fa81b..5ca3f91cf0 100644 --- a/activerecord/test/cases/schema_dumper_test.rb +++ b/activerecord/test/cases/schema_dumper_test.rb @@ -78,6 +78,11 @@ def test_types_line_up end end.compact + if lengths.uniq.length != 1 + p lengths.uniq.length + puts column_set + end + assert_equal 1, lengths.uniq.length end end diff --git a/activerecord/test/cases/type/integer_test.rb b/activerecord/test/cases/type/integer_test.rb index 0c60f0690c..1e836f2142 100644 --- a/activerecord/test/cases/type/integer_test.rb +++ b/activerecord/test/cases/type/integer_test.rb @@ -113,7 +113,7 @@ class IntegerTest < ActiveRecord::TestCase test "values which are out of range can be re-assigned" do klass = Class.new(ActiveRecord::Base) do self.table_name = 'posts' - attribute :foo, Type::Integer.new + attribute :foo, :integer end model = klass.new diff --git a/activerecord/test/cases/validations_test.rb b/activerecord/test/cases/validations_test.rb index b0f34e5f47..f4f316f393 100644 --- a/activerecord/test/cases/validations_test.rb +++ b/activerecord/test/cases/validations_test.rb @@ -150,7 +150,7 @@ def test_validators def test_numericality_validation_with_mutation Topic.class_eval do - attribute :wibble, ActiveRecord::Type::String.new + attribute :wibble, :string validates_numericality_of :wibble, only_integer: true end