Skip to content
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

fix: Invalid database connection is saved AND used despite "CANCEL" #5798

Merged
merged 6 commits into from
Nov 29, 2024
Merged

fix: Invalid database connection is saved AND used despite "CANCEL" #5798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4e8d594
chore: light improvements on readability
befc Nov 26, 2024
cd6555e
fix: Invalid database connection is saved AND used despite "CANCEL"
befc Nov 27, 2024
bbd202a
chore: address checkstyle issues in edited files
befc Nov 27, 2024
d4c623b
fix: fix issue in the code
befc Nov 27, 2024
a37ddc4
chore: checkstyle
befc Nov 27, 2024
d2a73e2
fix: fix failing unit test
befc Nov 29, 2024
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
3 changes: 1 addition & 2 deletions ...c/main/java/org/pentaho/platform/engine/services/connection/PentahoConnectionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,8 +116,7 @@ public static IPentahoConnection getConnection( final String datasourceType, Pro
IPentahoConnection connection = null;
try {
//Validate if properties connection name is system DB and do not allow connection
if ( isSystemConnection( properties ) &&
!hasSystemDataSourcePermission( session ) ) {
if ( isSystemConnection( properties ) && !hasSystemDataSourcePermission( session ) ) {
throw new ObjectFactoryException( "Missing required permissions to make connection" );
}
connection = PentahoSystem.getObjectFactory().get( IPentahoConnection.class, key, session );
Expand Down
27 changes: 16 additions & 11 deletions ...g/pentaho/platform/engine/services/connection/datasource/dbcp/PooledDatasourceHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,11 @@ public class PooledDatasourceHelper {

public static PoolingDataSource setupPooledDataSource( IDatabaseConnection databaseConnection )
throws DBDatasourceServiceException {
return setupPooledDataSource( databaseConnection, true );
}

public static PoolingDataSource setupPooledDataSource( IDatabaseConnection databaseConnection, boolean useCache )
throws DBDatasourceServiceException {
try {
if ( databaseConnection.getAccessType().equals( DatabaseAccessType.JNDI ) ) {
throwDBDatasourceServiceException( databaseConnection.getName(), "PooledDatasourceHelper.ERROR_0008_UNABLE_TO_POOL_DATASOURCE_IT_IS_JNDI" );
Expand All @@ -66,9 +70,10 @@ public static PoolingDataSource setupPooledDataSource( IDatabaseConnection datab
loadDriverClass( databaseConnection, dialect, driverClass );

PoolingManagedDataSource poolingDataSource = new PoolingManagedDataSource( databaseConnection, dialect );

ICacheManager cacheManager = PentahoSystem.getCacheManager( null );
cacheManager.putInRegionCache( IDBDatasourceService.JDBC_DATASOURCE, databaseConnection.getName(), poolingDataSource );
if ( useCache ) {
ICacheManager cacheManager = PentahoSystem.getCacheManager( null );
cacheManager.putInRegionCache( IDBDatasourceService.JDBC_DATASOURCE, databaseConnection.getName(), poolingDataSource );
}
return poolingDataSource;
} catch ( Exception e ) {
throw new DBDatasourceServiceException( e );
Expand Down Expand Up @@ -100,13 +105,13 @@ private static void configurePool( IDatabaseConnection databaseConnection, IData
pool.setMaxTotal( databaseConnection.getMaximumPoolSize() );

// Configure connection pool properties
int maxIdleConnection = getIntegerPropertyValue( attributes, IDBDatasourceService.MAX_IDLE_KEY, PentahoSystem.getSystemSetting( "dbcp-defaults/max-idle-conn", null) );
int minIdleConnection = getIntegerPropertyValue( attributes, IDBDatasourceService.MIN_IDLE_KEY, PentahoSystem.getSystemSetting( "dbcp-defaults/min-idle-conn", null) );
int maxActiveConnection = getIntegerPropertyValue( attributes, IDBDatasourceService.MAX_ACTIVE_KEY, PentahoSystem.getSystemSetting( "dbcp-defaults/max-act-conn", null) );
long waitTime = getLongPropertyValue( attributes, IDBDatasourceService.MAX_WAIT_KEY, PentahoSystem.getSystemSetting( "dbcp-defaults/wait", null) );
boolean testWhileIdle = getBooleanPropertyValue( attributes, IDBDatasourceService.TEST_WHILE_IDLE, PentahoSystem.getSystemSetting( "dbcp-defaults/test-while-idle", null) );
boolean testOnBorrow = getBooleanPropertyValue( attributes, IDBDatasourceService.TEST_ON_BORROW, PentahoSystem.getSystemSetting( "dbcp-defaults/test-on-borrow", null) );
boolean testOnReturn = getBooleanPropertyValue( attributes, IDBDatasourceService.TEST_ON_RETURN, PentahoSystem.getSystemSetting( "dbcp-defaults/test-on-return", null) );
int maxIdleConnection = getIntegerPropertyValue( attributes, IDBDatasourceService.MAX_IDLE_KEY, PentahoSystem.getSystemSetting( "dbcp-defaults/max-idle-conn", null ) );
int minIdleConnection = getIntegerPropertyValue( attributes, IDBDatasourceService.MIN_IDLE_KEY, PentahoSystem.getSystemSetting( "dbcp-defaults/min-idle-conn", null ) );
int maxActiveConnection = getIntegerPropertyValue( attributes, IDBDatasourceService.MAX_ACTIVE_KEY, PentahoSystem.getSystemSetting( "dbcp-defaults/max-act-conn", null ) );
long waitTime = getLongPropertyValue( attributes, IDBDatasourceService.MAX_WAIT_KEY, PentahoSystem.getSystemSetting( "dbcp-defaults/wait", null ) );
boolean testWhileIdle = getBooleanPropertyValue( attributes, IDBDatasourceService.TEST_WHILE_IDLE, PentahoSystem.getSystemSetting( "dbcp-defaults/test-while-idle", null ) );
boolean testOnBorrow = getBooleanPropertyValue( attributes, IDBDatasourceService.TEST_ON_BORROW, PentahoSystem.getSystemSetting( "dbcp-defaults/test-on-borrow", null ) );
boolean testOnReturn = getBooleanPropertyValue( attributes, IDBDatasourceService.TEST_ON_RETURN, PentahoSystem.getSystemSetting( "dbcp-defaults/test-on-return", null ) );

// Tuning the connection pool
pool.setMaxTotal( maxActiveConnection );
Expand Down Expand Up @@ -223,7 +228,7 @@ private static GenericObjectPool initializeObjectPool( Map<String, String> attri
}

private static String getPropertyValue( Map<String, String> attributes, String key, String defaultValue ) {
if ( attributes.containsKey( key ) ){
if ( attributes.containsKey( key ) ) {
return attributes.get( key );
}
return defaultValue;
Expand Down
93 changes: 57 additions & 36 deletions ...ons/src/main/java/org/pentaho/platform/plugin/services/connections/sql/SQLConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,10 +223,50 @@ protected void initWithJNDI( final String jndiName ) {
}
}

void initOther( Properties props ) {
String connectionName = props.getProperty( IPentahoConnection.CONNECTION_NAME );
if ( connectionName != null && !connectionName.isEmpty() ) {
boolean isTestConnection = isTestConnection( connectionName );
if ( isTestConnection ) {
handleTestConnection( props, connectionName );
}
IDatabaseConnection databaseConnection = (IDatabaseConnection) props.get( IPentahoConnection.CONNECTION );
// If the connection is a test connection, we don't want to cache it
initDataSource( databaseConnection, !isTestConnection );
} else {
String driver = props.getProperty( IPentahoConnection.DRIVER_KEY );
String provider = props.getProperty( IPentahoConnection.LOCATION_KEY );
String userName = props.getProperty( IPentahoConnection.USERNAME_KEY );
String password = props.getProperty( IPentahoConnection.PASSWORD_KEY );
init( driver, provider, userName, password );
String query = props.getProperty( IPentahoConnection.QUERY_KEY );
if ( query != null && !query.isEmpty() ) {
try {
executeQuery( query );
} catch ( Exception e ) {
logger.error( "Can't execute query", e );
}
}
}
}

private static boolean isTestConnection( String connectionName ) {
return connectionName.length() > 16
&& connectionName.startsWith( "__TEST__" )
&& connectionName.endsWith( "__TEST__" );
}

private static void handleTestConnection( Properties props, String connectionName ) {
connectionName = connectionName.substring( 8, connectionName.length() - 8 );
props.setProperty( IPentahoConnection.CONNECTION_NAME, connectionName );
}



/**
* Allows the native SQL Connection to be enhanced in a subclass. Best used when a connection needs to be enhanced
* with an "effective user"
*
*
* @param connection
*/
protected void enhanceConnection( Connection connection ) throws SQLException {
Expand All @@ -235,7 +275,7 @@ protected void enhanceConnection( Connection connection ) throws SQLException {
/**
* Allows enhancements to the native SQL Connection to be removed in a subclass. Best used when a connection needs to
* be enhanced with an "effective user"
*
*
* @param connection
*/
protected void unEnhanceConnection( Connection connection ) throws SQLException {
Expand All @@ -244,7 +284,7 @@ protected void unEnhanceConnection( Connection connection ) throws SQLException
/**
* Allow wrapping/proxying of the native SQL connection by a subclass. Best used when a connection needs to be be
* enhanced or proxied for Single Signon or possibly tenanting.
*
*
* @param connection
* @return
*/
Expand All @@ -255,7 +295,7 @@ protected Connection captureConnection( Connection connection ) throws SQLExcept
/**
* Allows the native SQL Statement to be enhanced by a subclass. Examples may be to allow additional information like
* a user to be bound to the statement.
*
*
* @param statement
*/
protected void enhanceStatement( Statement statement ) throws SQLException {
Expand Down Expand Up @@ -326,7 +366,7 @@ public void close() {

/*
* (non-Javadoc)
*
*
* @see org.pentaho.connection.IPentahoConnection#getLastQuery()
*/
public String getLastQuery() {
Expand All @@ -335,7 +375,7 @@ public String getLastQuery() {

/**
* Executes the specified query.
*
*
* @param query
* the query to execute
* @return the resultset from the query
Expand All @@ -352,7 +392,7 @@ public IPentahoResultSet executeQuery( final String query ) throws SQLException,

/**
* Executes the specified query with the defined parameters
*
*
* @param query
* the query to be executed
* @param scrollType
Expand Down Expand Up @@ -420,7 +460,7 @@ public IPentahoResultSet prepareAndExecuteQuery( final String query, final List
* The purpose of this method is to set limitations such as fetchSize and maxrows on the provided statement. If the
* JDBC driver does not support the setting and throws an Exception, we will re-throw iff the limit was explicitly
* set.
*
*
* @param stmt
* Either a Statement or PreparedStatement
* @throws SQLException
Expand Down Expand Up @@ -535,7 +575,7 @@ public boolean preparedQueriesSupported() {

/*
* (non-Javadoc)
*
*
* @see org.pentaho.connection.IPentahoConnection#isClosed()
*/
public boolean isClosed() {
Expand All @@ -550,10 +590,10 @@ public boolean isClosed() {

/*
* (non-Javadoc)
*
*
* @see org.pentaho.connection.IPentahoConnection#isReadOnly()
*
* Right now this archetecture only support selects (read only)
*
* Right now this architecture only support selects (read only)
*/
public boolean isReadOnly() {
return true;
Expand All @@ -571,10 +611,9 @@ public IPentahoResultSet getResultSet() {
return sqlResultSet;
}

void initDataSource( IDatabaseConnection databaseConnection ) {
DataSource dataSource = null;
void initDataSource( IDatabaseConnection databaseConnection, boolean useCache ) {
try {
dataSource = PooledDatasourceHelper.setupPooledDataSource( databaseConnection );
DataSource dataSource = PooledDatasourceHelper.setupPooledDataSource( databaseConnection, useCache );
nativeConnection = captureConnection( dataSource.getConnection() );
} catch ( Exception e ) {
logger.error( "Can't get connection from Pool", e );
Expand All @@ -584,30 +623,12 @@ void initDataSource( IDatabaseConnection databaseConnection ) {
public boolean connect( final Properties props ) {
close();
String jndiName = props.getProperty( IPentahoConnection.JNDI_NAME_KEY );
if ( ( jndiName != null ) && ( jndiName.length() > 0 ) ) {
if ( jndiName != null && !jndiName.isEmpty() ) {
initWithJNDI( jndiName );
} else {
String connectionName = props.getProperty( IPentahoConnection.CONNECTION_NAME );
if ( ( connectionName != null ) && ( connectionName.length() > 0 ) ) {
IDatabaseConnection databaseConnection = (IDatabaseConnection) props.get( IPentahoConnection.CONNECTION );
initDataSource( databaseConnection );
} else {
String driver = props.getProperty( IPentahoConnection.DRIVER_KEY );
String provider = props.getProperty( IPentahoConnection.LOCATION_KEY );
String userName = props.getProperty( IPentahoConnection.USERNAME_KEY );
String password = props.getProperty( IPentahoConnection.PASSWORD_KEY );
init( driver, provider, userName, password );
String query = props.getProperty( IPentahoConnection.QUERY_KEY );
if ( ( query != null ) && ( query.length() > 0 ) ) {
try {
executeQuery( query );
} catch ( Exception e ) {
logger.error( "Can't execute query", e );
}
}
}
initOther( props );
}
return ( ( nativeConnection != null ) && !isClosed() );
return ( nativeConnection != null && !isClosed() );
}

public int execute( final String query ) throws SQLException {
Expand Down
39 changes: 34 additions & 5 deletions ...src/test/java/org/pentaho/platform/plugin/services/connections/sql/SQLConnectionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,18 +24,30 @@
import org.junit.Test;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;

import org.pentaho.commons.connection.IPentahoConnection;
import org.pentaho.database.model.IDatabaseConnection;
import org.pentaho.platform.api.data.DBDatasourceServiceException;
import org.pentaho.platform.api.data.IDBDatasourceService;
import org.pentaho.platform.api.engine.ILogger;
import org.pentaho.platform.api.engine.IPentahoObjectFactory;
import org.pentaho.platform.api.engine.IPentahoSession;
import org.pentaho.platform.api.engine.ObjectFactoryException;
import org.pentaho.platform.engine.core.system.PentahoSystem;

import static org.junit.Assert.assertTrue;
import static org.mockito.ArgumentMatchers.nullable;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.argThat;
import static org.mockito.Mockito.*;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.nullable;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

import org.mockito.ArgumentMatcher;

public class SQLConnectionTest {
Expand Down Expand Up @@ -82,21 +94,38 @@ public boolean matches( final Class<?> arg ) {

@Test
public void testConnect() {
// Create a spy of SQLConnection to verify method calls
SQLConnection sqlc = spy( new SQLConnection() );
Properties props = new Properties();

props = new Properties();
// Mock the logger to avoid actual logging
ILogger logger = mock( ILogger.class );
doNothing().when( logger ).error( anyString(), any( Throwable.class ) );
sqlc.logger = logger;

// Test connection using JNDI name
Properties props = new Properties();
props.put( IPentahoConnection.JNDI_NAME_KEY, "test" );
assertTrue( "JNDI Test", sqlc.connect( props ) );
verify( sqlc ).initWithJNDI( "test" );
assertTrue( sqlc.initialized() );

// Test connection without JNDI name
props = new Properties();
doNothing().when( sqlc ).close();
doNothing().when( sqlc ).init( nullable( String.class ), nullable( String.class ), nullable( String.class ), nullable( String.class ) );
assertTrue( "NonPool Test", sqlc.connect( props ) );
verify( sqlc ).init( nullable( String.class ), nullable( String.class ), nullable( String.class ), nullable( String.class ) );
assertTrue( sqlc.initialized() );

doNothing().when( sqlc ).initDataSource( nullable( IDatabaseConnection.class ) );
// Test connection with a mock IDatabaseConnection
IDatabaseConnection mockDatabaseConnection = mock( IDatabaseConnection.class );
doNothing().when( sqlc ).initDataSource( eq( mockDatabaseConnection ), eq( false ) );

// Initialize the data source and test connection
props.put( IPentahoConnection.CONNECTION_NAME, "test" );
sqlc.initDataSource( mockDatabaseConnection, false );
assertTrue( "Pool Test", sqlc.connect( props ) );
verify( sqlc ).initDataSource( eq( mockDatabaseConnection ), eq( false ) );
assertTrue( sqlc.initialized() );
}
}