improved reasoning around nullability of finder method parameters

documentation/src/main/asciidoc/introduction/Introduction.adoc

@@ -100,9 +100,9 @@ But it's something to watch out for.
 
 If you're completely new to Hibernate and JPA, you might already be wondering how the persistence-related code is structured.
 
-Well, typically, your persistence-related code comes in two layers:
+Well, typically, our persistence-related code comes in two layers:
 
-. a representation of your data model in Java, which takes the form of a set of annotated entity classes, and
+. a representation of our data model in Java, which takes the form of a set of annotated entity classes, and
 . a larger number of functions which interact with Hibernate's APIs to perform the persistence operations associated with your various transactions.
 
 The first part, the data or "domain" model, is usually easier to write, but doing a great and very clean job of it will strongly affect your success in the second part.

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

@@ -12,7 +12,6 @@ import org.hibernate.jpamodelgen.util.Constants;
 import java.util.List;
 import java.util.Locale;
 
-import static org.hibernate.internal.util.StringHelper.qualifier;
 import static org.hibernate.jpamodelgen.util.StringUtil.getUpperUnderscoreCaseFromLowerCamelCase;
 
 /**
@@ -57,8 +56,6 @@ public abstract class AbstractFinderMethod extends AbstractQueryMethod  {
 		return entity;
 	}
 
-	abstract boolean isId();
-
 	@Override
 	public String getAttributeNameDeclarationString() {
 		return new StringBuilder()
@@ -131,6 +128,11 @@ public abstract class AbstractFinderMethod extends AbstractQueryMethod  {
 				.append("\n **/\n");
 	}
 
