From 8ee33b2e50410ed50a294c2647ee12a4dcc63dd3 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 19 May 2022 10:46:17 +0200 Subject: [PATCH] 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. --- .../data/mongodb/core/MappedDocument.java | 10 ++ .../data/mongodb/core/MongoTemplate.java | 15 ++- .../data/mongodb/core/QueryOperations.java | 68 ++++++++++++- .../mongodb/core/ReactiveMongoTemplate.java | 9 +- .../data/mongodb/core/MongoTemplateTests.java | 33 +++++++ .../core/QueryOperationsUnitTests.java | 97 +++++++++++++++++++ .../core/ReactiveMongoTemplateTests.java | 48 ++++++++- src/main/asciidoc/reference/mapping.adoc | 4 +- 8 files changed, 267 insertions(+), 17 deletions(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MappedDocument.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MappedDocument.java index 95b1e2095..b3ba8ce49 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MappedDocument.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/MappedDocument.java @@ -97,6 +97,16 @@ public class MappedDocument { return this.document; } + /** + * Updates the documents {@link #ID_FIELD}. + * + * @param value the {@literal _id} value to set. + * @since 3.4.3 + */ + public void updateId(Object value) { + document.put(ID_FIELD, value); + } + /** * An {@link UpdateDefinition} that indicates that the {@link #getUpdateObject() update object} has already been * mapped to the specific domain type. 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 727f099cc..d93a66c95 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 @@ -1543,18 +1543,21 @@ public class MongoTemplate implements MongoOperations, ApplicationContextAware, collectionName)); } + MappedDocument mappedDocument = queryOperations.createInsertContext(MappedDocument.of(document)) + .prepareId(entityClass); + return execute(collectionName, collection -> { MongoAction mongoAction = new MongoAction(writeConcern, MongoActionOperation.INSERT, collectionName, entityClass, - document, null); + mappedDocument.getDocument(), null); WriteConcern writeConcernToUse = prepareWriteConcern(mongoAction); if (writeConcernToUse == null) { - collection.insertOne(document); + collection.insertOne(mappedDocument.getDocument()); } else { - collection.withWriteConcern(writeConcernToUse).insertOne(document); + collection.withWriteConcern(writeConcernToUse).insertOne(mappedDocument.getDocument()); } - return operations.forEntity(document).getId(); + return operations.forEntity(mappedDocument.getDocument()).getId(); }); } @@ -1605,7 +1608,9 @@ public class MongoTemplate implements MongoOperations, ApplicationContextAware, : collection.withWriteConcern(writeConcernToUse); if (!mapped.hasId()) { - collectionToUse.insertOne(dbDoc); + + mapped = queryOperations.createInsertContext(mapped).prepareId(mappingContext.getPersistentEntity(entityClass)); + collectionToUse.insertOne(mapped.getDocument()); } else { MongoPersistentEntity entity = mappingContext.getPersistentEntity(entityClass); diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/QueryOperations.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/QueryOperations.java index a8633fcf1..fc712461a 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/QueryOperations.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/QueryOperations.java @@ -28,7 +28,7 @@ import java.util.stream.Collectors; import org.bson.BsonValue; import org.bson.Document; import org.bson.codecs.Codec; - +import org.bson.types.ObjectId; import org.springframework.data.mapping.PropertyPath; import org.springframework.data.mapping.PropertyReferenceException; import org.springframework.data.mapping.context.MappingContext; @@ -46,6 +46,7 @@ import org.springframework.data.mongodb.core.aggregation.TypeBasedAggregationOpe import org.springframework.data.mongodb.core.aggregation.TypedAggregation; import org.springframework.data.mongodb.core.convert.QueryMapper; import org.springframework.data.mongodb.core.convert.UpdateMapper; +import org.springframework.data.mongodb.core.mapping.MongoId; import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty; import org.springframework.data.mongodb.core.mapping.ShardKey; @@ -107,6 +108,14 @@ class QueryOperations { this.aggregationUtil = new AggregationUtil(queryMapper, mappingContext); } + InsertContext createInsertContext(Document source) { + return createInsertContext(MappedDocument.of(source)); + } + + InsertContext createInsertContext(MappedDocument mappedDocument) { + return new InsertContext(mappedDocument); + } + /** * Create a new {@link QueryContext} instance. * @@ -227,6 +236,57 @@ class QueryOperations { return new AggregationDefinition(aggregation, aggregationOperationContext); } + /** + * {@link InsertContext} encapsulates common tasks required to interact with {@link Document} to be inserted. + * + * @since 3.4.3 + */ + class InsertContext { + + private final MappedDocument source; + + private InsertContext(MappedDocument source) { + this.source = source; + } + + /** + * Prepare the {@literal _id} field. May generate a new {@literal id} value and convert it to the id properties + * {@link MongoPersistentProperty#getFieldType() target type}. + * + * @param type must not be {@literal null}. + * @param + * @return the {@link MappedDocument} containing the changes. + * @see #prepareId(MongoPersistentEntity) + */ + MappedDocument prepareId(Class type) { + return prepareId(mappingContext.getPersistentEntity(type)); + } + + /** + * Prepare the {@literal _id} field. May generate a new {@literal id} value and convert it to the id properties + * {@link MongoPersistentProperty#getFieldType() target type}. + * + * @param entity can be {@literal null}. + * @param + * @return the {@link MappedDocument} containing the changes. + */ + MappedDocument prepareId(@Nullable MongoPersistentEntity entity) { + + if (entity == null) { + return source; + } + + MongoPersistentProperty idProperty = entity.getIdProperty(); + if (idProperty != null + && (idProperty.hasExplicitWriteTarget() || idProperty.isAnnotationPresent(MongoId.class))) { + if (!ClassUtils.isAssignable(ObjectId.class, idProperty.getFieldType())) { + source.updateId(queryMapper.convertId(new ObjectId(), idProperty.getFieldType())); + } + } + return source; + } + } + /** * {@link QueryContext} encapsulates common tasks required to convert a {@link Query} into its MongoDB document * representation, mapping field names, as well as determining and applying {@link Collation collations}. @@ -288,8 +348,7 @@ class QueryOperations { return queryMapper.getMappedObject(getQueryObject(), entity); } - Document getMappedFields(@Nullable MongoPersistentEntity entity, - EntityProjection projection) { + Document getMappedFields(@Nullable MongoPersistentEntity entity, EntityProjection projection) { Document fields = evaluateFields(entity); @@ -402,8 +461,7 @@ class QueryOperations { } @Override - Document getMappedFields(@Nullable MongoPersistentEntity entity, - EntityProjection projection) { + Document getMappedFields(@Nullable MongoPersistentEntity entity, EntityProjection projection) { return getMappedFields(entity); } 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 b6b351dd0..5e892d8e0 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 @@ -1686,7 +1686,8 @@ public class ReactiveMongoTemplate implements ReactiveMongoOperations, Applicati .format("Inserting Document containing fields: " + dbDoc.keySet() + " in collection: " + collectionName)); } - Document document = new Document(dbDoc); + MappedDocument document = MappedDocument.of(dbDoc); + queryOperations.createInsertContext(document).prepareId(entityClass); Flux execute = execute(collectionName, collection -> { @@ -1696,10 +1697,10 @@ public class ReactiveMongoTemplate implements ReactiveMongoOperations, Applicati MongoCollection collectionToUse = prepareCollection(collection, writeConcernToUse); - return collectionToUse.insertOne(document); + return collectionToUse.insertOne(document.getDocument()); }); - return Flux.from(execute).last().map(success -> MappedDocument.of(document).getId()); + return Flux.from(execute).last().map(success -> document.getId()); } protected Flux insertDocumentList(String collectionName, List dbDocList) { @@ -1763,7 +1764,7 @@ public class ReactiveMongoTemplate implements ReactiveMongoOperations, Applicati Publisher publisher; if (!mapped.hasId()) { - publisher = collectionToUse.insertOne(document); + publisher = collectionToUse.insertOne(queryOperations.createInsertContext(mapped).prepareId(entityClass).getDocument()); } else { MongoPersistentEntity entity = mappingContext.getPersistentEntity(entityClass); diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java index ddb4ece68..652bff84c 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/MongoTemplateTests.java @@ -39,6 +39,7 @@ import java.util.concurrent.atomic.AtomicReference; import java.util.stream.Collectors; import java.util.stream.IntStream; +import org.bson.Document; import org.bson.types.ObjectId; import org.joda.time.DateTime; import org.junit.jupiter.api.AfterEach; @@ -3658,6 +3659,38 @@ public class MongoTemplateTests { assertThat(target).isEqualTo(source); } + @Test // GH-4026 + void saveShouldGenerateNewIdOfTypeIfExplicitlyDefined() { + + RawStringId source = new RawStringId(); + source.value = "new value"; + + template.save(source); + + template.execute(RawStringId.class, collection -> { + + org.bson.Document first = collection.find(new org.bson.Document()).first(); + assertThat(first.get("_id")).isInstanceOf(String.class); + return null; + }); + } + + @Test // GH-4026 + void insertShouldGenerateNewIdOfTypeIfExplicitlyDefined() { + + RawStringId source = new RawStringId(); + source.value = "new value"; + + template.insert(source); + + template.execute(RawStringId.class, collection -> { + + org.bson.Document first = collection.find(new org.bson.Document()).first(); + assertThat(first.get("_id")).isInstanceOf(String.class); + return null; + }); + } + @Test // DATAMONGO-2193 public void shouldNotConvertStringToObjectIdForNonIdField() { diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/QueryOperationsUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/QueryOperationsUnitTests.java index afd1658e9..0a01bed2b 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/QueryOperationsUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/QueryOperationsUnitTests.java @@ -18,6 +18,8 @@ package org.springframework.data.mongodb.core; import static org.assertj.core.api.Assertions.*; import static org.mockito.Mockito.*; +import org.bson.Document; +import org.bson.types.ObjectId; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -32,7 +34,10 @@ import org.springframework.data.mongodb.core.aggregation.RelaxedTypeBasedAggrega import org.springframework.data.mongodb.core.aggregation.TypeBasedAggregationOperationContext; import org.springframework.data.mongodb.core.convert.QueryMapper; import org.springframework.data.mongodb.core.convert.UpdateMapper; +import org.springframework.data.mongodb.core.mapping.MongoId; import org.springframework.data.mongodb.core.mapping.MongoMappingContext; +import org.springframework.data.mongodb.core.mapping.MongoPersistentEntity; +import org.springframework.data.mongodb.core.mapping.MongoPersistentProperty; /** * Unit tests for {@link QueryOperations}. @@ -109,7 +114,99 @@ class QueryOperationsUnitTests { assertThat(ctx.getAggregationOperationContext()).isInstanceOf(TypeBasedAggregationOperationContext.class); } + @Test // GH-4026 + void insertContextDoesNotAddIdIfNoPersistentEntityCanBeFound() { + + assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())// + .satisfies(result -> { + assertThat(result).isEqualTo(new Document("value", "one")); + }); + } + + @Test // GH-4026 + void insertContextDoesNotAddIdIfNoIdPropertyCanBeFound() { + + MongoPersistentEntity entity = mock(MongoPersistentEntity.class); + when(entity.getIdProperty()).thenReturn(null); + when(mappingContext.getPersistentEntity(eq(Person.class))).thenReturn((MongoPersistentEntity) entity); + + assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())// + .satisfies(result -> { + assertThat(result).isEqualTo(new Document("value", "one")); + }); + } + + @Test // GH-4026 + void insertContextDoesNotAddConvertedIdForNonExplicitFieldTypes() { + + MongoPersistentEntity entity = mock(MongoPersistentEntity.class); + MongoPersistentProperty property = mock(MongoPersistentProperty.class); + when(entity.getIdProperty()).thenReturn(property); + when(property.hasExplicitWriteTarget()).thenReturn(false); + doReturn(entity).when(mappingContext).getPersistentEntity(eq(Person.class)); + + assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())// + .satisfies(result -> { + assertThat(result).isEqualTo(new Document("value", "one")); + }); + } + + @Test // GH-4026 + void insertContextAddsConvertedIdForExplicitFieldTypes() { + + MongoPersistentEntity entity = mock(MongoPersistentEntity.class); + MongoPersistentProperty property = mock(MongoPersistentProperty.class); + when(entity.getIdProperty()).thenReturn(property); + when(property.hasExplicitWriteTarget()).thenReturn(true); + doReturn(String.class).when(property).getFieldType(); + doReturn(entity).when(mappingContext).getPersistentEntity(eq(Person.class)); + + when(queryMapper.convertId(any(), eq(String.class))).thenReturn("☮"); + + assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())// + .satisfies(result -> { + assertThat(result).isEqualTo(new Document("value", "one").append("_id", "☮")); + }); + } + + @Test // GH-4026 + void insertContextAddsConvertedIdForMongoIdTypes() { + + MongoPersistentEntity entity = mock(MongoPersistentEntity.class); + MongoPersistentProperty property = mock(MongoPersistentProperty.class); + when(entity.getIdProperty()).thenReturn(property); + when(property.hasExplicitWriteTarget()).thenReturn(false); + when(property.isAnnotationPresent(eq(MongoId.class))).thenReturn(true); + doReturn(String.class).when(property).getFieldType(); + doReturn(entity).when(mappingContext).getPersistentEntity(eq(Person.class)); + + when(queryMapper.convertId(any(), eq(String.class))).thenReturn("☮"); + + assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())// + .satisfies(result -> { + assertThat(result).isEqualTo(new Document("value", "one").append("_id", "☮")); + }); + } + + @Test // GH-4026 + void insertContextDoesNotAddConvertedIdForMongoIdTypesTargetingObjectId() { + + MongoPersistentEntity entity = mock(MongoPersistentEntity.class); + MongoPersistentProperty property = mock(MongoPersistentProperty.class); + when(entity.getIdProperty()).thenReturn(property); + when(property.hasExplicitWriteTarget()).thenReturn(false); + when(property.isAnnotationPresent(eq(MongoId.class))).thenReturn(true); + doReturn(ObjectId.class).when(property).getFieldType(); + doReturn(entity).when(mappingContext).getPersistentEntity(eq(Person.class)); + + assertThat(queryOperations.createInsertContext(new Document("value", "one")).prepareId(Person.class).getDocument())// + .satisfies(result -> { + assertThat(result).isEqualTo(new Document("value", "one")); + }); + } + static class Person { } + } 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 2891b7323..931b6926c 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 @@ -72,6 +72,7 @@ import org.springframework.data.mongodb.core.index.GeoSpatialIndexType; import org.springframework.data.mongodb.core.index.GeospatialIndex; import org.springframework.data.mongodb.core.index.Index; import org.springframework.data.mongodb.core.index.IndexOperationsAdapter; +import org.springframework.data.mongodb.core.mapping.MongoId; import org.springframework.data.mongodb.core.mapping.event.AbstractMongoEventListener; import org.springframework.data.mongodb.core.mapping.event.AfterSaveEvent; import org.springframework.data.mongodb.core.query.Criteria; @@ -118,7 +119,8 @@ public class ReactiveMongoTemplateTests { void setUp() { template - .flush(Person.class, MyPerson.class, Sample.class, Venue.class, PersonWithVersionPropertyOfTypeInteger.class) // + .flush(Person.class, MyPerson.class, Sample.class, Venue.class, PersonWithVersionPropertyOfTypeInteger.class, + RawStringId.class) // .as(StepVerifier::create) // .verifyComplete(); @@ -181,6 +183,42 @@ public class ReactiveMongoTemplateTests { assertThat(person.getId()).isNotNull(); } + @Test // GH-4026 + void saveShouldGenerateNewIdOfTypeIfExplicitlyDefined() { + + RawStringId source = new RawStringId(); + source.value = "new value"; + + template.save(source).then().as(StepVerifier::create).verifyComplete(); + + template.execute(RawStringId.class, collection -> { + return collection.find(new org.bson.Document()).first(); + }) // + .map(it -> it.get("_id")) // + .as(StepVerifier::create) // + .consumeNextWith(id -> { + assertThat(id).isInstanceOf(String.class); + }).verifyComplete(); + } + + @Test // GH-4026 + void insertShouldGenerateNewIdOfTypeIfExplicitlyDefined() { + + RawStringId source = new RawStringId(); + source.value = "new value"; + + template.insert(source).then().as(StepVerifier::create).verifyComplete(); + + template.execute(RawStringId.class, collection -> { + return collection.find(new org.bson.Document()).first(); + }) // + .map(it -> it.get("_id")) // + .as(StepVerifier::create) // + .consumeNextWith(id -> { + assertThat(id).isInstanceOf(String.class); + }).verifyComplete(); + } + @Test // DATAMONGO-1444 void insertsSimpleEntityCorrectly() { @@ -1821,4 +1859,12 @@ public class ReactiveMongoTemplateTests { interface MyPersonProjection { String getName(); } + + @Data + static class RawStringId { + + @MongoId String id; + String value; + } + } diff --git a/src/main/asciidoc/reference/mapping.adoc b/src/main/asciidoc/reference/mapping.adoc index f837bfb6d..9d5ec3a8c 100644 --- a/src/main/asciidoc/reference/mapping.adoc +++ b/src/main/asciidoc/reference/mapping.adoc @@ -58,8 +58,8 @@ The following outlines what field will be mapped to the `_id` document field: The following outlines what type conversion, if any, will be done on the property mapped to the _id document field. * If a field named `id` is declared as a String or BigInteger in the Java class it will be converted to and stored as an ObjectId if possible. ObjectId as a field type is also valid. If you specify a value for `id` in your application, the conversion to an ObjectId is detected to the MongoDB driver. If the specified `id` value cannot be converted to an ObjectId, then the value will be stored as is in the document's _id field. This also applies if the field is annotated with `@Id`. -* If a field is annotated with `@MongoId` in the Java class it will be converted to and stored as using its actual type. No further conversion happens unless `@MongoId` declares a desired field type. -* If a field is annotated with `@MongoId(FieldType.…)` in the Java class it will be attempted to convert the value to the declared `FieldType.` +* If a field is annotated with `@MongoId` in the Java class it will be converted to and stored as using its actual type. No further conversion happens unless `@MongoId` declares a desired field type. If no value is provided for the `id` field, a new `ObjectId` will be created and converted to the properties type. +* If a field is annotated with `@MongoId(FieldType.…)` in the Java class it will be attempted to convert the value to the declared `FieldType`. If no value is provided for the `id` field, a new `ObjectId` will be created and converted to the declared type. * If a field named `id` id field is not declared as a String, BigInteger, or ObjectID in the Java class then you should assign it a value in your application so it can be stored 'as-is' in the document's _id field. * If no field named `id` is present in the Java class then an implicit `_id` file will be generated by the driver but not mapped to a property or field of the Java class.