Skip to content

Commit 718a7ff

Browse files
mp911dechristophstrobl
mp911de
authored and
christophstrobl
committed
DATAMONGO-2150 - Fixed broken auditing for entities using optimistic locking.
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
1 parent f7106dc commit 718a7ff

File tree

3 files changed

+181
-39
lines changed

3 files changed

+181
-39
lines changed

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

Lines changed: 23 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1235,23 +1235,22 @@ public <T> Mono<T> insert(T objectToSave, String collectionName) {
12351235

12361236
protected <T> Mono<T> doInsert(String collectionName, T objectToSave, MongoWriter<Object> writer) {
12371237

1238-
assertUpdateableIdIfNotSet(objectToSave);
1239-
12401238
return Mono.defer(() -> {
12411239

1242-
AdaptibleEntity<T> entity = operations.forEntity(objectToSave, mongoConverter.getConversionService());
1243-
T toSave = entity.initializeVersionProperty();
1244-
1245-
maybeEmitEvent(new BeforeConvertEvent<>(toSave, collectionName));
1240+
BeforeConvertEvent<T> event = new BeforeConvertEvent<>(objectToSave, collectionName);
1241+
T toConvert = maybeEmitEvent(event).getSource();
1242+
AdaptibleEntity<T> entity = operations.forEntity(toConvert, mongoConverter.getConversionService());
12461243

1244+
entity.assertUpdateableIdIfNotSet();
1245+
T initialized = entity.initializeVersionProperty();
12471246
Document dbDoc = entity.toMappedDocument(writer).getDocument();
12481247

1249-
maybeEmitEvent(new BeforeSaveEvent<>(toSave, dbDoc, collectionName));
1248+
maybeEmitEvent(new BeforeSaveEvent<>(initialized, dbDoc, collectionName));
12501249

1251-
Mono<T> afterInsert = insertDBObject(collectionName, dbDoc, toSave.getClass()).map(id -> {
1250+
Mono<T> afterInsert = insertDBObject(collectionName, dbDoc, initialized.getClass()).map(id -> {
12521251

12531252
T saved = entity.populateIdIfNecessary(id);
1254-
maybeEmitEvent(new AfterSaveEvent<>(saved, dbDoc, collectionName));
1253+
maybeEmitEvent(new AfterSaveEvent<>(initialized, dbDoc, collectionName));
12551254
return saved;
12561255
});
12571256

@@ -1389,34 +1388,27 @@ public <T> Mono<T> save(T objectToSave, String collectionName) {
13891388
Assert.notNull(objectToSave, "Object to save must not be null!");
13901389
Assert.hasText(collectionName, "Collection name must not be null or empty!");
13911390

1392-
MongoPersistentEntity<?> mongoPersistentEntity = getPersistentEntity(objectToSave.getClass());
1391+
AdaptibleEntity<T> source = operations.forEntity(objectToSave, mongoConverter.getConversionService());
13931392

1394-
// No optimistic locking -> simple save
1395-
if (mongoPersistentEntity == null || !mongoPersistentEntity.hasVersionProperty()) {
1396-
return doSave(collectionName, objectToSave, this.mongoConverter);
1397-
}
1398-
1399-
return doSaveVersioned(objectToSave, mongoPersistentEntity, collectionName);
1393+
return source.isVersionedEntity() ? doSaveVersioned(source, collectionName)
1394+
: doSave(collectionName, objectToSave, this.mongoConverter);
14001395
}
14011396

1402-
private <T> Mono<T> doSaveVersioned(T objectToSave, MongoPersistentEntity<?> entity, String collectionName) {
1397+
private <T> Mono<T> doSaveVersioned(AdaptibleEntity<T> source, String collectionName) {
14031398

1404-
AdaptibleEntity<T> forEntity = operations.forEntity(objectToSave, mongoConverter.getConversionService());
1399+
if (source.isNew()) {
1400+
return doInsert(collectionName, source.getBean(), this.mongoConverter);
1401+
}
14051402

14061403
return createMono(collectionName, collection -> {
14071404

1408-
Number versionNumber = forEntity.getVersion();
1409-
1410-
// Fresh instance -> initialize version property
1411-
if (versionNumber == null) {
1412-
return doInsert(collectionName, objectToSave, mongoConverter);
1413-
}
1414-
1415-
forEntity.assertUpdateableIdIfNotSet();
1405+
// Create query for entity with the id and old version
1406+
Query query = source.getQueryForVersion();
14161407

1417-
Query query = forEntity.getQueryForVersion();
1408+
// Bump version number
1409+
T toSave = source.incrementVersion();
14181410

1419-
T toSave = forEntity.incrementVersion();
1411+
source.assertUpdateableIdIfNotSet();
14201412

14211413
BeforeConvertEvent<T> event = new BeforeConvertEvent<>(toSave, collectionName);
14221414
T afterEvent = ReactiveMongoTemplate.this.maybeEmitEvent(event).getSource();
@@ -1427,7 +1419,9 @@ private <T> Mono<T> doSaveVersioned(T objectToSave, MongoPersistentEntity<?> ent
14271419
ReactiveMongoTemplate.this.maybeEmitEvent(new BeforeSaveEvent<>(afterEvent, document, collectionName));
14281420

14291421
return doUpdate(collectionName, query, mapped.updateWithoutId(), afterEvent.getClass(), false, false)
1430-
.map(updateResult -> maybeEmitEvent(new AfterSaveEvent<T>(afterEvent, document, collectionName)).getSource());
1422+
.map(result -> {
1423+
return maybeEmitEvent(new AfterSaveEvent<T>(afterEvent, document, collectionName)).getSource();
1424+
});
14311425
});
14321426
}
14331427

‎spring-data-mongodb/src/test/java/org/springframework/data/mongodb/config/ReactiveAuditingTests.java‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
/*
2+
* Copyright 2018 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package org.springframework.data.mongodb.config;
17+
18+
import static org.assertj.core.api.Assertions.*;
19+
import static org.mockito.Mockito.*;
20+
21+
import reactor.core.publisher.Mono;
22+
import reactor.test.StepVerifier;
23+
24+
import java.util.concurrent.atomic.AtomicReference;
25+
import java.util.function.Function;
26+
27+
import org.junit.Test;
28+
import org.junit.runner.RunWith;
29+
import org.springframework.beans.factory.annotation.Autowired;
30+
import org.springframework.context.annotation.Bean;
31+
import org.springframework.context.annotation.Configuration;
32+
import org.springframework.data.annotation.Version;
33+
import org.springframework.data.domain.AuditorAware;
34+
import org.springframework.data.mongodb.core.AuditablePerson;
35+
import org.springframework.data.mongodb.core.ReactiveMongoOperations;
36+
import org.springframework.data.mongodb.core.mapping.MongoMappingContext;
37+
import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity;
38+
import org.springframework.data.mongodb.repository.ReactiveMongoRepository;
39+
import org.springframework.data.mongodb.repository.config.EnableReactiveMongoRepositories;
40+
import org.springframework.test.context.ContextConfiguration;
41+
import org.springframework.test.context.junit4.SpringRunner;
42+
43+
import com.mongodb.reactivestreams.client.MongoClient;
44+
import com.mongodb.reactivestreams.client.MongoClients;
45+
46+
/**
47+
* Integration test for the auditing support via {@link org.springframework.data.mongodb.core.ReactiveMongoTemplate}.
48+
*
49+
* @author Mark Paluch
50+
*/
51+
@RunWith(SpringRunner.class)
52+
@ContextConfiguration
53+
public class ReactiveAuditingTests {
54+
55+
@Autowired ReactiveAuditablePersonRepository auditablePersonRepository;
56+
@Autowired AuditorAware<AuditablePerson> auditorAware;
57+
@Autowired MongoMappingContext context;
58+
@Autowired ReactiveMongoOperations operations;
59+
60+
@Configuration
61+
@EnableMongoAuditing(auditorAwareRef = "auditorProvider")
62+
@EnableReactiveMongoRepositories(basePackageClasses = ReactiveAuditingTests.class, considerNestedRepositories = true)
63+
static class Config extends AbstractReactiveMongoConfiguration {
64+
65+
@Override
66+
protected String getDatabaseName() {
67+
return "database";
68+
}
69+
70+
@Override
71+
public MongoClient reactiveMongoClient() {
72+
return MongoClients.create();
73+
}
74+
75+
@Bean
76+
@SuppressWarnings("unchecked")
77+
public AuditorAware<AuditablePerson> auditorProvider() {
78+
return mock(AuditorAware.class);
79+
}
80+
}
81+
82+
@Test // DATAMONGO-2139, DATAMONGO-2150
83+
public void auditingWorksForVersionedEntityWithWrapperVersion() {
84+
85+
verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), //
86+
it -> it.version, //
87+
auditablePersonRepository::save, //
88+
null, 0L, 1L);
89+
}
90+
91+
@Test // DATAMONGO-2139, DATAMONGO-2150
92+
public void auditingWorksForVersionedEntityWithSimpleVersion() {
93+
94+
verifyAuditingViaVersionProperty(new SimpleVersionedAuditablePerson(), //
95+
it -> it.version, //
96+
auditablePersonRepository::save, //
97+
0L, 1L, 2L);
98+
}
99+
100+
@Test // DATAMONGO-2139, DATAMONGO-2150
101+
public void auditingWorksForVersionedEntityWithWrapperVersionOnTemplate() {
102+
103+
verifyAuditingViaVersionProperty(new VersionedAuditablePerson(), //
104+
it -> it.version, //
105+
operations::save, //
106+
null, 0L, 1L);
107+
}
108+
109+
@Test // DATAMONGO-2139, DATAMONGO-2150
110+
public void auditingWorksForVersionedEntityWithSimpleVersionOnTemplate() {
111+
verifyAuditingViaVersionProperty(new SimpleVersionedAuditablePerson(), //
112+
it -> it.version, //
113+
operations::save, //
114+
0L, 1L, 2L);
115+
}
116+
117+
private <T extends AuditablePerson> void verifyAuditingViaVersionProperty(T instance,
118+
Function<T, Object> versionExtractor, Function<T, Mono<T>> persister, Object... expectedValues) {
119+
120+
AtomicReference<T> instanceHolder = new AtomicReference<>(instance);
121+
MongoPersistentEntity<?> entity = context.getRequiredPersistentEntity(instance.getClass());
122+
123+
assertThat(versionExtractor.apply(instance)).isEqualTo(expectedValues[0]);
124+
assertThat(entity.isNew(instance)).isTrue();
125+
126+
persister.apply(instanceHolder.get()) //
127+
.as(StepVerifier::create).consumeNextWith(actual -> {
128+
129+
instanceHolder.set(actual);
130+
131+
assertThat(versionExtractor.apply(actual)).isEqualTo(expectedValues[1]);
132+
assertThat(entity.isNew(actual)).isFalse();
133+
}).verifyComplete();
134+
135+
persister.apply(instanceHolder.get()) //
136+
.as(StepVerifier::create).consumeNextWith(actual -> {
137+
138+
instanceHolder.set(actual);
139+
140+
assertThat(versionExtractor.apply(actual)).isEqualTo(expectedValues[2]);
141+
assertThat(entity.isNew(actual)).isFalse();
142+
}).verifyComplete();
143+
}
144+
145+
interface ReactiveAuditablePersonRepository extends ReactiveMongoRepository<AuditablePerson, String> {}
146+
147+
static class VersionedAuditablePerson extends AuditablePerson {
148+
@Version Long version;
149+
}
150+
151+
static class SimpleVersionedAuditablePerson extends AuditablePerson {
152+
@Version long version;
153+
}
154+
}

‎spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/ReactiveMongoTemplateTests.java‎

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -773,12 +773,9 @@ public void savesMapCorrectly() {
773773
StepVerifier.create(template.save(map, "maps")).expectNextCount(1).verifyComplete();
774774
}
775775

776-
@Test // DATAMONGO-1444, DATAMONGO-1730
776+
@Test(expected = MappingException.class) // DATAMONGO-1444, DATAMONGO-1730, DATAMONGO-2150
777777
public void savesMongoPrimitiveObjectCorrectly() {
778-
779-
StepVerifier.create(template.save(new Object(), "collection")) //
780-
.expectError(MappingException.class) //
781-
.verify();
778+
template.save(new Object(), "collection");
782779
}
783780

784781
@Test // DATAMONGO-1444
@@ -851,12 +848,9 @@ public void writesPlainString() {
851848
.verifyComplete();
852849
}
853850

854-
@Test // DATAMONGO-1444
851+
@Test(expected = MappingException.class) // DATAMONGO-1444, DATAMONGO-2150
855852
public void rejectsNonJsonStringForSave() {
856-
857-
StepVerifier.create(template.save("Foobar!", "collection")) //
858-
.expectError(MappingException.class) //
859-
.verify();
853+
template.save("Foobar!", "collection");
860854
}
861855

862856
@Test // DATAMONGO-1444

0 commit comments

Comments
 (0)