Object attributes transformed in array

[aclec]

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 versions of Parse Server and the Parse JS SDK.

Issue Description

When I get an Object attribute on my object with Parse 5.2, it returns an array instead of the expected object.
(Parse 5.1 return an Object)

Steps to reproduce

Get an Object attribute with parse 5.2

Server

• Parse Server version: 7.1

Database

• System (MongoDB or Postgres): MongoDB
• Database version: 7
• Local or remote host (MongoDB Atlas, mLab, AWS, Azure, Google Cloud, etc): Local

Client

• Parse JS SDK version: 5.2

Logs

Parse 5.1:

Parse 5.2:

[parse-github-assistant]

Thanks for opening this issue!

• 🚀 You can help us to fix this issue faster by opening a pull request with a failing test. See our Contribution Guide for how to make a pull request, or read our New Contributor's Guide if this is your first time contributing.

[mtrezza]

The server version did not change, only the JS SDK version? Could you open a PR with a failing test that demonstrates the issue?

[aclec]

Yes, the server is in 7.1.
The two screen was taken by changing only the sdk from 5.1 to 5.2
When I downgrade to 5.1, it reworks.

[mtrezza]

Could you add a test so we can verify the issue? And maybe share a simple example code here to understand how exactly you are getting to this result.

[mtrezza]

@aclec I have added a PR with a test, see parse-community/parse-server#9174. It's using the Parse JS SDK 5.2.0. I cannot get it to fail. Could you please take a look and share how we can reproduce this issue?

Edit: I was able to reproduce the issue, updated the test. The issue occurs when the field key is a number string. Could you please try this out with lower versions of the JS SDK and let us know which version introduced the issue? Please also try try the alpha versions.

[mtrezza]

The issue seems to come from #2120. A field value of object { 1: 1 } gets converted to an array [1] here:

Parse-SDK-JS/src/ObjectStateMutations.ts

Lines 188 to 198 in 4ce1f24

	// Check for JSON array { '0': { something }, '1': { something } }
	if (
	val &&
	typeof val === 'object' &&
	!Array.isArray(val) &&
	Object.keys(val).length > 0 &&
	Object.keys(val).some(k => !isNaN(parseInt(k))) &&
	!['sentPerUTCOffset', 'failedPerUTCOffset'].includes(attr)
	) {
	val = Object.values(val);
	}

I think there are 2 issues:

• If a key is a number, it is interpreted as an array index. But in JS a number string is a valid key name. So this representation may need to be changed because it's conflicting with valid JS object key names.
• There is a bug in the logic above where a key name containing a mix of numbers and letters gets converted to a number by parseInt despite containing letters. That leads to @aclec's object key 120x120 being converted to number 120 and the field value gets converted to an array, which is the reported issue.

I suspect a broader issue with how nested fields are represented:

// Check for JSON array { '0': { something }, '1': { something } }

also in:

Parse-SDK-JS/src/__tests__/ObjectStateMutations-test.js

Line 320 in 4ce1f24

items: { '0': { count: 20 }, '1': { count: 5 } },

A nested field should be represented with dots. If instead it's represented by an object where a number key represents an array index, it conflicts with objects that contain number keys. I believe the fix in #2194 does not really address the underlying issue. It excludes sentPerUTCOffset: { '1': { count: 20 } }, from the conversion based on the key name, but a fix should actually allow a key name to be a number.

A quick fix for @aclec's reported issue would be to change this check:

Parse-SDK-JS/src/ObjectStateMutations.ts

Line 194 in 4ce1f24

Object.keys(val).some(k => !isNaN(parseInt(k))) &&

to:

Object.keys(val).some(k => k === String(Number(k)) && Number.isInteger(Number(k)))

This avoids that the key 120x120 is converted to a number and then interpreted as an array index, but instead it's correctly identified as NaN. See #2201 where the test with key 1x1 passes.

But the underlying issue seems to be the conflicting representation, see also #2201 where the test with key 1 fails. Not sure how complex the solution is, maybe @dplewis has more insight here?

[aclec]

I see that .some() is being used. Shouldn't we, in addition to checking that it's a number, use .every() to ensure that all keys are numbers?
It might be a misunderstanding on my part, but it seems to me that the transformation occurs as soon as there is a number?

[mtrezza]

I think you could be right. But I also think this whole logic should be removed, because an array cannot be represented by key names that are numbers. Number strings are valid key names in JS and MongoDB.

