From 3bad1be792e07d4cddc71957e1ab312238a52a10 Mon Sep 17 00:00:00 2001
From: Andreas Mandel <amandel@users.noreply.github.com>
Date: Sun, 28 Aug 2022 20:33:16 +0200
Subject: [PATCH] fix JENKINS-69375: ensure properties are passed to Hikari
 (#504)

* JENKINS-69375 ensure properties are passed to Hikari

The generic properties must be passed to Hikari in the constructor of `HikariConfig` otherwise they are not fully applied in the expected way. To maintain backward compatibility, I've also added the former `dsConfig.setDataSourceProperties(p)`, so that the properties are also still fully available as data source properties.

* add unit test

Co-authored-by: Benoit GUERIN <benoit.guerin1@free.fr>
---
 .../maven/GlobalPipelineMavenConfig.java      |  92 +++++----
 .../maven/GlobalPipelineMavenConfigTest.java  | 174 ++++++++++++++++++
 2 files changed, 219 insertions(+), 47 deletions(-)
 create mode 100644 jenkins-plugin/src/test/java/org/jenkinsci/plugins/pipeline/maven/GlobalPipelineMavenConfigTest.java

diff --git a/jenkins-plugin/src/main/java/org/jenkinsci/plugins/pipeline/maven/GlobalPipelineMavenConfig.java b/jenkins-plugin/src/main/java/org/jenkinsci/plugins/pipeline/maven/GlobalPipelineMavenConfig.java
index 68f4c0bb..e99b668d 100644
--- a/jenkins-plugin/src/main/java/org/jenkinsci/plugins/pipeline/maven/GlobalPipelineMavenConfig.java
+++ b/jenkins-plugin/src/main/java/org/jenkinsci/plugins/pipeline/maven/GlobalPipelineMavenConfig.java
@@ -62,7 +62,6 @@
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.sql.DataSource;
-import java.io.Closeable;
 import java.io.File;
 import java.io.IOException;
 import java.io.StringReader;
@@ -258,39 +257,9 @@ public synchronized PipelineMavenPluginDao getDao() {
                     jdbcPassword = Secret.toString(jdbcCredentials.getPassword());
                 }
 
-                HikariConfig dsConfig = new HikariConfig();
-                dsConfig.setJdbcUrl(jdbcUrl);
-                dsConfig.setUsername(jdbcUserName);
-                dsConfig.setPassword(jdbcPassword);
+                HikariConfig dsConfig = createHikariConfig(properties, jdbcUrl, jdbcUserName, jdbcPassword);
                 dsConfig.setAutoCommit(false);
 
-                Properties p = new Properties();
-                // todo refactor the DAO to inject config defaults in the DAO
-                if (jdbcUrl.startsWith("jdbc:mysql")) {
-                    // https://github.com/brettwooldridge/HikariCP#configuration-knobs-baby
-                    // https://github.com/brettwooldridge/HikariCP/wiki/MySQL-Configuration
-                    p.setProperty("dataSource.cachePrepStmts", "true");
-                    p.setProperty("dataSource.prepStmtCacheSize", "250");
-                    p.setProperty("dataSource.prepStmtCacheSqlLimit", "2048");
-                    p.setProperty("dataSource.useServerPrepStmts", "true");
-                    p.setProperty("dataSource.useLocalSessionState", "true");
-                    p.setProperty("dataSource.rewriteBatchedStatements", "true");
-                    p.setProperty("dataSource.cacheResultSetMetadata", "true");
-                    p.setProperty("dataSource.cacheServerConfiguration", "true");
-                    p.setProperty("dataSource.elideSetAutoCommits", "true");
-                    p.setProperty("dataSource.maintainTimeStats", "false");
-                } else if (jdbcUrl.startsWith("jdbc:postgresql")) {
-                    // no tuning recommendations found for postgresql
-                } else if (jdbcUrl.startsWith("jdbc:h2")) {
-                    // dsConfig.setDataSourceClassName("org.h2.jdbcx.JdbcDataSource"); don't specify the datasource due to a classloading issue
-                } else {
-                    // unsupported config
-                }
-                if (StringUtils.isNotBlank(properties)) {
-                    p.load(new StringReader(properties));
-                }
-                dsConfig.setDataSourceProperties(p);
-
                 // TODO cleanup this quick fix for JENKINS-54587, we should have a better solution with the JDBC driver loaded by the DAO itself
                 try {
                     DriverManager.getDriver(jdbcUrl);
@@ -324,7 +293,7 @@ public synchronized PipelineMavenPluginDao getDao() {
                     }
                 }
 
-                LOGGER.log(Level.INFO, "Connect to database {0} with username {1} and properties {2}", new Object[]{jdbcUrl, jdbcUserName, p});
+                LOGGER.log(Level.INFO, "Connect to database {0} with username {1}", new Object[]{jdbcUrl, jdbcUserName});
                 DataSource ds = new HikariDataSource(dsConfig);
 
                 Class<? extends PipelineMavenPluginDao> daoClass;
@@ -346,7 +315,7 @@ public synchronized PipelineMavenPluginDao getDao() {
                 }
 
 
-            } catch (RuntimeException | SQLException | IOException e) {
+            } catch (RuntimeException | SQLException e) {
                 LOGGER.log(Level.WARNING, "Exception creating database dao, skip", e);
                 dao = new PipelineMavenPluginNullDao();
             }
