Add Within Polygon to Query #320

dplewis · 2017-05-30T22:30:16Z

I added support for querying within polygon on the latest server update

Let me know if I should add more tests.

montymxb

@dplewis I took a look at this over on the server, nice job getting this added in!

If you can put in those extra tests we should be good. Although I will ask that we hold onto this PR for now, at least until the next parse-server release is out and the requested feature can be used/tested against. Once this feature is out I'll run the tests and verify everything.

montymxb · 2017-05-31T03:41:36Z

src/Parse/ParseQuery.php

+
+        return $this;
+    }
+


Can you add an additional test case where an empty array is passed to withinPolygon. I'm curious what the defined behavior should be for that, either to return no objects or all objects?

Also is the 'within' inclusive in regards to the bounds? Perhaps you could add a test that defines whether or not an object should be returned if it matches one of the bounds exactly?

Thanks for the input. I honestly don't know how this works or the limitations. We'll know soon enough.

parse-community/parse-server#3889

so i've added your tests in the server. what tests should go here?

@dplewis If you can make sure you have the following:

Test with less than 3 points (should fail)

Test with no points/empty array (should fail i think...)

Test with at least 3 points (just to verify the min works)

Test for returning an object that lies on the bounds, can have the same coords as one of the points for example

Test for returning an object that lies beyond the bounds (fail, you may already have this though, if you do ignore this)

Test for returning an object within bounds (pretty sure you have this already so you should be good)

Test passing something other than a GeoPoint to withinPolygon (should fail)

That should tightly define what we will be expecting in terms of behavior from this additional query parameter in this sdk. There might be one or two more but that sums it up for now. Some of the above tests you already have included, and that's fine, it's just a summary of all the cases I can think of to account for.

I've added additional tests for open vs closed paths. The are both supported.

@dplewis I did not. I run all tests against mongodb by default. I'm making an assumption that database integrations are sufficiently vetted/tested on the server side of things to ensure that using an alternate db would not change behavior in the API.

However that's not always the case in reality. Did you find something running against an alternate db or were you just checking?

I have postgres running locally at all times. Even though I don't use it in production
I found an issue with Bytes support on postgres.

parse-community/parse-server#3894

@dplewis nice catch! Although it is helpful to check against multiple db types I would say such checks are more appropriate when running parse-server's test suite. That issue should have been caught during routine testing.

Basically when I run tests against this I'm making a certain degree of assumptions, first and foremost being that the database integrations are properly tested for compatibility before rolling out. Granted this is only an assumption, so issues may come to light from time to time.

I know and postgres has limited support that isn't documented.
The parse-server test's suite had it_exclude_dbs(['postgres'])('byte work') to prevent failing on postgres. Other than that php looks good.

No kidding...well that's good to know that you've caught that on the server. The code base here should be OK for now. There's always more to change given enough time though!

dplewis · 2017-06-20T23:28:45Z

@montymxb Can we see if travis runs on this? Close and reopen?

montymxb · 2017-06-20T23:33:55Z

@dplewis sure thing!

dplewis · 2017-06-20T23:50:24Z

I changed the error messages on parse server 2.5.0. We can try again when it is released

montymxb · 2017-06-20T23:56:25Z

Sure, the tests should be ok when it's republished. We can close/open it again or you can push a commit, either will trigger another travis build. Last I looked at them your tests seemed okay though! Just need to wait for the next release.

dplewis · 2017-06-21T00:30:33Z

Thanks for setting up travis. I couldn't wrap my head around running composer and npm timing

montymxb · 2017-06-21T00:37:11Z

No problem, it can be a bit much at first sometimes. I've been meaning to put in Travis CI for some time, but I suppose better late than never?

flovilmart · 2017-06-21T00:51:29Z

Or you can point Parse-server-tests to use parse-community/Parse-server:latest instead of a fixed version, this way you’ll always be ahead of breaking changes

montymxb · 2017-06-21T02:45:04Z

Running it against the git repo would keep it up to date with the latest changes. Normally I wouldn't be too fond of jumping ahead of official releases, but considering it's a 'test' server just for development that would help speed up these dev cycles; rather than wait for the next release.

Once I make those changes I'll verify against travis again.

montymxb · 2017-06-21T17:47:59Z

@dplewis after much tinkering I was unable to get a running server straight from the parse-server github off the latest branch. It would appear there is some issue with resolving dependencies within a 'git' dependency. There's a way to do it of course, it's just escaped me for the moment...

I did run your code locally against the latest parse-server to verify, and the test results are the same against the current release that was tested on travis. I reran the tests from Travis to clean up some of the results for your viewing. You can select the 'Details' next to the CI build status to see more, specifically here's a job in that build for your viewing. You can get to this by selecting 'Details' as well.

