Skip to content

HHH-19383 improvements #10198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 22, 2025
Merged

HHH-19383 improvements #10198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6a367a
HHH-19383 add more tests
gavinking May 21, 2025
709180a
HHH-19383 partial fix to checking of constructor signature
gavinking May 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 28 additions & 18 deletions hibernate-core/src/main/java/org/hibernate/query/sql/internal/NativeQueryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
package org.hibernate.query.sql.internal;

import java.io.Serializable;
import java.lang.reflect.Constructor;
import java.time.Instant;
import java.util.Calendar;
import java.util.Collection;
Expand Down Expand Up @@ -355,32 +354,43 @@ private void checkResultType(Class<R> resultType, ResultSetMapping resultSetMapp
}
break;
default:
// The return type has to be a class with an appropriate constructor, i.e. one whose parameter types match
// the types of the result builders. If none such constructor is found, throw an IAE
// The return type has to be a class with an appropriate constructor,
// i.e. one whose parameter types match the types of the result builders.
// If no such constructor is found, throw an IAE
if ( !validConstructorFoundForResultType( resultType, resultSetMapping ) ) {
throw new IllegalArgumentException( "The declared return type for a multi-valued result set mapping should be Object[], Map, List, or Tuple" );
throw new IllegalArgumentException(
"The return type for a multivalued result set mapping should be Object[], Map, List, or Tuple"
+ " or it must have an appropriate constructor"
);
}
}
}
}

