Skip to content

Commit ae2846c

Browse files
christophstroblmp911de
christophstrobl
authored and
mp911de
committed
Generate and convert id on insert if explicitly defined.
We now make sure to provide an id value that matches the desired target type when no id is set, and the property defines an explicit conversion target. Previously a new ObjectId would have been generated which leads to type inconsistencies when querying for _id. Closes #4026 Original pull request: #4057.
1 parent e88c9cf commit ae2846c

File tree

8 files changed

+267
-17
lines changed

8 files changed

+267
-17
lines changed

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,16 @@ public Document getDocument() {
9797
return this.document;
9898
}
9999

100+
/**
101+
* Updates the documents {@link #ID_FIELD}.
102+
*
103+
* @param value the {@literal _id} value to set.
104+
* @since 3.4.3
105+
*/
106+
public void updateId(Object value) {
107+
document.put(ID_FIELD, value);
108+
}
109+
100110
/**
101111
* An {@link UpdateDefinition} that indicates that the {@link #getUpdateObject() update object} has already been
102112
* mapped to the specific domain type.

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

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1443,18 +1443,21 @@ protected Object insertDocument(String collectionName, Document document, Class<
14431443
collectionName));
14441444
}
14451445

1446+
MappedDocument mappedDocument = queryOperations.createInsertContext(MappedDocument.of(document))
1447+
.prepareId(entityClass);
1448+
14461449
return execute(collectionName, collection -> {
14471450
MongoAction mongoAction = new MongoAction(writeConcern, MongoActionOperation.INSERT, collectionName, entityClass,
1448-
document, null);
1451+
mappedDocument.getDocument(), null);
14491452
WriteConcern writeConcernToUse = prepareWriteConcern(mongoAction);
14501453

14511454
if (writeConcernToUse == null) {
1452-
collection.insertOne(document);
1455+
collection.insertOne(mappedDocument.getDocument());
14531456
} else {
1454-
collection.withWriteConcern(writeConcernToUse).insertOne(document);
1457+
collection.withWriteConcern(writeConcernToUse).insertOne(mappedDocument.getDocument());
14551458
}
14561459

1457-
return operations.forEntity(document).getId();
1460+
return operations.forEntity(mappedDocument.getDocument()).getId();
14581461
});
14591462
}
14601463

@@ -1505,7 +1508,9 @@ protected Object saveDocument(String collectionName, Document dbDoc, Class<?> en
15051508
: collection.withWriteConcern(writeConcernToUse);
15061509

15071510
if (!mapped.hasId()) {
1508-
collectionToUse.insertOne(dbDoc);
1511+
1512+
mapped = queryOperations.createInsertContext(mapped).prepareId(mappingContext.getPersistentEntity(entityClass));
1513+
collectionToUse.insertOne(mapped.getDocument());
15091514
} else {
15101515

15111516
MongoPersistentEntity<?> entity = mappingContext.getPersistentEntity(entityClass);

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

Lines changed: 63 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828
import org.bson.BsonValue;
2929
import org.bson.Document;
3030
import org.bson.codecs.Codec;
31-
31+
import org.bson.types.ObjectId;
3232
import org.springframework.data.mapping.PropertyPath;
3333
import org.springframework.data.mapping.PropertyReferenceException;
3434
import org.springframework.data.mapping.context.MappingContext;
@@ -46,6 +46,7 @@
4646
import org.springframework.data.mongodb.core.aggregation.TypedAggregation;
4747
import org.springframework.data.mongodb.core.convert.QueryMapper;
4848
import org.springframework.data.mongodb.core.convert.UpdateMapper;
49+
import org.springframework.data.mongodb.core.mapping.MongoId;
4950
import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity;
5051
import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty;
5152
import org.springframework.data.mongodb.core.mapping.ShardKey;
@@ -107,6 +108,14 @@ class QueryOperations {
107108
this.aggregationUtil = new AggregationUtil(queryMapper, mappingContext);
108109
}
109110

111+
InsertContext createInsertContext(Document source) {
112+
return createInsertContext(MappedDocument.of(source));
113+
}
114+
115+
InsertContext createInsertContext(MappedDocument mappedDocument) {
116+
return new InsertContext(mappedDocument);
117+
}
118+
110119
/**
111120
* Create a new {@link QueryContext} instance.
112121
*
@@ -227,6 +236,57 @@ AggregationDefinition createAggregation(Aggregation aggregation,
227236
return new AggregationDefinition(aggregation, aggregationOperationContext);
228237
}
229238

239+
/**
240+
* {@link InsertContext} encapsulates common tasks required to interact with {@link Document} to be inserted.
241+
*
242+
* @since 3.4.3
243+
*/
244+
class InsertContext {
245+
246+
private final MappedDocument source;
247+
248+
private InsertContext(MappedDocument source) {
249+
this.source = source;
250+
}
251+
252+
/**
253+
* Prepare the {@literal _id} field. May generate a new {@literal id} value and convert it to the id properties
254+
* {@link MongoPersistentProperty#getFieldType() target type}.
255+
*
256+
* @param type must not be {@literal null}.
257+
* @param <T>
258+
* @return the {@link MappedDocument} containing the changes.
259+
* @see #prepareId(MongoPersistentEntity)
260+
*/
261+
<T> MappedDocument prepareId(Class<T> type) {
262+
return prepareId(mappingContext.getPersistentEntity(type));
263+
}
264+
265+
/**
266+
* Prepare the {@literal _id} field. May generate a new {@literal id} value and convert it to the id properties
267+
* {@link MongoPersistentProperty#getFieldType() target type}.
268+
*
269+
* @param entity can be {@literal null}.
270+
* @param <T>
271+
* @return the {@link MappedDocument} containing the changes.
272+
*/
273+
<T> MappedDocument prepareId(@Nullable MongoPersistentEntity<T> entity) {
274+
275+
if (entity == null) {
276+
return source;
277+
}
278+
279+
MongoPersistentProperty idProperty = entity.getIdProperty();
280+
if (idProperty != null
281+
&& (idProperty.hasExplicitWriteTarget() || idProperty.isAnnotationPresent(MongoId.class))) {
282+
if (!ClassUtils.isAssignable(ObjectId.class, idProperty.getFieldType())) {
283+
source.updateId(queryMapper.convertId(new ObjectId(), idProperty.getFieldType()));
284+
}
285+
}
286+
return source;
287+
}
288+
}
289+
230290
/**
231291
* {@link QueryContext} encapsulates common tasks required to convert a {@link Query} into its MongoDB document
232292
* representation, mapping field names, as well as determining and applying {@link Collation collations}.
@@ -288,8 +348,7 @@ <T> Document getMappedQuery(@Nullable MongoPersistentEntity<T> entity) {
288348
return queryMapper.getMappedObject(getQueryObject(), entity);
289349
}
290350

291-
Document getMappedFields(@Nullable MongoPersistentEntity<?> entity,
292-
EntityProjection<?, ?> projection) {
351+
Document getMappedFields(@Nullable MongoPersistentEntity<?> entity, EntityProjection<?, ?> projection) {
293352

294353
Document fields = evaluateFields(entity);
295354

@@ -402,8 +461,7 @@ private DistinctQueryContext(@Nullable Object query, String fieldName) {
402461
}
403462

404463
@Override
405-
Document getMappedFields(@Nullable MongoPersistentEntity<?> entity,
406-
EntityProjection<?, ?> projection) {
464+
Document getMappedFields(@Nullable MongoPersistentEntity<?> entity, EntityProjection<?, ?> projection) {
407465
return getMappedFields(entity);
408466
}
409467

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1448,7 +1448,8 @@ protected Mono<Object> insertDocument(String collectionName, Document dbDoc, Cla
14481448
.format("Inserting Document containing fields: " + dbDoc.keySet() + " in collection: " + collectionName));
14491449
}
14501450

1451-
Document document = new Document(dbDoc);
1451+
MappedDocument document = MappedDocument.of(dbDoc);
1452+
queryOperations.createInsertContext(document).prepareId(entityClass);
14521453

14531454
Flux<InsertOneResult> execute = execute(collectionName, collection -> {
14541455

@@ -1458,10 +1459,10 @@ protected Mono<Object> insertDocument(String collectionName, Document dbDoc, Cla
14581459

14591460
MongoCollection<Document> collectionToUse = prepareCollection(collection, writeConcernToUse);
14601461

1461-
return collectionToUse.insertOne(document);
1462+
return collectionToUse.insertOne(document.getDocument());
14621463
});
14631464

1464-
return Flux.from(execute).last().map(success -> MappedDocument.of(document).getId());
1465+
return Flux.from(execute).last().map(success -> document.getId());
14651466
}
14661467

14671468
protected Flux<ObjectId> insertDocumentList(String collectionName, List<Document> dbDocList) {
@@ -1525,7 +1526,7 @@ protected Mono<Object> saveDocument(String collectionName, Document document, Cl
15251526

15261527
Publisher<?> publisher;
15271528
if (!mapped.hasId()) {
1528-
publisher = collectionToUse.insertOne(document);
1529+
publisher = collectionToUse.insertOne(queryOperations.createInsertContext(mapped).prepareId(entityClass).getDocument());
15291530
} else {
15301531

15311532
MongoPersistentEntity<?> entity = mappingContext.getPersistentEntity(entityClass);

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
import java.util.stream.IntStream;
4343
import java.util.stream.Stream;
4444

45+
import org.bson.Document;
4546
import org.bson.types.ObjectId;
4647
import org.junit.jupiter.api.AfterEach;
4748
import org.junit.jupiter.api.Test;
@@ -3636,6 +3637,38 @@ public void saveAndLoadStringThatIsAnObjectIdAsString() {
36363637
assertThat(target).isEqualTo(source);
36373638
}
36383639

3640+
@Test // GH-4026
3641+
void saveShouldGenerateNewIdOfTypeIfExplicitlyDefined() {
3642+
3643+
RawStringId source = new RawStringId();
3644+
source.value = "new value";
3645+
3646+
template.save(source);
3647+
3648+
template.execute(RawStringId.class, collection -> {
3649+
3650+
org.bson.Document first = collection.find(new org.bson.Document()).first();
3651+
assertThat(first.get("_id")).isInstanceOf(String.class);
3652+
return null;
3653+
});
3654+
}
3655+
3656+
@Test // GH-4026
3657+
void insertShouldGenerateNewIdOfTypeIfExplicitlyDefined() {
3658+
3659+
RawStringId source = new RawStringId();
3660+
source.value = "new value";
3661+
3662+
template.insert(source);
3663+
3664+
template.execute(RawStringId.class, collection -> {
3665+
3666+
org.bson.Document first = collection.find(new org.bson.Document()).first();
3667+
assertThat(first.get("_id")).isInstanceOf(String.class);
3668+
return null;
3669+
});
3670+
}
3671+
36393672
@Test // DATAMONGO-2193
36403673
public void shouldNotConvertStringToObjectIdForNonIdField() {
36413674

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

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818
import static org.assertj.core.api.Assertions.*;
1919
import static org.mockito.Mockito.*;
2020

21+
import org.bson.Document;
22+
import org.bson.types.ObjectId;
2123
import org.junit.jupiter.api.BeforeEach;
2224
import org.junit.jupiter.api.Test;
2325
import org.junit.jupiter.api.extension.ExtendWith;
@@ -32,7 +34,10 @@
3234
import org.springframework.data.mongodb.core.aggregation.TypeBasedAggregationOperationContext;
3335
import org.springframework.data.mongodb.core.convert.QueryMapper;
3436
import org.springframework.data.mongodb.core.convert.UpdateMapper;
37+
import org.springframework.data.mongodb.core.mapping.MongoId;
3538
import org.springframework.data.mongodb.core.mapping.MongoMappingContext;
39+
import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity;
40+
import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty;
3641

3742
/**
3843
* Unit tests for {@link QueryOperations}.
@@ -109,7 +114,99 @@ void createAggregationContextUsesStrictlyTypedContextForTypedAggregationsWhenReq
109114
assertThat(ctx.getAggregationOperationContext()).isInstanceOf(TypeBasedAggregationOperationContext.class);
110115
}
111116

117+
@Test // GH-4026
118+
void insertContextDoesNotAddIdIfNoPersistentEntityCanBeFound() {
119+
120+
assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())//
121+
.satisfies(result -> {
122+
assertThat(result).isEqualTo(new Document("value", "one"));
123+
});
124+
}
125+
126+
@Test // GH-4026
127+
void insertContextDoesNotAddIdIfNoIdPropertyCanBeFound() {
128+
129+
MongoPersistentEntity<Person> entity = mock(MongoPersistentEntity.class);
130+
when(entity.getIdProperty()).thenReturn(null);
131+
when(mappingContext.getPersistentEntity(eq(Person.class))).thenReturn((MongoPersistentEntity) entity);
132+
133+
assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())//
134+
.satisfies(result -> {
135+
assertThat(result).isEqualTo(new Document("value", "one"));
136+
});
137+
}
138+
139+
@Test // GH-4026
140+
void insertContextDoesNotAddConvertedIdForNonExplicitFieldTypes() {
141+
142+
MongoPersistentEntity<Person> entity = mock(MongoPersistentEntity.class);
143+
MongoPersistentProperty property = mock(MongoPersistentProperty.class);
144+
when(entity.getIdProperty()).thenReturn(property);
145+
when(property.hasExplicitWriteTarget()).thenReturn(false);
146+
doReturn(entity).when(mappingContext).getPersistentEntity(eq(Person.class));
147+
148+
assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())//
149+
.satisfies(result -> {
150+
assertThat(result).isEqualTo(new Document("value", "one"));
151+
});
152+
}
153+
154+
@Test // GH-4026
155+
void insertContextAddsConvertedIdForExplicitFieldTypes() {
156+
157+
MongoPersistentEntity<Person> entity = mock(MongoPersistentEntity.class);
158+
MongoPersistentProperty property = mock(MongoPersistentProperty.class);
159+
when(entity.getIdProperty()).thenReturn(property);
160+
when(property.hasExplicitWriteTarget()).thenReturn(true);
161+
doReturn(String.class).when(property).getFieldType();
162+
doReturn(entity).when(mappingContext).getPersistentEntity(eq(Person.class));
163+
164+
when(queryMapper.convertId(any(), eq(String.class))).thenReturn("&#9774;");
165+
166+
assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())//
167+
.satisfies(result -> {
168+
assertThat(result).isEqualTo(new Document("value", "one").append("_id", "&#9774;"));
169+
});
170+
}
171+
172+
@Test // GH-4026
173+
void insertContextAddsConvertedIdForMongoIdTypes() {
174+
175+
MongoPersistentEntity<Person> entity = mock(MongoPersistentEntity.class);
176+
MongoPersistentProperty property = mock(MongoPersistentProperty.class);
177+
when(entity.getIdProperty()).thenReturn(property);
178+
when(property.hasExplicitWriteTarget()).thenReturn(false);
179+
when(property.isAnnotationPresent(eq(MongoId.class))).thenReturn(true);
180+
doReturn(String.class).when(property).getFieldType();
181+
doReturn(entity).when(mappingContext).getPersistentEntity(eq(Person.class));
182+
183+
when(queryMapper.convertId(any(), eq(String.class))).thenReturn("&#9774;");
184+
185+
assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())//
186+
.satisfies(result -> {
187+
assertThat(result).isEqualTo(new Document("value", "one").append("_id", "&#9774;"));
188+
});
189+
}
190+
191+
@Test // GH-4026
192+
void insertContextDoesNotAddConvertedIdForMongoIdTypesTargetingObjectId() {
193+
194+
MongoPersistentEntity<Person> entity = mock(MongoPersistentEntity.class);
195+
MongoPersistentProperty property = mock(MongoPersistentProperty.class);
196+
when(entity.getIdProperty()).thenReturn(property);
197+
when(property.hasExplicitWriteTarget()).thenReturn(false);
198+
when(property.isAnnotationPresent(eq(MongoId.class))).thenReturn(true);
199+
doReturn(ObjectId.class).when(property).getFieldType();
200+
doReturn(entity).when(mappingContext).getPersistentEntity(eq(Person.class));
201+
202+
assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())//
203+
.satisfies(result -> {
204+
assertThat(result).isEqualTo(new Document("value", "one"));
205+
});
206+
}
207+
112208
static class Person {
113209

114210
}
211+
115212
}

0 commit comments

Comments
 (0)