From adf16bb31f8d8165f95cc0ff3208baeba773a155 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 5 Dec 2018 16:12:28 +0100 Subject: [PATCH] DATAMONGO-2150 - Fixed broken auditing for entities using optimistic locking. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous implementation of ReactiveMongoTemplate.doSaveVersioned(…) prematurely initialized the version property so that the entity wasn't considered new by the auditing subsystem. Even worse, for primitive version properties, the initialization kept the property at a value of 0, so that the just persisted entity was still considered new. This mean that via the repository route, inserts are triggered even for subsequent attempts to save an entity which caused duplicate key exceptions. We now make sure we fire the BeforeConvertEvent before the version property is initialized or updated. Also, the initialization of the property now sets primitive properties to 1 initially. Added integration tests for the auditing via ReactiveMongoTemplate and repositories. Related ticket: DATAMONGO-2139. Original Pull Request: #627 --- .../mongodb/core/ReactiveMongoTemplate.java | 52 +++--- .../mongodb/config/ReactiveAuditingTests.java | 154 ++++++++++++++++++ .../core/ReactiveMongoTemplateTests.java | 14 +- 3 files changed, 181 insertions(+), 39 deletions(-) create mode 100644 spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/ReactiveAuditingTests.java 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 0ea20fbf8..fb39c0f78 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 @@ -1234,23 +1234,22 @@ public class ReactiveMongoTemplate implements ReactiveMongoOperations, Applicati protected Mono doInsert(String collectionName, T objectToSave, MongoWriter writer) { - assertUpdateableIdIfNotSet(objectToSave); - return Mono.defer(() -> { - AdaptibleEntity entity = operations.forEntity(objectToSave, mongoConverter.getConversionService()); - T toSave = entity.initializeVersionProperty(); - - maybeEmitEvent(new BeforeConvertEvent<>(toSave, collectionName)); + BeforeConvertEvent event = new BeforeConvertEvent<>(objectToSave, collectionName); + T toConvert = maybeEmitEvent(event).getSource(); + AdaptibleEntity entity = operations.forEntity(toConvert, mongoConverter.getConversionService()); + entity.assertUpdateableIdIfNotSet(); + T initialized = entity.initializeVersionProperty(); Document dbDoc = entity.toMappedDocument(writer).getDocument(); - maybeEmitEvent(new BeforeSaveEvent<>(toSave, dbDoc, collectionName)); + maybeEmitEvent(new BeforeSaveEvent<>(initialized, dbDoc, collectionName)); - Mono afterInsert = insertDBObject(collectionName, dbDoc, toSave.getClass()).map(id -> { + Mono afterInsert = insertDBObject(collectionName, dbDoc, initialized.getClass()).map(id -> { T saved = entity.populateIdIfNecessary(id); - maybeEmitEvent(new AfterSaveEvent<>(saved, dbDoc, collectionName)); + maybeEmitEvent(new AfterSaveEvent<>(initialized, dbDoc, collectionName)); return saved; }); @@ -1388,34 +1387,27 @@ public class ReactiveMongoTemplate implements ReactiveMongoOperations, Applicati Assert.notNull(objectToSave, "Object to save must not be null!"); Assert.hasText(collectionName, "Collection name must not be null or empty!"); - MongoPersistentEntity mongoPersistentEntity = getPersistentEntity(objectToSave.getClass()); + AdaptibleEntity source = operations.forEntity(objectToSave, mongoConverter.getConversionService()); - // No optimistic locking -> simple save - if (mongoPersistentEntity == null || !mongoPersistentEntity.hasVersionProperty()) { - return doSave(collectionName, objectToSave, this.mongoConverter); - } - - return doSaveVersioned(objectToSave, mongoPersistentEntity, collectionName); + return source.isVersionedEntity() ? doSaveVersioned(source, collectionName) + : doSave(collectionName, objectToSave, this.mongoConverter); } - private Mono doSaveVersioned(T objectToSave, MongoPersistentEntity entity, String collectionName) { + private Mono doSaveVersioned(AdaptibleEntity source, String collectionName) { - AdaptibleEntity forEntity = operations.forEntity(objectToSave, mongoConverter.getConversionService()); + if (source.isNew()) { + return doInsert(collectionName, source.getBean(), this.mongoConverter); + } return createMono(collectionName, collection -> { - Number versionNumber = forEntity.getVersion(); - - // Fresh instance -> initialize version property - if (versionNumber == null) { - return doInsert(collectionName, objectToSave, mongoConverter); - } - - forEntity.assertUpdateableIdIfNotSet(); + // Create query for entity with the id and old version + Query query = source.getQueryForVersion(); - Query query = forEntity.getQueryForVersion(); + // Bump version number + T toSave = source.incrementVersion(); - T toSave = forEntity.incrementVersion(); + source.assertUpdateableIdIfNotSet(); BeforeConvertEvent event = new BeforeConvertEvent<>(toSave, collectionName); T afterEvent = ReactiveMongoTemplate.this.maybeEmitEvent(event).getSource(); @@ -1426,7 +1418,9 @@ public class ReactiveMongoTemplate implements ReactiveMongoOperations, Applicati ReactiveMongoTemplate.this.maybeEmitEvent(new BeforeSaveEvent<>(afterEvent, document, collectionName)); return doUpdate(collectionName, query, mapped.updateWithoutId(), afterEvent.getClass(), false, false) - .map(updateResult -> maybeEmitEvent(new AfterSaveEvent(afterEvent, document, collectionName)).getSource()); + .map(result -> { + return maybeEmitEvent(new AfterSaveEvent(afterEvent, document, collectionName)).getSource(); + }); }); } diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/ReactiveAuditingTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/ReactiveAuditingTests.java new file mode 100644 index 000000000..c643f82f3 --- /dev/null +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/ReactiveAuditingTests.java @@ -0,0 +1,154 @@ +/* + * Copyright 2018 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.config; + +import static org.assertj.core.api.Assertions.*; +import static org.mockito.Mockito.*; + +import reactor.core.publisher.Mono; +import reactor.test.StepVerifier; + +import java.util.concurrent.atomic.AtomicReference; +import java.util.function.Function; + +import org.junit.Test; +import org.junit.runner.RunWith; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.data.annotation.Version; +import org.springframework.data.domain.AuditorAware; +import org.springframework.data.mongodb.core.AuditablePerson; +import org.springframework.data.mongodb.core.ReactiveMongoOperations; +import org.springframework.data.mongodb.core.mapping.MongoMappingContext; +import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; +import org.springframework.data.mongodb.repository.ReactiveMongoRepository; +import org.springframework.data.mongodb.repository.config.EnableReactiveMongoRepositories; +import org.springframework.test.context.ContextConfiguration; +import org.springframework.test.context.junit4.SpringRunner; + +import com.mongodb.reactivestreams.client.MongoClient; +import com.mongodb.reactivestreams.client.MongoClients; + +/** + * Integration test for the auditing support via {@link org.springframework.data.mongodb.core.ReactiveMongoTemplate}. + * + * @author Mark Paluch + */ +@RunWith(SpringRunner.class) +@ContextConfiguration +public class ReactiveAuditingTests { + + @Autowired ReactiveAuditablePersonRepository auditablePersonRepository; + @Autowired AuditorAware auditorAware; + @Autowired MongoMappingContext context; + @Autowired ReactiveMongoOperations operations; + + @Configuration + @EnableMongoAuditing(auditorAwareRef = "auditorProvider") + @EnableReactiveMongoRepositories(basePackageClasses = ReactiveAuditingTests.class, considerNestedRepositories = true) + static class Config extends AbstractReactiveMongoConfiguration { + + @Override + protected String getDatabaseName() { + return "database"; + } + + @Override + public MongoClient reactiveMongoClient() { + return MongoClients.create(); + } + + @Bean + @SuppressWarnings("unchecked") + public AuditorAware auditorProvider() { + return mock(AuditorAware.class); + } + } + + @Test // DATAMONGO-2139, DATAMONGO-2150 + public void auditingWorksForVersionedEntityWithWrapperVersion() { + + verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), // + it -> it.version, // + auditablePersonRepository::save, // + null, 0L, 1L); + } + + @Test // DATAMONGO-2139, DATAMONGO-2150 + public void auditingWorksForVersionedEntityWithSimpleVersion() { + + verifyAuditingViaVersionProperty(new SimpleVersionedAuditablePerson(), // + it -> it.version, // + auditablePersonRepository::save, // + 0L, 1L, 2L); + } + + @Test // DATAMONGO-2139, DATAMONGO-2150 + public void auditingWorksForVersionedEntityWithWrapperVersionOnTemplate() { + + verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), // + it -> it.version, // + operations::save, // + null, 0L, 1L); + } + + @Test // DATAMONGO-2139, DATAMONGO-2150 + public void auditingWorksForVersionedEntityWithSimpleVersionOnTemplate() { + verifyAuditingViaVersionProperty(new SimpleVersionedAuditablePerson(), // + it -> it.version, // + operations::save, // + 0L, 1L, 2L); + } + + private void verifyAuditingViaVersionProperty(T instance, + Function versionExtractor, Function> persister, Object... expectedValues) { + + AtomicReference instanceHolder = new AtomicReference<>(instance); + MongoPersistentEntity entity = context.getRequiredPersistentEntity(instance.getClass()); + + assertThat(versionExtractor.apply(instance)).isEqualTo(expectedValues[0]); + assertThat(entity.isNew(instance)).isTrue(); + + persister.apply(instanceHolder.get()) // + .as(StepVerifier::create).consumeNextWith(actual -> { + + instanceHolder.set(actual); + + assertThat(versionExtractor.apply(actual)).isEqualTo(expectedValues[1]); + assertThat(entity.isNew(actual)).isFalse(); + }).verifyComplete(); + + persister.apply(instanceHolder.get()) // + .as(StepVerifier::create).consumeNextWith(actual -> { + + instanceHolder.set(actual); + + assertThat(versionExtractor.apply(actual)).isEqualTo(expectedValues[2]); + assertThat(entity.isNew(actual)).isFalse(); + }).verifyComplete(); + } + + interface ReactiveAuditablePersonRepository extends ReactiveMongoRepository {} + + static class VersionedAuditablePerson extends AuditablePerson { + @Version Long version; + } + + static class SimpleVersionedAuditablePerson extends AuditablePerson { + @Version long version; + } +} 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 5f8879568..4aaa18480 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 @@ -774,12 +774,9 @@ public class ReactiveMongoTemplateTests { StepVerifier.create(template.save(map, "maps")).expectNextCount(1).verifyComplete(); } - @Test // DATAMONGO-1444, DATAMONGO-1730 + @Test(expected = MappingException.class) // DATAMONGO-1444, DATAMONGO-1730, DATAMONGO-2150 public void savesMongoPrimitiveObjectCorrectly() { - - StepVerifier.create(template.save(new Object(), "collection")) // - .expectError(MappingException.class) // - .verify(); + template.save(new Object(), "collection"); } @Test // DATAMONGO-1444 @@ -852,12 +849,9 @@ public class ReactiveMongoTemplateTests { .verifyComplete(); } - @Test // DATAMONGO-1444 + @Test(expected = MappingException.class) // DATAMONGO-1444, DATAMONGO-2150 public void rejectsNonJsonStringForSave() { - - StepVerifier.create(template.save("Foobar!", "collection")) // - .expectError(MappingException.class) // - .verify(); + template.save("Foobar!", "collection"); } @Test // DATAMONGO-1444