Merge pull request #20448 from sgrif/sg-postgresql-point-type

Return a `Point` object from the PG Point type

6 files changed, 189 insertions, 11 deletions

diff --git a/activerecord/CHANGELOG.md b/activerecord/CHANGELOG.md
index f6a0777faf..02b2096ef6 100644
--- a/activerecord/CHANGELOG.md
+++ b/activerecord/CHANGELOG.md
@@ -1,3 +1,8 @@
+*   Deprecate the PG `:point` type in favor of a new one which will return
+    `Point` objects instead of an `Array`
+
+    *Sean Griffin*
+
 *   Ensure symbols passed to `ActiveRecord::Relation#select` are always treated
     as columns.
 
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb
index 92349e2f9b..68752cdd80 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid.rb
@@ -12,6 +12,7 @@ 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'
+require 'active_record/connection_adapters/postgresql/oid/rails_5_1_point'
 require 'active_record/connection_adapters/postgresql/oid/range'
 require 'active_record/connection_adapters/postgresql/oid/specialized_string'
 require 'active_record/connection_adapters/postgresql/oid/uuid'
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql/oid/rails_5_1_point.rb b/activerecord/lib/active_record/connection_adapters/postgresql/oid/rails_5_1_point.rb
new file mode 100644
index 0000000000..7427a25ad5
--- /dev/null
+++ b/activerecord/lib/active_record/connection_adapters/postgresql/oid/rails_5_1_point.rb
@@ -0,0 +1,50 @@
+module ActiveRecord
+  Point = Struct.new(:x, :y)
+
+  module ConnectionAdapters
+    module PostgreSQL
+      module OID # :nodoc:
+        class Rails51Point < Type::Value # :nodoc:
+          include Type::Helpers::Mutable
+
+          def type
+            :point
+          end
+
+          def cast(value)
+            case value
+            when ::String
+              if value[0] == '(' && value[-1] == ')'
+                value = value[1...-1]
+              end
+              x, y = value.split(",")
+              build_point(x, y)
+            when ::Array
+              build_point(*value)
+            else
+              value
+            end
+          end
+
+          def serialize(value)
+            if value.is_a?(ActiveRecord::Point)
+              "(#{number_for_point(value.x)},#{number_for_point(value.y)})"
+            else
+              super
+            end
+          end
+
+          private
+
+          def number_for_point(number)
+            number.to_s.gsub(/\.0$/, '')
+          end
+
+          def build_point(x, y)
+            ActiveRecord::Point.new(Float(x), Float(y))
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index 93fa3984e6..2c43c46a3d 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -838,6 +838,8 @@ module ActiveRecord
         ActiveRecord::Type.register(:jsonb, OID::Jsonb, adapter: :postgresql)
         ActiveRecord::Type.register(:money, OID::Money, adapter: :postgresql)
         ActiveRecord::Type.register(:point, OID::Point, adapter: :postgresql)
+        ActiveRecord::Type.register(:legacy_point, OID::Point, adapter: :postgresql)
+        ActiveRecord::Type.register(:rails_5_1_point, OID::Rails51Point, adapter: :postgresql)
         ActiveRecord::Type.register(:uuid, OID::Uuid, adapter: :postgresql)
         ActiveRecord::Type.register(:vector, OID::Vector, adapter: :postgresql)
         ActiveRecord::Type.register(:xml, OID::Xml, adapter: :postgresql)
diff --git a/activerecord/lib/active_record/model_schema.rb b/activerecord/lib/active_record/model_schema.rb
index 3674f672cb..5a6f42ba09 100644
--- a/activerecord/lib/active_record/model_schema.rb
+++ b/activerecord/lib/active_record/model_schema.rb
@@ -310,6 +310,7 @@ module ActiveRecord
       def load_schema!
         @columns_hash = connection.schema_cache.columns_hash(table_name)
         @columns_hash.each do |name, column|
