Return a `Point` object from the PG Point type

This introduces a deprecation cycle to change the behavior of the
default point type in the PostgreSQL adapter. The old behavior will
continue to be available for the immediate future as `:legacy_point`.

The current behavior of returning an `Array` causes several problems,
the most significant of which is that we cannot differentiate between an
array of points, and a point itself in the case of a column with the
`point[]` type.

The attributes API gives us a reasonable way to have a proper
deprecation cycle for this change, so let's take advantage of it. If we
like this change, we can also add proper support for the other geometric
types (line, lseg, box, path, polygon, and circle), all of which are
just aliases for string today.

Fixes #20441

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