Select and Exclude queries do not properly digest JSON arrays

[cbaker6]

New Issue Checklist

• I am not disclosing a vulnerability.
• I am not just asking a question.
• I have searched through existing issues.
• I can reproduce the issue with the latest version of Parse Server.

Issue Description

The keys and excludeKeys on the server are not digesting actual JSON arrays, ["yolo",...], but instead digesting "yolo,..." JS treats both of these the same (I discuss why below), but the server doesn't like it the way the Swift JSON encoder sends an encoded array for these properties, ["yolo",...]. Note that this is the JSON encoder straight from Apples Swift source code and not the custom one used here for ParseObjects, so if it was encoding arrays incorrectly I'm sure it would have been caught awhile ago as the Apple JSON encoder is used by a ton of people.

At first glance this may appear to be a client side bug as there are currently server-side tests and the JS SDK(select and exclude), Flutter SDK (keysToReturn and excludeKeysToReturn), and the Android SDK (selectedKeys) uses it. The iOS SDK uses selectedKeys, but never sends them to the server, instead it strips the keys not selected after the server returns the ParseObject, so I'll ignore this SDK for the rest of the discussion. Below is why I believe the above SDKs work, but also why their current design (which can remain the same) has led to the problem not being discovered until now:

• The JS, Flutter, and Android SDKs don't actually encode JSON arrays for select and exclude. Instead, they all join array elements into a string separated by commas before sending to the server, always sending something like, "yolo,..." which isn't an JSON array. This is why their implementations work with the current Parse Server implementation. Conversely, the Parse-Swift SDK uses Apples native JSON encoder and sends ["yolo",...]. When I make the Parse-Swift use a join array like the aforementioned SDKs, it works fine with the current implementation on the server.
• The changes in the PR doesn't break any of the parse-server test cases which means the JS, Flutter, and Android SDKs will probably not require any changes. If they do end up requiring changes, then it will be a change for the better because they should have originally been sending JSON arrays. Instead, the JS, Flutter, and Android SDKs have essentially adapted to a bug. Example from the REST documentation for sending arrays (notice the "$all":[2,3,4]):

curl -X GET \
  -H "X-Parse-Application-Id: ${APPLICATION_ID}" \
  -H "X-Parse-REST-API-Key: ${REST_API_KEY}" \
  -G \
  --data-urlencode 'where={"arrayKey":{"$all":[2,3,4]}}' \
  https://YOUR.PARSE-SERVER.HERE/parse/classes/RandomObject

• The REST documentation for query constraints highlights the current problem with keys and excludeKeys by stating:

You can restrict the fields returned by passing keys or excludeKeys a comma-separated list.

IMO this deviates from purpose of REST and JSON as it suggests manually parsing/creating lists/strings instead of using underlying libraries that encode/decode JSON based on standards. I’ll create a PR to fix this as well

The linked PR has the possibility of being breaking change for some users due to:

This is the test case I removed because it 1) doesn't work anymore 2) not sure why we want this particular functionality for select. If you really want to exclude all keys, there should probably create an excludeAll or an exclude(["*"] (I have sense added back in this feature in the PR, but my thoughts on this are below)

I didn't attempt to make that old test case work anymore, because I don't think the feature should have been there in the first place, but I'm open to discussion on this.

Approach

Make keys and excludeKeys use the same strategy as a similar property like include which actually digests a JSON string array,

parse-server/src/Routers/ClassesRouter.js

Lines 60 to 68 in b4920a6

	if (body.keys) {
	options.keys = String(body.keys);
	}
	if (body.include) {
	options.include = String(body.include);
	}
	if (body.excludeKeys) {
	options.excludeKeys = String(body.excludeKeys);
	}

Steps to reproduce

1. Make your favorite client SDK take an array (["yolo", ...]) of string for keys or excludeKeys. You can use the links for each SDK above. You can see an example when the issue was first discovered on Parse-Swift, .select() in finds and firsts does not seem to exclude any fields Parse-Swift#86
2. Create a select or excludeKey query
3. Use first or find to query the server

Actual Outcome

All keys will be returned as the server ignores the selected or excludedKeys

Expected Outcome

The server should honor what ever keys you select/exclude in an JSON array

Failing Test Case / Pull Request

