DATAMONGO-2195 - Consider version when removing an entity.

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(…

Author: christophstrobl

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/EntityOperations.java

@@ -199,6 +199,21 @@ interface Entity<T> {
 		 */
 		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()));
 		}
 
 		/*

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MongoOperations.java

@@ -1350,19 +1350,29 @@ <S, T> 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);
 

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<Document> 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<Document> 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<Document> 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)));
+						}
+
+					}
+				}
+			}
 		});
 	}
 

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/ReactiveMongoTemplate.java

@@ -1717,7 +1717,7 @@ public Mono<DeleteResult> 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<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!");
 
-		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 <T> Mono<DeleteResult> 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 <T> Mono<DeleteResult> 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<Document> 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 <T> Mono<DeleteResult> 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<Document> 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();
 	}
 

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());
 	}
 
 	/*

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/repository/support/SimpleReactiveMongoRepository.java

@@ -355,7 +355,7 @@ public Mono<Void> 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();
 	}
 
 	/*

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() {