HHH-16633 completely rework validation for parameters of @HQL query methods

tooling/metamodel-generator/src/main/java/org/hibernate/jpamodelgen/annotation/AnnotationMeta.java

@@ -22,8 +22,10 @@ import javax.lang.model.element.AnnotationValue;
 import javax.lang.model.element.Element;
 import java.util.List;
 
-import static java.util.Collections.emptySet;
+import static org.hibernate.jpamodelgen.util.TypeUtils.containsAnnotation;
-import static org.hibernate.jpamodelgen.util.TypeUtils.*;
+import static org.hibernate.jpamodelgen.util.TypeUtils.getAnnotationMirror;
+import static org.hibernate.jpamodelgen.util.TypeUtils.getAnnotationValue;
+import static org.hibernate.jpamodelgen.util.TypeUtils.getAnnotationValueRef;
 
 public abstract class AnnotationMeta implements Metamodel {
 
@@ -91,8 +93,7 @@ public abstract class AnnotationMeta implements Metamodel {
 					final SqmStatement<?> statement =
 							Validation.validate(
 									hql,
-									false, true,
+									true,
-									emptySet(), emptySet(),
 									// If we are in the scope of @CheckHQL, semantic errors in the
 									// query result in compilation errors. Otherwise, they only
 									// result in warnings, so we don't break working code.

tooling/metamodel-generator/src/main/java/org/hibernate/jpamodelgen/annotation/AnnotationMetaEntity.java

@@ -38,9 +38,11 @@ import org.hibernate.jpamodelgen.util.AccessTypeInformation;
 import org.hibernate.jpamodelgen.util.Constants;
 import org.hibernate.jpamodelgen.validation.ProcessorSessionFactory;
 import org.hibernate.jpamodelgen.validation.Validation;
+import org.hibernate.query.sqm.SqmExpressible;
+import org.hibernate.query.sqm.tree.SqmStatement;
+import org.hibernate.query.sqm.tree.expression.SqmParameter;
 
 import static java.util.Collections.emptyList;
-import static java.util.Collections.emptySet;
 import static java.util.stream.Collectors.toList;
 import static javax.lang.model.util.ElementFilter.fieldsIn;
 import static javax.lang.model.util.ElementFilter.methodsIn;
@@ -721,6 +723,7 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 				final String hql = (String) query;
 				final List<String> paramNames = parameterNames( method );
 				final List<String> paramTypes = parameterTypes( method );
+
 				final QueryMethod attribute =
 						new QueryMethod(
 								this,
@@ -737,21 +740,95 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 						);
 				putMember( attribute.getPropertyName() + paramTypes, attribute );
 
-				checkParameters( method, paramNames, paramTypes, mirror, value, hql );
 				if ( !isNative ) {
-	//				checkHqlSyntax( method, mirror, hql );
+					final SqmStatement<?> statement =
-					Validation.validate(
+							Validation.validate(
-							hql,
+									hql,
-							false, true,
+									true,
-							emptySet(), emptySet(),
+									new ErrorHandler( context, method, mirror, value, hql ),
-							new ErrorHandler(context, method, mirror, value, hql),
+									ProcessorSessionFactory.create( context.getProcessingEnvironment() )
-							ProcessorSessionFactory.create( context.getProcessingEnvironment() )
+							);
-					);
+					if ( statement != null ) {
+						for ( SqmParameter<?> param : statement.getSqmParameters() ) {
+							checkParameter( param, paramNames, paramTypes, method, mirror, value );
+						}
+					}
 				}
+				//TODO: for SQL queries check that there is a method parameter for every query parameter
+
+				// now check that the query has a parameter for every method parameter
+				checkParameters( method, paramNames, paramTypes, mirror, value, hql );
 			}
 		}
 	}
 