+          warn_if_deprecated_type(column)
           define_attribute(
             name,
             connection.lookup_cast_type_from_column(column),
@@ -356,6 +357,28 @@ module ActiveRecord
           base.table_name
         end
       end
+
+      def warn_if_deprecated_type(column)
+        return if attributes_to_define_after_schema_loads.key?(column.name)
+        if column.respond_to?(:oid) && column.sql_type.start_with?("point")
+          if column.array?
+            array_arguments = ", array: true"
+          else
+            array_arguments = ""
+          end
+          ActiveSupport::Deprecation.warn(<<-WARNING.strip_heredoc)
+            The behavior of the `:point` type will be changing in Rails 5.1 to
+            return a `Point` object, rather than an `Array`. If you'd like to
+            keep the old behavior, you can add this line to #{self.name}:
+
+              attribute :#{column.name}, :legacy_point#{array_arguments}
+
+            If you'd like the new behavior today, you can add this line:
+
+              attribute :#{column.name}, :rails_5_1_point#{array_arguments}
+          WARNING
+        end
+      end
     end
   end
 end
diff --git a/activerecord/test/cases/adapters/postgresql/geometric_test.rb b/activerecord/test/cases/adapters/postgresql/geometric_test.rb
index 41e9572907..7057848eb0 100644
--- a/activerecord/test/cases/adapters/postgresql/geometric_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/geometric_test.rb
@@ -6,7 +6,15 @@ class PostgresqlPointTest < ActiveRecord::TestCase
   include ConnectionHelper
   include SchemaDumpingHelper
 
-  class PostgresqlPoint < ActiveRecord::Base; end
+  class PostgresqlPoint < ActiveRecord::Base
+    attribute :x, :rails_5_1_point
+    attribute :y, :rails_5_1_point
+    attribute :z, :rails_5_1_point
+    attribute :array_of_points, :rails_5_1_point, array: true
+    attribute :legacy_x, :legacy_point
+    attribute :legacy_y, :legacy_point
+    attribute :legacy_z, :legacy_point
+  end
 
   def setup
     @connection = ActiveRecord::Base.connection
@@ -14,11 +22,27 @@ class PostgresqlPointTest < ActiveRecord::TestCase
       t.point :x
       t.point :y, default: [12.2, 13.3]
       t.point :z, default: "(14.4,15.5)"
+      t.point :array_of_points, array: true
+      t.point :legacy_x
+      t.point :legacy_y, default: [12.2, 13.3]
+      t.point :legacy_z, default: "(14.4,15.5)"
+    end
+    @connection.create_table('deprecated_points') do |t|
+      t.point :x
     end
   end
 
   teardown do
     @connection.drop_table 'postgresql_points', if_exists: true
+    @connection.drop_table 'deprecated_points', if_exists: true
+  end
+
+  class DeprecatedPoint < ActiveRecord::Base; end
+
+  def test_deprecated_legacy_type
+    assert_deprecated do
+      DeprecatedPoint.new
+    end
   end
 
   def test_column
@@ -32,11 +56,11 @@ class PostgresqlPointTest < ActiveRecord::TestCase
   end
 
   def test_default
-    assert_equal [12.2, 13.3], PostgresqlPoint.column_defaults['y']
-    assert_equal [12.2, 13.3], PostgresqlPoint.new.y
+    assert_equal ActiveRecord::Point.new(12.2, 13.3), PostgresqlPoint.column_defaults['y']
+    assert_equal ActiveRecord::Point.new(12.2, 13.3), PostgresqlPoint.new.y
 
-    assert_equal [14.4, 15.5], PostgresqlPoint.column_defaults['z']
-    assert_equal [14.4, 15.5], PostgresqlPoint.new.z
+    assert_equal ActiveRecord::Point.new(14.4, 15.5), PostgresqlPoint.column_defaults['z']
+    assert_equal ActiveRecord::Point.new(14.4, 15.5), PostgresqlPoint.new.z
   end
 
   def test_schema_dumping
@@ -49,22 +73,95 @@ class PostgresqlPointTest < ActiveRecord::TestCase
   def test_roundtrip
     PostgresqlPoint.create! x: [10, 25.2]
     record = PostgresqlPoint.first
-    assert_equal [10, 25.2], record.x
+    assert_equal ActiveRecord::Point.new(10, 25.2), record.x
 
-    record.x = [1.1, 2.2]
+    record.x = ActiveRecord::Point.new(1.1, 2.2)
     record.save!
     assert record.reload
-    assert_equal [1.1, 2.2], record.x
+    assert_equal ActiveRecord::Point.new(1.1, 2.2), record.x
   end
 
   def test_mutation
-    p = PostgresqlPoint.create! x: [10, 20]
+    p = PostgresqlPoint.create! x: ActiveRecord::Point.new(10, 20)
+
+    p.x.y = 25
+    p.save!
+    p.reload
+
+    assert_equal ActiveRecord::Point.new(10.0, 25.0), p.x
+    assert_not p.changed?
+  end
+
+  def test_array_assignment
+    p = PostgresqlPoint.new(x: [1, 2])
+
+    assert_equal ActiveRecord::Point.new(1, 2), p.x
+  end
+
+  def test_string_assignment
+    p = PostgresqlPoint.new(x: "(1, 2)")
+
+    assert_equal ActiveRecord::Point.new(1, 2), p.x
+  end
+
+  def test_array_of_points_round_trip
+    expected_value = [
+      ActiveRecord::Point.new(1, 2),
+      ActiveRecord::Point.new(2, 3),
+      ActiveRecord::Point.new(3, 4),
+    ]
+    p = PostgresqlPoint.new(array_of_points: expected_value)
+
+    assert_equal expected_value, p.array_of_points
+    p.save!
+    p.reload
+    assert_equal expected_value, p.array_of_points
+  end
+
+  def test_legacy_column
+    column = PostgresqlPoint.columns_hash["legacy_x"]
+    assert_equal :point, column.type
+    assert_equal "point", column.sql_type
+    assert_not column.array?
+
+    type = PostgresqlPoint.type_for_attribute("legacy_x")
+    assert_not type.binary?
+  end
+
+  def test_legacy_default
+    assert_equal [12.2, 13.3], PostgresqlPoint.column_defaults['legacy_y']
+    assert_equal [12.2, 13.3], PostgresqlPoint.new.legacy_y
+
+    assert_equal [14.4, 15.5], PostgresqlPoint.column_defaults['legacy_z']
+    assert_equal [14.4, 15.5], PostgresqlPoint.new.legacy_z
+  end
+
+  def test_legacy_schema_dumping
+    output = dump_table_schema("postgresql_points")
+    assert_match %r{t\.point\s+"legacy_x"$}, output
+    assert_match %r{t\.point\s+"legacy_y",\s+default: \[12\.2, 13\.3\]$}, output
+    assert_match %r{t\.point\s+"legacy_z",\s+default: \[14\.4, 15\.5\]$}, output
+  end
+
+  def test_legacy_roundtrip
+    PostgresqlPoint.create! legacy_x: [10, 25.2]
+    record = PostgresqlPoint.first
+    assert_equal [10, 25.2], record.legacy_x
+
+    record.legacy_x = [1.1, 2.2]
+    record.save!
+    assert record.reload
+    assert_equal [1.1, 2.2], record.legacy_x
+  end
+
+  def test_legacy_mutation
+    p = PostgresqlPoint.create! legacy_x: [10, 20]
 
-    p.x[1] = 25
+    p.legacy_x[1] = 25
     p.save!
     p.reload
 
-    assert_equal [10.0, 25.0], p.x
+    assert_equal [10.0, 25.0], p.legacy_x
     assert_not p.changed?
   end
 end