From 4ac9908acf88c1d250978a63a52fea438edbdef5 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Wed, 30 Jan 2019 15:27:51 +0100 Subject: [PATCH 1/6] DATAMONGO-2195 - Prepare issue branch. --- pom.xml | 2 +- spring-data-mongodb-benchmarks/pom.xml | 2 +- spring-data-mongodb-cross-store/pom.xml | 4 ++-- spring-data-mongodb-distribution/pom.xml | 2 +- spring-data-mongodb/pom.xml | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pom.xml b/pom.xml index 657607148e..3a0995dc62 100644 --- a/pom.xml +++ b/pom.xml @@ -5,7 +5,7 @@ org.springframework.data spring-data-mongodb-parent - 2.2.0.BUILD-SNAPSHOT + 2.2.0.DATAMONGO-2195-SNAPSHOT pom Spring Data MongoDB diff --git a/spring-data-mongodb-benchmarks/pom.xml b/spring-data-mongodb-benchmarks/pom.xml index c2ff37b35c..2a407e3243 100644 --- a/spring-data-mongodb-benchmarks/pom.xml +++ b/spring-data-mongodb-benchmarks/pom.xml @@ -7,7 +7,7 @@ org.springframework.data spring-data-mongodb-parent - 2.2.0.BUILD-SNAPSHOT + 2.2.0.DATAMONGO-2195-SNAPSHOT ../pom.xml diff --git a/spring-data-mongodb-cross-store/pom.xml b/spring-data-mongodb-cross-store/pom.xml index fd36f227c0..b3b7b7ff34 100644 --- a/spring-data-mongodb-cross-store/pom.xml +++ b/spring-data-mongodb-cross-store/pom.xml @@ -6,7 +6,7 @@ org.springframework.data spring-data-mongodb-parent - 2.2.0.BUILD-SNAPSHOT + 2.2.0.DATAMONGO-2195-SNAPSHOT ../pom.xml @@ -50,7 +50,7 @@ org.springframework.data spring-data-mongodb - 2.2.0.BUILD-SNAPSHOT + 2.2.0.DATAMONGO-2195-SNAPSHOT diff --git a/spring-data-mongodb-distribution/pom.xml b/spring-data-mongodb-distribution/pom.xml index fc8d28a2b6..917730a3aa 100644 --- a/spring-data-mongodb-distribution/pom.xml +++ b/spring-data-mongodb-distribution/pom.xml @@ -14,7 +14,7 @@ org.springframework.data spring-data-mongodb-parent - 2.2.0.BUILD-SNAPSHOT + 2.2.0.DATAMONGO-2195-SNAPSHOT ../pom.xml diff --git a/spring-data-mongodb/pom.xml b/spring-data-mongodb/pom.xml index f3c85a046a..8231920bf1 100644 --- a/spring-data-mongodb/pom.xml +++ b/spring-data-mongodb/pom.xml @@ -11,7 +11,7 @@ org.springframework.data spring-data-mongodb-parent - 2.2.0.BUILD-SNAPSHOT + 2.2.0.DATAMONGO-2195-SNAPSHOT ../pom.xml From 21f7e997e0f3d5b496641e049e8617265d98ebc3 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 31 Jan 2019 13:03:48 +0100 Subject: [PATCH 2/6] DATAMONGO-2196 - Remove applies WriteConcern to single Document delete operations. We now make sure to apply the WriteConcern correctly when calling deleteOne on MongoCollection. --- .../data/mongodb/core/MongoTemplate.java | 2 +- .../mongodb/core/MongoTemplateUnitTests.java | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java index 94fe71d4ca..7ed5a18831 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java @@ -1721,7 +1721,7 @@ public DeleteResult doInCollection(MongoCollection collection) : collection; DeleteResult result = multi ? collectionToUse.deleteMany(removeQuery, options) - : collection.deleteOne(removeQuery, options); + : collectionToUse.deleteOne(removeQuery, options); maybeEmitEvent(new AfterDeleteEvent<>(queryObject, entityClass, collectionName)); diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateUnitTests.java index 480dbebc00..41699e6414 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateUnitTests.java @@ -83,6 +83,7 @@ import com.mongodb.MongoException; import com.mongodb.MongoNamespace; import com.mongodb.ReadPreference; +import com.mongodb.WriteConcern; import com.mongodb.client.AggregateIterable; import com.mongodb.client.FindIterable; import com.mongodb.client.MapReduceIterable; @@ -96,6 +97,7 @@ import com.mongodb.client.model.MapReduceAction; import com.mongodb.client.model.ReplaceOptions; import com.mongodb.client.model.UpdateOptions; +import com.mongodb.client.result.DeleteResult; import com.mongodb.client.result.UpdateResult; /** @@ -114,11 +116,13 @@ public class MongoTemplateUnitTests extends MongoOperationsUnitTests { @Mock MongoClient mongo; @Mock MongoDatabase db; @Mock MongoCollection collection; + @Mock MongoCollection collectionWithWriteConcern; @Mock MongoCursor cursor; @Mock FindIterable findIterable; @Mock AggregateIterable aggregateIterable; @Mock MapReduceIterable mapReduceIterable; @Mock UpdateResult updateResult; + @Mock DeleteResult deleteResult; Document commandResultDocument = new Document(); @@ -141,6 +145,8 @@ public void setUp() { when(collection.aggregate(any(List.class), any())).thenReturn(aggregateIterable); when(collection.withReadPreference(any())).thenReturn(collection); when(collection.replaceOne(any(), any(), any(ReplaceOptions.class))).thenReturn(updateResult); + when(collection.withWriteConcern(any())).thenReturn(collectionWithWriteConcern); + when(collectionWithWriteConcern.deleteOne(any(Bson.class), any())).thenReturn(deleteResult); when(findIterable.projection(any())).thenReturn(findIterable); when(findIterable.sort(any(org.bson.Document.class))).thenReturn(findIterable); when(findIterable.collation(any())).thenReturn(findIterable); @@ -731,6 +737,19 @@ public void findAndRemoveShouldUseCollationWhenPresent() { assertThat(options.getValue().getCollation().getLocale(), is("fr")); } + @Test // DATAMONGO-2196 + public void removeShouldApplyWriteConcern() { + + Person person = new Person(); + person.id = "id-1"; + + template.setWriteConcern(WriteConcern.UNACKNOWLEDGED); + template.remove(person); + + verify(collection).withWriteConcern(eq(WriteConcern.UNACKNOWLEDGED)); + verify(collectionWithWriteConcern).deleteOne(any(Bson.class), any()); + } + @Test // DATAMONGO-1518 public void findAndRemoveManyShouldUseCollationWhenPresent() { From d68636fb2b8ce7f9a78502c901a29c6da25674c5 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Mon, 4 Feb 2019 12:48:02 +0100 Subject: [PATCH 3/6] DATAMONGO-2195 - Consider version when removing an entity. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We now consider a potential @Version when removing an entity. MongoOperations#remove(Object) and MongoOperations#remove(Object, String) new throw an OptimisticLockingFailureException when the object to remove is versioned and the version of the given entity does not match the one within the store. If no document with matching _id can be found the remove operation passes without an error. MongoRepository now also considers the entities version, following the very same logic as MongoOperations. To remove an entity without version check use MongoOperations#remove(Query,…) or MongoRepository#deleteById(… --- .../data/mongodb/core/EntityOperations.java | 19 +- .../data/mongodb/core/MongoOperations.java | 14 +- .../data/mongodb/core/MongoTemplate.java | 47 ++++- .../mongodb/core/ReactiveMongoTemplate.java | 56 ++++-- .../support/SimpleMongoRepository.java | 2 +- .../SimpleReactiveMongoRepository.java | 2 +- .../data/mongodb/core/MongoTemplateTests.java | 58 ++++++- .../core/ReactiveMongoTemplateTests.java | 72 ++++++++ ...ReactiveMongoTemplateTransactionTests.java | 50 ++++++ .../mongodb/repository/VersionedPerson.java | 47 +++++ ...leMongoRepositoryVersionedEntityTests.java | 162 ++++++++++++++++++ src/main/asciidoc/reference/mongodb.adoc | 5 + 12 files changed, 512 insertions(+), 22 deletions(-) create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/VersionedPerson.java create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java index 7560b5bdef..ae32a2cbce 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java @@ -199,6 +199,21 @@ interface Entity { */ Query getByIdQuery(); + /** + * Returns the {@link Query} to remove an entity by its {@literal id} and if applicable {@literal version}. + * + * @return the {@link Query} to use for removing the entity. Never {@literal null}. + * @since 2.2 + */ + default Query getRemoveByQuery() { + + if (isVersionedEntity()) { + return getQueryForVersion(); + } + + return getByIdQuery(); + } + /** * Returns the {@link Query} to find the entity in its current version. * @@ -490,10 +505,10 @@ public Query getByIdQuery() { public Query getQueryForVersion() { MongoPersistentProperty idProperty = entity.getRequiredIdProperty(); - MongoPersistentProperty property = entity.getRequiredVersionProperty(); + MongoPersistentProperty versionProperty = entity.getRequiredVersionProperty(); return new Query(Criteria.where(idProperty.getName()).is(getId())// - .and(property.getName()).is(getVersion())); + .and(versionProperty.getName()).is(getVersion())); } /* diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoOperations.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoOperations.java index 2d66986d60..89f693d152 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoOperations.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoOperations.java @@ -1350,19 +1350,29 @@ T findAndReplace(Query query, S replacement, FindAndReplaceOptions option UpdateResult updateMulti(Query query, Update update, Class entityClass, String collectionName); /** - * Remove the given object from the collection by id. + * Remove the given object from the collection by {@literal id} and (if applicable) its + * {@link org.springframework.data.annotation.Version}. * * @param object must not be {@literal null}. * @return the {@link DeleteResult} which lets you access the results of the previous delete. + * @throws org.springframework.dao.OptimisticLockingFailureException if the given {@literal object} is + * {@link org.springframework.data.annotation.Version versioned} and the current version does not match the + * one within the store. If no document with matching {@literal _id} exists in the collection the operation + * completes without error. */ DeleteResult remove(Object object); /** - * Removes the given object from the given collection. + * Removes the given object from the given collection by {@literal id} and (if applicable) its + * {@link org.springframework.data.annotation.Version}. * * @param object must not be {@literal null}. * @param collectionName name of the collection where the objects will removed, must not be {@literal null} or empty. * @return the {@link DeleteResult} which lets you access the results of the previous delete. + * @throws org.springframework.dao.OptimisticLockingFailureException if the given {@literal object} is + * {@link org.springframework.data.annotation.Version versioned} and the current version does not match the + * one within the store. If no document with matching {@literal _id} exists in the collection the operation + * completes without error. */ DeleteResult remove(Object object, String collectionName); diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java index 7ed5a18831..5e993557de 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java @@ -68,7 +68,16 @@ import org.springframework.data.mongodb.core.aggregation.Fields; import org.springframework.data.mongodb.core.aggregation.TypeBasedAggregationOperationContext; import org.springframework.data.mongodb.core.aggregation.TypedAggregation; -import org.springframework.data.mongodb.core.convert.*; +import org.springframework.data.mongodb.core.convert.DbRefResolver; +import org.springframework.data.mongodb.core.convert.DefaultDbRefResolver; +import org.springframework.data.mongodb.core.convert.JsonSchemaMapper; +import org.springframework.data.mongodb.core.convert.MappingMongoConverter; +import org.springframework.data.mongodb.core.convert.MongoConverter; +import org.springframework.data.mongodb.core.convert.MongoCustomConversions; +import org.springframework.data.mongodb.core.convert.MongoJsonSchemaMapper; +import org.springframework.data.mongodb.core.convert.MongoWriter; +import org.springframework.data.mongodb.core.convert.QueryMapper; +import org.springframework.data.mongodb.core.convert.UpdateMapper; import org.springframework.data.mongodb.core.index.IndexOperations; import org.springframework.data.mongodb.core.index.IndexOperationsProvider; import org.springframework.data.mongodb.core.index.MongoMappingEventPublisher; @@ -1638,9 +1647,7 @@ public DeleteResult remove(Object object) { Assert.notNull(object, "Object must not be null!"); - Query query = operations.forEntity(object).getByIdQuery(); - - return remove(query, object.getClass()); + return remove(object, operations.determineCollectionName(object.getClass())); } @Override @@ -1649,7 +1656,7 @@ public DeleteResult remove(Object object, String collectionName) { Assert.notNull(object, "Object must not be null!"); Assert.hasText(collectionName, "Collection name must not be null or empty!"); - Query query = operations.forEntity(object).getByIdQuery(); + Query query = operations.forEntity(object).getRemoveByQuery(); return doRemove(collectionName, query, object.getClass(), false); } @@ -1723,10 +1730,40 @@ public DeleteResult doInCollection(MongoCollection collection) DeleteResult result = multi ? collectionToUse.deleteMany(removeQuery, options) : collectionToUse.deleteOne(removeQuery, options); + checkForOptimisticLockingFailures(result, removeQuery, collectionToUse); + maybeEmitEvent(new AfterDeleteEvent<>(queryObject, entityClass, collectionName)); return result; } + + private void checkForOptimisticLockingFailures(DeleteResult result, Document removeQuery, + MongoCollection collectionToUse) { + + if (!multi && entity != null && entity.hasVersionProperty() && result.wasAcknowledged()) { + + if (result.getDeletedCount() == 0) { + + String versionFieldName = entity.getVersionProperty().getFieldName(); + Document idQuery = new Document(removeQuery); + idQuery.remove(versionFieldName); + + Iterator it = collectionToUse.find(idQuery) + .projection(Projections.include("_id", versionFieldName)).limit(1).iterator(); + + if (it.hasNext()) { + + Document source = it.next(); + + throw new OptimisticLockingFailureException(String + .format("The entity with id %s in %s has changed and cannot be deleted! " + System.lineSeparator() + // + "Expected version %s but was %s.", source.get("_id"), collectionName, removeQuery.get(versionFieldName), + source.get(versionFieldName))); + } + + } + } + } }); } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java index 73b9ea6b55..3b0f4db9ac 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java @@ -1717,7 +1717,7 @@ public Mono remove(Object object) { Assert.notNull(object, "Object must not be null!"); - return remove(operations.forEntity(object).getByIdQuery(), object.getClass()); + return remove(operations.forEntity(object).getRemoveByQuery(), object.getClass()); } /* @@ -1729,7 +1729,7 @@ public Mono remove(Object object, String collectionName) { Assert.notNull(object, "Object must not be null!"); Assert.hasText(collectionName, "Collection name must not be null or empty!"); - return doRemove(collectionName, operations.forEntity(object).getByIdQuery(), object.getClass()); + return doRemove(collectionName, operations.forEntity(object).getRemoveByQuery(), object.getClass()); } private void assertUpdateableIdIfNotSet(Object value) { @@ -1787,15 +1787,14 @@ protected Mono doRemove(String collectionName, Query query, @N Document queryObject = query.getQueryObject(); MongoPersistentEntity entity = getPersistentEntity(entityClass); + Document removeQuery = queryMapper.getMappedObject(queryObject, entity); return execute(collectionName, collection -> { - Document removeQuey = queryMapper.getMappedObject(queryObject, entity); - - maybeEmitEvent(new BeforeDeleteEvent<>(removeQuey, entityClass, collectionName)); + maybeEmitEvent(new BeforeDeleteEvent<>(removeQuery, entityClass, collectionName)); MongoAction mongoAction = new MongoAction(writeConcern, MongoActionOperation.REMOVE, collectionName, entityClass, - null, removeQuey); + null, removeQuery); DeleteOptions deleteOptions = new DeleteOptions(); query.getCollation().map(Collation::toMongoCollation).ifPresent(deleteOptions::collation); @@ -1805,13 +1804,13 @@ protected Mono doRemove(String collectionName, Query query, @N if (LOGGER.isDebugEnabled()) { LOGGER.debug("Remove using query: {} in collection: {}.", - new Object[] { serializeToJsonSafely(removeQuey), collectionName }); + new Object[] { serializeToJsonSafely(removeQuery), collectionName }); } if (query.getLimit() > 0 || query.getSkip() > 0) { FindPublisher cursor = new QueryFindPublisherPreparer(query, entityClass) - .prepare(collection.find(removeQuey)) // + .prepare(collection.find(removeQuery)) // .projection(MappedDocument.getIdOnlyProjection()); return Flux.from(cursor) // @@ -1822,10 +1821,47 @@ protected Mono doRemove(String collectionName, Query query, @N return collectionToUse.deleteMany(MappedDocument.getIdIn(val), deleteOptions); }); } else { - return collectionToUse.deleteMany(removeQuey, deleteOptions); + return collectionToUse.deleteMany(removeQuery, deleteOptions); + } + + }).flatMap(deleteResult -> { + + if (entity != null && entity.hasVersionProperty()) { + + if (deleteResult.wasAcknowledged() && deleteResult.getDeletedCount() == 0) { + + if (query.getQueryObject().containsKey(entity.getVersionProperty().getFieldName())) { + + return execute(collectionName, (coll -> { + + String versionFieldName = entity.getVersionProperty().getFieldName(); + + Document daQuery = new Document(removeQuery); + daQuery.remove(versionFieldName); + + Publisher publisher = coll.find(daQuery) + .projection(Projections.include("_id", entity.getVersionProperty().getFieldName())).limit(1).first(); + + return Mono.from(publisher) // + .defaultIfEmpty(new Document()) // + .flatMap(it -> { + + if (it.isEmpty()) { + return Mono.just(deleteResult); + } + return Mono.error(new OptimisticLockingFailureException(String.format( + "The entity with id %s in %s has changed and cannot be deleted! " + System.lineSeparator() + // + "Expected version %s but was %s.", it.get("_id"), collectionName, removeQuery.get(versionFieldName), + it.get(versionFieldName)))); + + }); + })).next(); + } + } } - }).doOnNext(deleteResult -> maybeEmitEvent(new AfterDeleteEvent<>(queryObject, entityClass, collectionName))) + return Mono.just(deleteResult); + }).doOnNext(it -> maybeEmitEvent(new AfterDeleteEvent<>(queryObject, entityClass, collectionName))) // .next(); } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepository.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepository.java index de52950c54..c2fc4935cd 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepository.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepository.java @@ -161,7 +161,7 @@ public void delete(T entity) { Assert.notNull(entity, "The given entity must not be null!"); - deleteById(entityInformation.getRequiredId(entity)); + mongoOperations.remove(entity, entityInformation.getCollectionName()); } /* diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepository.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepository.java index 6802a20c91..928255da15 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepository.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepository.java @@ -355,7 +355,7 @@ public Mono delete(T entity) { Assert.notNull(entity, "The given entity must not be null!"); - return deleteById(entityInformation.getRequiredId(entity)); + return mongoOperations.remove(entity, entityInformation.getCollectionName()).then(); } /* diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java index bb073c87c2..f78acc1e85 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java @@ -43,9 +43,9 @@ import java.util.stream.Collectors; import java.util.stream.IntStream; +import org.assertj.core.api.Assertions; import org.bson.types.ObjectId; import org.hamcrest.collection.IsMapContaining; -import org.hamcrest.core.IsSame; import org.joda.time.DateTime; import org.junit.After; import org.junit.Before; @@ -1646,6 +1646,62 @@ public void initializesVersionOnInsert() { assertThat(person.version, is(0)); } + @Test // DATAMONGO-2195 + public void removeEntityWithMatchingVersion() { + + PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); + person.firstName = "Dave"; + + template.insert(person); + assertThat(person.version, is(0)); + + template.remove(person); + assertThat(template.count(new Query(), Person.class)).isZero(); + } + + @Test // DATAMONGO-2195 + public void removeEntityWithoutMatchingVersionThrowsOptimisticLockingFailureException() { + + PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); + person.firstName = "Dave"; + + template.insert(person); + assertThat(person.version, is(0)); + template.update(PersonWithVersionPropertyOfTypeInteger.class).matching(query(where("id").is(person.id))) + .apply(new Update().set("firstName", "Walter")).first(); + + Assertions.assertThatThrownBy(() -> template.remove(person)).isInstanceOf(OptimisticLockingFailureException.class) + .hasMessageContaining("Expected version 0 but was 1"); + assertThat(template.count(new Query(), PersonWithVersionPropertyOfTypeInteger.class)).isOne(); + } + + @Test // DATAMONGO-2195 + public void removeNonExistingVersionedEntityJustPasses() { + + PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); + person.id = "id-1"; + person.firstName = "Dave"; + person.version = 10; + + assertThat(template.remove(person).getDeletedCount()).isZero(); + } + + @Test // DATAMONGO-2195 + @DirtiesContext + public void removeEntityWithoutMatchingVersionWhenUsingUnaknowledgedWriteDoesNotThrowsOptimisticLockingFailureException() { + + PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); + person.firstName = "Dave"; + + template.insert(person); + assertThat(person.version, is(0)); + template.update(PersonWithVersionPropertyOfTypeInteger.class).matching(query(where("id").is(person.id))) + .apply(new Update().set("firstName", "Walter")).first(); + + template.setWriteConcern(WriteConcern.UNACKNOWLEDGED); + assertThat(template.remove(person).wasAcknowledged()).isFalse(); + } + @Test // DATAMONGO-588 public void initializesVersionOnBatchInsert() { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTests.java index e1856704c7..6cd2b9dd2b 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTests.java @@ -704,6 +704,78 @@ public void removeWithMonoAndCollectionShouldDeleteElement() { StepVerifier.create(template.count(new Query(), Sample.class)).expectNext(2L).verifyComplete(); } + @Test // DATAMONGO-2195 + public void removeEntityWithMatchingVersion() { + + PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); + person.firstName = "Dave"; + + template.insert(person).as(StepVerifier::create).expectNextCount(1).verifyComplete(); + assertThat(person.version).isZero(); + + template.remove(person).as(StepVerifier::create).expectNextCount(1).verifyComplete(); + template.count(new Query(), PersonWithVersionPropertyOfTypeInteger.class) // + .as(StepVerifier::create) // + .expectNext(0L) // + .verifyComplete(); + } + + @Test // DATAMONGO-2195 + public void removeEntityWithoutMatchingVersionThrowsOptimisticLockingFailureException() { + + PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); + person.firstName = "Dave"; + + template.insert(person).as(StepVerifier::create).expectNextCount(1).verifyComplete(); + assertThat(person.version).isZero(); + template.update(PersonWithVersionPropertyOfTypeInteger.class).matching(query(where("id").is(person.id))) + .apply(new Update().set("firstName", "Walter")).first() // + .as(StepVerifier::create) // + .expectNextCount(1) // + .verifyComplete(); + + template.remove(person).as(StepVerifier::create).expectError(OptimisticLockingFailureException.class).verify(); + template.count(new Query(), PersonWithVersionPropertyOfTypeInteger.class) // + .as(StepVerifier::create) // + .expectNext(1L) // + .verifyComplete(); + } + + @Test // DATAMONGO-2195 + public void removeNonExistingVersionedEntityJustPasses() { + + PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); + person.id = "id-1"; + person.firstName = "Dave"; + person.version = 10; + + template.remove(person).as(StepVerifier::create).expectNextCount(1).verifyComplete(); + } + + @Test // DATAMONGO-2195 + @DirtiesContext + public void removeEntityWithoutMatchingVersionWhenUsingUnaknowledgedWriteDoesNotThrowsOptimisticLockingFailureException() { + + PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); + person.firstName = "Dave"; + + template.insert(person).as(StepVerifier::create).expectNextCount(1).verifyComplete(); + assertThat(person.version).isZero(); + template.update(PersonWithVersionPropertyOfTypeInteger.class).matching(query(where("id").is(person.id))) + .apply(new Update().set("firstName", "Walter")).first() // + .as(StepVerifier::create) // + .expectNextCount(1) // + .verifyComplete(); + + template.setWriteConcern(WriteConcern.UNACKNOWLEDGED); + + template.remove(person).as(StepVerifier::create).expectNextCount(1).verifyComplete(); + template.count(new Query(), PersonWithVersionPropertyOfTypeInteger.class) // + .as(StepVerifier::create) // + .expectNext(1L) // + .verifyComplete(); + } + @Test // DATAMONGO-1444 public void optimisticLockingHandling() { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTransactionTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTransactionTests.java index d98757d3ae..2a2bd13576 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTransactionTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTransactionTests.java @@ -31,6 +31,7 @@ import org.junit.Test; import org.junit.rules.TestRule; import org.reactivestreams.Publisher; +import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.data.domain.Sort; import org.springframework.data.mongodb.core.query.Query; import org.springframework.data.mongodb.test.util.MongoTestUtils; @@ -83,6 +84,12 @@ public void setUp() { .expectNext(Success.SUCCESS) // .verifyComplete(); + StepVerifier + .create( + MongoTestUtils.createOrReplaceCollection(DATABASE_NAME, "personWithVersionPropertyOfTypeInteger", client)) + .expectNext(Success.SUCCESS) // + .verifyComplete(); + StepVerifier.create(template.insert(DOCUMENT, COLLECTION_NAME)).expectNextCount(1).verifyComplete(); template.insertAll(Arrays.asList(AHMANN, ARLEN, LEESHA, RENNA)) // @@ -270,4 +277,47 @@ public void errorInFlowOutsideTransactionDoesNotAbortIt() { .expectNext(2L) // .verifyComplete(); } + + @Test // DATAMONGO-2195 + public void deleteWithMatchingVersion() { + + PersonWithVersionPropertyOfTypeInteger rojer = new PersonWithVersionPropertyOfTypeInteger(); + rojer.firstName = "rojer"; + + PersonWithVersionPropertyOfTypeInteger saved = template.insert(rojer).block(); + + template.inTransaction().execute(action -> action.remove(saved)) // + .as(StepVerifier::create) // + .consumeNextWith(result -> assertThat(result.getDeletedCount()).isOne()) // + .verifyComplete(); + } + + @Test // DATAMONGO-2195 + public void deleteWithVersionMismatch() { + + PersonWithVersionPropertyOfTypeInteger rojer = new PersonWithVersionPropertyOfTypeInteger(); + rojer.firstName = "rojer"; + + PersonWithVersionPropertyOfTypeInteger saved = template.insert(rojer).block(); + saved.version = 5; + + template.inTransaction().execute(action -> action.remove(saved)) // + .as(StepVerifier::create) // + .expectError(OptimisticLockingFailureException.class) // + .verify(); + } + + @Test // DATAMONGO-2195 + public void deleteNonExistingWithVersion() { + + PersonWithVersionPropertyOfTypeInteger rojer = new PersonWithVersionPropertyOfTypeInteger(); + rojer.id = "deceased"; + rojer.firstName = "rojer"; + rojer.version = 5; + + template.inTransaction().execute(action -> action.remove(rojer)) // + .as(StepVerifier::create) // + .consumeNextWith(result -> assertThat(result.getDeletedCount()).isZero()) // + .verifyComplete(); + } } diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/VersionedPerson.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/VersionedPerson.java new file mode 100644 index 0000000000..049e801e4d --- /dev/null +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/VersionedPerson.java @@ -0,0 +1,47 @@ +/* + * Copyright 2019 the original 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.springframework.data.mongodb.repository; + +import lombok.Data; +import lombok.NoArgsConstructor; + +import org.springframework.data.annotation.Version; +import org.springframework.data.mongodb.core.mapping.Document; +import org.springframework.lang.Nullable; + +/** + * @author Christoph Strobl + */ +@Document +@Data +@NoArgsConstructor +public class VersionedPerson extends Contact { + + private String firstname; + private @Nullable String lastname; + + private @Version Long version; + + public VersionedPerson(String firstname) { + this(firstname, null); + } + + public VersionedPerson(String firstname, @Nullable String lastname) { + + this.firstname = firstname; + this.lastname = lastname; + } +} diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java new file mode 100644 index 0000000000..d3d950c542 --- /dev/null +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java @@ -0,0 +1,162 @@ +/* + * Copyright 2019 the original 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.springframework.data.mongodb.repository.support; + +import static org.assertj.core.api.Assertions.*; +import static org.springframework.data.mongodb.core.query.Criteria.*; +import static org.springframework.data.mongodb.core.query.Query.*; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Configuration; +import org.springframework.dao.OptimisticLockingFailureException; +import org.springframework.data.mongodb.MongoTransactionManager; +import org.springframework.data.mongodb.config.AbstractMongoClientConfiguration; +import org.springframework.data.mongodb.core.MongoTemplate; +import org.springframework.data.mongodb.core.mapping.BasicMongoPersistentEntity; +import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; +import org.springframework.data.mongodb.repository.VersionedPerson; +import org.springframework.data.mongodb.repository.query.MongoEntityInformation; +import org.springframework.data.util.ClassTypeInformation; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; +import org.springframework.transaction.support.TransactionTemplate; + +import com.mongodb.client.MongoClient; +import com.mongodb.client.MongoClients; + +/** + * @author Christoph Strobl + */ +@RunWith(SpringJUnit4ClassRunner.class) +@ContextConfiguration +public class SimpleMongoRepositoryVersionedEntityTests { + + @Configuration + static class Config extends AbstractMongoClientConfiguration { + + @Override + public MongoClient mongoClient() { + return MongoClients.create(); + } + + @Override + protected String getDatabaseName() { + return "database"; + } + } + + private static final MongoPersistentEntity ENTITY = new BasicMongoPersistentEntity( + ClassTypeInformation.from(VersionedPerson.class)); + + @Autowired // + private MongoTemplate template; + + private MongoEntityInformation personEntityInformation; + private SimpleMongoRepository repository; + + private VersionedPerson sarah; + + @Before + public void setUp() { + + personEntityInformation = new MappingMongoEntityInformation(ENTITY); + repository = new SimpleMongoRepository<>(personEntityInformation, template); + repository.deleteAll(); + + sarah = repository.save(new VersionedPerson("Sarah", "Connor")); + } + + @Test // DATAMONGO-2195 + public void deleteWithMatchingVersion() { + + repository.delete(sarah); + + assertThat(template.count(query(where("id").is(sarah.getId())), VersionedPerson.class)).isZero(); + } + + @Test // DATAMONGO-2195 + public void deleteWithMatchingVersionInTx() { + + long countBefore = repository.count(); + + initTxTemplate().execute(status -> { + + VersionedPerson t800 = repository.save(new VersionedPerson("T-800")); + repository.delete(t800); + + return Void.TYPE; + }); + + assertThat(repository.count()).isEqualTo(countBefore); + } + + @Test // DATAMONGO-2195 + public void deleteWithVersionMismatch() { + + sarah.setVersion(5L); + + assertThatExceptionOfType(OptimisticLockingFailureException.class).isThrownBy(() -> repository.delete(sarah)) // + .withMessageContaining("Expected version 5 but was 0"); + + assertThat(template.count(query(where("id").is(sarah.getId())), VersionedPerson.class)).isOne(); + } + + @Test // DATAMONGO-2195 + public void deleteWithVersionMismatchInTx() { + + long countBefore = repository.count(); + + assertThatExceptionOfType(OptimisticLockingFailureException.class) + .isThrownBy(() -> initTxTemplate().execute(status -> { + + VersionedPerson t800 = repository.save(new VersionedPerson("T-800")); + t800.setVersion(5L); + repository.delete(t800); + + return Void.TYPE; + })); + + assertThat(repository.count()).isEqualTo(countBefore); + } + + @Test // DATAMONGO-2195 + public void deleteNonExisting() { + repository.delete(new VersionedPerson("T-800")); + } + + @Test // DATAMONGO-2195 + public void deleteNonExistingInTx() { + + initTxTemplate().execute(status -> { + + repository.delete(new VersionedPerson("T-800")); + + return Void.TYPE; + }); + } + + TransactionTemplate initTxTemplate() { + + MongoTransactionManager txmgr = new MongoTransactionManager(template.getMongoDbFactory()); + TransactionTemplate tt = new TransactionTemplate(txmgr); + tt.afterPropertiesSet(); + + return tt; + } +} diff --git a/src/main/asciidoc/reference/mongodb.adoc b/src/main/asciidoc/reference/mongodb.adoc index 0f5a11211d..11b9095c18 100644 --- a/src/main/asciidoc/reference/mongodb.adoc +++ b/src/main/asciidoc/reference/mongodb.adoc @@ -1080,6 +1080,11 @@ template.save(tmp); // throws OptimisticLockingFailureException IMPORTANT: Optimistic Locking requires to set the `WriteConcern` to `ACKNOWLEDGED`. Otherwise `OptimisticLockingFailureException` can be silently swallowed. +NOTE: As of Version 2.2 `MongoOperations` also includes the `@Version` property when removing an entity from the database, raising an +`OptimisticLockingFailureException` if a document with matching `_id` but a `version` mismatch shall be deleted. In cases +where no Document with matching `_id` can be found the operation passes without error. +To remove a `Document` without version check use `MongoOperations#remove(Query,...)` instead of `MongoOperations#remove(Object)`. + [[mongo.query]] == Querying Documents From 777d4784fcc395566a271d21c1b646d6e4de1b9a Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 5 Feb 2019 11:44:07 +0100 Subject: [PATCH 4/6] DATAMONGO-2195 - Move common logic to ResultOperations. And guard transaction tests. --- .../data/mongodb/core/EntityOperations.java | 7 +- .../data/mongodb/core/MongoTemplate.java | 30 ++++---- .../mongodb/core/ReactiveMongoTemplate.java | 48 +++++------- .../data/mongodb/core/ResultOperations.java | 76 +++++++++++++++++++ ...leMongoRepositoryVersionedEntityTests.java | 12 +++ 5 files changed, 122 insertions(+), 51 deletions(-) create mode 100644 spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ResultOperations.java diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java index ae32a2cbce..cb7870d253 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java @@ -206,12 +206,7 @@ interface Entity { * @since 2.2 */ default Query getRemoveByQuery() { - - if (isVersionedEntity()) { - return getQueryForVersion(); - } - - return getByIdQuery(); + return isVersionedEntity() ? getQueryForVersion() : getByIdQuery(); } /** diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java index 5e993557de..15ba3d16b6 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java @@ -1740,29 +1740,25 @@ public DeleteResult doInCollection(MongoCollection collection) private void checkForOptimisticLockingFailures(DeleteResult result, Document removeQuery, MongoCollection collectionToUse) { - if (!multi && entity != null && entity.hasVersionProperty() && result.wasAcknowledged()) { - - if (result.getDeletedCount() == 0) { - - String versionFieldName = entity.getVersionProperty().getFieldName(); - Document idQuery = new Document(removeQuery); - idQuery.remove(versionFieldName); + if (multi || !ResultOperations.isUndecidedDeleteResult(result, removeQuery, entity)) { + return; + } - Iterator it = collectionToUse.find(idQuery) - .projection(Projections.include("_id", versionFieldName)).limit(1).iterator(); + String versionFieldName = entity.getVersionProperty().getFieldName(); + Document idQuery = new Document(removeQuery); + idQuery.remove(versionFieldName); - if (it.hasNext()) { + Iterator it = collectionToUse.find(idQuery).projection(Projections.include("_id", versionFieldName)) + .limit(1).iterator(); - Document source = it.next(); + if (it.hasNext()) { - throw new OptimisticLockingFailureException(String - .format("The entity with id %s in %s has changed and cannot be deleted! " + System.lineSeparator() + // - "Expected version %s but was %s.", source.get("_id"), collectionName, removeQuery.get(versionFieldName), - source.get(versionFieldName))); - } + Document source = it.next(); + throw ResultOperations.newDeleteVersionedOptimisticLockingException(source.get("_id"), collectionName, + removeQuery.get(versionFieldName), source.get(versionFieldName)); - } } + } }); } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java index 3b0f4db9ac..17b1807245 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java @@ -1688,7 +1688,7 @@ private boolean containsVersionProperty(Document document, @Nullable MongoPersis return false; } - return document.containsKey(persistentEntity.getRequiredIdProperty().getFieldName()); + return document.containsKey(persistentEntity.getRequiredVersionProperty().getFieldName()); } /* @@ -1826,41 +1826,33 @@ protected Mono doRemove(String collectionName, Query query, @N }).flatMap(deleteResult -> { - if (entity != null && entity.hasVersionProperty()) { - - if (deleteResult.wasAcknowledged() && deleteResult.getDeletedCount() == 0) { - - if (query.getQueryObject().containsKey(entity.getVersionProperty().getFieldName())) { + if (!ResultOperations.isUndecidedDeleteResult(deleteResult, removeQuery, entity)) { + return Mono.just(deleteResult); + } - return execute(collectionName, (coll -> { + return execute(collectionName, (coll -> { - String versionFieldName = entity.getVersionProperty().getFieldName(); + String versionFieldName = entity.getVersionProperty().getFieldName(); - Document daQuery = new Document(removeQuery); - daQuery.remove(versionFieldName); + Document queryWithoutVersion = new Document(removeQuery); + queryWithoutVersion.remove(versionFieldName); - Publisher publisher = coll.find(daQuery) - .projection(Projections.include("_id", entity.getVersionProperty().getFieldName())).limit(1).first(); + Publisher publisher = coll.find(queryWithoutVersion) + .projection(Projections.include("_id", entity.getVersionProperty().getFieldName())).limit(1).first(); - return Mono.from(publisher) // - .defaultIfEmpty(new Document()) // - .flatMap(it -> { + return Mono.from(publisher) // + .defaultIfEmpty(new Document()) // + .flatMap(it -> { - if (it.isEmpty()) { - return Mono.just(deleteResult); - } - return Mono.error(new OptimisticLockingFailureException(String.format( - "The entity with id %s in %s has changed and cannot be deleted! " + System.lineSeparator() + // - "Expected version %s but was %s.", it.get("_id"), collectionName, removeQuery.get(versionFieldName), - it.get(versionFieldName)))); + if (it.isEmpty()) { + return Mono.just(deleteResult); + } - }); - })).next(); - } - } - } + return Mono.error(ResultOperations.newDeleteVersionedOptimisticLockingException(it.get("_id"), + collectionName, removeQuery.get(versionFieldName), it.get(versionFieldName))); + }); + })).next(); - return Mono.just(deleteResult); }).doOnNext(it -> maybeEmitEvent(new AfterDeleteEvent<>(queryObject, entityClass, collectionName))) // .next(); } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ResultOperations.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ResultOperations.java new file mode 100644 index 0000000000..da43ff0faf --- /dev/null +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ResultOperations.java @@ -0,0 +1,76 @@ +/* + * Copyright 2019 the original 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.springframework.data.mongodb.core; + +import org.bson.Document; +import org.springframework.dao.OptimisticLockingFailureException; +import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; +import org.springframework.lang.Nullable; + +import com.mongodb.client.result.DeleteResult; +import com.mongodb.client.result.UpdateResult; + +/** + * {@link ResultOperations} is intended to share logic depending on {@link DeleteResult} and + * {@link com.mongodb.client.result.UpdateResult} between reactive and imperative implementations. + * + * @author Christoph Strobl + * @since 2.2 + */ +class ResultOperations { + + /** + * Decide if a given {@link DeleteResult} needs further inspection. Returns {@literal true} if the delete of a + * {@link org.springframework.data.annotation.Version versioned} entity did not remove any documents although the + * operation has been {@link DeleteResult#wasAcknowledged() acknowledged}. + * + * @param deleteResult must not be {@literal null}. + * @param query the actual query used for the delete operation. + * @param entity can be {@literal null}. + * @return {@literal true} if it cannot be decided if nothing got deleted because of a potential + * {@link org.springframework.data.annotation.Version version} mismatch, or if the document did not exist in + * first place. + */ + static boolean isUndecidedDeleteResult(DeleteResult deleteResult, org.bson.Document query, + @Nullable MongoPersistentEntity entity) { + + if (!deleteResult.wasAcknowledged() || deleteResult.getDeletedCount() > 0) { + return false; + } + + if (entity == null) { + return false; + } + + if (!entity.hasVersionProperty()) { + return false; + } + + return containsVersionProperty(query, entity); + } + + static OptimisticLockingFailureException newDeleteVersionedOptimisticLockingException(Object id, + String collectionName, Object expectedVersion, Object actualVersion) { + + throw new OptimisticLockingFailureException( + String.format("The entity with id %s in %s has changed and cannot be deleted! " + System.lineSeparator() + // + "Expected version %s but was %s.", id, collectionName, expectedVersion, actualVersion)); + } + + private static boolean containsVersionProperty(Document document, MongoPersistentEntity entity) { + return document.containsKey(entity.getRequiredVersionProperty().getFieldName()); + } +} diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java index d3d950c542..fc003b6f32 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java @@ -16,6 +16,7 @@ package org.springframework.data.mongodb.repository.support; import static org.assertj.core.api.Assertions.*; +import static org.assertj.core.api.Assumptions.assumeThat; import static org.springframework.data.mongodb.core.query.Criteria.*; import static org.springframework.data.mongodb.core.query.Query.*; @@ -32,6 +33,8 @@ import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; import org.springframework.data.mongodb.repository.VersionedPerson; import org.springframework.data.mongodb.repository.query.MongoEntityInformation; +import org.springframework.data.mongodb.test.util.MongoVersion; +import org.springframework.data.mongodb.test.util.ReplicaSet; import org.springframework.data.util.ClassTypeInformation; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; @@ -91,8 +94,11 @@ public void deleteWithMatchingVersion() { } @Test // DATAMONGO-2195 + @MongoVersion(asOf = "4.0") public void deleteWithMatchingVersionInTx() { + assumeThat(ReplicaSet.required().runsAsReplicaSet()).isTrue(); + long countBefore = repository.count(); initTxTemplate().execute(status -> { @@ -118,8 +124,11 @@ public void deleteWithVersionMismatch() { } @Test // DATAMONGO-2195 + @MongoVersion(asOf = "4.0") public void deleteWithVersionMismatchInTx() { + assumeThat(ReplicaSet.required().runsAsReplicaSet()).isTrue(); + long countBefore = repository.count(); assertThatExceptionOfType(OptimisticLockingFailureException.class) @@ -141,8 +150,11 @@ public void deleteNonExisting() { } @Test // DATAMONGO-2195 + @MongoVersion(asOf = "4.0") public void deleteNonExistingInTx() { + assumeThat(ReplicaSet.required().runsAsReplicaSet()).isTrue(); + initTxTemplate().execute(status -> { repository.delete(new VersionedPerson("T-800")); From d702e4bd5214b7b6052869c2c3e1980683d6108d Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 6 Feb 2019 11:29:17 +0100 Subject: [PATCH 5/6] DATAMONGO-2195 - Throw OptimisticLockingFailureException on delete only in repositories. OptimisticLockingFailureException is now thrown only when deleting an entity through a repository and no longer when using the Template API. --- .../data/mongodb/core/MongoTemplate.java | 26 ---- .../mongodb/core/ReactiveMongoTemplate.java | 29 ----- .../data/mongodb/core/ResultOperations.java | 76 ------------ .../query/MongoEntityInformation.java | 23 ++++ .../MappingMongoEntityInformation.java | 34 +++++- .../support/SimpleMongoRepository.java | 12 +- .../SimpleReactiveMongoRepository.java | 24 +++- .../data/mongodb/core/MongoTemplateTests.java | 48 +------- .../core/ReactiveMongoTemplateTests.java | 60 ++-------- ...ReactiveMongoTemplateTransactionTests.java | 8 +- ...leMongoRepositoryVersionedEntityTests.java | 20 ++-- ...veMongoRepositoryVersionedEntityTests.java | 112 ++++++++++++++++++ src/main/asciidoc/new-features.adoc | 2 + src/main/asciidoc/reference/mongodb.adoc | 7 +- 14 files changed, 233 insertions(+), 248 deletions(-) delete mode 100644 spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ResultOperations.java create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepositoryVersionedEntityTests.java diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java index 15ba3d16b6..2bd0e75696 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoTemplate.java @@ -1730,36 +1730,10 @@ public DeleteResult doInCollection(MongoCollection collection) DeleteResult result = multi ? collectionToUse.deleteMany(removeQuery, options) : collectionToUse.deleteOne(removeQuery, options); - checkForOptimisticLockingFailures(result, removeQuery, collectionToUse); - maybeEmitEvent(new AfterDeleteEvent<>(queryObject, entityClass, collectionName)); return result; } - - private void checkForOptimisticLockingFailures(DeleteResult result, Document removeQuery, - MongoCollection collectionToUse) { - - if (multi || !ResultOperations.isUndecidedDeleteResult(result, removeQuery, entity)) { - return; - } - - String versionFieldName = entity.getVersionProperty().getFieldName(); - Document idQuery = new Document(removeQuery); - idQuery.remove(versionFieldName); - - Iterator it = collectionToUse.find(idQuery).projection(Projections.include("_id", versionFieldName)) - .limit(1).iterator(); - - if (it.hasNext()) { - - Document source = it.next(); - throw ResultOperations.newDeleteVersionedOptimisticLockingException(source.get("_id"), collectionName, - removeQuery.get(versionFieldName), source.get(versionFieldName)); - - } - - } }); } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java index 17b1807245..f92cd7632c 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java @@ -1824,35 +1824,6 @@ protected Mono doRemove(String collectionName, Query query, @N return collectionToUse.deleteMany(removeQuery, deleteOptions); } - }).flatMap(deleteResult -> { - - if (!ResultOperations.isUndecidedDeleteResult(deleteResult, removeQuery, entity)) { - return Mono.just(deleteResult); - } - - return execute(collectionName, (coll -> { - - String versionFieldName = entity.getVersionProperty().getFieldName(); - - Document queryWithoutVersion = new Document(removeQuery); - queryWithoutVersion.remove(versionFieldName); - - Publisher publisher = coll.find(queryWithoutVersion) - .projection(Projections.include("_id", entity.getVersionProperty().getFieldName())).limit(1).first(); - - return Mono.from(publisher) // - .defaultIfEmpty(new Document()) // - .flatMap(it -> { - - if (it.isEmpty()) { - return Mono.just(deleteResult); - } - - return Mono.error(ResultOperations.newDeleteVersionedOptimisticLockingException(it.get("_id"), - collectionName, removeQuery.get(versionFieldName), it.get(versionFieldName))); - }); - })).next(); - }).doOnNext(it -> maybeEmitEvent(new AfterDeleteEvent<>(queryObject, entityClass, collectionName))) // .next(); } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ResultOperations.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ResultOperations.java deleted file mode 100644 index da43ff0faf..0000000000 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ResultOperations.java +++ /dev/null @@ -1,76 +0,0 @@ -/* - * Copyright 2019 the original 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.springframework.data.mongodb.core; - -import org.bson.Document; -import org.springframework.dao.OptimisticLockingFailureException; -import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; -import org.springframework.lang.Nullable; - -import com.mongodb.client.result.DeleteResult; -import com.mongodb.client.result.UpdateResult; - -/** - * {@link ResultOperations} is intended to share logic depending on {@link DeleteResult} and - * {@link com.mongodb.client.result.UpdateResult} between reactive and imperative implementations. - * - * @author Christoph Strobl - * @since 2.2 - */ -class ResultOperations { - - /** - * Decide if a given {@link DeleteResult} needs further inspection. Returns {@literal true} if the delete of a - * {@link org.springframework.data.annotation.Version versioned} entity did not remove any documents although the - * operation has been {@link DeleteResult#wasAcknowledged() acknowledged}. - * - * @param deleteResult must not be {@literal null}. - * @param query the actual query used for the delete operation. - * @param entity can be {@literal null}. - * @return {@literal true} if it cannot be decided if nothing got deleted because of a potential - * {@link org.springframework.data.annotation.Version version} mismatch, or if the document did not exist in - * first place. - */ - static boolean isUndecidedDeleteResult(DeleteResult deleteResult, org.bson.Document query, - @Nullable MongoPersistentEntity entity) { - - if (!deleteResult.wasAcknowledged() || deleteResult.getDeletedCount() > 0) { - return false; - } - - if (entity == null) { - return false; - } - - if (!entity.hasVersionProperty()) { - return false; - } - - return containsVersionProperty(query, entity); - } - - static OptimisticLockingFailureException newDeleteVersionedOptimisticLockingException(Object id, - String collectionName, Object expectedVersion, Object actualVersion) { - - throw new OptimisticLockingFailureException( - String.format("The entity with id %s in %s has changed and cannot be deleted! " + System.lineSeparator() + // - "Expected version %s but was %s.", id, collectionName, expectedVersion, actualVersion)); - } - - private static boolean containsVersionProperty(Document document, MongoPersistentEntity entity) { - return document.containsKey(entity.getRequiredVersionProperty().getFieldName()); - } -} diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoEntityInformation.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoEntityInformation.java index 71caf6bfbf..e1f9ae8eeb 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoEntityInformation.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoEntityInformation.java @@ -16,11 +16,13 @@ package org.springframework.data.mongodb.repository.query; import org.springframework.data.repository.core.EntityInformation; +import org.springframework.lang.Nullable; /** * Mongo specific {@link EntityInformation}. * * @author Oliver Gierke + * @author Mark Paluch */ public interface MongoEntityInformation extends EntityInformation { @@ -37,4 +39,25 @@ public interface MongoEntityInformation extends EntityInformation * @return */ String getIdAttribute(); + + /** + * Returns whether the entity uses optimistic locking. + * + * @return + * @since 2.2 + */ + default boolean isVersioned() { + return false; + } + + /** + * Returns the version value for the entity or {@literal null} if the entity is not {@link #isVersioned() versioned}. + * + * @param entity must not be {@literal null} + * @return + */ + @Nullable + default Object getVersion(T entity) { + return null; + } } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/MappingMongoEntityInformation.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/MappingMongoEntityInformation.java index 4d4fc6de9a..e9c1814edf 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/MappingMongoEntityInformation.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/MappingMongoEntityInformation.java @@ -16,6 +16,7 @@ package org.springframework.data.mongodb.repository.support; import org.bson.types.ObjectId; +import org.springframework.data.mapping.PersistentPropertyAccessor; import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; import org.springframework.data.mongodb.repository.query.MongoEntityInformation; import org.springframework.data.repository.core.support.PersistentEntityInformation; @@ -87,14 +88,16 @@ private MappingMongoEntityInformation(MongoPersistentEntity entity, @Nullable this.fallbackIdType = idType != null ? idType : (Class) ObjectId.class; } - /* (non-Javadoc) + /* + * (non-Javadoc) * @see org.springframework.data.mongodb.repository.MongoEntityInformation#getCollectionName() */ public String getCollectionName() { return customCollectionName == null ? entityMetadata.getCollection() : customCollectionName; } - /* (non-Javadoc) + /* + * (non-Javadoc) * @see org.springframework.data.mongodb.repository.MongoEntityInformation#getIdAttribute() */ public String getIdAttribute() { @@ -106,7 +109,6 @@ public String getIdAttribute() { * @see org.springframework.data.repository.core.support.PersistentEntityInformation#getIdType() */ @Override - @SuppressWarnings("unchecked") public Class getIdType() { if (this.entityMetadata.hasIdProperty()) { @@ -115,4 +117,30 @@ public Class getIdType() { return fallbackIdType; } + + /* + * (non-Javadoc) + * @see org.springframework.data.mongodb.repository.MongoEntityInformation#isVersioned() + */ + @Override + public boolean isVersioned() { + return this.entityMetadata.hasVersionProperty(); + } + + /* + * (non-Javadoc) + * @see org.springframework.data.mongodb.repository.MongoEntityInformation#getVersion(Object) + */ + @Override + public Object getVersion(T entity) { + + if (!isVersioned()) { + return null; + } + + PersistentPropertyAccessor accessor = this.entityMetadata.getPropertyAccessor(entity); + + return accessor.getProperty(this.entityMetadata.getRequiredVersionProperty()); + } + } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepository.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepository.java index c2fc4935cd..cccfcea6f4 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepository.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepository.java @@ -23,6 +23,7 @@ import java.util.Optional; import java.util.stream.Collectors; +import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.data.domain.Example; import org.springframework.data.domain.Page; import org.springframework.data.domain.PageImpl; @@ -40,6 +41,8 @@ import org.springframework.lang.Nullable; import org.springframework.util.Assert; +import com.mongodb.client.result.DeleteResult; + /** * Repository base implementation for Mongo. * @@ -161,7 +164,14 @@ public void delete(T entity) { Assert.notNull(entity, "The given entity must not be null!"); - mongoOperations.remove(entity, entityInformation.getCollectionName()); + DeleteResult deleteResult = mongoOperations.remove(entity, entityInformation.getCollectionName()); + + if (entityInformation.isVersioned() && deleteResult.wasAcknowledged() && deleteResult.getDeletedCount() == 0) { + throw new OptimisticLockingFailureException(String.format( + "The entity with id %s with version %s in %s cannot be deleted! Was it modified or deleted in the meantime?", + entityInformation.getId(entity), entityInformation.getVersion(entity), + entityInformation.getCollectionName())); + } } /* diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepository.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepository.java index 928255da15..019fdd3ec1 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepository.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepository.java @@ -28,6 +28,7 @@ import org.reactivestreams.Publisher; import org.springframework.dao.IncorrectResultSizeDataAccessException; +import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.data.domain.Example; import org.springframework.data.domain.Sort; import org.springframework.data.mongodb.core.ReactiveMongoOperations; @@ -39,6 +40,8 @@ import org.springframework.data.util.Streamable; import org.springframework.util.Assert; +import com.mongodb.client.result.DeleteResult; + /** * Reactive repository base implementation for Mongo. * @@ -355,7 +358,24 @@ public Mono delete(T entity) { Assert.notNull(entity, "The given entity must not be null!"); - return mongoOperations.remove(entity, entityInformation.getCollectionName()).then(); + Mono remove = mongoOperations.remove(entity, entityInformation.getCollectionName()); + + if (entityInformation.isVersioned()) { + + remove = remove.handle((deleteResult, sink) -> { + + if (deleteResult.wasAcknowledged() && deleteResult.getDeletedCount() == 0) { + sink.error(new OptimisticLockingFailureException(String.format( + "The entity with id %s with version %s in %s cannot be deleted! Was it modified or deleted in the meantime?", + entityInformation.getId(entity), entityInformation.getVersion(entity), + entityInformation.getCollectionName()))); + } else { + sink.next(deleteResult); + } + }); + } + + return remove.then(); } /* @@ -367,7 +387,7 @@ public Mono deleteAll(Iterable entities) { Assert.notNull(entities, "The given Iterable of entities must not be null!"); - return Flux.fromIterable(entities).flatMap(entity -> deleteById(entityInformation.getRequiredId(entity))).then(); + return Flux.fromIterable(entities).flatMap(this::delete).then(); } /* diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java index f78acc1e85..435a8e3370 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java @@ -1647,20 +1647,7 @@ public void initializesVersionOnInsert() { } @Test // DATAMONGO-2195 - public void removeEntityWithMatchingVersion() { - - PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); - person.firstName = "Dave"; - - template.insert(person); - assertThat(person.version, is(0)); - - template.remove(person); - assertThat(template.count(new Query(), Person.class)).isZero(); - } - - @Test // DATAMONGO-2195 - public void removeEntityWithoutMatchingVersionThrowsOptimisticLockingFailureException() { + public void removeVersionedEntityConsidersVersion() { PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); person.firstName = "Dave"; @@ -1670,36 +1657,11 @@ public void removeEntityWithoutMatchingVersionThrowsOptimisticLockingFailureExce template.update(PersonWithVersionPropertyOfTypeInteger.class).matching(query(where("id").is(person.id))) .apply(new Update().set("firstName", "Walter")).first(); - Assertions.assertThatThrownBy(() -> template.remove(person)).isInstanceOf(OptimisticLockingFailureException.class) - .hasMessageContaining("Expected version 0 but was 1"); - assertThat(template.count(new Query(), PersonWithVersionPropertyOfTypeInteger.class)).isOne(); - } - - @Test // DATAMONGO-2195 - public void removeNonExistingVersionedEntityJustPasses() { - - PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); - person.id = "id-1"; - person.firstName = "Dave"; - person.version = 10; - - assertThat(template.remove(person).getDeletedCount()).isZero(); - } - - @Test // DATAMONGO-2195 - @DirtiesContext - public void removeEntityWithoutMatchingVersionWhenUsingUnaknowledgedWriteDoesNotThrowsOptimisticLockingFailureException() { - - PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); - person.firstName = "Dave"; - - template.insert(person); - assertThat(person.version, is(0)); - template.update(PersonWithVersionPropertyOfTypeInteger.class).matching(query(where("id").is(person.id))) - .apply(new Update().set("firstName", "Walter")).first(); + DeleteResult deleteResult = template.remove(person); - template.setWriteConcern(WriteConcern.UNACKNOWLEDGED); - assertThat(template.remove(person).wasAcknowledged()).isFalse(); + assertThat(deleteResult.wasAcknowledged()).isTrue(); + assertThat(deleteResult.getDeletedCount()).isZero(); + assertThat(template.count(new Query(), PersonWithVersionPropertyOfTypeInteger.class)).isOne(); } @Test // DATAMONGO-588 diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTests.java index 6cd2b9dd2b..7f67c69f86 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTests.java @@ -15,10 +15,10 @@ */ package org.springframework.data.mongodb.core; +import static org.assertj.core.api.Assertions.*; import static org.springframework.data.mongodb.core.aggregation.Aggregation.*; import static org.springframework.data.mongodb.core.query.Criteria.*; import static org.springframework.data.mongodb.core.query.Query.*; -import static org.springframework.data.mongodb.test.util.Assertions.*; import lombok.AllArgsConstructor; import lombok.Data; @@ -705,56 +705,7 @@ public void removeWithMonoAndCollectionShouldDeleteElement() { } @Test // DATAMONGO-2195 - public void removeEntityWithMatchingVersion() { - - PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); - person.firstName = "Dave"; - - template.insert(person).as(StepVerifier::create).expectNextCount(1).verifyComplete(); - assertThat(person.version).isZero(); - - template.remove(person).as(StepVerifier::create).expectNextCount(1).verifyComplete(); - template.count(new Query(), PersonWithVersionPropertyOfTypeInteger.class) // - .as(StepVerifier::create) // - .expectNext(0L) // - .verifyComplete(); - } - - @Test // DATAMONGO-2195 - public void removeEntityWithoutMatchingVersionThrowsOptimisticLockingFailureException() { - - PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); - person.firstName = "Dave"; - - template.insert(person).as(StepVerifier::create).expectNextCount(1).verifyComplete(); - assertThat(person.version).isZero(); - template.update(PersonWithVersionPropertyOfTypeInteger.class).matching(query(where("id").is(person.id))) - .apply(new Update().set("firstName", "Walter")).first() // - .as(StepVerifier::create) // - .expectNextCount(1) // - .verifyComplete(); - - template.remove(person).as(StepVerifier::create).expectError(OptimisticLockingFailureException.class).verify(); - template.count(new Query(), PersonWithVersionPropertyOfTypeInteger.class) // - .as(StepVerifier::create) // - .expectNext(1L) // - .verifyComplete(); - } - - @Test // DATAMONGO-2195 - public void removeNonExistingVersionedEntityJustPasses() { - - PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); - person.id = "id-1"; - person.firstName = "Dave"; - person.version = 10; - - template.remove(person).as(StepVerifier::create).expectNextCount(1).verifyComplete(); - } - - @Test // DATAMONGO-2195 - @DirtiesContext - public void removeEntityWithoutMatchingVersionWhenUsingUnaknowledgedWriteDoesNotThrowsOptimisticLockingFailureException() { + public void removeVersionedEntityConsidersVersion() { PersonWithVersionPropertyOfTypeInteger person = new PersonWithVersionPropertyOfTypeInteger(); person.firstName = "Dave"; @@ -767,9 +718,12 @@ public void removeEntityWithoutMatchingVersionWhenUsingUnaknowledgedWriteDoesNot .expectNextCount(1) // .verifyComplete(); - template.setWriteConcern(WriteConcern.UNACKNOWLEDGED); + template.remove(person).as(StepVerifier::create) // + .consumeNextWith(actual -> { - template.remove(person).as(StepVerifier::create).expectNextCount(1).verifyComplete(); + assertThat(actual.wasAcknowledged()).isTrue(); + assertThat(actual.getDeletedCount()).isZero(); + }).verifyComplete(); template.count(new Query(), PersonWithVersionPropertyOfTypeInteger.class) // .as(StepVerifier::create) // .expectNext(1L) // diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTransactionTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTransactionTests.java index 2a2bd13576..c2ece77e84 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTransactionTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTransactionTests.java @@ -31,7 +31,6 @@ import org.junit.Test; import org.junit.rules.TestRule; import org.reactivestreams.Publisher; -import org.springframework.dao.OptimisticLockingFailureException; import org.springframework.data.domain.Sort; import org.springframework.data.mongodb.core.query.Query; import org.springframework.data.mongodb.test.util.MongoTestUtils; @@ -303,8 +302,11 @@ public void deleteWithVersionMismatch() { template.inTransaction().execute(action -> action.remove(saved)) // .as(StepVerifier::create) // - .expectError(OptimisticLockingFailureException.class) // - .verify(); + .consumeNextWith(actual -> { + + assertThat(actual.wasAcknowledged()).isTrue(); + assertThat(actual.getDeletedCount()).isZero(); + }).verifyComplete(); } @Test // DATAMONGO-2195 diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java index fc003b6f32..99b0cd088d 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleMongoRepositoryVersionedEntityTests.java @@ -16,7 +16,7 @@ package org.springframework.data.mongodb.repository.support; import static org.assertj.core.api.Assertions.*; -import static org.assertj.core.api.Assumptions.assumeThat; +import static org.assertj.core.api.Assumptions.*; import static org.springframework.data.mongodb.core.query.Criteria.*; import static org.springframework.data.mongodb.core.query.Query.*; @@ -45,6 +45,7 @@ /** * @author Christoph Strobl + * @author Mark Paluch */ @RunWith(SpringJUnit4ClassRunner.class) @ContextConfiguration @@ -64,9 +65,6 @@ protected String getDatabaseName() { } } - private static final MongoPersistentEntity ENTITY = new BasicMongoPersistentEntity( - ClassTypeInformation.from(VersionedPerson.class)); - @Autowired // private MongoTemplate template; @@ -78,7 +76,10 @@ protected String getDatabaseName() { @Before public void setUp() { - personEntityInformation = new MappingMongoEntityInformation(ENTITY); + MongoPersistentEntity entity = template.getConverter().getMappingContext() + .getRequiredPersistentEntity(VersionedPerson.class); + + personEntityInformation = new MappingMongoEntityInformation(entity); repository = new SimpleMongoRepository<>(personEntityInformation, template); repository.deleteAll(); @@ -117,8 +118,7 @@ public void deleteWithVersionMismatch() { sarah.setVersion(5L); - assertThatExceptionOfType(OptimisticLockingFailureException.class).isThrownBy(() -> repository.delete(sarah)) // - .withMessageContaining("Expected version 5 but was 0"); + assertThatExceptionOfType(OptimisticLockingFailureException.class).isThrownBy(() -> repository.delete(sarah)); assertThat(template.count(query(where("id").is(sarah.getId())), VersionedPerson.class)).isOne(); } @@ -146,7 +146,8 @@ public void deleteWithVersionMismatchInTx() { @Test // DATAMONGO-2195 public void deleteNonExisting() { - repository.delete(new VersionedPerson("T-800")); + assertThatThrownBy(() -> repository.delete(new VersionedPerson("T-800"))) + .isInstanceOf(OptimisticLockingFailureException.class); } @Test // DATAMONGO-2195 @@ -157,7 +158,8 @@ public void deleteNonExistingInTx() { initTxTemplate().execute(status -> { - repository.delete(new VersionedPerson("T-800")); + assertThatThrownBy(() -> repository.delete(new VersionedPerson("T-800"))) + .isInstanceOf(OptimisticLockingFailureException.class); return Void.TYPE; }); diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepositoryVersionedEntityTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepositoryVersionedEntityTests.java new file mode 100644 index 0000000000..b4c2f0e30c --- /dev/null +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepositoryVersionedEntityTests.java @@ -0,0 +1,112 @@ +/* + * Copyright 2019 the original 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.springframework.data.mongodb.repository.support; + +import static org.springframework.data.mongodb.core.query.Criteria.*; +import static org.springframework.data.mongodb.core.query.Query.*; + +import reactor.test.StepVerifier; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Configuration; +import org.springframework.dao.OptimisticLockingFailureException; +import org.springframework.data.mongodb.config.AbstractReactiveMongoConfiguration; +import org.springframework.data.mongodb.core.ReactiveMongoTemplate; +import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; +import org.springframework.data.mongodb.repository.VersionedPerson; +import org.springframework.data.mongodb.repository.query.MongoEntityInformation; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; + +import com.mongodb.reactivestreams.client.MongoClient; +import com.mongodb.reactivestreams.client.MongoClients; + +/** + * @author Mark Paluch + */ +@RunWith(SpringJUnit4ClassRunner.class) +@ContextConfiguration +public class SimpleReactiveMongoRepositoryVersionedEntityTests { + + @Configuration + static class Config extends AbstractReactiveMongoConfiguration { + + @Override + public MongoClient reactiveMongoClient() { + return MongoClients.create(); + } + + @Override + protected String getDatabaseName() { + return "database"; + } + } + + @Autowired // + private ReactiveMongoTemplate template; + + private MongoEntityInformation personEntityInformation; + private SimpleReactiveMongoRepository repository; + + private VersionedPerson sarah; + + @Before + public void setUp() { + + MongoPersistentEntity entity = template.getConverter().getMappingContext() + .getRequiredPersistentEntity(VersionedPerson.class); + + personEntityInformation = new MappingMongoEntityInformation(entity); + repository = new SimpleReactiveMongoRepository<>(personEntityInformation, template); + repository.deleteAll().as(StepVerifier::create).verifyComplete(); + + sarah = repository.save(new VersionedPerson("Sarah", "Connor")).block(); + } + + @Test // DATAMONGO-2195 + public void deleteWithMatchingVersion() { + + repository.delete(sarah).as(StepVerifier::create).verifyComplete(); + + template.count(query(where("id").is(sarah.getId())), VersionedPerson.class) // + .as(StepVerifier::create) // + .expectNext(0L).verifyComplete(); + } + + @Test // DATAMONGO-2195 + public void deleteWithVersionMismatch() { + + sarah.setVersion(5L); + + repository.delete(sarah).as(StepVerifier::create).verifyError(OptimisticLockingFailureException.class); + + template.count(query(where("id").is(sarah.getId())), VersionedPerson.class) // + .as(StepVerifier::create) // + .expectNextCount(1) // + .verifyComplete(); + } + + @Test // DATAMONGO-2195 + public void deleteNonExisting() { + + repository.delete(new VersionedPerson("T-800")).as(StepVerifier::create) + .verifyError(OptimisticLockingFailureException.class); + } + +} diff --git a/src/main/asciidoc/new-features.adoc b/src/main/asciidoc/new-features.adoc index 80ec95981e..b3eedd7ce5 100644 --- a/src/main/asciidoc/new-features.adoc +++ b/src/main/asciidoc/new-features.adoc @@ -6,6 +6,8 @@ * <> * <> via `ReactiveQuerydslPredicateExecutor`. * <>. +* Template API delete by entity considers the version property in delete queries. +* Repository deletes now throw `OptimisticLockingFailureException` when a versioned entity cannot be deleted. [[new-features.2-1-0]] == What's New in Spring Data MongoDB 2.1 diff --git a/src/main/asciidoc/reference/mongodb.adoc b/src/main/asciidoc/reference/mongodb.adoc index 11b9095c18..3d7fa2a784 100644 --- a/src/main/asciidoc/reference/mongodb.adoc +++ b/src/main/asciidoc/reference/mongodb.adoc @@ -1080,11 +1080,12 @@ template.save(tmp); // throws OptimisticLockingFailureException IMPORTANT: Optimistic Locking requires to set the `WriteConcern` to `ACKNOWLEDGED`. Otherwise `OptimisticLockingFailureException` can be silently swallowed. -NOTE: As of Version 2.2 `MongoOperations` also includes the `@Version` property when removing an entity from the database, raising an -`OptimisticLockingFailureException` if a document with matching `_id` but a `version` mismatch shall be deleted. In cases -where no Document with matching `_id` can be found the operation passes without error. +NOTE: As of Version 2.2 `MongoOperations` also includes the `@Version` property when removing an entity from the database. To remove a `Document` without version check use `MongoOperations#remove(Query,...)` instead of `MongoOperations#remove(Object)`. +NOTE: As of Version 2.2 repositories check for the outcome of acknowledged deletes when removing versioned entities. +An `OptimisticLockingFailureException` is raised if a versioned entity cannot be deleted through `CrudRepository.delete(Object)`. In such case, the version was changed or the object was deleted in the meantime. Use `CrudRepository.deleteById(ID)` to bypass optimistic locking functionality and delete objects regardless of their version. + [[mongo.query]] == Querying Documents From c5ada7f0d4d3fa51cb051369983e65b979fb0723 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 7 Feb 2019 14:53:16 +0100 Subject: [PATCH 6/6] DATAMONGO-2195 - Documentation update. --- .../data/mongodb/core/EntityOperations.java | 10 ++++++---- .../data/mongodb/core/MongoOperations.java | 16 ++++++---------- .../repository/query/MongoEntityInformation.java | 5 +++-- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java index cb7870d253..e990ce587c 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java @@ -239,9 +239,11 @@ default boolean isVersionedEntity() { } /** - * Returns the value of the version if the entity has a version property, {@literal null} otherwise. + * Returns the value of the version if the entity {@link #isVersionedEntity() has a version property}. * - * @return + * @return the entity version. Can be {@literal null}. + * @throws IllegalStateException if the entity does not define a {@literal version} property. Make sure to check + * {@link #isVersionedEntity()}. */ @Nullable Object getVersion(); @@ -297,8 +299,8 @@ interface AdaptibleEntity extends Entity { /** * Returns the current version value if the entity has a version property. * - * @return the current version or {@literal null} in case it's uninitialized or the entity doesn't expose a version - * property. + * @return the current version or {@literal null} in case it's uninitialized. + * @throws IllegalStateException if the entity does not define a {@literal version} property. */ @Nullable Number getVersion(); diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoOperations.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoOperations.java index 89f693d152..85f3d411ac 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoOperations.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoOperations.java @@ -1351,28 +1351,24 @@ T findAndReplace(Query query, S replacement, FindAndReplaceOptions option /** * Remove the given object from the collection by {@literal id} and (if applicable) its - * {@link org.springframework.data.annotation.Version}. + * {@link org.springframework.data.annotation.Version}.
+ * Use {@link DeleteResult#getDeletedCount()} for insight whether an {@link DeleteResult#wasAcknowledged() + * acknowledged} remove operation was successful or not. * * @param object must not be {@literal null}. * @return the {@link DeleteResult} which lets you access the results of the previous delete. - * @throws org.springframework.dao.OptimisticLockingFailureException if the given {@literal object} is - * {@link org.springframework.data.annotation.Version versioned} and the current version does not match the - * one within the store. If no document with matching {@literal _id} exists in the collection the operation - * completes without error. */ DeleteResult remove(Object object); /** * Removes the given object from the given collection by {@literal id} and (if applicable) its - * {@link org.springframework.data.annotation.Version}. + * {@link org.springframework.data.annotation.Version}.
+ * Use {@link DeleteResult#getDeletedCount()} for insight whether an {@link DeleteResult#wasAcknowledged() + * acknowledged} remove operation was successful or not. * * @param object must not be {@literal null}. * @param collectionName name of the collection where the objects will removed, must not be {@literal null} or empty. * @return the {@link DeleteResult} which lets you access the results of the previous delete. - * @throws org.springframework.dao.OptimisticLockingFailureException if the given {@literal object} is - * {@link org.springframework.data.annotation.Version versioned} and the current version does not match the - * one within the store. If no document with matching {@literal _id} exists in the collection the operation - * completes without error. */ DeleteResult remove(Object object, String collectionName); diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoEntityInformation.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoEntityInformation.java index e1f9ae8eeb..c58732f589 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoEntityInformation.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/query/MongoEntityInformation.java @@ -43,7 +43,7 @@ public interface MongoEntityInformation extends EntityInformation /** * Returns whether the entity uses optimistic locking. * - * @return + * @return true if the entity defines a {@link org.springframework.data.annotation.Version} property. * @since 2.2 */ default boolean isVersioned() { @@ -54,7 +54,8 @@ default boolean isVersioned() { * Returns the version value for the entity or {@literal null} if the entity is not {@link #isVersioned() versioned}. * * @param entity must not be {@literal null} - * @return + * @return can be {@literal null}. + * @since 2.2 */ @Nullable default Object getVersion(T entity) {