From a3a9e8a2b5dfe4e4c87039665406e2de6e7ad040 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Thu, 23 Nov 2023 13:25:12 +0100 Subject: [PATCH 1/3] [#1792] Add new HibernateException for when there are no results It should get thrown when a user call `.getSingleResultOrNull()` but the SQL query doesn't return any result. For example, when creating a stored procedure in PosgtreSQL --- .../src/main/java/org/hibernate/reactive/logging/impl/Log.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/logging/impl/Log.java b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/logging/impl/Log.java index 1d1324652..72760a26b 100644 --- a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/logging/impl/Log.java +++ b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/logging/impl/Log.java @@ -255,6 +255,9 @@ public interface Log extends BasicLogger { @Message(id = 79, value = "The configuration property '%1$s' was not provided, or is in invalid format. This is required when using the default DefaultSqlClientPool: either provide the configuration setting or integrate with a different SqlClientPool implementation") HibernateException blankConnectionString(String property); + @Message(id = 80, value = "No results were returned by the query (you can try running it with '.executeUpdate()'): %1$s") + HibernateException noResultException(String sql); + // Same method that exists in CoreMessageLogger @LogMessage(level = WARN) @Message(id = 104, value = "firstResult/maxResults specified with collection fetch; applying in memory!" ) From d6699f1eb09b2d154b63f10fe2b9980ab0ea3604 Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Thu, 23 Nov 2023 11:48:51 +0100 Subject: [PATCH 2/3] [#1792] Throw an exception if there is no result When running a native query that doesn't return any result (for example, the creation of a stored procedure), `io.vertx.sqlclient.SqlResult#columnsNames` will be null. This commit doesn two things: 1. add null check to avoid a NPE if `.columnsNames` returns null; 2. Throw an HibernateException if the user call getSingleResultOrNull but the query doesn't return any result. Note that we could return null in this situation, but we've decided to be consistent with Hibernate ORM, where the JDBC driver will throw the following exception (with PostgreSQL at least): ``` Caused by: org.postgresql.util.PSQLException: No results were returned by the query. at org.postgresql.jdbc.PgPreparedStatement.executeQuery(PgPreparedStatement.java:135) at org.hibernate.sql.results.jdbc.internal.DeferredResultSetAccess.executeQuery(DeferredResultSetAccess.java:240) ``` --- .../adaptor/impl/ResultSetAdaptor.java | 29 +++++++--- .../ReactiveDeferredResultSetAccess.java | 53 ++++++++++++++----- 2 files changed, 62 insertions(+), 20 deletions(-) diff --git a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/adaptor/impl/ResultSetAdaptor.java b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/adaptor/impl/ResultSetAdaptor.java index 24f3f5b9d..2a3b12c07 100644 --- a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/adaptor/impl/ResultSetAdaptor.java +++ b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/adaptor/impl/ResultSetAdaptor.java @@ -28,9 +28,9 @@ import java.time.LocalDateTime; import java.time.LocalTime; import java.util.Calendar; +import java.util.List; import java.util.Map; import java.util.NoSuchElementException; -import java.util.Objects; import java.util.function.Function; import org.hibernate.engine.jdbc.BlobProxy; @@ -42,6 +42,9 @@ import io.vertx.sqlclient.RowSet; import io.vertx.sqlclient.desc.ColumnDescriptor; +import static java.util.Collections.emptyList; +import static java.util.Objects.requireNonNull; + /** * An adaptor that allows Hibernate core code which expects a JDBC * {@code ResultSet} to read values from Vert.x's {@code RowSet}. @@ -54,6 +57,7 @@ public class ResultSetAdaptor implements ResultSet { private boolean wasNull; public ResultSetAdaptor(RowSet rows) { + requireNonNull( rows ); this.iterator = rows.iterator(); this.rows = rows; } @@ -199,15 +203,26 @@ public String getString(String columnLabel) { } private T caseInsensitiveGet(String columnLabel, Function produce) { - for ( String columnName : rows.columnsNames() ) { + for ( String columnName : getColumnsNames() ) { if ( columnName.equalsIgnoreCase( columnLabel ) ) { return produce.apply( columnName ); } } + // Same error thrown by io.vertx.sqlclient.Row when it doesn't find the label throw new NoSuchElementException( "Column " + columnLabel + " does not exist" ); } + /** + * rows.columnsNames() might return null for some databases. + * For example, when creating a stored procedure in PostgreSQL using a native query. + * + * @return A list of column names or an empty list. + */ + private List getColumnsNames() { + return rows.columnsNames() == null ? emptyList() : rows.columnsNames(); + } + @Override public boolean getBoolean(String columnLabel) { try { @@ -368,7 +383,7 @@ public ResultSetMetaData getMetaData() { return new ResultSetMetaData() { @Override public int getColumnCount() { - return rows.columnsNames().size(); + return getColumnsNames() == null ? 0 : getColumnsNames().size(); } @Override @@ -379,12 +394,12 @@ public int getColumnType(int column) { @Override public String getColumnLabel(int column) { - return rows.columnsNames().get( column - 1 ); + return getColumnsNames().get( column - 1 ); } @Override public String getColumnName(int column) { - return rows.columnsNames().get( column - 1 ); + return getColumnsNames().get( column - 1 ); } @Override @@ -502,7 +517,7 @@ public Object getObject(String columnLabel) { public int findColumn(String columnLabel) { // JDBC parameters index start from 1 int index = 1; - for ( String column : rows.columnsNames() ) { + for ( String column : getColumnsNames() ) { // Some dbs, like Oracle and Db2, return the column names always in uppercase if ( column.equalsIgnoreCase( columnLabel ) ) { return index; @@ -865,7 +880,7 @@ private static class RowIdAdaptor implements RowId { private final Buffer buffer; private RowIdAdaptor(Buffer buffer) { - Objects.requireNonNull( buffer ); + requireNonNull( buffer ); this.buffer = buffer; } diff --git a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/sql/results/internal/ReactiveDeferredResultSetAccess.java b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/sql/results/internal/ReactiveDeferredResultSetAccess.java index 88fd2346e..0ae26c795 100644 --- a/hibernate-reactive-core/src/main/java/org/hibernate/reactive/sql/results/internal/ReactiveDeferredResultSetAccess.java +++ b/hibernate-reactive-core/src/main/java/org/hibernate/reactive/sql/results/internal/ReactiveDeferredResultSetAccess.java @@ -10,6 +10,7 @@ import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; +import java.util.concurrent.CompletionException; import java.util.concurrent.CompletionStage; import java.util.function.Function; @@ -24,6 +25,7 @@ import org.hibernate.reactive.pool.ReactiveConnection; import org.hibernate.reactive.pool.impl.Parameters; import org.hibernate.reactive.session.ReactiveConnectionSupplier; +import org.hibernate.reactive.util.impl.CompletionStages; import org.hibernate.resource.jdbc.spi.LogicalConnectionImplementor; import org.hibernate.sql.exec.spi.ExecutionContext; import org.hibernate.sql.exec.spi.JdbcOperationQuerySelect; @@ -41,7 +43,6 @@ public class ReactiveDeferredResultSetAccess extends DeferredResultSetAccess imp private static final Log LOG = LoggerFactory.make( Log.class, MethodHandles.lookup() ); private final SqlStatementLogger sqlStatementLogger; - private final Function statementCreator; private final ExecutionContext executionContext; @@ -59,7 +60,6 @@ public ReactiveDeferredResultSetAccess( super( jdbcSelect, jdbcParameterBindings, executionContext, statementCreator ); this.executionContext = executionContext; this.sqlStatementLogger = executionContext.getSession().getJdbcServices().getSqlStatementLogger(); - this.statementCreator = statementCreator; } @Override @@ -159,17 +159,33 @@ private CompletionStage executeQuery() { eventListenerManager.jdbcExecuteStatementStart(); return connection() .selectJdbc( sql, parameters ) + .thenCompose( this::validateResultSet ) .whenComplete( (resultSet, throwable) -> { // FIXME: I don't know if this event makes sense for Vert.x eventListenerManager.jdbcExecuteStatementEnd(); sqlStatementLogger.logSlowQuery( getFinalSql(), executeStartNanos ); } ) .thenCompose( this::reactiveSkipRows ) - .handle( this::convertException ); + .handle( CompletionStages::handle ) + .thenCompose( handler -> handler.hasFailed() + ? convertException( resultSet, handler.getThrowable() ) + : handler.getResultAsCompletionStage() + ); } ) .whenComplete( (o, throwable) -> logicalConnection.afterStatement() ); } + private CompletionStage validateResultSet(ResultSet resultSet) { + try { + return resultSet.getMetaData().getColumnCount() == 0 + ? failedFuture( LOG.noResultException( getFinalSql() ) ) + : completedFuture( resultSet ); + } + catch (SQLException e) { + throw new RuntimeException( e ); + } + } + private ResultSet saveResultSet(ResultSet resultSet) { this.resultSet = resultSet; return saveColumnCount( resultSet ); @@ -185,18 +201,29 @@ private ReactiveConnection connection() { return ( (ReactiveConnectionSupplier) executionContext.getSession() ).getReactiveConnection(); } - private ResultSet convertException(ResultSet resultSet, Throwable throwable) { - // FIXME: Vert.x will probably throw another exception. Check this. - if ( throwable instanceof SQLException) { - throw executionContext.getSession().getJdbcServices() - .getSqlExceptionHelper() - // FIXME: Add this to the logger? - .convert( (SQLException) throwable, "Exception executing SQL [" + getFinalSql() + "]" ); - } + private CompletionStage convertException(T object, Throwable throwable) { if ( throwable != null ) { - throw new HibernateException( throwable ); + Throwable cause = throwable; + if ( throwable instanceof CompletionException ) { + cause = throwable.getCause(); + } + // I doubt this is ever going to happen because Vert.x is not going to throw an SQLException + if ( cause instanceof SQLException ) { + return failedFuture( executionContext + .getSession().getJdbcServices() + .getSqlExceptionHelper() + .convert( + (SQLException) cause, + "Exception executing SQL [" + getFinalSql() + "]" + ) + ); + } + if ( cause instanceof HibernateException ) { + return failedFuture( cause ); + } + return failedFuture( new HibernateException( cause ) ); } - return resultSet; + return completedFuture( object ); } private CompletionStage reactiveSkipRows(ResultSet resultSet) { From 3071c29cfd58b5518cf9385186b81fe35390e24a Mon Sep 17 00:00:00 2001 From: Davide D'Alto Date: Thu, 23 Nov 2023 11:58:08 +0100 Subject: [PATCH 3/3] [#1792] Add test for getSingleResultOrNull and getSingleResult We are checking that it doesn't throw a NullPointerException if the result of the query doesn't have any columns names. --- .../CustomOneToOneStoredProcedureSqlTest.java | 19 +++++++++++++++++++ .../CustomStoredProcedureSqlTest.java | 19 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/hibernate-reactive-core/src/test/java/org/hibernate/reactive/CustomOneToOneStoredProcedureSqlTest.java b/hibernate-reactive-core/src/test/java/org/hibernate/reactive/CustomOneToOneStoredProcedureSqlTest.java index ef542e8a3..451c95fee 100644 --- a/hibernate-reactive-core/src/test/java/org/hibernate/reactive/CustomOneToOneStoredProcedureSqlTest.java +++ b/hibernate-reactive-core/src/test/java/org/hibernate/reactive/CustomOneToOneStoredProcedureSqlTest.java @@ -9,9 +9,11 @@ import io.vertx.junit5.VertxTestContext; import jakarta.persistence.*; +import org.hibernate.HibernateException; import org.hibernate.annotations.SQLDelete; import org.hibernate.annotations.SQLInsert; import org.hibernate.reactive.testing.DBSelectionExtension; +import org.hibernate.reactive.testing.ReactiveAssertions; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -23,6 +25,7 @@ import java.util.List; import static java.util.concurrent.TimeUnit.*; +import static org.assertj.core.api.Assertions.assertThat; import static org.hibernate.reactive.containers.DatabaseConfiguration.DBType.*; import static org.hibernate.reactive.testing.DBSelectionExtension.*; import static org.junit.jupiter.api.Assertions.*; @@ -84,6 +87,22 @@ public void populateDb(VertxTestContext context) { ); } + @Test + public void testFailureWithGetSingleResultOrNull(VertxTestContext context) { + test( context, ReactiveAssertions.assertThrown( HibernateException.class, getMutinySessionFactory() + .withTransaction( s -> s.createNativeQuery( INSERT_DRIVER_LICENCE_SQL ).getSingleResultOrNull() ) ) + .invoke( e -> assertThat( e ).hasMessageContainingAll( "HR000080:", INSERT_DRIVER_LICENCE_SQL ) ) + ); + } + + @Test + public void testFailureWithGetSingleResult(VertxTestContext context) { + test( context, ReactiveAssertions.assertThrown( HibernateException.class, getSessionFactory() + .withTransaction( s -> s.createNativeQuery( INSERT_DRIVER_LICENCE_SQL ).getSingleResult() ) ) + .thenAccept( e -> assertThat( e ).hasMessageContainingAll( "HR000080:", INSERT_DRIVER_LICENCE_SQL ) ) + ); + } + @Test public void testInsertStoredProcedureDriverLicence(VertxTestContext context) { test( context, openSession().thenCompose( session -> session diff --git a/hibernate-reactive-core/src/test/java/org/hibernate/reactive/CustomStoredProcedureSqlTest.java b/hibernate-reactive-core/src/test/java/org/hibernate/reactive/CustomStoredProcedureSqlTest.java index ac002b9db..512b23dda 100644 --- a/hibernate-reactive-core/src/test/java/org/hibernate/reactive/CustomStoredProcedureSqlTest.java +++ b/hibernate-reactive-core/src/test/java/org/hibernate/reactive/CustomStoredProcedureSqlTest.java @@ -9,10 +9,12 @@ import java.util.Collection; import java.util.List; +import org.hibernate.HibernateException; import org.hibernate.annotations.SQLDelete; import org.hibernate.annotations.SQLInsert; import org.hibernate.annotations.SQLUpdate; import org.hibernate.reactive.testing.DBSelectionExtension; +import org.hibernate.reactive.testing.ReactiveAssertions; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; @@ -29,6 +31,7 @@ import jakarta.persistence.Table; import static java.util.concurrent.TimeUnit.MINUTES; +import static org.assertj.core.api.Assertions.assertThat; import static org.hibernate.reactive.containers.DatabaseConfiguration.DBType.POSTGRESQL; import static org.hibernate.reactive.testing.DBSelectionExtension.runOnlyFor; import static org.junit.jupiter.api.Assertions.assertEquals; @@ -102,6 +105,22 @@ public void populateDb(VertxTestContext context) { ); } + @Test + public void testFailureWithGetSingleResultOrNull(VertxTestContext context) { + test( context, ReactiveAssertions.assertThrown( HibernateException.class, getMutinySessionFactory() + .withTransaction( s -> s.createNativeQuery( INSERT_SP_SQL ).getSingleResultOrNull() ) ) + .invoke( e -> assertThat( e ).hasMessageContainingAll( "HR000080:", INSERT_SP_SQL ) ) + ); + } + + @Test + public void testFailureWithGetSingleResult(VertxTestContext context) { + test( context, ReactiveAssertions.assertThrown( HibernateException.class, getSessionFactory() + .withTransaction( s -> s.createNativeQuery( INSERT_SP_SQL ).getSingleResult() ) ) + .thenAccept( e -> assertThat( e ).hasMessageContainingAll( "HR000080:", INSERT_SP_SQL ) ) + ); + } + @Test public void testInsertStoredProcedure(VertxTestContext context) { test( context, openSession().thenCompose( session -> session