Skip to content

Commit

Permalink
Merge pull request #5798 from befc/BISERVER-15144
Browse files Browse the repository at this point in the history 
fix: Invalid database connection is saved AND used despite "CANCEL"
  • Loading branch information
@smmribeiro
smmribeiro authored Nov 29, 2024
2 parents b7ab326 + d2a73e2 commit 55e15ac
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 54 deletions.
3 changes: 1 addition & 2 deletions ...c/main/java/org/pentaho/platform/engine/services/connection/PentahoConnectionFactory.java
View file
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
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
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
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() );
}
}

0 comments on commit 55e15ac

Please sign in to comment.