Skip to content

Conversation

marko-bekhta
Copy link
Member

https://hibernate.atlassian.net/browse/HHH-19629
https://hibernate.atlassian.net/browse/HHH-19630

cc: @cigaly


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.


@marko-bekhta marko-bekhta requested a review from gavinking July 21, 2025 13:24
@cigaly
Copy link
Contributor

cigaly commented Jul 21, 2025

It is working, with that comment that parameter will be annotated with two Nonnull annotations, but this will not cause any error. And if some purist have problems with that, (s)he can fix it.

@cigaly
Copy link
Contributor

cigaly commented Jul 21, 2025

And if you really want some nitpicking, maybe you can (in typeAsString) move

result = type.toString();

into else branch (when type is DeclaredType result this is redundant call).
Also, you may consider to use collect( Collectors.joining( ",", "<", ">" ) ) instead of concatenating prefix and suffix in separate statements.
Of course, neither of those changes will make big difference.

@marko-bekhta marko-bekhta force-pushed the fix/HHH-19629-Hibernate-Processor-may-fail-when-repository-method-parameter-has-more-than-one-annotation branch from f4a1451 to 774bbfb Compare July 21, 2025 15:00
String result;
if ( type instanceof DeclaredType dt ) {
// get the "fqcn" without any type arguments
result = dt.asElement().toString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been avoiding the use of toString() for this, since the Javadoc doesn't clearly say it's the qualified name, and there are more things than javac.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is, I would prefer to see TypeElement.getQualifiedName() here.

It's obnoxious that there is no DeclaredType.getQualifiedName() method, and that DeclaredType.getElement() doesn't return a TypeElement.

@marko-bekhta marko-bekhta force-pushed the fix/HHH-19629-Hibernate-Processor-may-fail-when-repository-method-parameter-has-more-than-one-annotation branch from 774bbfb to c345a55 Compare July 22, 2025 09:13
Comment on lines +3396 to +3411
if ( type instanceof DeclaredType dt && dt.asElement() instanceof TypeElement te ) {
// get the "fqcn" without any type arguments
String result = te.getQualifiedName().toString();
// add the < ? ,? ....> as necessary:
if ( !dt.getTypeArguments().isEmpty() ) {
result += dt.getTypeArguments().stream()
.map( arg -> typeAsString( arg, true ) )
.collect( Collectors.joining( ", ", "<", ">" ) );
}
if ( includeAnnotations ) {
for ( AnnotationMirror annotation : type.getAnnotationMirrors() ) {
result = annotation.toString() + ' ' + result;
}
}
return result;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe will be better to use StringBuilder. I guess that this will reduce amount of job for garbage collector.

Suggested change
if ( type instanceof DeclaredType dt && dt.asElement() instanceof TypeElement te ) {
// get the "fqcn" without any type arguments
String result = te.getQualifiedName().toString();
// add the < ? ,? ....> as necessary:
if ( !dt.getTypeArguments().isEmpty() ) {
result += dt.getTypeArguments().stream()
.map( arg -> typeAsString( arg, true ) )
.collect( Collectors.joining( ", ", "<", ">" ) );
}
if ( includeAnnotations ) {
for ( AnnotationMirror annotation : type.getAnnotationMirrors() ) {
result = annotation.toString() + ' ' + result;
}
}
return result;
}
if ( type instanceof DeclaredType dt && dt.asElement() instanceof TypeElement te ) {
final StringBuilder sb = new StringBuilder();
if ( includeAnnotations ) {
for ( AnnotationMirror annotation : type.getAnnotationMirrors() ) {
sb.append( annotation.toString() ).append( ' ' );
}
}
// get the "fqcn" without any type arguments
sb.append( te.getQualifiedName().toString() );
// add the < ? ,? ....> as necessary:
if ( !dt.getTypeArguments().isEmpty() ) {
sb.append( '<' );
dt.getTypeArguments()
.forEach( arg -> sb.append( typeAsString( arg, true ) ) );
sb.append( '>' );
}
return sb.toString();
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree.

@@ -2452,7 +2453,7 @@ private void createSingleParameterFinder(
new CriteriaFinderMethod(
this, method,
methodName,
returnType.toString(),
typeAsString( returnType, false ),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@marko-bekhta there are three places in AnnotationMetaEntity where this constructor is called. I believe this change should be replicated to all three invocations, do you agree?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants