Repository: firebase/firebase-admin-java
Pull request #242 (2019-01-09 08:52:19)

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

matejsp: I have updated changelog, and resolved two issues that you pointed out.
And commented on last issue.
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?
hiranya911: I'll try to find the two nodes in the console. This is the failing test:

https://github.com/firebase/firebase-admin-java/blob/6b46deda57c18c534f2bc261b0927a52d4b89ba2/src/test/java/com/google/firebase/database/integration/QueryTestIT.java#L1813-L1829
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.
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":""}}}
```
mikelehen: I have left a few comments. If we resolve those, then I think I'm happy with this.
matejsp: commented, fixed, rebased, squashed :)
matejsp: fixed
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)

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index df571de..7763b85 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,6 +1,7 @@
 # Unreleased
 
--
+- [changed] Removed org.json dependency and replaced with com.google.code.gson.
+- [changed] Upgraded Mockito dependency, and fixed the build on Java 11.
 
 # v6.7.0
 
diff --git a/pom.xml b/pom.xml
index df453ac..0fcfe20 100644
--- a/pom.xml
+++ b/pom.xml
@@ -437,11 +437,6 @@
             <artifactId>guava</artifactId>
             <version>20.0</version>
         </dependency>
-        <dependency>
-            <groupId>org.json</groupId>
-            <artifactId>json</artifactId>
-            <version>20160810</version>
-        </dependency>
         <dependency>
             <groupId>org.slf4j</groupId>
             <artifactId>slf4j-api</artifactId>
@@ -467,7 +462,7 @@
         <dependency>
             <groupId>org.mockito</groupId>
             <artifactId>mockito-core</artifactId>
-            <version>2.7.21</version>
+            <version>2.23.4</version>
             <scope>test</scope>
         </dependency>
         <dependency>
diff --git a/src/main/java/com/google/firebase/database/util/JsonMapper.java b/src/main/java/com/google/firebase/database/util/JsonMapper.java
index 0c178f7..0321c8b 100644
--- a/src/main/java/com/google/firebase/database/util/JsonMapper.java
+++ b/src/main/java/com/google/firebase/database/util/JsonMapper.java
@@ -16,20 +16,18 @@
 
 package com.google.firebase.database.util;
 
+import com.google.gson.Gson;
+import com.google.gson.GsonBuilder;
+import com.google.gson.JsonSyntaxException;
+import com.google.gson.stream.JsonReader;
+import com.google.gson.stream.JsonToken;
 import java.io.IOException;
+import java.io.StringReader;
 import java.util.ArrayList;
-import java.util.Collection;
 import java.util.HashMap;
-import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 
-import org.json.JSONArray;
-import org.json.JSONException;
-import org.json.JSONObject;
-import org.json.JSONStringer;
-import org.json.JSONTokener;
-
 /**
  * Helper class to convert from/to JSON strings. TODO: This class should ideally not live in
  * firebase-database-connection, but it's required by both firebase-database and
@@ -37,102 +35,82 @@ import org.json.JSONTokener;
  */
 public class JsonMapper {
 
-  public static String serializeJson(Map<String, Object> object) throws IOException {
-    return serializeJsonValue(object);
-  }
-
-  @SuppressWarnings("unchecked")
-  public static String serializeJsonValue(Object object) throws IOException {
-    if (object == null) {
-      return "null";
-    } else if (object instanceof String) {
-      return JSONObject.quote((String) object);
-    } else if (object instanceof Number) {
-      try {
-        return JSONObject.numberToString((Number) object);
-      } catch (JSONException e) {
-        throw new IOException("Could not serialize number", e);
-      }
-    } else if (object instanceof Boolean) {
-      return ((Boolean) object) ? "true" : "false";
-    } else {
-      try {
-        JSONStringer stringer = new JSONStringer();
-        serializeJsonValue(object, stringer);
-        return stringer.toString();
-      } catch (JSONException e) {
-        throw new IOException("Failed to serialize JSON", e);
-      }
-    }
-  }
+  private static final Gson GSON = new GsonBuilder().serializeNulls().create();
 
-  private static void serializeJsonValue(Object object, JSONStringer stringer)
-      throws IOException, JSONException {
-    if (object instanceof Map) {
-      stringer.object();
-      @SuppressWarnings("unchecked")
-      Map<String, Object> map = (Map<String, Object>) object;
-      for (Map.Entry<String, Object> entry : map.entrySet()) {
-        stringer.key(entry.getKey());
-        serializeJsonValue(entry.getValue(), stringer);
-      }
-      stringer.endObject();
-    } else if (object instanceof Collection) {
-      Collection<?> collection = (Collection<?>) object;
-      stringer.array();
-      for (Object entry : collection) {
-        serializeJsonValue(entry, stringer);
-      }
-      stringer.endArray();
-    } else {
-      stringer.value(object);
+  public static String serializeJson(Object object) throws IOException {
+    try {
+      return GSON.toJson(object);
+    } catch (JsonSyntaxException e) {
+      throw new IOException(e);
     }
   }
 
   public static Map<String, Object> parseJson(String json) throws IOException {
     try {
-      return unwrapJsonObject(new JSONObject(json));
-    } catch (JSONException e) {
+      JsonReader jsonReader = new JsonReader(new StringReader(json));
+      return unwrapJsonObject(jsonReader);
+    } catch (IllegalStateException | JsonSyntaxException e) {
       throw new IOException(e);
     }
   }
 
   public static Object parseJsonValue(String json) throws IOException {
     try {
-      return unwrapJson(new JSONTokener(json).nextValue());
-    } catch (JSONException e) {
+      JsonReader jsonReader = new JsonReader(new StringReader(json));
+      jsonReader.setLenient(true);
+      return unwrapJson(jsonReader);
+    } catch (IllegalStateException | JsonSyntaxException e) {
       throw new IOException(e);
     }
   }
 
-  @SuppressWarnings("unchecked")
-  private static Map<String, Object> unwrapJsonObject(JSONObject jsonObject) throws JSONException {
-    Map<String, Object> map = new HashMap<>(jsonObject.length());
-    Iterator<String> keys = jsonObject.keys();
-    while (keys.hasNext()) {
-      String key = keys.next();
-      map.put(key, unwrapJson(jsonObject.get(key)));
+  private static Map<String, Object> unwrapJsonObject(JsonReader jsonReader) throws IOException {
+    Map<String, Object> map = new HashMap<>();
+    jsonReader.beginObject();
+    while (jsonReader.peek() != JsonToken.END_OBJECT) {
+      String key = jsonReader.nextName();
+      map.put(key, unwrapJson(jsonReader));
     }
+    jsonReader.endObject();
     return map;
   }
 
-  private static List<Object> unwrapJsonArray(JSONArray jsonArray) throws JSONException {
-    List<Object> list = new ArrayList<>(jsonArray.length());
-    for (int i = 0; i < jsonArray.length(); i++) {
-      list.add(unwrapJson(jsonArray.get(i)));
+  private static List<Object> unwrapJsonArray(JsonReader jsonReader) throws IOException {
+    List<Object> list = new ArrayList<>();
+    jsonReader.beginArray();
+    while (jsonReader.peek() != JsonToken.END_ARRAY) {
+      list.add(unwrapJson(jsonReader));
     }
+    jsonReader.endArray();
     return list;
   }
 
-  private static Object unwrapJson(Object o) throws JSONException {
-    if (o instanceof JSONObject) {
-      return unwrapJsonObject((JSONObject) o);
-    } else if (o instanceof JSONArray) {
-      return unwrapJsonArray((JSONArray) o);
-    } else if (o.equals(JSONObject.NULL)) {
-      return null;
-    } else {
-      return o;
+  private static Object unwrapJson(JsonReader jsonReader) throws IOException {
+    switch (jsonReader.peek()) {
+      case BEGIN_ARRAY:
+        return unwrapJsonArray(jsonReader);
+      case BEGIN_OBJECT:
+        return unwrapJsonObject(jsonReader);
+      case STRING:
+        return jsonReader.nextString();
+      case NUMBER:
+        String value = jsonReader.nextString();
+        if (value.matches("-?\\d+")) {
+          long longValue = Long.parseLong(value);
+          if (longValue <= Integer.MAX_VALUE && longValue >= Integer.MIN_VALUE) {
+            return (int) longValue;
+          }
+          return Long.valueOf(value);
+        }
+        return Double.parseDouble(value);
+      case BOOLEAN:
+        return jsonReader.nextBoolean();
+      case NULL:
+        jsonReader.nextNull();
+        return null;
+      default:
+        throw new IllegalStateException("unknown type " + jsonReader.peek());
     }
   }
+
 }


Test patch:
diff --git a/src/test/java/com/google/firebase/database/connection/WebsocketConnectionTest.java b/src/test/java/com/google/firebase/database/connection/WebsocketConnectionTest.java
index 896d4fe..02f2bfe 100644
--- a/src/test/java/com/google/firebase/database/connection/WebsocketConnectionTest.java
+++ b/src/test/java/com/google/firebase/database/connection/WebsocketConnectionTest.java
@@ -112,9 +112,7 @@ public class WebsocketConnectionTest {
   public void testIncomingMessageIOError() {
     MockClientFactory clientFactory = new MockClientFactory();
     ImmutableMap<String, Object> data = ImmutableMap.<String, Object>of("key", "value");
-    Mockito.doThrow(IOException.class).when(clientFactory.delegate).onMessage(data);
-    clientFactory.eventHandler.onMessage("{\"key\":\"value\"}");
-    Mockito.verify(clientFactory.delegate, Mockito.times(1)).onMessage(data);
+    clientFactory.eventHandler.onMessage("ERR{\"key\":\"value\"}");
     Mockito.verify(clientFactory.client, Mockito.times(1)).close();
     Mockito.verify(clientFactory.delegate, Mockito.times(1)).onDisconnect(false);
   }
diff --git a/src/test/java/com/google/firebase/database/core/JvmAuthTokenProviderTest.java b/src/test/java/com/google/firebase/database/core/JvmAuthTokenProviderTest.java
index a174b3e..7b8fe52 100644
--- a/src/test/java/com/google/firebase/database/core/JvmAuthTokenProviderTest.java
+++ b/src/test/java/com/google/firebase/database/core/JvmAuthTokenProviderTest.java
@@ -30,6 +30,7 @@ import com.google.firebase.FirebaseApp;
 import com.google.firebase.FirebaseOptions;
 import com.google.firebase.TestOnlyImplFirebaseTrampolines;
 import com.google.firebase.auth.MockGoogleCredentials;
+import com.google.firebase.database.util.JsonMapper;
 import com.google.firebase.testing.TestUtils;
 
 import java.io.IOException;
@@ -46,7 +47,6 @@ import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
-import org.json.JSONObject;
 import org.junit.After;
 import org.junit.Test;
 
@@ -100,7 +100,7 @@ public class JvmAuthTokenProviderTest {
   }
 
   @Test
-  public void testGetTokenWithAuthOverrides() throws InterruptedException {
+  public void testGetTokenWithAuthOverrides() throws InterruptedException, IOException {
     MockGoogleCredentials credentials = new MockGoogleCredentials("mock-token");
     Map<String, Object> auth = ImmutableMap.<String, Object>of("uid", "test");
     FirebaseOptions options = new FirebaseOptions.Builder()
@@ -226,13 +226,15 @@ public class JvmAuthTokenProviderTest {
     assertTrue(semaphore.tryAcquire(TestUtils.TEST_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS));
   }
 
-  private void assertToken(String token, String expectedToken, Map<String, Object> expectedAuth) {
+  private void assertToken(String token, String expectedToken, Map<String, Object> expectedAuth)
+      throws IOException {
     assertTrue(token.startsWith("gauth|"));
     String jsonString = token.substring(6);
-    JSONObject json = new JSONObject(jsonString);
-    assertEquals(expectedToken, json.getString("token"));
+    Map<String, Object> map = JsonMapper.parseJson(jsonString);
 
-    Map<String, Object> auth = json.getJSONObject("auth").toMap();
+    assertEquals(expectedToken, map.get("token"));
+
+    Map<String, Object> auth = (Map)map.get("auth");
     DeepEquals.deepEquals(expectedAuth, auth);
   }
 
diff --git a/src/test/java/com/google/firebase/database/util/JsonMapperTest.java b/src/test/java/com/google/firebase/database/util/JsonMapperTest.java
index ceecfe1..0134d27 100644
--- a/src/test/java/com/google/firebase/database/util/JsonMapperTest.java
+++ b/src/test/java/com/google/firebase/database/util/JsonMapperTest.java
@@ -17,7 +17,6 @@
 package com.google.firebase.database.util;
 
 import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
@@ -28,55 +27,42 @@ import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
-import org.json.JSONException;
-import org.junit.Ignore;
 import org.junit.Test;
 
 public class JsonMapperTest {
 
   @Test
   public void testNull() throws IOException {
-    assertEquals("null", JsonMapper.serializeJsonValue(null));
+    assertEquals("null", JsonMapper.serializeJson(null));
   }
 
   @Test
   public void testString() throws IOException {
-    assertEquals("\"foo\"", JsonMapper.serializeJsonValue("foo"));
+    assertEquals("\"foo\"", JsonMapper.serializeJson("foo"));
   }
 
   @Test
   public void testBoolean() throws IOException {
-    assertEquals("true", JsonMapper.serializeJsonValue(true));
-    assertEquals("false", JsonMapper.serializeJsonValue(false));
+    assertEquals("true", JsonMapper.serializeJson(true));
+    assertEquals("false", JsonMapper.serializeJson(false));
   }
 
   @Test
   public void testMap() throws IOException {
-    assertEquals("{\"foo\":\"bar\"}", JsonMapper.serializeJsonValue(ImmutableMap.of("foo", "bar")));
+    assertEquals("{\"foo\":\"bar\"}", JsonMapper.serializeJson(ImmutableMap.of("foo", "bar")));
   }
 
   @Test
   public void testList() throws IOException {
-    assertEquals("[\"foo\",\"bar\"]", JsonMapper.serializeJsonValue(
+    assertEquals("[\"foo\",\"bar\"]", JsonMapper.serializeJson(
         ImmutableList.of("foo", "bar")));
   }
 
-  @Test
-  public void testInvalidObject() {
-    try {
-      JsonMapper.serializeJsonValue(new Object());
-      fail("No error thrown for invalid object");
-    } catch (IOException expected) {
-      // expected
-      assertTrue(expected.getCause() instanceof JSONException);
-    }
-  }
-
   @Test
   public void canConvertLongs() throws IOException {
     List<Long> longs = Arrays.asList(Long.MAX_VALUE, Long.MIN_VALUE);
     for (Long original : longs) {
-      String jsonString = JsonMapper.serializeJsonValue(original);
+      String jsonString = JsonMapper.serializeJson(original);
       long converted = (Long) JsonMapper.parseJsonValue(jsonString);
       assertEquals((long) original, converted);
     }
@@ -86,15 +72,13 @@ public class JsonMapperTest {
   public void canConvertDoubles() throws IOException {
     List<Double> doubles = Arrays.asList(Double.MAX_VALUE, Double.MIN_VALUE, Double.MIN_NORMAL);
     for (Double original : doubles) {
-      String jsonString = JsonMapper.serializeJsonValue(original);
+      String jsonString = JsonMapper.serializeJson(original);
       double converted = (Double) JsonMapper.parseJsonValue(jsonString);
       assertEquals(original, converted, 0);
     }
   }
 
   @Test
-  @Ignore
-  // TODO: Stop ignoring this test once JSON parsing has been fixed.
   public void canNest33LevelsDeep() throws IOException {
     Map<String, Object> root = new HashMap<>();
     Map<String, Object> currentMap = root;
@@ -103,7 +87,7 @@ public class JsonMapperTest {
       currentMap.put("key", newMap);
       currentMap = newMap;
     }
-    String jsonString = JsonMapper.serializeJsonValue(root);
+    String jsonString = JsonMapper.serializeJson(root);
     Object value = JsonMapper.parseJsonValue(jsonString);
     assertEquals(root, value);
   }