fix: server crashes on MongoDB GeoJSON error instead of throwing Parse.Error #7347

mstniy · 2021-04-14T19:46:05Z

New Pull Request Checklist

[ x] I am not disclosing a vulnerability.
[ x] I am creating this PR in reference to an issue.

Issue Description

Creating an object in a collection with a spatially indexed field fails if that field of the new object does not contain valid geometry. This is not handled by Parse, causing an INTERNAL_SERVER_ERROR that makes it difficult to diagnose invalid geojsons.

Related issue: #7331

Approach

Geojson errors produced by Mongo are cought and turned into Parse errors.

TODOs before merging

[ x ] Add test cases
Add entry to changelog
Add changes to documentation (guides, repository pages, in-code descriptions)
Add security check
[ x ] Add new Parse Error codes to Parse JS SDK
Depend on a release of the js sdk, instead of a commit hash.

Also added a corresponding test

ghost · 2021-04-14T19:47:27Z

	Warnings
⚠️	Please add a changelog entry for your changes.

Generated by 🚫 dangerJS

codecov · 2021-04-14T19:53:47Z

Codecov Report

Merging #7347 (ec1f80d) into alpha (8a126fc) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head ec1f80d differs from pull request most recent head 0301472. Consider uploading reports for the commit 0301472 to get more accurate results

@@            Coverage Diff             @@
##            alpha    #7347      +/-   ##
==========================================
- Coverage   94.20%   94.19%   -0.01%     
==========================================
  Files         182      182              
  Lines       13587    13590       +3     
==========================================
+ Hits        12799    12801       +2     
- Misses        788      789       +1

Impacted Files	Coverage Δ
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	`93.41% <100.00%> (+0.25%)`	⬆️
src/RestWrite.js	`93.88% <0.00%> (-0.32%)`	⬇️

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 8a126fc...0301472. Read the comment docs.

davimacedo

Thanks for the PR! Let's wait for the new version of the SDK and we are good to merge this one.

package.json

mtrezza · 2021-04-22T10:56:25Z

spec/ParseAPI.spec.js

+    testSchema.addIndex('geospatial_index', {
+      geometry: '2dsphere',
+    });
+    await testSchema.save();


I want to make sure we don't introduce a flaky test here. This awaits the schema save, but not the index building in DB.

Maybe you want to look into schema tests to see what do we usually use in these cases, a timeout?

cc @dplewis

The test is not flaky for me. The collection starts empty, so index building should take close to no time.
I couldn't find any tests for addIndex on the server codebase. The JS sdk has several, but none of them seem to explicitly wait on index-building.

I think there is no way to verify index building via Parse, so the only way would be connecting directly to mongodb and checking the status. Or modify Parse adapter to include that. I believe this test will not be flaky but a timeout (maybe 1 or 2s) could be helpful as well.

Ok then let's merge it as it is and see how it goes. I will run the tests a few times here to see whether its flaky already.

Modified an existing test to work with the new sdk release ParseUser.get('password') used to return undefined, now password is not an attribute

mstniy · 2021-08-17T13:09:06Z

@davimacedo The PR now uses the latest SDK release, which includes the INVALID_VALUE error code

mtrezza · 2021-09-03T00:56:56Z

⚠️ Important change for merging PRs from Parse Server 5.0 onwards!

We are planning to release the first beta version of Parse Server 5.0 in October 2021.

If a PR contains a breaking change and is not merged before the beta release of Parse Server 5.0, it cannot be merged until the end of 2022. Instead it has to follow the Deprecation Policy and phase-in breaking changes to be merged during the course of 2022.

One of the most voiced community feedbacks was the demand for predictability in breaking changes to make it easy to upgrade Parse Server. We have made a first step towards this by introducing the Deprecation Policy in February 2021 that assists to phase-in breaking changes, giving developers time to adapt. We will follow-up with the introduction of Release Automation and a branch model that will allow breaking changes only with a new major release, scheduled for the beginning of each calendar year.

We understand that some PRs are a long time in the making and we very much appreciate your contribution. We want to make it easy for PRs that contain a breaking change and were created before the introduction of the Deprecation Policy. These PRs can be merged with a breaking change without being phased-in before the beta release of Parse Server 5.0. We are making this exception because we appreciate that this is a time of transition that requires additional effort from contributors to adapt. We encourage everyone to prepare their PRs until the end of September and account for review time and possible adaptions.

If a PR contains a breaking change and should be merged before the beta release, please mention @parse-community/server-maintenance and we will coordinate with you to merge the PR.

Thanks for your contribution and support during this transition to Parse Server release automation!

mtrezza · 2022-02-12T18:33:35Z

@mstniy Do you think we could get this ready for merge?

davimacedo · 2022-03-31T15:28:35Z

spec/ParseAPI.spec.js

