Skip to content

Mongo: Fix reversing polygon coordinates #4609

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.

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

Merged
merged 5 commits into from
Mar 10, 2018

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented Mar 7, 2018

Closes: #4607

Has been tested on the latest parse-dashboard.

https://docs.mongodb.com/manual/reference/glossary/#term-wgs84

@codecov
Copy link

codecov bot commented Mar 7, 2018

Codecov Report

Merging #4609 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4609      +/-   ##
==========================================
+ Coverage   92.59%   92.65%   +0.06%     
==========================================
  Files         119      119              
  Lines        8562     8566       +4     
==========================================
+ Hits         7928     7937       +9     
+ Misses        634      629       -5
Impacted Files Coverage Δ
src/Adapters/Storage/Mongo/MongoTransform.js 85.99% <100%> (+0.09%) ⬆️
src/RestWrite.js 93.41% <0%> (+0.36%) ⬆️
src/Routers/PushRouter.js 96.42% <0%> (+3.57%) ⬆️
src/Adapters/Cache/InMemoryCache.js 100% <0%> (+8.33%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c723582...9dfbd6b. Read the comment docs.

@JacobJT
Copy link

JacobJT commented Mar 7, 2018

I'm still not getting the expected results when querying using query.polygonContains('polygonKey', geoPoint); No results are found when the point is definitely inside the polygon. I cloned @dplewis 's repo, switched to the regression-4608 branch, ran npm install and npm run build, then copied the lib folder and replaced the lib folder in my node_modules/parse-server/ folder on my local server I'm using to test.

Example:

This is the polygon object attached to an instance of my class:

[
  [
    42.631655189280224,
    -83.78406753121705
  ],
  [
    42.633047793854814,
    -83.75333640366955
  ],
  [
    42.61625254348911,
    -83.75149921669944
  ],
  [
    42.61526926650296,
    -83.78161794858735
  ],
  [
    42.631655189280224,
    -83.78406753121705
  ]
]

This is the point I'm searching for: (42.624599,-83.770162) (this is a Parse GeoPoint)

The point is definitely within the bounds of the polygon.

I modified my code to create a new Parse GeoPoint with the latitude = original.latitude, and longitude = original.longitude, and I get the expected result.

@JacobJT
Copy link

JacobJT commented Mar 7, 2018

OK, did more testing and it looks like this change modified how Polygons are saved. That's where the coordinate reversal is taking place. So, the Polygon format should be [long, lat], but it was [lat, long].

I created a new instance of a Polygon instead of using the old one and saw that change. The method works as expected.

So, the problem is that this is going to mean that all polygons currently stored are in reverse order, at least for people using mongo (I recall my Postgres tests were still passing when I modified the test case to highlight this error).

@dplewis
Copy link
Member Author

dplewis commented Mar 8, 2018

@JacobJT What you see in the dashboard and input into your objects are in lat/lng. The databases are saved as lng/lat.(Postgres already stored at lng/lat)

I didn't realize until today that I haven't updated my dashboard to support type Polygon which is why I missed this issue.

@flovilmart How does this look?

@JacobJT
Copy link

JacobJT commented Mar 8, 2018

The polygon in my comment above was taken directly from the dashboard, and was created using the release version of parse-server. The query using a polygonContains constraint yielded no results when using your parse-server branch.

The polygon value below is created using the same Parse GeoPoints, but created using your parse-server branch. The query now works as expected and returns this object, but the fix is not compatible with old polygon data

[
  [
    -83.78406753121705,
    42.631655189280224
  ],
  [
    -83.75333640366955,
    42.633047793854814
  ],
  [
    -83.75149921669944,
    42.61625254348911
  ],
  [
    -83.78161794858735,
    42.61526926650296
  ],
  [
    -83.78406753121705,
    42.631655189280224
  ]
]

@dplewis dplewis requested a review from flovilmart March 8, 2018 15:33
const lat = -45;
const lng = 45;
// Mongo stores polygon in WGS84 lng/lat
const input = {location: { type: 'Polygon', coordinates: [[[lng, lat],[lng, lat]]]}};
Copy link
Contributor

Choose a reason for hiding this comment

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

this is against the documentation for the GeoPoints,m where the list of corrdinates should be provided as lat,lng pairs, not lng,lat pairs.

http://parseplatform.org/Parse-SDK-JS/api/v1.11.1/Parse.GeoPoint.html

If the storage need to be reveresed, that's another issue but should not for something un natural for the user.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wrote it this way to highlight that the input and output are going to be reversed.

The current GeoPoint transform tests are slightly confusing at a glance.

Shouldn't this be testing mongoObjectToParseObject input -> lng/lat output -> lat/lng?

Copy link
Contributor

Choose a reason for hiding this comment

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

OK I misread. Do you have a test that ensures the input / output orders are preserved? ie the mongo transformation is properly applied and reversed?

@dplewis dplewis merged commit c6bc81c into parse-community:master Mar 10, 2018
@dplewis dplewis deleted the regression-4608 branch March 10, 2018 20:27
@dplewis
Copy link
Member Author

dplewis commented Mar 10, 2018

@JacobJT Thanks for helping with this one.

UnderratedDev pushed a commit to UnderratedDev/parse-server that referenced this pull request Mar 21, 2020
* Fix reversing polygon coordinates

* comments fix

* real data test

* improved tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants