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

Jump to bottom

Extend graphql queries with a geo query #3084

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Extend graphql queries with a geo query #3084

wants to merge 5 commits into from

Conversation

ysf-simsoft
Copy link
Contributor

@ysf-simsoft ysf-simsoft commented Jun 26, 2020 •
edited
Loading

Added a column named geometry to locations table which is type of geometry. By using this column we can perform queries by using gis functions. Specifically we aim to use ST_Within gis function to query locations, reports and positions within a given polygon.

  • SqlServer has built-in support for geometry type and gis functions. No configuration changes required for SqlServer.
  • Enable PostGIS extension on PostgreSQL by using postgis docker image
  • Add db migration for adding geometry column (geometry) to locations table
  • Add db migration to populate geometry column with a geometry type from existing lat,lng columns
  • Update test data with geometry values
  • Update test data conversion script for geometry values
  • Add gradle task for dropping postgis extension and related objects. Also needed to make sure that it runs before dbDrop task otherwise drop-all liquibase command fails.
  • Add withinPolygon method to graphql schema for ReportSearchQuery, LocationSearchQuery and PositionSearchQuery beans.

Release notes

Closes #3043

User changes

  • none

Super User changes

  • none

Admin changes

  • none

System admin changes

  • anet.yml needs change
  • db needs migration
  • documentation has changed
  • graphql schema has changed

Checklist

  • Described the user behavior in PR body
  • Referenced/updated all related issues
  • commits follow a repo#issue: Title title format and these 7 rules
  • commits have a clean history, otherwise PR may be squash-merged
  • Added and/or updated unit tests
  • Added and/or updated e2e tests
  • Added and/or updated data migrations
  • Updated documentation
  • Resolved all build errors and warnings
  • Opened debt issues for anything not resolved here

@ysf-simsoft ysf-simsoft added this to the 2020 June milestone Jun 26, 2020
@ysf-simsoft ysf-simsoft force-pushed the GH-3043-extend-graphql-queries-with-a-geo-query branch from b137335 to 45492d0 Compare June 26, 2020 10:42
@ysf-simsoft ysf-simsoft marked this pull request as ready for review June 26, 2020 11:03
VassilIordanov
VassilIordanov reviewed Jun 27, 2020
View reviewed changes
insertBaseData-mssql.sql Outdated
VALUES (N'7339f9e3-99d1-497a-9e3b-1269c4c287fe', 'Harbour Grace Police Station', 47.705133, -53.214422, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP);
INSERT INTO locations (uuid, name, lat, lng, createdAt, updatedAt)
VALUES (N'f2207d9b-204b-4cb5-874d-3fe6bc6f8acd', 'Conception Bay South Police Station', 47.526784, -52.954739, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP);
INSERT INTO locations (uuid, name, lat, lng, gisPoint, createdAt, updatedAt)
Copy link
Contributor

@VassilIordanov VassilIordanov Jun 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

we need to remove the lat and lng columns as they are redundant with the with GIS geometry (named gisPoint here, but probably better named geometry as we may not need to be constrained to a point in the future).
So I suggest to drop the columns in a migration, and refactor the API to return/set a geometry instead of lat and lon

Copy link
Contributor Author

@ysf-simsoft ysf-simsoft Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Removing lat and lng columns;

Due to above reasons and in order to keep things simple, I suggest keeping this PR as is and open a debt issue for removing lat and lng columns. Also postpone that issue until #3073 #3062 and #3056 are merged and #3044 is implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed column name to geometry

Copy link
Contributor

Choose a reason for hiding this comment

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

The issue is that by merging this PR, we will have the geo data twice in the database - once as lat/lon and once as a geometry. Moreover, when new location objects are created or the ones modified, their geomerty is not updated, so the two do get out of sync. Even if they were in sync, it would be combersome to manipulate the database if needed with external scripts.

@ysf-simsoft ysf-simsoft force-pushed the GH-3043-extend-graphql-queries-with-a-geo-query branch from 45492d0 to 77159e0 Compare June 30, 2020 12:06
gjvoosten
gjvoosten requested changes Jul 6, 2020
View reviewed changes
Copy link
Collaborator

@gjvoosten gjvoosten left a comment

Choose a reason for hiding this comment

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

Also, the PR description still mentions gis_point which should be geometry I believe.

prepare-psql.sql
@@ -11,3 +11,5 @@ CREATE OR REPLACE AGGREGATE tsvector_agg (tsvector) (

-- make sure the preconditions for UUID prefix search are met
CREATE EXTENSION IF NOT EXISTS pg_trgm;

CREATE EXTENSION IF NOT EXISTS postgis;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add a comment why this extension is added.

@ysf-simsoft ysf-simsoft force-pushed the GH-3043-extend-graphql-queries-with-a-geo-query branch from 77159e0 to ef0f584 Compare July 6, 2020 20:02
@ysf-simsoft ysf-simsoft marked this pull request as draft July 9, 2020 18:35
@ysf-simsoft ysf-simsoft force-pushed the GH-3043-extend-graphql-queries-with-a-geo-query branch from ef0f584 to bccb7cc Compare July 24, 2020 09:25
@ysf-simsoft ysf-simsoft force-pushed the GH-3043-extend-graphql-queries-with-a-geo-query branch from bccb7cc to 7d5fd43 Compare August 22, 2020 19:48
@lgtm-com
Copy link

lgtm-com bot commented Aug 22, 2020

This pull request introduces 3 alerts when merging 7d5fd43 into 05d6382 - view on LGTM.com

new alerts:

  • 3 for Missing Override annotation

@ysf-simsoft ysf-simsoft force-pushed the GH-3043-extend-graphql-queries-with-a-geo-query branch from 7d5fd43 to 57db4d9 Compare August 23, 2020 13:08
@ysf-simsoft
#3043: Configure mssql and psql for geosearch queries
e12a116 
 - Enable PostGIS extension on PostgreSQL
 - Add db migration for adding geometry column to locations
 - Add db migration to populate geometry from existing lat,lng columns
 - Update test data with geometry values
 - Update test data conversion script for geometry values
 - Add gradle task for dropping postgis extension and make sure that it
   runs before dbDrop task otherwise drop-all liquibase command fails.
@ysf-simsoft
#3043: Add geosearch query for locations
63f2ba5
@ysf-simsoft
#3043: Add geosearch query for reports
04409f1
@ysf-simsoft
#3043: Add geosearch query for positions
cda4cdc
@ysf-simsoft
#3043: Add e2e api tests for geosearch queries
304ce58
@ysf-simsoft ysf-simsoft force-pushed the GH-3043-extend-graphql-queries-with-a-geo-query branch from 57db4d9 to 304ce58 Compare November 26, 2020 09:42
@gjvoosten gjvoosten changed the title GH 3043 extend graphql queries with a geo query Extend graphql queries with a geo query Apr 7, 2021
@gjvoosten gjvoosten requested review from midmarch and removed request for oayvazoglusim December 23, 2021 13:45
@midmarch midmarch removed their request for review August 16, 2023 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VassilIordanov VassilIordanov VassilIordanov left review comments

@gjvoosten gjvoosten gjvoosten requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2020 June
Development

Successfully merging this pull request may close these issues.

Extend graphql queries with a geo query
3 participants
@ysf-simsoft @gjvoosten @VassilIordanov