From dcb2c60d4e3a43b3e0c37db42df561258f00cae3 Mon Sep 17 00:00:00 2001 From: Gavin King Date: Sun, 11 Feb 2024 15:46:04 +0100 Subject: [PATCH] HHH-17729 move validation of constructors in HQL instantiations to SemanticQueryBuilder also validate injection via fields/properties --- .../hql/internal/SemanticQueryBuilder.java | 10 +- .../tree/select/SqmDynamicInstantiation.java | 40 ++++---- ...icInstantiationAssemblerInjectionImpl.java | 95 ++++++------------- .../internal/InstantiationHelper.java | 62 ++++++++++++ 4 files changed, 120 insertions(+), 87 deletions(-) diff --git a/hibernate-core/src/main/java/org/hibernate/query/hql/internal/SemanticQueryBuilder.java b/hibernate-core/src/main/java/org/hibernate/query/hql/internal/SemanticQueryBuilder.java index 9a22b5e154..dc77428c56 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/hql/internal/SemanticQueryBuilder.java +++ b/hibernate-core/src/main/java/org/hibernate/query/hql/internal/SemanticQueryBuilder.java @@ -1466,9 +1466,13 @@ public class SemanticQueryBuilder extends HqlParserBaseVisitor implem } if ( !dynamicInstantiation.checkInstantiation( creationContext.getTypeConfiguration() ) ) { - throw new SemanticException( "No matching constructor for type '" - + dynamicInstantiation.getJavaType().getSimpleName() + "'", - query ); + final String typeName = dynamicInstantiation.getJavaType().getSimpleName(); + if ( dynamicInstantiation.isFullyAliased() ) { + throw new SemanticException( "Missing constructor or attributes for injection into type '" + typeName + "'", query ); + } + else { + throw new SemanticException( "Missing constructor for type '" + typeName + "'", query ); + } } return dynamicInstantiation; diff --git a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmDynamicInstantiation.java b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmDynamicInstantiation.java index 369b297536..fd1a337729 100644 --- a/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmDynamicInstantiation.java +++ b/hibernate-core/src/main/java/org/hibernate/query/sqm/tree/select/SqmDynamicInstantiation.java @@ -6,7 +6,6 @@ */ package org.hibernate.query.sqm.tree.select; -import java.lang.reflect.Constructor; import java.util.ArrayList; import java.util.List; import java.util.Map; @@ -31,6 +30,7 @@ import static org.hibernate.query.sqm.DynamicInstantiationNature.CLASS; import static org.hibernate.query.sqm.DynamicInstantiationNature.LIST; import static org.hibernate.query.sqm.DynamicInstantiationNature.MAP; import static org.hibernate.sql.results.graph.instantiation.internal.InstantiationHelper.isConstructorCompatible; +import static org.hibernate.sql.results.graph.instantiation.internal.InstantiationHelper.isInjectionCompatible; /** * Represents a dynamic instantiation ({@code select new XYZ(...) ...}) as part of the SQM. @@ -122,28 +122,35 @@ public class SqmDynamicInstantiation // where Class objects not available during build return true; } - if ( getArguments().stream().allMatch(arg -> arg.getAlias() != null ) ) { - // it's probably a bean injection-type instantiator, don't check it now - return true; + final List> argTypes = argumentTypes(); + if ( isFullyAliased() ) { + final List aliases = + getArguments().stream() + .map(SqmDynamicInstantiationArgument::getAlias) + .collect(toList()); + return isInjectionCompatible( getJavaType(), aliases, argTypes ) + || isConstructorCompatible( getJavaType(), argTypes, typeConfiguration ); } else { - final List> argTypes = - getArguments().stream() - .map(arg -> arg.getNodeJavaType().getJavaTypeClass()) - .collect(toList()); - for ( Constructor constructor : getJavaType().getDeclaredConstructors() ) { - if ( isConstructorCompatible( constructor, argTypes, typeConfiguration ) ) { - return true; - } - } - return false; + return isConstructorCompatible( getJavaType(), argTypes, typeConfiguration ); } } else { + // TODO: is there anything we need to check for list/map instantiation? return true; } } + private List> argumentTypes() { + return getArguments().stream() + .map(arg -> arg.getNodeJavaType().getJavaTypeClass()) + .collect(toList()); + } + + public boolean isFullyAliased() { + return getArguments().stream().allMatch( arg -> arg.getAlias() != null ); + } + @Override public SqmDynamicInstantiation copy(SqmCopyContext context) { final SqmDynamicInstantiation existing = context.getCopy( this ); @@ -323,9 +330,4 @@ public class SqmDynamicInstantiation visitSubSelectableNodes( list::add ); return list; } - - @Override - public boolean isCompoundSelection() { - return false; - } } diff --git a/hibernate-core/src/main/java/org/hibernate/sql/results/graph/instantiation/internal/DynamicInstantiationAssemblerInjectionImpl.java b/hibernate-core/src/main/java/org/hibernate/sql/results/graph/instantiation/internal/DynamicInstantiationAssemblerInjectionImpl.java index 8df680784c..835a03d121 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/results/graph/instantiation/internal/DynamicInstantiationAssemblerInjectionImpl.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/results/graph/instantiation/internal/DynamicInstantiationAssemblerInjectionImpl.java @@ -6,21 +6,25 @@ */ package org.hibernate.sql.results.graph.instantiation.internal; +import java.beans.BeanInfo; import java.beans.PropertyDescriptor; import java.lang.reflect.Constructor; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List; import org.hibernate.internal.util.beans.BeanInfoHelper; import org.hibernate.query.sqm.sql.internal.InstantiationException; -import org.hibernate.query.sqm.tree.expression.Compatibility; import org.hibernate.sql.results.graph.DomainResultAssembler; import org.hibernate.sql.results.jdbc.spi.JdbcValuesSourceProcessingOptions; import org.hibernate.sql.results.jdbc.spi.RowProcessingState; import org.hibernate.type.descriptor.java.JavaType; +import static org.hibernate.sql.results.graph.instantiation.internal.InstantiationHelper.findField; +import static org.hibernate.sql.results.graph.instantiation.internal.InstantiationHelper.propertyMatches; + /** * @author Steve Ebersole */ @@ -38,52 +42,7 @@ public class DynamicInstantiationAssemblerInjectionImpl implements DomainResu targetJavaType, beanInfo -> { for ( ArgumentReader argumentReader : argumentReaders ) { - boolean found = false; - for ( PropertyDescriptor propertyDescriptor : beanInfo.getPropertyDescriptors() ) { - if ( argumentReader.getAlias().equals( propertyDescriptor.getName() ) ) { - if ( propertyDescriptor.getWriteMethod() != null ) { - final boolean assignmentCompatible = Compatibility.areAssignmentCompatible( - propertyDescriptor.getWriteMethod().getParameterTypes()[0], - argumentReader.getAssembledJavaType().getClass() - ); - if ( assignmentCompatible ) { - propertyDescriptor.getWriteMethod().setAccessible( true ); - beanInjections.add( - new BeanInjection( - new BeanInjectorSetter<>( propertyDescriptor.getWriteMethod() ), - argumentReader - ) - ); - found = true; - break; - } - } - } - } - if ( found ) { - continue; - } - - // see if we can find a Field with the given name... - final Field field = findField( - targetJavaType, - argumentReader.getAlias(), - argumentReader.getAssembledJavaType().getJavaTypeClass() - ); - if ( field != null ) { - beanInjections.add( - new BeanInjection( - new BeanInjectorField<>( field ), - argumentReader - ) - ); - } - else { - throw new InstantiationException( - "Cannot set field '" + argumentReader.getAlias() - + "' to instantiate '" + targetJavaType.getName() + "'" - ); - } + beanInjections.add( injection( beanInfo, argumentReader, targetJavaType ) ); } } ); @@ -93,22 +52,29 @@ public class DynamicInstantiationAssemblerInjectionImpl implements DomainResu } } - private Field findField(Class declaringClass, String name, Class javaType) { - try { - final Field field = declaringClass.getDeclaredField( name ); - // field should never be null - if ( Compatibility.areAssignmentCompatible( field.getType(), javaType ) ) { - field.setAccessible( true ); - return field; - } - } - catch (NoSuchFieldException ignore) { - if ( declaringClass.getSuperclass() != null ) { - return findField( declaringClass.getSuperclass(), name, javaType ); + private static BeanInjection injection(BeanInfo beanInfo, ArgumentReader argument, Class targetJavaType) { + final Class argType = argument.getAssembledJavaType().getJavaTypeClass(); + final String alias = argument.getAlias(); + + // see if we can find a property with the given name... + for ( PropertyDescriptor propertyDescriptor : beanInfo.getPropertyDescriptors() ) { + if ( propertyMatches( alias, argType, propertyDescriptor ) ) { + final Method setter = propertyDescriptor.getWriteMethod(); + setter.setAccessible(true); + return new BeanInjection( new BeanInjectorSetter<>( setter ), argument ); } } - return null; + // see if we can find a Field with the given name... + final Field field = findField( targetJavaType, alias, argType ); + if ( field != null ) { + return new BeanInjection( new BeanInjectorField<>( field ), argument ); + } + else { + throw new InstantiationException( + "Cannot set field '" + alias + "' to instantiate '" + targetJavaType.getName() + "'" + ); + } } @Override @@ -125,15 +91,14 @@ public class DynamicInstantiationAssemblerInjectionImpl implements DomainResu constructor.setAccessible( true ); result = constructor.newInstance(); } - catch (NoSuchMethodException | IllegalAccessException | InvocationTargetException | InstantiationException | java.lang.InstantiationException e) { + catch ( NoSuchMethodException | IllegalAccessException | InvocationTargetException | InstantiationException + | java.lang.InstantiationException e ) { throw new InstantiationException( "Error instantiating class '" + target.getJavaType().getTypeName() + "' using default constructor: " + e.getMessage(), e ); } for ( BeanInjection beanInjection : beanInjections ) { - beanInjection.getBeanInjector().inject( - result, - beanInjection.getValueAssembler().assemble( rowProcessingState, options ) - ); + final Object assembled = beanInjection.getValueAssembler().assemble( rowProcessingState, options ); + beanInjection.getBeanInjector().inject( result, assembled ); } return result; } diff --git a/hibernate-core/src/main/java/org/hibernate/sql/results/graph/instantiation/internal/InstantiationHelper.java b/hibernate-core/src/main/java/org/hibernate/sql/results/graph/instantiation/internal/InstantiationHelper.java index 6dc237aa04..50409ea3da 100644 --- a/hibernate-core/src/main/java/org/hibernate/sql/results/graph/instantiation/internal/InstantiationHelper.java +++ b/hibernate-core/src/main/java/org/hibernate/sql/results/graph/instantiation/internal/InstantiationHelper.java @@ -6,10 +6,14 @@ */ package org.hibernate.sql.results.graph.instantiation.internal; +import org.hibernate.internal.util.beans.BeanInfoHelper; import org.hibernate.type.spi.TypeConfiguration; import org.jboss.logging.Logger; +import java.beans.BeanInfo; +import java.beans.PropertyDescriptor; import java.lang.reflect.Constructor; +import java.lang.reflect.Field; import java.lang.reflect.Type; import java.util.List; @@ -27,6 +31,40 @@ public class InstantiationHelper { // disallow direct instantiation } + public static boolean isInjectionCompatible(Class targetJavaType, List aliases, List> argTypes) { + return BeanInfoHelper.visitBeanInfo( + targetJavaType, + beanInfo -> { + for ( int i = 0; i < aliases.size(); i++ ) { + final String alias = aliases.get(i); + final Class argType = argTypes.get(i); + if ( !checkArgument( targetJavaType, beanInfo, alias, argType ) ) { + return false; + } + } + return true; + } + ); + } + + private static boolean checkArgument(Class targetJavaType, BeanInfo beanInfo, String alias, Class argType) { + for ( PropertyDescriptor propertyDescriptor : beanInfo.getPropertyDescriptors() ) { + if ( propertyMatches( alias, argType, propertyDescriptor ) ) { + return true; + } + } + return findField(targetJavaType, alias, argType) != null; + } + + public static boolean isConstructorCompatible(Class javaClass, List> argTypes, TypeConfiguration typeConfiguration) { + for ( Constructor constructor : javaClass.getDeclaredConstructors() ) { + if ( isConstructorCompatible( constructor, argTypes, typeConfiguration) ) { + return true; + } + } + return false; + } + public static boolean isConstructorCompatible( Constructor constructor, List> argumentTypes, @@ -58,4 +96,28 @@ public class InstantiationHelper { return false; } } + + static Field findField(Class declaringClass, String name, Class javaType) { + try { + final Field field = declaringClass.getDeclaredField( name ); + // field should never be null + if ( areAssignmentCompatible( field.getType(), javaType ) ) { + field.setAccessible( true ); + return field; + } + } + catch (NoSuchFieldException ignore) { + if ( declaringClass.getSuperclass() != null ) { + return findField( declaringClass.getSuperclass(), name, javaType ); + } + } + + return null; + } + + static boolean propertyMatches(String alias, Class argType, PropertyDescriptor propertyDescriptor) { + return alias.equals(propertyDescriptor.getName()) + && propertyDescriptor.getWriteMethod() != null + && areAssignmentCompatible( propertyDescriptor.getWriteMethod().getParameterTypes()[0], argType ); + } }