Fix GeoPoint issues on PostgreSQL (#3285 & #3659) #3660

vferdiansyah · 2017-03-23T04:44:32Z

This is a small fix to the GeoPoint issues that I experienced on #3285 and #3659

natanrolnik · 2017-03-23T09:02:26Z

@vitaly-t @kulshekhar and @cherukumilli - would you guys mind reviewing this PR?

kulshekhar

Apart from the comment, I think this needs a closer look by someone who uses GeoPoint (I don't) as the tests are failing. My suspicion is that the second set of change might not be necessary - this is just a guess though.

kulshekhar · 2017-03-23T09:28:46Z

src/Adapters/Storage/Postgres/PostgresStorageAdapter.js

-      patterns.push(`ST_distance_sphere($${index}:name::geometry, POINT($${index + 1}, $${index + 2})::geometry) <= $${index + 3}`);
-      sorts.push(`ST_distance_sphere($${index}:name::geometry, POINT($${index + 1}, $${index + 2})::geometry) ASC`)
+      patterns.push(`ST_distance_sphere($${index}:name::geometry, POINT($${index + 2}, $${index + 1})::geometry) <= $${index + 3}`);
+      sorts.push(`ST_distance_sphere($${index}:name::geometry, POINT($${index + 2}, $${index + 1})::geometry) ASC`)
      values.push(fieldName, point.longitude, point.latitude, distanceInKM);
      index += 4;


This looks fine.
Although I think it might be more readable to change the order of latitude/longitude when pushing them in values.

So, leaving lines 328/329 as they originally were and replacing values.push(fieldName, point.longitude, point.latitude, distanceInKM); with values.push(fieldName, point.latitude, point.longitude, distanceInKM); on line 330

whenever in doubt about what pg-promise outputs, you can always run it through pgp.as.format(query, values) locally, throw the result into the console and see what's there ;)

I may have incomplete understanding in what that ST_distance_sphere does, but just to make sure, the :name formatting results in the value being wrapped into double quotes, as in ST_distance_sphere("value", , as per the SQL Names formatting.

Are you guys sure this is what the function actually expects? 'Cos I'm not sure...

It looks very suspecious. Looks to me from the API documentation that it should be a string. Or am I wrong there?

Original tests were passing, so I'd trust it. But I may be wrong

Solution suggested by @kulshekhar also works actually. Just tried it on my local machine.

Not sure if this is what you meant @vitaly-t but as far as I can see, the results aren't wrapped in double quotes at all.

Yes, original tests were passing @flovilmart and it works fine so far on my app.

The best way to see what is really going on in terms of the queries being executed, is with pg-monitor. You can diagnose issues much quicker than.

For that just update PosgresClient.js like this:

Add var monitor = require('pg-monitor');

Call monitor.attach(dbOptions); to attach to the pg options object

Optional, for best look: monitor.setTheme('matrix');

And of course install it first npm install pg-monitor --save

Then you will be able to see every query the module executes 😉

flovilmart · 2017-03-25T18:33:45Z

@vferdiansyah what is very odd in your case, is that the test suite that covers what you describe fails with your changes. The same test suite is used against mongodb.

Can you design a single test that would fail on PG currently and that shows what part is failing?

Particularly have a look at that test suite

Which specifically test the different use cases you describe here and here.

To run the tests on your machine, I encourage you to use:

PARSE_SERVER_TEST_DB=postgres npm test spec/ParseGeoPoint.spec.js

With a local PG database running.

facebook-github-bot · 2017-03-27T01:32:45Z

@vferdiansyah updated the pull request - view changes

vferdiansyah · 2017-03-27T01:34:59Z

Committed new fix that passed all tests on my local machine.

flovilmart · 2017-03-27T20:08:51Z

Tests seems to be passing ;)

vferdiansyah · 2017-03-28T02:44:48Z

Yep. Hopefully this is gonna be released soon.

kulshekhar · 2017-03-28T06:51:00Z

The tests were passing even before this PR but the query wasn't behaving in the expected manner.

@vferdiansyah would it be possible to add a test (similar to the existing ones) that would fail with the existing code but pass with this PR? I suspect it might be as simple as copying the existing test and plugging in the values that didn't work for you.

This would ensure that any future changes to this part of the code won't inadvertently break what your PR fixes!

vferdiansyah · 2017-03-28T09:32:11Z

So I've tried making a new test case which I think is gonna fail, that is returning nearest location since that is the one that breaks on my app. The test looks like this:

it('returns nearest location', (done) => {
    makeSomeGeoPoints(function() {
      var sfo = new Parse.GeoPoint(37.6189722, -122.3748889);
      var query = new Parse.Query(TestObject);
      query.near('location', sfo);
      query.find({
        success: function(results) {
          equal(results[0].get('name'), 'San Francisco');
          equal(results[1].get('name'), 'Sacramento');
          done();
        }
      });
    });
  });

BUT, the test still PASSED on both the current version and the one with my PR.

HOWEVER, when I tried it in my app, both returns a very different results. I'm querying a place nearby a location with coordinates (35.0056643, 135.7745649).

Current version:
With PR:

As you can see in the picture above, the latitude returned by the current version is wrong. Latitude should be ranged from -90 to +90. And I have checked my database and can confirm that the latitude value is actually a longitude and vice versa.

And with the current PR, it returns the desired value.

The thing that I don't understand is, according to the test, it returns the desired results. So how can it breaks? Or maybe I miss something with my test case above?

vferdiansyah · 2017-03-29T12:18:01Z

Any inputs on this @flovilmart @vitaly-t?

flovilmart · 2017-03-29T12:52:25Z

Yeah, that's a bummer, I dont recall why I reversed latitudes and longitude in the point, but because this is against the 'natural' order, there's probably a good reason. Otherwise, it's good, as it reversing the points twice, but may break when updating the server.

codecov · 2017-04-07T11:45:17Z

Codecov Report

Merging #3660 into master will increase coverage by 0.29%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3660      +/-   ##
==========================================
+ Coverage   90.16%   90.45%   +0.29%     
==========================================
  Files         114      114              
  Lines        7532     7406     -126     
==========================================
- Hits         6791     6699      -92     
+ Misses        741      707      -34

Impacted Files	Coverage Δ
...dapters/Storage/Postgres/PostgresStorageAdapter.js	`95.58% <100%> (+0.26%)`	⬆️
src/password.js	`87.5% <0%> (-12.5%)`	⬇️
src/Adapters/Cache/InMemoryCache.js	`92.3% <0%> (-7.7%)`	⬇️
src/AccountLockout.js	`91.8% <0%> (-5.82%)`	⬇️
src/Routers/UsersRouter.js	`87.02% <0%> (-1.87%)`	⬇️
src/Controllers/LoggerController.js	`88.05% <0%> (-0.83%)`	⬇️
src/Push/PushQueue.js	`96% <0%> (-0.56%)`	⬇️
src/triggers.js	`87.63% <0%> (-0.14%)`	⬇️
src/StatusHandler.js	`98.97% <0%> (-0.08%)`	⬇️
src/Controllers/DatabaseController.js	`94.3% <0%> (-0.06%)`	⬇️
... and 22 more

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 17a2d26...f18cc3c. Read the comment docs.

flovilmart · 2017-04-07T11:47:53Z

@vferdiansyah Can you add a test that shows the fix is effective? From what I can see, we flipped the order of the coordinates inside the storage but all tests are still good.

flovilmart · 2017-04-07T15:03:05Z

@vferdiansyah I just ran the test locally alone, on the top of the master branch and it doesn'T seem to validate the error (it should fail)

vferdiansyah · 2017-04-08T02:02:13Z

I just ran the test locally alone, on the top of the master branch and it doesn'T seem to validate the error (it should fail)

That's why I wrote this in my previous comment:

BUT, the test still PASSED on both the current version and the one with my PR.

It fails on my app but it passed all tests. So something must be wrong down the road but I still can't find where.

If you see the screenshots of the response on my previous comment, you'll know that it's a bug.

flovilmart · 2017-04-08T12:55:22Z

That's what bugs me, it seems that we don't really understand where it fails and why.

vferdiansyah · 2017-04-09T06:40:08Z

@flovilmart I've tried several things today (like exchanging latitude to longitude, etc), but this is the only one that works :/

flovilmart · 2017-04-09T13:16:49Z

I can understand your frustration, however, this bug seems to be critical, and without a proper test that makes an example out of it, I'm not really sure what to think.

vferdiansyah · 2017-04-09T23:44:34Z

Yeah I know. Do you still can't recall why did you reverse the order when you first created this adapter?

flovilmart · 2017-04-09T23:52:51Z

Because latitude / longitude translate to y/x more than x/y

flovilmart · 2017-04-30T14:31:48Z

We can't find the actual underlying by creating proper reproduction in the tests.

dplewis · 2017-05-28T04:19:25Z

Hi.

I've encountered the reversing issue. Since Postgres uses WGS84 Longitude goes first.

If you want fix the reversing it happened on this line. updateObjectByQuery.

https://github.com/parse-community/parse-server/blob/master/src/Adapters/Storage/Postgres/PostgresStorageAdapter.js#L925

Old

values.push(fieldName, fieldValue.latitude, fieldValue.longitude);

It should be written like.

values.push(fieldName, fieldValue.longitude, fieldValue.latitude);

I can do a PR and test for this

flovilmart · 2017-05-28T12:29:53Z

That would be good! Thanks!

flovilmart · 2017-05-29T00:57:04Z

Closing as supersed by #3874

Fix GeoPoint issues on PostgreSQL (#3285 & #3659)

05917b6

facebook-github-bot added the GH Review: review-needed label Mar 23, 2017

natanrolnik requested a review from cherukumilli March 23, 2017 09:02

kulshekhar reviewed Mar 23, 2017

View reviewed changes

Fix for failed tests

651d785

Merge branch 'master' into master

6c8c494

Add a near location test case

859fe26

Merge branch 'master' into master

f18cc3c

flovilmart closed this May 29, 2017

parseplatformorg mentioned this pull request Jun 2, 2023

refactor: Upgrade mongodb from 4.10.0 to 5.5.0 #8602

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix GeoPoint issues on PostgreSQL (#3285 & #3659) #3660

Fix GeoPoint issues on PostgreSQL (#3285 & #3659) #3660

vferdiansyah commented Mar 23, 2017

natanrolnik commented Mar 23, 2017

kulshekhar left a comment •

edited

Loading

kulshekhar Mar 23, 2017

vitaly-t Mar 23, 2017 •

edited

Loading

vitaly-t Mar 23, 2017 •

edited

Loading

flovilmart Mar 23, 2017

vferdiansyah Mar 24, 2017

vitaly-t Mar 25, 2017 •

edited

Loading

flovilmart commented Mar 25, 2017 •

edited

Loading

facebook-github-bot commented Mar 27, 2017

vferdiansyah commented Mar 27, 2017

flovilmart commented Mar 27, 2017

vferdiansyah commented Mar 28, 2017

kulshekhar commented Mar 28, 2017 •

edited

Loading

vferdiansyah commented Mar 28, 2017 •

edited

Loading

vferdiansyah commented Mar 29, 2017

flovilmart commented Mar 29, 2017

codecov bot commented Apr 7, 2017 •

edited

Loading

flovilmart commented Apr 7, 2017

flovilmart commented Apr 7, 2017

vferdiansyah commented Apr 8, 2017

flovilmart commented Apr 8, 2017

vferdiansyah commented Apr 9, 2017

flovilmart commented Apr 9, 2017

vferdiansyah commented Apr 9, 2017

flovilmart commented Apr 9, 2017

flovilmart commented Apr 30, 2017

dplewis commented May 28, 2017 •

edited

Loading

flovilmart commented May 28, 2017

flovilmart commented May 29, 2017

Fix GeoPoint issues on PostgreSQL (#3285 & #3659) #3660

Fix GeoPoint issues on PostgreSQL (#3285 & #3659) #3660

Conversation

vferdiansyah commented Mar 23, 2017

natanrolnik commented Mar 23, 2017

kulshekhar left a comment • edited Loading

Choose a reason for hiding this comment

kulshekhar Mar 23, 2017

Choose a reason for hiding this comment

vitaly-t Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

vitaly-t Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

flovilmart Mar 23, 2017

Choose a reason for hiding this comment

vferdiansyah Mar 24, 2017

Choose a reason for hiding this comment

vitaly-t Mar 25, 2017 • edited Loading

Choose a reason for hiding this comment

flovilmart commented Mar 25, 2017 • edited Loading

facebook-github-bot commented Mar 27, 2017

vferdiansyah commented Mar 27, 2017

flovilmart commented Mar 27, 2017

vferdiansyah commented Mar 28, 2017

kulshekhar commented Mar 28, 2017 • edited Loading

vferdiansyah commented Mar 28, 2017 • edited Loading

vferdiansyah commented Mar 29, 2017

flovilmart commented Mar 29, 2017

codecov bot commented Apr 7, 2017 • edited Loading

Codecov Report

flovilmart commented Apr 7, 2017

flovilmart commented Apr 7, 2017

vferdiansyah commented Apr 8, 2017

flovilmart commented Apr 8, 2017

vferdiansyah commented Apr 9, 2017

flovilmart commented Apr 9, 2017

vferdiansyah commented Apr 9, 2017

flovilmart commented Apr 9, 2017

flovilmart commented Apr 30, 2017

dplewis commented May 28, 2017 • edited Loading

flovilmart commented May 28, 2017

flovilmart commented May 29, 2017

kulshekhar left a comment •

edited

Loading

vitaly-t Mar 23, 2017 •

edited

Loading

vitaly-t Mar 23, 2017 •

edited

Loading

vitaly-t Mar 25, 2017 •

edited

Loading

flovilmart commented Mar 25, 2017 •

edited

Loading

kulshekhar commented Mar 28, 2017 •

edited

Loading

vferdiansyah commented Mar 28, 2017 •

edited

Loading

codecov bot commented Apr 7, 2017 •

edited

Loading

dplewis commented May 28, 2017 •

edited

Loading