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

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.
 

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'

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

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)

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

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 ActiveRecord::Point.new(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.new.y
 
-    assert_equal [14.4, 15.5], PostgresqlPoint.column_defaults['z']
+    assert_equal ActiveRecord::Point.new(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.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[1] = 25
+    p.x.y = 25
     p.save!
     p.reload
 
-    assert_equal [10.0, 25.0], p.x
+    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.legacy_x[1] = 25
+    p.save!
+    p.reload
+
+    assert_equal [10.0, 25.0], p.legacy_x
     assert_not p.changed?
   end
 end