DATAMONGO-1050 - Explicitly annotated Field should not be considered Id.

We changed the id resolution to skip properties having an explicit name set via @field unless they are marked with @id. This means that

@field(“id”) String id;

will be stored as “id” within mongodb. Prior to this change the fieldname would have been changed to “_id”.
Added tests to ensure proper field mapping for various "id" field variants.

Original pull request: spring-projects#225.

Author: christophstrobl

spring-data-mongodb/src/main/java/org/springframework/data/mongodb/core/mapping/BasicMongoPersistentProperty.java

@@ -100,7 +100,8 @@ public boolean isIdProperty() {
 		}
 
 		// We need to support a wider range of ID types than just the ones that can be converted to an ObjectId
-		return SUPPORTED_ID_PROPERTY_NAMES.contains(getName());
+		// but still we need to check if there happens to be an explicit name set
+		return SUPPORTED_ID_PROPERTY_NAMES.contains(getName()) && !hasExplicitFieldName();
 	}
 
 	/*
@@ -134,10 +135,8 @@ public String getFieldName() {
 			}
 		}
 
-		org.springframework.data.mongodb.core.mapping.Field annotation = findAnnotation(org.springframework.data.mongodb.core.mapping.Field.class);
-
-		if (annotation != null && StringUtils.hasText(annotation.value())) {
-			return annotation.value();
+		if (hasExplicitFieldName()) {
+			return getAnnotatedFieldName();
 		}
 
 		String fieldName = fieldNamingStrategy.getFieldName(this);
@@ -150,6 +149,26 @@ public String getFieldName() {
 		return fieldName;
 	}
 
+	/**
+	 * @return true if {@link org.springframework.data.mongodb.core.mapping.Field} having non blank
+	 *         {@link org.springframework.data.mongodb.core.mapping.Field#value()} present.
+	 * @since 1.7
+	 */
+	protected boolean hasExplicitFieldName() {
+		return StringUtils.hasText(getAnnotatedFieldName());
+	}
+
+	private String getAnnotatedFieldName() {
+
+		org.springframework.data.mongodb.core.mapping.Field annotation = findAnnotation(org.springframework.data.mongodb.core.mapping.Field.class);
+
+		if (annotation != null && StringUtils.hasText(annotation.value())) {
+			return annotation.value();
+		}
+
+		return null;
+	}
+
 	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.mongodb.core.mapping.MongoPersistentProperty#getFieldOrder()

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/MappingMongoConverterUnitTests.java

@@ -83,6 +83,7 @@
 
 import com.mongodb.BasicDBList;
 import com.mongodb.BasicDBObject;
+import com.mongodb.BasicDBObjectBuilder;
 import com.mongodb.DB;
 import com.mongodb.DBObject;
 import com.mongodb.DBRef;
@@ -1869,6 +1870,81 @@ public void readShouldRespectExplicitFieldNameForDbRef() {
 				Mockito.any(DbRefResolverCallback.class), Mockito.any(DbRefProxyHandler.class));
 	}
 
+	/**
+	 * @see DATAMONGO-1050
+	 */
+	@Test
+	public void writeShouldUseExplicitFieldnameForIdPropertyWhenAnnotated() {
+
+		RootForClassWithExplicitlyRenamedIdField source = new RootForClassWithExplicitlyRenamedIdField();
+		source.id = "rootId";
+		source.nested = new ClassWithExplicitlyRenamedField();
+		source.nested.id = "nestedId";
+
+		DBObject sink = new BasicDBObject();
+		converter.write(source, sink);
+
+		assertThat((String) sink.get("_id"), is("rootId"));
+		assertThat((DBObject) sink.get("nested"), is(new BasicDBObjectBuilder().add("id", "nestedId").get()));
+	}
+
+	/**
+	 * @see DATAMONGO-1050
+	 */
+	@Test
+	public void readShouldUseExplicitFieldnameForIdPropertyWhenAnnotated() {
+
+		DBObject source = new BasicDBObjectBuilder().add("_id", "rootId")
+				.add("nested", new BasicDBObject("id", "nestedId")).get();
+
+		RootForClassWithExplicitlyRenamedIdField sink = converter.read(RootForClassWithExplicitlyRenamedIdField.class,
+				source);
+
+		assertThat(sink.id, is("rootId"));
+		assertThat(sink.nested, notNullValue());
+		assertThat(sink.nested.id, is("nestedId"));
+	}
+
+	/**
+	 * @see DATAMONGO-1050
+	 */
+	@Test
+	public void namedIdFieldShouldExtractValueFromUnderscoreIdField() {
+
+		DBObject dbo = new BasicDBObjectBuilder().add("_id", "A").add("id", "B").get();
+
+		ClassWithNamedIdField withNamedIdField = converter.read(ClassWithNamedIdField.class, dbo);
+
+		assertThat(withNamedIdField.id, is("A"));
+	}
+	
+	/**
+	 * @see DATAMONGO-1050
+	 */
+	@Test
+	public void explicitlyRenamedIfFieldShouldExtractValueFromIdField() {
+
+		DBObject dbo = new BasicDBObjectBuilder().add("_id", "A").add("id", "B").get();
+
+		ClassWithExplicitlyRenamedField withExplicitlyRenamedField = converter.read(ClassWithExplicitlyRenamedField.class,
+				dbo);
+
+		assertThat(withExplicitlyRenamedField.id, is("B"));
+	}
+	
+	/**
+	 * @see DATAMONGO-1050
+	 */
+	@Test
+	public void annotatedIdFieldShouldExtractValueFromUnderscoreIdField() {
+
+		DBObject dbo = new BasicDBObjectBuilder().add("_id", "A").add("id", "B").get();
+
+		ClassWithAnnotatedIdField withAnnotatedIdField = converter.read(ClassWithAnnotatedIdField.class, dbo);
+
+		assertThat(withAnnotatedIdField.key, is("A"));
+	}
+
 	static class GenericType<T> {
 		T content;
 	}
