From 1f627d34f1b028f079807a32ea65f01b8e5e8e06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Mon, 2 Dec 2024 13:21:49 +0100 Subject: [PATCH] Make sure polygons contain valid rings According to the GeoJSON specification [1]: > * A linear ring is a closed LineString with four or more positions. > * The first and last positions are equivalent, and they MUST contain > identical values; their representation SHOULD also be identical. > (...) > * For type "Polygon", the "coordinates" member MUST be an array of > linear ring coordinate arrays. Note that, for simplicity, right now we aren't checking whether the coordinates are defined counterclockwise for exterior rings and clockwise for interior rings, which is what the specification expects. [1] https://datatracker.ietf.org/doc/html/rfc7946#section-3.1.6 --- .../concerns/geojson_format_validator.rb | 11 +- spec/factories/administration.rb | 2 +- spec/models/geojson_format_validator_spec.rb | 215 ++++++++++++++++++ spec/system/admin/geozones_spec.rb | 4 +- spec/system/budgets/investments_spec.rb | 4 +- 5 files changed, 228 insertions(+), 8 deletions(-) diff --git a/app/models/concerns/geojson_format_validator.rb b/app/models/concerns/geojson_format_validator.rb index 42950c1e2..2182aa3b3 100644 --- a/app/models/concerns/geojson_format_validator.rb +++ b/app/models/concerns/geojson_format_validator.rb @@ -115,8 +115,13 @@ class GeojsonFormatValidator < ActiveModel::EachValidator end def valid_polygon_coordinates?(polygon_coordinates) - polygon_coordinates.all? do |ring_coordinates| - valid_coordinates_array?(ring_coordinates) - end + polygon_coordinates.is_a?(Array) && + polygon_coordinates.all? { |ring_coordinates| valid_ring_coordinates?(ring_coordinates) } + end + + def valid_ring_coordinates?(ring_coordinates) + valid_coordinates_array?(ring_coordinates) && + ring_coordinates.size >= 4 && + ring_coordinates.first == ring_coordinates.last end end diff --git a/spec/factories/administration.rb b/spec/factories/administration.rb index b02cfcb20..21d1e9b60 100644 --- a/spec/factories/administration.rb +++ b/spec/factories/administration.rb @@ -25,7 +25,7 @@ FactoryBot.define do "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[[0.117, 51.513], [0.118, 51.512], [0.119, 51.514]]] + "coordinates": [[[0.117, 51.513], [0.118, 51.512], [0.119, 51.514], [0.117, 51.513]]] } } JSON diff --git a/spec/models/geojson_format_validator_spec.rb b/spec/models/geojson_format_validator_spec.rb index c4b4780f9..a0a5f4eef 100644 --- a/spec/models/geojson_format_validator_spec.rb +++ b/spec/models/geojson_format_validator_spec.rb @@ -217,6 +217,221 @@ describe GeojsonFormatValidator do end end + context "Polygon geometry" do + it "is not valid with a ring having less than four elements" do + record.geojson = <<~JSON + { + "type": "Polygon", + "coordinates": [[ + [1.23, 4.56], + [7.89, 10.11], + [1.23, 4.56] + ]] + } + JSON + + expect(record).not_to be_valid + end + + it "is not valid with a ring which with different starting and end points" do + record.geojson = <<~JSON + { + "type": "Polygon", + "coordinates": [[ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [16.17, 18.19] + ]] + } + JSON + + expect(record).not_to be_valid + end + + it "is valid with one valid ring" do + record.geojson = <<~JSON + { + "type": "Polygon", + "coordinates": [[ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [1.23, 4.56] + ]] + } + JSON + + expect(record).to be_valid + end + + it "is valid with multiple valid rings" do + record.geojson = <<~JSON + { + "type": "Polygon", + "coordinates": [ + [ + [100.0, 0.0], + [101.0, 0.0], + [101.0, 1.0], + [100.0, 1.0], + [100.0, 0.0] + ], + [ + [100.8, 0.8], + [100.8, 0.2], + [100.2, 0.2], + [100.2, 0.8], + [100.8, 0.8] + ] + ] + } + JSON + + expect(record).to be_valid + end + + it "is not valid with multiple rings if some rings are invalid" do + record.geojson = <<~JSON + { + "type": "Polygon", + "coordinates": [ + [ + [100.0, 0.0], + [101.0, 0.0], + [101.0, 1.0], + [100.0, 1.0], + [100.0, 0.0] + ], + [ + [100.8, 0.8], + [100.8, 0.2], + [100.2, 0.2] + ] + ] + } + JSON + + expect(record).not_to be_valid + end + end + + context "MultiPolygon geometry" do + it "is not valid with a one-dimensional array of coordinates" do + record.geojson = '{ "type": "MultiPolygon", "coordinates": [1.23, 4.56] }' + + expect(record).not_to be_valid + end + + it "is not valid with a two-dimensional array of coordinates" do + record.geojson = '{ "type": "MultiPolygon", "coordinates": [[1.23, 4.56], [7.89, 4.56]] }' + + expect(record).not_to be_valid + end + + it "is not valid with a three-dimensional polygon coordinates array" do + record.geojson = <<~JSON + { + "type": "MultiPolygon", + "coordinates": [[ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [1.23, 4.56] + ]] + } + JSON + + expect(record).not_to be_valid + end + + it "is valid with a valid polygon" do + record.geojson = <<~JSON + { + "type": "MultiPolygon", + "coordinates": [[[ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [1.23, 4.56] + ]]] + } + JSON + + expect(record).to be_valid + end + + it "is valid with multiple valid polygons" do + record.geojson = <<~JSON + { + "type": "MultiPolygon", + "coordinates": [ + [ + [ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [1.23, 4.56] + ] + ], + [ + [ + [100.0, 0.0], + [101.0, 0.0], + [101.0, 1.0], + [100.0, 1.0], + [100.0, 0.0] + ], + [ + [100.8, 0.8], + [100.8, 0.2], + [100.2, 0.2], + [100.2, 0.8], + [100.8, 0.8] + ] + ] + ] + } + JSON + + expect(record).to be_valid + end + + it "is not valid with multiple polygons if some polygons are invalid" do + record.geojson = <<~JSON + { + "type": "MultiPolygon", + "coordinates": [ + [ + [ + [1.23, 4.56], + [7.89, 10.11], + [12.13, 14.15], + [1.23, 4.56] + ] + ], + [ + [ + [100.0, 0.0], + [101.0, 0.0], + [101.0, 1.0], + [100.0, 1.0], + [100.0, 0.0] + ], + [ + [100.8, 0.8], + [100.8, 0.2], + [100.2, 0.2] + ] + ] + ] + } + JSON + + expect(record).not_to be_valid + end + end + context "GeometryCollection" do it "is not valid if it doesn't contain geometries" do record.geojson = '{ "type": "GeometryCollection" }' diff --git a/spec/system/admin/geozones_spec.rb b/spec/system/admin/geozones_spec.rb index 37b448d2a..27b9f48f0 100644 --- a/spec/system/admin/geozones_spec.rb +++ b/spec/system/admin/geozones_spec.rb @@ -115,7 +115,7 @@ describe "Admin geozones", :admin do "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6]]] + "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6], [-0.1, 51.5]]] } } JSON @@ -158,7 +158,7 @@ describe "Admin geozones", :admin do "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6]]] + "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6], [-0.1, 51.5]]] } } JSON diff --git a/spec/system/budgets/investments_spec.rb b/spec/system/budgets/investments_spec.rb index 45930fe45..87a9ed0b1 100644 --- a/spec/system/budgets/investments_spec.rb +++ b/spec/system/budgets/investments_spec.rb @@ -1661,7 +1661,7 @@ describe "Budget Investments" do "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6]]] + "coordinates": [[[-0.1, 51.5], [-0.2, 51.4], [-0.3, 51.6], [-0.1, 51.5]]] } } JSON @@ -1671,7 +1671,7 @@ describe "Budget Investments" do "type": "Feature", "geometry": { "type": "Polygon", - "coordinates": [[[-0.1, 51.5], [-0.2, 51.5], [-0.2, 51.6], [-0.1, 51.6]]] + "coordinates": [[[-0.1, 51.5], [-0.2, 51.5], [-0.2, 51.6], [-0.1, 51.6], [-0.1, 51.5]]] } } JSON