Skip to content
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.

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

Setting An element to an array with index = 0 creates an Object #646

Closed
redlock opened this issue Apr 12, 2017 · 9 comments
Closed

Setting An element to an array with index = 0 creates an Object #646

redlock opened this issue Apr 12, 2017 · 9 comments

Comments

@redlock
Copy link
Contributor

redlock commented Apr 12, 2017

Hi,
While I was trying to set a record array with index 0 like so

 record.set(`items[0]`, 51)

deepstream create an object rather than an array as follows:
{ items: { "0" : 51 } }

I traced the error to json-path.js line 40:

} else if (this._tokens[i + 1] && !isNaN(this._tokens[i + 1])) {

in this conditional that checks for an index, if the value of the index in this._tokens[i + 1] is zero it will fail.

A quick fix is to replace the conditional with this:

if (this._tokens[i + 1] !== undefined && !isNaN(this._tokens[i + 1])) {

Hopefully you can patch it up soon. And thanks for your great work with deepstream.

@datasage
Copy link
Contributor

Using arrays with systems like deepstream can get messy. Because clients do not have identical states at any given time, making changes to array elements can cause merge issues when two clients update the same array at the same time in different ways.

Lets say for example client a removes element 5, client b updates element 5. If client b gets processed first, the the update will be removed by client a. If client a wins out, client b will actually update was was element 6.

You can only reliably use arrays if you update the entire array in a single operation.

If you want to add remove element, you are best using an identifier that is sequential based on time, but gives each element a unique id that will not be confused in a merge operation.

The client does have a helper method, getUid(), that you can use to generate the identifier.

@redlock
Copy link
Contributor Author

redlock commented Apr 12, 2017

That is true for your scenario. My use case has one publisher that modifies that array and the rest of the clients are subscribers with no ability to modify. Nonetheless, the issue I mention above is still a bug in the json-path.js file as it doesn't behave as intended.

@yasserf
Copy link
Contributor

yasserf commented Apr 16, 2017

hey @redlock, we will be looking to fix this once easter is over. Thanks for raising!

@honorcode
Copy link

@WolframHempel @yasserf @AlexBHarley
FYI the website link for the Contributor License Agreement is broken, so I couldn't do a PR but below is my deepstream.io/master patch with my FIXes for the server-side issues #651 and #646.

The two file updates are .txt attachments on #651 (json-path.js/txt and json-pathSpec.js/txt deepstream.io/master 2.2.0)

@honorcode
Copy link

FYI the website link for the Contributor License Agreement is broken, so I couldn't do a PR but ^^^ is my deepstream.io/master patches with my FIXes for the server-side issues #651 and #646.

@honorcode
Copy link

@WolframHempel @yasserf @AlexBHarley
Opened related java-client issue found when testing:
deepstreamIO/deepstream.io-client-java#84

@mrby
Copy link
Contributor

mrby commented Aug 3, 2017

when tested as mentioned in the first comment, the record is being set as an array, not an object.

result of testing:
Screen Shot 2017-08-03 at 12.42.12.png

@yasserf
Copy link
Contributor

yasserf commented Aug 3, 2017

Can we also try this out with the java sdk? Looks like its fixed though!

@mrby mrby assigned mrby and unassigned mrby Aug 4, 2017
@yasserf
Copy link
Contributor

yasserf commented May 3, 2020

Closing in favour of #1046

@yasserf yasserf closed this as completed May 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants