Hibernate
/
hibernate-orm
mirror of https://github.com/hibernate/hibernate-orm
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

HHH-10309 - HHH-10242 is too aggressive in cases of FIELD access 

(cherry picked from commit 1183a4719d)
This commit is contained in:
Steve Ebersole 2015-11-20 12:49:42 -06:00
parent c7abe770de
commit f5197a3f04
4 changed files with 324 additions and 152 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

34
hibernate-core/src/main/java/org/hibernate/cfg/AnnotationBinder.java
View File

@ -1468,8 +1468,6 @@ public final class AnnotationBinder {
/**
* @param elements List of {@code ProperyData} instances
* @param defaultAccessType The default value access strategy which has to be used in case no explicit local access
* strategy is used
* @param propertyContainer Metadata about a class and its properties
*
* @return the number of id properties found while iterating the elements of {@code annotatedClass} using
@ -1477,23 +1475,16 @@ public final class AnnotationBinder {
*/
static int addElementsOfClass(
List<PropertyData> elements,
AccessType defaultAccessType,
PropertyContainer propertyContainer,
MetadataBuildingContext context) {
int idPropertyCounter = 0;
AccessType accessType = defaultAccessType;
if ( propertyContainer.hasExplicitAccessStrategy() ) {
accessType = propertyContainer.getExplicitAccessStrategy();
}
Collection<XProperty> properties = propertyContainer.getProperties( accessType );
Collection<XProperty> properties = propertyContainer.getProperties();
for ( XProperty p : properties ) {
final int currentIdPropertyCounter = addProperty(
propertyContainer,
p,
elements,
accessType.getType(),
context
);
idPropertyCounter += currentIdPropertyCounter;
@ -1505,7 +1496,6 @@ public final class AnnotationBinder {
PropertyContainer propertyContainer,
XProperty property,
List<PropertyData> annElts,
String propertyAccessor,
MetadataBuildingContext context) {
final XClass declaringClass = propertyContainer.getDeclaringClass();
final XClass entity = propertyContainer.getEntityAtStake();
@ -1513,7 +1503,7 @@ public final class AnnotationBinder {
PropertyData propertyAnnotatedElement = new PropertyInferredData(
declaringClass,
property,
propertyAccessor,
propertyContainer.getClassLevelAccessType().getType(),
context.getBuildingOptions().getReflectionManager()
);
@ -1560,7 +1550,7 @@ public final class AnnotationBinder {
//same dec
prop,
// the actual @XToOne property
propertyAccessor,
propertyContainer.getClassLevelAccessType().getType(),
//TODO we should get the right accessor but the same as id would do
context.getBuildingOptions().getReflectionManager()
);
@ -2560,8 +2550,8 @@ public final class AnnotationBinder {
baseClassElements = new ArrayList<PropertyData>();
baseReturnedClassOrElement = baseInferredData.getClassOrElement();
bindTypeDefs( baseReturnedClassOrElement, buildingContext );
PropertyContainer propContainer = new PropertyContainer( baseReturnedClassOrElement, xClassProcessed );
addElementsOfClass( baseClassElements, propertyAccessor, propContainer, buildingContext );
PropertyContainer propContainer = new PropertyContainer( baseReturnedClassOrElement, xClassProcessed, propertyAccessor );
addElementsOfClass( baseClassElements, propContainer, buildingContext );
for ( PropertyData element : baseClassElements ) {
orderedBaseClassElements.put( element.getPropertyName(), element );
}
@ -2569,15 +2559,15 @@ public final class AnnotationBinder {
//embeddable elements can have type defs
bindTypeDefs( returnedClassOrElement, buildingContext );
PropertyContainer propContainer = new PropertyContainer( returnedClassOrElement, xClassProcessed );
addElementsOfClass( classElements, propertyAccessor, propContainer, buildingContext );
PropertyContainer propContainer = new PropertyContainer( returnedClassOrElement, xClassProcessed, propertyAccessor );
addElementsOfClass( classElements, propContainer, buildingContext );
//add elements of the embeddable superclass
XClass superClass = xClassProcessed.getSuperclass();
while ( superClass != null && superClass.isAnnotationPresent( MappedSuperclass.class ) ) {
//FIXME: proper support of typevariables incl var resolved at upper levels
propContainer = new PropertyContainer( superClass, xClassProcessed );
addElementsOfClass( classElements, propertyAccessor, propContainer, buildingContext );
propContainer = new PropertyContainer( superClass, xClassProcessed, propertyAccessor );
addElementsOfClass( classElements, propContainer, buildingContext );
superClass = superClass.getSuperclass();
}
if ( baseClassElements != null ) {
@ -2790,9 +2780,11 @@ public final class AnnotationBinder {
List<PropertyData> baseClassElements = new ArrayList<PropertyData>();
XClass baseReturnedClassOrElement = baseInferredData.getClassOrElement();
PropertyContainer propContainer = new PropertyContainer(
baseReturnedClassOrElement, inferredData.getPropertyClass()
baseReturnedClassOrElement,
inferredData.getPropertyClass(),
propertyAccessor
);
addElementsOfClass( baseClassElements, propertyAccessor, propContainer, context );
addElementsOfClass( baseClassElements, propContainer, context );
//Id properties are on top and there is only one
return baseClassElements.get( 0 );
}

4
hibernate-core/src/main/java/org/hibernate/cfg/InheritanceState.java
View File

@ -216,11 +216,11 @@ public class InheritanceState {
for ( XClass classToProcessForMappedSuperclass : classesToProcessForMappedSuperclass ) {
PropertyContainer propertyContainer = new PropertyContainer(
classToProcessForMappedSuperclass,
clazz
clazz,
accessType
);
int currentIdPropertyCount = AnnotationBinder.addElementsOfClass(
elements,
accessType,
propertyContainer,
buildingContext
);

373
hibernate-core/src/main/java/org/hibernate/cfg/PropertyContainer.java
View File

@ -11,6 +11,8 @@ package org.hibernate.cfg;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.TreeMap;
@ -28,7 +30,6 @@ import org.hibernate.annotations.Target;
import org.hibernate.annotations.Type;
import org.hibernate.annotations.common.reflection.XClass;
import org.hibernate.annotations.common.reflection.XProperty;
import org.hibernate.annotations.common.reflection.java.JavaXMember;
import org.hibernate.boot.jaxb.Origin;
import org.hibernate.boot.jaxb.SourceType;
import org.hibernate.cfg.annotations.HCANNHelper;
@ -43,47 +44,177 @@ import org.jboss.logging.Logger;
* @author Hardy Ferentschik
*/
class PropertyContainer {
static {
System.setProperty("jboss.i18n.generate-proxies", "true");
}
//
// static {
// System.setProperty("jboss.i18n.generate-proxies", "true");
// }
private static final CoreMessageLogger LOG = Logger.getMessageLogger(CoreMessageLogger.class, PropertyContainer.class.getName());
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) {
/**
* Holds the AccessType indicated for use at the class/container-level for cases where persistent attribute
* did not specify.
*/
private final AccessType classLevelAccessType;
private final TreeMap<String, XProperty> persistentAttributeMap;
PropertyContainer(XClass clazz, XClass entityAtStake, AccessType defaultClassLevelAccessType) {
this.xClass = clazz;
this.entityAtStake = entityAtStake;
explicitClassDefinedAccessType = determineClassDefinedAccessStrategy();
if ( defaultClassLevelAccessType == AccessType.DEFAULT ) {
// this is effectively what the old code did when AccessType.DEFAULT was passed in
// to getProperties(AccessType) from AnnotationBinder and InheritanceState
defaultClassLevelAccessType = AccessType.PROPERTY;
}
// first add all properties to field and property map
fieldAccessMap = initProperties( AccessType.FIELD );
propertyAccessMap = initProperties( AccessType.PROPERTY );
AccessType localClassLevelAccessType = determineLocalClassDefinedAccessStrategy();
assert localClassLevelAccessType != null;
considerExplicitFieldAndPropertyAccess();
this.classLevelAccessType = localClassLevelAccessType != AccessType.DEFAULT
? localClassLevelAccessType
: defaultClassLevelAccessType;
assert classLevelAccessType == AccessType.FIELD || classLevelAccessType == AccessType.PROPERTY;
this.persistentAttributeMap = new TreeMap<String, XProperty>();
final List<XProperty> fields = xClass.getDeclaredProperties( AccessType.FIELD.getType() );
final List<XProperty> getters = xClass.getDeclaredProperties( AccessType.PROPERTY.getType() );
preFilter( fields, getters );
final Map<String,XProperty> persistentAttributesFromGetters = new HashMap<String, XProperty>();
collectPersistentAttributesUsingLocalAccessType(
persistentAttributeMap,
persistentAttributesFromGetters,
fields,
getters
);
collectPersistentAttributesUsingClassLevelAccessType(
persistentAttributeMap,
persistentAttributesFromGetters,
fields,
getters
);
}
private void preFilter(List<XProperty> fields, List<XProperty> getters) {
Iterator<XProperty> propertyIterator = fields.iterator();
while ( propertyIterator.hasNext() ) {
final XProperty property = propertyIterator.next();
if ( mustBeSkipped( property ) ) {
propertyIterator.remove();
}
}
propertyIterator = getters.iterator();
while ( propertyIterator.hasNext() ) {
final XProperty property = propertyIterator.next();
if ( mustBeSkipped( property ) ) {
propertyIterator.remove();
}
}
}
private void collectPersistentAttributesUsingLocalAccessType(
TreeMap<String, XProperty> persistentAttributeMap,
Map<String,XProperty> persistentAttributesFromGetters,
List<XProperty> fields,
List<XProperty> getters) {
// Check fields...
Iterator<XProperty> propertyIterator = fields.iterator();
while ( propertyIterator.hasNext() ) {
final XProperty xProperty = propertyIterator.next();
final Access localAccessAnnotation = xProperty.getAnnotation( Access.class );
if ( localAccessAnnotation == null
|| localAccessAnnotation.value() != javax.persistence.AccessType.FIELD ) {
continue;
}
propertyIterator.remove();
persistentAttributeMap.put( xProperty.getName(), xProperty );
}
// Check getters...
propertyIterator = getters.iterator();
while ( propertyIterator.hasNext() ) {
final XProperty xProperty = propertyIterator.next();
final Access localAccessAnnotation = xProperty.getAnnotation( Access.class );
if ( localAccessAnnotation == null
|| localAccessAnnotation.value() != javax.persistence.AccessType.PROPERTY ) {
continue;
}
propertyIterator.remove();
final String name = xProperty.getName();
// HHH-10242 detect registration of the same property getter twice - eg boolean isId() + UUID getId()
final XProperty previous = persistentAttributesFromGetters.get( name );
if ( previous != null ) {
throw new org.hibernate.boot.MappingException(
LOG.ambiguousPropertyMethods(
xClass.getName(),
HCANNHelper.annotatedElementSignature( previous ),
HCANNHelper.annotatedElementSignature( xProperty )
),
new Origin( SourceType.ANNOTATION, xClass.getName() )
);
}
persistentAttributeMap.put( name, xProperty );
persistentAttributesFromGetters.put( name, xProperty );
}
}
private void collectPersistentAttributesUsingClassLevelAccessType(
TreeMap<String, XProperty> persistentAttributeMap,
Map<String,XProperty> persistentAttributesFromGetters,
List<XProperty> fields,
List<XProperty> getters) {
if ( classLevelAccessType == AccessType.FIELD ) {
for ( XProperty field : fields ) {
if ( persistentAttributeMap.containsKey( field.getName() ) ) {
continue;
}
persistentAttributeMap.put( field.getName(), field );
}
}
else {
for ( XProperty getter : getters ) {
final String name = getter.getName();
// HHH-10242 detect registration of the same property getter twice - eg boolean isId() + UUID getId()
final XProperty previous = persistentAttributesFromGetters.get( name );
if ( previous != null ) {
throw new org.hibernate.boot.MappingException(
LOG.ambiguousPropertyMethods(
xClass.getName(),
HCANNHelper.annotatedElementSignature( previous ),
HCANNHelper.annotatedElementSignature( getter )
),
new Origin( SourceType.ANNOTATION, xClass.getName() )
);
}
if ( persistentAttributeMap.containsKey( name ) ) {
continue;
}
persistentAttributeMap.put( getter.getName(), getter );
persistentAttributesFromGetters.put( name, getter );
}
}
}
public XClass getEntityAtStake() {
@ -94,120 +225,104 @@ class PropertyContainer {
return xClass;
}
public AccessType getExplicitAccessStrategy() {
return explicitClassDefinedAccessType;
public AccessType getClassLevelAccessType() {
return classLevelAccessType;
}
public boolean hasExplicitAccessStrategy() {
return !explicitClassDefinedAccessType.equals( AccessType.DEFAULT );
public Collection<XProperty> getProperties() {
assertTypesAreResolvable();
return Collections.unmodifiableCollection( persistentAttributeMap.values() );
}
public Collection<XProperty> getProperties(AccessType accessType) {
assertTypesAreResolvable( accessType );
if ( AccessType.DEFAULT == accessType || AccessType.PROPERTY == accessType ) {
return Collections.unmodifiableCollection( propertyAccessMap.values() );
}
else {
return Collections.unmodifiableCollection( fieldAccessMap.values() );
}
}
private void assertTypesAreResolvable(AccessType access) {
Map<String, XProperty> xprops;
if ( AccessType.PROPERTY.equals( access ) || AccessType.DEFAULT.equals( access ) ) {
xprops = propertyAccessMap;
}
else {
xprops = fieldAccessMap;
}
for ( XProperty property : xprops.values() ) {
if ( !property.isTypeResolved() && !discoverTypeWithoutReflection( property ) ) {
String msg = "Property " + StringHelper.qualify( xClass.getName(), property.getName() ) +
private void assertTypesAreResolvable() {
for ( XProperty xProperty : persistentAttributeMap.values() ) {
if ( !xProperty.isTypeResolved() && !discoverTypeWithoutReflection( xProperty ) ) {
String msg = "Property " + StringHelper.qualify( xClass.getName(), xProperty.getName() ) +
" has an unbound type and no explicit target entity. Resolve this Generic usage issue" +
" or set an explicit target attribute (eg @OneToMany(target=) or use an explicit @Type";
throw new AnnotationException( msg );
}
}
}
//
// private void considerExplicitFieldAndPropertyAccess() {
// 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.FIELD) {
// propertyAccessMap.put(property.getName(), property);
// }
// else {
// LOG.debug( "Placing @Access(AccessType.FIELD) on a field does not have any effect." );
// }
// }
//
// for ( XProperty property : propertyAccessMap.values() ) {
// Access access = property.getAnnotation( Access.class );
// if ( access == null ) {
// continue;
// }
//
// AccessType accessType = AccessType.getAccessStrategy( access.value() );
//
// // 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
// // return use method access even if the default class access type is AccessType.FIELD
// if (accessType == AccessType.PROPERTY) {
// fieldAccessMap.put(property.getName(), property);
// }
// else {
// LOG.debug( "Placing @Access(AccessType.PROPERTY) on a field does not have any effect." );
// }
// }
// }
private void considerExplicitFieldAndPropertyAccess() {
for ( XProperty property : fieldAccessMap.values() ) {
Access access = property.getAnnotation( Access.class );
if ( access == null ) {
continue;
}
// /**
// * 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( "Access 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() );
// for ( XProperty property : properties ) {
// if ( mustBeSkipped( property ) ) {
// continue;
// }
// // HHH-10242 detect registration of the same property twice eg boolean isId() + UUID getId()
// XProperty oldProperty = propertiesMap.get( property.getName() );
// if ( oldProperty != null ) {
// throw new org.hibernate.boot.MappingException(
// LOG.ambiguousPropertyMethods(
// xClass.getName(),
// HCANNHelper.annotatedElementSignature( oldProperty ),
// HCANNHelper.annotatedElementSignature( property )
// ),
// new Origin( SourceType.ANNOTATION, xClass.getName() )
// );
// }
//
// propertiesMap.put( property.getName(), property );
// }
// return propertiesMap;
// }
// 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.FIELD) {
propertyAccessMap.put(property.getName(), property);
}
else {
LOG.debug( "Placing @Access(AccessType.FIELD) on a field does not have any effect." );
}
}
for ( XProperty property : propertyAccessMap.values() ) {
Access access = property.getAnnotation( Access.class );
if ( access == null ) {
continue;
}
AccessType accessType = AccessType.getAccessStrategy( access.value() );
// 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
// return use method access even if the default class access type is AccessType.FIELD
if (accessType == AccessType.PROPERTY) {
fieldAccessMap.put(property.getName(), property);
}
else {
LOG.debug( "Placing @Access(AccessType.PROPERTY) on a field 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( "Access 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() );
for ( XProperty property : properties ) {
if ( mustBeSkipped( property ) ) {
continue;
}
// HHH-10242 detect registration of the same property twice eg boolean isId() + UUID getId()
XProperty oldProperty = propertiesMap.get( property.getName() );
if ( oldProperty != null ) {
throw new org.hibernate.boot.MappingException(
LOG.ambiguousPropertyMethods(
xClass.getName(),
HCANNHelper.annotatedElementSignature( oldProperty ),
HCANNHelper.annotatedElementSignature( property )
),
new Origin( SourceType.ANNOTATION, xClass.getName() )
);
}
propertiesMap.put( property.getName(), property );
}
return propertiesMap;
}
private AccessType determineClassDefinedAccessStrategy() {
private AccessType determineLocalClassDefinedAccessStrategy() {
AccessType classDefinedAccessType;
AccessType hibernateDefinedAccessType = AccessType.DEFAULT;

65
hibernate-core/src/test/java/org/hibernate/property/GetAndIsVariantGetterTest.java
View File

@ -6,10 +6,14 @@
*/
package org.hibernate.property;
import javax.persistence.Access;
import javax.persistence.AccessType;
import javax.persistence.Entity;
import javax.persistence.Id;
import javax.persistence.Transient;
import org.hibernate.MappingException;
import org.hibernate.boot.Metadata;
import org.hibernate.boot.MetadataSources;
import org.hibernate.boot.registry.StandardServiceRegistry;
import org.hibernate.boot.registry.StandardServiceRegistryBuilder;
@ -20,6 +24,7 @@ import org.junit.BeforeClass;
import org.junit.Test;
import static org.hamcrest.CoreMatchers.startsWith;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.fail;
@ -71,6 +76,27 @@ public class GetAndIsVariantGetterTest {
}
}
@Test
@TestForIssue( jiraKey = "HHH-10242" )
public void testAnnotationsCorrected() {
Metadata metadata = new MetadataSources( ssr )
.addAnnotatedClass( TheEntity2.class )
.buildMetadata();
assertNotNull( metadata.getEntityBinding( TheEntity2.class.getName() ).getIdentifier() );
assertNotNull( metadata.getEntityBinding( TheEntity2.class.getName() ).getIdentifierProperty() );
}
@Test
@TestForIssue( jiraKey = "HHH-10309" )
public void testAnnotationsFieldAccess() {
// this one should be ok because the AccessType is FIELD
Metadata metadata = new MetadataSources( ssr )
.addAnnotatedClass( AnotherEntity.class )
.buildMetadata();
assertNotNull( metadata.getEntityBinding( AnotherEntity.class.getName() ).getIdentifier() );
assertNotNull( metadata.getEntityBinding( AnotherEntity.class.getName() ).getIdentifierProperty() );
}
@Entity
public static class TheEntity {
private Integer id;
@ -88,4 +114,43 @@ public class GetAndIsVariantGetterTest {
this.id = id;
}
}
@Entity
public static class TheEntity2 {
private Integer id;
@Transient
public boolean isId() {
return false;
}
@Id
public Integer getId() {
return id;
}
public void setId(Integer id) {
this.id = id;
}
}
@Entity
@Access(AccessType.PROPERTY)
public static class AnotherEntity {
@Id
@Access(AccessType.FIELD)
private Integer id;
public boolean isId() {
return false;
}
public Integer getId() {
return id;
}
public void setId(Integer id) {
this.id = id;
}
}
}