DATAMONGO-688 - Improve detection of id properties.

Added support for precedence of explicit id property mapping over implicit property mappings. Changed BasicMongoPersistentProperty.getFieldName() to return the mongo _id field name only for the "effective" id property considering the owner entity if already set). Added some test cases for all possible cases to MongoMappingContextUnitTests.

Original pull request: #48.

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

@@ -15,6 +15,7 @@
  */
 package org.springframework.data.mongodb.core.mapping;
 
+import java.lang.reflect.Field;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Map;
@@ -24,6 +25,7 @@ import org.springframework.context.ApplicationContext;
 import org.springframework.context.ApplicationContextAware;
 import org.springframework.context.expression.BeanFactoryAccessor;
 import org.springframework.context.expression.BeanFactoryResolver;
+import org.springframework.data.annotation.Id;
 import org.springframework.data.mapping.Association;
 import org.springframework.data.mapping.AssociationHandler;
 import org.springframework.data.mapping.PropertyHandler;
@@ -35,6 +37,7 @@ import org.springframework.expression.Expression;
 import org.springframework.expression.ParserContext;
 import org.springframework.expression.spel.standard.SpelExpressionParser;
 import org.springframework.expression.spel.support.StandardEvaluationContext;
+import org.springframework.util.Assert;
 import org.springframework.util.StringUtils;
 
 /**
@@ -43,6 +46,7 @@ import org.springframework.util.StringUtils;
  * 
  * @author Jon Brisbin
  * @author Oliver Gierke
+ * @author Thomas Darimont
  */
 public class BasicMongoPersistentEntity<T> extends BasicPersistentEntity<T, MongoPersistentProperty> implements
 		MongoPersistentEntity<T>, ApplicationContextAware {
@@ -136,6 +140,60 @@ public class BasicMongoPersistentEntity<T> extends BasicPersistentEntity<T, Mong
 		}
 	}
 