Worst case we may have to remove the dot notation feature from the JS SDK.

[aclec]

Can you merge your pull request so that I can update my SDK to the next version?
Removing all the logic would be a breaking change for some people, I think.

We need to use dot notation to access nested values, but if it's not an array, we should not transform it. An object must stay an object.

The bug did not exist before. Maybe we should revert the changes?
Or is it better to delete the logic?

[mtrezza]

#2201 treats a symptom of something that is fundamentally broken, so I'm hesitant to merge it. I believe reverting #2120 and #2194 may be the correct way forward, since it seems to be based on an incorrect implementation. That would be a breaking change for people who use the feature, but the feature was adding a bug for other people, so the trade off I believe would be justified to merge it as a simple bug fix. However, the best way forward would be to fix the underlying bug.

[aclec]

I will try to create a patch today to pass the tests.

The goal is not to convert an object into an array; the object should remain as is until the end.

I will try today to see if we can reconcile everything.

[dplewis]

@mtrezza The fix for this is to improve the JSON array check. A JSON array is a json with keys index 0-n like a normal array anything else should be ignored. I’ll do a separate PR with test.

[mtrezza]

A JSON array is a json with keys index 0-n

I'm not familiar with the term "JSON array", what is that? An array in JSON syntax is using square brackets, like

["a","b"]

If such an array would be represented in JSON as

{"0":"a","1":"b"}

then how would it be differentiated from an object

const obj = {"0":"a","1":"b"};

[dplewis]

You access the values the same way, this is basically how dot notation supports both json and arrays mixed internally in MongoDB. Array.isArray is how you would differentiate the two

const arr = ['a', 'b'];
const obj = { 0: 'a', 1: 'b' };

arr[0] == obj[0]; // 'a'
Object.keys(arr) == Object.keys(obj) // [0, 1]

const arr = [{0: 'a'}, 'b'];
const obj = { 0: {0: 'a'}, 1: 'b' };

arr[0][0] == obj[0][0]; // 'a'

const notJSONArray = {1: 'b'}; // missing an index

I'm not familiar with the term "JSON array", what is that?

There is JSON Objects so I call this JSON Array. Not sure what the official term is for this.

[mtrezza]

I think I'm getting it. I understand it converts the array to an object representation so that the array becomes accessible like an object. But this conversion from array to object seems to be irreversible. Because from the output of the conversion you cannot infer anymore whether this is a representation of an array or just a normal object.

For example, how can you infer from

const obj = { 0: {0: 'a'}, 1: 'b' };

that it is a representation of

const arr = [{0: 'a'}, 'b'];

The obj may well be an object as is, and not a representation of an array. The logic tries to infer that, and fails when it incorrectly interprets a number key as an array index and incorrectly converts the object to an array.

Or the other way around:

const notJSONArray = {1: 'b'};

You say it's not array because it's missing index 0. But this is also not an array, just a normal JS object:

const notJSONArray = {0: 'b'};

Then again, JS allows for an array to be missing indexes in between, so only having an index 1 can also be a valid array.

Maybe I'm still missing something, so I'm curious to see your PR.

[mkmandar123]

@mtrezza parse-community/parse-server#9174 will fail for the following scenario:
{ '1x1': 1, '2': 2, '3': 3 }

I have fixed it slightly differently, please have a look:
#2206

[mtrezza]

@mkmandar123 I've closed the server PR parse-community/parse-server#9174, please see #2201 for a possible fix, which however doesn't fix the underlying issue, hence the CI fails for {'1': 1}.

I'm not sure about your PR, see #2198 (comment) for why that may convert objects to arrays that shouldn't be converted. I believe your fix would fail for example for { '0': 0 }.

[mkmandar123]

@mtrezza I am not sure why there is a requirement of converting a jsonArray to array because data format should not change while saving any data. I believe data should remain in same format it is getting saved while retrieving the data. But if the functionality is added there would be some solid reason for having this functionality. Now I see following two outcomes.

1. Parse community decides not to support this conversion as it can lead to incompatible data type in before saving a object and getting the same value out of the object after a save. In this case then the entire if statement is not needed.
2. Parse community decides to keep this functionality then my PR fix: Objects wrongly getting converted to Array #2206 covers following cases.
   1. All array index numbers should be present, if any sequence is missing then it is not considered as jsonArray.
   2. If the json has keys which are number and non-number texts then this is not considered as jsonArray.
   3. If all the json is passing the conditions to consider it as a jsonArray then the order of the json keys is not considered.

Note: Even my test case will fail for the case { '0': 0 } because it is not possible to identify if data needs to be fetched as jsonArray or jsonObject

[mtrezza]

Even my test case will fail for the case { '0': 0 } because it is not possible to identify if data needs to be fetched as jsonArray or jsonObject

Thanks for confirming this. That's exactly the point. How can we solve this? Can we get rid of this strange conversion?

[dplewis]

@mtrezza The reason for this conversion is because using Dot notation on array fields messes up the internal state of objects. If we keep @mkmandar123 fix and accept the fact that { '0': 'foo' } converts to ['foo'] (which it should since we are working with array fields in this case) do you see any other issue that would require us to remove this?

JS allows for an array to be missing indexes in between

I don't think so, can you prove it?

> Array.from(['a',,'c'].keys())
[ 0, 1, 2 ]

[mtrezza]

accept the fact that { '0': 'foo' } converts to ['foo'] (which it should since we are working with array fields in this case)

You mean when saving { '0': 'foo' } as the property of a Parse Object it will be retrieved as ['foo']? Why should it?

[dplewis]

@mtrezza We can add a dot notation check too since thats the point and cause for the need of conversion.

[mtrezza]

Would that solve the issue? What would that check look like? Maybe you can add a review code suggestion to #2201?

[dplewis]

A dot notation on array looks like items.0.count just check for a dot and an index. This will fix this issue.

The result from the server would return something like

{ items: { '0': { count: 20 } } }

[mtrezza]

Could you make a code suggestion in the PR? I'm not sure what this fix could look like. I have also added more cases to the test in the PR; I haven't found a solution that works for all cases.

[dplewis]

@mtrezza Can you make dot notation on arrays experimental? I realized that this will need some configuration on the server side. I don't want to completely remove this feature.

CoreManager.set('ENABLE_DOT_NOTATION_ARRAY', true)

[mtrezza]

We currently do not have such a thing as experimental features (there's a long explanation somewhere why that is). We can either remove the feature as a whole or keep it with a bug until that is fixed. The bug is significant I believe, because it affects existing deployments even if the feature is not used, so maybe keeping it "as is" is not a good option.

Maybe if we could isolate the bug to affect only code that is using the dot notation, then we could keep it as a bug without removing the whole feature. If we assume that this will reduce the bug's impact.

Even if we made the feature experimental, it would be a breaking change for anyone using it already. So we may as well remove it altogether. Plus the larger problem with the current implementation is that it affects other features (fetching normal objects), which can introduce bugs in developer apps that are difficult to analyze.

[dplewis]

Let's address the issue in the original post using @mkmandar123 fix first.

As for the issue with { '0': 'foo' } we can address it if it comes up. Storing JSON.stringify(array) to an Object field instead of an Array field is something I don't think developers are doing. Or remove it. I'll let you decide.

[mtrezza]

Even with the fixes that we considered so far, it means that objects with numbers as keys (starting with key 0) are returned incorrectly as arrays instead of objects.

If we keep the feature, the bug may affect a large number of deployments, as a JS object with numbers as keys doesn't sound out of the ordinary. It's also a really nasty bug to track down because the object is correctly stored in the DB, but wrong in memory when fetched.

If we remove the feature we will break deployments that already use dot notation, but the feature was only released last week, so that may only be a handful.

I believe removing the feature is best choice with the least impact for developers.

[dplewis]

Dot notation on arrays aren't supported on the server yet. Was waiting on SDK release

parse-community/parse-server#9115

[mtrezza]

The feature isn't working yet at all? That would be even better, so removing it from the JS SDK wouldn't affect anyone. That makes the choice even easier.

But that also means once we add that feature, we need to add a compatibility table to the JS SDK, because this feature will require a min. version of Parse Server.

[dplewis]

@mtrezza We can remove the JSONArray check. This is an issue on the server side. The server converts an array to an object and sends it client side.

Edit: Fixed the server side issue, parse-community/parse-server#9115 we can remove the check in the SDK.

[mtrezza]

@dplewis Could you please take a look at the removal compared to the feature addition. I've removed the JSON array check, but there are other changes I'm not sure whether to keep or not.

[parseplatformorg]

🎉 This change has been released in version 5.3.0-alpha.2

[parseplatformorg]

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

[parseplatformorg]

🎉 This change has been released in version 5.3.0