Skip to content

Fix: don't match enum columns as spatial #433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix: don't match enum columns as spatial #433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion lib/active_record/connection_adapters/postgis/spatial_column.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ def initialize(name, default, sql_type_metadata = nil, null = true,
serial: nil, generated: nil, spatial: nil, identity: nil)
@sql_type_metadata = sql_type_metadata
@geographic = !!(sql_type_metadata.sql_type =~ /geography\(/i)
if spatial
if sql_type_metadata.type == :enum
Copy link
Member

@BuonOmo BuonOmo Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should not be there if we are handling enums. Otherwise this also means that sql_type_metadata is set for every column, spatial or not.

I believe we should verify how the step before that one is working to make sure this is the correct place (e.g. who is initialising this)

Copy link
Author

@modosc modosc Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaict this is by design.

this is initialized from #new_column_from_field which overrides the rails implementation and returns a SpatialColumn (instead of a PostgreSQL::Column) for all columns.

i'm not sure what would happen if some fields were one type and some were another but it seems out of scope for this pr.

# noop - enum types are not spatial but their names may match against the regexes below
elsif spatial
# This case comes from an entry in the geometry_columns table
set_geometric_type_from_name(spatial[:type])
@srid = spatial[:srid].to_i
Expand Down
12 changes: 12 additions & 0 deletions test/cases/basic_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,18 @@ def test_multi_polygon_column
assert_equal wkt, rec.m_poly.to_s
end

def test_spatial_column_matching_enum
SpatialModel.lease_connection.create_enum(:point_type, ["point", "line_string", "polygon"])
SpatialModel.lease_connection.create_table(:spatial_models, force: true) do |t|
Comment on lines +214 to +215
Copy link
Member

@BuonOmo BuonOmo Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it properly cleanup afterwards ? If so, how ?

Copy link
Author

@modosc modosc Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't know - i copied this from the other examples, can you point me to where any explicit cleanup happens?

t.column "latlon", :st_point, srid: 3785
t.column "latlon_geo", :st_point, srid: 4326, geographic: true
t.column "default_latlon", :st_point, srid: 0, default: "POINT(0.0 0.0)"
Comment on lines +216 to +218
Copy link
Member

@BuonOmo BuonOmo Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we have these ?

Copy link
Author

@modosc modosc Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these were copied from create_model below, happy to remove them if you think it's necessary.

t.enum "point_type", enum_type: :point_type
end
SpatialModel.reset_column_information
point_type = SpatialModel.columns.find { |c| c.name == "point_type" }
assert_nil point_type.geometric_type
end
private

def create_model
Expand Down