Remove production use of DatabaseConfigPersistence #19275

cgardens · 2022-11-10T05:14:57Z

Depends on PR to add new db base test calss: #19640

What

Removes DatabaseConfigPersistence!!!

How

It does this by moving relevant database queries into the ConfigRepository
Took this opportunity to start to split tests into their own classes by resource (e.g. workspace, actor definition). I did not break resources into their own persistences. That's too much to do in one PR, though we should do that next. I did the tests, because otherwise I couldn't wade through what was going on safely enough.
Removes test classes that were just testing ConfigPersistence. Moved any relevant business logic tests that were hidden in there (which there were a few, which is kinda scary because those tests were testing the old ConfigPersistence implementations and not the ones that are currently used) and moved them into appropriate classes.
Even with all the code that is just being moved, we are still managing to drop nearly 2K lines of code. 😄

Future Work

ConfigRepositoryE2EReadWriteTest.java - this class is a monster. Decided not to tackle it now, but as we split persistences and their tests by resource, we should pick at this. It basically has tests for CRUD operations for every single resource.
Split out test suites for workspace, actor definitions, and health checks. It should make it relatively straight forward to write separate persistences for them as a follow up.

🚨 User Impact 🚨

This PR does move around a lot of the queries from the ConfigPersistence to the ConfigRepository. There is a lot of testing but it's not that methodical or perfect coverage. There is some risk here that there is some mistake in a query that isn't caught by testing.

...rsistence/src/test/java/io/airbyte/config/persistence/BaseDatabaseConfigPersistenceTest.java

cgardens · 2022-11-20T22:15:27Z

...fig/config-persistence/src/test/java/io/airbyte/config/persistence/ConfigRepositoryTest.java

@@ -1,511 +0,0 @@
-/*


all tests in this class were moved into separate *PersistenceTest (HealthCheckPersistenceTest.java WorkspacePersistenceTest.java, and ActorDefinitionPersistenceTest.java). Also discovered we effective have no tests for the Actor resource.

cgardens · 2022-11-20T22:17:49Z

...a/io/airbyte/config/persistence/DatabaseConfigPersistenceUpdateConnectorDefinitionsTest.java

-import org.junit.jupiter.api.Test;
-
-/**
- * Unit test for the {@link DatabaseConfigPersistence#updateConnectorDefinitions} method.


this code path wasn't actually used anymore, so these tests were for a dead code path.

...fig/config-persistence/src/test/java/io/airbyte/config/persistence/StatePersistenceTest.java

...nfig-persistence/src/test/java/io/airbyte/config/persistence/StreamResetPersistenceTest.java

cgardens · 2022-11-20T22:24:33Z

...-persistence/src/test/java/io/airbyte/config/persistence/ActorDefinitionPersistenceTest.java

+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+class ActorDefinitionPersistenceTest extends BaseConfigDatabaseTest {


Changes in this file:

lift and shift of tests from ConfigRepository.

replace ConfigPersistence with ConfigRepository.

intentionally not changing any of the tests themselves.

cgardens · 2022-11-20T22:25:42Z

...config-persistence/src/test/java/io/airbyte/config/persistence/WorkspacePersistenceTest.java

+import org.junit.jupiter.params.provider.ValueSource;
+
+@SuppressWarnings({"PMD.LongVariable", "PMD.AvoidInstantiatingObjectsInLoops"})
+class WorkspacePersistenceTest extends BaseConfigDatabaseTest {


Changes in this file:

lift and shift of tests from ConfigRepository.

replace ConfigPersistence with ConfigRepository.

intentionally not changing any of the tests themselves.

cgardens · 2022-11-20T22:27:07Z

...nfig-persistence/src/test/java/io/airbyte/config/persistence/HealthCheckPersistenceTest.java

+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class HealthCheckPersistenceTest {


Changes in this file:

lift and shift of tests from ConfigRepository.

intentionally not changing any of the tests themselves.

cgardens · 2022-11-20T22:27:46Z

.../config-persistence/src/main/java/io/airbyte/config/persistence/StandardSyncPersistence.java

@@ -117,6 +119,11 @@ public void clearUnsupportedProtocolVersionFlag(final UUID actorDefinitionId,
    });
  }

+  public List<StreamDescriptor> getAllStreamsForConnection(final UUID connectionId) throws ConfigNotFoundException, IOException {


moved from ConfigRepository

...ersistence/src/test/java/io/airbyte/config/persistence/ConfigRepositoryE2EReadWriteTest.java

...fig-persistence/src/test/java/io/airbyte/config/persistence/ActorDefinitionMigratorTest.java

jdpgrailsdev

wip add queries build, but lots of wips delete DatabaseConfigPersistence pmd fix build local -x test fix names port DatabaseConfigPersistenceTest tests to ActorDefinitionMigratorTest clean up ActorDefinitionMigratorTest add CRUD tests for standard sync clean up ConfigRepository

This reverts commit 2c5dabc.

cgardens temporarily deployed to more-secrets November 10, 2022 05:16 Inactive

cgardens changed the title ~~Remove DatabaseConfigPersistence~~ Remove production use of DatabaseConfigPersistence Nov 10, 2022

cgardens force-pushed the cgardens/convert-config-persistence-to-jooq branch from 7cf9a24 to 4437060 Compare November 20, 2022 21:43

cgardens temporarily deployed to more-secrets November 20, 2022 21:46 Inactive

cgardens temporarily deployed to more-secrets November 20, 2022 21:48 Inactive

cgardens temporarily deployed to more-secrets November 20, 2022 21:57 Inactive

cgardens temporarily deployed to more-secrets November 20, 2022 22:08 Inactive