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

Add Within Polygon to Query #320

Merged
merged 4 commits into from
Jun 21, 2017
Merged

Add Within Polygon to Query #320

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2b6ed75
Add Within Polygon to Query
dplewis May 30, 2017
b017225
lint
dplewis May 30, 2017
12e1745
added test for open and closed paths
dplewis May 31, 2017
858b177
remove helper
dplewis May 31, 2017
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
20 changes: 20 additions & 0 deletions src/Parse/ParseQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,26 @@ public function withinGeoBox($key, $southwest, $northeast)
return $this;
}

/**
* Add a constraint to the query that requires a particular key's
* coordinates be contained within a given polygon
*
* @param string $key The key of the ParseGeoPoint
* @param array $points Array of ParseGeoPoint
*
* @return ParseQuery Returns this query, so you can chain this call.
*/
public function withinPolygon($key, $points)
{
$this->addCondition(
$key,
'$geoWithin',
['$polygon' => $points]
);

return $this;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

parse-community/parse-server#3889

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

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've added additional tests for open vs closed paths. The are both supported.

Copy link
Contributor

@montymxb montymxb Jun 2, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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?

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 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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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!

/**
* Add a constraint to the query that requires a particular key's value to
* be contained in the provided list of values.
Expand Down
44 changes: 36 additions & 8 deletions tests/Parse/ParseGeoPointTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,20 +200,48 @@ public function testGeoMaxDistanceWithUnits()
$query->withinMiles('location', $point, 10.0);
$results = $query->find();
$this->assertEquals(0, count($results));

}

public function testBadLatitude() {
$this->setExpectedException('\Parse\ParseException',
'Latitude must be within range [-90.0, 90.0]');
public function testBadLatitude()
{
$this->setExpectedException(
'\Parse\ParseException',
'Latitude must be within range [-90.0, 90.0]'
);
new ParseGeoPoint(-180, 32);

}

public function testBadLongitude() {
$this->setExpectedException('\Parse\ParseException',
'Longitude must be within range [-180.0, 180.0]');
public function testBadLongitude()
{
$this->setExpectedException(
'\Parse\ParseException',
'Longitude must be within range [-180.0, 180.0]'
);
new ParseGeoPoint(32, -360);
}

public function testWithinPolygon()
{
$obj1 = ParseObject::create('TestObject');
$obj2 = ParseObject::create('TestObject');
$obj3 = ParseObject::create('TestObject');

$obj1->set('location', new ParseGeoPoint(1.5, 1.5));
$obj2->set('location', new ParseGeoPoint(2, 8));
$obj3->set('location', new ParseGeoPoint(20, 20));

ParseObject::saveAll([$obj1, $obj2, $obj3]);

$points = [
new ParseGeoPoint(0, 0),
new ParseGeoPoint(0, 10),
new ParseGeoPoint(10, 10),
new ParseGeoPoint(10, 0)
];

$query = new ParseQuery('TestObject');
$query->withinPolygon('location', $points);
$results = $query->find();
$this->assertEquals(2, count($results));
}
}