@@ -2128,6 +2204,32 @@ class ClassWithExplicitlyNamedDBRefProperty {
 		public ClassWithIntId getDbRefProperty() {
 			return dbRefProperty;
 		}
+	}
+
+	static class RootForClassWithExplicitlyRenamedIdField {
+
+		@Id String id;
+		ClassWithExplicitlyRenamedField nested;
+	}
+
+	static class ClassWithExplicitlyRenamedField {
+
+		@Field("id") String id;
+	}
+
+	static class RootForClassWithNamedIdField {
+
+		String id;
+		ClassWithNamedIdField nested;
+	}
+
+	static class ClassWithNamedIdField {
+
+		String id;
+	}
+
+	static class ClassWithAnnotatedIdField {
 
+		@Id String key;
 	}
 }

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/convert/QueryMapperUnitTests.java

@@ -674,6 +674,34 @@ public void mapsIdReferenceToDBRefCorrectly() {
 		assertThat(reference.getId(), is(instanceOf(ObjectId.class)));
 	}
 
+	/**
+	 * @see DATAMONGO-1050
+	 */
+	@Test
+	public void shouldUseExplicitlySetFieldnameForIdPropertyCandidates() {
+
+		Query query = query(where("nested.id").is("bar"));
+
+		DBObject dbo = mapper.getMappedObject(query.getQueryObject(),
+				context.getPersistentEntity(RootForClassWithExplicitlyRenamedIdField.class));
+
+		assertThat(dbo, equalTo(new BasicDBObjectBuilder().add("nested.id", "bar").get()));
+	}
+
+	/**
+	 * @see DATAMONGO-1050
+	 */
+	@Test
+	public void shouldUseExplicitlySetFieldnameForIdPropertyCandidatesUsedInSortClause() {
+
+		Query query = new Query().with(new Sort("nested.id"));
+
+		DBObject dbo = mapper.getMappedSort(query.getSortObject(),
+				context.getPersistentEntity(RootForClassWithExplicitlyRenamedIdField.class));
+
+		assertThat(dbo, equalTo(new BasicDBObjectBuilder().add("nested.id", 1).get()));
+	}
+
 	@Document
 	public class Foo {
 		@Id private ObjectId id;
@@ -755,4 +783,15 @@ class WithTextScoreProperty {
 		@Id String id;
 		@TextScore @Field("score") Float textScore;
 	}
+
+	static class RootForClassWithExplicitlyRenamedIdField {
+
+		@Id String id;
+		ClassWithExplicitlyRenamedField nested;
+	}
+
+	static class ClassWithExplicitlyRenamedField {
+
+		@Field("id") String id;
+	}
 }

spring-data-mongodb/src/test/java/org/springframework/data/mongodb/core/mapping/BasicMongoPersistentPropertyUnitTests.java