@@ -452,20 +421,8 @@ public FormValidation doValidateJdbcConnection(
                 jdbcUserName = jdbcCredentials.getUsername();
                 jdbcPassword = Secret.toString(jdbcCredentials.getPassword());
             }
-            HikariConfig dsConfig = new HikariConfig();
-            dsConfig.setJdbcUrl(jdbcUrl);
-            dsConfig.setUsername(jdbcUserName);
-            dsConfig.setPassword(jdbcPassword);
 
-            if (StringUtils.isNotBlank(properties)) {
-                Properties p = new Properties();
-                try {
-                    p.load(new StringReader(properties));
-                } catch (IOException e) {
-                    throw new IllegalStateException(e);
-                }
-                dsConfig.setDataSourceProperties(p);
-            }
+            HikariConfig dsConfig = createHikariConfig(properties, jdbcUrl, jdbcUserName, jdbcPassword);
 
             try (HikariDataSource ds = new HikariDataSource(dsConfig)) {
                 try (Connection cnn = ds.getConnection()) {
@@ -579,6 +536,47 @@ public FormValidation doValidateJdbcConnection(
 
     }
 
+    private static HikariConfig createHikariConfig(String properties, String jdbcUrl, String jdbcUserName, String jdbcPassword) {
+        Properties p = new Properties();
+        // todo refactor the DAO to inject config defaults in the DAO
+        if (jdbcUrl.startsWith("jdbc:mysql")) {
+            // https://github.com/brettwooldridge/HikariCP#configuration-knobs-baby
+            // https://github.com/brettwooldridge/HikariCP/wiki/MySQL-Configuration
+            p.setProperty("dataSource.cachePrepStmts", "true");
+            p.setProperty("dataSource.prepStmtCacheSize", "250");
+            p.setProperty("dataSource.prepStmtCacheSqlLimit", "2048");
+            p.setProperty("dataSource.useServerPrepStmts", "true");
+            p.setProperty("dataSource.useLocalSessionState", "true");
+            p.setProperty("dataSource.rewriteBatchedStatements", "true");
+            p.setProperty("dataSource.cacheResultSetMetadata", "true");
+            p.setProperty("dataSource.cacheServerConfiguration", "true");
+            p.setProperty("dataSource.elideSetAutoCommits", "true");
+            p.setProperty("dataSource.maintainTimeStats", "false");
+        } else if (jdbcUrl.startsWith("jdbc:postgresql")) {
+            // no tuning recommendations found for postgresql
+        } else if (jdbcUrl.startsWith("jdbc:h2")) {
+            // dsConfig.setDataSourceClassName("org.h2.jdbcx.JdbcDataSource"); don't specify the datasource due to a classloading issue
+        } else {
+            // unsupported config
+        }
+
+        if (StringUtils.isNotBlank(properties)) {
+            try {
+                p.load(new StringReader(properties));
+            } catch (IOException e) {
+                throw new IllegalStateException("Failed to read properties.", e);
+            }
+        }
+        LOGGER.log(Level.INFO, "Applied pool properties {0}", p);
+        HikariConfig dsConfig = new HikariConfig(p);
+        dsConfig.setJdbcUrl(jdbcUrl);
+        dsConfig.setUsername(jdbcUserName);
+        dsConfig.setPassword(jdbcPassword);
+        // to mimic the old behaviour pre JENKINS-69375 fix
+        dsConfig.setDataSourceProperties(p);
+        return dsConfig;
+    }
+
     @Terminator
     public synchronized void closeDatasource() {
         if (dao != null) {
diff --git a/jenkins-plugin/src/test/java/org/jenkinsci/plugins/pipeline/maven/GlobalPipelineMavenConfigTest.java b/jenkins-plugin/src/test/java/org/jenkinsci/plugins/pipeline/maven/GlobalPipelineMavenConfigTest.java
new file mode 100644
index 00000000..baadfae8
--- /dev/null
+++ b/jenkins-plugin/src/test/java/org/jenkinsci/plugins/pipeline/maven/GlobalPipelineMavenConfigTest.java
@@ -0,0 +1,174 @@
+package org.jenkinsci.plugins.pipeline.maven;
+
+import static com.cloudbees.plugins.credentials.CredentialsScope.GLOBAL;
+import static java.util.Arrays.asList;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.core.Is.is;
+import static org.hamcrest.core.IsEqual.equalTo;
+import static org.hamcrest.core.IsInstanceOf.instanceOf;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
+import java.sql.Connection;
+import java.util.List;
+
+import org.acegisecurity.Authentication;
+import org.jenkinsci.plugins.pipeline.maven.dao.CustomTypePipelineMavenPluginDaoDecorator;
+import org.jenkinsci.plugins.pipeline.maven.dao.MonitoringPipelineMavenPluginDaoDecorator;
+import org.jenkinsci.plugins.pipeline.maven.dao.PipelineMavenPluginDao;
+import org.jenkinsci.plugins.pipeline.maven.dao.PipelineMavenPluginH2Dao;
+import org.jenkinsci.plugins.pipeline.maven.dao.PipelineMavenPluginMySqlDao;
+import org.jenkinsci.plugins.pipeline.maven.dao.PipelineMavenPluginPostgreSqlDao;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.jvnet.hudson.test.JenkinsRule;
+import org.testcontainers.containers.MySQLContainer;
+import org.testcontainers.containers.PostgreSQLContainer;
+
+import com.cloudbees.plugins.credentials.Credentials;
+import com.cloudbees.plugins.credentials.CredentialsProvider;
+import com.cloudbees.plugins.credentials.domains.DomainRequirement;
+import com.cloudbees.plugins.credentials.impl.UsernamePasswordCredentialsImpl;
+import com.mysql.cj.conf.RuntimeProperty;
+import com.mysql.cj.jdbc.ConnectionImpl;
+import com.zaxxer.hikari.HikariDataSource;
+import com.zaxxer.hikari.pool.HikariProxyConnection;
+
+import hudson.ExtensionList;
+import hudson.model.ItemGroup;
+import jenkins.model.Jenkins;
+
+public class GlobalPipelineMavenConfigTest {
+
+    @ClassRule
+    public static MySQLContainer<?> MYSQL_DB = new MySQLContainer<>(MySQLContainer.NAME).withUsername("aUser")
+            .withPassword("aPass");
+
+    @ClassRule
+    public static PostgreSQLContainer<?> POSTGRE_DB = new PostgreSQLContainer<>(PostgreSQLContainer.IMAGE)
+            .withUsername("aUser").withPassword("aPass");
+
+    @ClassRule
+    public static JenkinsRule j = new JenkinsRule();
+
+    private static class FakeCredentialsProvider extends CredentialsProvider {
+        public FakeCredentialsProvider() {
+        }
+
+        @Override
+        public boolean isEnabled(Object context) {
+            return true;
+        }
+
+        @Override
+        public <C extends Credentials> List<C> getCredentials(Class<C> type, ItemGroup itemGroup,
+                Authentication authentication, List<DomainRequirement> domainRequirements) {
+            return (List<C>) asList(new UsernamePasswordCredentialsImpl(GLOBAL, "credsId", "", "aUser", "aPass"));
+        }
+
+        @Override
+        public <C extends Credentials> List<C> getCredentials(Class<C> type, ItemGroup itemGroup,
+                Authentication authentication) {
+            return getCredentials(type, itemGroup, authentication, null);
+        }
+    }
+
+    private GlobalPipelineMavenConfig config = new GlobalPipelineMavenConfig();
+
+    @Test
+    public void shouldBuildH2Dao() throws Exception {
+        PipelineMavenPluginDao dao = config.getDao();
+
+        assertThat(dao, instanceOf(MonitoringPipelineMavenPluginDaoDecorator.class));
+        Object innerDao = getField(dao, "delegate");
+        assertThat(innerDao, instanceOf(CustomTypePipelineMavenPluginDaoDecorator.class));
+        innerDao = getField(innerDao, "delegate");
+        assertThat(innerDao, instanceOf(PipelineMavenPluginH2Dao.class));
+    }
+
+    @Test
+    public void shouldBuildMysqlDao() throws Exception {
+        ExtensionList<CredentialsProvider> extensionList = Jenkins.getInstance()
+                .getExtensionList(CredentialsProvider.class);
+        extensionList.add(extensionList.size(), new FakeCredentialsProvider());
+        config.setJdbcUrl(MYSQL_DB.getJdbcUrl());
+        config.setJdbcCredentialsId("credsId");
+        config.setProperties("maxLifetime=42000");
+
+        PipelineMavenPluginDao dao = config.getDao();
+
+        assertThat(dao, instanceOf(MonitoringPipelineMavenPluginDaoDecorator.class));
+        Object innerDao = getField(dao, "delegate");
+        assertThat(innerDao, instanceOf(CustomTypePipelineMavenPluginDaoDecorator.class));
+        innerDao = getField(innerDao, "delegate");
+        assertThat(innerDao, instanceOf(PipelineMavenPluginMySqlDao.class));
+        Object ds = getField(innerDao, "ds");
+        assertThat(ds, instanceOf(HikariDataSource.class));
+        HikariDataSource datasource = (HikariDataSource) ds;
+        assertThat(datasource.getJdbcUrl(), equalTo(MYSQL_DB.getJdbcUrl()));
+        assertThat(datasource.getUsername(), equalTo("aUser"));
+        assertThat(datasource.getPassword(), equalTo("aPass"));
+        assertThat(datasource.getMaxLifetime(), is(42000L));
+        assertThat(datasource.getDataSourceProperties().containsKey("dataSource.cachePrepStmts"), is(true));
+        assertThat(datasource.getDataSourceProperties().getProperty("dataSource.cachePrepStmts"), is("true"));
+        assertThat(datasource.getDataSourceProperties().containsKey("dataSource.prepStmtCacheSize"), is(true));
+        assertThat(datasource.getDataSourceProperties().getProperty("dataSource.prepStmtCacheSize"), is("250"));
+        Connection connection = datasource.getConnection();
+        assertThat(connection, instanceOf(HikariProxyConnection.class));
+        connection = (Connection) getField(connection, "delegate");
+        assertThat(connection, instanceOf(ConnectionImpl.class));
+        RuntimeProperty<Boolean> cachePrepStmts = (RuntimeProperty<Boolean>) getField(connection, "cachePrepStmts");
+        assertThat(cachePrepStmts.getValue(), is(true));
+    }
+
+    @Test
+    public void shouldBuildPostgresqlDao() throws Exception {
+        ExtensionList<CredentialsProvider> extensionList = Jenkins.getInstance()
+                .getExtensionList(CredentialsProvider.class);
+        extensionList.add(extensionList.size(), new FakeCredentialsProvider());
+        config.setJdbcUrl(POSTGRE_DB.getJdbcUrl());
+        config.setJdbcCredentialsId("credsId");
+
+        PipelineMavenPluginDao dao = config.getDao();
+
+        assertThat(dao, instanceOf(MonitoringPipelineMavenPluginDaoDecorator.class));
+        Object innerDao = getField(dao, "delegate");
+        assertThat(innerDao, instanceOf(CustomTypePipelineMavenPluginDaoDecorator.class));
+        innerDao = getField(innerDao, "delegate");
+        assertThat(innerDao, instanceOf(PipelineMavenPluginPostgreSqlDao.class));
+        Object ds = getField(innerDao, "ds");
+        assertThat(ds, instanceOf(HikariDataSource.class));
+        HikariDataSource datasource = (HikariDataSource) ds;
+        assertThat(datasource.getJdbcUrl(), equalTo(POSTGRE_DB.getJdbcUrl()));
+        assertThat(datasource.getUsername(), equalTo("aUser"));
+        assertThat(datasource.getPassword(), equalTo("aPass"));
+    }
+
+    private Object getField(Object targetObject, String name) throws Exception {
+        Class<?> targetClass = targetObject.getClass();
+        Field field = findField(targetClass, name, null);
+        makeAccessible(field);
+        return field.get(targetObject);
+    }
+
+    private Field findField(Class<?> clazz, String name, Class<?> type) {
+        Class<?> searchType = clazz;
+        while (Object.class != searchType && searchType != null) {
+            Field[] fields = searchType.getDeclaredFields();
+            for (Field field : fields) {
+                if ((name == null || name.equals(field.getName())) && (type == null || type.equals(field.getType()))) {
+                    return field;
+                }
+            }
+            searchType = searchType.getSuperclass();
+        }
+        return null;
+    }
+
+    private void makeAccessible(Field field) {
+        if ((!Modifier.isPublic(field.getModifiers()) || !Modifier.isPublic(field.getDeclaringClass().getModifiers())
+                || Modifier.isFinal(field.getModifiers())) && !field.isAccessible()) {
+            field.setAccessible(true);
+        }
+    }
+}