AJ-1653: remove initialize-collection-on-startup; add integration tes…

…t cleanup (#588)

service/src/integrationTest/java/org/databiosphere/workspacedataservice/IntegrationServiceTestBase.java

@@ -0,0 +1,60 @@
+package org.databiosphere.workspacedataservice;
+
+import java.util.Map;
+import java.util.UUID;
+import org.databiosphere.workspacedataservice.dao.CollectionDao;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
+
+public class IntegrationServiceTestBase {
+
+  /**
+   * Reusable utility to wipe the db of all user state created by integration tests, while leaving
+   * in place the sys_wds.* table definitions (i.e. don't need to re-run Liquibase)
+   *
+   * @param collectionDao dao to use for collection management
+   * @param namedTemplate to use for direct SQL queries
+   */
+  protected void cleanDb(CollectionDao collectionDao, NamedParameterJdbcTemplate namedTemplate) {
+    // drop all rows from backup, clone, restore tables
+    // may need to add the job table at some point; current integration tests do not write to it
+    namedTemplate.getJdbcTemplate().update("delete from sys_wds.backup");
+    namedTemplate.getJdbcTemplate().update("delete from sys_wds.clone");
+    namedTemplate.getJdbcTemplate().update("delete from sys_wds.restore");
+
+    // delete all collections that are currently known to WDS
+    collectionDao.listCollectionSchemas().forEach(collectionDao::dropSchema);
+
+    // and drop any orphaned Postgres schemas
+    dropOrphanedSchemas(namedTemplate);
+  }
+
+  /**
+   * Clean the db of any orphaned Postgres schemas - schemas that were created by WDS as a
+   * collection but which do not have a corresponding row in sys_wds.collection. This can happen
+   * when restores fail.
+   *
+   * <p>This is a standalone method in order to @SuppressWarnings on it in a targeted fashion. This
+   * method uses string concatenation to build a SQL statement, since we cannot use bind params in a
+   * `drop schema` statement. Since the value we are concatenating already validated as a UUID, we
+   * know it does not contain SQL injection/escape sequences.
+   *
+   * @param namedTemplate to use for direct SQL queries
+   */
+  @SuppressWarnings("squid:S2077")
+  private void dropOrphanedSchemas(NamedParameterJdbcTemplate namedTemplate) {
+    namedTemplate
+        .queryForList(
+            "SELECT schema_name FROM information_schema.schemata;", Map.of(), String.class)
+        .forEach(
+            schemaName -> {
+              // is this a UUID? We only want to drop orphaned schemas whose name is a UUID;
+              // we don't want to drop other critical schemas like "public" or "sys_wds"
+              try {
+                UUID schemaUuid = UUID.fromString(schemaName);
+                namedTemplate.update("DROP schema \"" + schemaUuid + "\" cascade;", Map.of());
+              } catch (IllegalArgumentException iae) {
+                // schema name was not a valid uuid; we should not drop this schema.
+              }
+            });
+  }
+}

service/src/integrationTest/java/org/databiosphere/workspacedataservice/service/BackupRestoreServiceFailureIntegrationTest.java

@@ -5,15 +5,19 @@
 import static org.junit.jupiter.api.Assertions.assertSame;
 
 import java.util.UUID;
+import org.databiosphere.workspacedataservice.IntegrationServiceTestBase;
+import org.databiosphere.workspacedataservice.dao.CollectionDao;
 import org.databiosphere.workspacedataservice.shared.model.BackupRestoreRequest;
 import org.databiosphere.workspacedataservice.shared.model.RestoreResponse;
 import org.databiosphere.workspacedataservice.shared.model.job.Job;
 import org.databiosphere.workspacedataservice.shared.model.job.JobInput;
 import org.databiosphere.workspacedataservice.shared.model.job.JobStatus;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
 import org.springframework.test.annotation.DirtiesContext;
 import org.springframework.test.context.ActiveProfiles;
 import org.springframework.test.context.ContextConfiguration;
@@ -30,12 +34,20 @@
       "twds.instance.source-workspace-id=123e4567-e89b-12d3-a456-426614174001",
       "twds.pg_dump.host="
     })
