ObjectIdSerializer: Support serialization of known non-null ObjectId

The implementation of ObjectIdSerializer, added in change I7599cf8bd,
is not equivalent to the original implementation in Gerrit [1].

The Gerrit implementation provides separate methods to (de)serialize
instances of ObjectId that are known to be non-null. In these methods,
no "marker" is written to the stream. Replacing Gerrit's implementation
with ObjectIdSerializer [2] broke persistent caches because it started
writing markers where they were not expected [3].

Since ObjectIdSerializer is included in JGit 4.11 we can't change the
existing #write and #read methods. Keep those as-is, but extend the
Javadoc to clarify that they support possibly null ObjectId instances.

Add new methods #writeWithoutMarker and #readWithoutMarker to support
the cases where the ObjectId is known to be non-null and the marker
should not be written to the serialization stream.

Also:

- Replace the hard-coded `0` and `1` markers with constants that can
  be linked from the Javadocs.

- Include the marker value in the "Invalid flag before ObjectId"
  exception message.

[1] https://gerrit-review.googlesource.com/c/gerrit/+/9792
[2] https://gerrit-review.googlesource.com/c/gerrit/+/165851
[3] https://gerrit-review.googlesource.com/c/gerrit/+/165952

Change-Id: Iaf84c3ec32ecf83efffb306fdb4940cc85740f3f
Signed-off-by: David Pursehouse <david.pursehouse@gmail.com>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ObjectIdSerializerTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ObjectIdSerializerTest.java
index 24bc40eee8df48a945f1f54e2bea9cf2221d5e09..d98b792d7589ccf465a5773da38ae367292558cc 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ObjectIdSerializerTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/lib/ObjectIdSerializerTest.java
@@ -79,10 +79,18 @@ public class ObjectIdSerializerTest {
                        throws Exception {
                File file = File.createTempFile("ObjectIdSerializerTest_", "");
                try (OutputStream out = new FileOutputStream(file)) {
-                       ObjectIdSerializer.write(out, objectId);
+                       if (objectId == null) {
+                               ObjectIdSerializer.write(out, objectId);
+                       } else {
+                               ObjectIdSerializer.writeWithoutMarker(out, objectId);
+                       }
                }
                try (InputStream in = new FileInputStream(file)) {
-                       return ObjectIdSerializer.read(in);
+                       if (objectId == null) {
+                               return ObjectIdSerializer.read(in);
+                       } else {
+                               return ObjectIdSerializer.readWithoutMarker(in);
+                       }
                }
        }
 }

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSerializer.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSerializer.java
index 59da408306e24f9ec476fc6ff4f23624eb5d148f..96c7cee1c6e3f120ccf62227e162c4486238be5e 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSerializer.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/ObjectIdSerializer.java
@@ -51,57 +51,110 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
 
+import org.eclipse.jgit.annotations.NonNull;
 import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.util.IO;
 
 /**
  * Helper to serialize {@link ObjectId} instances. {@link ObjectId} is already
- * serializable, but this class provides a more optimal implementation. It
- * writes out a flag (0 or 1) followed by the object's words, or nothing if it
- * was a null id.
+ * serializable, but this class provides methods to handle null and non-null
+ * instances.
  *
  * @since 4.11
  */
 public class ObjectIdSerializer {
+       /*
+        * Marker to indicate a null ObjectId instance.
+        */
+       private static final byte NULL_MARKER = 0;
+
+       /*
+        * Marker to indicate a non-null ObjectId instance.
+        */
+       private static final byte NON_NULL_MARKER = 1;
+
        /**
+        * Write a possibly null {@link ObjectId} to the stream, using markers to
+        * differentiate null and non-null instances.
+        *
+        * <p>
+        * If the id is non-null, writes a {@link #NON_NULL_MARKER} followed by the
+        * id's words. If it is null, writes a {@link #NULL_MARKER} and nothing
+        * else.
+        *
         * @param out
         *            the output stream
         * @param id
-        *            the object id to serialize
+        *            the object id to serialize; may be null
         * @throws IOException
         *             the stream writing failed
         */
        public static void write(OutputStream out, @Nullable AnyObjectId id)
                        throws IOException {
                if (id != null) {
-                       out.write((byte) 1);
-                       id.copyRawTo(out);
+                       out.write(NON_NULL_MARKER);
+                       writeWithoutMarker(out, id);
                } else {
-                       out.write((byte) 0);
+                       out.write(NULL_MARKER);
                }
        }
 
        /**
+        * Write a non-null {@link ObjectId} to the stream.
+        *
+        * @param out
+        *            the output stream
+        * @param id
+        *            the object id to serialize; never null
+        * @throws IOException
+        *             the stream writing failed
+        */
+       public static void writeWithoutMarker(OutputStream out, @NonNull AnyObjectId id)
+                       throws IOException {
+               id.copyRawTo(out);
+       }
+
+       /**
+        * Read a possibly null {@link ObjectId} from the stream.
+        *
+        * Reads the first byte of the stream, which is expected to be either
+        * {@link #NON_NULL_MARKER} or {@link #NULL_MARKER}.
+        *
         * @param in
         *            the input stream
-        * @return the object id
+        * @return the object id, or null
         * @throws IOException
         *             there was an error reading the stream
         */
        @Nullable
        public static ObjectId read(InputStream in) throws IOException {
-               switch (in.read()) {
-               case 0:
+               byte marker = (byte) in.read();
+               switch (marker) {
+               case NULL_MARKER:
                        return null;
-               case 1:
-                       final byte[] b = new byte[OBJECT_ID_LENGTH];
-                       IO.readFully(in, b, 0, OBJECT_ID_LENGTH);
-                       return ObjectId.fromRaw(b);
+               case NON_NULL_MARKER:
+                       return readWithoutMarker(in);
                default:
-                       throw new IOException("Invalid flag before ObjectId"); //$NON-NLS-1$
+                       throw new IOException("Invalid flag before ObjectId: " + marker); //$NON-NLS-1$
                }
        }
 
+       /**
+        * Read a non-null {@link ObjectId} from the stream.
+        *
+        * @param in
+        *            the input stream
+        * @return the object id; never null
+        * @throws IOException
+        *             there was an error reading the stream
+        */
+       @NonNull
+       public static ObjectId readWithoutMarker(InputStream in) throws IOException {
+               final byte[] b = new byte[OBJECT_ID_LENGTH];
+               IO.readFully(in, b, 0, OBJECT_ID_LENGTH);
+               return ObjectId.fromRaw(b);
+       }
+
        private ObjectIdSerializer() {
        }
 }