+	/**
+	 * As a general note: An implicit id property has a name that matches "id" or "_id". An explicit id property is one
+	 * that is annotated with @see {@link Id}. The property id is updated according to the following rules: 1) An id
+	 * property which is defined explicitly takes precedence over an implicitly defined id property. 2) In case of any
+	 * ambiguity a @see {@link MappingException} is thrown.
+	 * 
+	 * @param property - the new id property candidate
+	 * @return
+	 */
+	@Override
+	protected MongoPersistentProperty returnPropertyIfBetterIdPropertyCandidateOrNull(MongoPersistentProperty property) {
+
+		Assert.notNull(property);
+
+		if (!property.isIdProperty()) {
+			return null;
+		}
+
+		MongoPersistentProperty currentIdProperty = getIdProperty();
+
+		boolean currentIdPropertyIsSet = currentIdProperty != null;
+		@SuppressWarnings("null")
+		boolean currentIdPropertyIsExplicit = currentIdPropertyIsSet ? currentIdProperty.isExplicitIdProperty() : false;
+		boolean newIdPropertyIsExplicit = property.isExplicitIdProperty();
+
+		if (!currentIdPropertyIsSet) {
+			return property;
+
+		}
+
+		@SuppressWarnings("null")
+		Field currentIdPropertyField = currentIdProperty.getField();
+
+		if (newIdPropertyIsExplicit && currentIdPropertyIsExplicit) {
+			throw new MappingException(String.format(
+					"Attempt to add explicit id property %s but already have an property %s registered "
+							+ "as explicit id. Check your mapping configuration!", property.getField(), currentIdPropertyField));
+
+		} else if (newIdPropertyIsExplicit && !currentIdPropertyIsExplicit) {
+			// explicit id property takes precedence over implicit id property
+			return property;
+
+		} else if (!newIdPropertyIsExplicit && currentIdPropertyIsExplicit) {
+			// no id property override - current property is explicitly defined
+
+		} else {
+			throw new MappingException(String.format(
+					"Attempt to add id property %s but already have an property %s registered "
+							+ "as id. Check your mapping configuration!", property.getField(), currentIdPropertyField));
+		}
+
+		return null;
+	}
+
 	/**
 	 * Handler to collect {@link MongoPersistentProperty} instances and check that each of them is mapped to a distinct
 	 * field name.

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

@@ -24,6 +24,7 @@ import java.util.Set;
 import org.bson.types.ObjectId;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
+import org.springframework.data.annotation.Id;
 import org.springframework.data.mapping.Association;
 import org.springframework.data.mapping.model.AnnotationBasedPersistentProperty;
 import org.springframework.data.mapping.model.MappingException;
@@ -38,6 +39,7 @@ import com.mongodb.DBObject;
  * 
  * @author Oliver Gierke
  * @author Patryk Wasik
+ * @author Thomas Darimont
  */
 public class BasicMongoPersistentProperty extends AnnotationBasedPersistentProperty<MongoPersistentProperty> implements
 		MongoPersistentProperty {
@@ -109,6 +111,15 @@ public class BasicMongoPersistentProperty extends AnnotationBasedPersistentPrope
 		return SUPPORTED_ID_PROPERTY_NAMES.contains(field.getName());
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mongodb.core.mapping.MongoPersistentProperty#isExplicitIdProperty()
+	 */
+	@Override
+	public boolean isExplicitIdProperty() {
+		return isAnnotationPresent(Id.class);
+	}
+
 	/**
 	 * Returns the key to be used to store the value of the property inside a Mongo {@link DBObject}.
 	 * 
@@ -117,7 +128,18 @@ public class BasicMongoPersistentProperty extends AnnotationBasedPersistentPrope
 	public String getFieldName() {
 
 		if (isIdProperty()) {
-			return ID_FIELD_NAME;
+
+			if (owner == null) {
+				return ID_FIELD_NAME;
+			}
+
+			if (owner.getIdProperty() == null) {
+				return ID_FIELD_NAME;
+			}
+
+			if (owner.isIdProperty(this)) {
+				return ID_FIELD_NAME;
+			}
 		}
 
 		org.springframework.data.mongodb.core.mapping.Field annotation = findAnnotation(org.springframework.data.mongodb.core.mapping.Field.class);

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2011-2012 the original author or authors.
+ * Copyright 2011-2013 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,6 +16,8 @@
 package org.springframework.data.mongodb.core.mapping;
 
 import org.springframework.core.convert.converter.Converter;
+import org.springframework.data.annotation.Id;
+import org.springframework.data.mapping.PersistentEntity;
 import org.springframework.data.mapping.PersistentProperty;
 
 /**
@@ -23,6 +25,7 @@ import org.springframework.data.mapping.PersistentProperty;
  * 
  * @author Oliver Gierke
  * @author Patryk Wasik
+ * @author Thomas Darimont
  */
 public interface MongoPersistentProperty extends PersistentProperty<MongoPersistentProperty> {
 
@@ -48,6 +51,14 @@ public interface MongoPersistentProperty extends PersistentProperty<MongoPersist
 	 */
 	boolean isDbReference();
 
+	/**
+	 * Returns whether the property is explicitly marked as an identifier property of the owning {@link PersistentEntity}.
+	 * A property is an explicit id property if it is annotated with @see {@link Id}.
+	 * 
+	 * @return
+	 */
+	boolean isExplicitIdProperty();
+
 	/**
 	 * Returns the {@link DBRef} if the property is a reference.
 	 * 

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

@@ -39,15 +39,14 @@ import com.mongodb.DBRef;
  * Unit tests for {@link MongoMappingContext}.
  * 
  * @author Oliver Gierke
+ * @author Thomas Darimont
  */
 @RunWith(MockitoJUnitRunner.class)
 public class MongoMappingContextUnitTests {
 
-	@Mock
-	ApplicationContext applicationContext;
+	@Mock ApplicationContext applicationContext;
 
-	@Rule
-	public ExpectedException exception = ExpectedException.none();
+	@Rule public ExpectedException exception = ExpectedException.none();
 
 	@Test
 	public void addsSelfReferencingPersistentEntityCorrectly() throws Exception {
@@ -58,13 +57,6 @@ public class MongoMappingContextUnitTests {
 		context.initialize();
 	}
 
-	@Test(expected = MappingException.class)
-	public void rejectsEntityWithMultipleIdProperties() {
-
-		MongoMappingContext context = new MongoMappingContext();
-		context.getPersistentEntity(ClassWithMultipleIdProperties.class);
-	}
-
 	@Test
 	public void doesNotReturnPersistentEntityForMongoSimpleType() {
 
@@ -128,12 +120,61 @@ public class MongoMappingContextUnitTests {
 		context.getPersistentEntity(Child.class);
 	}
 
-	class ClassWithMultipleIdProperties {
+	/**
+	 * @see DATAMONGO-688
+	 */
+	@Test
+	public void mappingContextShouldAcceptClassWithImplicitIdProperty() {
 
-		@Id
-		String myId;
+		MongoMappingContext context = new MongoMappingContext();
+		BasicMongoPersistentEntity<?> pe = context.getPersistentEntity(ClassWithImplicitId.class);
 
-		String id;
+		assertThat(pe, is(not(nullValue())));
+		assertThat(pe.isIdProperty(pe.getPersistentProperty("id")), is(true));
+	}
+
+	/**
+	 * @see DATAMONGO-688
+	 */
+	@Test
+	public void mappingContextShouldAcceptClassWithExplicitIdProperty() {
+
+		MongoMappingContext context = new MongoMappingContext();
+		BasicMongoPersistentEntity<?> pe = context.getPersistentEntity(ClassWithExplicitId.class);
+
+		assertThat(pe, is(not(nullValue())));
+		assertThat(pe.isIdProperty(pe.getPersistentProperty("myId")), is(true));
+	}
+
+	/**
+	 * @see DATAMONGO-688
+	 */
+	@Test
+	public void mappingContextShouldAcceptClassWithExplicitAndImplicitIdPropertyByGivingPrecedenceToExplicitIdProperty() {
+
+		MongoMappingContext context = new MongoMappingContext();
+		BasicMongoPersistentEntity<?> pe = context.getPersistentEntity(ClassWithExplicitIdAndImplicitId.class);
+		assertThat(pe, is(not(nullValue())));
+	}
+
+	/**
+	 * @see DATAMONGO-688
+	 */
+	@Test(expected = MappingException.class)
+	public void rejectsClassWithAmbiguousExplicitIdPropertyFieldMappings() {
+
+		MongoMappingContext context = new MongoMappingContext();
+		context.getPersistentEntity(ClassWithMultipleExplicitIds.class);
+	}
+
+	/**
+	 * @see DATAMONGO-688
+	 */
+	@Test(expected = MappingException.class)
+	public void rejectsClassWithAmbiguousImplicitIdPropertyFieldMappings() {
+
+		MongoMappingContext context = new MongoMappingContext();
+		context.getPersistentEntity(ClassWithMultipleImplicitIds.class);
 	}
 
 	public class SampleClass {
@@ -148,8 +189,7 @@ public class MongoMappingContextUnitTests {
 
 	class InvalidPerson {
 
-		@org.springframework.data.mongodb.core.mapping.Field("foo")
-		String firstname, lastname;
+		@org.springframework.data.mongodb.core.mapping.Field("foo") String firstname, lastname;
 	}
 
 	class Parent {
@@ -168,4 +208,34 @@ public class MongoMappingContextUnitTests {
 			return super.getName();
 		}
 	}
+
+	class ClassWithImplicitId {
+
+		String field;
+		String id;
+	}
+
+	class ClassWithExplicitId {
+
+		@Id String myId;
+		String field;
+	}
+
+	class ClassWithExplicitIdAndImplicitId {
+
+		@Id String myId;
+		String id;
+	}
+
+	class ClassWithMultipleExplicitIds {
+
+		@Id String myId;
+		@Id String id;
+	}
+
+	class ClassWithMultipleImplicitIds {
+
+		String _id;
+		String id;
+	}
 }