Extract StringSerializer to its own non-test class

The implementation is slightly more complicated than just calling
"new String(bytes, UTF_8)", because that constructor replaces
undecodable byte sequences with a plain question mark ('?'). This
shouldn't happen in practice, since we should only be deserializing
data previously written by this serializer, but bugs are possible, and
in this case they might lead to very subtle behavior problems.

As the String constructor javadoc indicates, we should use
CharsetDecoder directly in order to control the behavior.

Also use CharsetEncoder directly for serialization. This is even less
necessary than for deserialization, because UTF-8 by design can encode
any string. However, the code is exactly analogous to the decoding
case, so it's easy to write, and it reduces the number of assumptions
the code needs to make about how this Charset works.

Change-Id: I08b64b72d81df25bc8dae30c1b09a4ea3a49c3a2

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

@@ -0,0 +1,70 @@
+// 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.annotations.VisibleForTesting;
+import java.nio.ByteBuffer;
+import java.nio.CharBuffer;
+import java.nio.charset.CharacterCodingException;
+import java.nio.charset.Charset;
+import java.nio.charset.CodingErrorAction;
+
+public enum StringSerializer implements CacheSerializer<String> {
+  INSTANCE;
+
+  @Override
+  public byte[] serialize(String object) {
+    return serialize(UTF_8, object);
+  }
+
+  @VisibleForTesting
+  static byte[] serialize(Charset charset, String s) {
+    if (s.isEmpty()) {
+      return new byte[0];
+    }
+    try {
+      ByteBuffer buf =
+          charset
+              .newEncoder()
+              .onMalformedInput(CodingErrorAction.REPORT)
+              .onUnmappableCharacter(CodingErrorAction.REPORT)
+              .encode(CharBuffer.wrap(s));
+      byte[] result = new byte[buf.remaining()];
+      buf.get(result);
+      return result;
+    } catch (CharacterCodingException e) {
+      throw new IllegalStateException("Failed to serialize string", e);
+    }
+  }
+
+  @Override
+  public String deserialize(byte[] in) {
+    if (in.length == 0) {
+      return "";
+    }
+    try {
+      return UTF_8
+          .newDecoder()
+          .onMalformedInput(CodingErrorAction.REPORT)
+          .onUnmappableCharacter(CodingErrorAction.REPORT)
+          .decode(ByteBuffer.wrap(in))
+          .toString();
+    } catch (CharacterCodingException e) {
+      throw new IllegalStateException("Failed to deserialize string", e);
+    }
+  }
+}

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

@@ -0,0 +1,63 @@
+// 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 static com.google.common.truth.Truth.assert_;
+
+import java.nio.charset.CharacterCodingException;
+import java.nio.charset.StandardCharsets;
+import org.junit.Test;
+
+public class StringSerializerTest {
+  @Test
+  public void serialize() {
+    assertThat(StringSerializer.INSTANCE.serialize("")).isEmpty();
+    assertThat(StringSerializer.INSTANCE.serialize("abc")).isEqualTo(new byte[] {'a', 'b', 'c'});
+    assertThat(StringSerializer.INSTANCE.serialize("a\u1234c"))
+        .isEqualTo(new byte[] {'a', (byte) 0xe1, (byte) 0x88, (byte) 0xb4, 'c'});
+  }
+
+  @Test
+  public void serializeInvalidChar() {
+    // Can't use UTF-8 for the test, since it can encode all Unicode code points.
+    try {
+      StringSerializer.serialize(StandardCharsets.US_ASCII, "\u1234");
+      assert_().fail("expected IllegalStateException");
+    } catch (IllegalStateException expected) {
+      assertThat(expected).hasCauseThat().isInstanceOf(CharacterCodingException.class);
+    }
+  }
+
+  @Test
+  public void deserialize() {
+    assertThat(StringSerializer.INSTANCE.deserialize(new byte[0])).isEmpty();
+    assertThat(StringSerializer.INSTANCE.deserialize(new byte[] {'a', 'b', 'c'})).isEqualTo("abc");
+    assertThat(
+            StringSerializer.INSTANCE.deserialize(
+                new byte[] {'a', (byte) 0xe1, (byte) 0x88, (byte) 0xb4, 'c'}))
+        .isEqualTo("a\u1234c");
+  }
+
+  @Test
+  public void deserializeInvalidChar() {
+    try {
+      StringSerializer.INSTANCE.deserialize(new byte[] {(byte) 0xff});
+      assert_().fail("expected IllegalStateException");
+    } catch (IllegalStateException expected) {
+      assertThat(expected).hasCauseThat().isInstanceOf(CharacterCodingException.class);
+    }
+  }
+}

javatests/com/google/gerrit/server/cache/h2/H2CacheTest.java

@@ -15,12 +15,11 @@
 package com.google.gerrit.server.cache.h2;
 
 import static com.google.common.truth.Truth.assertThat;
-import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.cache.Cache;
 import com.google.common.cache.CacheBuilder;
 import com.google.common.util.concurrent.MoreExecutors;
-import com.google.gerrit.server.cache.CacheSerializer;
+import com.google.gerrit.server.cache.StringSerializer;
 import com.google.gerrit.server.cache.h2.H2CacheImpl.SqlStore;
 import com.google.gerrit.server.cache.h2.H2CacheImpl.ValueHolder;
 import com.google.inject.TypeLiteral;
@@ -124,23 +123,6 @@ public class H2CacheTest {
     assertThat(oldImpl.getIfPresent("key")).isNull();
   }
 
-  // TODO(dborowitz): Won't be necessary when we use a real StringSerializer in the server code.
-  private enum StringSerializer implements CacheSerializer<String> {
-    INSTANCE;
-
-    @Override
-    public byte[] serialize(String object) {
-      return object.getBytes(UTF_8);
-    }
-
-    @Override
-    public String deserialize(byte[] in) {
-      // TODO(dborowitz): Consider using CharsetDecoder directly in the real implementation, to get
-      // checked exceptions.
-      return new String(in, UTF_8);
-    }
-  }
-
   private static <K, V> Cache<K, ValueHolder<V>> disableMemCache() {
     return CacheBuilder.newBuilder().maximumSize(0).build();
   }