Custom serializers for ChangeKindCache, including protobuf

Change-Id: I7f2e56f78fd346170922c366c57465d6dfb136ef

java/com/google/gerrit/server/BUILD

@@ -89,6 +89,7 @@ java_library(
         "//lib/ow2:ow2-asm-tree",
         "//lib/ow2:ow2-asm-util",
         "//lib/prolog:runtime",
+        "//proto:cache_java_proto",
     ],
 )
 

java/com/google/gerrit/server/cache/CacheModule.java

@@ -26,7 +26,6 @@ import com.google.inject.Scopes;
 import com.google.inject.TypeLiteral;
 import com.google.inject.name.Names;
 import com.google.inject.util.Types;
-import java.io.Serializable;
 import java.lang.reflect.Type;
 
 /** Miniature DSL to support binding {@link Cache} instances in Guice. */
@@ -120,7 +119,7 @@ public abstract class CacheModule extends FactoryModule {
    * @param <V> type of value stored by the cache.
    * @return binding to describe the cache.
    */
-  protected <K extends Serializable, V extends Serializable> PersistentCacheBinding<K, V> persist(
+  protected <K, V> PersistentCacheBinding<K, V> persist(
       String name, Class<K> keyType, Class<V> valType) {
     return persist(name, TypeLiteral.get(keyType), TypeLiteral.get(valType));
   }
@@ -132,7 +131,7 @@ public abstract class CacheModule extends FactoryModule {
    * @param <V> type of value stored by the cache.
    * @return binding to describe the cache.
    */
-  protected <K extends Serializable, V extends Serializable> PersistentCacheBinding<K, V> persist(
+  protected <K, V> PersistentCacheBinding<K, V> persist(
       String name, Class<K> keyType, TypeLiteral<V> valType) {
     return persist(name, TypeLiteral.get(keyType), valType);
   }
@@ -144,7 +143,7 @@ public abstract class CacheModule extends FactoryModule {
    * @param <V> type of value stored by the cache.
    * @return binding to describe the cache.
    */
-  protected <K extends Serializable, V extends Serializable> PersistentCacheBinding<K, V> persist(
+  protected <K, V> PersistentCacheBinding<K, V> persist(
       String name, TypeLiteral<K> keyType, TypeLiteral<V> valType) {
     PersistentCacheProvider<K, V> m = new PersistentCacheProvider<>(this, name, keyType, valType);
     bindCache(m, name, keyType, valType);

java/com/google/gerrit/server/cache/EnumCacheSerializer.java

@@ -0,0 +1,41 @@
+// Copyright (C) 2018 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.server.cache;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+
+import com.google.common.base.Enums;
+import java.io.IOException;
+
+public class EnumCacheSerializer<E extends Enum<E>> implements CacheSerializer<E> {
+  private final Class<E> clazz;
+
+  public EnumCacheSerializer(Class<E> clazz) {
+    this.clazz = clazz;
+  }
+
+  @Override
+  public byte[] serialize(E object) throws IOException {
+    return object.name().getBytes(UTF_8);
+  }
+
+  @Override
+  public E deserialize(byte[] in) throws IOException {
+    String name = new String(in, UTF_8);
+    return Enums.getIfPresent(clazz, name)
+        .toJavaUtil()
+        .orElseThrow(() -> new IOException("Invalid " + clazz.getName() + " value: " + name));
+  }
+}

java/com/google/gerrit/server/cache/JavaCacheSerializer.java

@@ -19,10 +19,14 @@ import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
-import java.io.Serializable;
 
-/** Serializer that uses default Java serialization. */
+/**
-public class JavaCacheSerializer<T extends Serializable> implements CacheSerializer<T> {
+ * Serializer that uses default Java serialization.
+ *
+ * @param <T> type to serialize. Must implement {@code Serializable}, but due to implementation
+ *     details this is only checked at runtime.
+ */
+public class JavaCacheSerializer<T> implements CacheSerializer<T> {
   @Override
   public byte[] serialize(T object) throws IOException {
     try (ByteArrayOutputStream bout = new ByteArrayOutputStream();

java/com/google/gerrit/server/cache/PersistentCacheProvider.java

@@ -23,6 +23,7 @@ import com.google.gerrit.common.Nullable;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.TypeLiteral;
+import java.io.Serializable;
 import java.util.concurrent.TimeUnit;
 
 class PersistentCacheProvider<K, V> extends CacheProvider<K, V>
@@ -119,12 +120,24 @@ class PersistentCacheProvider<K, V> extends CacheProvider<K, V>
       return super.get();
     }
     checkState(version >= 0, "version is required");
-    checkState(keySerializer != null, "keySerializer is required");
+    checkSerializer(keyType(), keySerializer, "key");
-    checkState(valueSerializer != null, "valueSerializer is required");
+    checkSerializer(valueType(), valueSerializer, "value");
     freeze();
     CacheLoader<K, V> ldr = loader();
     return ldr != null
         ? persistentCacheFactory.build(this, ldr)
         : persistentCacheFactory.build(this);
   }
+
+  private static <T> void checkSerializer(
+      TypeLiteral<T> type, CacheSerializer<T> serializer, String name) {
+    checkState(serializer != null, "%sSerializer is required", name);
+    if (serializer instanceof JavaCacheSerializer) {
+      checkState(
+          Serializable.class.isAssignableFrom(type.getRawType()),
+          "%s type %s must implement Serializable",
+          name,
+          type);
+    }
+  }
 }

java/com/google/gerrit/server/cache/ProtoCacheSerializers.java

@@ -0,0 +1,47 @@
+// Copyright (C) 2018 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.server.cache;
+
+import com.google.protobuf.CodedOutputStream;
+import com.google.protobuf.MessageLite;
+import java.io.IOException;
+
+/** Static utilities for writing protobuf-based {@link CacheSerializer} implementations. */
+public class ProtoCacheSerializers {
+  /**
+   * Serializes a proto to a byte array.
+   *
+   * <p>Guarantees deterministic serialization and thus is suitable for use as a persistent cache
+   * key. Should be used in preference to {@link MessageLite#toByteArray()}, which is not guaranteed
+   * deterministic.
+   *
+   * @param message the proto message to serialize.
+   * @return a byte array with the message contents.
+   */
+  public static byte[] toByteArray(MessageLite message) {
+    byte[] bytes = new byte[message.getSerializedSize()];
+    CodedOutputStream cout = CodedOutputStream.newInstance(bytes);
+    cout.useDeterministicSerialization();
+    try {
+      message.writeTo(cout);
+      cout.checkNoSpaceLeft();
+      return bytes;
+    } catch (IOException e) {
+      throw new IllegalStateException("exception writing to byte array");
+    }
+  }
+
+  private ProtoCacheSerializers() {}
+}

java/com/google/gerrit/server/change/ChangeKindCacheImpl.java

@@ -16,8 +16,6 @@ package com.google.gerrit.server.change;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
-import static org.eclipse.jgit.lib.ObjectIdSerializer.readWithoutMarker;
-import static org.eclipse.jgit.lib.ObjectIdSerializer.writeWithoutMarker;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.cache.Cache;
@@ -30,6 +28,10 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.cache.CacheSerializer;
+import com.google.gerrit.server.cache.EnumCacheSerializer;
+import com.google.gerrit.server.cache.ProtoCacheSerializers;
+import com.google.gerrit.server.cache.proto.Cache.ChangeKindKeyProto;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.git.InMemoryInserter;
@@ -39,10 +41,8 @@ import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
 import com.google.inject.Module;
 import com.google.inject.name.Named;
+import com.google.protobuf.ByteString;
 import java.io.IOException;
-import java.io.ObjectInputStream;
-import java.io.ObjectOutputStream;
-import java.io.Serializable;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Objects;
@@ -51,6 +51,7 @@ import java.util.concurrent.Callable;
 import java.util.concurrent.ExecutionException;
 import org.eclipse.jgit.errors.LargeObjectException;
 import org.eclipse.jgit.lib.Config;
+import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.Repository;
@@ -72,7 +73,10 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
         bind(ChangeKindCache.class).to(ChangeKindCacheImpl.class);
         persist(ID_CACHE, Key.class, ChangeKind.class)
             .maximumWeight(2 << 20)
-            .weigher(ChangeKindWeigher.class);
+            .weigher(ChangeKindWeigher.class)
+            .version(1)
+            .keySerializer(new Key.Serializer())
+            .valueSerializer(new EnumCacheSerializer<>(ChangeKind.class));
       }
     };
   }
@@ -122,9 +126,7 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
     }
   }
 
-  public static class Key implements Serializable {
+  public static class Key {
-    private static final long serialVersionUID = 1L;
-
     private transient ObjectId prior;
     private transient ObjectId next;
     private transient String strategyName;
@@ -171,16 +173,28 @@ public class ChangeKindCacheImpl implements ChangeKindCache {
       return Objects.hash(prior, next, strategyName);
     }
 
-    private void writeObject(ObjectOutputStream out) throws IOException {
+    @VisibleForTesting
-      writeWithoutMarker(out, prior);
+    static class Serializer implements CacheSerializer<Key> {
-      writeWithoutMarker(out, next);
+      @Override
-      out.writeUTF(strategyName);
+      public byte[] serialize(Key object) throws IOException {
-    }
+        byte[] buf = new byte[Constants.OBJECT_ID_LENGTH];
+        ChangeKindKeyProto.Builder b = ChangeKindKeyProto.newBuilder();
+        object.getPrior().copyRawTo(buf, 0);
+        b.setPrior(ByteString.copyFrom(buf));
+        object.getNext().copyRawTo(buf, 0);
+        b.setNext(ByteString.copyFrom(buf));
+        b.setStrategyName(object.getStrategyName());
+        return ProtoCacheSerializers.toByteArray(b.build());
+      }
 
-    private void readObject(ObjectInputStream in) throws IOException {
+      @Override
-      prior = readWithoutMarker(in);
+      public Key deserialize(byte[] in) throws IOException {
-      next = readWithoutMarker(in);
+        ChangeKindKeyProto proto = ChangeKindKeyProto.parseFrom(in);
-      strategyName = in.readUTF();
+        return new Key(
+            ObjectId.fromRaw(proto.getPrior().toByteArray()),
+            ObjectId.fromRaw(proto.getNext().toByteArray()),
+            proto.getStrategyName());
+      }
     }
   }
 

javatests/com/google/gerrit/server/BUILD

@@ -53,6 +53,7 @@ junit_tests(
         "//lib:gson",
         "//lib:guava-retrying",
         "//lib:gwtorm",
+        "//lib:protobuf",
         "//lib:truth-java8-extension",
         "//lib/auto:auto-value",
         "//lib/auto:auto-value-annotations",
@@ -60,5 +61,6 @@ junit_tests(
         "//lib/guice",
         "//lib/jgit/org.eclipse.jgit:jgit",
         "//lib/jgit/org.eclipse.jgit.junit:junit",
+        "//proto:cache_java_proto",
     ],
 )

javatests/com/google/gerrit/server/cache/EnumCacheSerializerTest.java

@@ -0,0 +1,39 @@
+// Copyright (C) 2018 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.server.cache;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import org.junit.Test;
+
+public class EnumCacheSerializerTest {
+  @Test
+  public void serialize() throws Exception {
+    assertRoundTrip(MyEnum.FOO);
+    assertRoundTrip(MyEnum.BAR);
+    assertRoundTrip(MyEnum.BAZ);
+  }
+
+  private enum MyEnum {
+    FOO,
+    BAR,
+    BAZ;
+  }
+
+  private static void assertRoundTrip(MyEnum e) throws Exception {
+    CacheSerializer<MyEnum> s = new EnumCacheSerializer<>(MyEnum.class);
+    assertThat(s.deserialize(s.serialize(e))).isEqualTo(e);
+  }
+}

javatests/com/google/gerrit/server/change/ChangeKindCacheImplTest.java

@@ -0,0 +1,55 @@
+// Copyright (C) 2018 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.server.change;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import com.google.gerrit.server.cache.CacheSerializer;
+import com.google.gerrit.server.cache.proto.Cache.ChangeKindKeyProto;
+import com.google.protobuf.ByteString;
+import org.eclipse.jgit.lib.ObjectId;
+import org.junit.Test;
+
+public class ChangeKindCacheImplTest {
+  @Test
+  public void keySerializer() throws Exception {
+    ChangeKindCacheImpl.Key key =
+        new ChangeKindCacheImpl.Key(
+            ObjectId.zeroId(),
+            ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"),
+            "aStrategy");
+    CacheSerializer<ChangeKindCacheImpl.Key> s = new ChangeKindCacheImpl.Key.Serializer();
+    byte[] serialized = s.serialize(key);
+    assertThat(ChangeKindKeyProto.parseFrom(serialized))
+        .isEqualTo(
+            ChangeKindKeyProto.newBuilder()
+                .setPrior(bytes(0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0))
+                .setNext(
+                    bytes(
+                        0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef,
+                        0xde, 0xad, 0xbe, 0xef, 0xde, 0xad, 0xbe, 0xef))
+                .setStrategyName("aStrategy")
+                .build());
+    assertThat(s.deserialize(serialized)).isEqualTo(key);
+  }
+
+  private static ByteString bytes(int... ints) {
+    byte[] bytes = new byte[ints.length];
+    for (int i = 0; i < ints.length; i++) {
+      bytes[i] = (byte) ints[i];
+    }
+    return ByteString.copyFrom(bytes);
+  }
+}

proto/BUILD

@@ -0,0 +1,10 @@
+proto_library(
+    name = "cache_proto",
+    srcs = ["cache.proto"],
+)
+
+java_proto_library(
+    name = "cache_java_proto",
+    visibility = ["//visibility:public"],
+    deps = [":cache_proto"],
+)

proto/cache.proto

@@ -0,0 +1,27 @@
+// Copyright (C) 2018 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.
+
+syntax = "proto3";
+
+package gerrit.cache;
+
+option java_package = "com.google.gerrit.server.cache.proto";
+
+// Serialized form of com.google.gerrit.server.change.CHangeKindCacheImpl.Key.
+// Next ID: 4
+message ChangeKindKeyProto {
+  bytes prior = 1;
+  bytes next = 2;
+  string strategy_name = 3;
+}

tools/eclipse/project.py

@@ -145,6 +145,7 @@ def gen_classpath(ext):
   doc = make_classpath()
   src = set()
   lib = set()
+  proto = set()
   gwt_src = set()
   gwt_lib = set()
   plugins = set()
@@ -170,6 +171,9 @@ def gen_classpath(ext):
       # JGit dependency from external repository
       if 'gerrit-' not in p and 'jgit' in p:
         lib.add(p)
+      # Assume any jars in /proto/ are from java_proto_library rules
+      if '/bin/proto/' in p:
+        proto.add(p)
     else:
       # Don't mess up with Bazel internal test runner dependencies.
       # When we use Eclipse we rely on it for running the tests
@@ -239,6 +243,11 @@ def gen_classpath(ext):
             continue
         classpathentry('lib', j, s)
 
+  for p in sorted(proto):
+    s = p.replace('-fastbuild/bin/proto/lib', '-fastbuild/genfiles/proto/')
+    s = s.replace('.jar', '-src.jar')
+    classpathentry('lib', p, s)
+
   for s in sorted(gwt_src):
     p = path.join(ROOT, s, 'src', 'main', 'java')
     if path.exists(p):