From 0265c16eb239518d52b7e9fb4200b5b003418d5d Mon Sep 17 00:00:00 2001 From: Andreas Date: Thu, 28 Mar 2024 19:36:33 +0100 Subject: [PATCH] Migrator improvements (#588) --- app/build.gradle.kts | 2 + app/src/main/java/eu/kanade/tachiyomi/App.kt | 21 +++ .../kanade/tachiyomi/ui/main/MainActivity.kt | 22 +-- .../java/mihon/core/migration/Migration.kt | 3 + .../migration/MigrationCompletedListener.kt | 3 + .../mihon/core/migration/MigrationContext.kt | 2 +- .../core/migration/MigrationJobFactory.kt | 30 +++ .../mihon/core/migration/MigrationStrategy.kt | 55 ++++++ .../migration/MigrationStrategyFactory.kt | 23 +++ .../java/mihon/core/migration/Migrator.kt | 62 +++--- .../java/mihon/core/migration/MigratorTest.kt | 177 ++++++++++++------ 11 files changed, 284 insertions(+), 116 deletions(-) create mode 100644 app/src/main/java/mihon/core/migration/MigrationCompletedListener.kt create mode 100644 app/src/main/java/mihon/core/migration/MigrationJobFactory.kt create mode 100644 app/src/main/java/mihon/core/migration/MigrationStrategy.kt create mode 100644 app/src/main/java/mihon/core/migration/MigrationStrategyFactory.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index fad306768..9edadd5fc 100644 --- a/app/build.gradle.kts +++ b/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 { diff --git a/app/src/main/java/eu/kanade/tachiyomi/App.kt b/app/src/main/java/eu/kanade/tachiyomi/App.kt index 6c29060c1..3ffc83ebd 100644 --- a/app/src/main/java/eu/kanade/tachiyomi/App.kt +++ b/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() + 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 { diff --git a/app/src/main/java/eu/kanade/tachiyomi/ui/main/MainActivity.kt b/app/src/main/java/eu/kanade/tachiyomi/ui/main/MainActivity.kt index d49a7fc68..39c9dd5c7 100644 --- a/app/src/main/java/eu/kanade/tachiyomi/ui/main/MainActivity.kt +++ b/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() - 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. * diff --git a/app/src/main/java/mihon/core/migration/Migration.kt b/app/src/main/java/mihon/core/migration/Migration.kt index 2fa04d1c9..2c7283bb2 100644 --- a/app/src/main/java/mihon/core/migration/Migration.kt +++ b/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 diff --git a/app/src/main/java/mihon/core/migration/MigrationCompletedListener.kt b/app/src/main/java/mihon/core/migration/MigrationCompletedListener.kt new file mode 100644 index 000000000..52f3324a2 --- /dev/null +++ b/app/src/main/java/mihon/core/migration/MigrationCompletedListener.kt @@ -0,0 +1,3 @@ +package mihon.core.migration + +typealias MigrationCompletedListener = () -> Unit diff --git a/app/src/main/java/mihon/core/migration/MigrationContext.kt b/app/src/main/java/mihon/core/migration/MigrationContext.kt index 68ddf6464..3d0473f27 100644 --- a/app/src/main/java/mihon/core/migration/MigrationContext.kt +++ b/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 get(): T? { return Injekt.getInstanceOrNull(T::class.java) diff --git a/app/src/main/java/mihon/core/migration/MigrationJobFactory.kt b/app/src/main/java/mihon/core/migration/MigrationJobFactory.kt new file mode 100644 index 000000000..846ebc277 --- /dev/null +++ b/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): Deferred = with(scope) { + return migrations.sortedBy { it.version } + .fold(CompletableDeferred(true)) { acc: Deferred, 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) + } + } + } +} diff --git a/app/src/main/java/mihon/core/migration/MigrationStrategy.kt b/app/src/main/java/mihon/core/migration/MigrationStrategy.kt new file mode 100644 index 000000000..9fd5f4f91 --- /dev/null +++ b/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): Deferred +} + +class DefaultMigrationStrategy( + private val migrationJobFactory: MigrationJobFactory, + private val migrationCompletedListener: MigrationCompletedListener, + private val scope: CoroutineScope +) : MigrationStrategy { + + override operator fun invoke(migrations: List): Deferred = 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): Deferred { + return strategy(migrations.filter { it.isAlways }) + } +} + +class NoopMigrationStrategy(val state: Boolean) : MigrationStrategy { + + override fun invoke(migrations: List): Deferred { + return CompletableDeferred(state) + } +} + +class VersionRangeMigrationStrategy( + private val versions: IntRange, + private val strategy: DefaultMigrationStrategy +) : MigrationStrategy { + + override operator fun invoke(migrations: List): Deferred { + return strategy(migrations.filter { it.isAlways || it.version.toInt() in versions }) + } +} diff --git a/app/src/main/java/mihon/core/migration/MigrationStrategyFactory.kt b/app/src/main/java/mihon/core/migration/MigrationStrategyFactory.kt new file mode 100644 index 000000000..7e06fecb3 --- /dev/null +++ b/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 + } +} diff --git a/app/src/main/java/mihon/core/migration/Migrator.kt b/app/src/main/java/mihon/core/migration/Migrator.kt index 86288e2a0..11f22a8c9 100644 --- a/app/src/main/java/mihon/core/migration/Migrator.kt +++ b/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") - fun migrate( + private var result: Deferred? = null + val scope = CoroutineScope(Dispatchers.Main + Job()) + + fun initialize( old: Int, new: Int, migrations: List, dryrun: Boolean = false, onMigrationComplete: () -> Unit - ): Boolean { - val migrationContext = MigrationContext() - - if (old == 0) { - return migrationContext.migrate( - migrations = migrations.filter { it.isAlways() }, - dryrun = dryrun - ) - .also { onMigrationComplete() } - } - - if (old >= new) { - return false - } - - return migrationContext.migrate( - migrations = migrations.filter { it.isAlways() || it.version.toInt() in (old + 1)..new }, - dryrun = dryrun - ) - .also { onMigrationComplete() } + ) { + val migrationContext = MigrationContext(dryrun) + val migrationJobFactory = MigrationJobFactory(migrationContext, scope) + val migrationStrategyFactory = MigrationStrategyFactory(migrationJobFactory, onMigrationComplete) + val strategy = migrationStrategyFactory.create(old, new) + result = strategy(migrations) } - private fun Migration.isAlways() = version == Migration.ALWAYS + suspend fun await(): Boolean { + val result = result ?: CompletableDeferred(false) + return result.await() + } - @SuppressWarnings("MaxLineLength") - private fun MigrationContext.migrate(migrations: List, 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 } + fun release() { + result = null + } + + fun awaitAndRelease(): Boolean = runBlocking { + await().also { release() } } } diff --git a/app/src/test/java/mihon/core/migration/MigratorTest.kt b/app/src/test/java/mihon/core/migration/MigratorTest.kt index 89fe4db8c..e0feff3fd 100644 --- a/app/src/test/java/mihon/core/migration/MigratorTest.kt +++ 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 - fun initialVersion() { - val onMigrationComplete: () -> Unit = {} - val onMigrationCompleteSpy = spyk(onMigrationComplete) - val didMigration = Migrator.migrate( - old = 0, - new = 1, - migrations = listOf(Migration.of(Migration.ALWAYS) { true }, Migration.of(2f) { false }), - onMigrationComplete = onMigrationCompleteSpy - ) - verify { onMigrationCompleteSpy() } - Assertions.assertTrue(didMigration) + lateinit var migrationCompletedListener: MigrationCompletedListener + lateinit var migrationContext: MigrationContext + lateinit var migrationJobFactory: MigrationJobFactory + lateinit var migrationStrategyFactory: MigrationStrategyFactory + + @BeforeEach + fun initilize() { + migrationContext = MigrationContext(false) + migrationJobFactory = spyk(MigrationJobFactory(migrationContext, CoroutineScope(Dispatchers.Main + Job()))) + migrationCompletedListener = spyk<() -> Unit>({}) + migrationStrategyFactory = spyk(MigrationStrategyFactory(migrationJobFactory, migrationCompletedListener)) } @Test - fun sameVersion() { - val onMigrationComplete: () -> Unit = {} - val onMigrationCompleteSpy = spyk(onMigrationComplete) - val didMigration = Migrator.migrate( - old = 1, - new = 1, - migrations = listOf(Migration.of(Migration.ALWAYS) { true }, Migration.of(2f) { true }), - onMigrationComplete = onMigrationCompleteSpy - ) - verify { onMigrationCompleteSpy wasNot Called } - Assertions.assertFalse(didMigration) + fun initialVersion() = runBlocking { + val strategy = migrationStrategyFactory.create(0, 1) + assertInstanceOf(InitialMigrationStrategy::class.java, strategy) + + val migrations = slot>() + val execute = strategy(listOf(Migration.of(Migration.ALWAYS) { true }, Migration.of(2f) { false })) + + execute.await() + + verify { migrationJobFactory.create(capture(migrations)) } + assertEquals(1, migrations.captured.size) + verify { migrationCompletedListener() } } @Test - fun smallMigration() { - val onMigrationComplete: () -> Unit = {} - val onMigrationCompleteSpy = spyk(onMigrationComplete) - val didMigration = Migrator.migrate( - old = 1, - new = 2, - migrations = listOf(Migration.of(Migration.ALWAYS) { true }, Migration.of(2f) { true }), - onMigrationComplete = onMigrationCompleteSpy - ) - verify { onMigrationCompleteSpy() } - Assertions.assertTrue(didMigration) + fun sameVersion() = runBlocking { + val strategy = migrationStrategyFactory.create(1, 1) + assertInstanceOf(NoopMigrationStrategy::class.java, strategy) + + val execute = strategy(listOf(Migration.of(Migration.ALWAYS) { true }, Migration.of(2f) { false })) + + val result = execute.await() + assertFalse(result) + + verify { migrationJobFactory.create(any()) wasNot Called } } @Test - fun largeMigration() { - val onMigrationComplete: () -> Unit = {} - val onMigrationCompleteSpy = spyk(onMigrationComplete) + fun noMigrations() = runBlocking { + val strategy = migrationStrategyFactory.create(1, 2) + 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>() + 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, - new = 10, - migrations = input, - onMigrationComplete = onMigrationCompleteSpy - ) - verify { onMigrationCompleteSpy() } - Assertions.assertTrue(didMigration) + + val strategy = migrationStrategyFactory.create(1, 10) + assertInstanceOf(VersionRangeMigrationStrategy::class.java, strategy) + + val migrations = slot>() + val execute = strategy(input) + + execute.await() + + verify { migrationJobFactory.create(capture(migrations)) } + assertEquals(10, migrations.captured.size) + verify { migrationCompletedListener() } } @Test - fun withinRangeMigration() { - val onMigrationComplete: () -> Unit = {} - val onMigrationCompleteSpy = spyk(onMigrationComplete) - val didMigration = Migrator.migrate( - old = 1, - new = 2, - migrations = listOf( + fun withinRangeMigration() = runBlocking { + val strategy = migrationStrategyFactory.create(1, 2) + assertInstanceOf(VersionRangeMigrationStrategy::class.java, strategy) + + val migrations = slot>() + val execute = strategy( + 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() + } } }