+	private void checkParameter(
+			SqmParameter<?> param, List<String> paramNames, List<String> paramTypes,
+			ExecutableElement method, AnnotationMirror mirror, AnnotationValue value) {
+		final SqmExpressible<?> expressible = param.getExpressible();
+		final String queryParamType = expressible == null ? "unknown" : expressible.getTypeName(); //getTypeName() can return "unknown"
+		if ( param.getName() != null ) {
+			final String name = param.getName();
+			int index = paramNames.indexOf( name );
+			if ( index < 0 ) {
+				context.message( method, mirror, value,
+						"missing method parameter for query parameter :" + name
+						+ " (add a parameter '" + queryParamType + ' ' + name + "' to '" + method.getSimpleName() + "')",
+						Diagnostic.Kind.ERROR );
+			}
+			else if ( !isLegalAssignment( paramTypes.get(index), queryParamType ) ) {
+				context.message( method, mirror, value,
+						"parameter matching query parameter :" + name + " has the wrong type"
+								+ " (change the method parameter type to '" + queryParamType + "')",
+						Diagnostic.Kind.ERROR );
+			}
+		}
+		else if ( param.getPosition() != null ) {
+			int position = param.getPosition();
+			if ( position > paramNames.size() ) {
+				context.message( method, mirror, value,
+						"missing method parameter for query parameter ?" + position
+								+ " (add a parameter of type '" + queryParamType + "' to '" + method.getSimpleName() + "')",
+						Diagnostic.Kind.ERROR );
+			}
+			else if ( !isLegalAssignment( paramTypes.get(position-1), queryParamType ) ) {
+				context.message( method, mirror, value,
+						"parameter matching query parameter ?" + position + " has the wrong type"
+								+ " (change the method parameter type to '" + queryParamType + "')",
+						Diagnostic.Kind.ERROR );
+			}
+		}
+	}
+
+	private static boolean isLegalAssignment(String argType, String paramType) {
+		return paramType.equals("unknown")
+			|| paramType.equals(argType)
+			|| paramType.equals(fromPrimitive(argType));
+	}
+
+	private static @Nullable String fromPrimitive(String argType) {
+		switch (argType) {
+			case "boolean":
+				return Boolean.class.getName();
+			case "char":
+				return Character.class.getName();
+			case "int":
+				return Integer.class.getName();
+			case "long":
+				return Long.class.getName();
+			case "short":
+				return Short.class.getName();
+			case "byte":
+				return Byte.class.getName();
+			case "float":
+				return Float.class.getName();
+			case "double":
+				return Double.class.getName();
+			default:
+				return null;
+		}
+	}
+
 	private static List<String> parameterTypes(ExecutableElement method) {
 		return method.getParameters().stream()
 				.map(param -> param.asType().toString())
@@ -783,8 +860,7 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 	}
 
 	private static boolean parameterIsMissing(String hql, int i, String param, String type) {
-		return !hql.contains(":" + param)
+		return !hql.matches(".*(:" + param + "|\\?" + i + ")\\b.*")
-			&& !hql.contains("?" + i)
 			&& !isPageParam(type)
 			&& !isOrderParam(type);
 	}

tooling/metamodel-generator/src/main/java/org/hibernate/jpamodelgen/annotation/QueryMethod.java

@@ -276,13 +276,13 @@ public class QueryMethod implements MetaAttribute {
 		return type.endsWith("...") ? stripped + "..." : stripped;
 	}
 
