From 7d50e00c672707426725247e514f61a76851640d Mon Sep 17 00:00:00 2001 From: Marc Sardon Date: Thu, 16 Jan 2025 10:23:19 +0000 Subject: [PATCH] Precompute Centroid for Location Polygons The centroid is used for calculating the distances on the vacancy searches. The centroid for a polygon area doesn't change unless the area changes too. As we dinamically calculate it for every location polygon area against every vacancy during the searches, if we pre-compute it we save the DB to execute again the centroid calculation on every search. --- app/services/ons_data_import/base.rb | 21 ++++++--- .../ons_data_import/create_composites.rb | 15 +++--- config/analytics.yml | 1 + config/brakeman.ignore | 46 +++++++++---------- ...80542_add_centroid_to_location_polygons.rb | 8 ++++ db/schema.rb | 12 +++-- 6 files changed, 63 insertions(+), 40 deletions(-) create mode 100644 db/migrate/20250114180542_add_centroid_to_location_polygons.rb diff --git a/app/services/ons_data_import/base.rb b/app/services/ons_data_import/base.rb index aeeee16021..f50e541a6e 100644 --- a/app/services/ons_data_import/base.rb +++ b/app/services/ons_data_import/base.rb @@ -18,18 +18,27 @@ def call geometry = feature["geometry"].to_json Rails.logger.info("Persisting new area data for '#{name}' (#{type})") - ActiveRecord::Base.connection.exec_update(" - UPDATE location_polygons - SET area=ST_GeomFromGeoJSON(#{ActiveRecord::Base.connection.quote(geometry)}), - location_type=#{ActiveRecord::Base.connection.quote(type)} - WHERE id='#{location_polygon.id}' - ") + set_area_data(location_polygon, geometry, type) end end end private + def set_area_data(location_polygon, geometry, type) + ActiveRecord::Base.connection.exec_update(" + WITH geom AS ( + SELECT ST_GeomFromGeoJSON(#{ActiveRecord::Base.connection.quote(geometry)}) AS geo + ) + UPDATE location_polygons + SET area=geom.geo, + location_type=#{ActiveRecord::Base.connection.quote(type)}, + centroid=ST_Centroid(geom.geo) + FROM geom + WHERE id='#{location_polygon.id}' + ") + end + def arcgis_features(offset) params = [ "where=1%3D1", diff --git a/app/services/ons_data_import/create_composites.rb b/app/services/ons_data_import/create_composites.rb index 4074b3c67a..a7d71e47ba 100644 --- a/app/services/ons_data_import/create_composites.rb +++ b/app/services/ons_data_import/create_composites.rb @@ -7,13 +7,16 @@ def call quoted_constituents = constituents.map { |c| ActiveRecord::Base.connection.quote(c.downcase) } ActiveRecord::Base.connection.exec_update(" + WITH composite_area AS ( + SELECT ST_Union(area::geometry)::geography AS geo + FROM location_polygons + WHERE name IN (#{quoted_constituents.join(', ')}) + ) UPDATE location_polygons - SET area=( - SELECT ST_Union(area::geometry)::geography - FROM location_polygons - WHERE name IN (#{quoted_constituents.join(', ')}) - ), - location_type='composite' + SET area=composite_area.geo, + location_type='composite', + centroid=ST_Centroid(composite_area.geo) + FROM composite_area WHERE id='#{composite.id}' ") end diff --git a/config/analytics.yml b/config/analytics.yml index c7160d4bf2..e5c5574d56 100644 --- a/config/analytics.yml +++ b/config/analytics.yml @@ -208,6 +208,7 @@ shared: - name - location_type - area + - centroid - created_at - updated_at notifications: diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 1e67ce19db..4c3c783615 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -115,29 +115,6 @@ ], "note": "" }, - { - "warning_type": "SQL Injection", - "warning_code": 0, - "fingerprint": "549d2779be79702f98156c6ec1a388b35ff19d87b0978910d4fad9d59a424cb9", - "check_name": "SQL", - "message": "Possible SQL injection", - "file": "app/services/ons_data_import/create_composites.rb", - "line": 14, - "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", - "code": "ActiveRecord::Base.connection.exec_update(\"\\n UPDATE location_polygons\\n SET area=(\\n SELECT ST_Union(area::geometry)::geography\\n FROM location_polygons\\n WHERE name IN (#{constituents.map do\n ActiveRecord::Base.connection.quote(c.downcase)\n end.join(\", \")})\\n ),\\n location_type='composite'\\n WHERE id='#{LocationPolygon.find_or_create_by(:name => name).id}'\\n \")", - "render_path": null, - "location": { - "type": "method", - "class": "OnsDataImport::CreateComposites", - "method": "call" - }, - "user_input": "constituents.map do\n ActiveRecord::Base.connection.quote(c.downcase)\n end.join(\", \")", - "confidence": "Medium", - "cwe_id": [ - 89 - ], - "note": "This is our own static data with no user input provided in the query" - }, { "warning_type": "Unscoped Find", "warning_code": 82, @@ -218,6 +195,29 @@ ], "note": "" }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "b2035069089eae3a5d8ec3a3a615aee253ec3cf2cd68c5535af152b8de779f46", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/services/ons_data_import/create_composites.rb", + "line": 13, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "ActiveRecord::Base.connection.exec_update(\"\\n WITH composite_area AS (\\n SELECT ST_Union(area::geometry)::geography AS geo\\n FROM location_polygons\\n WHERE name IN (#{constituents.map do\n ActiveRecord::Base.connection.quote(c.downcase)\n end.join(\", \")})\\n )\\n UPDATE location_polygons\\n SET area=composite_area.geo,\\n location_type='composite',\\n centroid=ST_Centroid(composite_area.geo)\\n FROM composite_area\\n WHERE id='#{LocationPolygon.find_or_create_by(:name => name).id}'\\n \")", + "render_path": null, + "location": { + "type": "method", + "class": "OnsDataImport::CreateComposites", + "method": "call" + }, + "user_input": "constituents.map do\n ActiveRecord::Base.connection.quote(c.downcase)\n end.join(\", \")", + "confidence": "Medium", + "cwe_id": [ + 89 + ], + "note": "" + }, { "warning_type": "SQL Injection", "warning_code": 0, diff --git a/db/migrate/20250114180542_add_centroid_to_location_polygons.rb b/db/migrate/20250114180542_add_centroid_to_location_polygons.rb new file mode 100644 index 0000000000..27a5f95a0b --- /dev/null +++ b/db/migrate/20250114180542_add_centroid_to_location_polygons.rb @@ -0,0 +1,8 @@ +class AddCentroidToLocationPolygons < ActiveRecord::Migration[7.2] + disable_ddl_transaction! + + def change + add_column :location_polygons, :centroid, :st_point, geographic: true + add_index :location_polygons, :centroid, using: :gist, algorithm: :concurrently + end +end diff --git a/db/schema.rb b/db/schema.rb index 36d7384f0d..26879451b4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.2].define(version: 2025_01_13_144018) do +ActiveRecord::Schema[7.2].define(version: 2025_01_14_180542) do # These are extensions that must be enabled in order to support this database enable_extension "btree_gist" enable_extension "citext" @@ -78,11 +78,11 @@ t.uuid "job_application_id" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.integer "employment_type", default: 0 - t.text "reason_for_break", default: "" t.text "organisation_ciphertext" t.text "job_title_ciphertext" t.text "main_duties_ciphertext" + t.integer "employment_type", default: 0 + t.text "reason_for_break", default: "" t.uuid "jobseeker_profile_id" t.text "reason_for_leaving" t.index ["job_application_id"], name: "index_employments_on_job_application_id" @@ -313,9 +313,9 @@ t.date "account_closed_on" t.text "current_sign_in_ip_ciphertext" t.text "last_sign_in_ip_ciphertext" + t.string "govuk_one_login_id" t.string "account_merge_confirmation_code" t.datetime "account_merge_confirmation_code_generated_at" - t.string "govuk_one_login_id" t.index ["email"], name: "index_jobseekers_on_email", unique: true t.index ["govuk_one_login_id"], name: "index_jobseekers_on_govuk_one_login_id", unique: true end @@ -334,7 +334,9 @@ t.datetime "created_at", precision: nil, null: false t.datetime "updated_at", precision: nil, null: false t.geography "area", limit: {:srid=>4326, :type=>"geometry", :geographic=>true} + t.geography "centroid", limit: {:srid=>4326, :type=>"st_point", :geographic=>true} t.index ["area"], name: "index_location_polygons_on_area", using: :gist + t.index ["centroid"], name: "index_location_polygons_on_centroid", using: :gist t.index ["name"], name: "index_location_polygons_on_name" end @@ -685,8 +687,8 @@ t.boolean "include_additional_documents" t.boolean "visa_sponsorship_available" t.boolean "is_parental_leave_cover" - t.boolean "is_job_share" t.string "hourly_rate" + t.boolean "is_job_share" t.string "flexi_working" t.integer "extension_reason" t.string "other_extension_reason_details"