@@ -130,10 +130,7 @@ public void rejectsInvalidValueReturnedByFieldNamingStrategy() {
 	@Test
 	public void shouldDetectAnnotatedLanguagePropertyCorrectly() {
 
-		BasicMongoPersistentEntity<DocumentWithLanguageProperty> persistentEntity = new BasicMongoPersistentEntity<DocumentWithLanguageProperty>(
-				ClassTypeInformation.from(DocumentWithLanguageProperty.class));
-
-		MongoPersistentProperty property = getPropertyFor(persistentEntity, "lang");
+		MongoPersistentProperty property = getPropertyFor(DocumentWithLanguageProperty.class, "lang");
 		assertThat(property.isLanguageProperty(), is(true));
 	}
 
@@ -143,10 +140,7 @@ public void shouldDetectAnnotatedLanguagePropertyCorrectly() {
 	@Test
 	public void shouldDetectIplicitLanguagePropertyCorrectly() {
 
-		BasicMongoPersistentEntity<DocumentWithImplicitLanguageProperty> persistentEntity = new BasicMongoPersistentEntity<DocumentWithImplicitLanguageProperty>(
-				ClassTypeInformation.from(DocumentWithImplicitLanguageProperty.class));
-
-		MongoPersistentProperty property = getPropertyFor(persistentEntity, "language");
+		MongoPersistentProperty property = getPropertyFor(DocumentWithImplicitLanguageProperty.class, "language");
 		assertThat(property.isLanguageProperty(), is(true));
 	}
 
@@ -156,10 +150,7 @@ public void shouldDetectIplicitLanguagePropertyCorrectly() {
 	@Test
 	public void shouldDetectTextScorePropertyCorrectly() {
 
-		BasicMongoPersistentEntity<DocumentWithTextScoreProperty> persistentEntity = new BasicMongoPersistentEntity<DocumentWithTextScoreProperty>(
-				ClassTypeInformation.from(DocumentWithTextScoreProperty.class));
-
-		MongoPersistentProperty property = getPropertyFor(persistentEntity, "score");
+		MongoPersistentProperty property = getPropertyFor(DocumentWithTextScoreProperty.class, "score");
 		assertThat(property.isTextScoreProperty(), is(true));
 	}
 
@@ -169,17 +160,39 @@ public void shouldDetectTextScorePropertyCorrectly() {
 	@Test
 	public void shouldDetectTextScoreAsReadOnlyProperty() {
 
-		BasicMongoPersistentEntity<DocumentWithTextScoreProperty> persistentEntity = new BasicMongoPersistentEntity<DocumentWithTextScoreProperty>(
-				ClassTypeInformation.from(DocumentWithTextScoreProperty.class));
-
-		MongoPersistentProperty property = getPropertyFor(persistentEntity, "score");
+		MongoPersistentProperty property = getPropertyFor(DocumentWithTextScoreProperty.class, "score");
 		assertThat(property.isWritable(), is(false));
 	}
 
+	/**
+	 * @see DATAMONGO-1050
+	 */
+	@Test
+	public void shouldNotConsiderExplicitlyNameFieldAsIdProperty() {
+
+		MongoPersistentProperty property = getPropertyFor(DocumentWithExplicitlyRenamedIdProperty.class, "id");
+		assertThat(property.isIdProperty(), is(false));
+	}
+
+	/**
+	 * @see DATAMONGO-1050
+	 */
+	@Test
+	public void shouldConsiderPropertyAsIdWhenExplicitlyAnnotatedWithIdEvenWhenExplicitlyNamePresent() {
+
+		MongoPersistentProperty property = getPropertyFor(DocumentWithExplicitlyRenamedIdPropertyHavingIdAnnotation.class,
+				"id");
+		assertThat(property.isIdProperty(), is(true));
+	}
+
 	private MongoPersistentProperty getPropertyFor(Field field) {
 		return getPropertyFor(entity, field);
 	}
 
+	private <T> MongoPersistentProperty getPropertyFor(Class<T> type, String fieldname) {
+		return getPropertyFor(new BasicMongoPersistentEntity<T>(ClassTypeInformation.from(type)), fieldname);
+	}
+
 	private MongoPersistentProperty getPropertyFor(MongoPersistentEntity<?> persistentEntity, String fieldname) {
 		return getPropertyFor(persistentEntity, ReflectionUtils.findField(persistentEntity.getType(), fieldname));
 	}
@@ -230,4 +243,14 @@ static class DocumentWithImplicitLanguageProperty {
 	static class DocumentWithTextScoreProperty {
 		@TextScore Float score;
 	}
+
+	static class DocumentWithExplicitlyRenamedIdProperty {
+
+		@org.springframework.data.mongodb.core.mapping.Field("id") String id;
+	}
+
+	static class DocumentWithExplicitlyRenamedIdPropertyHavingIdAnnotation {
+
+		@Id @org.springframework.data.mongodb.core.mapping.Field("id") String id;
+	}
 }