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

[cbaker6]

New Pull Request Checklist

• I am not disclosing a vulnerability.
• I am creating this PR in reference to Mongo explain results incorrectly returning an object instead of an array of objects #7442.

Issue Description

explain for mongo is currently returning an object when returning results. This conflicts with trying to decode explain results because it should return an array of objects when using find instead of a single object.

This error was probably not seen before because JS and some other languages are "not strongly typed," meaning the language doesn't care if a single object is returned instead of an array of objects. This is not the case with the Swift SDK.

This also requires a fix to the JS SDK for proper support along with any other SDK that adapted to the bug.

This will be a breaking change as devs who use mongo probably programmed to the bug mentioned above. Close #7442

Originally discussed on community.parseplatform.org

Related issue: #7442

Approach

Make find in the MongoStorageAdapter return an array of objects to conform to the way it's suppose to return results.

TODOs before merging

• Fix current test cases that were adapted to bug
• Add entry to changelog

[codecov]

Codecov Report

Merging #7440 (7a40fa0) into alpha (45d29cc) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            alpha    #7440      +/-   ##
==========================================
- Coverage   93.93%   93.90%   -0.03%     
==========================================
  Files         181      181              
  Lines       13267    13267              
==========================================
- Hits        12462    12459       -3     
- Misses        805      808       +3     

Impacted Files	Coverage Δ
src/Adapters/Storage/Mongo/MongoStorageAdapter.js	92.37% <100.00%> (ø)
src/Adapters/Files/GridFSBucketAdapter.js	79.50% <0.00%> (-0.82%)	⬇️
src/RestWrite.js	93.76% <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 45d29cc...7a40fa0. Read the comment docs.

[cbaker6]

I'll submit the JS fix later today as that will be needed for the test cases. Essentially, an explainable first is needed.

[davimacedo]

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

[mtrezza]

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.

[cbaker6]

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.

[cbaker6]

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

[cbaker6]

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.

[cbaker6]

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.

[cbaker6]

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

[mtrezza]

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?

[cbaker6]

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.

[mtrezza]

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.

[mtrezza]

⚠️ 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]

Concerted to draft:

• If this means a breaking change, we may as well break for a more sustainable solution, see fix: mongo explain queries should always return as an array of results #7440 (comment).
• We don't introduce breaking changes this way anymore, we'd phase them in if possible.

[parse-github-assistant]

Thanks for opening this pull request!

• 🎉 We are excited about your hands-on contribution!