+	String qualifier(String name) {
+		final int index = name.indexOf('$');
+		return index > 0 ? name.substring(0, index) : name;
+	}
+
 	void unwrapSession(StringBuilder declaration) {
 		if ( isUsingEntityManager() ) {
 			declaration

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

@@ -14,6 +14,7 @@ import org.hibernate.jpamodelgen.util.Constants;
 import java.util.List;
 
 import static org.hibernate.jpamodelgen.util.Constants.SESSION_TYPES;
+import static org.hibernate.jpamodelgen.util.TypeUtils.isPrimitive;
 
 /**
  * @author Gavin King
@@ -64,7 +65,7 @@ public abstract class AbstractQueryMethod implements MetaAttribute {
 		return methodName;
 	}
 
-	abstract boolean isId();
+	abstract boolean isNullable(int index);
 
 	String parameterList() {
 		return paramTypes.stream()
@@ -90,7 +91,8 @@ public abstract class AbstractQueryMethod implements MetaAttribute {
 						.append(", ");
 			}
 			final String paramType = paramTypes.get(i);
-			if ( isId() || isSessionParameter(paramType) ) {
+			if ( !isNullable(i) && !isPrimitive(paramType)
+					|| isSessionParameter(paramType) ) {
 				notNull( declaration );
 			}
 			declaration

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

@@ -44,6 +44,7 @@ import org.hibernate.query.sqm.tree.SqmStatement;
 import org.hibernate.query.sqm.tree.expression.SqmParameter;
 
 import static java.beans.Introspector.decapitalize;
+import static java.lang.Boolean.FALSE;
 import static java.util.Collections.emptyList;
 import static java.util.stream.Collectors.toList;
 import static javax.lang.model.util.ElementFilter.fieldsIn;
@@ -545,7 +546,7 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 						containerType == null ? null : containerType.toString(),
 						paramNames,
 						paramTypes,
-						false,
+						parameterNullability(method, entity),
 						dao,
 						sessionType[0],
 						sessionType[1],
@@ -603,6 +604,7 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 							returnType.toString(),
 							paramNames,
 							paramTypes,
+							parameterNullability(method, entity),
 							dao,
 							sessionType[0],
 							sessionType[1],
@@ -620,7 +622,7 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 							null,
 							paramNames,
 							paramTypes,
-							false,
+							parameterNullability(method, entity),
 							dao,
 							sessionType[0],
 							sessionType[1],
@@ -669,6 +671,7 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 									returnType.toString(),
 									paramNames,
 									paramTypes,
+									parameterNullability(method, entity),
 									dao,
 									sessionType[0],
 									sessionType[1],
@@ -686,7 +689,7 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 									null,
 									paramNames,
 									paramTypes,
-									fieldType == FieldType.ID,
+									parameterNullability(method, entity),
 									dao,
 									sessionType[0],
 									sessionType[1],
@@ -746,22 +749,29 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 		return count;
 	}
 
-	private @Nullable FieldType validateFinderParameter(TypeElement entity, VariableElement param) {
+	private @Nullable FieldType validateFinderParameter(TypeElement entityType, VariableElement param) {
-		final AccessType accessType = getAccessType(entity);
+		final Element member = memberMatchingParameter(entityType, param);
-		for ( Element member : entity.getEnclosedElements() ) {
+		if ( member != null) {
-			if ( fieldMatchesParameter( entity, param, member, accessType ) ) {
+			final String memberType = memberType( member ).toString();
-				if ( containsAnnotation( member, Constants.ID, Constants.EMBEDDED_ID ) ) {
+			final String paramType = param.asType().toString();
-					return FieldType.ID;
+			if ( !isLegalAssignment( paramType, memberType ) ) {
-				}
+				context.message( param,
-				else if ( containsAnnotation( member, Constants.NATURAL_ID ) ) {
+						"matching field has type '" + memberType
-					return FieldType.NATURAL_ID;
+								+ "' in entity class '" + entityType + "'",
-				}
+						Diagnostic.Kind.ERROR );
-				else {
+			}
-					return FieldType.BASIC;
+
-				}
+			if ( containsAnnotation( member, Constants.ID, Constants.EMBEDDED_ID ) ) {
+				return FieldType.ID;
+			}
+			else if ( containsAnnotation( member, Constants.NATURAL_ID ) ) {
+				return FieldType.NATURAL_ID;
+			}
+			else {
+				return FieldType.BASIC;
 			}
 		}
-		final AnnotationMirror idClass = getAnnotationMirror( entity, Constants.ID_CLASS );
+		final AnnotationMirror idClass = getAnnotationMirror( entityType, Constants.ID_CLASS );
 		if ( idClass != null ) {
 			final Object value = getAnnotationValue( idClass, "value" );
 			if ( value instanceof TypeMirror ) {
@@ -773,11 +783,16 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 		context.message( param,
 				"no matching field named '"
 						+ param.getSimpleName().toString().replace('$', '.')
-						+ "' in entity class '" + entity + "'",
+						+ "' in entity class '" + entityType + "'",
 				Diagnostic.Kind.ERROR );
 		return null;
 	}
 
+	private boolean finderParameterNullable(TypeElement entity, VariableElement param) {
+		final Element member = memberMatchingParameter(entity, param);
+		return member == null || isNullable(member);
+	}
+
 	private AccessType getAccessType(TypeElement entity) {
 		final String entityClassName = entity.getQualifiedName().toString();
 		determineAccessTypeForHierarchy(entity, context );
@@ -794,67 +809,66 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 		}
 	}
 
-	private boolean fieldMatchesParameter(TypeElement entityType, VariableElement param, Element member, AccessType accessType) {
+	private @Nullable Element memberMatchingParameter(TypeElement entityType, VariableElement param) {
 		final StringTokenizer tokens = new StringTokenizer( param.getSimpleName().toString(), "$" );
-		return fieldMatchesParameter( entityType, param, member, accessType, tokens, tokens.nextToken() );
+		return memberMatchingParameter( entityType, param, tokens );
 	}
 
-	private boolean fieldMatchesParameter(
+	private @Nullable Element memberMatchingParameter(TypeElement entityType, VariableElement param, StringTokenizer tokens) {
+		final AccessType accessType = getAccessType(entityType);
+		final String nextToken = tokens.nextToken();
+		for ( Element member : entityType.getEnclosedElements() ) {
+			final Element match =
+					memberMatchingParameter(entityType, param, member, accessType, tokens, nextToken);
+			if ( match != null ) {
+				return match;
+			}
+		}
+		return null;
+	}
+
+	private @Nullable Element memberMatchingParameter(
 			TypeElement entityType,
 			VariableElement param,
-			Element member,
+			Element candidate,
 			AccessType accessType,
 			StringTokenizer tokens,
 			String token) {
-		final Name memberName = member.getSimpleName();
+		final Name memberName = candidate.getSimpleName();
 		final TypeMirror type;
-		if ( accessType == AccessType.FIELD && member.getKind() == ElementKind.FIELD ) {
+		if ( accessType == AccessType.FIELD && candidate.getKind() == ElementKind.FIELD ) {
 			if ( !fieldMatches(token, memberName) ) {
-				return false;
+				return null;
 			}
 			else {
-				type = member.asType();
+				type = candidate.asType();
 			}
 		}
-		else if ( accessType == AccessType.PROPERTY && member.getKind() == ElementKind.METHOD ) {
+		else if ( accessType == AccessType.PROPERTY && candidate.getKind() == ElementKind.METHOD ) {
 			if ( !getterMatches(token, memberName) ) {
-				return false;
+				return null;
 			}
 			else {
-				final ExecutableElement method = (ExecutableElement) member;
+				final ExecutableElement method = (ExecutableElement) candidate;
 				type = method.getReturnType();
 			}
 		}
 		else {
-			return false;
+			return null;
 		}
 
 		if ( tokens.hasMoreTokens() ) {
-			final String nextToken = tokens.nextToken();
 			if ( type.getKind() == TypeKind.DECLARED ) {
 				final DeclaredType declaredType = (DeclaredType) type;
 				final TypeElement memberType = (TypeElement) declaredType.asElement();
-				memberTypes.put( qualify(entityType.getQualifiedName().toString(), memberName.toString()),
+				memberTypes.put( qualify( entityType.getQualifiedName().toString(), memberName.toString() ),
-						memberType.getQualifiedName().toString()  );
+						memberType.getQualifiedName().toString() );
-				final AccessType memberAccessType = getAccessType(memberType);
+				return memberMatchingParameter( memberType, param, tokens );
-				for ( Element entityMember : memberType.getEnclosedElements() ) {
-					if ( fieldMatchesParameter(memberType, param, entityMember, memberAccessType, tokens, nextToken) ) {
-						return true;
-					}
-				}
 			}
-			return false;
+			return null;
 		}
 		else {
-			final String memberType = memberType( member ).toString();
+			return candidate;
-			final String paramType = param.asType().toString();
-			if ( !isLegalAssignment( paramType, memberType ) ) {
-				context.message( param,
-						"matching field has type '" + memberType
-								+ "' in entity class '" + entityType + "'",
-						Diagnostic.Kind.ERROR );
-			}
-			return true;
 		}
 	}
 
@@ -996,6 +1010,12 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 		}
 	}
 
+	private List<Boolean> parameterNullability(ExecutableElement method, TypeElement entity) {
+		return method.getParameters().stream()
+				.map(param -> finderParameterNullable(entity, param))
+				.collect(toList());
+	}
+
 	private static List<String> parameterTypes(ExecutableElement method) {
 		return method.getParameters().stream()
 				.map(param -> param.asType().toString())
@@ -1008,6 +1028,39 @@ public class AnnotationMetaEntity extends AnnotationMeta {
 				.collect(toList());
 	}
 
+	private static boolean isNullable(Element member) {
+		switch ( member.getKind() ) {
+			case METHOD:
+				final ExecutableElement method = (ExecutableElement) member;
+				if ( method.getReturnType().getKind().isPrimitive() ) {
+					return false;
+				}
+			case FIELD:
+				if ( member.asType().getKind().isPrimitive() ) {
+					return false;
+				}
+		}
+		boolean nullable = true;
+		for ( AnnotationMirror mirror : member.getAnnotationMirrors() ) {
+			final TypeElement annotationType = (TypeElement) mirror.getAnnotationType().asElement();
+			final Name name = annotationType.getQualifiedName();
+			if ( name.contentEquals(Constants.ID) ) {
+				nullable = false;
+			}
+			if ( name.contentEquals("jakarta.validation.constraints.NotNull")) {
+				nullable = false;
+			}
+			if ( name.contentEquals(Constants.BASIC)
+					|| name.contentEquals(Constants.MANY_TO_ONE)
+					|| name.contentEquals(Constants.ONE_TO_ONE)) {
+				if ( FALSE.equals( getAnnotationValue(mirror, "optional") ) ) {
+					nullable = false;
+				}
+			}
+		}
+		return nullable;
+	}
+
 	private void checkParameters(
 			ExecutableElement method,
 			List<String> paramNames, List<String> paramTypes,

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

@@ -10,23 +10,24 @@ import org.checkerframework.checker.nullness.qual.Nullable;
 import org.hibernate.jpamodelgen.util.Constants;
 
 import java.util.List;
-import java.util.Set;
 import java.util.StringTokenizer;
 
+import static org.hibernate.jpamodelgen.util.TypeUtils.isPrimitive;
+
 /**
  * @author Gavin King
  */
 public class CriteriaFinderMethod extends AbstractFinderMethod {
 
 	private final @Nullable String containerType;
-	private final boolean isId;
+	private final List<Boolean> paramNullability;
 
 	public CriteriaFinderMethod(
 			AnnotationMetaEntity annotationMetaEntity,
 			String methodName, String entity,
 			@Nullable String containerType,
 			List<String> paramNames, List<String> paramTypes,
-			boolean isId,
+			List<Boolean> paramNullability,
 			boolean belongsToDao,
 			String sessionType,
 			String sessionName,
@@ -35,12 +36,12 @@ public class CriteriaFinderMethod extends AbstractFinderMethod {
 		super( annotationMetaEntity, methodName, entity, belongsToDao, sessionType, sessionName, fetchProfiles,
 				paramNames, paramTypes, addNonnullAnnotation );
 		this.containerType = containerType;
-		this.isId = isId;
+		this.paramNullability = paramNullability;
 	}
 
 	@Override
-	public boolean isId() {
+	public boolean isNullable(int index) {
-		return isId;
+		return paramNullability.get(index);
 	}
 
 	@Override
@@ -55,11 +56,17 @@ public class CriteriaFinderMethod extends AbstractFinderMethod {
 		parameters( paramTypes, declaration );
 		declaration
 				.append(" {");
-		if ( isId ) {
+		for ( int i = 0; i< paramNames.size(); i++ ) {
-			declaration
+			final String paramName = paramNames.get(i);
-					.append("\n\tif (")
+			final String paramType = paramTypes.get(i);
-					.append(paramNames.get(0))
+			if ( !isNullable(i) && !isPrimitive(paramType) ) {
-					.append(" == null) throw new IllegalArgumentException(\"Null identifier\");");
+				declaration
+						.append("\n\tif (")
+						.append(paramName)
+						.append(" == null) throw new IllegalArgumentException(\"Null \" + ")
+						.append(paramName)
+						.append(");");
+			}
 		}
 		declaration
 				.append("\n\tvar builder = ")
@@ -89,7 +96,7 @@ public class CriteriaFinderMethod extends AbstractFinderMethod {
 				}
 				declaration
 						.append("\n\t\t\t");
-				if ( !isId && !isPrimitive(paramType) ) { //TODO: check the entity to see if it's @Basic(optional=false)
+				if ( isNullable(i) && !isPrimitive(paramType) ) {
 					declaration
 							.append(paramName)
 							.append("==null")
@@ -162,13 +169,6 @@ public class CriteriaFinderMethod extends AbstractFinderMethod {
 		}
 	}
 
-	private static boolean isPrimitive(String paramType) {
-		return PRIMITIVE_TYPES.contains( paramType );
-	}
-
-	private static final Set<String> PRIMITIVE_TYPES =
-			Set.of("boolean", "char", "long", "int", "short", "byte", "double", "float");
-
 	private StringBuilder returnType() {
 		StringBuilder type = new StringBuilder();
 		boolean returnsUni = isReactive()

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

@@ -39,8 +39,8 @@ public class IdFinderMethod extends AbstractFinderMethod {
 	}
 
 	@Override
-	boolean isId() {
+	boolean isNullable(int index) {
-		return true;
+		return false;
 	}
 
 	@Override

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

@@ -13,10 +13,13 @@ import java.util.List;
  */
 public class NaturalIdFinderMethod extends AbstractFinderMethod {
 
+	private final List<Boolean> paramNullability;
+
 	public NaturalIdFinderMethod(
 			AnnotationMetaEntity annotationMetaEntity,
 			String methodName, String entity,
 			List<String> paramNames, List<String> paramTypes,
+			List<Boolean> paramNullability,
 			boolean belongsToDao,
 			String sessionType,
 			String sessionName,
@@ -24,12 +27,13 @@ public class NaturalIdFinderMethod extends AbstractFinderMethod {
 			boolean addNonnullAnnotation) {
 		super( annotationMetaEntity, methodName, entity, belongsToDao, sessionType, sessionName, fetchProfiles,
 				paramNames, paramTypes, addNonnullAnnotation );
+		this.paramNullability = paramNullability;
 	}
 
 	@Override
-	boolean isId() {
+	boolean isNullable(int index) {
 		// natural ids can be null
-		return false;
+		return paramNullability.get(index);
 	}
 
 	@Override

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

@@ -63,8 +63,8 @@ public class QueryMethod extends AbstractQueryMethod {
 	}
 
 	@Override
-	boolean isId() {
+	boolean isNullable(int index) {
-		return false;
+		return true;
 	}
 
 	@Override

tooling/metamodel-generator/src/main/java/org/hibernate/jpamodelgen/util/TypeUtils.java

@@ -493,4 +493,11 @@ public final class TypeUtils {
 			}
 		}
 	}
+
+	public static boolean isPrimitive(String paramType) {
+		return PRIMITIVE_TYPES.contains( paramType );
+	}
+
+	public static final Set<String> PRIMITIVE_TYPES =
+			Set.of("boolean", "char", "long", "int", "short", "byte", "double", "float");
 }