From 2253d3e30152ddaf10d5fabff7a055d325c718c7 Mon Sep 17 00:00:00 2001 From: Oliver Drotbohm Date: Thu, 18 Oct 2018 16:54:54 +0200 Subject: [PATCH] DATAMONGO-2108 - 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 MongoTemplate.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 MongoOperations and repositories. --- .../data/mongodb/core/EntityOperations.java | 30 +++++++- .../data/mongodb/core/MongoTemplate.java | 73 +++++++++--------- ...uditingViaJavaConfigRepositoriesTests.java | 75 ++++++++++++++++++- 3 files changed, 137 insertions(+), 41 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 f50482c39..43e8339ea 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 @@ -242,6 +242,14 @@ class EntityOperations { * @return */ T getBean(); + + /** + * Returns whether the entity is considered to be new. + * + * @return + * @since 2.1.2 + */ + boolean isNew(); } /** @@ -387,6 +395,15 @@ class EntityOperations { public T getBean() { return map; } + + /* + * (non-Javadoc) + * @see org.springframework.data.mongodb.core.EntityOperations.Entity#isNew() + */ + @Override + public boolean isNew() { + return map.get(ID_FIELD) != null; + } } private static class SimpleMappedEntity> extends UnmappedEntity { @@ -549,6 +566,15 @@ class EntityOperations { public T getBean() { return propertyAccessor.getBean(); } + + /* + * (non-Javadoc) + * @see org.springframework.data.mongodb.core.EntityOperations.Entity#isNew() + */ + @Override + public boolean isNew() { + return entity.isNew(propertyAccessor.getBean()); + } } private static class AdaptibleMappedEntity extends MappedEntity implements AdaptibleEntity { @@ -631,7 +657,9 @@ class EntityOperations { return propertyAccessor.getBean(); } - propertyAccessor.setProperty(entity.getRequiredVersionProperty(), 0); + MongoPersistentProperty versionProperty = entity.getRequiredVersionProperty(); + + propertyAccessor.setProperty(versionProperty, versionProperty.getType().isPrimitive() ? 1 : 0); return propertyAccessor.getBean(); } 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 dc3d2b603..27de1506f 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 @@ -1212,20 +1212,19 @@ public class MongoTemplate implements MongoOperations, ApplicationContextAware, protected T doInsert(String collectionName, T objectToSave, MongoWriter writer) { - AdaptibleEntity entity = operations.forEntity(objectToSave, mongoConverter.getConversionService()); - T toSave = entity.initializeVersionProperty(); - - BeforeConvertEvent event = new BeforeConvertEvent<>(toSave, collectionName); - toSave = maybeEmitEvent(event).getSource(); + 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)); - Object id = insertDocument(collectionName, dbDoc, toSave.getClass()); + maybeEmitEvent(new BeforeSaveEvent<>(initialized, dbDoc, collectionName)); + Object id = insertDocument(collectionName, dbDoc, initialized.getClass()); - T saved = populateIdIfNecessary(toSave, id); + T saved = populateIdIfNecessary(initialized, id); maybeEmitEvent(new AfterSaveEvent<>(saved, dbDoc, collectionName)); return saved; @@ -1357,38 +1356,36 @@ public class MongoTemplate implements MongoOperations, ApplicationContextAware, @SuppressWarnings("unchecked") private T doSaveVersioned(AdaptibleEntity source, String collectionName) { - Number number = source.getVersion(); - - if (number != null) { - - // Create query for entity with the id and old version - Query query = source.getQueryForVersion(); - - // Bump version number - T toSave = source.incrementVersion(); - - toSave = maybeEmitEvent(new BeforeConvertEvent(toSave, collectionName)).getSource(); - - source.assertUpdateableIdIfNotSet(); - - MappedDocument mapped = source.toMappedDocument(mongoConverter); - - maybeEmitEvent(new BeforeSaveEvent<>(toSave, mapped.getDocument(), collectionName)); - Update update = mapped.updateWithoutId(); - - UpdateResult result = doUpdate(collectionName, query, update, toSave.getClass(), false, false); - - if (result.getModifiedCount() == 0) { - throw new OptimisticLockingFailureException( - String.format("Cannot save entity %s with version %s to collection %s. Has it been modified meanwhile?", - source.getId(), number, collectionName)); - } - maybeEmitEvent(new AfterSaveEvent<>(toSave, mapped.getDocument(), collectionName)); - - return toSave; + if (source.isNew()) { + return (T) doInsert(collectionName, source.getBean(), this.mongoConverter); } - return (T) doInsert(collectionName, source.getBean(), this.mongoConverter); + // Create query for entity with the id and old version + Query query = source.getQueryForVersion(); + + // Bump version number + T toSave = source.incrementVersion(); + + toSave = maybeEmitEvent(new BeforeConvertEvent(toSave, collectionName)).getSource(); + + source.assertUpdateableIdIfNotSet(); + + MappedDocument mapped = source.toMappedDocument(mongoConverter); + + maybeEmitEvent(new BeforeSaveEvent<>(toSave, mapped.getDocument(), collectionName)); + Update update = mapped.updateWithoutId(); + + UpdateResult result = doUpdate(collectionName, query, update, toSave.getClass(), false, false); + + if (result.getModifiedCount() == 0) { + + throw new OptimisticLockingFailureException( + String.format("Cannot save entity %s with version %s to collection %s. Has it been modified meanwhile?", + source.getId(), source.getVersion(), collectionName)); + } + maybeEmitEvent(new AfterSaveEvent<>(toSave, mapped.getDocument(), collectionName)); + + return toSave; } protected T doSave(String collectionName, T objectToSave, MongoWriter writer) { 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 aa3f1d7f5..41fff3136 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 @@ -15,11 +15,13 @@ */ package org.springframework.data.mongodb.config; +import static org.assertj.core.api.Assertions.assertThat; import static org.hamcrest.CoreMatchers.*; -import static org.junit.Assert.*; +import static org.junit.Assert.assertThat; import static org.mockito.Mockito.*; import java.util.Optional; +import java.util.function.Function; import org.junit.Before; import org.junit.Test; @@ -28,15 +30,18 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.AnnotationConfigApplicationContext; 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.MongoOperations; +import org.springframework.data.mongodb.core.mapping.MongoMappingContext; +import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; import org.springframework.data.mongodb.repository.MongoRepository; import org.springframework.data.mongodb.repository.config.EnableMongoRepositories; import org.springframework.stereotype.Repository; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit4.SpringJUnit4ClassRunner; -import com.mongodb.Mongo; import com.mongodb.MongoClient; /** @@ -51,6 +56,9 @@ public class AuditingViaJavaConfigRepositoriesTests { @Autowired AuditablePersonRepository auditablePersonRepository; @Autowired AuditorAware auditorAware; + @Autowired MongoMappingContext context; + @Autowired MongoOperations operations; + AuditablePerson auditor; @Configuration @@ -107,6 +115,61 @@ public class AuditingViaJavaConfigRepositoriesTests { new AnnotationConfigApplicationContext(SimpleConfig.class); } + @Test // DATAMONGO-2139 + public void auditingWorksForVersionedEntityWithWrapperVersion() { + + verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), // + it -> it.version, // + auditablePersonRepository::save, // + null, 0L, 1L); + } + + @Test // DATAMONGO-2139 + public void auditingWorksForVersionedEntityWithSimpleVersion() { + + verifyAuditingViaVersionProperty(new SimpleVersionedAuditablePerson(), // + it -> it.version, // + auditablePersonRepository::save, // + 0L, 1L, 2L); + } + + @Test // DATAMONGO-2139 + public void auditingWorksForVersionedEntityWithWrapperVersionOnTemplate() { + + verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), // + it -> it.version, // + operations::save, // + null, 0L, 1L); + } + + @Test // DATAMONGO-2139 + 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) { + + MongoPersistentEntity entity = context.getRequiredPersistentEntity(instance.getClass()); + + assertThat(versionExtractor.apply(instance)).isEqualTo(expectedValues[0]); + assertThat(entity.isNew(instance)).isTrue(); + + instance = auditablePersonRepository.save(instance); + + assertThat(versionExtractor.apply(instance)).isEqualTo(expectedValues[1]); + assertThat(entity.isNew(instance)).isFalse(); + + instance = auditablePersonRepository.save(instance); + + assertThat(versionExtractor.apply(instance)).isEqualTo(expectedValues[2]); + assertThat(entity.isNew(instance)).isFalse(); + } + @Repository static interface AuditablePersonRepository extends MongoRepository {} @@ -128,4 +191,12 @@ public class AuditingViaJavaConfigRepositoriesTests { return "database"; } } + + static class VersionedAuditablePerson extends AuditablePerson { + @Version Long version; + } + + static class SimpleVersionedAuditablePerson extends AuditablePerson { + @Version long version; + } }