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 1841203) +++ 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 1841203) +++ 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 1841203) +++ 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 1841203) +++ oak-store-document/src/main/java/org/apache/jackrabbit/oak/plugins/document/mongo/MongoDocumentStore.java (working copy) @@ -982,6 +982,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 +1004,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; } @@ -1216,6 +1219,7 @@ MongoCollection dbCollection = getDBCollection(collection); List> writes = new ArrayList<>(updateOps.size()); String[] bulkIds = new String[updateOps.size()]; + Set failedUpdates = new HashSet(); int i = 0; for (UpdateOp updateOp : updateOps) { String id = updateOp.getId(); @@ -1223,16 +1227,28 @@ 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, true), new UpdateOptions().upsert(updateOp.isNew()))); + if (!updateOp.isNew()) { + // the update operation will not create the document if it + // doesn't exist, but it also doesn't fail in that case. + // we'll forcefully have to mark it as failed + failedUpdates.add(id); + } } else { - query = Filters.and(query, Filters.eq(Document.MOD_COUNT, oldDoc.getModCount())); - writes.add(new UpdateOneModel<>(query, createUpdate(updateOp, false), new UpdateOptions().upsert(true))); + if (updateOp.isNew()) { + // force an upsert + query = Filters.and(query, Filters.exists(Document.MOD_COUNT, false)); + writes.add(new UpdateOneModel<>(query, createUpdate(updateOp, true), new UpdateOptions().upsert(true))); + } else { + // TODO: how to prevent upsert when document doesn't exist anymore + query = Filters.and(query, Filters.eq(Document.MOD_COUNT, oldDoc.getModCount())); + writes.add(new UpdateOneModel<>(query, createUpdate(updateOp, false), new UpdateOptions().upsert(true))); + } } bulkIds[i++] = id; } BulkWriteResult bulkResult; - Set failedUpdates = new HashSet(); Set upserts = new HashSet(); BulkWriteOptions options = new BulkWriteOptions().ordered(false); try { 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 1841203) +++ 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 @@ -567,7 +567,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 +1538,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 +1572,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 +1617,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 1841203) +++ 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 1841203) +++ 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 1841203) +++ 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 1841203) +++ 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()); + } + } + } }