• 🤩 I submitted a PR with a fix and a test case.
• 🧐 I submitted a PR with a failing test case. Not possible on the server side because the test cases use the JS SDK and I mentioned above JS doesn’t actually send JSON arrays for keys and excludeKeys, instead it joins the array into a comma delineated string on the client side, essentially removing the fact that it’s an array. To write this test, you would have to make the JS SDK send a real array, have that PR approved, bump JS on the server, etc.

Environment

Server

• Parse Server version: master, 4.5.0
• Operating system: Docker linux, macOS
• Local or remote host (AWS, Azure, Google Cloud, Heroku, Digital Ocean, etc): local

Database

• System (MongoDB or Postgres): mongo, postgres
• Database version: 4.4, 13.2
• Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): local

Client

• SDK (iOS, Android, JavaScript, PHP, Unity, etc): Parse-Swift
• SDK version: 1.1.0

Logs

[cbaker6]

@mtrezza I'm pasting your comment from the PR below to respond to:

I think we have to be careful in removing or changing a test case. I actually just fixed a Parse Server issue for select([]) some weeks ago and I would like to better understand what the issue around this PR is. Is this removing the possibility of select with an empty array? I would like to have this discussion in a separate Parse Server issue.

Can you link to the fix you made? Without seeing the code, I'm assuming it may fall into my comments below (originally from the PR):

2. not sure why we want this particular functionality for select. If you really want to exclude all keys, there should probably create an excludeAll or an exclude(["*"]

The current (before the PR) allowed a dev to pass select([]) and then only receive const AlwaysSelectedKeys = ['objectId', 'createdAt', 'updatedAt', 'ACL']; and exclude all others (this is also what the test case I deleted tested for). What I was trying to say in my comment above feature of select([]) doesn't logically make sense to me and if that feature is needed, it should probably be an excludeAll or an exclude(["*"]) to be consistent with other features on the server. This is my opinion of course based on my interpretation of the server code, so I'm open to hear opposing arguments...

[mtrezza]

Sure, it was #7161:

parse-server/src/RestQuery.js

Lines 103 to 106 in cac6951

	const keys = restOptions.keys
	.split(',')
	.filter(key => key.length > 0)
	.concat(AlwaysSelectedKeys);

select([]) doesn't logically make sense to me

It has a use case if you think about conditionally adding keys to an array:

const keys = [];
if (...) {
  a.push('aKey');
}
query.select(keys);

The alternative is adding additional logic like:

if (keys.length > 0) {
  query.select(keys);
} else {
  query.excludeAll();
}

[cbaker6]

It has a use case if you think about conditionally adding keys to an array:

If I'm understanding your fix correctly, it seems like there may have been a problem because keys = [] was previously allowed? But if it was never allowed it might not have been needed? I ask this because of the check for length > 0

If this is true, it seems like the change I added won't cause any issues with your fix, but I'm guessing

[cbaker6]

I'll note, that if we decide to "keep" the select([]) functionality, I think setting up keys and excludeKeys like hint will work for encoded arrays and strings:

parse-server/src/Routers/ClassesRouter.js

Lines 211 to 213 in 5640cd5

	if (body.hint && (typeof body.hint === 'string' || typeof body.hint === 'object')) {
	options.hint = body.hint;
	}

But, as I mentioned before, I currently don't think that functionality should be in select. With my current suggestion, select([]) simply will send all keys of a ParseObject to the client, just as if select wasn't present

[cbaker6]

@mtrezza thinking about it more, I take back some of my previous comments. I didn't see the PR for your change (so I don't know the original code), but looking at the code snippet you sent, your change is probably why select([]) doesn't work with my fix. If you want that functionality, your alternative:

The alternative is adding additional logic like:

if (keys.length > 0) { query.select(keys); } else { query.excludeAll(); }

will probably bring it back, but I still don't think it should be there

[mtrezza]

Would it be beneficial to change that fix? It actually fixed a change introduced in MongoDB 4.4 - since we are now also CI testing against that version, we should see if anything breaks.

I think if possible we should continue to allow select[] because this is allowed for years and we don't know how many deployments would be affected if we remove this. It would also be a somewhat delicate breaking change because the developer has to adapt the query build logic, not just a parameter name for example.

[cbaker6]

