HHH-6204 - JoinColumn on non key field fails to populate collection

hibernate-core/src/main/java/org/hibernate/cfg/BinderHelper.java

@@ -115,6 +115,136 @@ public static Property shallowCopy(Property property) {
 		return clone;
 	}
 
+// This is sooooooooo close in terms of not generating a synthetic property if we do not have to (where property ref
+// refers to a single property).  The sticking point is cases where the `referencedPropertyName` come from subclasses
+// or secondary tables.  Part of the problem is in PersistentClass itself during attempts to resolve the referenced
+// property; currently it only considers non-subclass and non-joined properties.  Part of the problem is in terms
+// of SQL generation.
+//	public static void createSyntheticPropertyReference(
+//			Ejb3JoinColumn[] columns,
+//			PersistentClass ownerEntity,
+//			PersistentClass associatedEntity,
+//			Value value,
+//			boolean inverse,
+//			Mappings mappings) {
+//		//associated entity only used for more precise exception, yuk!
+//		if ( columns[0].isImplicit() || StringHelper.isNotEmpty( columns[0].getMappedBy() ) ) return;
+//		int fkEnum = Ejb3JoinColumn.checkReferencedColumnsType( columns, ownerEntity, mappings );
+//		PersistentClass associatedClass = columns[0].getPropertyHolder() != null ?
+//				columns[0].getPropertyHolder().getPersistentClass() :
+//				null;
+//		if ( Ejb3JoinColumn.NON_PK_REFERENCE == fkEnum ) {
+//			//find properties associated to a certain column
+//			Object columnOwner = findColumnOwner( ownerEntity, columns[0].getReferencedColumn(), mappings );
+//			List<Property> properties = findPropertiesByColumns( columnOwner, columns, mappings );
+//
+//			if ( properties == null ) {
+//				//TODO use a ToOne type doing a second select
+//				StringBuilder columnsList = new StringBuilder();
+//				columnsList.append( "referencedColumnNames(" );
+//				for (Ejb3JoinColumn column : columns) {
+//					columnsList.append( column.getReferencedColumn() ).append( ", " );
+//				}
+//				columnsList.setLength( columnsList.length() - 2 );
+//				columnsList.append( ") " );
+//
+//				if ( associatedEntity != null ) {
+//					//overidden destination
+//					columnsList.append( "of " )
+//							.append( associatedEntity.getEntityName() )
+//							.append( "." )
+//							.append( columns[0].getPropertyName() )
+//							.append( " " );
+//				}
+//				else {
+//					if ( columns[0].getPropertyHolder() != null ) {
+//						columnsList.append( "of " )
+//								.append( columns[0].getPropertyHolder().getEntityName() )
+//								.append( "." )
+//								.append( columns[0].getPropertyName() )
+//								.append( " " );
+//					}
+//				}
+//				columnsList.append( "referencing " )
+//						.append( ownerEntity.getEntityName() )
+//						.append( " not mapped to a single property" );
+//				throw new AnnotationException( columnsList.toString() );
+//			}
+//
+//			final String referencedPropertyName;
+//
+//			if ( properties.size() == 1 ) {
+//				referencedPropertyName = properties.get(0).getName();
+//			}
+//			else {
+//				// Create a synthetic (embedded composite) property to use as the referenced property which
+//				// contains all the properties mapped to the referenced columns.  We need to make a shallow copy
+//				// of the properties to mark them as non-insertable/updatable.
+//
+//				// todo : what if the columns all match with an existing component?
+//
+//				StringBuilder propertyNameBuffer = new StringBuilder( "_" );
+//				propertyNameBuffer.append( associatedClass.getEntityName().replace( '.', '_' ) );
+//				propertyNameBuffer.append( "_" ).append( columns[0].getPropertyName() );
+//				String syntheticPropertyName = propertyNameBuffer.toString();
+//				//create an embeddable component
+//
+//				//todo how about properties.size() == 1, this should be much simpler
+//				Component embeddedComp = columnOwner instanceof PersistentClass ?
+//						new Component( mappings, (PersistentClass) columnOwner ) :
+//						new Component( mappings, (Join) columnOwner );
+//				embeddedComp.setEmbedded( true );
+//				embeddedComp.setNodeName( syntheticPropertyName );
+//				embeddedComp.setComponentClassName( embeddedComp.getOwner().getClassName() );
+//				for (Property property : properties) {
+//					Property clone = BinderHelper.shallowCopy( property );
+//					clone.setInsertable( false );
+//					clone.setUpdateable( false );
+//					clone.setNaturalIdentifier( false );
+//					clone.setGeneration( property.getGeneration() );
+//					embeddedComp.addProperty( clone );
+//				}
+//				SyntheticProperty synthProp = new SyntheticProperty();
+//				synthProp.setName( syntheticPropertyName );
+//				synthProp.setNodeName( syntheticPropertyName );
+//				synthProp.setPersistentClass( ownerEntity );
+//				synthProp.setUpdateable( false );
+//				synthProp.setInsertable( false );
+//				synthProp.setValue( embeddedComp );
+//				synthProp.setPropertyAccessorName( "embedded" );
+//				ownerEntity.addProperty( synthProp );
+//				//make it unique
+//				TableBinder.createUniqueConstraint( embeddedComp );
+//
+//				referencedPropertyName = syntheticPropertyName;
+//			}
+//
+//			/**
+//			 * creating the property ref to the new synthetic property
+//			 */
+//			if ( value instanceof ToOne ) {
+//				( (ToOne) value ).setReferencedPropertyName( referencedPropertyName );
+//				mappings.addUniquePropertyReference( ownerEntity.getEntityName(), referencedPropertyName );
+//			}
+//			else if ( value instanceof Collection ) {
+//				( (Collection) value ).setReferencedPropertyName( referencedPropertyName );
+//				//not unique because we could create a mtm wo association table
+//				mappings.addPropertyReference( ownerEntity.getEntityName(), referencedPropertyName );
+//			}
+//			else {
+//				throw new AssertionFailure(
+//						"Do a property ref on an unexpected Value type: "
+//								+ value.getClass().getName()
+//				);
+//			}
+//			mappings.addPropertyReferencedAssociation(
+//					( inverse ? "inverse__" : "" ) + associatedClass.getEntityName(),
+//					columns[0].getPropertyName(),
+//					referencedPropertyName
+//			);
+//		}
+//	}
+
 	public static void createSyntheticPropertyReference(
 			Ejb3JoinColumn[] columns,
 			PersistentClass ownerEntity,

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

@@ -1008,7 +1008,7 @@ private static SimpleValue buildCollectionKey(
 		}
 		else {
 			keyVal = (KeyValue) collValue.getOwner()
-					.getRecursiveProperty( propRef )
+					.getReferencedProperty( propRef )
 					.getValue();
 		}
 		DependantValue key = new DependantValue( mappings, collValue.getCollectionTable(), keyVal );

hibernate-core/src/main/java/org/hibernate/cfg/annotations/TableBinder.java

@@ -402,7 +402,7 @@ else if ( value instanceof DependantValue ) {
 							"No property ref found while expected"
 					);
 				}