Looks like it's coming down to 2 errors and 2 failures. As for the failures it looks like the server is consistently returning bad $geoWithin value instead of the expected bad $geoWithin value; $polygon should contain at least 3 GeoPoints. Cracking open parse-server release only one place where that terse message is being returned.

if (!GeoPointCoder.isValidJSON(point)) {
    throw new Parse.Error(Parse.Error.INVALID_JSON, 'bad $geoWithin value');
} else {
    Parse.GeoPoint._validate(point.latitude, point.longitude);
}

The message regarding at least 3 GeoPoints may only be returned if

!(polygon instanceof Array)

or

polygon.length < 3

Seems for some reason both of these checks are passing, even in cases where neither too small of an array or no array at all is passed as a constraint to withinPolygon.

As for the errors it would appear in both cases (which are both failure cases) no recognized exception is being thrown by the server, but no response is either. Leading the sdk to attempting to pull out the non-existant results. The first failure appears to have a server error of [MongoError: Polygon must have at least 3 points], the second does as well. It would appear that an error is occurring in advance of you checking the input, causing the exception to be thrown out.

It looks like the server will need a few more changes. I'll take a look at it myself now that I've noticed this. If I can find anything I'll provide some pointers as to what needs to be corrected.

dplewis · 2017-06-21T18:33:40Z

@montymxb parse-community/parse-server#3889

codecov · 2017-06-21T21:03:42Z

Codecov Report

❗ No coverage uploaded for pull request base (master@9bdbd71). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master     #320   +/-   ##
=========================================
  Coverage          ?   98.04%           
=========================================
  Files             ?       35           
  Lines             ?     2708           
  Branches          ?        0           
=========================================
  Hits              ?     2655           
  Misses            ?       53           
  Partials          ?        0

Impacted Files	Coverage Δ
src/Parse/ParseQuery.php	`99.23% <100%> (ø)`

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 9bdbd71...858b177. Read the comment docs.

montymxb · 2017-06-21T21:08:46Z

@dplewis figured it out, we're good! My node version locally was giving me false negatives due to being very much out of date :/ . Travis is now up to date tracking the latest successful build (of parse-server) and this looks good to merge in now.

The codecov base report will be resolved once PRs start branching off of already covered commits, what's important is just the CI here.

dplewis added 2 commits May 30, 2017 17:28

Add Within Polygon to Query

2b6ed75

lint

b017225

montymxb suggested changes May 31, 2017

View reviewed changes

dplewis added 2 commits May 31, 2017 16:03

added test for open and closed paths

12e1745

remove helper

858b177

montymxb closed this Jun 20, 2017

montymxb reopened this Jun 20, 2017

montymxb approved these changes Jun 21, 2017

View reviewed changes

montymxb merged commit 20dd7a5 into parse-community:master Jun 21, 2017

dplewis deleted the within-polygon branch June 22, 2017 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Within Polygon to Query #320

Add Within Polygon to Query #320

dplewis commented May 30, 2017

montymxb left a comment

montymxb May 31, 2017

dplewis May 31, 2017

dplewis May 31, 2017

montymxb May 31, 2017

dplewis May 31, 2017

montymxb Jun 2, 2017 •

edited

Loading

dplewis Jun 2, 2017

montymxb Jun 3, 2017

dplewis Jun 3, 2017

montymxb Jun 3, 2017

dplewis commented Jun 20, 2017

montymxb commented Jun 20, 2017

dplewis commented Jun 20, 2017

montymxb commented Jun 20, 2017

dplewis commented Jun 21, 2017

montymxb commented Jun 21, 2017

flovilmart commented Jun 21, 2017

montymxb commented Jun 21, 2017

montymxb commented Jun 21, 2017

dplewis commented Jun 21, 2017

codecov bot commented Jun 21, 2017 •

edited

Loading

montymxb commented Jun 21, 2017 •

edited

Loading

Add Within Polygon to Query #320

Add Within Polygon to Query #320

Conversation

dplewis commented May 30, 2017

montymxb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

montymxb Jun 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dplewis commented Jun 20, 2017

montymxb commented Jun 20, 2017

dplewis commented Jun 20, 2017

montymxb commented Jun 20, 2017

dplewis commented Jun 21, 2017

montymxb commented Jun 21, 2017

flovilmart commented Jun 21, 2017

montymxb commented Jun 21, 2017

montymxb commented Jun 21, 2017

dplewis commented Jun 21, 2017

codecov bot commented Jun 21, 2017 • edited Loading

Codecov Report

montymxb commented Jun 21, 2017 • edited Loading

montymxb Jun 2, 2017 •

edited

Loading

codecov bot commented Jun 21, 2017 •

edited

Loading

montymxb commented Jun 21, 2017 •

edited

Loading