Skip to content

fix: mongo explain queries should always return as an array of results #7440

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

Closed
wants to merge 7 commits into from
+8 −7
Closed

fix: mongo explain queries should always return as an array of results #7440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
865e323
failing testcases
cbaker6 Jun 20, 2021
74cb902
fix explain
cbaker6 Jun 20, 2021
7710f64
pass without modification to JS SDK
cbaker6 Jun 20, 2021
65941a1
add changelog entry
cbaker6 Jun 20, 2021
86f6863
Revert comment changes
cbaker6 Jun 22, 2021
5721b03
Merge branch 'master' into explain
cbaker6 Jul 12, 2021
7a40fa0
Merge branch 'master' into explain
cbaker6 Jul 28, 2021
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
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ ___
- Remove support for MongoDB 3.6 which has reached its End-of-Life date and PostgreSQL 10 (Manuel Trezza) [#7315](https://github.com/parse-community/parse-server/pull/7315)
- Remove support for Node 10 which has reached its End-of-Life date (Manuel Trezza) [#7314](https://github.com/parse-community/parse-server/pull/7314)
- Remove S3 Files Adapter from Parse Server, instead install separately as `@parse/s3-files-adapter` (Manuel Trezza) [#7324](https://github.com/parse-community/parse-server/pull/7324)
- Fix explain for mongoDB which was returning a single object for query results instead of an array of objects. This conflicts with the rest of the find query server responses as find is always suppose to return an array (Corey Baker) [#7440](https://github.com/parse-community/parse-server/pull/7440)
### Notable Changes
- Added Parse Server Security Check to report weak security settings (Manuel Trezza, dblythy) [#7247](https://github.com/parse-community/parse-server/issues/7247)
- EXPERIMENTAL: Added new page router with placeholder rendering and localization of custom and feature pages such as password reset and email verification (Manuel Trezza) [#7128](https://github.com/parse-community/parse-server/pull/7128)
Expand Down
4 changes: 2 additions & 2 deletions spec/MongoStorageAdapter.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,8 +338,8 @@ describe_only_db('mongo')('MongoStorageAdapter', () => {
{ username: 'bugs' },
{ caseInsensitive: true, explain: true }
);
expect(preIndexPlan.executionStats.executionStages.stage).toBe('COLLSCAN');
expect(postIndexPlan.executionStats.executionStages.stage).toBe('FETCH');
expect(preIndexPlan[0].executionStats.executionStages.stage).toBe('COLLSCAN');
expect(postIndexPlan[0].executionStats.executionStages.stage).toBe('FETCH');
});

it('should delete field without index', async () => {
Expand Down
4 changes: 2 additions & 2 deletions spec/ParseQuery.hint.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ describe_only_db('mongo')('Parse.Query hint', () => {
});
let response = await request(options);
let explain = response.data.results;
expect(explain.queryPlanner.winningPlan.inputStage.stage).toBe('COLLSCAN');
expect(explain[0].queryPlanner.winningPlan.inputStage.stage).toBe('COLLSCAN');

options = Object.assign({}, masterKeyOptions, {
url: Parse.serverURL + '/classes/TestObject',
Expand All @@ -164,7 +164,7 @@ describe_only_db('mongo')('Parse.Query hint', () => {
});
response = await request(options);
explain = response.data.results;
expect(explain.queryPlanner.winningPlan.inputStage.inputStage.indexName).toBe('_id_');
expect(explain[0].queryPlanner.winningPlan.inputStage.inputStage.indexName).toBe('_id_');
});

it_only_mongodb_version('<4.4')('query aggregate with hint (rest)', async () => {
Expand Down
4 changes: 2 additions & 2 deletions spec/ParseQuery.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5214,8 +5214,8 @@ describe('Parse.Query testing', () => {
const query = new Parse.Query('_User');
query.equalTo('objectId', user.id);
query.explain();
const result = await query.find();
const results = await query.find();
// Validate
expect(result.executionStats).not.toBeUndefined();
expect(results[0].executionStats).not.toBeUndefined();
});
});
2 changes: 1 addition & 1 deletion src/Adapters/Storage/Mongo/MongoStorageAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,7 @@ export class MongoStorageAdapter implements StorageAdapter {
)
.then(objects => {
if (explain) {
return objects;
return [objects];
Copy link
Member

Choose a reason for hiding this comment

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

Just to understand, will this array ever have more than one item?

Copy link
Member

@mtrezza mtrezza Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I will let @cbaker6 answer this, but to throw in my observation:

Although MongoDB has changed the structure of the explain output in the past, they did not change the root which is a dictionary with key queryPlanner. Also see https://docs.mongodb.com/manual/reference/explain-results/

The more likely reason this result array could contain more than 1 item is if we have a use case in Parse Server in the future where we may want to return more than 1 item.

I tend to think that this is a symptom of a conceptual flaw in our explain implementation. The underlying issue is that explain does not return the same type as find. That means that an explain operation should not be executed with find and explain as an option. It should be a distinct command which returns a distinct type, which is object.

Copy link
Contributor Author

@cbaker6 cbaker6 Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Just to understand, will this array ever have more than one item?

@davimacedo Not to my knowledge, but I think it can be possible in the future. @mtrezza’s explanation works here:

I tend to think that this is a symptom of a conceptual flaw in our explain implementation. The underlying issue is that explain does not return the same type as find.

This causes an issue for strongly typed languages because find on the parse server is expected to always return an array, not a single object. Currently, there is no way to run explain in the Swift SDK when using Mongo because of this flaw. A dev will have to run an afterFind trigger in Cloud Code and modify the results for it work. This isn’t the case for the Postgres implementation because it follows the rules of find.

That means that an explain operation should not be executed with find and explain as an option. It should be a distinct command which returns a distinct type, which is object.

@mtrezza comment about is a possible solution, but this will cause for additional complicated code on the server and Client SDKs (currently JS and Swift, none of the others seem to have this implemented). I recommend just making Mongo explain follow the rules of find (this PR) so explain can be used as an option in a find/first query.

Copy link
Contributor Author

@cbaker6 cbaker6 Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Also see https://docs.mongodb.com/manual/reference/explain-results/ The more likely reason this result array could contain more than 1 item is if we have a use case in Parse Server in the future where we may want to return more than 1 item.

@mtrezza’s comment above also is reasonable. I didn’t include it because it’s separate from the Mongo Adapter not following the current rules of find, but is still a plus when moving in direction of accepting this PR

Copy link
Contributor Author

@cbaker6 cbaker6 Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I’ll note, a few of the fixes I’ve added to the server have been due to:

  1. Bringing the SDK (Swift) that uses a strongly typed language up to parity with the server features.
  2. Using a strict JSON encoder/decoder (ParseEncoder/JSONDecoder in the Swift SDK)

Since the JS SDK seems to be the only other client SDK that seems to be at feature parity (the Flutter SDK has more features than the others but doesn’t seem to have adequate testcases from what I can find, so I don’t count it), it causes some mistakes because both JS don’t rely on the 2 items I listed above.

To be fair, I think all of the features added without testing against the 2 items above have been implemented well. There’s just some minor bugs in which I submitted PRs for, this Mongo explain PR being one of them.

Copy link
Contributor Author

@cbaker6 cbaker6 Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The "bug fix" is a breaking change. Moving to return an object is another breaking change. So why not break only 1 time in a way that is sustainable?

The bug fix will only break the explain portion of SDKs that use explain (I don't believe there are many, maybe just JS from what I can find), Swift will work. The feature add of 7637 will break query's in general across all SDKs. I don't place these types of breaks in the same category. Plus, one is finished while the other seems to be in a conceptual phase from what I can see.

Also, if I understand correctly, this is currently a "bug" only from the standpoint of the Swift SDK, not for any other SDK, is that correct?

I think you are asking if the Swift SDK the only SDK that doesn't conform to a known bug. The answer here is, "yes" I don't program anything to conform to bugs I know about. I consider bugs to be things coded on accidents or unintentional, not coded on purpose.

Copy link
Contributor Author

@cbaker6 cbaker6 Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, mongo's explain returns an object, Postgres returns an array of objects. Even in 7637, you are going to have to make a design decision with Mongo because of strongly typed languages; explain: [object] or explain: object. If it's the former, then it's an adjustment of this PR, which makes it closer to an incremental change. If you modify Postgres, you will have to pull the first item out of the array to make the explain property work as explain: object. IMO, an array of objects gives you the most flexibility...

Postgres reference (Look in Example section, FORMAT JSON): https://www.postgresql.org/docs/current/sql-explain.html

Copy link
Member

@mtrezza mtrezza Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A breaking change won't be introduced until 2023. If we change to retuning an object instead of an array in 2022, any other "intermediary" effort (i.e. returning an array for explain) will be practically unreleased.

To evaluate any intermediary steps, we would have to be know where we ultimately want to go. I'm starting to repeat myself, but in my opinion, the sustainable way forward is to return something like:

{
  results: [...],
  meta: {
    cursor: ...,
    explain: ...
  }
}

Because how else would we return results and meta data? Do we agree on that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The way you have the meta data looks good, just need to decide if cursor and explain will be an array of objects or an object.

Copy link
Member

@mtrezza mtrezza Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

They will be objects for the same reason as we want to return an object on the root level: We cannot know how the results of explain or cursor may change in the future. Once we use an array, we are confining the subsequent structure to be items in a list. We only want to do that if we have a strong reason to assume that we will always expect a list of items, such as results, which will likely always be a list of objects.

Object and array can of course be used interchangeably, but if we use an array, we'd have to remember what information we expect at which index. And the problems begin when a response is supposed to not contain any value on an intermediary index. In that sense, we want to push the use of arrays as far out as possible and necessary in the response structure.

}
return objects.map(object => mongoObjectToParseObject(className, object, schema));
})
Expand Down