Migrator improvements (#588)

app/build.gradle.kts

@@ -254,6 +254,8 @@ dependencies {
     // For detecting memory leaks; see https://square.github.io/leakcanary/
     // debugImplementation(libs.leakcanary.android)
     implementation(libs.leakcanary.plumber)
+
+    testImplementation(kotlinx.coroutines.test)
 }
 
 androidComponents {

app/src/main/java/eu/kanade/tachiyomi/App.kt

@@ -50,8 +50,12 @@ import kotlinx.coroutines.flow.onEach
 import logcat.AndroidLogcatLogger
 import logcat.LogPriority
 import logcat.LogcatLogger
+import mihon.core.migration.Migrator
+import mihon.core.migration.migrations.migrations
 import org.conscrypt.Conscrypt
 import tachiyomi.core.common.i18n.stringResource
+import tachiyomi.core.common.preference.Preference
+import tachiyomi.core.common.preference.PreferenceStore
 import tachiyomi.core.common.util.system.logcat
 import tachiyomi.i18n.MR
 import tachiyomi.presentation.widget.WidgetManager
@@ -131,6 +135,23 @@ class App : Application(), DefaultLifecycleObserver, SingletonImageLoader.Factor
         if (!LogcatLogger.isInstalled && networkPreferences.verboseLogging().get()) {
             LogcatLogger.install(AndroidLogcatLogger(LogPriority.VERBOSE))
         }
+
+        initializeMigrator()
+    }
+
+    private fun initializeMigrator() {
+        val preferenceStore = Injekt.get<PreferenceStore>()
+        val preference = preferenceStore.getInt(Preference.appStateKey("last_version_code"), 0)
+        logcat { "Migration from ${preference.get()} to ${BuildConfig.VERSION_CODE}" }
+        Migrator.initialize(
+            old = preference.get(),
+            new = BuildConfig.VERSION_CODE,
+            migrations = migrations,
+            onMigrationComplete = {
+                logcat { "Updating last version to ${BuildConfig.VERSION_CODE}" }
+                preference.set(BuildConfig.VERSION_CODE)
+            },
+        )
     }
 
     override fun newImageLoader(context: Context): ImageLoader {

app/src/main/java/eu/kanade/tachiyomi/ui/main/MainActivity.kt

@@ -87,10 +87,7 @@ import kotlinx.coroutines.flow.onEach
 import kotlinx.coroutines.launch
 import logcat.LogPriority
 import mihon.core.migration.Migrator
-import mihon.core.migration.migrations.migrations
 import tachiyomi.core.common.Constants
-import tachiyomi.core.common.preference.Preference
-import tachiyomi.core.common.preference.PreferenceStore
 import tachiyomi.core.common.util.lang.launchIO
 import tachiyomi.core.common.util.system.logcat
 import tachiyomi.domain.library.service.LibraryPreferences
@@ -99,8 +96,6 @@ import tachiyomi.i18n.MR
 import tachiyomi.presentation.core.components.material.Scaffold
 import tachiyomi.presentation.core.i18n.stringResource
 import tachiyomi.presentation.core.util.collectAsState
-import uy.kohesive.injekt.Injekt
-import uy.kohesive.injekt.api.get
 import uy.kohesive.injekt.injectLazy
 import androidx.compose.ui.graphics.Color.Companion as ComposeColor
 
@@ -129,7 +124,7 @@ class MainActivity : BaseActivity() {
 
         super.onCreate(savedInstanceState)
 
-        val didMigration = migrate()
+        val didMigration = Migrator.awaitAndRelease()
 
         // Do not let the launcher create a new activity http://stackoverflow.com/questions/16283079
         if (!isTaskRoot) {
@@ -340,21 +335,6 @@ class MainActivity : BaseActivity() {
         }
     }
 
-    private fun migrate(): Boolean {
-        val preferenceStore = Injekt.get<PreferenceStore>()
-        val preference = preferenceStore.getInt(Preference.appStateKey("last_version_code"), 0)
-        logcat { "Migration from ${preference.get()} to ${BuildConfig.VERSION_CODE}" }
-        return Migrator.migrate(
-            old = preference.get(),
-            new = BuildConfig.VERSION_CODE,
-            migrations = migrations,
-            onMigrationComplete = {
-                logcat { "Updating last version to ${BuildConfig.VERSION_CODE}" }
-                preference.set(BuildConfig.VERSION_CODE)
-            },
-        )
-    }
-
     /**
      * Sets custom splash screen exit animation on devices prior to Android 12.
      *

app/src/main/java/mihon/core/migration/Migration.kt

@@ -5,6 +5,9 @@ interface Migration {
 
     suspend operator fun invoke(migrationContext: MigrationContext): Boolean
 
+    val isAlways: Boolean
+        get() = version == ALWAYS
+
     companion object {
         const val ALWAYS = -1f
 

app/src/main/java/mihon/core/migration/MigrationCompletedListener.kt

@@ -0,0 +1,3 @@
+package mihon.core.migration
+
+typealias MigrationCompletedListener = () -> Unit

app/src/main/java/mihon/core/migration/MigrationContext.kt

@@ -2,7 +2,7 @@ package mihon.core.migration
 
 import uy.kohesive.injekt.Injekt
 
-class MigrationContext {
+class MigrationContext(val dryrun: Boolean) {
 
     inline fun <reified T> get(): T? {
         return Injekt.getInstanceOrNull(T::class.java)

app/src/main/java/mihon/core/migration/MigrationJobFactory.kt

@@ -0,0 +1,30 @@
+package mihon.core.migration
+
+import kotlinx.coroutines.CompletableDeferred
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Deferred
+import kotlinx.coroutines.async
+import tachiyomi.core.common.util.system.logcat
+
+class MigrationJobFactory(
+    private val migrationContext: MigrationContext,
+    private val scope: CoroutineScope
+) {
+
+    @SuppressWarnings("MaxLineLength")
+    fun create(migrations: List<Migration>): Deferred<Boolean> = with(scope) {
+        return migrations.sortedBy { it.version }
+            .fold(CompletableDeferred(true)) { acc: Deferred<Boolean>, migration: Migration ->
+                if (!migrationContext.dryrun) {
+                    logcat { "Running migration: { name = ${migration::class.simpleName}, version = ${migration.version} }" }
+                    async {
+                        val prev = acc.await()
+                        migration(migrationContext) || prev
+                    }
+                } else {
+                    logcat { "(Dry-run) Running migration: { name = ${migration::class.simpleName}, version = ${migration.version} }" }
+                    CompletableDeferred(true)
+                }
+            }
+    }
+}

app/src/main/java/mihon/core/migration/MigrationStrategy.kt

@@ -0,0 +1,55 @@
+package mihon.core.migration
+
+import kotlinx.coroutines.CompletableDeferred
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Deferred
+import kotlinx.coroutines.launch
+
+interface MigrationStrategy {
+    operator fun invoke(migrations: List<Migration>): Deferred<Boolean>
+}
+
+class DefaultMigrationStrategy(
+    private val migrationJobFactory: MigrationJobFactory,
+    private val migrationCompletedListener: MigrationCompletedListener,
+    private val scope: CoroutineScope
+) : MigrationStrategy {
+
+    override operator fun invoke(migrations: List<Migration>): Deferred<Boolean> = with(scope) {
+        if (migrations.isEmpty()) {
+            return@with CompletableDeferred(false)
+        }
+
+        val chain = migrationJobFactory.create(migrations)
+
+        launch {
+            if (chain.await()) migrationCompletedListener()
+        }.start()
+
+        chain
+    }
+}
+
+class InitialMigrationStrategy(private val strategy: DefaultMigrationStrategy) : MigrationStrategy {
+
+    override operator fun invoke(migrations: List<Migration>): Deferred<Boolean> {
+        return strategy(migrations.filter { it.isAlways })
+    }
+}
+
+class NoopMigrationStrategy(val state: Boolean) : MigrationStrategy {
+
+    override fun invoke(migrations: List<Migration>): Deferred<Boolean> {
+        return CompletableDeferred(state)
+    }
+}
+
+class VersionRangeMigrationStrategy(
+    private val versions: IntRange,
+    private val strategy: DefaultMigrationStrategy
+) : MigrationStrategy {
+
+    override operator fun invoke(migrations: List<Migration>): Deferred<Boolean> {
+        return strategy(migrations.filter { it.isAlways || it.version.toInt() in versions })
+    }
+}

app/src/main/java/mihon/core/migration/MigrationStrategyFactory.kt

@@ -0,0 +1,23 @@
+package mihon.core.migration
+
+class MigrationStrategyFactory(
+    private val factory: MigrationJobFactory,
+    private val migrationCompletedListener: MigrationCompletedListener,
+) {
+
+    fun create(old: Int, new: Int): MigrationStrategy {
+        val versions = (old + 1)..new
+        val strategy = when {
+            old == 0 -> InitialMigrationStrategy(
+                strategy = DefaultMigrationStrategy(factory, migrationCompletedListener, Migrator.scope),
+            )
+
+            old >= new -> NoopMigrationStrategy(false)
+            else -> VersionRangeMigrationStrategy(
+                versions = versions,
+                strategy = DefaultMigrationStrategy(factory, migrationCompletedListener, Migrator.scope),
+            )
+        }
+        return strategy
+    }
+}

app/src/main/java/mihon/core/migration/Migrator.kt

@@ -1,53 +1,41 @@
 package mihon.core.migration
 
+import kotlinx.coroutines.CompletableDeferred
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Deferred
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.Job
 import kotlinx.coroutines.runBlocking
-import tachiyomi.core.common.util.system.logcat
 
 object Migrator {
 
-    @SuppressWarnings("ReturnCount")
+    private var result: Deferred<Boolean>? = null
-    fun migrate(
+    val scope = CoroutineScope(Dispatchers.Main + Job())
+
+    fun initialize(
         old: Int,
         new: Int,
         migrations: List<Migration>,
         dryrun: Boolean = false,
         onMigrationComplete: () -> Unit
-    ): Boolean {
+    ) {
-        val migrationContext = MigrationContext()
+        val migrationContext = MigrationContext(dryrun)
-
+        val migrationJobFactory = MigrationJobFactory(migrationContext, scope)
-        if (old == 0) {
+        val migrationStrategyFactory = MigrationStrategyFactory(migrationJobFactory, onMigrationComplete)
-            return migrationContext.migrate(
+        val strategy = migrationStrategyFactory.create(old, new)
-                migrations = migrations.filter { it.isAlways() },
+        result = strategy(migrations)
-                dryrun = dryrun
-            )
-                .also { onMigrationComplete() }
     }
 
-        if (old >= new) {
+    suspend fun await(): Boolean {
-            return false
+        val result = result ?: CompletableDeferred(false)
+        return result.await()
     }
 
-        return migrationContext.migrate(
+    fun release() {
-            migrations = migrations.filter { it.isAlways() || it.version.toInt() in (old + 1)..new },
+        result = null
-            dryrun = dryrun
-        )
-            .also { onMigrationComplete() }
     }
 
-    private fun Migration.isAlways() = version == Migration.ALWAYS
+    fun awaitAndRelease(): Boolean = runBlocking {
-
+        await().also { release() }
-    @SuppressWarnings("MaxLineLength")
-    private fun MigrationContext.migrate(migrations: List<Migration>, dryrun: Boolean): Boolean {
-        return migrations.sortedBy { it.version }
-            .map { migration ->
-                if (!dryrun) {
-                    logcat { "Running migration: { name = ${migration::class.simpleName}, version = ${migration.version} }" }
-                    runBlocking { migration(this@migrate) }
-                } else {
-                    logcat { "(Dry-run) Running migration: { name = ${migration::class.simpleName}, version = ${migration.version} }" }
-                    true
-                }
-            }
-            .reduce { acc, b -> acc || b }
     }
 }

app/src/test/java/mihon/core/migration/MigratorTest.kt

@@ -1,59 +1,97 @@
 package mihon.core.migration
 
 import io.mockk.Called
+import io.mockk.slot
 import io.mockk.spyk
 import io.mockk.verify
-import org.junit.jupiter.api.Assertions
+import kotlinx.coroutines.CoroutineScope
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.Job
+import kotlinx.coroutines.newSingleThreadContext
+import kotlinx.coroutines.runBlocking
+import kotlinx.coroutines.test.resetMain
+import kotlinx.coroutines.test.setMain
+import org.junit.jupiter.api.AfterAll
+import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Assertions.assertFalse
+import org.junit.jupiter.api.Assertions.assertInstanceOf
+import org.junit.jupiter.api.BeforeAll
+import org.junit.jupiter.api.BeforeEach
 import org.junit.jupiter.api.Test
 
 class MigratorTest {
 
-    @Test
+    lateinit var migrationCompletedListener: MigrationCompletedListener
-    fun initialVersion() {
+    lateinit var migrationContext: MigrationContext
-        val onMigrationComplete: () -> Unit = {}
+    lateinit var migrationJobFactory: MigrationJobFactory
-        val onMigrationCompleteSpy = spyk(onMigrationComplete)
+    lateinit var migrationStrategyFactory: MigrationStrategyFactory
-        val didMigration = Migrator.migrate(
+
-            old = 0,
+    @BeforeEach
-            new = 1,
+    fun initilize() {
-            migrations = listOf(Migration.of(Migration.ALWAYS) { true }, Migration.of(2f) { false }),
+        migrationContext = MigrationContext(false)
-            onMigrationComplete = onMigrationCompleteSpy
+        migrationJobFactory = spyk(MigrationJobFactory(migrationContext, CoroutineScope(Dispatchers.Main + Job())))
-        )
+        migrationCompletedListener = spyk<() -> Unit>({})
-        verify { onMigrationCompleteSpy() }
+        migrationStrategyFactory = spyk(MigrationStrategyFactory(migrationJobFactory, migrationCompletedListener))
-        Assertions.assertTrue(didMigration)
     }
 
     @Test
-    fun sameVersion() {
+    fun initialVersion() = runBlocking {
-        val onMigrationComplete: () -> Unit = {}
+        val strategy = migrationStrategyFactory.create(0, 1)
-        val onMigrationCompleteSpy = spyk(onMigrationComplete)
+        assertInstanceOf(InitialMigrationStrategy::class.java, strategy)
-        val didMigration = Migrator.migrate(
+
-            old = 1,
+        val migrations = slot<List<Migration>>()
-            new = 1,
+        val execute = strategy(listOf(Migration.of(Migration.ALWAYS) { true }, Migration.of(2f) { false }))
-            migrations = listOf(Migration.of(Migration.ALWAYS) { true }, Migration.of(2f) { true }),
+
-            onMigrationComplete = onMigrationCompleteSpy
+        execute.await()
-        )
+
-        verify { onMigrationCompleteSpy wasNot Called }
+        verify { migrationJobFactory.create(capture(migrations)) }
-        Assertions.assertFalse(didMigration)
+        assertEquals(1, migrations.captured.size)
+        verify { migrationCompletedListener() }
     }
 
     @Test
-    fun smallMigration() {
+    fun sameVersion() = runBlocking {
-        val onMigrationComplete: () -> Unit = {}
+        val strategy = migrationStrategyFactory.create(1, 1)
-        val onMigrationCompleteSpy = spyk(onMigrationComplete)
+        assertInstanceOf(NoopMigrationStrategy::class.java, strategy)
-        val didMigration = Migrator.migrate(
+
-            old = 1,
+        val execute = strategy(listOf(Migration.of(Migration.ALWAYS) { true }, Migration.of(2f) { false }))
-            new = 2,
+
-            migrations = listOf(Migration.of(Migration.ALWAYS) { true }, Migration.of(2f) { true }),
+        val result = execute.await()
-            onMigrationComplete = onMigrationCompleteSpy
+        assertFalse(result)
-        )
+
-        verify { onMigrationCompleteSpy() }
+        verify { migrationJobFactory.create(any()) wasNot Called }
-        Assertions.assertTrue(didMigration)
     }
 
     @Test
-    fun largeMigration() {
+    fun noMigrations() = runBlocking {
-        val onMigrationComplete: () -> Unit = {}
+        val strategy = migrationStrategyFactory.create(1, 2)
-        val onMigrationCompleteSpy = spyk(onMigrationComplete)
+        assertInstanceOf(VersionRangeMigrationStrategy::class.java, strategy)
+
+        val execute = strategy(emptyList())
+
+        val result = execute.await()
+        assertFalse(result)
+
+        verify { migrationJobFactory.create(any()) wasNot Called }
+    }
+
+    @Test
+    fun smallMigration() = runBlocking {
+        val strategy = migrationStrategyFactory.create(1, 2)
+        assertInstanceOf(VersionRangeMigrationStrategy::class.java, strategy)
+
+        val migrations = slot<List<Migration>>()
+        val execute = strategy(listOf(Migration.of(Migration.ALWAYS) { true }, Migration.of(2f) { true }))
+
+        execute.await()
+
+        verify { migrationJobFactory.create(capture(migrations)) }
+        assertEquals(2, migrations.captured.size)
+        verify { migrationCompletedListener() }
+    }
+
+    @Test
+    fun largeMigration() = runBlocking {
         val input = listOf(
             Migration.of(Migration.ALWAYS) { true },
             Migration.of(2f) { true },
@@ -66,31 +104,56 @@ class MigratorTest {
             Migration.of(9f) { true },
             Migration.of(10f) { true },
         )
-        val didMigration = Migrator.migrate(
+
-            old = 1,
+        val strategy = migrationStrategyFactory.create(1, 10)
-            new = 10,
+        assertInstanceOf(VersionRangeMigrationStrategy::class.java, strategy)
-            migrations = input,
+
-            onMigrationComplete = onMigrationCompleteSpy
+        val migrations = slot<List<Migration>>()
-        )
+        val execute = strategy(input)
-        verify { onMigrationCompleteSpy() }
+
-        Assertions.assertTrue(didMigration)
+        execute.await()
+
+        verify { migrationJobFactory.create(capture(migrations)) }
+        assertEquals(10, migrations.captured.size)
+        verify { migrationCompletedListener() }
     }
 
     @Test
-    fun withinRangeMigration() {
+    fun withinRangeMigration() = runBlocking {
-        val onMigrationComplete: () -> Unit = {}
+        val strategy = migrationStrategyFactory.create(1, 2)
-        val onMigrationCompleteSpy = spyk(onMigrationComplete)
+        assertInstanceOf(VersionRangeMigrationStrategy::class.java, strategy)
-        val didMigration = Migrator.migrate(
+
-            old = 1,
+        val migrations = slot<List<Migration>>()
-            new = 2,
+        val execute = strategy(
-            migrations = listOf(
+            listOf(
                 Migration.of(Migration.ALWAYS) { true },
                 Migration.of(2f) { true },
                 Migration.of(3f) { false }
-            ),
-            onMigrationComplete = onMigrationCompleteSpy
             )
-        verify { onMigrationCompleteSpy() }
+        )
-        Assertions.assertTrue(didMigration)
+
+        execute.await()
+
+        verify { migrationJobFactory.create(capture(migrations)) }
+        assertEquals(2, migrations.captured.size)
+        verify { migrationCompletedListener() }
+    }
+
+    companion object {
+
+        val mainThreadSurrogate = newSingleThreadContext("UI thread")
+
+        @BeforeAll
+        @JvmStatic
+        fun setUp() {
+            Dispatchers.setMain(mainThreadSurrogate)
+        }
+
+        @AfterAll
+        @JvmStatic
+        fun tearDown() {
+            Dispatchers.resetMain() // reset the main dispatcher to the original Main dispatcher
+            mainThreadSurrogate.close()
+        }
     }
 }