From 673a81af0e458a4f130c978f3aaee6e0ae4e6e26 Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Tue, 5 Oct 2021 17:38:12 +0200 Subject: [PATCH] Fix query/update document reference computation for non-id properties. We now consider using the target keyword when computing document references. This fixes an issue where query/update statements had not been rendered correctly for references like { 'name' : ?#{#target} }. Closes #3853 Original pull request: #3856. --- .../core/convert/DocumentPointerFactory.java | 9 +- .../mongodb/core/convert/QueryMapper.java | 4 + .../core/convert/QueryMapperUnitTests.java | 74 +++++++++++++++ .../core/convert/UpdateMapperUnitTests.java | 91 +++++++++++++++++++ 4 files changed, 177 insertions(+), 1 deletion(-) diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DocumentPointerFactory.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DocumentPointerFactory.java index 2556e9130..45889049e 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DocumentPointerFactory.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/DocumentPointerFactory.java @@ -151,6 +151,7 @@ class DocumentPointerFactory { private final String lookup; private final org.bson.Document documentPointer; private final Map placeholderMap; + private final boolean isSimpleTargetPointer; static LinkageDocument from(String lookup) { return new LinkageDocument(lookup); @@ -177,6 +178,7 @@ class DocumentPointerFactory { } this.documentPointer = org.bson.Document.parse(targetLookup); + this.isSimpleTargetPointer = placeholderMap.size() == 1 && placeholderMap.containsValue("target") && lookup.contains("#target"); } private String placeholder(int index) { @@ -194,7 +196,7 @@ class DocumentPointerFactory { propertyAccessor); } - Document updatePlaceholders(org.bson.Document source, org.bson.Document target, + Object updatePlaceholders(org.bson.Document source, org.bson.Document target, MappingContext, MongoPersistentProperty> mappingContext, MongoPersistentEntity persistentEntity, PersistentPropertyAccessor propertyAccessor) { @@ -245,6 +247,11 @@ class DocumentPointerFactory { target.put(entry.getKey(), entry.getValue()); } + + if(target.size()==1 && isSimpleTargetPointer) { + return target.values().iterator().next(); + } + return target; } } diff --git a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java index 69c85d6b6..051bbdc7b 100644 --- a/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java +++ b/spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/convert/QueryMapper.java @@ -517,6 +517,10 @@ public class QueryMapper { return true; } + if(property.isDocumentReference()) { + return true; + } + MongoPersistentEntity entity = documentField.getPropertyEntity(); return entity.hasIdProperty() && (type.equals(DBRef.class) || entity.getRequiredIdProperty().getActualType().isAssignableFrom(type)); diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java index 11ea78fd4..d102ef16d 100755 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java @@ -50,6 +50,7 @@ import org.springframework.data.mongodb.core.geo.GeoJsonPoint; import org.springframework.data.mongodb.core.geo.GeoJsonPolygon; import org.springframework.data.mongodb.core.mapping.DBRef; import org.springframework.data.mongodb.core.mapping.Document; +import org.springframework.data.mongodb.core.mapping.DocumentReference; import org.springframework.data.mongodb.core.mapping.Field; import org.springframework.data.mongodb.core.mapping.FieldType; import org.springframework.data.mongodb.core.mapping.MongoId; @@ -424,6 +425,60 @@ public class QueryMapperUnitTests { assertThat(inClause.get(0)).isInstanceOf(com.mongodb.DBRef.class); } + @Test // GH-3853 + void convertsDocumentReferenceOnIdPropertyCorrectly() { + + Sample reference = new Sample(); + reference.foo = "s1"; + + Query query = query(where("sample").is(reference)); + org.bson.Document mappedQuery = mapper.getMappedObject(query.getQueryObject(), + context.getPersistentEntity(WithDocumentReference.class)); + + assertThat(mappedQuery).containsEntry("sample", "s1"); + } + + @Test // GH-3853 + void convertsListDocumentReferenceOnIdPropertyCorrectly() { + + Sample reference = new Sample(); + reference.foo = "s1"; + + Query query = query(where("samples").is(Arrays.asList(reference))); + org.bson.Document mappedQuery = mapper.getMappedObject(query.getQueryObject(), + context.getPersistentEntity(WithDocumentReference.class)); + + assertThat(mappedQuery).containsEntry("samples", Arrays.asList("s1")); + } + + @Test // GH-3853 + void convertsDocumentReferenceOnNonIdPropertyCorrectly() { + + Customer reference = new Customer(); + reference.id = new ObjectId(); + reference.name = "c1"; + + Query query = query(where("customer").is(reference)); + org.bson.Document mappedQuery = mapper.getMappedObject(query.getQueryObject(), + context.getPersistentEntity(WithDocumentReference.class)); + + assertThat(mappedQuery).containsEntry("customer", "c1"); + } + + @Test // GH-3853 + void convertsListDocumentReferenceOnNonIdPropertyCorrectly() { + + Customer reference = new Customer(); + reference.id = new ObjectId(); + reference.name = "c1"; + + Query query = query(where("customers").is(Arrays.asList(reference))); + org.bson.Document mappedQuery = mapper.getMappedObject(query.getQueryObject(), + context.getPersistentEntity(WithDocumentReference.class)); + + assertThat(mappedQuery).containsEntry("customers", Arrays.asList("c1")); + } + @Test // DATAMONGO-752 void mapsSimpleValuesStartingWith$Correctly() { @@ -1496,6 +1551,25 @@ public class QueryMapperUnitTests { @DBRef Map mapWithDBRef; } + static class WithDocumentReference { + + private ObjectId id; + + private String name; + + @DocumentReference(lookup = "{ 'name' : ?#{#target} }") // remove `lookup` for the other test case. + private Customer customer; + + @DocumentReference(lookup = "{ 'name' : ?#{#target} }") // remove `lookup` for the other test case. + private List customers; + + @DocumentReference + private Sample sample; + + @DocumentReference + private List samples; + } + class WithTextScoreProperty { @Id String id; diff --git a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/UpdateMapperUnitTests.java b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/UpdateMapperUnitTests.java index 44712fa8d..c217cd510 100644 --- a/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/UpdateMapperUnitTests.java +++ b/spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/UpdateMapperUnitTests.java @@ -31,6 +31,7 @@ import java.util.Map; import java.util.concurrent.atomic.AtomicInteger; 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; @@ -49,6 +50,7 @@ import org.springframework.data.mapping.MappingException; import org.springframework.data.mongodb.MongoDatabaseFactory; import org.springframework.data.mongodb.core.DocumentTestUtils; import org.springframework.data.mongodb.core.MongoExceptionTranslator; +import org.springframework.data.mongodb.core.mapping.DocumentReference; import org.springframework.data.mongodb.core.mapping.Field; import org.springframework.data.mongodb.core.mapping.MongoMappingContext; import org.springframework.data.mongodb.core.mapping.Unwrapped; @@ -1251,6 +1253,64 @@ class UpdateMapperUnitTests { assertThat(mappedUpdate).isEqualTo("{\"$set\": {\"intKeyedMap.1a.map.0b\": \"testing\"}}"); } + @Test // GH-3853 + void updateWithDocuRefOnId() { + + Sample sample = new Sample(); + sample.foo = "s1"; + + Update update = new Update().set("sample", sample); + + Document mappedUpdate = mapper.getMappedObject(update.getUpdateObject(), + context.getPersistentEntity(WithDocumentReference.class)); + + assertThat(mappedUpdate).isEqualTo(new org.bson.Document("$set",new org.bson.Document("sample","s1"))); + } + + @Test // GH-3853 + void updateListWithDocuRefOnId() { + + Sample sample = new Sample(); + sample.foo = "s1"; + + Update update = new Update().set("samples", Arrays.asList(sample)); + + Document mappedUpdate = mapper.getMappedObject(update.getUpdateObject(), + context.getPersistentEntity(WithDocumentReference.class)); + + assertThat(mappedUpdate).isEqualTo(new org.bson.Document("$set",new org.bson.Document("samples",Arrays.asList("s1")))); + } + + @Test // GH-3853 + void updateWithDocuRefOnProperty() { + + Customer customer = new Customer(); + customer.id = new ObjectId(); + customer.name = "c-name"; + + Update update = new Update().set("customer", customer); + + Document mappedUpdate = mapper.getMappedObject(update.getUpdateObject(), + context.getPersistentEntity(WithDocumentReference.class)); + + assertThat(mappedUpdate).isEqualTo(new org.bson.Document("$set",new org.bson.Document("customer","c-name"))); + } + + @Test // GH-3853 + void updateListWithDocuRefOnProperty() { + + Customer customer = new Customer(); + customer.id = new ObjectId(); + customer.name = "c-name"; + + Update update = new Update().set("customers", Arrays.asList(customer)); + + Document mappedUpdate = mapper.getMappedObject(update.getUpdateObject(), + context.getPersistentEntity(WithDocumentReference.class)); + + assertThat(mappedUpdate).isEqualTo(new org.bson.Document("$set",new org.bson.Document("customers", Arrays.asList("c-name")))); + } + static class DomainTypeWrappingConcreteyTypeHavingListOfInterfaceTypeAttributes { ListModelWrapper concreteTypeWithListAttributeOfInterfaceType; } @@ -1621,4 +1681,35 @@ class UpdateMapperUnitTests { Map>> levelOne; } + static class Customer { + + @Id + private ObjectId id; + private String name; + } + + static class Sample { + + @Id private String foo; + } + + static class WithDocumentReference { + + private ObjectId id; + + private String name; + + @DocumentReference(lookup = "{ 'name' : ?#{#target} }") // remove `lookup` for the other test case. + private Customer customer; + + @DocumentReference(lookup = "{ 'name' : ?#{#target} }") // remove `lookup` for the other test case. + private List customers; + + @DocumentReference + private Sample sample; + + @DocumentReference + private List samples; + } + }