Do not close JNDI-resolved DataSource in JDBCQuotaStore.close()

JNDI lookups return a DataSource owned by the JNDI provider; consumers must
not close it, since the same name can be (and typically is) shared with other
parts of the application.

Add an ownsDataSource flag (default true to keep the previous behavior for
direct setDataSource callers) that JDBCQuotaStoreFactory clears whenever the
configuration uses a JNDISource.

on-behalf-of: @camptocamp <info@camptocamp.com>

Author: groldan

geowebcache/diskquota/jdbc/src/main/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStore.java

@@ -91,6 +91,14 @@ public class JDBCQuotaStore implements QuotaStore {
 
     private DataSource dataSource;
 
+    /**
+     * Whether {@link #close()} should close {@link #dataSource}. Set to {@code false} by {@link JDBCQuotaStoreFactory}
+     * when the DataSource is resolved from JNDI - in that case its lifecycle is owned by the JNDI provider, not by this
+     * store. Defaults to {@code true} so callers using only {@link #setDataSource(DataSource)} keep the previous
+     * behavior.
+     */
+    private boolean ownsDataSource = true;
+
     public JDBCQuotaStore(DefaultStorageFinder finder, TilePageCalculator tilePageCalculator) {
         this.finder = finder;
         this.calculator = tilePageCalculator;
@@ -117,6 +125,24 @@ public void setSchema(String schema) {
         this.schema = schema;
     }
 
+    /**
+     * @return whether {@link #close()} will close the {@link #setDataSource(DataSource) DataSource}. Defaults to
+     *     {@code true}; set to {@code false} for JNDI-resolved DataSources, whose lifecycle belongs to the JNDI
+     *     provider.
+     */
+    public boolean isOwnsDataSource() {
+        return ownsDataSource;
+    }
+
+    /**
+     * Controls whether {@link #close()} will close the configured {@link #setDataSource(DataSource) DataSource}. Set to
+     * {@code false} when the DataSource is borrowed (typically resolved from JNDI) so this store does not shut down a
+     * connection pool it does not own.
+     */
+    public void setOwnsDataSource(boolean ownsDataSource) {
+        this.ownsDataSource = ownsDataSource;
+    }
+
     /** Sets the connection pool provider and initializes the tables in the dbms if missing */
     @SuppressFBWarnings("NP_NULL_ON_SOME_PATH_FROM_RETURN_VALUE")
     public void setDataSource(DataSource dataSource) {
@@ -652,11 +678,14 @@ public PageStats setTruncated(final TilePage page) throws InterruptedException {
     public void close() throws Exception {
         log.info("Closing up the JDBC quota store ");
 
-        // try to close the data source if possible
-        if (dataSource instanceof BasicDataSource source) {
-            source.close();
-        } else if (dataSource instanceof Closeable closeable) {
-            closeable.close();
+        // try to close the data source if possible, but only if we own it - JNDI-resolved
+        // DataSources are owned by the JNDI provider and must not be closed here.
+        if (ownsDataSource) {
+            if (dataSource instanceof BasicDataSource source) {
+                source.close();
+            } else if (dataSource instanceof Closeable closeable) {
+                closeable.close();
+            }
         }
 
         // release the templates

geowebcache/diskquota/jdbc/src/main/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStoreFactory.java

@@ -161,6 +161,9 @@ private QuotaStore getJDBCStore(
         store.setDialect(dialect);
         // sets schema if configured in geowebcache-diskquota-jdbc.xml
         store.setSchema(expandedConfig.getSchema());
+        // JNDI-resolved DataSources are owned by the JNDI provider; inline connection pools are
+        // created here and owned by the store.
+        store.setOwnsDataSource(expandedConfig.getJNDISource() == null);
 
         // initialize it
         store.initialize();

geowebcache/diskquota/jdbc/src/test/java/org/geowebcache/diskquota/jdbc/JDBCQuotaStoreFactoryTest.java

@@ -14,14 +14,19 @@
 package org.geowebcache.diskquota.jdbc;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertSame;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
+import java.io.Closeable;
 import java.sql.Connection;
 import javax.naming.InitialContext;
 import javax.sql.DataSource;
 import org.geotools.util.factory.GeoTools;
 import org.geowebcache.config.ConfigurationException;
+import org.geowebcache.diskquota.storage.TilePageCalculator;
+import org.geowebcache.storage.DefaultStorageFinder;
 import org.junit.Test;
 import org.mockito.Mockito;
 
@@ -60,4 +65,52 @@ public void testJNDILookup() throws Exception {
             GeoTools.clearInitialContext();
         }
     }
+
+    /** A {@link DataSource} that also implements {@link Closeable} so we can observe whether close() is called. */
+    private interface CloseableDataSource extends DataSource, Closeable {}
+
+    @Test
+    @SuppressWarnings({"PMD.CloseResource"})
+    public void closeDoesNotCloseJndiDataSource() throws Exception {
+        CloseableDataSource jndiDs = Mockito.mock(CloseableDataSource.class);
+
+        JDBCQuotaStore store =
+                new JDBCQuotaStore(Mockito.mock(DefaultStorageFinder.class), Mockito.mock(TilePageCalculator.class));
+        store.setDataSource(jndiDs);
+        // Mirror the wiring done by JDBCQuotaStoreFactory#getJDBCStore for a JNDI configuration.
+        store.setOwnsDataSource(false);
+
+        store.close();
+
+        assertFalse("ownsDataSource flag must be false after explicit set", store.isOwnsDataSource());
+        Mockito.verify(jndiDs, Mockito.never()).close();
+    }
+
+    @Test
+    @SuppressWarnings({"PMD.CloseResource"})
+    public void closeClosesOwnedDataSource() throws Exception {
+        CloseableDataSource ownedDs = Mockito.mock(CloseableDataSource.class);
+
+        JDBCQuotaStore store =
+                new JDBCQuotaStore(Mockito.mock(DefaultStorageFinder.class), Mockito.mock(TilePageCalculator.class));
+        store.setDataSource(ownedDs);
+        // Default ownsDataSource=true preserves the historical behavior.
+        assertTrue(store.isOwnsDataSource());
+
+        store.close();
+
+        Mockito.verify(ownedDs).close();
+    }
+
+    @Test
+    public void closeIsSafeWhenDataSourceLacksCloseable() throws Exception {
+        DataSource plainDs = Mockito.mock(DataSource.class);
+        JDBCQuotaStore store =
+                new JDBCQuotaStore(Mockito.mock(DefaultStorageFinder.class), Mockito.mock(TilePageCalculator.class));
+        store.setDataSource(plainDs);
+
+        // No assertion beyond the absence of an exception - the store should not try to close a
+        // DataSource that does not implement Closeable / BasicDataSource.
+        store.close();
+    }
 }