From 7a329eba5b274d807ccb39d46c318d69864ac8a1 Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Wed, 3 Feb 2021 18:46:58 +0100 Subject: [PATCH] Do not retain partial column metadata in SimpleJdbcInsert Prior to this commit, if an SQLException was thrown while retrieving column metadata from the database, SimpleJdbcInsert would generate an INSERT statement that was syntactically valid but missing columns, which could lead to data silently missing in the database (for nullable columns). This commit fixes this by clearing all collected column metadata if an SQLException is thrown while processing the metadata. The result is that an InvalidDataAccessApiUsageException will be thrown later while generating the INSERT statement. The exception message now contains an additional hint to make use of SimpleJdbcInsert#usingColumns() in order to ensure that all required columns are included in the generated INSERT statement. SimpleJdbcCall can also encounter an SQLException while retrieving column metadata for a stored procedure/function, but an exception is not thrown since a later invocation of the stored procedure/function will likely fail anyway due to missing arguments. Consequently, this commit only improves the warning level log message by including a hint to make use of SimpleJdbcCall#addDeclaredParameter(). Closes gh-26486 --- .../metadata/GenericCallMetaDataProvider.java | 12 +++- .../GenericTableMetaDataProvider.java | 9 ++- .../core/metadata/TableMetaDataContext.java | 11 +++- .../jdbc/core/simple/SimpleJdbcCallTests.java | 39 +++++++++++++ .../core/simple/SimpleJdbcInsertTests.java | 58 +++++++++++++++++++ 5 files changed, 122 insertions(+), 7 deletions(-) diff --git a/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/GenericCallMetaDataProvider.java b/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/GenericCallMetaDataProvider.java index f0b6991f682..8a9b2d0a3fb 100644 --- a/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/GenericCallMetaDataProvider.java +++ b/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/GenericCallMetaDataProvider.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -39,6 +39,7 @@ import org.springframework.util.StringUtils; * * @author Thomas Risberg * @author Juergen Hoeller + * @author Sam Brannen * @since 2.5 */ public class GenericCallMetaDataProvider implements CallMetaDataProvider { @@ -414,8 +415,15 @@ public class GenericCallMetaDataProvider implements CallMetaDataProvider { } catch (SQLException ex) { if (logger.isWarnEnabled()) { - logger.warn("Error while retrieving meta-data for procedure columns: " + ex); + logger.warn("Error while retrieving meta-data for procedure columns. " + + "Consider declaring explicit parameters -- for example, via SimpleJdbcCall#addDeclaredParameter().", + ex); } + // Although we could invoke `this.callParameterMetaData.clear()` so that + // we don't retain a partial list of column names (like we do in + // GenericTableMetaDataProvider.processTableColumns(...)), we choose + // not to do that here, since invocation of the stored procedure will + // likely fail anyway with an incorrect argument list. } } diff --git a/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/GenericTableMetaDataProvider.java b/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/GenericTableMetaDataProvider.java index d5c7727839a..5228cb9b05c 100644 --- a/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/GenericTableMetaDataProvider.java +++ b/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/GenericTableMetaDataProvider.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -39,6 +39,7 @@ import org.springframework.lang.Nullable; * * @author Thomas Risberg * @author Juergen Hoeller + * @author Sam Brannen * @since 2.5 */ public class GenericTableMetaDataProvider implements TableMetaDataProvider { @@ -422,8 +423,12 @@ public class GenericTableMetaDataProvider implements TableMetaDataProvider { } catch (SQLException ex) { if (logger.isWarnEnabled()) { - logger.warn("Error while retrieving meta-data for table columns: " + ex.getMessage()); + logger.warn("Error while retrieving meta-data for table columns. " + + "Consider specifying explicit column names -- for example, via SimpleJdbcInsert#usingColumns().", + ex); } + // Clear the metadata so that we don't retain a partial list of column names + this.tableParameterMetaData.clear(); } finally { JdbcUtils.closeResultSet(tableColumns); diff --git a/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/TableMetaDataContext.java b/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/TableMetaDataContext.java index 908b165befa..13e582a113d 100644 --- a/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/TableMetaDataContext.java +++ b/spring-jdbc/src/main/java/org/springframework/jdbc/core/metadata/TableMetaDataContext.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2021 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -43,6 +43,7 @@ import org.springframework.util.CollectionUtils; * * @author Thomas Risberg * @author Juergen Hoeller + * @author Sam Brannen * @since 2.5 */ public class TableMetaDataContext { @@ -302,8 +303,12 @@ public class TableMetaDataContext { } } else { - throw new InvalidDataAccessApiUsageException("Unable to locate columns for table '" + - getTableName() + "' so an insert statement can't be generated"); + String message = "Unable to locate columns for table '" + getTableName() + + "' so an insert statement can't be generated."; + if (isAccessTableColumnMetaData()) { + message += " Consider specifying explicit column names -- for example, via SimpleJdbcInsert#usingColumns()."; + } + throw new InvalidDataAccessApiUsageException(message); } } String params = String.join(", ", Collections.nCopies(columnCount, "?")); diff --git a/spring-jdbc/src/test/java/org/springframework/jdbc/core/simple/SimpleJdbcCallTests.java b/spring-jdbc/src/test/java/org/springframework/jdbc/core/simple/SimpleJdbcCallTests.java index b2589fa56d5..42df4595207 100644 --- a/spring-jdbc/src/test/java/org/springframework/jdbc/core/simple/SimpleJdbcCallTests.java +++ b/spring-jdbc/src/test/java/org/springframework/jdbc/core/simple/SimpleJdbcCallTests.java @@ -46,6 +46,7 @@ import static org.mockito.Mockito.verify; * * @author Thomas Risberg * @author Kiril Nugmanov + * @author Sam Brannen */ class SimpleJdbcCallTests { @@ -224,6 +225,44 @@ class SimpleJdbcCallTests { verifyStatement(adder, "{call ADD_INVOICE(AMOUNT => ?, CUSTID => ?, NEWID => ?)}"); } + /** + * This test demonstrates that a CALL statement will still be generated if + * an exception occurs while retrieving metadata, potentially resulting in + * missing metadata and consequently a failure while invoking the stored + * procedure. + */ + @Test // gh-26486 + void exceptionThrownWhileRetrievingColumnNamesFromMetadata() throws Exception { + ResultSet proceduresResultSet = mock(ResultSet.class); + ResultSet procedureColumnsResultSet = mock(ResultSet.class); + + given(databaseMetaData.getDatabaseProductName()).willReturn("Oracle"); + given(databaseMetaData.getUserName()).willReturn("ME"); + given(databaseMetaData.storesUpperCaseIdentifiers()).willReturn(true); + given(databaseMetaData.getProcedures("", "ME", "ADD_INVOICE")).willReturn(proceduresResultSet); + given(databaseMetaData.getProcedureColumns("", "ME", "ADD_INVOICE", null)).willReturn(procedureColumnsResultSet); + + given(proceduresResultSet.next()).willReturn(true, false); + given(proceduresResultSet.getString("PROCEDURE_NAME")).willReturn("add_invoice"); + + given(procedureColumnsResultSet.next()).willReturn(true, true, true, false); + given(procedureColumnsResultSet.getString("COLUMN_NAME")).willReturn("amount", "custid", "newid"); + given(procedureColumnsResultSet.getInt("DATA_TYPE")) + // Return a valid data type for the first 2 columns. + .willReturn(Types.INTEGER, Types.INTEGER) + // 3rd time, simulate an error while retrieving metadata. + .willThrow(new SQLException("error with DATA_TYPE for column 3")); + + SimpleJdbcCall adder = new SimpleJdbcCall(dataSource).withNamedBinding().withProcedureName("add_invoice"); + adder.compile(); + // If an exception were not thrown for column 3, we would expect: + // {call ADD_INVOICE(AMOUNT => ?, CUSTID => ?, NEWID => ?)} + verifyStatement(adder, "{call ADD_INVOICE(AMOUNT => ?, CUSTID => ?)}"); + + verify(proceduresResultSet).close(); + verify(procedureColumnsResultSet).close(); + } + private void verifyStatement(SimpleJdbcCall adder, String expected) { assertThat(adder.getCallString()).as("Incorrect call statement").isEqualTo(expected); diff --git a/spring-jdbc/src/test/java/org/springframework/jdbc/core/simple/SimpleJdbcInsertTests.java b/spring-jdbc/src/test/java/org/springframework/jdbc/core/simple/SimpleJdbcInsertTests.java index 023f6054f4a..d965d598f01 100644 --- a/spring-jdbc/src/test/java/org/springframework/jdbc/core/simple/SimpleJdbcInsertTests.java +++ b/spring-jdbc/src/test/java/org/springframework/jdbc/core/simple/SimpleJdbcInsertTests.java @@ -19,6 +19,8 @@ package org.springframework.jdbc.core.simple; import java.sql.Connection; import java.sql.DatabaseMetaData; import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Types; import java.util.Collections; import javax.sql.DataSource; @@ -29,6 +31,7 @@ import org.junit.jupiter.api.Test; import org.springframework.dao.InvalidDataAccessApiUsageException; +import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; @@ -38,6 +41,7 @@ import static org.mockito.Mockito.verify; * Mock object based tests for {@link SimpleJdbcInsert}. * * @author Thomas Risberg + * @author Sam Brannen */ class SimpleJdbcInsertTests { @@ -80,4 +84,58 @@ class SimpleJdbcInsertTests { verify(resultSet).close(); } + @Test // gh-26486 + void retrieveColumnNamesFromMetadata() throws Exception { + ResultSet tableResultSet = mock(ResultSet.class); + given(tableResultSet.next()).willReturn(true, false); + + given(databaseMetaData.getUserName()).willReturn("me"); + given(databaseMetaData.getTables(null, null, "me", null)).willReturn(tableResultSet); + + ResultSet columnResultSet = mock(ResultSet.class); + given(databaseMetaData.getColumns(null, "me", null, null)).willReturn(columnResultSet); + given(columnResultSet.next()).willReturn(true, true, false); + given(columnResultSet.getString("COLUMN_NAME")).willReturn("col1", "col2"); + given(columnResultSet.getInt("DATA_TYPE")).willReturn(Types.VARCHAR); + given(columnResultSet.getBoolean("NULLABLE")).willReturn(false); + + SimpleJdbcInsert insert = new SimpleJdbcInsert(dataSource).withTableName("me"); + insert.compile(); + assertThat(insert.getInsertString()).isEqualTo("INSERT INTO me (col1, col2) VALUES(?, ?)"); + + verify(columnResultSet).close(); + verify(tableResultSet).close(); + } + + @Test // gh-26486 + void exceptionThrownWhileRetrievingColumnNamesFromMetadata() throws Exception { + ResultSet tableResultSet = mock(ResultSet.class); + given(tableResultSet.next()).willReturn(true, false); + + given(databaseMetaData.getUserName()).willReturn("me"); + given(databaseMetaData.getTables(null, null, "me", null)).willReturn(tableResultSet); + + ResultSet columnResultSet = mock(ResultSet.class); + given(databaseMetaData.getColumns(null, "me", null, null)).willReturn(columnResultSet); + // true, true, false --> simulates processing of two columns + given(columnResultSet.next()).willReturn(true, true, false); + given(columnResultSet.getString("COLUMN_NAME")) + // Return a column name the first time. + .willReturn("col1") + // Second time, simulate an error while retrieving metadata. + .willThrow(new SQLException("error with col2")); + given(columnResultSet.getInt("DATA_TYPE")).willReturn(Types.VARCHAR); + given(columnResultSet.getBoolean("NULLABLE")).willReturn(false); + + SimpleJdbcInsert insert = new SimpleJdbcInsert(dataSource).withTableName("me"); + + assertThatExceptionOfType(InvalidDataAccessApiUsageException.class) + .isThrownBy(insert::compile) + .withMessage("Unable to locate columns for table 'me' so an insert statement can't be generated. " + + "Consider specifying explicit column names -- for example, via SimpleJdbcInsert#usingColumns()."); + + verify(columnResultSet).close(); + verify(tableResultSet).close(); + } + }