fix: make copy-on-read complete, so no read hands out the stored instance (#1294)

A stored document on MVStore is the live object held in the page, and MVStore
serializes pages on a background thread that any write can start through
tryCommit. Whatever a read hands out therefore must not share mutable state
with the stored instance, or a caller's in-place edit is written straight into
the store, bypasses the indexes, and can race the serialization into a
ConcurrentModificationException and a store panic.

The cursor has cloned each document it yields since 4.x, but two gaps remained:

- Document.clone() copied the top-level map and embedded documents only. A
  List, Set, Map, array, byte[], Date or Calendar inside the copy was still the
  instance in the store, so `found.get("tags").add(x)` reached the page.
  clone() is now a deep copy: containers and arrays are copied recursively,
  preserving the concrete collection class when it has a public no-arg
  constructor and the comparator of sorted sets and maps; Dates and Calendars
  are cloned; immutable values are shared; values of a type the copy does not
  know are shared as well, which the javadoc now states.

- NitriteCollection.getById() returned the stored instance itself. It now
  hands out a clone, as find() does, and returns null for a missing id instead
  of passing null through the processor chain.

The extra cost is a structural copy of each result, which is what find()
already paid at the top level. No serialization round-trip is involved.

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>

Author: brettwooldridge

nitrite/src/main/java/org/dizitart/no2/collection/NitriteDocument.java

@@ -27,6 +27,8 @@
 import java.io.ObjectInputStream;
 import java.io.ObjectOutputStream;
 import java.io.Serializable;
+import java.lang.reflect.Array;
+import java.lang.reflect.Modifier;
 import java.text.MessageFormat;
 import java.util.*;
 
@@ -155,23 +157,122 @@ public void remove(String field) {
         }
     }
 
