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.

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

Added 'withinPolygon' to query #3866

Merged
merged 7 commits into from
May 28, 2017
Merged

Added 'withinPolygon' to query #3866

merged 7 commits into from
May 28, 2017

Conversation

dplewis
Copy link
Member

@dplewis dplewis commented May 27, 2017

This PR addresses issue #3290

My first PR any help would be great.

I don't know much about Postgres. Is there a guide to setup Postgres with parse server locally?

curl -X POST \
  -H "X-Parse-Application-Id: app-id-here" \
  -H "Content-Type: application/json" \
  -d '{
        "location": {
          "__type": "GeoPoint",
          "latitude": 1.5,
          "longitude": 1.5
        }
      }' \
http://localhost:1337/parse/classes/TestObject
curl -X GET \
-H "X-Parse-Application-Id: app-id-here" \
-H "Content-Type: application/json" \
  -G \
  --data-urlencode 'where={
        "location": {
          "$geoWithin": {
            "$polygon": [
              {
                "__type": "GeoPoint",
                "latitude": 1,
                "longitude": 1
              },
              {
                "__type": "GeoPoint",
                "latitude": 1,
                "longitude": 2
              },
              {
                "__type": "GeoPoint",
                "latitude": 2,
                "longitude": 2
              },
              {
                "__type": "GeoPoint",
                "latitude": 2,
                "longitude": 1
              }
            ]
          }
        }
      }' \
http://localhost:1337/parse/classes/TestObject

@codecov
Copy link

codecov bot commented May 27, 2017

Codecov Report

Merging #3866 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3866      +/-   ##
==========================================
+ Coverage   90.17%   90.22%   +0.05%     
==========================================
  Files         114      114              
  Lines        7550     7571      +21     
==========================================
+ Hits         6808     6831      +23     
+ Misses        742      740       -2
Impacted Files Coverage Δ
src/Adapters/Storage/Mongo/MongoTransform.js 83.33% <100%> (+0.35%) ⬆️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 95.52% <100%> (+0.06%) ⬆️
src/Adapters/Cache/InMemoryCache.js 100% <0%> (+7.69%) ⬆️

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 aedaae1...7f50180. Read the comment docs.

@flovilmart
Copy link
Contributor

Hi @dplewis Thanks for this PR! That'S great that you jump in on such requested feature!

Would you please add some unit tests to cover this feature?

The best location would be here where other geoPoints tests are run.

If you can add a tests that covers:

  1. Bad input (malformed inputs, etc...)
  2. Successfully fetches all objects that have points within the polygon, while excluding the ones outside.

Because the JS SDK doesn't have the within polygon yet, you should use request and craft the query directly as a REST request. You can find an example query request here
Thanks!

@dplewis
Copy link
Member Author

dplewis commented May 27, 2017

Test have been added

@flovilmart
Copy link
Contributor

flovilmart commented May 27, 2017 via email

@dplewis
Copy link
Member Author

dplewis commented May 27, 2017

Sorry for the late reply. I've added more tests

@dplewis
Copy link
Member Author

dplewis commented May 27, 2017

Quick question when writing test do the test objects get deleted before and after the Unit Tests

@flovilmart
Copy link
Contributor

Yes, the whole DB is cleaned up between tests

@dplewis
Copy link
Member Author

dplewis commented May 27, 2017

Interesting at one point the test passed the first time but second and third, etc failed until I deleted all objects.

@flovilmart
Copy link
Contributor

Seems that it's not properly supported on Postgres. Can you check the Travis logs?

@dplewis
Copy link
Member Author

dplewis commented May 27, 2017

How do I go about running tests against Postgres

@flovilmart
Copy link
Contributor

You can disable the Postgres test for now with it_exclude_db() (Check in other tests for the exact syntax)
Or to run the pg tests:

PARSE_SERVER_TEST_DB=postgres npm test

You'll need Postgres installed and configured (as in the .travis.yaml)

@dplewis
Copy link
Member Author

dplewis commented May 27, 2017

I got the test to pass locally with postgres

@dplewis
Copy link
Member Author

dplewis commented May 27, 2017

Can you create add testing Postgres to CONTRIBUTING.md?

I can try to help out on the Postgres Adapter.

@flovilmart
Copy link
Contributor

Can you create add testing Postgres to CONTRIBUTING.md?

I'll work on the contributing guidelines :)

@@ -10,6 +10,7 @@ const PostgresDuplicateObjectError = '42710';
const PostgresUniqueIndexViolationError = '23505';
const PostgresTransactionAbortedError = '25P02';
const logger = require('../../../logger');
const _typeof = typeof Symbol === "function" && typeof Symbol.iterator === "symbol" ? function (obj) { return typeof obj; } : function (obj) { return obj && typeof Symbol === "function" && obj.constructor === Symbol && obj !== Symbol.prototype ? "symbol" : typeof obj; };
Copy link
Contributor

Choose a reason for hiding this comment

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

does that come from transpired babel source? please remove :)

throw new Parse.Error(Parse.Error.INVALID_JSON, 'bad $geoWithin value');
}
const points = polygon.map((point) => {
if (!((typeof point === 'undefined' ? 'undefined' : _typeof(point)) === 'object' && point !== null && point.__type === 'GeoPoint')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

doesn't seem to be something a human would write :)
can we replace with:

if (typeof point !== 'object' || point.__type !== 'GeoPoint') {

Copy link
Member Author

Choose a reason for hiding this comment

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

Good thing I'm not human

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry if I offended you, it looked like generated code

@flovilmart
Copy link
Contributor

@dplewis just one small nit, otherwise it's looking awesome!

nit?
@flovilmart flovilmart merged commit c99fdea into parse-community:master May 28, 2017
@dplewis dplewis deleted the within-polygon branch May 28, 2017 16:55
@dplewis
Copy link
Member Author

dplewis commented May 28, 2017

Thanks, when can we update the SDKs?

Also theres a debug function in PostgresAdapter. How do I run that?

@flovilmart
Copy link
Contributor

With PARSE_SERVER_LOG_LEVEL=debug

@flovilmart
Copy link
Contributor

@dplewis we can update the SDK's anytime, on old parse-server versions that will just fail as unsupported queries, with next release, this should work nicely

@madsb
Copy link
Contributor

madsb commented May 30, 2017

Holy smokes, this looks awesome - can't wait to try it out. Thanks, @dplewis! 🙏🏼

@otymartin
Copy link

@dplewis will you be updating the iOS SDK with this aswell?

@dplewis
Copy link
Member Author

dplewis commented Jun 20, 2017

@otymartin I'm working on it now should be there soon

@montymxb
Copy link
Contributor

@dplewis nice to see this bumping along! Has the android sdk received this yet by chance?

@otymartin
Copy link

@dplewis nice thanks man

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.

None yet

5 participants