-	static boolean isPageParam(String ptype) {
+	static boolean isPageParam(String parameterType) {
-		return Page.class.getName().equals(ptype);
+		return Page.class.getName().equals(parameterType);
 	}
 
-	static boolean isOrderParam(String ptype) {
+	static boolean isOrderParam(String parameterType) {
-		return ptype.startsWith(Order.class.getName())
+		return parameterType.startsWith(Order.class.getName())
-			|| ptype.startsWith(List.class.getName() + "<" + Order.class.getName());
+			|| parameterType.startsWith(List.class.getName() + "<" + Order.class.getName());
 	}
 
 	private void unwrap(StringBuilder declaration, boolean unwrapped) {

tooling/metamodel-generator/src/main/java/org/hibernate/jpamodelgen/validation/Validation.java

@@ -9,7 +9,6 @@ package org.hibernate.jpamodelgen.validation;
 import org.antlr.v4.runtime.ANTLRErrorListener;
 import org.antlr.v4.runtime.BailErrorStrategy;
 import org.antlr.v4.runtime.DefaultErrorStrategy;
-import org.antlr.v4.runtime.Token;
 import org.antlr.v4.runtime.atn.PredictionMode;
 import org.antlr.v4.runtime.misc.ParseCancellationException;
 import org.checkerframework.checker.nullness.qual.Nullable;
@@ -26,13 +25,6 @@ import org.hibernate.query.sqm.TerminalPathException;
 import org.hibernate.query.sqm.tree.SqmStatement;
 import org.hibernate.type.descriptor.java.spi.JdbcTypeRecommendationException;
 
-import java.util.ArrayList;
-import java.util.List;
-import java.util.Set;
-
-import static java.lang.Character.isJavaIdentifierStart;
-import static java.lang.Integer.parseInt;
-import static java.util.stream.Stream.concat;
 
 /**
  * The entry point for HQL validation.
@@ -50,33 +42,22 @@ public class Validation {
 
 	public static @Nullable SqmStatement<?> validate(
 			String hql,
-			boolean checkParams, boolean checkTyping,
+			boolean checkTyping,
-			Set<Integer> setParameterLabels,
-			Set<String> setParameterNames,
 			Handler handler,
 			SessionFactoryImplementor factory) {
-		return validate( hql, checkParams, checkTyping, setParameterLabels, setParameterNames, handler, factory, 0 );
+		return validate( hql, checkTyping, handler, factory, 0 );
 	}
 
 	public static @Nullable SqmStatement<?> validate(
 			String hql,
-			boolean checkParams, boolean checkTyping,
+			boolean checkTyping,
-			Set<Integer> setParameterLabels,
-			Set<String> setParameterNames,
 			Handler handler,
 			SessionFactoryImplementor factory,
 			int errorOffset) {
-//		handler = new Filter(handler, errorOffset);
-
 		try {
 			final HqlParser.StatementContext statementContext = parseAndCheckSyntax( hql, handler );
 			if ( checkTyping && handler.getErrorCount() == 0 ) {
-				final SqmStatement<?> statement =
+				return checkTyping( hql, handler, factory, errorOffset, statementContext );
-						checkTyping( hql, handler, factory, errorOffset, statementContext );
-				if ( checkParams ) {
-					checkParameterBinding( hql, setParameterLabels, setParameterNames, handler, errorOffset );
-				}
-				return statement;
 			}
 		}
 		catch (Exception e) {
@@ -132,110 +113,4 @@ public class Validation {
 			return hqlParser.statement();
 		}
 	}
-
-	private static void checkParameterBinding(
-			String hql,
-			Set<Integer> setParameterLabels,
-			Set<String> setParameterNames,
-			Handler handler,
-			int errorOffset) {
-		try {
-			String unsetParams = null;
-			String notSet = null;
-			String parameters = null;
-			int start = -1;
-			int end = -1;
-			List<String> names = new ArrayList<>();
-			List<Integer> labels = new ArrayList<>();
-			final HqlLexer hqlLexer = HqlParseTreeBuilder.INSTANCE.buildHqlLexer( hql );
-			loop:
-			while (true) {
-				Token token = hqlLexer.nextToken();
-				int tokenType = token.getType();
-				switch (tokenType) {
-					case HqlLexer.EOF:
-						break loop;
-					case HqlLexer.QUESTION_MARK:
-					case HqlLexer.COLON:
-						Token next = hqlLexer.nextToken();
-						String text = next.getText();
-						switch (tokenType) {
-							case HqlLexer.COLON:
-								if ( !text.isEmpty()
-										&& isJavaIdentifierStart( text.codePointAt(0) ) ) {
-									names.add(text);
-									if ( setParameterNames.contains(text) ) {
-										continue;
-									}
-								}
-								else {
-									continue;
-								}
-								break;
-							case HqlLexer.QUESTION_MARK:
-								if ( next.getType() == HqlLexer.INTEGER_LITERAL ) {
-									int label;
-									try {
-										label = parseInt(text);
-									}
-									catch (NumberFormatException nfe) {
-										continue;
-									}
-									labels.add(label);
-									if ( setParameterLabels.contains(label) ) {
-										continue;
-									}
-								}
-								else {
-									continue;
-								}
-								break;
-							default:
-								continue;
-						}
-						parameters = unsetParams == null ? "Parameter " : "Parameters ";
-						notSet = unsetParams == null ? " is not set" : " are not set";
-						unsetParams = unsetParams == null ? "" : unsetParams + ", ";
-						unsetParams += token.getText() + text;
-						if (start == -1) {
-							start = token.getCharPositionInLine(); //TODO: wrong for multiline query strings!
-						}
-						end = token.getCharPositionInLine() + text.length() + 1;
-						break;
-				}
-			}
-			if ( unsetParams != null ) {
-				handler.warn( start-errorOffset+1, end-errorOffset, parameters + unsetParams + notSet );
-			}
-
-			setParameterNames.removeAll(names);
-			setParameterLabels.removeAll(labels);
-
-			reportMissingParams( setParameterLabels, setParameterNames, handler );
-		}
-		finally {
-			setParameterNames.clear();
-			setParameterLabels.clear();
-		}
-	}
-
-	private static void reportMissingParams(Set<Integer> setParameterLabels, Set<String> setParameterNames, Handler handler) {
-		final int count = setParameterNames.size() + setParameterLabels.size();
-		if (count > 0) {
-			final String missingParams =
-					concat( setParameterNames.stream().map(name -> ":" + name),
-							setParameterLabels.stream().map(label -> "?" + label) )
-							.reduce((x, y) -> x + ", " + y)
-							.orElse(null);
-			final String params =
-					count == 1 ?
-							"Parameter " :
-							"Parameters ";
-			final String notOccur =
-					count == 1 ?
-							" does not occur in the query" :
-							" do not occur in the query";
-			handler.warn(0, 0, params + missingParams + notOccur);
-		}
-	}
 }