Object attributes transformed in array

aclec commented 6 days ago

New Issue Checklist

[x] I am not disclosing a vulnerability.
[x] I am not just asking a question.
[x] I have searched through existing issues.
[x] 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[bot] commented 6 days ago

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 commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

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 commented 5 days ago

@aclec I have added a PR with a test, see https://github.com/parse-community/parse-server/pull/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 commented 5 days ago

The issue seems to come from https://github.com/parse-community/Parse-SDK-JS/pull/2120. A field value of object { 1: 1 } gets converted to an array [1] here:

https://github.com/parse-community/Parse-SDK-JS/blob/4ce1f2475f2847a456142ec86666d55228d3353b/src/ObjectStateMutations.ts#L188-L198

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:

https://github.com/parse-community/Parse-SDK-JS/blob/4ce1f2475f2847a456142ec86666d55228d3353b/src/__tests__/ObjectStateMutations-test.js#L320

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 https://github.com/parse-community/Parse-SDK-JS/pull/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:

https://github.com/parse-community/Parse-SDK-JS/blob/4ce1f2475f2847a456142ec86666d55228d3353b/src/ObjectStateMutations.ts#L194

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 https://github.com/parse-community/Parse-SDK-JS/pull/2201 where the test with key 1x1 passes.

But the underlying issue seems to be the conflicting representation, see also https://github.com/parse-community/Parse-SDK-JS/pull/2201 where the test with key 1 fails. Not sure how complex the solution is, maybe @dplewis has more insight here?

aclec commented 5 days ago

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 commented 5 days ago

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 commented 3 days ago

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 commented 3 days ago

https://github.com/parse-community/Parse-SDK-JS/pull/2201 treats a symptom of something that is fundamentally broken, so I'm hesitant to merge it. I believe reverting https://github.com/parse-community/Parse-SDK-JS/pull/2120 and https://github.com/parse-community/Parse-SDK-JS/pull/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 commented 3 days ago

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 commented 3 days ago

@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 commented 3 days ago

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 commented 3 days ago

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 commented 3 days ago

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 commented 2 days ago

@mtrezza https://github.com/parse-community/parse-server/pull/9174 will fail for the following scenario: { '1x1': 1, '2': 2, '3': 3 }

I have fixed it slightly differently, please have a look: https://github.com/parse-community/Parse-SDK-JS/pull/2206

mtrezza commented 1 day ago

@mkmandar123 I've closed the server PR https://github.com/parse-community/parse-server/pull/9174, please see https://github.com/parse-community/Parse-SDK-JS/pull/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 https://github.com/parse-community/Parse-SDK-JS/issues/2198#issuecomment-2208635633 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 commented 1 day ago

@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.

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.
Parse community decides to keep this functionality then my PR https://github.com/parse-community/Parse-SDK-JS/pull/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 commented 1 day ago

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 commented 1 day ago

@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 commented 1 day ago

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 commented 1 day ago

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

mtrezza commented 1 day ago

Would that solve the issue? What would that check look like? Maybe you can add a review code suggestion to https://github.com/parse-community/Parse-SDK-JS/pull/2201?

dplewis commented 1 day ago

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 commented 21 hours ago

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 commented 15 hours ago

@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 commented 15 hours ago

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 commented 15 hours ago

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 commented 14 hours ago

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 commented 14 hours ago

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

https://github.com/parse-community/parse-server/pull/9115

mtrezza commented 14 hours ago

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 commented 10 hours ago

@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, https://github.com/parse-community/parse-server/pull/9115 we can remove the check in the SDK.

parse-community / Parse-SDK-JS