+    /**
+     * Returns a deep copy of this document.
+     * <p>
+     * Every value that can be modified in place is copied: embedded documents, collections,
+     * maps, arrays (including {@code byte[]}), {@link Date}s and {@link Calendar}s, recursively
+     * at any depth. Immutable values (strings, boxed primitives, enums, {@code java.time}
+     * types and the like) are shared. A value of a type this method does not know how to copy
+     * is shared as well, so store immutable values or copy them yourself.
+     * <p>
+     * This is what makes copy-on-read safe: the cursor hands out clones, and nothing reachable
+     * from a clone is the instance kept by the store.
+     */
     @Override
     @SuppressWarnings("unchecked")
     public Document clone() {
         Map<String, Object> cloned = (Map<String, Object>) super.clone();
-
-        // create the clone of any embedded documents as well
         for (Map.Entry<String, Object> entry : cloned.entrySet()) {
-            if (entry.getValue() instanceof Document) {
-                Document value = (Document) entry.getValue();
+            entry.setValue(deepCopy(entry.getValue()));
+        }
+        return new NitriteDocument(cloned);
+    }
 
-                // this will recursively take care any embedded document
-                // of the clone as well
-                Document clonedValue = value.clone();
-                cloned.put(entry.getKey(), clonedValue);
+    private static Object deepCopy(Object value) {
+        if (value == null) {
+            return null;
+        }
+        if (value instanceof Document) {
+            return ((Document) value).clone();
+        }
+        if (value instanceof Collection) {
+            return deepCopyCollection((Collection<Object>) value);
+        }
+        if (value instanceof Map) {
+            return deepCopyMap((Map<Object, Object>) value);
+        }
+        if (value.getClass().isArray()) {
+            return deepCopyArray(value);
+        }
+        if (value instanceof Date) {
+            return ((Date) value).clone();
+        }
+        if (value instanceof Calendar) {
+            return ((Calendar) value).clone();
+        }
+        // immutable scalars, and opaque objects that cannot be copied generically
+        return value;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static Collection<Object> deepCopyCollection(Collection<Object> source) {
+        Collection<Object> copy;
+        if (source instanceof SortedSet) {
+            // a fresh instance of the same class would lose the comparator
+            copy = new TreeSet<>(((SortedSet<Object>) source).comparator());
+        } else {
+            copy = newInstanceOrNull(source);
+            if (copy == null) {
+                copy = source instanceof Set ? new LinkedHashSet<>() : new ArrayList<>(source.size());
             }
         }
-        return new NitriteDocument(cloned);
+        for (Object element : source) {
+            copy.add(deepCopy(element));
+        }
+        return copy;
+    }
+
+    @SuppressWarnings("unchecked")
+    private static Map<Object, Object> deepCopyMap(Map<Object, Object> source) {
+        Map<Object, Object> copy;
+        if (source instanceof SortedMap) {
+            copy = new TreeMap<>(((SortedMap<Object, Object>) source).comparator());
+        } else {
+            copy = newInstanceOrNull(source);
+            if (copy == null) {
+                copy = new LinkedHashMap<>();
+            }
+        }
+        // keys are expected to be immutable; only the values are copied
+        for (Map.Entry<Object, Object> entry : source.entrySet()) {
+            copy.put(entry.getKey(), deepCopy(entry.getValue()));
+        }
+        return copy;
+    }
+
+    private static Object deepCopyArray(Object source) {
+        Class<?> componentType = source.getClass().getComponentType();
+        int length = Array.getLength(source);
+        Object copy = Array.newInstance(componentType, length);
+        if (componentType.isPrimitive()) {
+            System.arraycopy(source, 0, copy, 0, length);
+        } else {
+            Object[] from = (Object[]) source;
+            Object[] to = (Object[]) copy;
+            for (int i = 0; i < length; i++) {
+                to[i] = deepCopy(from[i]);
+            }
+        }
+        return copy;
+    }
+
+    /**
+     * A new, empty instance of the same class as {@code source} when it has a public no-arg
+     * constructor (ArrayList, LinkedList, HashSet, ...), otherwise {@code null}. Unmodifiable
+     * and JDK-internal collections fall through to the caller's default.
+     */
+    @SuppressWarnings("unchecked")
+    private static <T> T newInstanceOrNull(T source) {
+        Class<?> type = source.getClass();
+        if (!Modifier.isPublic(type.getModifiers())) {
+            return null;
+        }
+        try {
+            return (T) type.getConstructor().newInstance();
+        } catch (ReflectiveOperationException | RuntimeException e) {
+            return null;
+        }
     }
 
     @Override

nitrite/src/main/java/org/dizitart/no2/collection/operation/ReadOperations.java

@@ -76,10 +76,15 @@ public DocumentCursor find(Filter filter, FindOptions findOptions) {
 
     Document getById(NitriteId nitriteId) {
         Document document = nitriteMap.get(nitriteId);
+        if (document == null) {
+            return null;
+        }
+        // hand out a copy, as the cursor does: the caller must never reach the stored instance
+        Document copy = document.clone();
         if (processorChain != null) {
-            document = processorChain.processAfterRead(document);
+            copy = processorChain.processAfterRead(copy);
         }
-        return document;
+        return copy;
     }
 
     private void prepareFilter(Filter filter) {

nitrite/src/test/java/org/dizitart/no2/collection/CopyOnReadTest.java

@@ -0,0 +1,110 @@
+/*
+ * Copyright (c) 2017-2020. Nitrite author or authors.
+ *
+ * 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 org.dizitart.no2.collection;
+
+import org.dizitart.no2.Nitrite;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+import static org.dizitart.no2.filters.FluentFilter.where;
+import static org.dizitart.no2.integration.TestUtil.createDb;
+import static org.junit.Assert.*;
+
+/**
+ * Whatever a read hands out must not be the instance the store keeps, at any depth.
+ * Otherwise a caller's in-place edit is written straight into the store, bypasses the
+ * indexes, and races the background serialization of the page it lives in.
+ */
+public class CopyOnReadTest {
+    private Nitrite db;
+    private NitriteCollection collection;
+    private NitriteId id;
+
+    @Before
+    public void setUp() {
+        db = createDb();
+        collection = db.getCollection("copy-on-read");
+        Document document = Document.createDocument("name", "one")
+            .put("tags", new ArrayList<>(Arrays.asList("a", "b")))
+            .put("nested", Document.createDocument("list", new ArrayList<>(Arrays.asList(1, 2))));
+        collection.insert(document);
+        id = collection.find().firstOrNull().getId();
+    }
+
+    @After
+    public void tearDown() {
+        db.close();
+    }
+
+    @Test
+    public void testCursorHandsOutIndependentCopies() {
+        Document first = collection.find(where("name").eq("one")).firstOrNull();
+        Document second = collection.find(where("name").eq("one")).firstOrNull();
+        assertNotSame(first, second);
+        assertNotSame(first.get("tags"), second.get("tags"));
+    }
+
+    @Test
+    public void testMutatingNestedContainerOfFoundDocumentDoesNotReachStore() {
+        Document found = collection.find(where("name").eq("one")).firstOrNull();
+        ((List<Object>) found.get("tags")).add("c");
+        ((List<Object>) found.get("nested", Document.class).get("list")).clear();
+
+        Document stored = collection.find(where("name").eq("one")).firstOrNull();
+        assertEquals(Arrays.asList("a", "b"), stored.get("tags"));
+        assertEquals(Arrays.asList(1, 2), stored.get("nested", Document.class).get("list"));
+    }
+
+    @Test
+    public void testGetByIdHandsOutIndependentCopies() {
+        Document first = collection.getById(id);
+        Document second = collection.getById(id);
+        assertNotSame(first, second);
+        assertNotSame(first.get("tags"), second.get("tags"));
+    }
+
+    @Test
+    public void testMutatingGetByIdResultDoesNotReachStore() {
+        Document found = collection.getById(id);
+        found.put("name", "changed");
+        ((List<Object>) found.get("tags")).add("c");
+
+        assertEquals("one", collection.getById(id).get("name"));
+        assertEquals(Arrays.asList("a", "b"), collection.getById(id).get("tags"));
+        assertEquals(1, collection.find(where("name").eq("one")).size());
+        assertEquals(0, collection.find(where("name").eq("changed")).size());
+    }
+
+    @Test
+    public void testGetByIdOfMissingDocumentIsNull() {
+        assertNull(collection.getById(NitriteId.createId(-1L)));
+    }
+
+    @Test
+    public void testCopyThenUpdateIsTheSupportedWriteShape() {
+        Document found = collection.getById(id);
+        found.put("name", "two");
+        collection.update(found);
+        assertEquals("two", collection.getById(id).get("name"));
+        assertEquals(1, collection.find(where("name").eq("two")).size());
+    }
+}

nitrite/src/test/java/org/dizitart/no2/collection/NitriteDocumentTest.java

@@ -6,8 +6,7 @@
 import org.dizitart.no2.exceptions.ValidationException;
 import org.junit.Test;
 
-import java.util.ArrayList;
-import java.util.Set;
+import java.util.*;
 
 import static org.junit.Assert.*;
 
@@ -146,6 +145,70 @@ public void testClone3() {
         assertEquals(1, nitriteDocument.clone().size());
     }
 
+    @Test
+    public void testCloneCopiesNestedContainers() {
+        List<Object> tags = new LinkedList<>(Arrays.asList("a", "b"));
+        NitriteDocument embedded = new NitriteDocument();
+        embedded.put("n", 1);
+        List<Document> embeddedList = new ArrayList<>(Collections.singletonList(embedded));
+        Map<String, Object> map = new HashMap<>();
+        map.put("k", new ArrayList<>(Collections.singletonList("v")));
+        byte[] bytes = {1, 2, 3};
+        int[] ints = {1, 2, 3};
+        Date date = new Date(1000L);
+        SortedSet<String> sorted = new TreeSet<>(Comparator.reverseOrder());
+        sorted.addAll(Arrays.asList("x", "y"));
+
+        NitriteDocument original = new NitriteDocument();
+        original.put("tags", tags);
+        original.put("docs", embeddedList);
+        original.put("map", map);
+        original.put("bytes", bytes);
+        original.put("ints", ints);
+        original.put("date", date);
+        original.put("sorted", sorted);
+        original.put("fixed", Collections.unmodifiableList(Arrays.asList("p", "q")));
+        original.put("name", "immutable");
+
+        Document clone = original.clone();
+        assertEquals(original, clone);
+
+        // nothing mutable is shared
+        assertNotSame(tags, clone.get("tags"));
+        assertNotSame(embeddedList, clone.get("docs"));
+        assertNotSame(embedded, ((List<?>) clone.get("docs")).get(0));
+        assertNotSame(map, clone.get("map"));
+        assertNotSame(map.get("k"), ((Map<?, ?>) clone.get("map")).get("k"));
+        assertNotSame(bytes, clone.get("bytes"));
+        assertNotSame(ints, clone.get("ints"));
+        assertNotSame(date, clone.get("date"));
+        assertNotSame(sorted, clone.get("sorted"));
+        assertSame("immutable values are shared", original.get("name"), clone.get("name"));
+
+        // mutating the clone leaves the original untouched
+        ((List<Object>) clone.get("tags")).add("c");
+        ((Document) ((List<?>) clone.get("docs")).get(0)).put("n", 2);
+        ((List<Object>) ((Map<?, ?>) clone.get("map")).get("k")).clear();
+        ((byte[]) clone.get("bytes"))[0] = 9;
+        ((int[]) clone.get("ints"))[0] = 9;
+        ((Date) clone.get("date")).setTime(2000L);
+        ((List<Object>) clone.get("fixed")).add("r");
+        assertEquals(Arrays.asList("a", "b"), tags);
+        assertEquals(1, embedded.get("n"));
+        assertEquals(Collections.singletonList("v"), map.get("k"));
+        assertEquals(1, bytes[0]);
+        assertEquals(1, ints[0]);
+        assertEquals(1000L, date.getTime());
+        assertEquals(2, ((List<?>) original.get("fixed")).size());
+
+        // container types and comparators survive the copy
+        assertTrue(clone.get("tags") instanceof LinkedList);
+        assertTrue(clone.get("map") instanceof HashMap);
+        SortedSet<?> sortedCopy = (SortedSet<?>) clone.get("sorted");
+        assertEquals("y", sortedCopy.first());
+        assertNotNull(sortedCopy.comparator());
+    }
+
     @Test
     public void testContainsKey() {
         assertFalse((new NitriteDocument()).containsKey("key"));