Maybe, can you post the link to your PR that made the change?

What your thoughts on about keeping/removing the functionality in select? Also, if anyone else has thoughts about it.

[cbaker6]

I think if possible we should continue to allow select[] because this is possible since years and we don't know how many deployments would be affected if we remove this. It would also be a somewhat delicate breaking change because the developer has to adapt the query build logic, not just a parameter name for example.

I can understand the reasoning for this... The counter would be then we are continuing to leave something in that shouldn't be there, enabling new devs to use it, increasing the dependence on something that probably shouldn't have been there in the first place

Basically the longer we leave it in, the harder it will be to remove in the future... Though, it's not something serious, so leaving it in may not be that big of a deal, assuming it's easy to add the functionality back to my fix

[cbaker6]

I found the PR where your change occurred #7161. I tested with fix I have with/without it as well as some other combinations with the code block below, but none of them brought back the select([]) functionality`:

• I also tried:

if (body.keys && (typeof body.keys === 'string' || typeof body.keys === 'object')) {
      options.keys = body.keys;
}

So if the select([]) is to be kept, it needs further investigation

[cbaker6]

To keep the functionality, the latest commit on the PR has the changes to allow select([]) to still work:

if (body.keys || body.keys === '') {
      options.keys = String(body.keys);
}

[mtrezza]

continuing to leave something in that shouldn't be there

What shouldn't it? I think I missed the argument why this should be removed, can you explain again?

[cbaker6]

What shouldn't it? I think I missed the argument why this should be removed, can you explain again?

My argument was here:

The current (before the PR) allowed a dev to pass select([]) and then only receive const AlwaysSelectedKeys = ['objectId', 'createdAt', 'updatedAt', 'ACL']; and exclude all others (this is also what the test case I deleted tested for). What I was trying to say in my comment above feature of select([]) doesn't logically make sense to me and if that feature is needed, it should probably be an excludeAll or an exclude(["*"]) to be consistent with other features on the server. This is my opinion of course based on my interpretation of the server code, so I'm open to hear opposing arguments...

If you all think it should remain, it doesn't really bother me as it not something I will end up using. It just seems like it should be an exclude. My reasoning is what happens when you use include([]) is used on a query? (I don't know the answer to this) Does it include only the default Parse Keys?

If you all agree with my logic, but decide to keep it, then what I said earlier may be relevant:

Basically the longer we leave it in, the harder it will be to remove in the future... Though, it's not something serious, so leaving it in may not be that big of a deal, assuming it's easy to add the functionality back to my fix

I updated the PR to work with select([]).

[cbaker6]

For clarity, I want to emphasize there are two topics here:

1. The original PR itself which fixes a problem with how the server decodes/handles arrays of strings for keys and excludeKeys. Basically, for some reason keys and excludeKeys wasn't using the same design pattern as their counterpart include. If there are concerns for/against this, please be sure to mention them
2. Leaving in the functionality of select([]), rather it should remain or not. Most of the discussion has been about this topic, but not the former

[mtrezza]

Let's see...

select([]) doesn't logically make sense to me

To me it makes sense semantically, I read it as "select none". It basically overrides the default behavior where all fields are selected and returned. However, the actual justification lies in examining it as member of logical extrema:

• Select none: select([])
• Select all: select([...allFields])
• Exclude none: exclude([])
• Exclude all: exclude([...allFields])

The select([]) forms an extrema pair with exclude([...allFields]), just as select([...allFields]) and exclude([]) form a pair.

I think we should make sure that all logical extrema yield correct results and make sure there are tests for each.

What never stops to confuse me is that select is the opponent of exclude, but include is something completely different.

[mtrezza]

The original PR itself which fixes a problem with how the server decodes/handles arrays of strings for keys and excludeKeys. Basically, for some reason keys and excludeKeys wasn't using the same design pattern as their counterpart include. If there are concerns for/against this, please be sure to mention them

Thanks for clarifying, that was the part I missed, I think they should be handled equally. If this PR currently contains changes for both issues you mentioned, maybe we can focus on the issue quoted and make sure select([]) still works after this PR. If necessary we can open a separate issue to discuss whether select([]) should be there.

[cbaker6]

If this PR currently contains changes for both issues you mentioned, maybe we can focus on the issue quoted and make sure select([]) still works after this PR. If necessary we can open a separate issue to discuss whether select([]) should be there.