-class BackupRestoreServiceFailureIntegrationTest {
+class BackupRestoreServiceFailureIntegrationTest extends IntegrationServiceTestBase {
   @Autowired private BackupRestoreService backupRestoreService;
+  @Autowired CollectionDao collectionDao;
+  @Autowired NamedParameterJdbcTemplate namedTemplate;
 
   @Value("${twds.instance.source-workspace-id}")
   private String sourceWorkspaceId;
 
+  // ensure we clean up the db after our tests
+  @AfterEach
+  void cleanUp() {
+    cleanDb(collectionDao, namedTemplate);
+  }
+
   @Test
   void testRestoreAzureWDSErrorHandling() {
     Job<JobInput, RestoreResponse> response =

service/src/integrationTest/java/org/databiosphere/workspacedataservice/service/BackupServiceIntegrationTest.java

@@ -4,29 +4,42 @@
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 
 import java.util.UUID;
+import org.databiosphere.workspacedataservice.IntegrationServiceTestBase;
 import org.databiosphere.workspacedataservice.dao.BackupRestoreDao;
+import org.databiosphere.workspacedataservice.dao.CollectionDao;
 import org.databiosphere.workspacedataservice.shared.model.BackupResponse;
 import org.databiosphere.workspacedataservice.shared.model.BackupRestoreRequest;
 import org.databiosphere.workspacedataservice.shared.model.job.JobStatus;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
+import org.springframework.test.annotation.DirtiesContext;
 import org.springframework.test.context.ActiveProfiles;
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.context.TestPropertySource;
 
 @ActiveProfiles({"mock-storage", "local-cors", "local", "data-plane"})
 @ContextConfiguration(name = "mockStorage")
+@DirtiesContext
 @SpringBootTest
 @TestPropertySource(
     properties = {
       "twds.instance.workspace-id=123e4567-e89b-12d3-a456-426614174000",
       "twds.pg_dump.useAzureIdentity=false"
     })
-class BackupServiceIntegrationTest {
+class BackupServiceIntegrationTest extends IntegrationServiceTestBase {
   @Autowired private BackupRestoreService backupRestoreService;
-
   @Autowired private BackupRestoreDao<BackupResponse> backupDao;
+  @Autowired CollectionDao collectionDao;
+  @Autowired NamedParameterJdbcTemplate namedTemplate;
+
+  // ensure we clean up the db after our tests
+  @AfterEach
+  void cleanUp() {
+    cleanDb(collectionDao, namedTemplate);
+  }
 
   @Test
   void testBackupAzureWDS() {

service/src/integrationTest/java/org/databiosphere/workspacedataservice/service/RestoreServiceIntegrationTest.java

@@ -6,55 +6,52 @@
 
 import java.util.List;
 import java.util.UUID;
+import org.databiosphere.workspacedataservice.IntegrationServiceTestBase;
 import org.databiosphere.workspacedataservice.dao.CollectionDao;
 import org.databiosphere.workspacedataservice.dao.RecordDao;
 import org.databiosphere.workspacedataservice.shared.model.RecordType;
 import org.databiosphere.workspacedataservice.shared.model.job.JobStatus;
-import org.junit.jupiter.api.AfterAll;
-import org.junit.jupiter.api.BeforeEach;
+import org.databiosphere.workspacedataservice.startup.CollectionInitializer;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.TestInstance;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.boot.test.context.SpringBootTest;
+import org.springframework.boot.test.mock.mockito.MockBean;
+import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
 import org.springframework.test.annotation.DirtiesContext;
 import org.springframework.test.context.ActiveProfiles;
 import org.springframework.test.context.ContextConfiguration;
 import org.springframework.test.context.TestPropertySource;
 
 @ActiveProfiles({"mock-storage", "local-cors", "local", "data-plane"})
 @ContextConfiguration(name = "mockStorage")
-@TestInstance(TestInstance.Lifecycle.PER_CLASS)
 @DirtiesContext
 @SpringBootTest
 @TestPropertySource(
     properties = {
-      "twds.instance.initialize-collection-on-startup=false",
       "twds.instance.workspace-id=123e4567-e89b-12d3-a456-426614174000",
       "twds.instance.source-workspace-id=10000000-0000-0000-0000-000000000111",
       "twds.pg_dump.useAzureIdentity=false"
     })
-class RestoreServiceIntegrationTest {
-  @Autowired private BackupRestoreService backupRestoreService;
+class RestoreServiceIntegrationTest extends IntegrationServiceTestBase {
 
+  @Autowired private BackupRestoreService backupRestoreService;
   @Autowired CollectionDao collectionDao;
-
+  @Autowired NamedParameterJdbcTemplate namedTemplate;
   @Autowired RecordDao recordDao;
 
+  // Don't run the CollectionInitializer on startup, so this test can start with a clean slate.
+  // By making an (empty) mock bean to replace CollectionInitializer, we ensure it is a noop.
+  @MockBean CollectionInitializer mockCollectionInitializer;
+
   @Value("${twds.instance.workspace-id:}")
   private String workspaceId;
 
-  // this @BeforeEach makes the initialize-collection-on-startup property redundant, but is a
-  // workaround for integration test cleanup
-  @BeforeEach
-  @AfterAll
-  void tearDown() {
-    // clean up any collection left in the db
-    List<UUID> allCollections = collectionDao.listCollectionSchemas();
-    allCollections.forEach(collectionId -> collectionDao.dropSchema(collectionId));
-    // TODO: also drop any orphaned pg schemas that don't have an entry in the sys_wds.collection
-    // table.
-    // this can happen when restores fail.
+  // ensure we clean up the db after our tests
+  @AfterEach
+  void cleanUp() {
+    cleanDb(collectionDao, namedTemplate);
   }
 
   // this test references the file src/integrationTest/resources/backup-test.sql as its backup

service/src/integrationTest/java/org/databiosphere/workspacedataservice/startup/CollectionInitializerCloneTest.java

@@ -14,6 +14,7 @@
 import org.databiosphere.workspacedata.client.ApiException;
 import org.databiosphere.workspacedata.model.BackupJob;
 import org.databiosphere.workspacedata.model.BackupResponse;
+import org.databiosphere.workspacedataservice.IntegrationServiceTestBase;
 import org.databiosphere.workspacedataservice.dao.CloneDao;
 import org.databiosphere.workspacedataservice.dao.CollectionDao;
 import org.databiosphere.workspacedataservice.dao.RecordDao;
@@ -26,10 +27,8 @@
 import org.databiosphere.workspacedataservice.shared.model.job.JobStatus;
 import org.databiosphere.workspacedataservice.sourcewds.WorkspaceDataServiceClientFactory;
 import org.databiosphere.workspacedataservice.workspacemanager.WorkspaceManagerDao;
-import org.junit.jupiter.api.AfterAll;
-import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.TestInstance;
 import org.mockito.Mockito;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
@@ -46,16 +45,14 @@
 @ActiveProfiles({"mock-storage", "local-cors", "mock-sam", "local", "data-plane"})
 @TestPropertySource(
     properties = {
-      "twds.instance.initialize-collection-on-startup=false",
       "twds.instance.workspace-id=5a9b583c-17ee-4c88-a14c-0edbf31175db",
       // source id must match value in WDS-integrationTest-LocalFileStorage-input.sql
       "twds.instance.source-workspace-id=10000000-0000-0000-0000-000000000111",
       "twds.pg_dump.useAzureIdentity=false"
     })
-@TestInstance(TestInstance.Lifecycle.PER_CLASS)
 @DirtiesContext
 @SpringBootTest
-class CollectionInitializerCloneTest {
+class CollectionInitializerCloneTest extends IntegrationServiceTestBase {
 
   // standard beans
   @Autowired CollectionInitializerBean collectionInitializerBean;
@@ -69,26 +66,21 @@ class CollectionInitializerCloneTest {
   @MockBean LeonardoDao mockLeonardoDao;
   @MockBean WorkspaceManagerDao workspaceManagerDao;
 
+  // Don't run the CollectionInitializer on startup, so this test can start with a clean slate.
+  // By making an (empty) mock bean to replace CollectionInitializer, we ensure it is a noop.
+  @MockBean CollectionInitializer mockCollectionInitializer;
+
   // values
   @Value("${twds.instance.workspace-id}")
   String workspaceId;
 
   @Value("${twds.instance.source-workspace-id}")
   String sourceWorkspaceId;
 
-  // this @BeforeEach makes the initialize-collection-on-startup property redundant, but is a
-  // workaround for integration test cleanup
-  @BeforeEach
-  @AfterAll
-  void tearDown() {
-    // clean up any collections left in the db
-    List<UUID> allCollections = collectionDao.listCollectionSchemas();
-    allCollections.forEach(collectionId -> collectionDao.dropSchema(collectionId));
-    // clean up any clone entries
-    namedTemplate.getJdbcTemplate().update("delete from sys_wds.clone");
-    // TODO: also drop any orphaned pg schemas that don't have an entry in the sys_wds.collection
-    // table.
-    // this can happen when restores fail.
+  // ensure we clean up the db after our tests
+  @AfterEach
+  void cleanUp() {
+    cleanDb(collectionDao, namedTemplate);
   }
 
   /*

service/src/main/java/org/databiosphere/workspacedataservice/config/InstanceProperties.java

@@ -12,7 +12,6 @@
  */
 public class InstanceProperties {
   private boolean validWorkspaceId = false; // assume false until configured otherwise
-  private boolean initializeCollectionOnStartup;
   private WorkspaceId workspaceId;
   @Nullable private WorkspaceId sourceWorkspaceId;
 
@@ -56,12 +55,4 @@ void setSourceWorkspaceId(String sourceWorkspaceId) {
       }
     }
   }
-
-  void setInitializeCollectionOnStartup(boolean initializeCollectionOnStartup) {
-    this.initializeCollectionOnStartup = initializeCollectionOnStartup;
-  }
-
-  public boolean getInitializeCollectionOnStartup() {
-    return initializeCollectionOnStartup;
-  }
 }

service/src/main/java/org/databiosphere/workspacedataservice/startup/CollectionInitializer.java

@@ -1,7 +1,6 @@
 package org.databiosphere.workspacedataservice.startup;
 
 import org.databiosphere.workspacedataservice.annotations.DeploymentMode.DataPlane;
-import org.databiosphere.workspacedataservice.config.InstanceProperties;
 import org.jetbrains.annotations.NotNull;
 import org.springframework.context.ApplicationListener;
 import org.springframework.context.event.ContextRefreshedEvent;
@@ -12,18 +11,13 @@
 public class CollectionInitializer implements ApplicationListener<ContextRefreshedEvent> {
 
   private final CollectionInitializerBean collectionInitializerBean;
-  private final boolean runOnStartup;
 
-  public CollectionInitializer(
-      CollectionInitializerBean collectionInitializerBean, InstanceProperties instanceProperties) {
+  public CollectionInitializer(CollectionInitializerBean collectionInitializerBean) {
     this.collectionInitializerBean = collectionInitializerBean;
-    this.runOnStartup = instanceProperties.getInitializeCollectionOnStartup();
   }
 
   @Override
   public void onApplicationEvent(@NotNull ContextRefreshedEvent event) {
-    if (runOnStartup) {
-      collectionInitializerBean.initializeCollection();
-    }
+    collectionInitializerBean.initializeCollection();
   }
 }

service/src/main/resources/application.yml

@@ -120,7 +120,6 @@ twds:
   write.batch.size: 5000
   streaming.fetch.size: 5000
   instance:
-    initialize-collection-on-startup: true
     # Workspace Id for launching instance
     workspace-id: ${WORKSPACE_ID:}
     source-workspace-id: ${SOURCE_WORKSPACE_ID:}