DATACMNS-186 - Improved the way id properties are detected.

The id properties are now discovered by the BasicPersistentEntity during the call to addPersistentProperty(…). Beyond that the method rejects the id property if one was already set.

spring-data-commons-core/src/main/java/org/springframework/data/mapping/PersistentEntity.java

@@ -1,11 +1,11 @@
 /*
- * Copyright (c) 2011-2012 by the original author(s).
+ * Copyright 2011-2012 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.
  * You may obtain a copy of the License at
  *
- *     http://www.apache.org/licenses/LICENSE-2.0
+ *      http://www.apache.org/licenses/LICENSE-2.0
  *
  * Unless required by applicable law or agreed to in writing, software
  * distributed under the License is distributed on an "AS IS" BASIS,
@@ -15,10 +15,11 @@
  */
 package org.springframework.data.mapping;
 
+import org.springframework.data.convert.EntityInstantiator;
 import org.springframework.data.util.TypeInformation;
 
 /**
- * Represents a persistent entity
+ * Represents a persistent entity.
  * 
  * @author Oliver Gierke
  * @author Graeme Rocher
@@ -27,7 +28,7 @@ import org.springframework.data.util.TypeInformation;
 public interface PersistentEntity<T, P extends PersistentProperty<P>> {
 
 	/**
-	 * The entity name including any package prefix
+	 * The entity name including any package prefix.
 	 * 
 	 * @return must never return {@literal null}
 	 */