The latest version of the PR only contains just the first and the second (`select([])) can be done as a separate discussion as you mentioned. I've updated the original description to reflect this. The separate discussion is a low priority IMO...

[cbaker6]

I think we should make sure that all logical extrema yield correct results and make sure there are tests for each.

When focusing on the first problem:

The JS, Flutter, and Android SDKs don't actually encode JSON arrays for select and exclude. Instead, they all join array elements into a string separated by commas before sending to the server, always sending something like, "yolo,..." which isn't an JSON array. This is why their implementations work with the current Parse Server implementation. Conversely, the Parse-Swift SDK uses Apples native JSON encoder and sends ["yolo",...]. When I make the Parse-Swift use a join array like the aforementioned SDKs, it works fine with the current implementation on the server.

The test cases are in the PR, excluding what I stated in the description:

🧐 I submitted a PR with a failing test case. Not possible on the server side because the test cases use the JS SDK and I mentioned above JS doesn’t actually send JSON arrays for keys and excludeKeys, instead it joins the array into a comma delineated string on the client side, essentially removing the fact that it’s an array. To write this test, you would have to make the JS SDK send a real array, have that PR approved, bump JS on the server, etc.

To my point above, I also state in the description:

• The changes in the PR doesn't break any of the parse-server test cases which means the JS, Flutter, and Android SDKs will probably not require any changes. If they do end up requiring changes, then it will be a change for the better because they should have originally been sending JSON arrays. Instead, the JS, Flutter, and Android SDKs have essentially adapted to a bug. Example from the REST documentation for sending arrays (notice the "$all":[2,3,4]):

[mtrezza]

they all join array elements into a string separated by commas
(...)
JS doesn’t actually send JSON arrays for keys and excludeKeys, instead it joins the array into a comma delineated string

I see many places in the JS SDK and Parse Server where this is converted back and forth between JSON and comma delimited string. I am not sure why but surely inconvenient. Even the MongoDB client takes the project keys as JSON:

parse-server/src/Adapters/Storage/Mongo/MongoCollection.js

Line 89 in 033a0bd

findOperation = findOperation.project(keys);

This conversion is done with many other parameters as well, looking at toJSON in the JS SDK.

I agree that there could be some clean-up, maybe in steps:

1. Remove the internal handling of comma delimited string within server and JS SDK but keep both accepting it as input to not break the SDKs
2. Remove sending comma delimited string in other SDKs
3. Deprecate the old SDKs
4. Remove accepting comma delimited string as input in server and JS SDK

[cbaker6]

I agree that there could be some clean-up, maybe in steps:

My guess is that when the Parse Server, iOS, Android, and JS SDKs were originally created there "may" have not been native JSON encoders out at the time. I believe this was the case for iOS, so they resorted to manually parsing and creating dictionaries with data.

In the Parse-Swift SDK, a JSON Encoder is used for everything that goes to/from the parse server (we don't parse anything manually in the SDK), so it looks like much of the manual parsing on the server-side is correct when comparing to something based on JSON encode/decode standard, at least in regards to what has been tested in Parse-Swift anyways (I have a couple apps with the SDK, but not running at a large scale yet). The Flutter SDK was probably just following exactly what the other SDKs were doing at the time.

[mtrezza]

I think that makes sense, let's see which noble contributor takes on this clean-up.

For this PR, I think you have nailed it down. Is this good for review yet? Is there a related PR necessary in the JS SDK?

[cbaker6]

Remove sending comma delimited string in other SDKs

In the the future, in the other SDKs may want to even take this a step further and be modified to use native encoding/decoding instead of populating dictionaries. This can also be a case of, "if it isn't broken, don't fix it", but I personally don't trust myself to manually parse something.

For this PR, I think you have nailed it down. Is this good for review yet? Is there a related PR necessary in the JS SDK?

Yup, it's ready

[mtrezza]

use native encoding/decoding instead of populating dictionaries

Absolutely!

[parseplatformorg]

🎉 This change has been released in version 5.0.0-beta.1

[parse-github-assistant]

The label type:feature cannot be used in combination with type:improvement.

[parseplatformorg]

🎉 This change has been released in version 5.0.0