private boolean validConstructorFoundForResultType(Class<R> resultType, ResultSetMapping resultSetMapping) {
// Only 1 constructor with the right number of parameters is allowed (see NativeQueryConstructorTransformer)
Constructor<?> constructor = resultType.getConstructors()[0];
if ( constructor.getParameterCount() != resultSetMapping.getNumberOfResultBuilders() ) {
return false;
}
final List<ResultBuilder> resultBuilders = resultSetMapping.getResultBuilders();
Class<?>[] paramTypes = constructor.getParameterTypes();
for ( int i = 0; i < resultBuilders.size(); i++ ) {
if (
resultBuilders.get( i ).getJavaType() != ( paramTypes[i].isPrimitive() ?
getDescriptorByPrimitiveType(paramTypes[i] ).getWrapperClass() :
paramTypes[i]) ) {
return false;
// TODO: Only one constructor with the right number of parameters is allowed
// (see NativeQueryConstructorTransformer) so we should validate that
Copy link
Member

@jrenaat jrenaat May 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Wasn't I doing just that in my original code?
I don't quite understand why you would instead iterate all constructors, if clearly only exactly one with the right amt. of params is allowed ... ?
I removed the constructor iteration I originally also had when I saw there should only be one ...

Copy link
Member Author

@gavinking gavinking May 22, 2025

Choose a reason for hiding this comment

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

Well we have to skip over all the constructors with the wrong number of parameters until we get to one with the right number of parameters.

Copy link
Member

@jrenaat jrenaat May 23, 2025

Choose a reason for hiding this comment

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

IINM, that is not true; NativeQueryConstructorTransformershould allow only a constructor with the same number of params, else an InstantiationExceptionwould be thrown, so no need to iterate?

Copy link
Member Author

@gavinking gavinking May 23, 2025

Choose a reason for hiding this comment

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

That's just not correct. Look at the code in NativeQueryConstructorTransformer. It iterates over the constructors, looking for one with the right number of parameters, and ignores all the rest:

for ( final Constructor<?> candidate : resultClass.getDeclaredConstructors() ) {
	final Class<?>[] parameterTypes = candidate.getParameterTypes();
	if ( parameterTypes.length == elements.length ) {
		// found a candidate with the right number
		// of parameters

Copy link
Member

@jrenaat jrenaat May 23, 2025

Choose a reason for hiding this comment

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

Ok, I was mislead by the exception message ...
Maybe it's somewhat confusing since it's possible for a resultclass to have several constructors with the same number of params as needed, but with different param types, and then you'd still get that message?
I'm just annoyed by the fact I initially had a perfectly good implementation including constructor iteration, and then I took that out thinking it was not needed, only for the lead Hibernate guy to come in after me and slap me on the wrist by rewriting my code. Fucks up my self-esteem in so many ways ...

Copy link
Member Author

@gavinking gavinking May 23, 2025

Choose a reason for hiding this comment

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

it's possible for a resultclass to have several constructors with the same number of params as needed, but with different param types, and then you'd still get that message?

Correct. The potentially-ambiguous case is where you have multiple constructors with the same number of parameters but different types.

Fucks up my self-esteem in so many ways

I make mistakes all the time. I don't let it bother me.

outer: for ( var constructor : resultType.getConstructors() ) {
if ( constructor.getParameterCount() == resultSetMapping.getNumberOfResultBuilders() ) {
final var resultBuilders = resultSetMapping.getResultBuilders();
final var paramTypes = constructor.getParameterTypes();
for ( int i = 0; i < resultBuilders.size(); i++ ) {
if ( !constructorParameterMatches( resultBuilders.get( i ), paramTypes[i] ) ) {
continue outer;
}
}
return true;
}
}
return true;
return false;
}

private static boolean constructorParameterMatches(ResultBuilder resultBuilder, Class<?> paramType) {
final Class<?> parameterClass =
paramType.isPrimitive()
? getDescriptorByPrimitiveType( paramType ).getWrapperClass()
: paramType;
return resultBuilder.getJavaType() == parameterClass;
}

protected <T> void setTupleTransformerForResultType(Class<T> resultClass) {
Expand Down
166 changes: 137 additions & 29 deletions ...e-core/src/test/java/org/hibernate/orm/test/query/sql/NativeQueryResultCheckingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public void testForHHH19376(SessionFactoryScope scope) {
}

@Test
public void testOkMutateResultSetMappingWithString(SessionFactoryScope scope) {
public void testOneColumn(SessionFactoryScope scope) {
scope.inTransaction(
session ->
assertDoesNotThrow(
Expand All @@ -64,50 +64,139 @@ public void testOkMutateResultSetMappingWithString(SessionFactoryScope scope) {
.getResultList()
)
);
}

@Test
public void testNokMutateResultSetMappingWithString(SessionFactoryScope scope) {
scope.inTransaction(
session ->
assertThrows( IllegalArgumentException.class,
() -> session.createNativeQuery( "select title, isbn from Book", String.class )
.addScalar( "title", String.class )
() -> session.createNativeQuery( "select isbn from Book", Integer.class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> session.createNativeQuery( "select isbn from Book", Object[].class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> session.createNativeQuery( "select isbn from Book", Tuple.class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> session.createNativeQuery( "select isbn from Book", Map.class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> session.createNativeQuery( "select isbn from Book", Object.class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> session.createNativeQuery( "select isbn from Book" )
.addScalar( "isbn", String.class )
.getResultList()
)
);
}

@Test
public void testOkMutateResultSetMappingWithBook(SessionFactoryScope scope) {
public void testTwoStringColumns(SessionFactoryScope scope) {
scope.inTransaction(
session ->
assertThrows( IllegalArgumentException.class,
() -> session.createNativeQuery( "select name, isbn from Book", String.class )
.addScalar( "name", String.class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> session.createNativeQuery( "select id, name from Book", Book.class )
.addScalar( "id", Integer.class )
() -> session.createNativeQuery( "select name, isbn from Book", Object[].class )
.addScalar( "name", String.class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> session.createNativeQuery( "select name, isbn from Book", Tuple.class )
.addScalar( "name", String.class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> session.createNativeQuery( "select name, isbn from Book", Map.class )
.addScalar( "name", String.class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> session.createNativeQuery( "select name, isbn from Book", Object.class )
.addScalar( "name", String.class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> session.createNativeQuery( "select name, isbn from Book" )
.addScalar( "name", String.class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
}

@Test
public void testNokMutateResultSetMappingWithBook(SessionFactoryScope scope) {
scope.inTransaction(
session ->
assertThrows( IllegalArgumentException.class,
() -> session.createNativeQuery( "select title, isbn from Book", Book.class )
() -> session.createNativeQuery( "select name, isbn from Book", Book.class )
// this mapping doesn't have an appropriate constructor in Book, should throw error
.addScalar( "title", String.class )
.addScalar( "name", String.class )
.addScalar( "isbn", String.class )
.getResultList()
)
);
}

@Test
public void testMutateResultSetMappingWithObjectArray(SessionFactoryScope scope) {
public void testOkMutateResultSetMappingWithBook(SessionFactoryScope scope) {
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> session.createNativeQuery( "select id, name from Book", Book.class )
.addScalar( "id", Integer.class )
.addScalar( "name", String.class )
.getResultList()
)
);
}

@Test
public void testAllColumns(SessionFactoryScope scope) {
scope.inTransaction(
session ->
assertDoesNotThrow(
Expand All @@ -119,10 +208,6 @@ public void testMutateResultSetMappingWithObjectArray(SessionFactoryScope scope)
}
)
);
}

@Test
public void testMutateResultSetMappingWithTuple(SessionFactoryScope scope) {
scope.inTransaction(
session ->
assertDoesNotThrow(
Expand All @@ -134,10 +219,6 @@ public void testMutateResultSetMappingWithTuple(SessionFactoryScope scope) {
}
)
);
}

@Test
public void testMutateResultSetMappingWithMap(SessionFactoryScope scope) {
scope.inTransaction(
session ->
assertDoesNotThrow(
Expand All @@ -149,10 +230,28 @@ public void testMutateResultSetMappingWithMap(SessionFactoryScope scope) {
}
)
);
}

@Test
public void testMutateResultSetMappingWithList(SessionFactoryScope scope) {
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> {
session.createNativeQuery( "select * from Book", Object.class )
.addScalar( "id", Integer.class )
.addScalar( "name", String.class )
.getResultList();
}
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
() -> {
session.createNativeQuery( "select * from Book", Book.class )
.addScalar( "id", Integer.class )
.addScalar( "name", String.class )
.getResultList();
}
)
);
scope.inTransaction(
session ->
assertDoesNotThrow(
Expand All @@ -164,6 +263,15 @@ public void testMutateResultSetMappingWithList(SessionFactoryScope scope) {
}
)
);
scope.inTransaction(
session ->
assertThrows( IllegalArgumentException.class,
() -> session.createNativeQuery( "select * from Book", Book.class )
.addScalar( "isbn", String.class )
.addScalar( "name", String.class )
.getResultList()
)
);
}

@Test
Expand Down