Log encounters of invalid enum values in REST requests

A specified string value for an enum which doesn't match any existing
enum value results in the enum field being simply ignored
(= unset) in the request. This also means that simple typos in the
name of an enum or the wrong case of it go unnoticed. We intend to
reject such kind of requests and force callers to adjust their input.
The base for this is already laid in this change. As a first step,
we intend to only log such encounters, though. A follow-up change will
change the behavior to rejections.

As it's highly desirable to not re-implement all of the logic to map
an enum to JSON and the other way around (and to also keep up with
new Gson features), this change introduces a delegate implementation
for enums which just does an additional check on top of the regular
implementation. Due to this and limitations of Gson, we can't log the
invalid enum value, though.

This change also introduces some tests which document the current
behavior. Those tests will also be helpful when we introduce the
rejections in the future.

Change-Id: I820fde9bca07038171da5d181518b9655d3a4dd2

4 files changed, 165 insertions, 1 deletions

diff --git a/java/com/google/gerrit/json/BUILD b/java/com/google/gerrit/json/BUILD
index 7282dc46ed..030dddcb63 100644
--- a/java/com/google/gerrit/json/BUILD
+++ b/java/com/google/gerrit/json/BUILD
@@ -4,5 +4,6 @@ java_library(
     visibility = ["//visibility:public"],
     deps = [
         "//lib:gson",
+        "//lib/flogger:api",
     ],
 )
