Remove Dependency on org.json:json Artifact (#113), Java 11 build problems (#243)

[matejsp]

fixing #113

[hiranya911]

Thanks @matejsp for putting this together. Code looks pretty good. I only had some minor comments. Make sure to also update the CHANGELOG file.

[hiranya911]

@schmidt-sebastian can you also take a look from the RTDB perspective, and see if you find any problems.

[matejsp]

I have updated changelog, and resolved two issues that you pointed out.
And commented on last issue.

[hiranya911]

This LGTM for me. I will run the integration test suite soon to make sure everything's good there as well.

[hiranya911]

@matejsp this is failing an integration test:

Failed tests: QueryTestIT.testEndAtWithTwoArgumentsAndLimit:1828 Values different. Expected: {b=b, c=c, d=d, e=e, f=f} Actual: {d=d, e=e, f=f} 

[hiranya911]

RTDB is responding differently when different json libraries are used.

Existing behavior (org.json):

[firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data: {t=d, d={a=s, r=0, b={c={sdk.admin_java.6-7-1-SNAPSHOT=1}}}} [firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data (contents hidden) [firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data: {t=d, d={a=p, r=2, b={p=-LVyO5qTnbP6ihEsnzCr, d={a=a, b=b, c=c, d=d, e=e, f=f, g=g, h=h}}}} [firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data: {t=d, d={a=q, r=3, b={p=-LVyO5qTnbP6ihEsnzCr, q={vf=r, en=f, ep=null, l=5}, t=1, h=}}} [firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data: {t=d, d={a=n, r=4, b={p=-LVyO5qTnbP6ihEsnzCr, q={vf=r, en=f, ep=null, l=5}, t=1}}} [firebase-websocket-worker] DEBUG com.google.firebase.database.connection.WebsocketConnection - [ws_0] WS message: {"t":"c","d":{"t":"h","d":{"ts":1547234996349,"v":"5","h":"s-usc1c-nss-235.firebaseio.com","s":"lBzQdRCSRFKgGwGChqvtxvxWEQkUv8cO"}}} [firebase-websocket-worker] DEBUG com.google.firebase.database.connection.WebsocketConnection - [ws_0] WS message: {"t":"d","d":{"r":0,"b":{"s":"ok","d":""}}} [firebase-websocket-worker] DEBUG com.google.firebase.database.connection.WebsocketConnection - [ws_0] WS message: {"t":"d","d":{"r":1,"b":{"s":"ok","d":{"auth":null,"expires":1547238596}}}} [firebase-websocket-worker] DEBUG com.google.firebase.database.connection.WebsocketConnection - [ws_0] WS message: {"t":"d","d":{"r":2,"b":{"s":"ok","d":""}}} [firebase-websocket-worker] DEBUG com.google.firebase.database.connection.WebsocketConnection - [ws_0] WS message: {"t":"d","d":{"b":{"p":"-LVyO5qTnbP6ihEsnzCr","d":{"b":"b","c":"c","d":"d","e":"e","f":"f"},"t":1},"a":"d"}} 

Modified behavior (gson)

[firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data: {t=d, d={a=s, r=0, b={c={sdk.admin_java.6-7-1-SNAPSHOT=1}}}} [firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data (contents hidden) [firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data: {t=d, d={a=p, r=2, b={p=-LVyOZEzspYgAQKjyxxW, d={a=a, b=b, c=c, d=d, e=e, f=f, g=g, h=h}}}} [firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data: {t=d, d={a=q, r=3, b={p=-LVyOZEzspYgAQKjyxxW, q={vf=r, en=f, ep=null, l=5}, t=1, h=}}} [firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data: {t=d, d={a=n, r=4, b={p=-LVyOZEzspYgAQKjyxxW, q={vf=r, en=f, ep=null, l=5}, t=1}}} [firebase-websocket-worker] DEBUG com.google.firebase.database.connection.WebsocketConnection - [ws_0] WS message: {"t":"c","d":{"t":"h","d":{"ts":1547235117146,"v":"5","h":"s-usc1c-nss-235.firebaseio.com","s":"xgg5G1TWJrs6fUaQwcNttDD8IMV6lhBZ"}}} [firebase-websocket-worker] DEBUG com.google.firebase.database.connection.WebsocketConnection - [ws_0] WS message: {"t":"d","d":{"r":0,"b":{"s":"ok","d":""}}} [firebase-websocket-worker] DEBUG com.google.firebase.database.connection.WebsocketConnection - [ws_0] WS message: {"t":"d","d":{"r":1,"b":{"s":"ok","d":{"auth":null,"expires":1547238717}}}} [firebase-websocket-worker] DEBUG com.google.firebase.database.connection.WebsocketConnection - [ws_0] WS message: {"t":"d","d":{"r":2,"b":{"s":"ok","d":""}}} [firebase-websocket-worker] DEBUG com.google.firebase.database.connection.WebsocketConnection - [ws_0] WS message: {"t":"d","d":{"b":{"p":"-LVyOZEzspYgAQKjyxxW","d":{"d":"d","e":"e","f":"f","g":"g","h":"h"},"t":1},"a":"d"}} 

Note that the result of the query is different, which causes the test failure. @rockwotj @mikelehen any ideas?

[rockwotj]

I'm not surprised that there are differences here. Is possible to get the objects that are being rendered differently?

Also @hiranya911 which test is your comment from?

[hiranya911]

I'll try to find the two nodes in the console. This is the failing test:

firebase-admin-java/src/test/java/com/google/firebase/database/integration/QueryTestIT.java

Lines 1813 to 1829 in 6b46ded

	@Test
	public void testEndAtWithTwoArgumentsAndLimit()
	throws TestFailure, ExecutionException, TimeoutException, InterruptedException {
	DatabaseReference ref = IntegrationTestUtils.getRandomNode(masterApp);
	Map<String, Object> toSet = new MapBuilder().put("a", "a")
	.put("b", "b").put("c", "c").put("d", "d").put("e", "e").put("f", "f")
	.put("g", "g").put("h", "h").build();
	new WriteFuture(ref, toSet).timedGet();
	DataSnapshot snap = TestHelpers.getSnap(ref.endAt(null, "f").limitToLast(5));
	Map<String, Object> expected = new MapBuilder().put("b", "b")
	.put("c", "c").put("d", "d").put("e", "e").put("f", "f").build();
	TestHelpers.assertDeepEquals(expected, snap.getValue());
	}

[hiranya911]

@rockwotj The 2 nodes are rendered identically in RTDB afaict. If I run the gson-based implementation against the node created by org.json, the test still fails.

[rockwotj]

I would then expect the difference is reading from the network? That looks consistent with the test.

[hiranya911]

So these are the two responses we get.

Success case:

{"t":"d","d":{"b":{"p":"-LVyO5qTnbP6ihEsnzCr","d":{"b":"b","c":"c","d":"d","e":"e","f":"f"},"t":1},"a":"d"}} 

Failure case:

{"t":"d","d":{"b":{"p":"-LVyOZEzspYgAQKjyxxW","d":{"d":"d","e":"e","f":"f","g":"g","h":"h"},"t":1},"a":"d"}} 

It seems RTDB returns 5 things in each case, but one of them doesn't respect the endAt constraint.

[mikelehen]

@schmidt-sebastian is OOO. I'll take a look at this when I get a few spare cycles. For the test failure, can you see what the SDK is sending to the backend? If it's getting wrong query results, it's probably sending the wrong query somehow (thanks for digging in!).

[rockwotj]

@mikelehen the data looks the same in both cases from the client - if I knew how to run these locally I'd look at what the server is seeing (tcpdump?).

[mikelehen]

@rockwotj Gah, sorry. I see you guys covered that above. My bad.

[hiranya911]

@rockwotj you can do this to run the test locally.

1. Clone this repo, and import into Idea as a Java project.
2. Get a service account json file from a Firebase project, rename it to integration_cert.json, and add to the root of the project.
3. Now you can open QueryTestIT class from Idea, pick the test case in question, and just run it.

EDIT:

BTW you can also see the SDK outgoing messages in the debug logs I've posted above.

[firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data: {t=d, d={a=p, r=2, b={p=-LVyOZEzspYgAQKjyxxW, d={a=a, b=b, c=c, d=d, e=e, f=f, g=g, h=h}}}} [firebase-database-worker] DEBUG com.google.firebase.database.connection.Connection - [conn_0] Sending data: {t=d, d={a=q, r=3, b={p=-LVyOZEzspYgAQKjyxxW, q={vf=r, en=f, ep=null, l=5}, t=1, h=}}} 

[matejsp]

There was a problem with "null" handling. It seems that firebase code does not handle "null" values very well. I have changed Gson to emit null values (instead of skipping them as is default Gson behaviour).

This test now works again.

[rockwotj]

Ah strange those logs must have been wrong then? Our server does explicitly expect null and not missing properties. Good catch!

[hiranya911]

Very interesting. I think logs get generated pre-serialization. So it logs the Map in the memory as opposed to the actual outgoing json string.

[hiranya911]

Adding a bit of extra logging exposes the problem.

Success case:

[ws_0] Raw outgoing message: {"t":"d","d":{"a":"q","r":3,"b":{"p":"-LWClm_BS9_eIPsUPBKw","q":{"vf":"r","en":"f","ep":null,"l":5},"t":1,"h":""}}} 

Failure case:

[ws_0] Raw outgoing message: {"t":"d","d":{"a":"q","r":3,"b":{"p":"-LWClKjGYfTPnSarEl7S","q":{"vf":"r","en":"f","l":5},"t":1,"h":""}}} 

[hiranya911]

@rockwotj @mikelehen I'm at an LGTM with this. Can somebody from the RTDB team take a look and approve before we merge?

[mikelehen]

I have left a few comments. If we resolve those, then I think I'm happy with this.

[matejsp]

commented, fixed, rebased, squashed :)

[hiranya911]

LGTM from me. Will merge once that last comment on CHANGELOG is addressed.

[matejsp]

fixed

[mikelehen]

Thanks for the changes! A few more comments after taking another look (sorry).

[mikelehen]

Thanks, I think this looks good!

As a minor side-note, in general it's preferable not to force-push changes to a PR since:

1. As a reviewer it's very nice for me to be able to see only what changed since my last review, but if you force-push commits, I can't do that and have to look at the whole diff again.
2. github has trouble dealing with comments that are made on a commit that is then lost due to a force-push. So sometimes comments disappear, etc.

[mikelehen]

(and since we always do "squash merge", it's okay to have tons of commits in a PR, so there's no need to force-push)