From 723b481f8251316eb7ca951f942f542571c110ae Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Mon, 7 Jan 2019 08:42:43 +0100 Subject: [PATCH] DATAMONGO-2179 - Fixed broken auditing for entities using optimistic locking via batch save. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous implementation of (Reactive)MongoTemplate.doInsertBatch(…) 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. Related tickets: DATAMONGO-2139, DATAMONGO-2150. Original Pull Request: #632 --- .../data/mongodb/core/MongoTemplate.java | 13 +++++----- .../mongodb/core/ReactiveMongoTemplate.java | 24 +++++++++++-------- ...uditingViaJavaConfigRepositoriesTests.java | 20 +++++++++++++++- .../mongodb/config/ReactiveAuditingTests.java | 20 +++++++++++++++- 4 files changed, 59 insertions(+), 18 deletions(-) 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 28aa9dbd4..ecfb07dd6 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 @@ -1309,17 +1309,18 @@ public class MongoTemplate implements MongoOperations, ApplicationContextAware, List initializedBatchToSave = new ArrayList<>(batchToSave.size()); for (T uninitialized : batchToSave) { - AdaptibleEntity entity = operations.forEntity(uninitialized, mongoConverter.getConversionService()); - T toSave = entity.initializeVersionProperty(); + BeforeConvertEvent event = new BeforeConvertEvent<>(uninitialized, collectionName); + T toConvert = maybeEmitEvent(event).getSource(); - BeforeConvertEvent event = new BeforeConvertEvent<>(toSave, collectionName); - toSave = maybeEmitEvent(event).getSource(); + AdaptibleEntity entity = operations.forEntity(toConvert, mongoConverter.getConversionService()); + entity.assertUpdateableIdIfNotSet(); + T initialized = entity.initializeVersionProperty(); Document document = entity.toMappedDocument(writer).getDocument(); + maybeEmitEvent(new BeforeSaveEvent<>(initialized, document, collectionName)); - maybeEmitEvent(new BeforeSaveEvent<>(toSave, document, collectionName)); documentList.add(document); - initializedBatchToSave.add(toSave); + initializedBatchToSave.add(initialized); } List ids = insertDocumentList(collectionName, documentList); 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 0bf8facf9..30b0ded61 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 @@ -1239,9 +1239,10 @@ public class ReactiveMongoTemplate implements ReactiveMongoOperations, Applicati BeforeConvertEvent event = new BeforeConvertEvent<>(objectToSave, collectionName); T toConvert = maybeEmitEvent(event).getSource(); - AdaptibleEntity entity = operations.forEntity(toConvert, mongoConverter.getConversionService()); + AdaptibleEntity entity = operations.forEntity(toConvert, mongoConverter.getConversionService()); entity.assertUpdateableIdIfNotSet(); + T initialized = entity.initializeVersionProperty(); Document dbDoc = entity.toMappedDocument(writer).getDocument(); @@ -1314,19 +1315,22 @@ public class ReactiveMongoTemplate implements ReactiveMongoOperations, Applicati Assert.notNull(writer, "MongoWriter must not be null!"); - Mono, Document>>> prepareDocuments = Flux.fromIterable(batchToSave).map(o -> { + Mono, Document>>> prepareDocuments = Flux.fromIterable(batchToSave) + .map(uninitialized -> { - AdaptibleEntity entity = operations.forEntity(o, mongoConverter.getConversionService()); - T toSave = entity.initializeVersionProperty(); + BeforeConvertEvent event = new BeforeConvertEvent<>(uninitialized, collectionName); + T toConvert = maybeEmitEvent(event).getSource(); - BeforeConvertEvent event = new BeforeConvertEvent<>(toSave, collectionName); - toSave = maybeEmitEvent(event).getSource(); + AdaptibleEntity entity = operations.forEntity(toConvert, mongoConverter.getConversionService()); + entity.assertUpdateableIdIfNotSet(); - Document dbDoc = entity.toMappedDocument(writer).getDocument(); + T initialized = entity.initializeVersionProperty(); + Document dbDoc = entity.toMappedDocument(writer).getDocument(); - maybeEmitEvent(new BeforeSaveEvent<>(toSave, dbDoc, collectionName)); - return Tuples.of(entity, dbDoc); - }).collectList(); + maybeEmitEvent(new BeforeSaveEvent<>(initialized, dbDoc, collectionName)); + + return Tuples.of(entity, dbDoc); + }).collectList(); Flux, Document>> insertDocuments = prepareDocuments.flatMapMany(tuples -> { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/AuditingViaJavaConfigRepositoriesTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/AuditingViaJavaConfigRepositoriesTests.java index 7c1978bd5..fb7dbdcf2 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/AuditingViaJavaConfigRepositoriesTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/AuditingViaJavaConfigRepositoriesTests.java @@ -20,6 +20,7 @@ import static org.hamcrest.CoreMatchers.*; import static org.junit.Assert.assertThat; import static org.mockito.Mockito.*; +import java.util.Collections; import java.util.Optional; import java.util.function.Function; @@ -121,15 +122,27 @@ public class AuditingViaJavaConfigRepositoriesTests { verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), // it -> it.version, // + AuditablePerson::getCreatedAt, // auditablePersonRepository::save, // null, 0L, 1L); } + @Test // DATAMONGO-2179 + public void auditingWorksForVersionedEntityBatchWithWrapperVersion() { + + verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), // + it -> it.version, // + AuditablePerson::getCreatedAt, // + s -> auditablePersonRepository.saveAll(Collections.singletonList(s)).get(0), // + null, 0L, 1L); + } + @Test // DATAMONGO-2139 public void auditingWorksForVersionedEntityWithSimpleVersion() { verifyAuditingViaVersionProperty(new SimpleVersionedAuditablePerson(), // it -> it.version, // + AuditablePerson::getCreatedAt, // auditablePersonRepository::save, // 0L, 1L, 2L); } @@ -139,6 +152,7 @@ public class AuditingViaJavaConfigRepositoriesTests { verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), // it -> it.version, // + AuditablePerson::getCreatedAt, // operations::save, // null, 0L, 1L); } @@ -148,21 +162,25 @@ public class AuditingViaJavaConfigRepositoriesTests { verifyAuditingViaVersionProperty(new SimpleVersionedAuditablePerson(), // it -> it.version, // + AuditablePerson::getCreatedAt, // operations::save, // 0L, 1L, 2L); } private void verifyAuditingViaVersionProperty(T instance, - Function versionExtractor, Function persister, Object... expectedValues) { + Function versionExtractor, Function createdDateExtractor, Function persister, + Object... expectedValues) { MongoPersistentEntity entity = context.getRequiredPersistentEntity(instance.getClass()); assertThat(versionExtractor.apply(instance)).isEqualTo(expectedValues[0]); + assertThat(createdDateExtractor.apply(instance)).isNull(); assertThat(entity.isNew(instance)).isTrue(); instance = persister.apply(instance); assertThat(versionExtractor.apply(instance)).isEqualTo(expectedValues[1]); + assertThat(createdDateExtractor.apply(instance)).isNotNull(); assertThat(entity.isNew(instance)).isFalse(); instance = persister.apply(instance); 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 index 6cf477c51..05bba3478 100644 --- 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 @@ -21,6 +21,7 @@ import static org.mockito.Mockito.*; import reactor.core.publisher.Mono; import reactor.test.StepVerifier; +import java.util.Collections; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Function; @@ -84,15 +85,27 @@ public class ReactiveAuditingTests { verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), // it -> it.version, // + AuditablePerson::getCreatedAt, // auditablePersonRepository::save, // null, 0L, 1L); } + @Test // DATAMONGO-2179 + public void auditingWorksForVersionedEntityBatchWithWrapperVersion() { + + verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), // + it -> it.version, // + AuditablePerson::getCreatedAt, // + s -> auditablePersonRepository.saveAll(Collections.singletonList(s)).next(), // + null, 0L, 1L); + } + @Test // DATAMONGO-2139, DATAMONGO-2150 public void auditingWorksForVersionedEntityWithSimpleVersion() { verifyAuditingViaVersionProperty(new SimpleVersionedAuditablePerson(), // it -> it.version, // + AuditablePerson::getCreatedAt, // auditablePersonRepository::save, // 0L, 1L, 2L); } @@ -102,6 +115,7 @@ public class ReactiveAuditingTests { verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), // it -> it.version, // + AuditablePerson::getCreatedAt, // operations::save, // null, 0L, 1L); } @@ -110,17 +124,20 @@ public class ReactiveAuditingTests { public void auditingWorksForVersionedEntityWithSimpleVersionOnTemplate() { verifyAuditingViaVersionProperty(new SimpleVersionedAuditablePerson(), // it -> it.version, // + AuditablePerson::getCreatedAt, // operations::save, // 0L, 1L, 2L); } private void verifyAuditingViaVersionProperty(T instance, - Function versionExtractor, Function> persister, Object... expectedValues) { + Function versionExtractor, Function createdDateExtractor, Function> persister, + Object... expectedValues) { AtomicReference instanceHolder = new AtomicReference<>(instance); MongoPersistentEntity entity = context.getRequiredPersistentEntity(instance.getClass()); assertThat(versionExtractor.apply(instance)).isEqualTo(expectedValues[0]); + assertThat(createdDateExtractor.apply(instance)).isNull(); assertThat(entity.isNew(instance)).isTrue(); persister.apply(instanceHolder.get()) // @@ -129,6 +146,7 @@ public class ReactiveAuditingTests { instanceHolder.set(actual); assertThat(versionExtractor.apply(actual)).isEqualTo(expectedValues[1]); + assertThat(createdDateExtractor.apply(instance)).isNotNull(); assertThat(entity.isNew(actual)).isFalse(); }).verifyComplete();