-				Property synthProp = referencedEntity.getRecursiveProperty( referencedPropertyName );
+				Property synthProp = referencedEntity.getReferencedProperty( referencedPropertyName );
 				if ( synthProp == null ) {
 					throw new AssertionFailure(
 							"Cannot find synthProp: " + referencedEntity.getEntityName() + "." + referencedPropertyName

hibernate-core/src/main/java/org/hibernate/engine/internal/StatefulPersistenceContext.java

@@ -76,7 +76,9 @@
 import org.hibernate.pretty.MessageHelper;
 import org.hibernate.proxy.HibernateProxy;
 import org.hibernate.proxy.LazyInitializer;
+import org.hibernate.sql.Select;
 import org.hibernate.tuple.ElementWrapper;
+import org.hibernate.type.CollectionType;
 
 /**
  * A <strong>stateful</strong> implementation of the {@link PersistenceContext} contract meaning that we maintain this
@@ -775,6 +777,64 @@ public Object proxyFor(Object impl) throws HibernateException {
 	 */
 	@Override
 	public Object getCollectionOwner(Serializable key, CollectionPersister collectionPersister) throws MappingException {
+		// todo : we really just need to add a split in the notions of:
+		//		1) collection key
+		//		2) collection owner key
+		// these 2 are not always the same.  Same is true in the case of ToOne associations with property-ref...
+		final EntityPersister ownerPersister = collectionPersister.getOwnerEntityPersister();
+		if ( ownerPersister.getIdentifierType().getReturnedClass().isInstance( key ) ) {
+			return getEntity( session.generateEntityKey( key, collectionPersister.getOwnerEntityPersister() ) );
+		}
+
+		// we have a property-ref type mapping for the collection key.  But that could show up a few ways here...
+		//
+		//		1) The incoming key could be the entity itself...
+		if ( ownerPersister.isInstance( key ) ) {
+			final Serializable owenerId = ownerPersister.getIdentifier( key, session );
+			if ( owenerId == null ) {
+				return null;
+			}
+			return getEntity( session.generateEntityKey( owenerId, ownerPersister ) );
+		}
+
+		final CollectionType collectionType = collectionPersister.getCollectionType();
+
+		//		2) The incoming key is most likely the collection key which we need to resolve to the owner key
+		//			find the corresponding owner instance
+		//			a) try by EntityUniqueKey
+		if ( collectionType.getLHSPropertyName() != null ) {
+			Object owner = getEntity(
+					new EntityUniqueKey(
+							ownerPersister.getEntityName(),
+							collectionType.getLHSPropertyName(),
+							key,
+							collectionPersister.getKeyType(),
+							ownerPersister.getEntityMode(),
+							session.getFactory()
+					)
+			);
+			if ( owner != null ) {
+				return owner;
+			}
+
+			//		b) try by EntityKey, which means we need to resolve owner-key -> collection-key
+			//			IMPL NOTE : yes if we get here this impl is very non-performant, but PersistenceContext
+			//					was never designed to handle this case; adding that capability for real means splitting
+			//					the notions of:
+			//						1) collection key
+			//						2) collection owner key
+			// 					these 2 are not always the same (same is true in the case of ToOne associations with
+			// 					property-ref).  That would require changes to (at least) CollectionEntry and quite
+			//					probably changes to how the sql for collection initializers are generated
+			//
+			//			We could also possibly see if the referenced property is a natural id since we already have caching
+			//			in place of natural id snapshots.  BUt really its better to just do it the right way ^^ if we start
+			// 			going that route
+			final Serializable ownerId = ownerPersister.getIdByUniqueKey( key, collectionType.getLHSPropertyName(), session );
+			return getEntity( session.generateEntityKey( ownerId, ownerPersister ) );
+		}
+
+		// as a last resort this is what the old code did...
 		return getEntity( session.generateEntityKey( key, collectionPersister.getOwnerEntityPersister() ) );
 	}
 

hibernate-core/src/main/java/org/hibernate/engine/spi/CollectionEntry.java

@@ -182,6 +182,9 @@ private void dirty(PersistentCollection collection) throws HibernateException {
 	}
 
 	public void preFlush(PersistentCollection collection) throws HibernateException {
+		if ( loadedKey == null && collection.getKey() != null ) {
+			loadedKey = collection.getKey();
+		}
 
 		boolean nonMutableChange = collection.isDirty() &&
 				getLoadedPersister()!=null &&

hibernate-core/src/main/java/org/hibernate/mapping/JoinedSubclass.java

@@ -72,7 +72,7 @@ public void validate(Mapping mapping) throws MappingException {
 	public Iterator getReferenceablePropertyIterator() {
 		return getPropertyIterator();
 	}
-	
+
 	public Object accept(PersistentClassVisitor mv) {
 		return mv.accept(this);
 	}

hibernate-core/src/main/java/org/hibernate/persister/entity/AbstractEntityPersister.java

@@ -44,6 +44,7 @@
 import org.hibernate.LockMode;
 import org.hibernate.LockOptions;
 import org.hibernate.MappingException;
+import org.hibernate.PropertyAccessException;
 import org.hibernate.QueryException;
 import org.hibernate.StaleObjectStateException;
 import org.hibernate.StaleStateException;
@@ -1503,6 +1504,107 @@ public Object[] getDatabaseSnapshot(Serializable id, SessionImplementor session)
 
 	}
 
+	@Override
+	public Serializable getIdByUniqueKey(Serializable key, String uniquePropertyName, SessionImplementor session) throws HibernateException {
+		if ( LOG.isTraceEnabled() ) {
+			LOG.tracef(
+					"resolving unique key [%s] to identifier for entity [%s]",
+					key,
+					getEntityName()
+			);
+		}
+
+		int propertyIndex = getSubclassPropertyIndex( uniquePropertyName );
+		if ( propertyIndex < 0 ) {
+			throw new HibernateException(
+					"Could not determine Type for property [" + uniquePropertyName + "] on entity [" + getEntityName() + "]"
+			);
+		}
+		Type propertyType = getSubclassPropertyType( propertyIndex );
+
+		try {
+			PreparedStatement ps = session.getTransactionCoordinator()
+					.getJdbcCoordinator()
+					.getStatementPreparer()
+					.prepareStatement( generateIdByUniqueKeySelectString( uniquePropertyName ) );
+			try {
+				propertyType.nullSafeSet( ps, key, 1, session );
+				ResultSet rs = ps.executeQuery();
+				try {
+					//if there is no resulting row, return null
+					if ( !rs.next() ) {
+						return null;
+					}
+					return (Serializable) getIdentifierType().nullSafeGet( rs, getIdentifierAliases(), session, null );
+				}
+				finally {
+					rs.close();
+				}
+			}
+			finally {
+				ps.close();
+			}
+		}
+		catch ( SQLException e ) {
+			throw getFactory().getSQLExceptionHelper().convert(
+					e,
+					String.format(
+							"could not resolve unique property [%s] to identifier for entity [%s]",
+							uniquePropertyName,
+							getEntityName()
+					),
+					getSQLSnapshotSelectString()
+			);
+		}
+
+	}
+
+	protected String generateIdByUniqueKeySelectString(String uniquePropertyName) {
+		Select select = new Select( getFactory().getDialect() );
+
+		if ( getFactory().getSettings().isCommentsEnabled() ) {
+			select.setComment( "resolve id by unique property [" + getEntityName() + "." + uniquePropertyName + "]" );
+		}
+
+		final String rooAlias = getRootAlias();
+
+		select.setFromClause( fromTableFragment( rooAlias ) + fromJoinFragment( rooAlias, true, false ) );
+
+		SelectFragment selectFragment = new SelectFragment();
+		selectFragment.addColumns( rooAlias, getIdentifierColumnNames(), getIdentifierAliases() );
+		select.setSelectClause( selectFragment );
+
+		StringBuilder whereClauseBuffer = new StringBuilder();
+		final int uniquePropertyIndex = getSubclassPropertyIndex( uniquePropertyName );
+		final String uniquePropertyTableAlias = generateTableAlias(
+				rooAlias,
+				getSubclassPropertyTableNumber( uniquePropertyIndex )
+		);
+		String sep = "";
+		for ( String columnTemplate : getSubclassPropertyColumnReaderTemplateClosure()[uniquePropertyIndex] ) {
+			if ( columnTemplate == null ) {
+				continue;
+			}
+			final String columnReference = StringHelper.replace( columnTemplate, Template.TEMPLATE, uniquePropertyTableAlias );
+			whereClauseBuffer.append( sep ).append( columnReference ).append( "=?" );
+			sep = " and ";
+		}
+		for ( String formulaTemplate : getSubclassPropertyFormulaTemplateClosure()[uniquePropertyIndex] ) {
+			if ( formulaTemplate == null ) {
+				continue;
+			}
+			final String formulaReference = StringHelper.replace( formulaTemplate, Template.TEMPLATE, uniquePropertyTableAlias );
+			whereClauseBuffer.append( sep ).append( formulaReference ).append( "=?" );
+			sep = " and ";
+		}
+		whereClauseBuffer.append( whereJoinFragment( rooAlias, true, false ) );
+
+		select.setWhereClause( whereClauseBuffer.toString() );
+
+		return select.setOuterJoins( "", "" ).toStatementString();
+	}
+
+
 	/**
 	 * Generate the SQL that selects the version number by id
 	 */

hibernate-core/src/main/java/org/hibernate/persister/entity/EntityPersister.java

@@ -509,6 +509,8 @@ public void update(
 	public Object[] getDatabaseSnapshot(Serializable id, SessionImplementor session)
 	throws HibernateException;
 
+	public Serializable getIdByUniqueKey(Serializable key, String uniquePropertyName, SessionImplementor session);
+
 	/**
 	 * Get the current version of the object, or return null if there is no row for
 	 * the given identifier. In the case of unversioned data, return any object

hibernate-core/src/main/java/org/hibernate/sql/Select.java

@@ -151,6 +151,11 @@ public Select setSelectClause(String selectClause) {
 		return this;
 	}
 
+	public Select setSelectClause(SelectFragment selectFragment) {
+		setSelectClause( selectFragment.toFragmentString().substring( 2 ) );
+		return this;
+	}
+
 	/**
 	 * Sets the whereClause.
 	 * @param whereClause The whereClause to set
@@ -204,5 +209,4 @@ public Select setLockOptions(LockOptions lockOptions) {
 		LockOptions.copy(lockOptions, this.lockOptions);
 		return this;
 	}
-
 }