Skip to content
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

Set option for faster geography evaluation #7422

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
+81 −182
Draft

Set option for faster geography evaluation #7422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e67f28e
Set option for faster geography evaluation
scruti Jan 16, 2025
5f268fc
Update Brakeman ignored config
scruti Jan 16, 2025
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
2 changes: 1 addition & 1 deletion app/models/organisation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class Organisation < ApplicationRecord
scope :within_polygon, ->(location_polygon) { where("ST_Intersects(?, geopoint)", location_polygon.area.to_s) if location_polygon }
scope :within_area, lambda { |coordinates, radius|
point = "POINT(#{coordinates&.second} #{coordinates&.first})"
where("ST_DWithin(geopoint, ?, ?)", point, radius) if coordinates && radius
where("ST_DWithin(geopoint, ?, ?, false)", point, radius) if coordinates && radius
}
scope :in_vacancy_ids, ->(ids) { joins(:organisation_vacancies).where(organisation_vacancies: { vacancy_id: ids }).distinct }

Expand Down
14 changes: 8 additions & 6 deletions app/queries/location_query.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def nationwide_location?(query)
def handle_polygon_location(field_name, polygon, radius, sort_by_distance)
@scope = scope.joins("
INNER JOIN location_polygons
ON ST_DWithin(#{field_name}, location_polygons.area, #{radius})
ON ST_DWithin(#{field_name}, location_polygons.area, #{radius}, false)
").where("location_polygons.id = ?", polygon.id)

sort_by_polygon_distance(field_name) if sort_by_distance
Expand All @@ -48,20 +48,22 @@ def handle_coordinates(field_name, query, radius, sort_by_distance)
return scope.none if coordinates == [0, 0]

point = "POINT(#{coordinates.second} #{coordinates.first})"
@scope = scope.where("ST_DWithin(#{field_name}, ?, ?)", point, radius)
@scope = scope.where("ST_DWithin(#{field_name}, ?, ?, false)", point, radius)

sort_by_coordinates_distance(field_name, point) if sort_by_distance

scope
end

def sort_by_polygon_distance(field_name)
@scope = scope.select("vacancies.*, ST_Distance(#{field_name}, ST_Centroid(location_polygons.area)) AS distance")
.order(Arel.sql("ST_Distance(#{field_name}, ST_Centroid(location_polygons.area))"))
@scope = scope.select("vacancies.*, ST_Distance(#{field_name}, ST_Centroid(location_polygons.area), false) AS distance")
# why not using 'distance' alias? is not defined when calling this query with a 'pluck'
.order(Arel.sql("ST_Distance(#{field_name}, ST_Centroid(location_polygons.area), false)"))
end

def sort_by_coordinates_distance(field_name, point)
@scope = scope.select("vacancies.*, ST_Distance(#{field_name}, '#{point}') AS distance")
.order(Arel.sql("ST_Distance(#{field_name}, '#{point}')"))
@scope = scope.select("vacancies.*, ST_Distance(#{field_name}, '#{point}', false) AS distance")
# why not using 'distance' alias? is not defined when calling this query with a 'pluck'
.order(Arel.sql("ST_Distance(#{field_name}, '#{point}', false)"))
end
end
243 changes: 70 additions & 173 deletions config/brakeman.ignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,48 @@
{
"ignored_warnings": [
{
"warning_type": "Cross-Site Scripting",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I had to add the modified SQL queries to the Brakeman ignore file, I also did an automated cleanup of legacy ignored warnings that are no longer needed. Hence all the code removal.

This is automated by brakeman -I tool.

"warning_code": 2,
"fingerprint": "24062c57b5ebf41e370cdc934e79a6465da62a505547bc540b364ce04ab2fd4c",
"check_name": "CrossSiteScripting",
"message": "Unescaped parameter value",
"file": "app/views/posts/show.html.slim",
"line": 18,
"link": "https://brakemanscanner.org/docs/warning_types/cross_site_scripting",
"code": "MarkdownDocument.new(params[:section], params[:post_name]).content",
"render_path": [
{
"type": "controller",
"class": "PostsController",
"method": "show",
"line": 9,
"file": "app/controllers/posts_controller.rb",
"rendered": {
"name": "posts/show",
"file": "app/views/posts/show.html.slim"
}
}
],
"warning_type": "SQL Injection",
"warning_code": 0,
"fingerprint": "11470d12efb4e5d5b0f114eab8b39b18f1c89aba310d7ccf0a0a401058ad8403",
"check_name": "SQL",
"message": "Possible SQL injection",
"file": "app/queries/location_query.rb",
"line": 51,
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
"code": "scope.where(\"ST_DWithin(#{field_name}, ?, ?, false)\", \"POINT(#{Geocoding.new(query).coordinates.second} #{Geocoding.new(query).coordinates.first})\", radius)",
"render_path": null,
"location": {
"type": "template",
"template": "posts/show"
"type": "method",
"class": "LocationQuery",
"method": "handle_coordinates"
},
"user_input": "params[:section]",
"user_input": "field_name",
"confidence": "Weak",
"cwe_id": [
79
89
],
"note": ""
},
{
"warning_type": "SQL Injection",
"warning_code": 0,
"fingerprint": "247ffca0a9eb003f3adee0fc7c76367a73fedf3282be496fa76f54a99d496c50",
"check_name": "SQL",
"message": "Possible SQL injection",
"file": "app/queries/location_query.rb",
"line": 61,
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
"code": "Arel.sql(\"ST_Distance(#{field_name}, ST_Centroid(location_polygons.area), false)\")",
"render_path": null,
"location": {
"type": "method",
"class": "LocationQuery",
"method": "sort_by_polygon_distance"
},
"user_input": "field_name",
"confidence": "Medium",
"cwe_id": [
89
],
"note": ""
},
Expand Down Expand Up @@ -64,7 +76,7 @@
"check_name": "UnscopedFind",
"message": "Unscoped call to `EmergencyLoginKey#find_by`",
"file": "app/controllers/jobseekers/login_keys_controller.rb",
"line": 36,
"line": 33,
"link": "https://brakemanscanner.org/docs/warning_types/unscoped_find/",
"code": "EmergencyLoginKey.find_by(:id => params[:id])",
"render_path": null,
Expand All @@ -80,6 +92,29 @@
],
"note": "Token is a UUID, only valid for 10 mins, and deleted after use."
},
{
"warning_type": "SQL Injection",
"warning_code": 0,
"fingerprint": "46ba98a6804b79053060718da628101f86c86959fda772ff0ead453d1028041a",
"check_name": "SQL",
"message": "Possible SQL injection",
"file": "app/queries/location_query.rb",
"line": 67,
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
"code": "Arel.sql(\"ST_Distance(#{field_name}, '#{point}', false)\")",
"render_path": null,
"location": {
"type": "method",
"class": "LocationQuery",
"method": "sort_by_coordinates_distance"
},
"user_input": "field_name",
"confidence": "Medium",
"cwe_id": [
89
],
"note": ""
},
{
"warning_type": "SQL Injection",
"warning_code": 0,
Expand Down Expand Up @@ -126,45 +161,22 @@
],
"note": "Support users are meant to be able to see jobseeker profiles"
},
{
"warning_type": "SQL Injection",
"warning_code": 0,
"fingerprint": "807960ad995ced9524fef98fa8770d355bd5f44365c88ad4eba82831afcb9258",
"check_name": "SQL",
"message": "Possible SQL injection",
"file": "app/queries/location_query.rb",
"line": 65,
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
"code": "Arel.sql(\"ST_Distance(#{field_name}, '#{point}')\")",
"render_path": null,
"location": {
"type": "method",
"class": "LocationQuery",
"method": "sort_by_coordinates_distance"
},
"user_input": "field_name",
"confidence": "Medium",
"cwe_id": [
89
],
"note": "field_name does not come from user_input, field_name is hardcoded in the child classes of LocationQuery. Point also does not come from user input it is derived from Geocoding#coordinates which returns an array with coordinates returned by our cache or third party geocoding APIs. It defaults to [0,0] if relevant coordinates are not found."
},
{
"warning_type": "Cross-Site Scripting",
"warning_code": 2,
"fingerprint": "84c4e6fcded5a5ae1fd4d47b22ee04c3ecab2f120a83166aaf15a51ecec836e7",
"check_name": "CrossSiteScripting",
"message": "Unescaped parameter value",
"file": "app/views/posts/show.html.slim",
"line": 16,
"line": 17,
"link": "https://brakemanscanner.org/docs/warning_types/cross_site_scripting",
"code": "MarkdownDocument.new(:section => params[:section], :subcategory => params[:subcategory], :post_name => params[:post_name]).content",
"render_path": [
{
"type": "controller",
"class": "PostsController",
"method": "show",
"line": 14,
"line": 18,
"file": "app/controllers/posts_controller.rb",
"rendered": {
"name": "posts/show",
Expand Down Expand Up @@ -206,37 +218,14 @@
],
"note": ""
},
{
"warning_type": "SQL Injection",
"warning_code": 0,
"fingerprint": "99f83ac3d521eb41d6d7ed163e2a31092b0b21150b72e860a95f44376ea4ce7a",
"check_name": "SQL",
"message": "Possible SQL injection",
"file": "app/queries/location_query.rb",
"line": 35,
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
"code": "scope.joins(\"\\n INNER JOIN location_polygons\\n ON ST_DWithin(#{field_name}, location_polygons.area, #{radius})\\n \")",
"render_path": null,
"location": {
"type": "method",
"class": "LocationQuery",
"method": "handle_polygon_location"
},
"user_input": "field_name",
"confidence": "Weak",
"cwe_id": [
89
],
"note": "Neither field_name or radius come directly from user input. field_name is hardcoded in the child classes of LocationQuery and radius comes from Search::RadiusBuilder#get_radius which sanitises the input."
},
{
"warning_type": "SQL Injection",
"warning_code": 0,
"fingerprint": "bdb6e5e218cff279e924b58b25fb8cbadd3161da685319b6725256b791286670",
"check_name": "SQL",
"message": "Possible SQL injection",
"file": "app/models/location_polygon.rb",
"line": 11,
"line": 8,
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
"code": "select(\"*, ST_Buffer(area, #{convert_miles_to_metres((radius_in_miles or 0))}) AS area\")",
"render_path": null,
Expand All @@ -255,118 +244,26 @@
{
"warning_type": "SQL Injection",
"warning_code": 0,
"fingerprint": "e1c81bdbf55fe817f0de198b3e49bfc46cfb1c14505a65055bc0d48baa1ebe7b",
"check_name": "SQL",
"message": "Possible SQL injection",
"file": "app/queries/location_query.rb",
"line": 60,
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
"code": "Arel.sql(\"ST_Distance(#{field_name}, ST_Centroid(location_polygons.area))\")",
"render_path": null,
"location": {
"type": "method",
"class": "LocationQuery",
"method": "sort_by_polygon_distance"
},
"user_input": "field_name",
"confidence": "Medium",
"cwe_id": [
89
],
"note": "field_name does not come from user_input, field_name is hardcoded in the child classes of LocationQuery."
},
{
"warning_type": "Cross-Site Scripting",
"warning_code": 2,
"fingerprint": "eddde082170c2fcbbc5dc3de2db3109eff12fb0e3fd16dbc83082e8fac8a9d91",
"check_name": "CrossSiteScripting",
"message": "Unescaped parameter value",
"file": "app/views/posts/show.html.slim",
"line": 15,
"link": "https://brakemanscanner.org/docs/warning_types/cross_site_scripting",
"code": "sanitize(MarkdownDocument.new(params[:section], params[:subcategory], params[:post_name]).content)",
"render_path": [
{
"type": "controller",
"class": "PostsController",
"method": "show",
"line": 14,
"file": "app/controllers/posts_controller.rb",
"rendered": {
"name": "posts/show",
"file": "app/views/posts/show.html.slim"
}
}
],
"location": {
"type": "template",
"template": "posts/show"
},
"user_input": "params[:section]",
"confidence": "Weak",
"cwe_id": [
79
],
"note": ""
},
{
"warning_type": "SQL Injection",
"warning_code": 0,
"fingerprint": "f11a61ecb831b6d9e0448ef9ea7cd7107bb307154933c07c4a35c3a835a4d406",
"fingerprint": "f7c2efcdfcd1f854891e68c002de7a7b13153238e0a4bab01658786920810555",
"check_name": "SQL",
"message": "Possible SQL injection",
"file": "app/queries/location_query.rb",
"line": 51,
"line": 35,
"link": "https://brakemanscanner.org/docs/warning_types/sql_injection/",
"code": "scope.where(\"ST_DWithin(#{field_name}, ?, ?)\", \"POINT(#{Geocoding.new(query).coordinates.second} #{Geocoding.new(query).coordinates.first})\", radius)",
"code": "scope.joins(\"\\n INNER JOIN location_polygons\\n ON ST_DWithin(#{field_name}, location_polygons.area, #{radius}, false)\\n \")",
"render_path": null,
"location": {
"type": "method",
"class": "LocationQuery",
"method": "handle_coordinates"
"method": "handle_polygon_location"
},
"user_input": "field_name",
"confidence": "Weak",
"cwe_id": [
89
],
"note": "Neither field_name or the coordinates come directly from user input. field_name is hardcoded in the child classes of LocationQuery and the coordinates come from Geocoding#coordinates which returns an array with coordinates returned by our cache or third party geocoding APIs. It defaults to [0,0] if relevant coordinates are not found."
},
{
"warning_type": "Cross-Site Scripting",
"warning_code": 2,
"fingerprint": "f5170bf560d4ed79a5b9ada6e914fe7bd7f90624d63bdd4498454ef6ba60274f",
"check_name": "CrossSiteScripting",
"message": "Unescaped parameter value",
"file": "app/views/posts/show.html.slim",
"line": 15,
"link": "https://brakemanscanner.org/docs/warning_types/cross_site_scripting",
"code": "MarkdownDocument.new(params[:section], params[:subcategory], params[:post_name]).content",
"render_path": [
{
"type": "controller",
"class": "PostsController",
"method": "show",
"line": 14,
"file": "app/controllers/posts_controller.rb",
"rendered": {
"name": "posts/show",
"file": "app/views/posts/show.html.slim"
}
}
],
"location": {
"type": "template",
"template": "posts/show"
},
"user_input": "params[:section]",
"confidence": "Weak",
"cwe_id": [
79
],
"note": ""
}
],
"updated": "2024-10-30 15:08:53 +0000",
"brakeman_version": "6.2.2"
"brakeman_version": "7.0.0"
}
4 changes: 2 additions & 2 deletions spec/queries/vacancy_location_query_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@

before do
expect(default_scope).to receive(:joins).with(
/\s*INNER JOIN location_polygons\s*ON\s*ST_DWithin\(vacancies.geolocation, location_polygons.area, 67578\)\s*/i,
/\s*INNER JOIN location_polygons\s*ON\s*ST_DWithin\(vacancies.geolocation, location_polygons.area, 67578, false\)\s*/i,
).and_return(join_scope)

expect(join_scope).to receive(:where).with("location_polygons.id = ?", location_polygon.id).and_return(where_scope)
Expand All @@ -60,7 +60,7 @@
expect(Geocoding).to receive(:new).with("louth").and_return(geocoder)

expect(default_scope).to receive(:where).with(
"ST_DWithin(vacancies.geolocation, ?, ?)",
"ST_DWithin(vacancies.geolocation, ?, ?, false)",
"POINT(7 -7)",
143_201,
).and_return(where_scope)
Expand Down
Loading