Index: oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentStore.java =================================================================== --- oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentStore.java (revision 1841462) +++ oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentStore.java (working copy) @@ -257,7 +257,9 @@ /** * Atomically checks if the document exists and updates it, otherwise the - * document is created (aka upsert). The returned document is immutable. + * document is created (aka "upsert"), unless the update operation requires + * the document to be present (see {@link UpdateOp#isNew()}). The returned + * document is immutable. *

* If this method fails with a {@code DocumentStoreException}, then the * document may or may not have been created or updated. It is the @@ -267,13 +269,20 @@ * cache. That is, an implementation could simply evict documents with the * given keys from the cache. * - * @param the document type - * @param collection the collection - * @param update the update operation (where {@link Condition}s are not allowed) - * @return the old document or null if it didn't exist before. - * @throws IllegalArgumentException when the {@linkplain UpdateOp} is conditional - * @throws DocumentStoreException if the operation failed. E.g. because of - * an I/O error. + * @param + * the document type + * @param collection + * the collection + * @param update + * the update operation (where {@link Condition}s are not + * allowed) + * @return the old document or {@code null} if it either didn't exist + * before, or the {@linkplain UpdateOp} required the document to be + * present but {@link UpdateOp#isNew()} was {@code false}. + * @throws IllegalArgumentException + * when the {@linkplain UpdateOp} is conditional + * @throws DocumentStoreException + * if the operation failed. E.g. because of an I/O error. */ @Nullable T createOrUpdate(Collection collection, Index: oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/RecoveryLock.java =================================================================== --- oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/RecoveryLock.java (revision 1841462) +++ oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/RecoveryLock.java (working copy) @@ -108,7 +108,7 @@ } ClusterNodeInfoDocument old = store.findAndUpdate(CLUSTER_NODES, update); if (old == null) { - throw new RuntimeException("ClusterNodeInfo document for " + clusterId + " missing."); + throw new DocumentStoreException("ClusterNodeInfo document for " + clusterId + " does not exist."); } LOG.info("Released recovery lock for cluster id {} (recovery successful: {})", clusterId, success); Index: oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/memory/MemoryDocumentStore.java =================================================================== --- oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/memory/MemoryDocumentStore.java (revision 1841462) +++ oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/memory/MemoryDocumentStore.java (working copy) @@ -327,7 +327,7 @@ T doc = collection.newDocument(this); if (oldDoc == null) { if (!update.isNew()) { - throw new DocumentStoreException("Document does not exist: " + update.getId()); + return null; } } else { oldDoc.deepCopy(doc); Index: oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java =================================================================== --- oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java (revision 1841462) +++ oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java (working copy) @@ -140,6 +140,9 @@ public static final int IN_CLAUSE_BATCH_SIZE = 500; + private static final BasicDBObject CONFLICTING_ID + = new BasicDBObject(Document.ID, "a").append(Document.ID, "b"); + private MongoCollection nodes; private final MongoCollection clusterNodes; private final MongoCollection settings; @@ -891,7 +894,7 @@ MongoCollection dbCollection = getDBCollection(collection); // make sure we don't modify the original updateOp updateOp = updateOp.copy(); - Bson update = createUpdate(updateOp, false); + Bson update = createUpdate(updateOp, !upsert); Lock lock = null; if (collection == Collection.NODES) { @@ -959,7 +962,7 @@ } }); - if (oldNode == null){ + if (oldNode == null && upsert) { newEntry = true; } @@ -982,6 +985,9 @@ } else { // updateOp without conditions and not an upsert // this means the document does not exist + if (collection == Collection.NODES) { + nodesCache.invalidate(updateOp.getId()); + } } return oldDoc; } catch (Exception e) { @@ -1001,7 +1007,7 @@ throws DocumentStoreException { log("createOrUpdate", update); UpdateUtils.assertUnconditional(update); - T doc = findAndModify(collection, update, true, false); + T doc = findAndModify(collection, update, update.isNew(), false); log("createOrUpdate returns ", doc); return doc; } @@ -1220,13 +1226,21 @@ for (UpdateOp updateOp : updateOps) { String id = updateOp.getId(); Bson query = createQueryForUpdate(id, updateOp.getConditions()); + // fail on insert when isNew == false + boolean failInsert = !updateOp.isNew(); T oldDoc = oldDocs.get(id); if (oldDoc == null || oldDoc == NodeDocument.NULL) { query = Filters.and(query, Filters.exists(Document.MOD_COUNT, false)); - writes.add(new UpdateOneModel<>(query, createUpdate(updateOp, true), new UpdateOptions().upsert(true))); + writes.add(new UpdateOneModel<>(query, + createUpdate(updateOp, failInsert), + new UpdateOptions().upsert(true)) + ); } else { query = Filters.and(query, Filters.eq(Document.MOD_COUNT, oldDoc.getModCount())); - writes.add(new UpdateOneModel<>(query, createUpdate(updateOp, false), new UpdateOptions().upsert(true))); + writes.add(new UpdateOneModel<>(query, + createUpdate(updateOp, failInsert), + new UpdateOptions().upsert(true)) + ); } bulkIds[i++] = id; } @@ -1631,11 +1645,11 @@ * Creates a MongoDB update object from the given UpdateOp. * * @param updateOp the update op. - * @param includeId whether to include the SET id operation + * @param failOnInsert whether to create an update that will fail on insert. * @return the DBObject. */ - @NotNull - private static BasicDBObject createUpdate(UpdateOp updateOp, boolean includeId) { + private static BasicDBObject createUpdate(UpdateOp updateOp, + boolean failOnInsert) { BasicDBObject setUpdates = new BasicDBObject(); BasicDBObject maxUpdates = new BasicDBObject(); BasicDBObject incUpdates = new BasicDBObject(); @@ -1643,9 +1657,6 @@ // always increment modCount updateOp.increment(Document.MOD_COUNT, 1); - if (includeId) { - setUpdates.append(Document.ID, updateOp.getId()); - } // other updates for (Entry entry : updateOp.getChanges().entrySet()) { @@ -1687,6 +1698,10 @@ update.append("$unset", unsetUpdates); } + if (failOnInsert) { + update.append("$setOnInsert", CONFLICTING_ID); + } + return update; } Index: oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java =================================================================== --- oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java (revision 1841462) +++ oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/rdb/RDBDocumentStore.java (working copy) @@ -359,7 +359,7 @@ @Override public T createOrUpdate(Collection collection, UpdateOp update) { UpdateUtils.assertUnconditional(update); - return internalCreateOrUpdate(collection, update, true, false); + return internalCreateOrUpdate(collection, update, update.isNew(), false, RETRIES); } @Override @@ -521,11 +521,15 @@ for (UpdateOp update : updates) { String id = update.getId(); T modifiedDoc = collection.newDocument(this); - if (oldDocs.containsKey(id)) { - oldDocs.get(id).deepCopy(modifiedDoc); + T oldDoc = oldDocs.get(id); + if (oldDoc != null) { + oldDoc.deepCopy(modifiedDoc); } UpdateUtils.applyChanges(modifiedDoc, update); - docsToUpdate.add(modifiedDoc); + if (oldDoc != null || update.isNew()) { + // only create if updateOp allows it + docsToUpdate.add(modifiedDoc); + } keysToUpdate.add(id); } @@ -567,7 +571,7 @@ @Override public T findAndUpdate(Collection collection, UpdateOp update) { - return internalCreateOrUpdate(collection, update, false, true); + return internalCreateOrUpdate(collection, update, false, true, RETRIES); } @Override @@ -1538,14 +1542,12 @@ @Nullable private T internalCreateOrUpdate(Collection collection, UpdateOp update, boolean allowCreate, - boolean checkConditions) { + boolean checkConditions, int retries) { T oldDoc = readDocumentCached(collection, update.getId(), Integer.MAX_VALUE); if (oldDoc == null) { - if (!allowCreate) { + if (!allowCreate || !update.isNew()) { return null; - } else if (!update.isNew()) { - throw new DocumentStoreException("Document does not exist: " + update.getId()); } T doc = collection.newDocument(this); if (checkConditions && !checkConditions(doc, update.getConditions())) { @@ -1574,15 +1576,19 @@ LOG.error("insert failed, but document " + update.getId() + " is not present, aborting", ex); throw (ex); } - return internalUpdate(collection, update, oldDoc, checkConditions, RETRIES); + return internalUpdate(collection, update, oldDoc, checkConditions, retries); } } else { - T result = internalUpdate(collection, update, oldDoc, checkConditions, RETRIES); + T result = internalUpdate(collection, update, oldDoc, checkConditions, retries); if (allowCreate && result == null) { - // TODO OAK-2655 need to implement some kind of retry - LOG.error("update of " + update.getId() + " failed, race condition?"); - throw new DocumentStoreException("update of " + update.getId() + " failed, race condition?", null, - DocumentStoreException.Type.TRANSIENT); + if (retries > 0) { + result = internalCreateOrUpdate(collection, update, allowCreate, checkConditions, retries - 1); + } + else { + LOG.error("update of " + update.getId() + " failed, race condition?"); + throw new DocumentStoreException("update of " + update.getId() + " failed, race condition?", null, + DocumentStoreException.Type.TRANSIENT); + } } return result; } @@ -1615,6 +1621,9 @@ if (lastmodcount == newmodcount) { // cached copy did not change so it probably was // updated by a different instance, get a fresh one + if (collection == Collection.NODES) { + nodesCache.invalidate(update.getId()); + } oldDoc = readDocumentUncached(collection, update.getId(), null); } } Index: oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/BasicDocumentStoreTest.java =================================================================== --- oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/BasicDocumentStoreTest.java (revision 1841462) +++ oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/BasicDocumentStoreTest.java (working copy) @@ -111,11 +111,11 @@ assertNull(nd.get(NodeDocument.HAS_BINARY_FLAG)); assertFalse(nd.wasDeletedOnce()); assertFalse(nd.hasBinary()); - + up = new UpdateOp(id, false); up.set(NodeDocument.DELETED_ONCE, true); super.ds.findAndUpdate(Collection.NODES, up); - + super.ds.invalidateCache(); nd = super.ds.find(Collection.NODES, id, 0); assertEquals(true, nd.get(NodeDocument.DELETED_ONCE)); @@ -127,7 +127,7 @@ up.set(NodeDocument.DELETED_ONCE, false); up.set(NodeDocument.HAS_BINARY_FLAG, NodeDocument.HAS_BINARY_VAL); super.ds.findAndUpdate(Collection.NODES, up); - + super.ds.invalidateCache(); nd = super.ds.find(Collection.NODES, id, 0); assertEquals(false, nd.get(NodeDocument.DELETED_ONCE)); @@ -134,13 +134,13 @@ assertEquals(NodeDocument.HAS_BINARY_VAL, nd.get(NodeDocument.HAS_BINARY_FLAG)); assertFalse(nd.wasDeletedOnce()); assertTrue(nd.hasBinary()); - + // remove up = new UpdateOp(id, false); up.remove(NodeDocument.DELETED_ONCE); up.remove(NodeDocument.HAS_BINARY_FLAG); super.ds.findAndUpdate(Collection.NODES, up); - + super.ds.invalidateCache(); nd = super.ds.find(Collection.NODES, id, 0); assertNull(nd.get(NodeDocument.DELETED_ONCE)); @@ -162,11 +162,7 @@ public void testCreateOrUpdate() { String id = this.getClass().getName() + ".testCreateOrUpdate"; - // remove if present - NodeDocument nd = super.ds.find(Collection.NODES, id); - if (nd != null) { - super.ds.remove(Collection.NODES, id); - } + super.ds.remove(Collection.NODES, id); // create UpdateOp up = new UpdateOp(id, true); @@ -179,6 +175,21 @@ } @Test + public void testCreateOrUpdateIsNewFalse() { + String id = this.getClass().getName() + ".testCreateOrUpdateIsNewFalse"; + + super.ds.remove(Collection.NODES, id); + + // create with isNew==false + UpdateOp up = new UpdateOp(id, false); + assertNull(super.ds.createOrUpdate(Collection.NODES, up)); + + // has the document been created? + NodeDocument nd = super.ds.find(Collection.NODES, id); + assertNull(nd); + } + + @Test public void testCreateOrUpdateWithoutIdInUpdateOp() { String id = this.getClass().getName() + ".testCreateOrUpdateWithoutIdInUpdateOp"; Index: oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/BulkCreateOrUpdateTest.java =================================================================== --- oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/BulkCreateOrUpdateTest.java (revision 1841462) +++ oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/BulkCreateOrUpdateTest.java (working copy) @@ -350,4 +350,68 @@ assertEquals("The value is not correct", 100l, newDoc.get("prop_" + i)); } } + + @Test + public void testBulkCreateOrUpdateIsNewFalse() { + bulkCreateOrUpdateIsNewFalse(2); + } + + @Test + public void testBulkCreateOrUpdateIsNewFalseMany() { + bulkCreateOrUpdateIsNewFalse(10); + } + + private void bulkCreateOrUpdateIsNewFalse(int numUpdates) { + String id1 = this.getClass().getName() + ".testBulkCreateOrUpdateIsNewFalse"; + List ids = new ArrayList<>(); + for (int i = 1; i < numUpdates; i++) { + ids.add(id1 + "b" + i); + } + + removeMe.add(id1); + removeMe.addAll(ids); + + // remove id1 + super.ds.remove(Collection.NODES, id1); + List initial = new ArrayList<>(); + // insert other ids + for (String id : ids) { + UpdateOp up = new UpdateOp(id, true); + assertNull(super.ds.createOrUpdate(Collection.NODES, up)); + NodeDocument doc = super.ds.find(Collection.NODES, id); + assertNotNull(doc); + initial.add(doc); + } + + // bulk update + List ops = new ArrayList<>(); + ops.add(new UpdateOp(id1, false)); + for (String id : ids) { + ops.add(new UpdateOp(id, false)); + } + + List result = super.ds.createOrUpdate(Collection.NODES, ops); + assertEquals(numUpdates, result.size()); + + // id1 result should be reported as null and not be created + assertNull(result.get(0)); + assertNull(ds.find(Collection.NODES, id1)); + + // for other ids result should be reported with previous doc + for (int i = 1; i < numUpdates; i++) { + NodeDocument prev = result.get(i); + assertNotNull(prev); + String id = ids.get(i - 1); + assertEquals(id, prev.getId()); + if (prev.getModCount() != null) { + assertEquals(initial.get(i - 1).getModCount(), prev.getModCount()); + } + + NodeDocument updated = super.ds.find(Collection.NODES, id); + assertNotNull(updated); + if (prev.getModCount() != null) { + assertNotEquals(updated.getModCount(), prev.getModCount()); + } + } + } } Index: oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/CommitTest.java =================================================================== --- oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/CommitTest.java (revision 1841462) +++ oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/CommitTest.java (working copy) @@ -124,7 +124,7 @@ try { c.apply(); fail("commit must fail"); - } catch (DocumentStoreException e) { + } catch (ConflictException e) { // expected assertTrue("Unexpected exception message: " + e.getMessage(), e.getMessage().contains("does not exist")); Index: oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/MultiDocumentStoreTest.java =================================================================== --- oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/MultiDocumentStoreTest.java (revision 1841462) +++ oak-store-document/src/test/java/org/apache/jackrabbit/oak/plugins/document/MultiDocumentStoreTest.java (working copy) @@ -18,6 +18,7 @@ import static java.util.Collections.synchronizedList; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotEquals; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.fail; @@ -28,12 +29,12 @@ import java.util.Map; import java.util.concurrent.CountDownLatch; +import org.apache.jackrabbit.oak.plugins.document.util.Utils; +import org.junit.Test; + import com.google.common.collect.Lists; import com.google.common.collect.Maps; -import org.apache.jackrabbit.oak.plugins.document.util.Utils; -import org.junit.Test; - public class MultiDocumentStoreTest extends AbstractMultiDocumentStoreTest { public MultiDocumentStoreTest(DocumentStoreFixture dsf) { @@ -225,4 +226,114 @@ assertEquals(2L, foo.get("_ds1")); assertEquals(1L, foo.get("_ds2")); } + + @Test + public void testUpdateOrCreateDeletedDocument() { + + String id = Utils.getIdFromPath("/foo"); + removeMe.add(id); + + UpdateOp op1 = new UpdateOp(id, true); + assertNull(ds1.createOrUpdate(Collection.NODES, op1)); + + NodeDocument d = ds1.find(Collection.NODES, id); + assertNotNull(d); + Long mc = d.getModCount(); + + ds2.remove(Collection.NODES, id); + + UpdateOp op2 = new UpdateOp(id, true); + NodeDocument prev = ds1.createOrUpdate(Collection.NODES, op2); + + assertNull(prev); + + // make sure created + NodeDocument created = ds1.find(Collection.NODES, id); + assertNotNull(created); + assertEquals(mc, created.getModCount()); + } + + @Test + public void testUpdateNoCreateDeletedDocument() { + String id = Utils.getIdFromPath("/foo"); + removeMe.add(id); + + UpdateOp op1 = new UpdateOp(id, true); + assertNull(ds1.createOrUpdate(Collection.NODES, op1)); + + NodeDocument d = ds1.find(Collection.NODES, id); + assertNotNull(d); + + ds2.remove(Collection.NODES, id); + + UpdateOp op2 = new UpdateOp(id, false); + NodeDocument prev = ds1.createOrUpdate(Collection.NODES, op2); + + assertNull(prev); + + // make sure not created + assertNull(ds1.find(Collection.NODES, id)); + } + + @Test + public void batchUpdateNoCreateDeletedDocument() { + batchUpdateNoCreateDeletedDocument(2); + } + + @Test + public void batchUpdateNoCreateDeletedDocumentMany() { + batchUpdateNoCreateDeletedDocument(10); + } + + private void batchUpdateNoCreateDeletedDocument(int numUpdates) { + String id1 = this.getClass().getName() + ".batchUpdateNoCreateDeletedDocument"; + List ids = new ArrayList<>(); + for (int i = 1; i < numUpdates; i++) { + ids.add(id1 + "b" + i); + } + + removeMe.add(id1); + removeMe.addAll(ids); + + List allIds = new ArrayList<>(ids); + allIds.add(id1); + // create docs + for (String id : allIds) { + UpdateOp up = new UpdateOp(id, true); + assertNull(ds1.createOrUpdate(Collection.NODES, up)); + NodeDocument doc = ds1.find(Collection.NODES, id); + assertNotNull(doc); + } + + // remove id1 on ds2 + ds2.remove(Collection.NODES, id1); + + // bulk update + List ops = new ArrayList<>(); + ops.add(new UpdateOp(id1, false)); + for (String id : ids) { + ops.add(new UpdateOp(id, false)); + } + + List result = ds1.createOrUpdate(Collection.NODES, ops); + assertEquals(numUpdates, result.size()); + + // id1 result should be reported as null and not be created + assertNull(result.get(0)); + assertNull(ds1.find(Collection.NODES, id1)); + + // for other ids result should be reported with previous doc + for (int i = 1; i < numUpdates; i++) { + NodeDocument prev = result.get(i); + assertNotNull(prev); + String id = ids.get(i - 1); + assertEquals(id, prev.getId()); + + NodeDocument updated = ds1.find(Collection.NODES, id); + assertNotNull(updated); + if (prev.getModCount() != null) { + assertNotEquals(updated.getModCount(), prev.getModCount()); + } + } + } }