+    testSchema.addIndex('geospatial_index', {
+      geometry: '2dsphere',
+    });
+    await testSchema.save();


I think there is no way to verify index building via Parse, so the only way would be connecting directly to mongodb and checking the status. Or modify Parse adapter to include that. I believe this test will not be flaky but a timeout (maybe 1 or 2s) could be helpful as well.

parse-github-assistant · 2022-03-31T16:36:14Z

Thanks for opening this pull request!

🎉 We are excited about your hands-on contribution!

mtrezza · 2022-03-31T16:47:15Z

@mstniy Could you rebase this on the alpha brach and resolve any conflicts?

Forward Mongo geojson errors as Parse.Error.INVALID_VALUE

7b3493f

Also added a corresponding test

davimacedo requested changes Apr 14, 2021

View reviewed changes

package.json Outdated Show resolved Hide resolved

This was referenced Apr 14, 2021

Error Code for Invalid Geojson #7331

Open

More specific error codes #7346

Closed

mstniy added 2 commits April 22, 2021 11:44

Additional unit test for valid geometry

8c79194

Merge branch 'master' into lean_7331

a9afea6

mtrezza requested changes Apr 22, 2021

View reviewed changes

mstniy added 2 commits August 17, 2021 13:50

Depend on the latest js sdk release for the INVALID_VALUE error code

1804175

Modified an existing test to work with the new sdk release ParseUser.get('password') used to return undefined, now password is not an attribute

Merge remote-tracking branch 'official/master' into lean_7331

ed742aa

Merge remote-tracking branch 'alpha' into HEAD

a9aad2e

mstniy marked this pull request as draft March 8, 2022 08:14

mstniy marked this pull request as ready for review March 8, 2022 08:58

mstniy requested a review from davimacedo March 8, 2022 09:03

Also use INVALID_VALUE for object updates

0301472

mstniy force-pushed the lean_7331 branch from fba80c8 to 0301472 Compare March 8, 2022 09:11

davimacedo approved these changes Mar 31, 2022

View reviewed changes

mtrezza changed the title ~~Forward Mongo geojson errors as Parse.Error.INVALID_VALUE~~ fix: server crashes on MongoDB GeoJSON errors instead of throwing Parse.Error Mar 31, 2022

mtrezza changed the title ~~fix: server crashes on MongoDB GeoJSON errors instead of throwing Parse.Error~~ fix: server crashes on MongoDB GeoJSON error instead of throwing Parse.Error Mar 31, 2022

mtrezza force-pushed the alpha branch from 59215e6 to e6d7d8f Compare May 1, 2022 02:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: server crashes on MongoDB GeoJSON error instead of throwing Parse.Error #7347

fix: server crashes on MongoDB GeoJSON error instead of throwing Parse.Error #7347

mstniy commented Apr 14, 2021

ghost commented Apr 14, 2021

codecov bot commented Apr 14, 2021 •

edited

Loading

davimacedo left a comment •

edited

Loading

mtrezza Apr 22, 2021

mstniy Mar 8, 2022

davimacedo Mar 31, 2022

mtrezza Mar 31, 2022

mstniy commented Aug 17, 2021

mtrezza commented Sep 3, 2021

⚠️ Important change for merging PRs from Parse Server 5.0 onwards!

mtrezza commented Feb 12, 2022

davimacedo Mar 31, 2022

parse-github-assistant bot commented Mar 31, 2022 •

edited

Loading

mtrezza commented Mar 31, 2022

fix: server crashes on MongoDB GeoJSON error instead of throwing Parse.Error #7347

Are you sure you want to change the base?

fix: server crashes on MongoDB GeoJSON error instead of throwing Parse.Error #7347

Conversation

mstniy commented Apr 14, 2021

New Pull Request Checklist

Issue Description

Approach

TODOs before merging

ghost commented Apr 14, 2021

codecov bot commented Apr 14, 2021 • edited Loading

Codecov Report

davimacedo left a comment • edited Loading

Choose a reason for hiding this comment

mtrezza Apr 22, 2021

Choose a reason for hiding this comment

mstniy Mar 8, 2022

Choose a reason for hiding this comment

davimacedo Mar 31, 2022

Choose a reason for hiding this comment

mtrezza Mar 31, 2022

Choose a reason for hiding this comment

mstniy commented Aug 17, 2021

mtrezza commented Sep 3, 2021

⚠️ Important change for merging PRs from Parse Server 5.0 onwards!

mtrezza commented Feb 12, 2022

davimacedo Mar 31, 2022

Choose a reason for hiding this comment

parse-github-assistant bot commented Mar 31, 2022 • edited Loading

Thanks for opening this pull request!

mtrezza commented Mar 31, 2022

codecov bot commented Apr 14, 2021 •

edited

Loading

davimacedo left a comment •

edited

Loading

parse-github-assistant bot commented Mar 31, 2022 •

edited

Loading