From 4b497485618c013d2ad0df24bb27ffa176c8d514 Mon Sep 17 00:00:00 2001 From: Rene Pfeuffer Date: Thu, 19 Jun 2025 09:41:03 +0200 Subject: [PATCH] Introduce HikariCP as connection pool Squash commits of branch feature/hikari: - Introduce HikariCP as connection pool - Log change - Close maintenance stores correctly - Do not force minimum number of connections - Log change - Assert stores are closed correctly ... and test QueryableStoreDeletionHandler - Fix license - Fix unit tests - Use constants - Change change log - Move hikari version to libraries - Enhance documentation - Use configuration for pool settings --- docs/en/administration/scm-server.md | 45 ++-- docs/en/development/storage.md | 63 +++-- gradle/changelog/hikari.yaml | 4 + gradle/dependencies.gradle | 6 +- .../scm/store/QueryableMaintenanceStore.java | 5 +- scm-persistence/build.gradle | 1 + .../sqlite/SQLiteQueryableStoreFactory.java | 77 +++++- .../scm/store/QueryableStoreExtension.java | 220 +++++++++++++++++- .../store/QueryableStoreExtensionTest.java | 14 +- .../RepositoryQueryableStoreExporter.java | 9 +- .../store/QueryableStoreDeletionHandler.java | 6 +- .../RepositoryQueryableStoreExporterTest.java | 91 +++++--- .../QueryableStoreDeletionHandlerTest.java | 121 ++++++++++ .../store/QueryableTypeWithGroupParent.java | 26 +++ .../QueryableTypeWithMultipleParents.java | 28 +++ 15 files changed, 610 insertions(+), 106 deletions(-) create mode 100644 gradle/changelog/hikari.yaml create mode 100644 scm-webapp/src/test/java/sonia/scm/store/QueryableStoreDeletionHandlerTest.java create mode 100644 scm-webapp/src/test/java/sonia/scm/store/QueryableTypeWithGroupParent.java create mode 100644 scm-webapp/src/test/java/sonia/scm/store/QueryableTypeWithMultipleParents.java diff --git a/docs/en/administration/scm-server.md b/docs/en/administration/scm-server.md index 10a717116c..2a681f5302 100644 --- a/docs/en/administration/scm-server.md +++ b/docs/en/administration/scm-server.md @@ -214,22 +214,37 @@ webapp: workingCopyPoolStrategy: sonia.scm.repository.work.SimpleCachingWorkingCopyPool ## Amount of "cached" working copies workingCopyPoolSize: 5 + ## Settings for queryable stores, which are backed by an SQLite database. + ## Timeouts and lifetimes are in seconds + queryableStores: + maxPoolSize: 10 + connectionTimeout: 30 + idleTimeout: 600 + maxLifetime: 1800 + leakDetectionThreshold: 30 + + ``` **Environment variables** -| Environment Variable | Corresponding config.yml property | Example | -| ----------------------------------- | --------------------------------- | ------------------------------------------------------------------------------------------------ | -| SCM_WEBAPP_WORKDIR | webapp.workDir | export SCM_WEBAPP_WORKDIR=/tmp/scm-work | -| SCM_WEBAPP_HOMEDIR | webapp.homeDir | export SCM_WEBAPP_HOMEDIR=/var/lib/scm | -| SCM_WEBAPP_CACHE_DATAFILE_ENABLED | webapp.cache.datafile.enabled | export SCM_WEBAPP_CACHE_DATAFILE_ENABLED=true | -| SCM_WEBAPP_CACHE_STORE_ENABLED | webapp.cache.store.enabled | export SCM_WEBAPP_CACHE_STORE_ENABLED=true | -| SCM_WEBAPP_ENDLESSJWT | webapp.endlessJwt | export SCM_WEBAPP_ENDLESSJWT=false | -| SCM_WEBAPP_ASYNCTHREADS | webapp.asyncThreads | export SCM_WEBAPP_ASYNCTHREADS=4 | -| SCM_WEBAPP_MAXASYNCABORTSECONDS | webapp.maxAsyncAbortSeconds | export SCM_WEBAPP_MAXASYNCABORTSECONDS=60 | -| SCM_WEBAPP_CENTRALWORKQUEUE_WORKERS | webapp.centralWorkQueue.workers | export SCM_WEBAPP_CENTRALWORKQUEUE_WORKERS=4 | -| SCM_WEBAPP_WORKINGCOPYPOOLSTRATEGY | webapp.workingCopyPoolStrategy | export SCM_WEBAPP_WORKINGCOPYPOOLSTRATEGY=sonia.scm.repository.work.SimpleCachingWorkingCopyPool | -| SCM_WEBAPP_WORKINGCOPYPOOLSIZE | webapp.workingCopyPoolSize | export SCM_WEBAPP_WORKINGCOPYPOOLSIZE=5 | -| SCM_WEBAPP_INITIALUSER | webapp.initialUser | export SCM_WEBAPP_INITIALUSER=scmadmin | -| SCM_WEBAPP_INITIALPASSWORD | webapp.initialPassword | export SCM_WEBAPP_INITIALPASSWORD=scmadmin | -| SCM_WEBAPP_SKIPADMINCREATION | webapp.skipAdminCreation | export SCM_WEBAPP_SKIPADMINCREATION=true | +| Environment Variable | Corresponding config.yml property | Example | +|---------------------------------------------------|-----------------------------------------------|--------------------------------------------------------------------------------------------------| +| SCM_WEBAPP_WORKDIR | webapp.workDir | export SCM_WEBAPP_WORKDIR=/tmp/scm-work | +| SCM_WEBAPP_HOMEDIR | webapp.homeDir | export SCM_WEBAPP_HOMEDIR=/var/lib/scm | +| SCM_WEBAPP_CACHE_DATAFILE_ENABLED | webapp.cache.datafile.enabled | export SCM_WEBAPP_CACHE_DATAFILE_ENABLED=true | +| SCM_WEBAPP_CACHE_STORE_ENABLED | webapp.cache.store.enabled | export SCM_WEBAPP_CACHE_STORE_ENABLED=true | +| SCM_WEBAPP_ENDLESSJWT | webapp.endlessJwt | export SCM_WEBAPP_ENDLESSJWT=false | +| SCM_WEBAPP_ASYNCTHREADS | webapp.asyncThreads | export SCM_WEBAPP_ASYNCTHREADS=4 | +| SCM_WEBAPP_MAXASYNCABORTSECONDS | webapp.maxAsyncAbortSeconds | export SCM_WEBAPP_MAXASYNCABORTSECONDS=60 | +| SCM_WEBAPP_CENTRALWORKQUEUE_WORKERS | webapp.centralWorkQueue.workers | export SCM_WEBAPP_CENTRALWORKQUEUE_WORKERS=4 | +| SCM_WEBAPP_WORKINGCOPYPOOLSTRATEGY | webapp.workingCopyPoolStrategy | export SCM_WEBAPP_WORKINGCOPYPOOLSTRATEGY=sonia.scm.repository.work.SimpleCachingWorkingCopyPool | +| SCM_WEBAPP_WORKINGCOPYPOOLSIZE | webapp.workingCopyPoolSize | export SCM_WEBAPP_WORKINGCOPYPOOLSIZE=5 | +| SCM_WEBAPP_INITIALUSER | webapp.initialUser | export SCM_WEBAPP_INITIALUSER=scmadmin | +| SCM_WEBAPP_INITIALPASSWORD | webapp.initialPassword | export SCM_WEBAPP_INITIALPASSWORD=scmadmin | +| SCM_WEBAPP_SKIPADMINCREATION | webapp.skipAdminCreation | export SCM_WEBAPP_SKIPADMINCREATION=true | +| SCM_WEBAPP_QUERYABLESTORES_MAXPOOLSIZE | webapp.queryableStores.maxPoolSize | export SCM_WEBAPP_QUERYABLESTORES_MAXPOOLSIZE=20 | +| SCM_WEBAPP_QUERYABLESTORES_CONNECTIONTIMEOUT | webapp.queryableStores.connectionTimeout | export SCM_WEBAPP_QUERYABLESTORES_CONNECTIONTIMEOUT=30 | +| SCM_WEBAPP_QUERYABLESTORES_IDLETIMEOUT | webapp.queryableStores.idleTimeout | export SCM_WEBAPP_QUERYABLESTORES_IDLETIMEOUT=600 | +| SCM_WEBAPP_QUERYABLESTORES_MAXLIFETIME | webapp.queryableStores.maxLifetime | export SCM_WEBAPP_QUERYABLESTORES_MAXLIFETIME=1800 | +| SCM_WEBAPP_QUERYABLESTORES_LEAKDETECTIONTHRESHOLD | webapp.queryableStores.leakDetectionThreshold | export SCM_WEBAPP_QUERYABLESTORES_LEAKDETECTIONTHRESHOLD=1800 | diff --git a/docs/en/development/storage.md b/docs/en/development/storage.md index de05733ecb..0bba5672c9 100644 --- a/docs/en/development/storage.md +++ b/docs/en/development/storage.md @@ -78,6 +78,13 @@ however, specific methods are created here based on the parent classes mentioned To create and change data, specific IDs must be specified to access the store if parent classes have been defined. This store implements the known Store API (the `DataStore` interface), so no adjustments are needed in the application. +Since the new persistence layer is based on SQL, the stored have to be closed after use. This is done best with +a try-with-resources block. If the store is not closed, the connection to the database will not be released, +which can lead to performance issues and memory leaks that will let the application crash in the long run. +If stores are not closed, the application will log a warning message. The best way to avoid this is to use +unit tests with the `QueryableStoreExtension`, which will assert that all stores are closed correctly in the tested +code. + ### Queryable Store For more advanced queries that also extend beyond the boundaries of the parent classes, there is a new store with a new @@ -308,33 +315,38 @@ public class Demo { entity.setAge(age); entity.setTags(tags); - QueryableMutableStore store = storeFactory.getMutable(); - return store.put(entity); + try (QueryableMutableStore store = storeFactory.getMutable()) { + return store.put(entity); + } } public MyEntity readById(String id) { - QueryableMutableStore store = storeFactory.getMutable(); - return store.get(id); + try (QueryableMutableStore store = storeFactory.getMutable()) { + return store.get(id); + } } public Collection findByAge(int age) { - QueryableStore store = storeFactory.get(); - return store.query(MyEntityQueryFields.AGE.eq(age)).findAll(); + try (QueryableStore store = storeFactory.get()) { + return store.query(MyEntityQueryFields.AGE.eq(age)).findAll(); + } } public Collection findByName(String name) { - QueryableStore store = storeFactory.get(); - return store.query( - Conditions.or( - MyEntityQueryFields.NAME.eq(name), - MyEntityQueryFields.ALIAS.eq(name) - ) - ).findAll(); + try (QueryableStore store = storeFactory.get()) { + return store.query( + Conditions.or( + MyEntityQueryFields.NAME.eq(name), + MyEntityQueryFields.ALIAS.eq(name) + ) + ).findAll(); + } } public Collection findByTag(String tag) { - QueryableStore store = storeFactory.get(); - return store.query(MyEntityQueryFields.TAGS.contains(tag)).findAll(); + try (QueryableStore store = storeFactory.get()) { + return store.query(MyEntityQueryFields.TAGS.contains(tag)).findAll(); + } } } ``` @@ -364,22 +376,25 @@ public class Demo { } public void addContact(User user, String mail) { - QueryableMutableStore store = storeFactory.getMutable(user); Contact contact = new Contact(); contact.setMail(mail); - store.put(contact); + try (QueryableMutableStore store = storeFactory.getMutable(user)) { + store.put(contact); + } } /** Get contact for a single user. */ public Collection getContacts(User user) { - QueryableMutableStore store = storeFactory.getMutable(user); - return store.getAll().values(); + try (QueryableMutableStore store = storeFactory.getMutable(user)) { + return store.getAll().values(); + } } /** Get all contacts for all users. */ public Collection getAllContacts() { - QueryableStore store = storeFactory.getOverall(); - return store.query().findAll(); + try (QueryableStore store = storeFactory.getOverall()) { + return store.query().findAll(); + } } } ``` @@ -481,6 +496,7 @@ public class Contact { The following update step can be used to add the `type` field to all `Contact` entities: ```java + @Extension public class AddTypeToContactsUpdateStep implements UpdateStep { @@ -493,8 +509,9 @@ public class AddTypeToContactsUpdateStep implements UpdateStep { @Override public void doUpdate() { - try (MaintenanceIterator iter = updateStepUtilFactory.forQueryableType(Contact.class).iterateAll()) { - while(iter.hasNext()) { + try (sonia.scm.store.QueryableMaintenanceStore store = updateStepUtilFactory.forQueryableType(Contact.class); + MaintenanceIterator iter = store.iterateAll()) { + while (iter.hasNext()) { MaintenanceStoreEntry entry = iter.next(); Contact contact = entry.get(); contact.setType("personal"); diff --git a/gradle/changelog/hikari.yaml b/gradle/changelog/hikari.yaml new file mode 100644 index 0000000000..4a17ecc496 --- /dev/null +++ b/gradle/changelog/hikari.yaml @@ -0,0 +1,4 @@ +- type: changed + description: HikariCP introduced as the default connection pool for queryable stores +- type: fixed + description: Closing the queryable stores in maintenance actions (repository import, export and cleanup) diff --git a/gradle/dependencies.gradle b/gradle/dependencies.gradle index 65bc5c3c1f..d8d0b392d1 100644 --- a/gradle/dependencies.gradle +++ b/gradle/dependencies.gradle @@ -15,7 +15,8 @@ ext { bouncycastleVersion = '2.73.6' jettyVersion = '11.0.24' luceneVersion = '8.11.4' - sqliteVersion = '3.49.1.0' + sqliteVersion = '3.50.1.0' + hikariCpVersion = '6.3.0' junitJupiterVersion = '5.10.3' hamcrestVersion = '3.0' @@ -197,6 +198,7 @@ ext { micrometerExtra: "io.github.mweirauch:micrometer-jvm-extras:0.2.2", // SQLite - sqlite: "org.xerial:sqlite-jdbc:${sqliteVersion}" + sqlite: "org.xerial:sqlite-jdbc:${sqliteVersion}", + hikariCp: "com.zaxxer:HikariCP:${hikariCpVersion}" ] } diff --git a/scm-core/src/main/java/sonia/scm/store/QueryableMaintenanceStore.java b/scm-core/src/main/java/sonia/scm/store/QueryableMaintenanceStore.java index 88ec1075a1..d163cac6c0 100644 --- a/scm-core/src/main/java/sonia/scm/store/QueryableMaintenanceStore.java +++ b/scm-core/src/main/java/sonia/scm/store/QueryableMaintenanceStore.java @@ -34,7 +34,7 @@ import java.util.stream.Stream; * * @param The entity type of the store. */ -public interface QueryableMaintenanceStore { +public interface QueryableMaintenanceStore extends AutoCloseable { Collection> readAll() throws SerializationException; @@ -138,6 +138,9 @@ public interface QueryableMaintenanceStore { void update(Object object); } + @Override + void close(); + class SerializationException extends RuntimeException { public SerializationException(String message, Throwable cause) { super(message, cause); diff --git a/scm-persistence/build.gradle b/scm-persistence/build.gradle index f5d17587b5..1a4985acf2 100644 --- a/scm-persistence/build.gradle +++ b/scm-persistence/build.gradle @@ -23,6 +23,7 @@ dependencies { implementation libraries.commonsIo implementation libraries.commonsLang3 implementation libraries.sqlite + implementation libraries.hikariCp api platform(project(':')) diff --git a/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStoreFactory.java b/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStoreFactory.java index 620c64def3..859ff18804 100644 --- a/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStoreFactory.java +++ b/scm-persistence/src/main/java/sonia/scm/store/sqlite/SQLiteQueryableStoreFactory.java @@ -18,12 +18,14 @@ package sonia.scm.store.sqlite; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.annotations.VisibleForTesting; +import com.zaxxer.hikari.HikariConfig; +import com.zaxxer.hikari.HikariDataSource; import jakarta.inject.Inject; import jakarta.inject.Singleton; import lombok.extern.slf4j.Slf4j; -import org.sqlite.SQLiteConfig; -import org.sqlite.SQLiteDataSource; +import org.sqlite.JDBC; import sonia.scm.SCMContextProvider; +import sonia.scm.config.ConfigValue; import sonia.scm.plugin.PluginLoader; import sonia.scm.plugin.QueryableTypeDescriptor; import sonia.scm.security.KeyGenerator; @@ -48,6 +50,12 @@ import static com.fasterxml.jackson.databind.SerializationFeature.WRITE_DATE_TIM @Singleton public class SQLiteQueryableStoreFactory implements QueryableStoreFactory { + public static final String DEFAULT_MAX_POOL_SIZE = "10"; + public static final int MIN_IDLE = 0; + public static final String DEFAULT_CONNECTION_TIMEOUT_IN_SECONDS = "30"; + public static final String DEFAULT_IDLE_TIMEOUT_IN_SECONDS = "600"; + public static final String DEFAULT_MAX_LIFETIME_IN_SECONDS = "1800"; + public static final String DEFAULT_LEAK_DETECTION_THRESHOLD_IN_SECONDS = "30"; private final ObjectMapper objectMapper; private final KeyGenerator keyGenerator; private final DataSource dataSource; @@ -60,12 +68,23 @@ public class SQLiteQueryableStoreFactory implements QueryableStoreFactory { public SQLiteQueryableStoreFactory(SCMContextProvider contextProvider, PluginLoader pluginLoader, ObjectMapper objectMapper, - KeyGenerator keyGenerator) { + KeyGenerator keyGenerator, + @ConfigValue(key = "queryableStore.maxPoolSize", defaultValue = DEFAULT_MAX_POOL_SIZE) int maxPoolSize, + @ConfigValue(key = "queryableStore.connectionTimeout", defaultValue = DEFAULT_CONNECTION_TIMEOUT_IN_SECONDS) int connectionTimeoutInSeconds, + @ConfigValue(key = "queryableStore.idleTimeout", defaultValue = DEFAULT_IDLE_TIMEOUT_IN_SECONDS) int idleTimeoutInSeconds, + @ConfigValue(key = "queryableStore.maxLifetime", defaultValue = DEFAULT_MAX_LIFETIME_IN_SECONDS) int maxLifetimeInSeconds, + @ConfigValue(key = "queryableStore.leakDetectionThreshold", defaultValue = DEFAULT_LEAK_DETECTION_THRESHOLD_IN_SECONDS) int leakDetectionThresholdInSeconds + ) { this( - "jdbc:sqlite:" + contextProvider.resolve(Path.of("scm.db")), + "jdbc:sqlite:" + contextProvider.resolve(Path.of("scm.db?shared_cache=true&journal_mode=WAL")), objectMapper, keyGenerator, - pluginLoader.getExtensionProcessor().getQueryableTypes() + pluginLoader.getExtensionProcessor().getQueryableTypes(), + maxPoolSize, + connectionTimeoutInSeconds, + idleTimeoutInSeconds, + maxLifetimeInSeconds, + leakDetectionThresholdInSeconds ); } @@ -74,14 +93,27 @@ public class SQLiteQueryableStoreFactory implements QueryableStoreFactory { ObjectMapper objectMapper, KeyGenerator keyGenerator, Iterable queryableTypeIterable) { - SQLiteConfig config = new SQLiteConfig(); - config.setSharedCache(true); - config.setJournalMode(SQLiteConfig.JournalMode.WAL); + this(connectionString, objectMapper, keyGenerator, queryableTypeIterable, 10, 30, 600, 1800, 30); + } + + private SQLiteQueryableStoreFactory(String connectionString, + ObjectMapper objectMapper, + KeyGenerator keyGenerator, + Iterable queryableTypeIterable, + int maxPoolSize, + int connectionTimeoutInSeconds, + int idleTimeoutInSeconds, + int maxLifetimeInSeconds, + int leakDetectionThresholdInSeconds) { + HikariConfig config = createConnectionPoolConfig( + connectionString, + maxPoolSize, + connectionTimeoutInSeconds, + idleTimeoutInSeconds, + maxLifetimeInSeconds, + leakDetectionThresholdInSeconds); + this.dataSource = new HikariDataSource(config); - this.dataSource = new SQLiteDataSource( - config - ); - ((SQLiteDataSource) dataSource).setUrl(connectionString); this.objectMapper = objectMapper .copy() .configure(WRITE_DATES_AS_TIMESTAMPS, true) @@ -104,6 +136,27 @@ public class SQLiteQueryableStoreFactory implements QueryableStoreFactory { } } + private static HikariConfig createConnectionPoolConfig(String connectionString, + int maxPoolSize, + int connectionTimeoutInSeconds, + int idleTimeoutInSeconds, + int maxLifetimeInSeconds, + int leakDetectionThresholdInSeconds) { + HikariConfig config = new HikariConfig(); + config.setJdbcUrl(connectionString); + config.setMaximumPoolSize(maxPoolSize); + config.setMinimumIdle(MIN_IDLE); + config.setConnectionTimeout(connectionTimeoutInSeconds * 1000L); + config.setIdleTimeout(idleTimeoutInSeconds * 1000L); + config.setMaxLifetime(maxLifetimeInSeconds * 1000L); + config.setConnectionTestQuery("SELECT 1"); + config.setPoolName("SCMM_SQLitePool"); + config.setDriverClassName(JDBC.class.getName()); + // If a connection is held for longer than 30 seconds, HikariCP will log a warning: + config.setLeakDetectionThreshold(leakDetectionThresholdInSeconds * 1000L); + return config; + } + private Connection openDefaultConnection() { try { log.debug("open connection"); diff --git a/scm-queryable-test/src/main/java/sonia/scm/store/QueryableStoreExtension.java b/scm-queryable-test/src/main/java/sonia/scm/store/QueryableStoreExtension.java index 05634ffb3f..2991727ef2 100644 --- a/scm-queryable-test/src/main/java/sonia/scm/store/QueryableStoreExtension.java +++ b/scm-queryable-test/src/main/java/sonia/scm/store/QueryableStoreExtension.java @@ -47,7 +47,11 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.Collection; import java.util.HashSet; +import java.util.Map; +import java.util.Optional; import java.util.Set; +import java.util.function.BooleanSupplier; +import java.util.stream.Stream; import static java.util.Arrays.stream; @@ -62,7 +66,8 @@ public class QueryableStoreExtension implements ParameterResolver, BeforeEachCal private final Set> storeFactoryClasses = new HashSet<>(); private Path tempDirectory; private Collection queryableTypeDescriptors; - private SQLiteQueryableStoreFactory storeFactory; + private QueryableStoreFactory storeFactory; + private Collection createdStores; private static ObjectMapper getObjectMapper() { // this should be the same as in ObjectMapperProvider @@ -90,17 +95,23 @@ public class QueryableStoreExtension implements ParameterResolver, BeforeEachCal String connectionString = "jdbc:sqlite:" + tempDirectory.toString() + "/test.db"; queryableTypeDescriptors = new ArrayList<>(); addDescriptors(context); - storeFactory = new SQLiteQueryableStoreFactory( - connectionString, - mapper, - new UUIDKeyGenerator(), - queryableTypeDescriptors + createdStores = new ArrayList<>(); + storeFactory = new ClosedCheckingQueryableStoreFactory( + new SQLiteQueryableStoreFactory( + connectionString, + mapper, + new UUIDKeyGenerator(), + queryableTypeDescriptors + ) ); } @Override public void afterEach(ExtensionContext context) throws IOException { IOUtil.delete(tempDirectory.toFile()); + for (ClosedChecking store : createdStores) { + store.assertClosed(); + } } private void addDescriptors(ExtensionContext context) { @@ -165,4 +176,201 @@ public class QueryableStoreExtension implements ParameterResolver, BeforeEachCal public @interface QueryableTypes { Class[] value(); } + + private class ClosedCheckingQueryableStoreFactory implements QueryableStoreFactory { + private final QueryableStoreFactory delegate; + + ClosedCheckingQueryableStoreFactory(QueryableStoreFactory delegate) { + this.delegate = delegate; + } + + @Override + public QueryableMaintenanceStore getForMaintenance(Class clazz, String... parentIds) { + ClosedCheckingQueryableMaintenanceStore store = new ClosedCheckingQueryableMaintenanceStore<>(delegate.getForMaintenance(clazz, parentIds)); + createdStores.add(store); + return store; + } + + @Override + public QueryableStore getReadOnly(Class clazz, String... parentIds) { + ClosedCheckingQueryableStore store = new ClosedCheckingQueryableStore<>(delegate.getReadOnly(clazz, parentIds)); + createdStores.add(store); + return store; + } + + @Override + public QueryableMutableStore getMutable(Class clazz, String... parentIds) { + ClosedCheckingQueryableMutableStore store = new ClosedCheckingQueryableMutableStore<>(delegate.getMutable(clazz, parentIds)); + createdStores.add(store); + return store; + } + } + + private interface ClosedChecking { + default void assertClosed() { + if (!isClosed()) { + throw new IllegalStateException("Store has not been closed. Use stores in a try-with-resources block or call close() manually."); + } + } + + boolean isClosed(); + } + + private static class ClosedCheckingQueryableMaintenanceStore implements QueryableMaintenanceStore, ClosedChecking { + private final QueryableMaintenanceStore delegate; + + private boolean closed = false; + + ClosedCheckingQueryableMaintenanceStore(QueryableMaintenanceStore delegate) { + this.delegate = delegate; + } + + @Override + public void clear() { + delegate.clear(); + } + + @Override + public void close() { + delegate.close(); + closed = true; + } + + @Override + public MaintenanceIterator iterateAll() { + return delegate.iterateAll(); + } + + @Override + public Collection> readAll() throws SerializationException { + return delegate.readAll(); + } + + @Override + public Collection> readAllAs(Class type) throws SerializationException { + return delegate.readAllAs(type); + } + + @Override + public Collection readRaw() { + return delegate.readRaw(); + } + + @Override + public void writeAll(Iterable rows) throws SerializationException { + delegate.writeAll(rows); + } + + @Override + public void writeAll(Stream rows) throws SerializationException { + delegate.writeAll(rows); + } + + @Override + public void writeRaw(Iterable rows) { + delegate.writeRaw(rows); + } + + @Override + public void writeRaw(Stream rows) { + delegate.writeRaw(rows); + } + + @Override + public boolean isClosed() { + return closed; + } + } + + private static class ClosedCheckingQueryableStore implements QueryableStore, ClosedChecking { + private final QueryableStore delegate; + + private boolean closed = false; + + ClosedCheckingQueryableStore(QueryableStore delegate) { + this.delegate = delegate; + } + + @Override + public void close() { + delegate.close(); + closed = true; + } + + @Override + public Query query(Condition... conditions) { + return delegate.query(conditions); + } + + @Override + public boolean isClosed() { + return closed; + } + } + + private static class ClosedCheckingQueryableMutableStore implements QueryableMutableStore, ClosedChecking { + private final QueryableMutableStore delegate; + + private boolean closed = false; + + ClosedCheckingQueryableMutableStore(QueryableMutableStore delegate) { + this.delegate = delegate; + } + + @Override + public void close() { + delegate.close(); + closed = true; + } + + @Override + public MutableQuery query(Condition... conditions) { + return delegate.query(conditions); + } + + @Override + public void transactional(BooleanSupplier callback) { + delegate.transactional(callback); + } + + @Override + public Map getAll() { + return delegate.getAll(); + } + + @Override + public void put(String id, T item) { + delegate.put(id, item); + } + + @Override + public String put(T item) { + return delegate.put(item); + } + + @Override + public void clear() { + delegate.clear(); + } + + @Override + public T get(String id) { + return delegate.get(id); + } + + @Override + public Optional getOptional(String id) { + return delegate.getOptional(id); + } + + @Override + public void remove(String id) { + delegate.remove(id); + } + + @Override + public boolean isClosed() { + return closed; + } + } } diff --git a/scm-queryable-test/src/test/java/sonia/scm/store/QueryableStoreExtensionTest.java b/scm-queryable-test/src/test/java/sonia/scm/store/QueryableStoreExtensionTest.java index 78f387b310..fd5d03a2d3 100644 --- a/scm-queryable-test/src/test/java/sonia/scm/store/QueryableStoreExtensionTest.java +++ b/scm-queryable-test/src/test/java/sonia/scm/store/QueryableStoreExtensionTest.java @@ -27,16 +27,18 @@ class QueryableStoreExtensionTest { @Test void shouldProvideQueryableStoreFactory(QueryableStoreFactory storeFactory) { - QueryableMutableStore store = storeFactory.getMutable(Spaceship.class); - store.put(new Spaceship("Heart Of Gold")); - assertEquals(1, store.getAll().size()); + try (QueryableMutableStore store = storeFactory.getMutable(Spaceship.class)) { + store.put(new Spaceship("Heart Of Gold")); + assertEquals(1, store.getAll().size()); + } } @Test void shouldProvideTypeRelatedStoreFactory(SpaceshipStoreFactory storeFactory) { - QueryableMutableStore store = storeFactory.getMutable(); - store.put(new Spaceship("Heart Of Gold")); - assertEquals(1, store.getAll().size()); + try (QueryableMutableStore store = storeFactory.getMutable()) { + store.put(new Spaceship("Heart Of Gold")); + assertEquals(1, store.getAll().size()); + } } } diff --git a/scm-webapp/src/main/java/sonia/scm/importexport/RepositoryQueryableStoreExporter.java b/scm-webapp/src/main/java/sonia/scm/importexport/RepositoryQueryableStoreExporter.java index ff68856d65..e1da7c27d9 100644 --- a/scm-webapp/src/main/java/sonia/scm/importexport/RepositoryQueryableStoreExporter.java +++ b/scm-webapp/src/main/java/sonia/scm/importexport/RepositoryQueryableStoreExporter.java @@ -85,7 +85,10 @@ public class RepositoryQueryableStoreExporter { JAXBContext jaxbContext = JAXBContext.newInstance(StoreExport.class); Marshaller marshaller = jaxbContext.createMarshaller(); for (Class type : metaDataProvider.getTypesWithParent(Repository.class)) { - Collection rows = storeFactory.getForMaintenance(type, repositoryId).readRaw(); + Collection rows; + try (QueryableMaintenanceStore store = storeFactory.getForMaintenance(type, repositoryId)) { + rows = store.readRaw(); + } StoreExport export = new StoreExport(type, rows); marshaller.marshal(export, new File(workdir, type.getName() + ".xml")); } @@ -116,7 +119,9 @@ public class RepositoryQueryableStoreExporter { continue; } - storeFactory.getForMaintenance(type, repositoryId).writeRaw(rows); + try (QueryableMaintenanceStore store = storeFactory.getForMaintenance(type, repositoryId)) { + store.writeRaw(rows); + } try { Files.delete(file.toPath()); diff --git a/scm-webapp/src/main/java/sonia/scm/store/QueryableStoreDeletionHandler.java b/scm-webapp/src/main/java/sonia/scm/store/QueryableStoreDeletionHandler.java index c0082d2471..b099e1a092 100644 --- a/scm-webapp/src/main/java/sonia/scm/store/QueryableStoreDeletionHandler.java +++ b/scm-webapp/src/main/java/sonia/scm/store/QueryableStoreDeletionHandler.java @@ -46,6 +46,10 @@ class QueryableStoreDeletionHandler implements StoreDeletionNotifier.DeletionHan ids[i] = classWithIds[i].id(); } Collection> typesWithParent = metaDataProvider.getTypesWithParent(classes); - typesWithParent.forEach(type -> storeFactory.getForMaintenance(type, ids).clear()); + typesWithParent.forEach(type -> { + try (QueryableMaintenanceStore store = storeFactory.getForMaintenance(type, ids)) { + store.clear(); + } + }); } } diff --git a/scm-webapp/src/test/java/sonia/scm/importexport/RepositoryQueryableStoreExporterTest.java b/scm-webapp/src/test/java/sonia/scm/importexport/RepositoryQueryableStoreExporterTest.java index fd75182e12..dac5600fa6 100644 --- a/scm-webapp/src/test/java/sonia/scm/importexport/RepositoryQueryableStoreExporterTest.java +++ b/scm-webapp/src/test/java/sonia/scm/importexport/RepositoryQueryableStoreExporterTest.java @@ -25,6 +25,7 @@ import org.junit.jupiter.api.io.TempDir; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; import sonia.scm.repository.Repository; +import sonia.scm.store.QueryableMutableStore; import sonia.scm.store.QueryableStoreExtension; import sonia.scm.store.QueryableStoreFactory; import sonia.scm.store.StoreMetaDataProvider; @@ -47,20 +48,25 @@ class RepositoryQueryableStoreExporterTest { @Mock private StoreMetaDataProvider storeMetaDataProvider; + private RepositoryQueryableStoreExporter exporter; + @BeforeEach - void initMetaDataProvider() { + void initExporter(QueryableStoreFactory storeFactory) { lenient().when(storeMetaDataProvider.getTypesWithParent(Repository.class)).thenReturn(List.of(SimpleType.class, SimpleTypeWithTwoParents.class)); + exporter = new RepositoryQueryableStoreExporter(storeMetaDataProvider, storeFactory); } @Nested class ExportStores { @Test - void shouldExportSimpleType(QueryableStoreFactory storeFactory, SimpleTypeStoreFactory simpleTypeStoreFactory, @TempDir java.nio.file.Path tempDir) { - simpleTypeStoreFactory.getMutable("23").put("1", new SimpleType("hack")); - simpleTypeStoreFactory.getMutable("42").put("1", new SimpleType("hitchhike")); - simpleTypeStoreFactory.getMutable("42").put("2", new SimpleType("heart of gold")); - - RepositoryQueryableStoreExporter exporter = new RepositoryQueryableStoreExporter(storeMetaDataProvider, storeFactory); + void shouldExportSimpleType(SimpleTypeStoreFactory simpleTypeStoreFactory, @TempDir java.nio.file.Path tempDir) { + try (QueryableMutableStore store = simpleTypeStoreFactory.getMutable("23")) { + store.put("1", new SimpleType("hack")); + } + try (QueryableMutableStore store = simpleTypeStoreFactory.getMutable("42")) { + store.put("1", new SimpleType("hitchhike")); + store.put("2", new SimpleType("heart of gold")); + } exporter.exportStores("42", tempDir.toFile()); @@ -68,12 +74,14 @@ class RepositoryQueryableStoreExporterTest { } @Test - void shouldExportTypeWithTwoParents(QueryableStoreFactory storeFactory, SimpleTypeWithTwoParentsStoreFactory simpleTypeStoreFactory, @TempDir java.nio.file.Path tempDir) { - simpleTypeStoreFactory.getMutable("23", "1").put("1", new SimpleTypeWithTwoParents("hack")); - simpleTypeStoreFactory.getMutable("42", "1").put("1", new SimpleTypeWithTwoParents("hitchhike")); - simpleTypeStoreFactory.getMutable("42", "1").put("2", new SimpleTypeWithTwoParents("heart of gold")); - - RepositoryQueryableStoreExporter exporter = new RepositoryQueryableStoreExporter(storeMetaDataProvider, storeFactory); + void shouldExportTypeWithTwoParents(SimpleTypeWithTwoParentsStoreFactory simpleTypeStoreFactory, @TempDir java.nio.file.Path tempDir) { + try (QueryableMutableStore store = simpleTypeStoreFactory.getMutable("23", "1")) { + store.put("1", new SimpleTypeWithTwoParents("hack")); + } + try (QueryableMutableStore store = simpleTypeStoreFactory.getMutable("42", "1")) { + store.put("1", new SimpleTypeWithTwoParents("hitchhike")); + store.put("2", new SimpleTypeWithTwoParents("heart of gold")); + } exporter.exportStores("42", tempDir.toFile()); @@ -96,68 +104,75 @@ class RepositoryQueryableStoreExporterTest { } @Test - void shouldImportSimpleType(QueryableStoreFactory storeFactory, SimpleTypeStoreFactory simpleTypeStoreFactory) throws IOException { - simpleTypeStoreFactory.getMutable("23").put("1", new SimpleType("hack")); + void shouldImportSimpleType(SimpleTypeStoreFactory simpleTypeStoreFactory) throws IOException { + try (QueryableMutableStore store = simpleTypeStoreFactory.getMutable("23")) { + store.put("1", new SimpleType("hack")); + } URL url = Resources.getResource("sonia/scm/importexport/SimpleType.xml"); Files.createFile(queryableStoreDir.toPath().resolve("sonia.scm.importexport.SimpleType.xml")); Files.writeString(queryableStoreDir.toPath().resolve("sonia.scm.importexport.SimpleType.xml"), Resources.toString(url, StandardCharsets.UTF_8)); - RepositoryQueryableStoreExporter exporter = new RepositoryQueryableStoreExporter(storeMetaDataProvider, storeFactory); - exporter.importStores("42", tempDir); - assertThat(simpleTypeStoreFactory.getMutable("42").getAll()).hasSize(2); + try (QueryableMutableStore store = simpleTypeStoreFactory.getMutable("42")) { + assertThat(store.getAll()).hasSize(2); + } } @Test - void shouldImportTypeWithTwoParents(QueryableStoreFactory storeFactory, SimpleTypeWithTwoParentsStoreFactory simpleTypeStoreFactory) throws IOException { - simpleTypeStoreFactory.getMutable("23", "1").put("1", new SimpleTypeWithTwoParents("hack")); + void shouldImportTypeWithTwoParents(SimpleTypeWithTwoParentsStoreFactory simpleTypeStoreFactory) throws IOException { + try (QueryableMutableStore store = simpleTypeStoreFactory.getMutable("23", "1")) { + store.put("1", new SimpleTypeWithTwoParents("hack")); + } URL url = Resources.getResource("sonia/scm/importexport/SimpleTypeWithTwoParents.xml"); Files.writeString(queryableStoreDir.toPath().resolve("sonia.scm.importexport.SimpleTypeWithTwoParents.xml"), Resources.toString(url, StandardCharsets.UTF_8)); - RepositoryQueryableStoreExporter exporter = new RepositoryQueryableStoreExporter(storeMetaDataProvider, storeFactory); exporter.importStores("42", tempDir); - assertThat(simpleTypeStoreFactory.getMutable("42", "1").getAll()).hasSize(2); + try (QueryableMutableStore store = simpleTypeStoreFactory.getMutable("42", "1")) { + assertThat(store.getAll()).hasSize(2); + } } @Test - void shouldNotImportWhenFileDoesNotExist(QueryableStoreFactory storeFactory, SimpleTypeStoreFactory simpleTypeStoreFactory) { - simpleTypeStoreFactory.getMutable("23").put("1", new SimpleType("hack")); + void shouldNotImportWhenFileDoesNotExist(SimpleTypeStoreFactory simpleTypeStoreFactory) { + try (QueryableMutableStore store = simpleTypeStoreFactory.getMutable("23")) { + store.put("1", new SimpleType("hack")); + } File nonExistentFile = queryableStoreDir.toPath().resolve("sonia.scm.importexport.SimpleType.xml").toFile(); assertThat(nonExistentFile).doesNotExist(); - RepositoryQueryableStoreExporter exporter = new RepositoryQueryableStoreExporter(storeMetaDataProvider, storeFactory); exporter.importStores("42", tempDir); - assertThat(simpleTypeStoreFactory.getMutable("42").getAll()).isEmpty(); + try (QueryableMutableStore store = simpleTypeStoreFactory.getMutable("42")) { + assertThat(store.getAll()).isEmpty(); + } } @Test - void shouldThrowExceptionForMalformedXML(QueryableStoreFactory storeFactory) throws IOException { + void shouldThrowExceptionForMalformedXML() throws IOException { Files.writeString(queryableStoreDir.toPath().resolve("sonia.scm.importexport.SimpleType.xml"), ""); - RepositoryQueryableStoreExporter exporter = new RepositoryQueryableStoreExporter(storeMetaDataProvider, storeFactory); - assertThrows(RuntimeException.class, () -> exporter.importStores("42", tempDir)); } @Test - void shouldNotImportFromEmptyFile(QueryableStoreFactory storeFactory, SimpleTypeStoreFactory simpleTypeStoreFactory) throws IOException { - simpleTypeStoreFactory.getMutable("42").put("1", new SimpleType("existing data")); + void shouldNotImportFromEmptyFile(SimpleTypeStoreFactory simpleTypeStoreFactory) throws IOException { + try (QueryableMutableStore store = simpleTypeStoreFactory.getMutable("42")) { + store.put("1", new SimpleType("existing data")); - Files.createFile(queryableStoreDir.toPath().resolve("sonia.scm.importexport.SimpleType.xml")); + Files.createFile(queryableStoreDir.toPath().resolve("sonia.scm.importexport.SimpleType.xml")); - RepositoryQueryableStoreExporter exporter = new RepositoryQueryableStoreExporter(storeMetaDataProvider, storeFactory); - exporter.importStores("42", tempDir); + exporter.importStores("42", tempDir); - SimpleType simpleType = simpleTypeStoreFactory.getMutable("42").get("1"); + SimpleType simpleType = store.get("1"); - assertThat(simpleType) - .extracting("someField") - .isEqualTo("existing data"); + assertThat(simpleType) + .extracting("someField") + .isEqualTo("existing data"); + } } } } diff --git a/scm-webapp/src/test/java/sonia/scm/store/QueryableStoreDeletionHandlerTest.java b/scm-webapp/src/test/java/sonia/scm/store/QueryableStoreDeletionHandlerTest.java new file mode 100644 index 0000000000..444f00f5b2 --- /dev/null +++ b/scm-webapp/src/test/java/sonia/scm/store/QueryableStoreDeletionHandlerTest.java @@ -0,0 +1,121 @@ +/* + * Copyright (c) 2020 - present Cloudogu GmbH + * + * This program is free software: you can redistribute it and/or modify it under + * the terms of the GNU Affero General Public License as published by the Free + * Software Foundation, version 3. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see https://www.gnu.org/licenses/. + */ + +package sonia.scm.store; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import sonia.scm.group.Group; +import sonia.scm.user.User; + +import java.util.Set; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.when; + +@ExtendWith({QueryableStoreExtension.class, MockitoExtension.class}) +@QueryableStoreExtension.QueryableTypes({QueryableTypeWithGroupParent.class, QueryableTypeWithMultipleParents.class}) +class QueryableStoreDeletionHandlerTest { + + @Mock + private StoreMetaDataProvider metaDataProvider; + + @Nested + class WithSingleParent { + + private QueryableStoreDeletionHandler queryableStoreDeletionHandler; + + @BeforeEach + void setUpHandler(QueryableStoreFactory storeFactory) { + when(metaDataProvider.getTypesWithParent(Group.class)) + .thenReturn(Set.of(QueryableTypeWithGroupParent.class)); + + queryableStoreDeletionHandler = new QueryableStoreDeletionHandler(Set.of(), metaDataProvider, storeFactory); + } + + @Test + void shouldDeleteStoresWithParent(QueryableTypeWithGroupParentStoreFactory groupParentStoreFactory) { + try (QueryableMutableStore groupParentStore = groupParentStoreFactory.getMutable("earth")) { + groupParentStore.put(new QueryableTypeWithGroupParent()); + + queryableStoreDeletionHandler.notifyDeleted(Group.class, "earth"); + + assertThat(groupParentStore.getAll()) + .isEmpty(); + } + } + + @Test + void shouldKeepStoresWithOtherParent(QueryableTypeWithGroupParentStoreFactory groupParentStoreFactory) { + try (QueryableMutableStore groupParentStore = groupParentStoreFactory.getMutable("hog")) { + groupParentStore.put(new QueryableTypeWithGroupParent()); + + queryableStoreDeletionHandler.notifyDeleted(Group.class, "earth"); + + assertThat(groupParentStore.getAll()) + .hasSize(1); + } + } + } + + @Nested + class WithMultipleParents { + + private QueryableStoreDeletionHandler queryableStoreDeletionHandler; + + @BeforeEach + void setUpHandler(QueryableStoreFactory storeFactory) { + queryableStoreDeletionHandler = new QueryableStoreDeletionHandler(Set.of(), metaDataProvider, storeFactory); + } + + @Test + void shouldDeleteStoresWhenSingleParentDeleted(QueryableTypeWithMultipleParentsStoreFactory storeFactory) { + when(metaDataProvider.getTypesWithParent(Group.class)) + .thenReturn(Set.of(QueryableTypeWithMultipleParents.class)); + + try (QueryableMutableStore store = storeFactory.getMutable("earth", "dent", "house")) { + store.put(new QueryableTypeWithMultipleParents()); + + queryableStoreDeletionHandler.notifyDeleted(Group.class, "earth"); + + assertThat(store.getAll()) + .isEmpty(); + } + } + + @Test + void shouldDeleteStoresWhenParentChantDeleted(QueryableTypeWithMultipleParentsStoreFactory storeFactory) { + when(metaDataProvider.getTypesWithParent(Group.class, User.class)) + .thenReturn(Set.of(QueryableTypeWithMultipleParents.class)); + + try (QueryableMutableStore store = storeFactory.getMutable("earth", "dent", "house")) { + store.put(new QueryableTypeWithMultipleParents()); + + queryableStoreDeletionHandler.notifyDeleted( + new StoreDeletionNotifier.ClassWithId(Group.class, "earth"), + new StoreDeletionNotifier.ClassWithId(User.class, "dent") + ); + + assertThat(store.getAll()) + .isEmpty(); + } + } + } +} diff --git a/scm-webapp/src/test/java/sonia/scm/store/QueryableTypeWithGroupParent.java b/scm-webapp/src/test/java/sonia/scm/store/QueryableTypeWithGroupParent.java new file mode 100644 index 0000000000..8d40f54441 --- /dev/null +++ b/scm-webapp/src/test/java/sonia/scm/store/QueryableTypeWithGroupParent.java @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2020 - present Cloudogu GmbH + * + * This program is free software: you can redistribute it and/or modify it under + * the terms of the GNU Affero General Public License as published by the Free + * Software Foundation, version 3. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see https://www.gnu.org/licenses/. + */ + +package sonia.scm.store; + +import lombok.Data; +import sonia.scm.group.Group; + +@Data +@QueryableType({Group.class}) +public class QueryableTypeWithGroupParent { + private String someValue; +} diff --git a/scm-webapp/src/test/java/sonia/scm/store/QueryableTypeWithMultipleParents.java b/scm-webapp/src/test/java/sonia/scm/store/QueryableTypeWithMultipleParents.java new file mode 100644 index 0000000000..0188c850d6 --- /dev/null +++ b/scm-webapp/src/test/java/sonia/scm/store/QueryableTypeWithMultipleParents.java @@ -0,0 +1,28 @@ +/* + * Copyright (c) 2020 - present Cloudogu GmbH + * + * This program is free software: you can redistribute it and/or modify it under + * the terms of the GNU Affero General Public License as published by the Free + * Software Foundation, version 3. + * + * This program is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS + * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more + * details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see https://www.gnu.org/licenses/. + */ + +package sonia.scm.store; + +import lombok.Data; +import sonia.scm.group.Group; +import sonia.scm.repository.Repository; +import sonia.scm.user.User; + +@Data +@QueryableType({Group.class, User.class, Repository.class}) +public class QueryableTypeWithMultipleParents { + private String someValue; +}