Skip to content
This repository has been archived by the owner on Apr 6, 2021. It is now read-only.

Setting jsonpath values when jsonpath contains numeric value does set/sync properly client-side but writes proper value server-side. #84

Closed
honorcode opened this issue Apr 18, 2017 · 8 comments

Comments

@honorcode
Copy link

honorcode commented Apr 18, 2017

Dev Note: this was discovered while testing with the java-client the two following and related deepstream.io server (master/v2.2.0) issues:

  1. Setting An element to an array with index = 0 creates an Object
    Setting An element to an array with index = 0 creates an Object deepstream.io#646
  2. Record Set JsonPath with numeric parts that are not array indices returned different results on client vs server side
    Record Set JsonPath with numeric parts that are not array indices returned different results on client vs server side deepstream.io#651

I have submitted FIXes for the above server-side json-path resolver issues. See comments and attachments in issue#651 deepstreamIO/deepstream.io#651.

Description of this java-client issue:

There is still a corner case with Java client (v2.0.8) see below (w redis cache plugin configured, all components running localhost/Mac osx, all latest versions):
e.g.

JsonObject newTixTextObj = new JsonObject();
newTixTextObj.addProperty("1","tt-1");
newTixTextObj.addProperty("2","tt-2");
newTixTextObj.addProperty("3","tt-3");
newTixTextObj.addProperty("4","tt-4");
newTixTextObj.addProperty("5","tt-5");
newTixTextObj.addProperty("6","tt-6");
                
Record rec_setPathNum = dsClient.record.getRecord("test/rec_setPathNum");
rec_setPathNum.set("a.b.4", newTixTextObj);
rec_setPathNum.set("aaa[1].333.bbb[0].222", newTixTextObj);
rec_setPathNum.set("a.b.0.2.xxx[4].a", newTixTextObj);
//rec_setPathNum.set("aaa[0].b.1.ccc[1].1", newTixTextObj);
JsonElement val = rec_setPathNum.get("aaa[0]");
rec_setPathNum.set("aaa[0].b", newTixTextObj);
val = rec_setPathNum.get("aaa[0]");

JsonElement rec_setPathNum_elem = rec_setPathNum.get();
System.out.println(">> rec_setPathNum.get() = " + rec_setPathNum_elem.toString());
rec_setPathNum.discard();

SnapshotResult rec_setPathNum_snap = dsClient.record.snapshot("test/rec_setPathNum");
System.out.println(">> rec_setPathNum SNAP = " + rec_setPathNum_snap.getData().toString());

rec_setPathNum = dsClient.record.getRecord("test/rec_setPathNum");
System.out.println(">> rec_setPathNum = " + rec_setPathNum_elem.toString());

rec_setPathNum.set("aaa[0].b") does not apply update on client-side BUT does update server-side with no errs logged.

I’ve tested and VERIFIED that the JAVA client corner case I sighted above ( rec_setPathNum.set(“aaa[0].b”, newTixTextObj); ) IS AN ISSUE with the JAVA-client somewhere. I haven’t traced the java-client side issue yet, but I did verify that the back-end/server/redis value is written correctly AND the above JAVA client SNAPSHOT call returns the correct data as well, but the java record that the “.set” is called on above does NOT reflect the correct updated server-side value. The client-side and server-side are NOT SYNC’D anymore in this use case.

@honorcode
Copy link
Author

honorcode commented Apr 18, 2017

TO BE CLEAR: This java-client ISSUE was observed, testing local server build, AFTER applying and rebuilding the server json-path.js fix attached (as .txt) in deepstreamIO/deepstream.io#651.

@honorcode
Copy link
Author

honorcode commented Apr 18, 2017

Tested again with deepstream server 2.2.0 master and java client 2.0.8 master and the result is not sync'd either. The client and server side values are different, NOT sync'd. The resulting values are different than the values after applying the above mentioned server-side patch fix found in deepstreamIO/deepstream.io#651 but the client and server values are still NOT the same on the client-side as on the server-side/redis-side.

@honorcode
Copy link
Author

honorcode commented Apr 19, 2017

** NOTICE **
UtilJsonPath.java source FIX (see attachments below) and tests are meant to be integrated with the Server-Side FIXes attached to deepstream.io issue deepstreamIO/deepstream.io#651
(which also resolves deepstream.io deepstreamIO/deepstream.io#646)

Cause of issue identified on the java-client side here was a missing condition check for iterating an array index item which was null and not instantiating an new object to build along the path. This fix is for a specific edge case resolution.

Summary: UtilJsonPath:setIterateThrough(...) : the fix is for array index items in path that are null -> add new object

            if (parentObject.get( prefix ).getAsJsonArray().get( index ).isJsonNull()) {
                parentObject.get( prefix ).getAsJsonArray().set( index, new JsonObject() );
            }

FYI the website link for the Contributor License Agreement is broken, so I couldn't do a PR but below is my deepstream.io-client-java/master patch with my FIXes (.txt -> .java source) for this client=side issue related to these server-side issues #651 and #646.

Source and New Tests:
UtilJSONPath.txt
JSONPathTest.txt

Coupled with the Server-Side FIXes attached to the issues #651 and #646 mentioned these together allow for a couple edge case bugfixes and more robust complex jsonpath support for array and (NEW) numeric object member name support.

Much of our data is streamed, deeply nested json objs, with numerous objects members having numeric fieldnames. These are automated transforms in another ETL application, not possible to refactor easily.The numeric fieldnames also help significantly as the number of records scales due to memory usage for each fieldname's length for every obj. Smaller fieldnames = smaller objects = smaller data stores = less bandwidth/memory/storage. (AWS == $$$$). We also don't want the added padding of nulls in sparse arrays of slotted values. And, of course, arrays are much harder to sync for.

@yasserf
Copy link
Contributor

yasserf commented Apr 19, 2017

Hey, thanks for raising and fixing. Can you raise a Pr? We'll solve cla issue on our side. Thanks!

@honorcode
Copy link
Author

@yasserf, How do I raise a PR without a signed agreement?
I tried to push my local branch with commits and got a permissions error.
So how can I select a branch to make a PR off of?
Forgive my ignorance here if I'm missing something obvious.
I believe @AlexBHarley might have already done this.

@honorcode
Copy link
Author

git -c diff.mnemonicprefix=false -c core.quotepath=false -c credential.helper=sourcetree push -v --tags --set-upstream origin refs/heads/honorcode/fix-jsonpath-arrays-empty-slot-and-numeric-obj-member-names:refs/heads/honorcode/fix-jsonpath-arrays-empty-slot-and-numeric-obj-member-names 
Pushing to https://github.com/deepstreamIO/deepstream.io-client-java.git
remote: Permission to deepstreamIO/deepstream.io-client-java.git denied to honorcode.
fatal: unable to access 'https://github.com/deepstreamIO/deepstream.io-client-java.git/': The requested URL returned error: 403
Completed with errors, see above

@honorcode
Copy link
Author

Yes, @AlexBHarley already did this. :-)

@yasserf
Copy link
Contributor

yasserf commented Apr 19, 2017

You'll need to create a fork, we dont add outside collaborators to the org due to github seats. Signing the CLA only means you don't hold the IP to the fix made.

Awesome, thanks @alex!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants