From ffc4710c2bff273b82ddb76675701f986d82ef4f Mon Sep 17 00:00:00 2001 From: Sean Griffin Date: Fri, 21 Aug 2015 17:45:46 -0600 Subject: [PATCH] JSON is still an adapter specific type. Several changes were made in #21110 which I am strongly opposed to. (this is what I get for going on vacation. :trollface:) No type should be introduced into the generic `ActiveRecord::Type` namespace, and *certainly* should not be registered into the registry unconstrained unless it is supported by *all* adapters (which basically means that it was specified in the ANSI SQL standard). I do not think `# :nodoc:` ing the type is sufficient, as it still makes the code of Rails itself very unclear as to what the role of that class is. While I would argue that this shouldn't even be a super class, and that MySql and PG's JSON types are only superficially duplicated (they might look the same but will change for different reasons in the future). However, I don't feel strongly enough about it as a point of contention (and the biggest cost of harming the blameability has already occured), so I simply moved the superclass into a namespace where its role is absolutely clear. After this change, `attribute :foo, :json` will once again work with MySQL and PG, but not with Sqlite3 or any third party adapters. Unresolved questions -------------------- The types that and adapter publishes (at least those are unique to that adapter, and not adding additional behavior like `MysqlString` should probably be part of the adapter's public API. Should we standardize the namespace for these, and document them? --- .../abstract_mysql_adapter.rb | 2 +- .../connection_adapters/postgresql/oid.rb | 1 + .../postgresql/oid/json.rb | 10 ++++++ .../postgresql/oid/jsonb.rb | 2 +- .../connection_adapters/postgresql_adapter.rb | 3 +- activerecord/lib/active_record/type.rb | 4 +-- .../type/internal/abstract_json.rb | 33 +++++++++++++++++++ activerecord/lib/active_record/type/json.rb | 31 ----------------- 8 files changed, 50 insertions(+), 36 deletions(-) create mode 100644 activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb create mode 100644 activerecord/lib/active_record/type/internal/abstract_json.rb delete mode 100644 activerecord/lib/active_record/type/json.rb 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 96ea866580..b3e55a0b90 100644 --- a/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/abstract_mysql_adapter.rb @@ -1050,7 +1050,7 @@ module ActiveRecord end end - class MysqlJson < Type::Json # :nodoc: + class MysqlJson < Type::Internal::AbstractJson # :nodoc: def changed_in_place?(raw_old_value, new_value) # Normalization is required because MySQL JSON data format includes # the space between the elements. diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb index 37226831dc..68752cdd80 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb @@ -8,6 +8,7 @@ require 'active_record/connection_adapters/postgresql/oid/decimal' require 'active_record/connection_adapters/postgresql/oid/enum' require 'active_record/connection_adapters/postgresql/oid/hstore' require 'active_record/connection_adapters/postgresql/oid/inet' +require 'active_record/connection_adapters/postgresql/oid/json' require 'active_record/connection_adapters/postgresql/oid/jsonb' require 'active_record/connection_adapters/postgresql/oid/money' require 'active_record/connection_adapters/postgresql/oid/point' diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb new file mode 100644 index 0000000000..dbc879ffd4 --- /dev/null +++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid/json.rb @@ -0,0 +1,10 @@ +module ActiveRecord + module ConnectionAdapters + module PostgreSQL + module OID # :nodoc: + class Json < Type::Internal::AbstractJson + end + end + end + end +end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid/jsonb.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid/jsonb.rb index 1f6d63582c..87391b5dc7 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql/oid/jsonb.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid/jsonb.rb @@ -2,7 +2,7 @@ module ActiveRecord module ConnectionAdapters module PostgreSQL module OID # :nodoc: - class Jsonb < Type::Json # :nodoc: + class Jsonb < Json # :nodoc: def type :jsonb end diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb index 861edbf3a2..27291bd2ea 100644 --- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb +++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb @@ -482,7 +482,7 @@ module ActiveRecord m.register_type 'bytea', OID::Bytea.new m.register_type 'point', OID::Point.new m.register_type 'hstore', OID::Hstore.new - m.register_type 'json', Type::Json.new + m.register_type 'json', OID::Json.new m.register_type 'jsonb', OID::Jsonb.new m.register_type 'cidr', OID::Cidr.new m.register_type 'inet', OID::Inet.new @@ -838,6 +838,7 @@ module ActiveRecord ActiveRecord::Type.register(:enum, OID::Enum, adapter: :postgresql) ActiveRecord::Type.register(:hstore, OID::Hstore, adapter: :postgresql) ActiveRecord::Type.register(:inet, OID::Inet, adapter: :postgresql) + ActiveRecord::Type.register(:json, OID::Json, adapter: :postgresql) ActiveRecord::Type.register(:jsonb, OID::Jsonb, adapter: :postgresql) ActiveRecord::Type.register(:money, OID::Money, adapter: :postgresql) ActiveRecord::Type.register(:point, OID::Point, adapter: :postgresql) diff --git a/activerecord/lib/active_record/type.rb b/activerecord/lib/active_record/type.rb index 8f56a37f3c..53f3b53bec 100644 --- a/activerecord/lib/active_record/type.rb +++ b/activerecord/lib/active_record/type.rb @@ -10,7 +10,6 @@ require 'active_record/type/decimal' require 'active_record/type/decimal_without_scale' require 'active_record/type/float' require 'active_record/type/integer' -require 'active_record/type/json' require 'active_record/type/serialized' require 'active_record/type/string' require 'active_record/type/text' @@ -21,6 +20,8 @@ require 'active_record/type/adapter_specific_registry' require 'active_record/type/type_map' require 'active_record/type/hash_lookup_type_map' +require 'active_record/type/internal/abstract_json' + module ActiveRecord module Type @registry = AdapterSpecificRegistry.new @@ -60,7 +61,6 @@ module ActiveRecord register(:decimal, Type::Decimal, override: false) register(:float, Type::Float, override: false) register(:integer, Type::Integer, override: false) - register(:json, Type::Json, override: false) register(:string, Type::String, override: false) register(:text, Type::Text, override: false) register(:time, Type::Time, override: false) diff --git a/activerecord/lib/active_record/type/internal/abstract_json.rb b/activerecord/lib/active_record/type/internal/abstract_json.rb new file mode 100644 index 0000000000..963a8245d0 --- /dev/null +++ b/activerecord/lib/active_record/type/internal/abstract_json.rb @@ -0,0 +1,33 @@ +module ActiveRecord + module Type + module Internal # :nodoc: + class AbstractJson < Type::Value # :nodoc: + include Type::Helpers::Mutable + + def type + :json + end + + def deserialize(value) + if value.is_a?(::String) + ::ActiveSupport::JSON.decode(value) rescue nil + else + value + end + end + + def serialize(value) + if value.is_a?(::Array) || value.is_a?(::Hash) + ::ActiveSupport::JSON.encode(value) + else + value + end + end + + def accessor + ActiveRecord::Store::StringKeyedHashAccessor + end + end + end + end +end diff --git a/activerecord/lib/active_record/type/json.rb b/activerecord/lib/active_record/type/json.rb deleted file mode 100644 index 1728bd3a8e..0000000000 --- a/activerecord/lib/active_record/type/json.rb +++ /dev/null @@ -1,31 +0,0 @@ -module ActiveRecord - module Type - class Json < Type::Value # :nodoc: - include Type::Helpers::Mutable - - def type - :json - end - - def deserialize(value) - if value.is_a?(::String) - ::ActiveSupport::JSON.decode(value) rescue nil - else - value - end - end - - def serialize(value) - if value.is_a?(::Array) || value.is_a?(::Hash) - ::ActiveSupport::JSON.encode(value) - else - value - end - end - - def accessor - ActiveRecord::Store::StringKeyedHashAccessor - end - end - end -end