diff --git a/java/com/google/gerrit/json/EnumTypeAdapterFactory.java b/java/com/google/gerrit/json/EnumTypeAdapterFactory.java
new file mode 100644
index 0000000000..dc74f67dbf
--- /dev/null
+++ b/java/com/google/gerrit/json/EnumTypeAdapterFactory.java
@@ -0,0 +1,78 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.json;
+
+import com.google.common.flogger.FluentLogger;
+import com.google.gson.Gson;
+import com.google.gson.TypeAdapter;
+import com.google.gson.TypeAdapterFactory;
+import com.google.gson.internal.bind.TypeAdapters;
+import com.google.gson.reflect.TypeToken;
+import com.google.gson.stream.JsonReader;
+import com.google.gson.stream.JsonToken;
+import com.google.gson.stream.JsonWriter;
+import java.io.IOException;
+
+/**
+ * A {@code TypeAdapterFactory} for enums.
+ *
+ * <p>This factory introduces a wrapper around Gson's own default enum handler to add the following
+ * special behavior: log when input which doesn't match any existing enum value is encountered.
+ */
+public class EnumTypeAdapterFactory implements TypeAdapterFactory {
+  private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+  @SuppressWarnings({"unchecked"})
+  @Override
+  public <T> TypeAdapter<T> create(Gson gson, TypeToken<T> typeToken) {
+    TypeAdapter<T> defaultEnumAdapter = TypeAdapters.ENUM_FACTORY.create(gson, typeToken);
+    if (defaultEnumAdapter == null) {
+      // Not an enum. -> Enum type adapter doesn't apply.
+      return null;
+    }
+
+    return (TypeAdapter<T>) new EnumTypeAdapter(defaultEnumAdapter, typeToken);
+  }
+
+  private static class EnumTypeAdapter<T extends Enum<T>> extends TypeAdapter<T> {
+
+    private final TypeAdapter<T> defaultEnumAdapter;
+    private final TypeToken<T> typeToken;
+
+    public EnumTypeAdapter(TypeAdapter<T> defaultEnumAdapter, TypeToken<T> typeToken) {
+      this.defaultEnumAdapter = defaultEnumAdapter;
+      this.typeToken = typeToken;
+    }
+
+    @Override
+    public T read(JsonReader in) throws IOException {
+      // Still handle null values. -> Check them first.
+      if (in.peek() == JsonToken.NULL) {
+        in.nextNull();
+        return null;
+      }
+      T enumValue = defaultEnumAdapter.read(in);
+      if (enumValue == null) {
+        logger.atWarning().log("Expected an existing value for enum %s.", typeToken);
+      }
+      return enumValue;
+    }
+
+    @Override
+    public void write(JsonWriter out, T value) throws IOException {
+      defaultEnumAdapter.write(out, value);
+    }
+  }
+}
diff --git a/java/com/google/gerrit/json/OutputFormat.java b/java/com/google/gerrit/json/OutputFormat.java
index a2d174f4cc..3e7c319aa7 100644
--- a/java/com/google/gerrit/json/OutputFormat.java
+++ b/java/com/google/gerrit/json/OutputFormat.java
@@ -55,7 +55,8 @@ public enum OutputFormat {
     GsonBuilder gb =
         new GsonBuilder()
             .setFieldNamingPolicy(FieldNamingPolicy.LOWER_CASE_WITH_UNDERSCORES)
-            .registerTypeAdapter(Timestamp.class, new SqlTimestampDeserializer());
+            .registerTypeAdapter(Timestamp.class, new SqlTimestampDeserializer())
+            .registerTypeAdapterFactory(new EnumTypeAdapterFactory());
     if (this == OutputFormat.JSON) {
       gb.setPrettyPrinting();
     }
diff --git a/javatests/com/google/gerrit/json/JsonEnumMappingTest.java b/javatests/com/google/gerrit/json/JsonEnumMappingTest.java
new file mode 100644
index 0000000000..6e57b011ef
--- /dev/null
+++ b/javatests/com/google/gerrit/json/JsonEnumMappingTest.java
@@ -0,0 +1,84 @@
+// Copyright (C) 2019 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.json;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gson.Gson;
+import org.junit.Test;
+
+public class JsonEnumMappingTest {
+
+  // Use the regular, pre-configured Gson object we use throughout the Gerrit server to ensure that
+  // the EnumTypeAdapterFactory is properly set up.
+  private final Gson gson = OutputFormat.JSON.newGson();
+
+  @Test
+  public void nullCanBeWrittenAndParsedBack() {
+    String resultingJson = gson.toJson(null, TestEnum.class);
+    TestEnum value = gson.fromJson(resultingJson, TestEnum.class);
+    assertThat(value).isNull();
+  }
+
+  @Test
+  public void enumValueCanBeWrittenAndParsedBack() {
+    String resultingJson = gson.toJson(TestEnum.ONE, TestEnum.class);
+    TestEnum value = gson.fromJson(resultingJson, TestEnum.class);
+    assertThat(value).isEqualTo(TestEnum.ONE);
+  }
+
+  @Test
+  public void enumValueCanBeParsed() {
+    TestData data = gson.fromJson("{\"value\":\"ONE\"}", TestData.class);
+    assertThat(data.value).isEqualTo(TestEnum.ONE);
+  }
+
+  @Test
+  public void mixedCaseEnumValueIsTreatedAsUnset() {
+    TestData data = gson.fromJson("{\"value\":\"oNe\"}", TestData.class);
+    assertThat(data.value).isNull();
+  }
+
+  @Test
+  public void lowerCaseEnumValueIsTreatedAsUnset() {
+    TestData data = gson.fromJson("{\"value\":\"one\"}", TestData.class);
+    assertThat(data.value).isNull();
+  }
+
+  @Test
+  public void notExistingEnumValueIsTreatedAsUnset() {
+    TestData data = gson.fromJson("{\"value\":\"FOUR\"}", TestData.class);
+    assertThat(data.value).isNull();
+  }
+
+  @Test
+  public void emptyEnumValueIsTreatedAsUnset() {
+    TestData data = gson.fromJson("{\"value\":\"\"}", TestData.class);
+    assertThat(data.value).isNull();
+  }
+
+  private static class TestData {
+    TestEnum value;
+
+    public TestData(TestEnum value) {
+      this.value = value;
+    }
+  }
+
+  private enum TestEnum {
+    ONE,
+    TWO
+  }
+}