HHH-5004 Fixed problems in PropertyContainer and added some more comments. Cleaned up some code in InheritanceState

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

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

@@ -1341,7 +1341,6 @@ public final class AnnotationBinder {
 			accessType = propertyContainer.getExplicitAccessStrategy();
 		}
 
-		propertyContainer.assertTypesAreResolvable( accessType );
 		Collection<XProperty> properties = propertyContainer.getProperties( accessType );
 		for ( XProperty p : properties ) {
 			final int currentIdPropertyCounter = addProperty(

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

@@ -35,7 +35,6 @@ import javax.persistence.InheritanceType;
 import javax.persistence.MappedSuperclass;
 
 import org.hibernate.AnnotationException;
-import org.hibernate.annotations.common.reflection.ReflectionManager;
 import org.hibernate.annotations.common.reflection.XAnnotatedElement;
 import org.hibernate.annotations.common.reflection.XClass;
 import org.hibernate.annotations.common.reflection.XProperty;
@@ -49,7 +48,6 @@ import org.hibernate.mapping.PersistentClass;
  */
 public class InheritanceState {
 	private XClass clazz;
-	private XClass identifierType;
 
 	/**
 	 * Has sibling (either mappedsuperclass entity)
@@ -75,7 +73,6 @@ public class InheritanceState {
 		this.setClazz( clazz );
 		this.mappings = mappings;
 		this.inheritanceStatePerClass = inheritanceStatePerClass;
-		this.identifierType = mappings.getReflectionManager().toXClass( void.class ); //not initialized
 		extractInheritanceType();
 	}
 
@@ -216,6 +213,7 @@ public class InheritanceState {
      * Get the annotated elements, guessing the access type from @Id or @EmbeddedId presence.
      * Change EntityBinder by side effect
      */
+
 	public ElementsToProcess getElementsToProcess() {
 		if ( elementsToProcess == null ) {
 			InheritanceState inheritanceState = inheritanceStatePerClass.get( clazz );
@@ -231,8 +229,14 @@ public class InheritanceState {
 			int idPropertyCount = 0;
 
 			for ( int index = 0; index < deep; index++ ) {
-				PropertyContainer propertyContainer = new PropertyContainer( classesToProcessForMappedSuperclass.get( index ), clazz );
+				PropertyContainer propertyContainer = new PropertyContainer(
-				int currentIdPropertyCount = AnnotationBinder.addElementsOfClass( elements, accessType, propertyContainer, mappings );
+						classesToProcessForMappedSuperclass.get(
+								index
+						), clazz
+				);
+				int currentIdPropertyCount = AnnotationBinder.addElementsOfClass(
+						elements, accessType, propertyContainer, mappings
+				);
 				idPropertyCount += currentIdPropertyCount;
 			}
 
@@ -251,25 +255,19 @@ public class InheritanceState {
 				for ( XProperty prop : xclass.getDeclaredProperties( AccessType.PROPERTY.getType() ) ) {
 					final boolean isEmbeddedId = prop.isAnnotationPresent( EmbeddedId.class );
 					if ( prop.isAnnotationPresent( Id.class ) || isEmbeddedId ) {
-						if (isEmbeddedId) {
-							identifierType = prop.getClassOrElementClass();
-						}
 						return AccessType.PROPERTY;
 					}
 				}
 				for ( XProperty prop : xclass.getDeclaredProperties( AccessType.FIELD.getType() ) ) {
 					final boolean isEmbeddedId = prop.isAnnotationPresent( EmbeddedId.class );
 					if ( prop.isAnnotationPresent( Id.class ) || isEmbeddedId ) {
-						if ( isEmbeddedId ) {
-							identifierType = prop.getClassOrElementClass();
-						}
 						return AccessType.FIELD;
 					}
 				}
 			}
 			xclass = xclass.getSuperclass();
 		}
-		throw new AnnotationException( "No identifier specified for entity: " + clazz.getName() );
+		throw new AnnotationException( "No identifier specified for entity: " + clazz );
 	}
 
 	private void getMappedSuperclassesTillNextEntityOrdered() {
@@ -305,7 +303,8 @@ public class InheritanceState {
 		final int lastMappedSuperclass = classesToProcessForMappedSuperclass.size() - 1;
 		for ( int index = 0; index < lastMappedSuperclass; index++ ) {
 			org.hibernate.mapping.MappedSuperclass parentSuperclass = mappedSuperclass;
-			final Class<?> type = mappings.getReflectionManager().toClass( classesToProcessForMappedSuperclass.get( index ) );
+			final Class<?> type = mappings.getReflectionManager()
+					.toClass( classesToProcessForMappedSuperclass.get( index ) );
 			//add MAppedSuperclass if not already there
 			mappedSuperclass = mappings.getMappedSuperclass( type );
 			if ( mappedSuperclass == null ) {

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

@@ -24,9 +24,10 @@
  */
 package org.hibernate.cfg;
 
-import java.util.ArrayList;
 import java.util.Collection;
+import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.TreeMap;
 import javax.persistence.Access;
 import javax.persistence.ManyToMany;
@@ -55,19 +56,40 @@ import org.hibernate.util.StringHelper;
 class PropertyContainer {
 
 	private static final Logger log = LoggerFactory.getLogger( AnnotationBinder.class );
-	private final XClass entityAtStake;
+
-	private final TreeMap<String, XProperty> fieldAccessMap;
-	private final TreeMap<String, XProperty> propertyAccessMap;
-	private final XClass xClass;
 	private final AccessType explicitClassDefinedAccessType;
 
+	/**
+	 * Constains the properties which must be returned in case the class is accessed via {@code AccessType.FIELD}. Note,
+	 * this does not mean that all {@code XProperty}s in this map are fields. Due to JPA access rules single properties
+	 * can have different access type than the overall class access type.
+	 */
+	private final TreeMap<String, XProperty> fieldAccessMap;
+
+	/**
+	 * Constains the properties which must be returned in case the class is accessed via {@code AccessType.Property}. Note,
+	 * this does not mean that all {@code XProperty}s in this map are properties/methods. Due to JPA access rules single properties
+	 * can have different access type than the overall class access type.
+	 */
+	private final TreeMap<String, XProperty> propertyAccessMap;
+
+	/**
+	 * The class for which this container is created.
+	 */
+	private final XClass xClass;
+	private final XClass entityAtStake;
+
 	PropertyContainer(XClass clazz, XClass entityAtStake) {
 		this.xClass = clazz;
 		this.entityAtStake = entityAtStake;
+
+		explicitClassDefinedAccessType = determineClassDefinedAccessStrategy();
+
+		// first add all properties to field and property map
 		fieldAccessMap = initProperties( AccessType.FIELD );
 		propertyAccessMap = initProperties( AccessType.PROPERTY );
-		explicitClassDefinedAccessType = determineClassDefinedAccessStrategy();
+
-		checkForJpaAccess();
+		considerExplicitFieldAndPropertyAccess();
 	}
 
 	public XClass getEntityAtStake() {
@@ -87,16 +109,17 @@ class PropertyContainer {
 	}
 
 	public Collection<XProperty> getProperties(AccessType accessType) {
+		assertTypesAreResolvable( accessType );
 		if ( AccessType.DEFAULT == accessType || AccessType.PROPERTY == accessType ) {
-			return propertyAccessMap.values();
+			return Collections.unmodifiableCollection( propertyAccessMap.values() );
 		}
 		else {
-			return fieldAccessMap.values();
+			return Collections.unmodifiableCollection( fieldAccessMap.values() );
 		}
 	}
 
-	public void assertTypesAreResolvable(AccessType access) {
+	private void assertTypesAreResolvable(AccessType access) {
-		TreeMap<String, XProperty> xprops;
+		Map<String, XProperty> xprops;
 		if ( AccessType.PROPERTY.equals( access ) || AccessType.DEFAULT.equals( access ) ) {
 			xprops = propertyAccessMap;
 		}
@@ -113,29 +136,25 @@ class PropertyContainer {
 		}
 	}
 
-	private void checkForJpaAccess() {
+	private void considerExplicitFieldAndPropertyAccess() {
-		List<XProperty> tmpList = new ArrayList<XProperty>();
 		for ( XProperty property : fieldAccessMap.values() ) {
 			Access access = property.getAnnotation( Access.class );
 			if ( access == null ) {
 				continue;
 			}
 
+			// see "2.3.2 Explicit Access Type" of JPA 2 spec
+			// the access type for this property is explicitly set to AccessType.FIELD, hence we have to
+			// use field access for this property even if the default access type for the class is AccessType.PROPERTY
 			AccessType accessType = AccessType.getAccessStrategy( access.value() );
-			if ( accessType == AccessType.PROPERTY ) {
+			if ( accessType == AccessType.FIELD ) {
-				log.warn( "Placing @Access(AccessType.PROPERTY) on a field does not have any effect." );
-				continue;
-			}
-
-			tmpList.add( property );
-		}
-		for ( XProperty property : tmpList ) {
-			fieldAccessMap.remove( property.getName() );
 				propertyAccessMap.put( property.getName(), property );
 			}
+			else {   // AccessType.PROPERTY
+				log.warn( "Placing @Access(AccessType.PROPERTY) on a field does not have any effect." );
+			}
+		}
 
-
-		tmpList.clear();
 		for ( XProperty property : propertyAccessMap.values() ) {
 			Access access = property.getAnnotation( Access.class );
 			if ( access == null ) {
@@ -143,20 +162,32 @@ class PropertyContainer {
 			}
 
 			AccessType accessType = AccessType.getAccessStrategy( access.value() );
-			if ( accessType == AccessType.FIELD ) {
-				log.warn( "Placing @Access(AccessType.FIELD) on a field does not have any effect." );
-				continue;
-			}
 
-			tmpList.add( property );
+			// see "2.3.2 Explicit Access Type" of JPA 2 spec
-		}
+			// the access type for this property is explicitly set to AccessType.PROPERTY, hence we have to
-		for ( XProperty property : tmpList ) {
+			// return use method access even if the default class access type is AccessType.FIELD
-			propertyAccessMap.remove( property.getName() );
+			if ( accessType == AccessType.PROPERTY ) {
 				fieldAccessMap.put( property.getName(), property );
 			}
+			else { // AccessType.FIELD
+				log.warn( "Placing @Access(AccessType.FIELD) on a property does not have any effect." );
+			}
+		}
 	}
 
+	/**
+	 * Retrieves all properties from the {@code xClass} with the specified access type. This method does not take
+	 * any jpa access rules/annotations into account yet.
+	 *
+	 * @param access The access type - {@code AccessType.FIELD}  or {@code AccessType.Property}
+	 *
+	 * @return A maps of the properties with the given access type keyed against their property name
+	 */
 	private TreeMap<String, XProperty> initProperties(AccessType access) {
+		if ( !( AccessType.PROPERTY.equals( access ) || AccessType.FIELD.equals( access ) ) ) {
+			throw new IllegalArgumentException( "Acces type has to be AccessType.FIELD or AccessType.Property" );
+		}
+
 		//order so that property are used in the same order when binding native query
 		TreeMap<String, XProperty> propertiesMap = new TreeMap<String, XProperty>();
 		List<XProperty> properties = xClass.getDeclaredProperties( access.getType() );

annotations/src/test/java/org/hibernate/test/annotations/access/jpa/AccessMappingTest.java

@@ -192,4 +192,14 @@ public class AccessMappingTest extends TestCase {
 				tuplizer.getGetter( 0 ) instanceof BasicPropertyAccessor.BasicGetter
 		);
 	}
+
+	/**
+	 * HHH-5004
+	 */
+	public void testAccessOnClassAndId() throws Exception {
+		AnnotationConfiguration cfg = new AnnotationConfiguration();
+		cfg.addAnnotatedClass( Course8.class );
+		cfg.addAnnotatedClass( Student.class );
+		cfg.buildSessionFactory();
+	}
 }

annotations/src/test/java/org/hibernate/test/annotations/access/jpa/AccessTest.java

@@ -34,6 +34,7 @@ import org.hibernate.test.annotations.access.Closet;
 
 /**
  * @author Emmanuel Bernard
+ * @author Hardy Ferentschik
  */
 public class AccessTest extends TestCase {
 

annotations/src/test/java/org/hibernate/test/annotations/access/jpa/Course8.java

@@ -0,0 +1,78 @@
+//$Id: AccessTest.java 15025 2008-08-11 09:14:39Z hardy.ferentschik $
+/*
+ * Hibernate, Relational Persistence for Idiomatic Java
+ *
+ * Copyright (c) 2008, Red Hat Middleware LLC or third-party contributors as
+ * indicated by the @author tags or express copyright attribution
+ * statements applied by the authors.  All third-party contributions are
+ * distributed under license by Red Hat Middleware LLC.
+ *
+ * This copyrighted material is made available to anyone wishing to use, modify,
+ * copy, or redistribute it subject to the terms and conditions of the GNU
+ * Lesser General Public License, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU Lesser General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this distribution; if not, write to:
+ * Free Software Foundation, Inc.
+ * 51 Franklin Street, Fifth Floor
+ * Boston, MA  02110-1301  USA
+ */
+package org.hibernate.test.annotations.access.jpa;
+
+import java.util.List;
+import javax.persistence.Access;
+import javax.persistence.AccessType;
+import javax.persistence.CascadeType;
+import javax.persistence.Entity;
+import javax.persistence.GeneratedValue;
+import javax.persistence.Id;
+import javax.persistence.OneToMany;
+
+
+/**
+ * Test class for HHH-5004
+ *
+ * @author Hardy Ferentschik
+ */
+@Entity
+@Access(AccessType.PROPERTY)
+public class Course8 {
+	private long id;
+
+	private String title;
+
+	private List<Student> students;
+
+	@Id
+	@GeneratedValue
+	@Access(AccessType.PROPERTY)
+	public long getId() {
+		return id;
+	}
+
+	public void setId(long id) {
+		this.id = id;
+	}
+
+	@OneToMany(cascade = CascadeType.ALL)
+	public List<Student> getStudents() {
+		return students;
+	}
+
+	public void setStudents(List<Student> students) {
+		this.students = students;
+	}
+
+	public String getTitle() {
+		return title;
+	}
+
+	public void setTitle(String title) {
+		this.title = title;
+	}
+}