diff --git a/nitrite/src/main/java/org/dizitart/no2/collection/NitriteDocument.java b/nitrite/src/main/java/org/dizitart/no2/collection/NitriteDocument.java index f5dc5823..a1550ed3 100644 --- a/nitrite/src/main/java/org/dizitart/no2/collection/NitriteDocument.java +++ b/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. + *

+ * 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. + *

+ * 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 cloned = (Map) super.clone(); - - // create the clone of any embedded documents as well for (Map.Entry 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) value); + } + if (value instanceof Map) { + return deepCopyMap((Map) 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 deepCopyCollection(Collection source) { + Collection copy; + if (source instanceof SortedSet) { + // a fresh instance of the same class would lose the comparator + copy = new TreeSet<>(((SortedSet) 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 deepCopyMap(Map source) { + Map copy; + if (source instanceof SortedMap) { + copy = new TreeMap<>(((SortedMap) 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 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 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 diff --git a/nitrite/src/main/java/org/dizitart/no2/collection/operation/ReadOperations.java b/nitrite/src/main/java/org/dizitart/no2/collection/operation/ReadOperations.java index 7d9c838e..42332eec 100644 --- a/nitrite/src/main/java/org/dizitart/no2/collection/operation/ReadOperations.java +++ b/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) { diff --git a/nitrite/src/test/java/org/dizitart/no2/collection/CopyOnReadTest.java b/nitrite/src/test/java/org/dizitart/no2/collection/CopyOnReadTest.java new file mode 100644 index 00000000..d7dde9f8 --- /dev/null +++ b/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) found.get("tags")).add("c"); + ((List) 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) 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()); + } +} diff --git a/nitrite/src/test/java/org/dizitart/no2/collection/NitriteDocumentTest.java b/nitrite/src/test/java/org/dizitart/no2/collection/NitriteDocumentTest.java index 0faf37cf..8d9beaf6 100644 --- a/nitrite/src/test/java/org/dizitart/no2/collection/NitriteDocumentTest.java +++ b/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 tags = new LinkedList<>(Arrays.asList("a", "b")); + NitriteDocument embedded = new NitriteDocument(); + embedded.put("n", 1); + List embeddedList = new ArrayList<>(Collections.singletonList(embedded)); + Map 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 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) clone.get("tags")).add("c"); + ((Document) ((List) clone.get("docs")).get(0)).put("n", 2); + ((List) ((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) 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"));