HHH-4529 clean @Id/@EmbeddedId mapping and merge with the regular basic and component mapping to open possibilities for @Id @ManyToOne support

git-svn-id: https://svn.jboss.org/repos/hibernate/core/trunk@18600 1b8cb986-b30d-0410-93ca-fae66ebed9b2

annotations/src/main/java/org/hibernate/cfg/AnnotationBinder.java

@@ -1253,27 +1253,11 @@ public final class AnnotationBinder {
 		}
 
 		final XClass returnedClass = inferredData.getClassOrElement();
-		boolean isId;
-		if ( !entityBinder.isIgnoreIdAnnotations() &&
+
+		boolean isId = !entityBinder.isIgnoreIdAnnotations() &&
 				( property.isAnnotationPresent( Id.class )
-						|| property.isAnnotationPresent( EmbeddedId.class ) ) ) {
-			isId = true;
-			//Override from @MapsId if needed
-			columns = overrideColumnFromMapsIdProperty( "", columns, propertyHolder, entityBinder, mappings );
-			processId(
-					propertyHolder,
-					property,
-					inferredData,
-					classGenerators,
-					entityBinder,
-					isIdentifierMapper,
-					mappings,
-					inheritanceStatePerClass,
-					columns,
-					returnedClass
-			);
-		}
-		else if ( property.isAnnotationPresent( Version.class ) ) {
+						|| property.isAnnotationPresent( EmbeddedId.class ) );
+		if ( property.isAnnotationPresent( Version.class ) ) {
 			if ( isIdentifierMapper ) {
 				throw new AnnotationException(
 						"@IdClass class should not have @Version property"
@@ -1304,7 +1288,7 @@ public final class AnnotationBinder {
 			propBinder.setReturnedClass( inferredData.getPropertyClass() );
 			propBinder.setMappings( mappings );
 			propBinder.setDeclaringClass( inferredData.getDeclaringClass() );
-			Property prop = propBinder.bind();
+			Property prop = propBinder.makePropertyValueAndBind();
 			propBinder.getSimpleValueBinder().setVersion(true);
 			rootClass.setVersion( prop );
 
@@ -1695,19 +1679,26 @@ public final class AnnotationBinder {
 			collectionBinder.bind();
 
 		}
-		else {
+		//Either a regular property or a basic @Id or @EmbeddedId while not ignoring id annotations
+		else if ( !isId || !entityBinder.isIgnoreIdAnnotations() ) {
 			//define whether the type is a component or not
 			boolean isComponent;
-			Embeddable embeddableAnn = returnedClass.getAnnotation( Embeddable.class );
-			Embedded embeddedAnn = property.getAnnotation( Embedded.class );
-			isComponent = embeddedAnn != null || embeddableAnn != null;
-
+			isComponent = property.isAnnotationPresent( Embedded.class )
+					|| property.isAnnotationPresent( EmbeddedId.class )
+					|| returnedClass.isAnnotationPresent( Embeddable.class );
+			PropertyBinder propertyBinder;
 			if ( isComponent ) {
 				AccessType propertyAccessor = entityBinder.getPropertyAccessor( property );
-				bindComponent(
-						inferredData, propertyHolder, propertyAccessor, entityBinder,
+				propertyBinder = bindComponent(
+						inferredData,
+						propertyHolder,
+						propertyAccessor,
+						entityBinder,
 						isIdentifierMapper,
-						mappings, isComponentEmbedded, inheritanceStatePerClass
+						mappings,
+						isComponentEmbedded,
+						isId,
+						inheritanceStatePerClass
 				);
 			}
 			else {
@@ -1720,35 +1711,53 @@ public final class AnnotationBinder {
 					lazy = ann.fetch() == FetchType.LAZY;
 				}
 				//implicit type will check basic types and Serializable classes
-				if ( !optional && nullability != Nullability.FORCED_NULL ) {
+				if ( isId || ( !optional && nullability != Nullability.FORCED_NULL ) ) {
 					//force columns to not null
 					for (Ejb3Column col : columns) {
 						col.forceNotNull();
 					}
 				}
 
-
 				//Override from @MapsId if needed
-				if ( propertyHolder.isOrWithinEmbeddedId() ) {
-					columns = overrideColumnFromMapsIdProperty( property.getName(), columns, propertyHolder, entityBinder, mappings );
+				if ( isId || propertyHolder.isOrWithinEmbeddedId() ) {
+					columns = overrideColumnFromMapsIdProperty(
+							isId ? "" : property.getName(), //@MapsId("") points to the id property
+							columns,
+							propertyHolder,
+							entityBinder,
+							mappings );
 				}
 
-				PropertyBinder propBinder = new PropertyBinder();
-				propBinder.setName( inferredData.getPropertyName() );
-				propBinder.setReturnedClassName( inferredData.getTypeName() );
-				propBinder.setLazy( lazy );
-				propBinder.setAccessType( inferredData.getDefaultAccess() );
-				propBinder.setColumns( columns );
-				propBinder.setHolder( propertyHolder );
-				propBinder.setProperty( property );
-				propBinder.setReturnedClass( inferredData.getPropertyClass() );
-				propBinder.setMappings( mappings );
+				propertyBinder = new PropertyBinder();
+				propertyBinder.setName( inferredData.getPropertyName() );
+				propertyBinder.setReturnedClassName( inferredData.getTypeName() );
+				propertyBinder.setLazy( lazy );
+				propertyBinder.setAccessType( inferredData.getDefaultAccess() );
+				propertyBinder.setColumns( columns );
+				propertyBinder.setHolder( propertyHolder );
+				propertyBinder.setProperty( property );
+				propertyBinder.setReturnedClass( inferredData.getPropertyClass() );
+				propertyBinder.setMappings( mappings );
 				if ( isIdentifierMapper ) {
-					propBinder.setInsertable( false );
-					propBinder.setUpdatable( false );
+					propertyBinder.setInsertable( false );
+					propertyBinder.setUpdatable( false );
 				}
-				propBinder.setDeclaringClass( inferredData.getDeclaringClass() );
-				propBinder.bind();
+				propertyBinder.setDeclaringClass( inferredData.getDeclaringClass() );
+				propertyBinder.setId(isId);
+				propertyBinder.setInheritanceStatePerClass(inheritanceStatePerClass);
+				propertyBinder.makePropertyValueAndBind();
+			}
+			if (isId) {
+				//components and regular basic types create SimpleValue objects
+				final SimpleValue value = ( SimpleValue ) propertyBinder.getValue();
+				processId(
+						propertyHolder,
+						inferredData,
+						value,
+						classGenerators,
+						isIdentifierMapper,
+						mappings
+				);
 			}
 		}
 		//init index
@@ -1785,14 +1794,15 @@ public final class AnnotationBinder {
 		}
 	}
 
-	private static void processId(PropertyHolder propertyHolder, XProperty property, PropertyData inferredData, HashMap<String, IdGenerator> classGenerators, EntityBinder entityBinder, boolean isIdentifierMapper, ExtendedMappings mappings, Map<XClass, InheritanceState> inheritanceStatePerClass, Ejb3Column[] columns, XClass returnedClass) {
+	private static void processId(PropertyHolder propertyHolder, PropertyData inferredData, SimpleValue idValue, HashMap<String, IdGenerator> classGenerators, boolean isIdentifierMapper, ExtendedMappings mappings) {
 		if ( isIdentifierMapper ) {
 			throw new AnnotationException(
 					"@IdClass class should not have @Id nor @EmbeddedId properties: "
 							+ BinderHelper.getPath( propertyHolder, inferredData )
 			);
 		}
-		log.trace( "{} is an id", inferredData.getPropertyName() );
+		XClass returnedClass = inferredData.getClassOrElement();
+		XProperty property = inferredData.getProperty();
 		//clone classGenerator and override with local values
 		HashMap<String, IdGenerator> localGenerators = (HashMap<String, IdGenerator>) classGenerators.clone();
 		localGenerators.putAll( buildLocalGenerators( property, mappings ) );
@@ -1801,31 +1811,16 @@ public final class AnnotationBinder {
 		//guess if its a component and find id data access (property, field etc)
 		final boolean isComponent = returnedClass.isAnnotationPresent( Embeddable.class )
 				|| property.isAnnotationPresent( EmbeddedId.class );
-		AccessType propertyAccessor = entityBinder.getPropertyAccessor( returnedClass );
 
 		GeneratedValue generatedValue = property.getAnnotation( GeneratedValue.class );
 		String generatorType = generatedValue != null ?
 				generatorType( generatedValue.strategy() ) :
 				"assigned";
-		String generator = generatedValue != null ?
+		String generatorName = generatedValue != null ?
 				generatedValue.generator() :
 				BinderHelper.ANNOTATION_STRING_DEFAULT;
 		if ( isComponent ) generatorType = "assigned"; //a component must not have any generator
-
-		bindId(
-				generatorType,
-				generator,
-				inferredData,
-				columns,
-				propertyHolder,
-				localGenerators,
-				isComponent,
-				propertyAccessor, entityBinder,
-				false,
-				isIdentifierMapper,
-				mappings,
-				inheritanceStatePerClass
-		);
+		BinderHelper.makeIdGenerator( idValue, generatorType, generatorName, mappings, localGenerators );
 
 		log.trace(
 				"Bind {} on {}", ( isComponent ? "@EmbeddedId" : "@Id" ), inferredData.getPropertyName()
@@ -1963,29 +1958,50 @@ public final class AnnotationBinder {
 		collectionBinder.setInverseJoinColumns( inverseJoinColumns );
 	}
 
-	private static void bindComponent(
+	private static PropertyBinder bindComponent(
 			PropertyData inferredData,
 			PropertyHolder propertyHolder,
-			AccessType propertyAccessor, EntityBinder entityBinder,
+			AccessType propertyAccessor,
+			EntityBinder entityBinder,
 			boolean isIdentifierMapper,
-			ExtendedMappings mappings, boolean isComponentEmbedded,
+			ExtendedMappings mappings,
+			boolean isComponentEmbedded,
+			boolean isId,
 			Map<XClass, InheritanceState> inheritanceStatePerClass
 	) {
 		Component comp = fillComponent(
-				propertyHolder, inferredData, propertyAccessor, true, entityBinder,
+				propertyHolder, inferredData, propertyAccessor, !isId, entityBinder,
 				isComponentEmbedded, isIdentifierMapper,
 				false, mappings, inheritanceStatePerClass
 		);
+		if (isId) {
+			comp.setKey( true );
+			if ( propertyHolder.getPersistentClass().getIdentifier() != null ) {
+				throw new AnnotationException(
+						comp.getComponentClassName()
+						+ " must not have @Id properties when used as an @EmbeddedId: "
+						+ BinderHelper.getPath( propertyHolder, inferredData ) );
+			}
+			if ( comp.getPropertySpan() == 0 ) {
+				throw new AnnotationException( comp.getComponentClassName()
+						+ " has no persistent id property"
+						+ BinderHelper.getPath( propertyHolder, inferredData ) );
+			}
+		}
 		XProperty property = inferredData.getProperty();
 		setupComponentTuplizer( property, comp );
-
 		PropertyBinder binder = new PropertyBinder();
 		binder.setName( inferredData.getPropertyName() );
 		binder.setValue( comp );
 		binder.setProperty( inferredData.getProperty() );
 		binder.setAccessType( inferredData.getDefaultAccess() );
-		Property prop = binder.make();
-		propertyHolder.addProperty( prop, inferredData.getDeclaringClass() );
+		binder.setEmbedded( isComponentEmbedded );
+		binder.setHolder( propertyHolder );
+		binder.setId( isId );
+		binder.setInheritanceStatePerClass( inheritanceStatePerClass );
+		binder.setMappings( mappings );
+		binder.makePropertyAndBind();
+		return binder;
 	}
 
 	public static Component fillComponent(
@@ -2159,7 +2175,7 @@ public final class AnnotationBinder {
 			binder.setValue( id );
 			binder.setAccessType( inferredData.getDefaultAccess() );
 			binder.setProperty( inferredData.getProperty() );
-			Property prop = binder.make();
+			Property prop = binder.makeProperty();
 			rootClass.setIdentifierProperty( prop );
 			//if the id property is on a superclass, update the metamodel
 			final org.hibernate.mapping.MappedSuperclass superclass = BinderHelper.getMappedSuperclassOrNull(
@@ -2291,7 +2307,7 @@ public final class AnnotationBinder {
 		binder.setAccessType( inferredData.getDefaultAccess() );
 		binder.setCascade( cascadeStrategy );
 		binder.setProperty( property );
-		Property prop = binder.make();
+		Property prop = binder.makeProperty();
 		//composite FK columns are in the same table so its OK
 		propertyHolder.addProperty( prop, columns, inferredData.getDeclaringClass() );
 	}
@@ -2446,7 +2462,7 @@ public final class AnnotationBinder {
 		}
 		binder.setAccessType( inferredData.getDefaultAccess() );
 		binder.setCascade( cascadeStrategy );
-		Property prop = binder.make();
+		Property prop = binder.makeProperty();
 		//composite FK columns are in the same table so its OK
 		propertyHolder.addProperty( prop, columns, inferredData.getDeclaringClass() );
 	}

annotations/src/main/java/org/hibernate/cfg/ClassPropertyHolder.java

@@ -72,7 +72,7 @@ public class ClassPropertyHolder extends AbstractPropertyHolder {
 
 	public void addProperty(Property prop, Ejb3Column[] columns, XClass declaringClass) {
 		//Ejb3Column.checkPropertyConsistency( ); //already called earlier
-		if ( columns[0].isSecondary() ) {
+		if ( columns != null && columns[0].isSecondary() ) {
 			//TODO move the getJoin() code here?
 			final Join join = columns[0].getJoin();
 			addPropertyToJoin( prop, declaringClass, join );

annotations/src/main/java/org/hibernate/cfg/ComponentPropertyHolder.java

@@ -61,16 +61,18 @@ public class ComponentPropertyHolder extends AbstractPropertyHolder {
 		 * if not, change the component table if no properties are set
 		 * if a property is set already the core cannot support that
 		 */
-		Table table = columns[0].getTable();
-		if ( !table.equals( component.getTable() ) ) {
-			if ( component.getPropertySpan() == 0 ) {
-				component.setTable( table );
-			}
-			else {
-				throw new AnnotationException(
-						"A component cannot hold properties split into 2 different tables: "
-								+ this.getPath()
-				);
+		if (columns != null) {
+			Table table = columns[0].getTable();
+			if ( !table.equals( component.getTable() ) ) {
+				if ( component.getPropertySpan() == 0 ) {
+					component.setTable( table );
+				}
+				else {
+					throw new AnnotationException(
+							"A component cannot hold properties split into 2 different tables: "
+									+ this.getPath()
+					);
+				}
 			}
 		}
 		addProperty( prop, declaringClass );

annotations/src/main/java/org/hibernate/cfg/OneToOneSecondPass.java

@@ -106,7 +106,7 @@ public class OneToOneSecondPass implements SecondPass {
 		binder.setValue( value );
 		binder.setCascade( cascadeStrategy );
 		binder.setAccessType( inferredData.getDefaultAccess() );
-		Property prop = binder.make();
+		Property prop = binder.makeProperty();
 		if ( BinderHelper.isDefault( mappedBy ) ) {
 			/*
 			 * we need to check if the columns are in the right order

annotations/src/main/java/org/hibernate/cfg/annotations/CollectionBinder.java

@@ -522,7 +522,7 @@ public abstract class CollectionBinder {
 		binder.setProperty( property );
 		binder.setInsertable( insertable );
 		binder.setUpdatable( updatable );
-		Property prop = binder.make();
+		Property prop = binder.makeProperty();
 		//we don't care about the join stuffs because the column is on the association table.
 		if (! declaringClassSet) throw new AssertionFailure( "DeclaringClass is not set in CollectionBinder while binding" );
 		propertyHolder.addProperty( prop, declaringClass );

annotations/src/main/java/org/hibernate/cfg/annotations/PropertyBinder.java

@@ -23,29 +23,35 @@
  */
 package org.hibernate.cfg.annotations;
 
+import java.util.Map;
 import javax.persistence.EmbeddedId;
 import javax.persistence.Id;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import org.hibernate.AnnotationException;
 import org.hibernate.annotations.Generated;
 import org.hibernate.annotations.GenerationTime;
 import org.hibernate.annotations.Immutable;
 import org.hibernate.annotations.NaturalId;
 import org.hibernate.annotations.OptimisticLock;
+import org.hibernate.annotations.common.AssertionFailure;
 import org.hibernate.annotations.common.reflection.XClass;
 import org.hibernate.annotations.common.reflection.XProperty;
-import org.hibernate.annotations.common.AssertionFailure;
 import org.hibernate.cfg.AccessType;
+import org.hibernate.cfg.BinderHelper;
 import org.hibernate.cfg.Ejb3Column;
 import org.hibernate.cfg.ExtendedMappings;
+import org.hibernate.cfg.InheritanceState;
 import org.hibernate.cfg.PropertyHolder;
+import org.hibernate.mapping.KeyValue;
 import org.hibernate.mapping.Property;
 import org.hibernate.mapping.PropertyGeneration;
+import org.hibernate.mapping.RootClass;
 import org.hibernate.mapping.SimpleValue;
 import org.hibernate.mapping.Value;
 import org.hibernate.util.StringHelper;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
 
 /**
  * @author Emmanuel Bernard
@@ -66,13 +72,21 @@ public class PropertyBinder {
 	private SimpleValueBinder simpleValueBinder;
 	private XClass declaringClass;
 	private boolean declaringClassSet;
+	private boolean embedded;
+
+	public void setEmbedded(boolean embedded) {
+		this.embedded = embedded;
+	}
 
 	/*
-	 * property can be null
-	 * prefer propertyName to property.getName() since some are overloaded
-	 */
+		 * property can be null
+		 * prefer propertyName to property.getName() since some are overloaded
+		 */
 	private XProperty property;
 	private XClass returnedClass;
+	private boolean isId;
+	private Map<XClass, InheritanceState> inheritanceStatePerClass;
+	private Property mappingProperty;
 
 	public void setInsertable(boolean insertable) {
 		this.insertable = insertable;
@@ -82,7 +96,6 @@ public class PropertyBinder {
 		this.updatable = updatable;
 	}
 
-
 	public void setName(String name) {
 		this.name = name;
 	}
@@ -128,12 +141,14 @@ public class PropertyBinder {
 	}
 
 	private void validateBind() {
-		if (property.isAnnotationPresent(Immutable.class)) {
-			throw new AnnotationException("@Immutable on property not allowed. " +
-					"Only allowed on entity level or on a collection.");
+		if ( property.isAnnotationPresent( Immutable.class ) ) {
+			throw new AnnotationException(
+					"@Immutable on property not allowed. " +
+							"Only allowed on entity level or on a collection."
+			);
 		}
 		if ( !declaringClassSet ) {
-			throw new AssertionFailure( "declaringClass has not been set before a bind");
+			throw new AssertionFailure( "declaringClass has not been set before a bind" );
 		}
 	}
 
@@ -141,7 +156,7 @@ public class PropertyBinder {
 		//TODO check necessary params for a make
 	}
 
-	public Property bind() {
+	private Property makePropertyAndValue() {
 		validateBind();
 		log.debug( "binding property {} with lazy={}", name, lazy );
 		String containerClassName = holder == null ?
@@ -157,12 +172,51 @@ public class PropertyBinder {
 		simpleValueBinder.setMappings( mappings );
 		SimpleValue propertyValue = simpleValueBinder.make();
 		setValue( propertyValue );
-		Property prop = make();
-		holder.addProperty( prop, columns, declaringClass );
+		return makeProperty();
+	}
+
+	//used when value is provided
+	public Property makePropertyAndBind() {
+		return bind( makeProperty() );
+	}
+
+	//used to build everything from scratch
+	public Property makePropertyValueAndBind() {
+		return bind( makePropertyAndValue() );
+	}
+
+	private Property bind(Property prop) {
+		if (isId) {
+			final RootClass rootClass = ( RootClass ) holder.getPersistentClass();
+			rootClass.setIdentifier( ( KeyValue ) getValue() );
+
+			if (embedded) {
+				rootClass.setEmbeddedIdentifier( true );
+			}
+			else {
+				rootClass.setIdentifierProperty( prop );
+				final org.hibernate.mapping.MappedSuperclass superclass = BinderHelper.getMappedSuperclassOrNull(
+						declaringClass,
+						inheritanceStatePerClass,
+						mappings
+				);
+				if (superclass != null) {
+					superclass.setDeclaredIdentifierProperty(prop);
+				}
+				else {
+					//we know the property is on the actual entity
+					rootClass.setDeclaredIdentifierProperty( prop );
+				}
+			}
+		}
+		else {
+			holder.addProperty( prop, columns, declaringClass );
+		}
 		return prop;
 	}
 
-	public Property make() {
+	//used when the value is provided and the binding is done elsewhere
+	public Property makeProperty() {
 		validateMake();
 		log.debug( "Building property " + name );
 		Property prop = new Property();
@@ -182,8 +236,10 @@ public class PropertyBinder {
 			if ( !GenerationTime.NEVER.equals( generated ) ) {
 				if ( property.isAnnotationPresent( javax.persistence.Version.class )
 						&& GenerationTime.INSERT.equals( generated ) ) {
-					throw new AnnotationException( "@Generated(INSERT) on a @Version property not allowed, use ALWAYS: "
-							+ StringHelper.qualify( holder.getPath(), name ) );
+					throw new AnnotationException(
+							"@Generated(INSERT) on a @Version property not allowed, use ALWAYS: "
+									+ StringHelper.qualify( holder.getPath(), name )
+					);
 				}
 				insertable = false;
 				if ( GenerationTime.ALWAYS.equals( generated ) ) {
@@ -213,11 +269,14 @@ public class PropertyBinder {
 					property.isAnnotationPresent( javax.persistence.Version.class )
 							|| property.isAnnotationPresent( Id.class )
 							|| property.isAnnotationPresent( EmbeddedId.class ) ) ) {
-				throw new AnnotationException( "@OptimisticLock.exclude=true incompatible with @Id, @EmbeddedId and @Version: "
-						+ StringHelper.qualify( holder.getPath(), name ) );
+				throw new AnnotationException(
+						"@OptimisticLock.exclude=true incompatible with @Id, @EmbeddedId and @Version: "
+								+ StringHelper.qualify( holder.getPath(), name )
+				);
 			}
 		}
 		log.trace( "Cascading " + name + " with " + cascade );
+		this.mappingProperty = prop;
 		return prop;
 	}
 
@@ -233,4 +292,15 @@ public class PropertyBinder {
 		return simpleValueBinder;
 	}
 
+	public Value getValue() {
+		return value;
+	}
+
+	public void setId(boolean id) {
+		this.isId = id;
+	}
+
+	public void setInheritanceStatePerClass(Map<XClass, InheritanceState> inheritanceStatePerClass) {
+		this.inheritanceStatePerClass = inheritanceStatePerClass;
+	}
 }

annotations/src/test/java/org/hibernate/test/annotations/derivedidentities/e4/b/DerivedIdentitySimpleParentSimpleDepMapsIdTest.java

@@ -9,7 +9,7 @@ import org.hibernate.test.util.SchemaUtil;
 /**
  * @author Emmanuel Bernard
  */
-public class DerivedIdentitySimpleParentSimpleDepTest extends TestCase {
+public class DerivedIdentitySimpleParentSimpleDepMapsIdTest extends TestCase {
 
 	public void testOneToOneExplicitJoinColumn() throws Exception {
 		assertTrue( SchemaUtil.isColumnPresent( "MedicalHistory", "FK", getCfg() ) );