@@ -36,7 +37,9 @@ public interface PersistentEntity<T, P extends PersistentProperty<P>> {
 	/**
 	 * Returns the {@link PreferredConstructor} to be used to instantiate objects of this {@link PersistentEntity}.
 	 * 
-	 * @return {@literal null} in case no suitable constructor for automatic construction can be found.
+	 * @return {@literal null} in case no suitable constructor for automatic construction can be found. This usually
+	 *         indicates that the instantiation of the object of tthat persistent entity is done through either a customer
+	 *         {@link EntityInstantiator} or handled by custom conversion mechanisms entirely.
 	 */
 	PreferredConstructor<T, P> getPersistenceConstructor();
 
@@ -51,18 +54,26 @@ public interface PersistentEntity<T, P extends PersistentProperty<P>> {
 	boolean isConstructorArgument(P property);
 
 	/**
-	 * Returns the id property of the {@link PersistentEntity}. Must never return {@literal null} as a
-	 * {@link PersistentEntity} instance must not be created if there is no id property.
+	 * Returns whether the given {@link PersistentProperty} is the id property of the entity.
+	 * 
+	 * @param property
+	 * @return
+	 */
+	boolean isIdProperty(P property);
+
+	/**
+	 * Returns the id property of the {@link PersistentEntity}. Can be {@literal null} in case this is an entity
+	 * completely handled by a custom conversion.
 	 * 
 	 * @return the id property of the {@link PersistentEntity}.
 	 */
 	P getIdProperty();
 
 	/**
-	 * Obtains a PersistentProperty instance by name.
+	 * Obtains a {@link PersistentProperty} instance by name.
 	 * 
 	 * @param name The name of the property
-	 * @return The {@link PersistentProperty} or {@literal null} if it doesn't exist
+	 * @return the {@link PersistentProperty} or {@literal null} if it doesn't exist.
 	 */
 	P getPersistentProperty(String name);
 

spring-data-commons-core/src/main/java/org/springframework/data/mapping/PersistentProperty.java

@@ -63,7 +63,10 @@ public interface PersistentProperty<P extends PersistentProperty<P>> {
 	boolean isEntity();
 
 	/**
-	 * Returns whether the property is the ID property of the owning {@link PersistentEntity}.
+	 * Returns whether the property is a <em>potential</em> identifier property of the owning {@link PersistentEntity}.
+	 * This method is mainly used by {@link PersistentEntity} implementation to discover id property candidates on
+	 * {@link PersistentEntity} creation you should rather call {@link PersistentEntity#isIdProperty(PersistentProperty)}
+	 * to determine whether the current property is the id property of that {@link PersistentEntity} under consideration.
 	 * 
 	 * @return
 	 */
@@ -128,5 +131,4 @@ public interface PersistentProperty<P extends PersistentProperty<P>> {
 	 * @return the map's value type or {@literal null} if no {@link java.util.Map}
 	 */
 	Class<?> getMapValueType();
-
 }

spring-data-commons-core/src/main/java/org/springframework/data/mapping/context/AbstractMappingContext.java

@@ -415,10 +415,6 @@ public abstract class AbstractMappingContext<E extends MutablePersistentEntity<?
 				entity.addAssociation(property.getAssociation());
 			}
 
-			if (property.isIdProperty()) {
-				entity.setIdProperty(property);
-			}
-
 			if (entity.getType().equals(property.getRawType())) {
 				return;
 			}

spring-data-commons-core/src/main/java/org/springframework/data/mapping/model/BasicPersistentEntity.java

@@ -35,8 +35,8 @@ import org.springframework.util.StringUtils;
 /**
  * Simple value object to capture information of {@link PersistentEntity}s.
  * 
- * @author Jon Brisbin <jbrisbin@vmware.com>
  * @author Oliver Gierke
+ * @author Jon Brisbin
  */
 public class BasicPersistentEntity<T, P extends PersistentProperty<P>> implements MutablePersistentEntity<T, P> {
 
@@ -91,6 +91,14 @@ public class BasicPersistentEntity<T, P extends PersistentProperty<P>> implement
 		return constructor == null ? false : constructor.isConstructorParameter(property);
 	}
 
+	/*
+	 * (non-Javadoc)
+	 * @see org.springframework.data.mapping.PersistentEntity#isIdProperty(org.springframework.data.mapping.PersistentProperty)
+	 */
+	public boolean isIdProperty(P property) {
+		return this.idProperty == null ? false : this.idProperty.equals(property);
+	}
+
 	/*
 	 * (non-Javadoc)
 	 * @see org.springframework.data.mapping.PersistentEntity#getName()
@@ -107,21 +115,25 @@ public class BasicPersistentEntity<T, P extends PersistentProperty<P>> implement
 		return idProperty;
 	}
 
-	/* 
-	 * (non-Javadoc)
-	 * @see org.springframework.data.mapping.MutablePersistentEntity#setIdProperty(P)
-	 */
-	public void setIdProperty(P property) {
-		idProperty = property;
-	}
-
 	/* 
 	 * (non-Javadoc)
 	 * @see org.springframework.data.mapping.MutablePersistentEntity#addPersistentProperty(P)
 	 */
 	public void addPersistentProperty(P property) {
+
 		Assert.notNull(property);
 		properties.add(property);
+
+		if (property.isIdProperty()) {
+
+			if (this.idProperty != null) {
+				throw new MappingException(String.format(
+						"Attempt to add id property %s but already have property %s registered "
+								+ "as id. Check your mapping configuration!", property.getField(), idProperty.getField()));
+			}
+
+			this.idProperty = property;
+		}
 	}
 
 	/* (non-Javadoc)

spring-data-commons-core/src/main/java/org/springframework/data/mapping/model/MutablePersistentEntity.java

@@ -26,13 +26,6 @@ import org.springframework.data.mapping.PersistentProperty;
  */
 public interface MutablePersistentEntity<T, P extends PersistentProperty<P>> extends PersistentEntity<T, P> {
 
-	/**
-	 * Sets the id property for the entity.
-	 * 
-	 * @param property
-	 */
-	void setIdProperty(P property);
-
 	/**
 	 * Adds a {@link PersistentProperty} to the entity.
 	 * 

spring-data-commons-core/src/test/java/org/springframework/data/mapping/model/BasicPersistentEntityUnitTests.java

@@ -9,7 +9,10 @@ import java.util.Iterator;
 import java.util.SortedSet;
 
 import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
 import org.mockito.Mockito;
+import org.mockito.runners.MockitoJUnitRunner;
 import org.springframework.data.annotation.TypeAlias;
 import org.springframework.data.mapping.PersistentEntity;
 import org.springframework.data.mapping.PersistentEntitySpec;
@@ -23,8 +26,12 @@ import org.springframework.test.util.ReflectionTestUtils;
  * 
  * @author Oliver Gierke
  */
+@RunWith(MockitoJUnitRunner.class)
 public class BasicPersistentEntityUnitTests<T extends PersistentProperty<T>> {
 
+	@Mock
+	T property;
+
 	@Test
 	public void assertInvariants() {
 		PersistentEntitySpec.assertInvariants(createEntity(null));
@@ -90,6 +97,40 @@ public class BasicPersistentEntityUnitTests<T extends PersistentProperty<T>> {
 		assertThat(iterator.next(), is(entity.getPersistentProperty("ssn")));
 	}
 
+	/**
+	 * @see DATACMNS-186
+	 */
+	@Test
+	public void addingAndIdPropertySetsIdPropertyInternally() {
+
+		MutablePersistentEntity<Person, T> entity = createEntity(null);
+		assertThat(entity.getIdProperty(), is(nullValue()));
+
+		when(property.isIdProperty()).thenReturn(true);
+		entity.addPersistentProperty(property);
+		assertThat(entity.getIdProperty(), is(property));
+	}
+
+	/**
+	 * @see DATACMNS-186
+	 */
+	@Test
+	public void rejectsIdPropertyIfAlreadySet() {
+
+		MutablePersistentEntity<Person, T> entity = createEntity(null);
+
+		when(property.isIdProperty()).thenReturn(true);
+
+		entity.addPersistentProperty(property);
+
+		try {
+			entity.addPersistentProperty(property);
+			fail("Expected MappingException!");
+		} catch (MappingException e) {
+			// expected
+		}
+	}
+
 	private BasicPersistentEntity<Person, T> createEntity(Comparator<T> comparator) {
 		return new BasicPersistentEntity<Person, T>(ClassTypeInformation.from(Person.class), comparator);
 	}