Migrate from GSON to our own serializer/deserializer

There's a problem where GSON will merrily decode a number as a double. While technically correct, this causes Bad Things to happen with interop. Fortunatley our own JSON <-> Bean converters Do The Right Thing already. Switch to using those.
SeleniumHQ · Aug 21, 2017 · d4eb9a2 · d4eb9a2
1 parent f9aa0e2
commit d4eb9a2
Show file tree

Hide file tree

Showing 9 changed files with 63 additions and 50 deletions.
diff --git a/java/server/src/org/openqa/selenium/remote/server/AllHandlers.java b/java/server/src/org/openqa/selenium/remote/server/AllHandlers.java
@@ -23,9 +23,9 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
-import com.google.gson.Gson;
-import com.google.gson.GsonBuilder;
 
+import org.openqa.selenium.remote.BeanToJsonConverter;
+import org.openqa.selenium.remote.JsonToBeanConverter;
 import org.openqa.selenium.remote.SessionId;
 import org.openqa.selenium.remote.http.HttpMethod;
 import org.openqa.selenium.remote.server.commandhandler.GetAllSessions;
@@ -51,12 +51,16 @@
 
 class AllHandlers {
 
-  private final static Gson GSON = new GsonBuilder().setLenient().serializeNulls().create();
+  private final JsonToBeanConverter toBean;
+  private final BeanToJsonConverter toJson;
   private final ActiveSessions allSessions;
 
   private final Map<HttpMethod, ImmutableList<Function<String, CommandHandler>>> additionalHandlers;
 
   public AllHandlers(ActiveSessions allSessions) {
+    this.toBean = new JsonToBeanConverter();
+    this.toJson = new BeanToJsonConverter();
+
     this.allSessions = allSessions;
 
     additionalHandlers = ImmutableMap.of(
@@ -99,12 +103,12 @@ public CommandHandler match(HttpServletRequest req) {
     if (id != null) {
       ActiveSession session = allSessions.get(id);
       if (session == null) {
-        return new NoSessionHandler(id);
+        return new NoSessionHandler(toJson, id);
       }
       return session;
     }
 
-    return new NoHandler();
+    return new NoHandler(toJson);
   }
 
   private <H extends CommandHandler> Function<String, CommandHandler> handler(
@@ -119,7 +123,8 @@ private <H extends CommandHandler> Function<String, CommandHandler> handler(
 
       ImmutableSet.Builder<Object> args = ImmutableSet.builder();
       args.add(allSessions);
-      args.add(GSON);
+      args.add(toBean);
+      args.add(toJson);
       if (match.getParameters().containsKey("sessionId")) {
         SessionId id = new SessionId(match.getParameters().get("sessionId"));
         args.add(id);

diff --git a/java/server/src/org/openqa/selenium/remote/server/commandhandler/GetAllSessions.java b/java/server/src/org/openqa/selenium/remote/server/commandhandler/GetAllSessions.java
@@ -23,8 +23,8 @@
 import static org.openqa.selenium.remote.ErrorCodes.SUCCESS;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.gson.GsonBuilder;
 
+import org.openqa.selenium.remote.BeanToJsonConverter;
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.HttpResponse;
 import org.openqa.selenium.remote.server.ActiveSessions;
@@ -34,13 +34,16 @@
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 
 public class GetAllSessions implements CommandHandler {
 
-  private volatile ActiveSessions allSessions;
+  private final ActiveSessions allSessions;
+  private final BeanToJsonConverter toJson;
 
-  public GetAllSessions(ActiveSessions allSessions) {
-    this.allSessions = allSessions;
+  public GetAllSessions(ActiveSessions allSessions, BeanToJsonConverter toJson) {
+    this.allSessions = Objects.requireNonNull(allSessions);
+    this.toJson = Objects.requireNonNull(toJson);
   }
 
   @Override
@@ -55,10 +58,7 @@ public void execute(HttpRequest req, HttpResponse resp) throws IOException {
         "value", value);
 
     // Write out a minimal W3C status response.
-    byte[] payload = new GsonBuilder()
-        .serializeNulls()
-        .create()
-        .toJson(payloadObj).getBytes(UTF_8);
+    byte[] payload = toJson.convert(payloadObj).getBytes(UTF_8);
 
     resp.setStatus(HTTP_OK);
     resp.setHeader("Content-Type", JSON_UTF_8.toString());

diff --git a/java/server/src/org/openqa/selenium/remote/server/commandhandler/GetLogTypes.java b/java/server/src/org/openqa/selenium/remote/server/commandhandler/GetLogTypes.java
@@ -21,31 +21,28 @@
 import static org.openqa.selenium.remote.http.HttpMethod.GET;
 
 import com.google.common.collect.ImmutableSet;
-import com.google.gson.Gson;
-import com.google.gson.reflect.TypeToken;
 
 import org.openqa.selenium.logging.LogType;
 import org.openqa.selenium.remote.ErrorCodes;
+import org.openqa.selenium.remote.JsonToBeanConverter;
 import org.openqa.selenium.remote.Response;
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.HttpResponse;
 import org.openqa.selenium.remote.server.ActiveSession;
 import org.openqa.selenium.remote.server.CommandHandler;
 
 import java.io.IOException;
-import java.lang.reflect.Type;
 import java.util.Collection;
 import java.util.Map;
 import java.util.Objects;
 
 public class GetLogTypes implements CommandHandler {
 
-  private static final Type MAP_TYPE = new TypeToken<Map<String, Object>>(){}.getType();
-  private final Gson gson;
+  private final JsonToBeanConverter toBean;
   private final ActiveSession session;
 
-  public GetLogTypes(Gson gson, ActiveSession session) {
-    this.gson = Objects.requireNonNull(gson);
+  public GetLogTypes(JsonToBeanConverter toBean, ActiveSession session) {
+    this.toBean = Objects.requireNonNull(toBean);
     this.session = Objects.requireNonNull(session);
   }
 
@@ -60,7 +57,7 @@ public void execute(HttpRequest req, HttpResponse resp) throws IOException {
     types.add(LogType.SERVER);
 
     if (upRes.getStatus() == HTTP_OK) {
-      Map<String, Object> upstream = gson.fromJson(upRes.getContentString(), MAP_TYPE);
+      Map<?, ?> upstream = toBean.convert(Map.class, upRes.getContentString());
       Object raw = upstream.get("value");
       if (raw instanceof Collection) {
         ((Collection<?>) raw).stream().map(String::valueOf).forEach(types::add);

diff --git a/java/server/src/org/openqa/selenium/remote/server/commandhandler/GetLogsOfType.java b/java/server/src/org/openqa/selenium/remote/server/commandhandler/GetLogsOfType.java
@@ -20,11 +20,10 @@
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.openqa.selenium.remote.http.HttpMethod.POST;
 
-import com.google.gson.Gson;
-
 import org.openqa.selenium.logging.LogEntries;
 import org.openqa.selenium.logging.LogType;
 import org.openqa.selenium.remote.ErrorCodes;
+import org.openqa.selenium.remote.JsonToBeanConverter;
 import org.openqa.selenium.remote.Response;
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.HttpResponse;
@@ -34,22 +33,23 @@
 
 import java.io.IOException;
 import java.util.Map;
+import java.util.Objects;
 
 public class GetLogsOfType implements CommandHandler {
 
-  private final Gson gson;
+  private final JsonToBeanConverter toBean;
   private final ActiveSession session;
 
-  public GetLogsOfType(Gson gson, ActiveSession session) {
-    this.gson = gson;
-    this.session = session;
+  public GetLogsOfType(JsonToBeanConverter toBean, ActiveSession session) {
+    this.toBean = Objects.requireNonNull(toBean);
+    this.session = Objects.requireNonNull(session);
   }
 
   @Override
   public void execute(HttpRequest req, HttpResponse resp) throws IOException {
     String originalPayload = req.getContentString();
 
-    Map<?, ?> args = gson.fromJson(originalPayload, Map.class);
+    Map<?, ?> args = toBean.convert(Map.class, originalPayload);
     String type = (String) args.get("type");
 
     if (!LogType.SERVER.equals(type)) {

diff --git a/java/server/src/org/openqa/selenium/remote/server/commandhandler/NoHandler.java b/java/server/src/org/openqa/selenium/remote/server/commandhandler/NoHandler.java
@@ -23,8 +23,8 @@
 import static org.openqa.selenium.remote.ErrorCodes.UNKNOWN_COMMAND;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.gson.GsonBuilder;
 
+import org.openqa.selenium.remote.BeanToJsonConverter;
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.HttpResponse;
 import org.openqa.selenium.remote.server.CommandHandler;
@@ -33,9 +33,16 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Objects;
 
 public class NoHandler implements CommandHandler {
 
+  private final BeanToJsonConverter toJson;
+
+  public NoHandler(BeanToJsonConverter toJson) {
+    this.toJson = Objects.requireNonNull(toJson);
+  }
+
   @Override
   public void execute(HttpRequest req, HttpResponse resp) throws IOException {
     // We're not using ImmutableMap for the outer map because it disallows null values.
@@ -51,8 +58,7 @@ public void execute(HttpRequest req, HttpResponse resp) throws IOException {
         "stacktrace", ""));
     responseMap = Collections.unmodifiableMap(responseMap);
 
-    byte[] payload = new GsonBuilder().serializeNulls().create().toJson(responseMap)
-        .getBytes(UTF_8);
+    byte[] payload = toJson.convert(responseMap).getBytes(UTF_8);
 
     resp.setStatus(HTTP_NOT_FOUND);
     resp.setHeader("Content-Type", JSON_UTF_8.toString());

diff --git a/java/server/src/org/openqa/selenium/remote/server/commandhandler/NoSessionHandler.java b/java/server/src/org/openqa/selenium/remote/server/commandhandler/NoSessionHandler.java
@@ -23,8 +23,8 @@
 import static org.openqa.selenium.remote.ErrorCodes.NO_SUCH_SESSION;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.gson.GsonBuilder;
 
+import org.openqa.selenium.remote.BeanToJsonConverter;
 import org.openqa.selenium.remote.SessionId;
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.HttpResponse;
@@ -34,13 +34,16 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
+import java.util.Objects;
 
 public class NoSessionHandler implements CommandHandler {
 
+  private final BeanToJsonConverter toJson;
   private final SessionId sessionId;
 
-  public NoSessionHandler(SessionId sessionId) {
-    this.sessionId = sessionId;
+  public NoSessionHandler(BeanToJsonConverter toJson, SessionId sessionId) {
+    this.toJson = Objects.requireNonNull(toJson);
+    this.sessionId = Objects.requireNonNull(sessionId);
   }
 
   @Override
@@ -55,8 +58,7 @@ public void execute(HttpRequest req, HttpResponse resp) throws IOException {
         "stacktrace", ""));
     responseMap = Collections.unmodifiableMap(responseMap);
 
-    byte[] payload = new GsonBuilder().serializeNulls().create().toJson(responseMap)
-        .getBytes(UTF_8);
+    byte[] payload = toJson.convert(responseMap).getBytes(UTF_8);
 
     resp.setStatus(HTTP_NOT_FOUND);
     resp.setHeader("Content-Type", JSON_UTF_8.toString());

diff --git a/java/server/src/org/openqa/selenium/remote/server/commandhandler/Status.java b/java/server/src/org/openqa/selenium/remote/server/commandhandler/Status.java
@@ -23,18 +23,25 @@
 import static org.openqa.selenium.remote.ErrorCodes.SUCCESS;
 
 import com.google.common.collect.ImmutableMap;
-import com.google.gson.GsonBuilder;
 
 import org.openqa.selenium.internal.BuildInfo;
+import org.openqa.selenium.remote.BeanToJsonConverter;
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.HttpResponse;
 import org.openqa.selenium.remote.server.CommandHandler;
 
 import java.io.IOException;
 import java.util.Map;
+import java.util.Objects;
 
 public class Status implements CommandHandler {
 
+  private final BeanToJsonConverter toJson;
+
+  Status(BeanToJsonConverter toJson) {
+    this.toJson = Objects.requireNonNull(toJson);
+  }
+
   @Override
   public void execute(HttpRequest req, HttpResponse resp) throws IOException {
     ImmutableMap.Builder<String, Object> value = ImmutableMap.builder();
@@ -63,10 +70,7 @@ public void execute(HttpRequest req, HttpResponse resp) throws IOException {
         "value", value.build());
 
     // Write out a minimal W3C status response.
-    byte[] payload = new GsonBuilder()
-        .serializeNulls()
-        .create()
-        .toJson(payloadObj).getBytes(UTF_8);
+    byte[] payload = toJson.convert(payloadObj).getBytes(UTF_8);
 
     resp.setStatus(HTTP_OK);
     resp.setHeader("Content-Type", JSON_UTF_8.toString());

diff --git a/java/server/src/org/openqa/selenium/remote/server/commandhandler/UploadFile.java b/java/server/src/org/openqa/selenium/remote/server/commandhandler/UploadFile.java
@@ -17,11 +17,10 @@
 
 package org.openqa.selenium.remote.server.commandhandler;
 
-import com.google.gson.Gson;
-
 import org.openqa.selenium.WebDriverException;
 import org.openqa.selenium.io.Zip;
 import org.openqa.selenium.remote.ErrorCodes;
+import org.openqa.selenium.remote.JsonToBeanConverter;
 import org.openqa.selenium.remote.Response;
 import org.openqa.selenium.remote.http.HttpRequest;
 import org.openqa.selenium.remote.http.HttpResponse;
@@ -35,17 +34,17 @@
 
 public class UploadFile implements CommandHandler {
 
-  private final Gson gson;
+  private final JsonToBeanConverter toBean;
   private final ActiveSession session;
 
-  public UploadFile(Gson gson, ActiveSession session) {
-    this.gson = Objects.requireNonNull(gson);
+  public UploadFile(JsonToBeanConverter toBean, ActiveSession session) {
+    this.toBean = Objects.requireNonNull(toBean);
     this.session = Objects.requireNonNull(session);
   }
 
   @Override
   public void execute(HttpRequest req, HttpResponse resp) throws IOException {
-    Map<?, ?> args = gson.fromJson(req.getContentString(), Map.class);
+    Map<?, ?> args = toBean.convert(Map.class, req.getContentString());
     String file = (String) args.get("file");
 
     File tempDir = session.getFileSystem().createTempDir("upload", "file");

diff --git a/java/server/test/org/openqa/selenium/remote/server/commandhandler/UploadFileTest.java b/java/server/test/org/openqa/selenium/remote/server/commandhandler/UploadFileTest.java
@@ -79,7 +79,7 @@ public void shouldWriteABase64EncodedZippedFileToDiskAndKeepName() throws Except
     String encoded = Zip.zip(tempFile);
 
     Gson gson = new Gson();
-    UploadFile uploadFile = new UploadFile(gson, session);
+    UploadFile uploadFile = new UploadFile(new JsonToBeanConverter(), session);
     Map<String, Object> args = ImmutableMap.of("file", (Object) encoded);
     HttpRequest request = new HttpRequest(HttpMethod.POST, "/session/%d/se/file");
     request.setContent(gson.toJson(args).getBytes(UTF_8));
@@ -106,7 +106,7 @@ public void shouldThrowAnExceptionIfMoreThanOneFileIsSent() throws Exception {
     String encoded = Zip.zip(baseDir);
 
     Gson gson = new Gson();
-    UploadFile uploadFile = new UploadFile(gson, session);
+    UploadFile uploadFile = new UploadFile(new JsonToBeanConverter(), session);
     Map<String, Object> args = ImmutableMap.of("file", (Object) encoded);
     HttpRequest request = new HttpRequest(HttpMethod.POST, "/session/%d/se/file");
     request.setContent(gson.toJson(args).getBytes(UTF_8));