refactor(kosync): introduce differentiated sync strategies (#1624)

* refactor(kosync): introduce differentiated sync strategies Replaces the single `koreaderSyncStrategy` setting with `koreaderSyncStrategyForward` and `koreaderSyncStrategyBackward`. This allows users to define distinct conflict resolution behaviors based on whether the remote progress is newer or older than the local progress. The `KoreaderSyncStrategy` enum has been simplified to `KoreaderSyncConflictStrategy` with four clear options: `PROMPT`, `KEEP_LOCAL`, `KEEP_REMOTE`, and `DISABLED`. The ambiguous `SILENT` option is removed, as its behavior is now implicitly covered by selecting `KEEP_REMOTE` for forward syncs and `KEEP_LOCAL` for backward syncs. The legacy `koreaderSyncStrategy` setting is now deprecated and is seamlessly migrated to the new dual-strategy system using `MigratedConfigValue`, ensuring backward compatibility for existing user configurations. * fix(kosync): correct proto numbers and setting order for sync strategies * fix(kosync): proto number 78 to 68 * fix(server): migrate KOReader sync strategy during settings cleanup Add migration logic to convert the old `server.koreaderSyncStrategy` key into the new `server.koreaderSyncStrategyForward` and `server.koreaderSyncStrategyBackward` keys during server setup.
2025-12-10 06:42:07 +01:00 · 2025-09-09 16:12:53 -06:00
parent 5bf2a4aed4
commit 257e1dd03d
4 changed files with 143 additions and 24 deletions
--- a/server/server-config/src/main/kotlin/suwayomi/tachidesk/graphql/types/KoreaderSyncTypes.kt
+++ b/server/server-config/src/main/kotlin/suwayomi/tachidesk/graphql/types/KoreaderSyncTypes.kt
@@ -5,7 +5,27 @@ enum class KoreaderSyncChecksumMethod {
    FILENAME,
 }

-enum class KoreaderSyncStrategy {
+/**
+ * Defines the resolution strategy for synchronization conflicts.
+ * This is applied separately for when the remote progress is newer (Forward)
+ * or older (Backward) than the local progress.
+ */
+enum class KoreaderSyncConflictStrategy {
+    /** Ask the client application to prompt the user for a decision. */
+    PROMPT,
+    /** Always keep the local progress, ignoring the remote version. */
+    KEEP_LOCAL,
+    /** Always overwrite local progress with the remote version. */
+    KEEP_REMOTE,
+    /** Do not perform any sync action for this scenario. */
+    DISABLED,
+}
+
+/**
+ * Legacy enum for migrating the old, single sync strategy setting.
+ */
+@Deprecated("Used for migration purposes only. Use KoreaderSyncConflictStrategy instead.")
+enum class KoreaderSyncLegacyStrategy {
    PROMPT, // Ask on conflict
    SILENT, // Always use latest
    SEND, // Send changes only
--- a/server/server-config/src/main/kotlin/suwayomi/tachidesk/server/ServerConfig.kt
+++ b/server/server-config/src/main/kotlin/suwayomi/tachidesk/server/ServerConfig.kt
@@ -27,7 +27,8 @@ import suwayomi.tachidesk.graphql.types.AuthMode
 import suwayomi.tachidesk.graphql.types.DatabaseType
 import suwayomi.tachidesk.graphql.types.DownloadConversion
 import suwayomi.tachidesk.graphql.types.KoreaderSyncChecksumMethod
-import suwayomi.tachidesk.graphql.types.KoreaderSyncStrategy
+import suwayomi.tachidesk.graphql.types.KoreaderSyncConflictStrategy
+import suwayomi.tachidesk.graphql.types.KoreaderSyncLegacyStrategy
 import suwayomi.tachidesk.graphql.types.SettingsDownloadConversionType
 import suwayomi.tachidesk.graphql.types.WebUIChannel
 import suwayomi.tachidesk.graphql.types.WebUIFlavor
@@ -599,12 +600,58 @@ class ServerConfig(
        typeInfo = SettingsRegistry.PartialTypeInfo(imports = listOf("suwayomi.tachidesk.graphql.types.KoreaderSyncChecksumMethod")),
    )

-    val koreaderSyncStrategy: MutableStateFlow<KoreaderSyncStrategy> by EnumSetting(
+    @Deprecated("Use koreaderSyncStrategyForward and koreaderSyncStrategyBackward instead")
+    val koreaderSyncStrategy: MutableStateFlow<KoreaderSyncLegacyStrategy> by MigratedConfigValue(
        protoNumber = 64,
+        defaultValue = KoreaderSyncLegacyStrategy.DISABLED,
        group = SettingGroup.KOREADER_SYNC,
-        defaultValue = KoreaderSyncStrategy.DISABLED,
-        enumClass = KoreaderSyncStrategy::class,
-        typeInfo = SettingsRegistry.PartialTypeInfo(imports = listOf("suwayomi.tachidesk.graphql.types.KoreaderSyncStrategy")),
+        typeInfo =
+            SettingsRegistry.PartialTypeInfo(
+                imports = listOf("suwayomi.tachidesk.graphql.types.KoreaderSyncLegacyStrategy"),
+            ),
+        deprecated =
+        SettingsRegistry.SettingDeprecated(
+            replaceWith = "koreaderSyncStrategyForward, koreaderSyncStrategyBackward",
+            message = "Replaced with koreaderSyncStrategyForward and koreaderSyncStrategyBackward",
+        ),
+        readMigrated = {
+            // This is a best-effort reverse mapping. It's not perfect but covers common cases.
+            when {
+                koreaderSyncStrategyForward.value == KoreaderSyncConflictStrategy.PROMPT &&
+                    koreaderSyncStrategyBackward.value == KoreaderSyncConflictStrategy.PROMPT -> KoreaderSyncLegacyStrategy.PROMPT
+                koreaderSyncStrategyForward.value == KoreaderSyncConflictStrategy.KEEP_REMOTE &&
+                    koreaderSyncStrategyBackward.value == KoreaderSyncConflictStrategy.KEEP_LOCAL -> KoreaderSyncLegacyStrategy.SILENT
+                koreaderSyncStrategyForward.value == KoreaderSyncConflictStrategy.KEEP_LOCAL &&
+                    koreaderSyncStrategyBackward.value == KoreaderSyncConflictStrategy.KEEP_LOCAL -> KoreaderSyncLegacyStrategy.SEND
+                koreaderSyncStrategyForward.value == KoreaderSyncConflictStrategy.KEEP_REMOTE &&
+                    koreaderSyncStrategyBackward.value == KoreaderSyncConflictStrategy.KEEP_REMOTE -> KoreaderSyncLegacyStrategy.RECEIVE
+                else -> KoreaderSyncLegacyStrategy.DISABLED
+            }
+        },
+        setMigrated = { value ->
+            when (value) {
+                KoreaderSyncLegacyStrategy.PROMPT -> {
+                    koreaderSyncStrategyForward.value = KoreaderSyncConflictStrategy.PROMPT
+                    koreaderSyncStrategyBackward.value = KoreaderSyncConflictStrategy.PROMPT
+                }
+                KoreaderSyncLegacyStrategy.SILENT -> {
+                    koreaderSyncStrategyForward.value = KoreaderSyncConflictStrategy.KEEP_REMOTE // Remote is newer
+                    koreaderSyncStrategyBackward.value = KoreaderSyncConflictStrategy.KEEP_LOCAL  // Local is newer
+                }
+                KoreaderSyncLegacyStrategy.SEND -> {
+                    koreaderSyncStrategyForward.value = KoreaderSyncConflictStrategy.KEEP_LOCAL
+                    koreaderSyncStrategyBackward.value = KoreaderSyncConflictStrategy.KEEP_LOCAL
+                }
+                KoreaderSyncLegacyStrategy.RECEIVE -> {
+                    koreaderSyncStrategyForward.value = KoreaderSyncConflictStrategy.KEEP_REMOTE
+                    koreaderSyncStrategyBackward.value = KoreaderSyncConflictStrategy.KEEP_REMOTE
+                }
+                KoreaderSyncLegacyStrategy.DISABLED -> {
+                    koreaderSyncStrategyForward.value = KoreaderSyncConflictStrategy.DISABLED
+                    koreaderSyncStrategyBackward.value = KoreaderSyncConflictStrategy.DISABLED
+                }
+            }
+        },
    )

    val koreaderSyncPercentageTolerance: MutableStateFlow<Double> by DoubleSetting(
@@ -663,6 +710,24 @@ class ServerConfig(
        defaultValue = "",
    )

+    val koreaderSyncStrategyForward: MutableStateFlow<KoreaderSyncConflictStrategy> by EnumSetting(
+        protoNumber = 73,
+        group = SettingGroup.KOREADER_SYNC,
+        defaultValue = KoreaderSyncConflictStrategy.PROMPT,
+        enumClass = KoreaderSyncConflictStrategy::class,
+        typeInfo = SettingsRegistry.PartialTypeInfo(imports = listOf("suwayomi.tachidesk.graphql.types.KoreaderSyncConflictStrategy")),
+        description = "Strategy to apply when remote progress is newer than local.",
+    )
+
+    val koreaderSyncStrategyBackward: MutableStateFlow<KoreaderSyncConflictStrategy> by EnumSetting(
+        protoNumber = 74,
+        group = SettingGroup.KOREADER_SYNC,
+        defaultValue = KoreaderSyncConflictStrategy.DISABLED,
+        enumClass = KoreaderSyncConflictStrategy::class,
+        typeInfo = SettingsRegistry.PartialTypeInfo(imports = listOf("suwayomi.tachidesk.graphql.types.KoreaderSyncConflictStrategy")),
+        description = "Strategy to apply when remote progress is older than local.",
+    )
+
    /** ****************************************************************** **/
    /**                                                                    **/
    /**                          Renamed settings                          **/
--- a/server/src/main/kotlin/suwayomi/tachidesk/manga/impl/sync/KoreaderSyncService.kt
+++ b/server/src/main/kotlin/suwayomi/tachidesk/manga/impl/sync/KoreaderSyncService.kt
@@ -16,7 +16,7 @@ import org.jetbrains.exposed.sql.transactions.transaction
 import org.jetbrains.exposed.sql.update
 import suwayomi.tachidesk.graphql.types.KoSyncStatusPayload
 import suwayomi.tachidesk.graphql.types.KoreaderSyncChecksumMethod
-import suwayomi.tachidesk.graphql.types.KoreaderSyncStrategy
+import suwayomi.tachidesk.graphql.types.KoreaderSyncConflictStrategy
 import suwayomi.tachidesk.manga.impl.ChapterDownloadHelper
 import suwayomi.tachidesk.manga.impl.util.KoreaderHelper
 import suwayomi.tachidesk.manga.model.table.ChapterTable
@@ -274,8 +274,15 @@ object KoreaderSyncService {
    }

    suspend fun pushProgress(chapterId: Int) {
-        val strategy = serverConfig.koreaderSyncStrategy.value
-        if (strategy == KoreaderSyncStrategy.DISABLED || strategy == KoreaderSyncStrategy.RECEIVE) return
+        val forwardStrategy = serverConfig.koreaderSyncStrategyForward.value
+        val backwardStrategy = serverConfig.koreaderSyncStrategyBackward.value
+
+        // if both directions keep remote, is in receive-only mode, so don't push.
+        if (forwardStrategy == KoreaderSyncConflictStrategy.KEEP_REMOTE &&
+            backwardStrategy == KoreaderSyncConflictStrategy.KEEP_REMOTE
+        ) {
+            return
+        }

        val username = serverConfig.koreaderSyncUsername.value
        val userkey = serverConfig.koreaderSyncUserkey.value
@@ -346,8 +353,15 @@ object KoreaderSyncService {
    }

    suspend fun checkAndPullProgress(chapterId: Int): SyncResult? {
-        val strategy = serverConfig.koreaderSyncStrategy.value
-        if (strategy == KoreaderSyncStrategy.DISABLED || strategy == KoreaderSyncStrategy.SEND) return null
+        val forwardStrategy = serverConfig.koreaderSyncStrategyForward.value
+        val backwardStrategy = serverConfig.koreaderSyncStrategyBackward.value
+
+        // Skip remote fetch if both directions disabled OR both keep local (no remote data needed)
+        if ((forwardStrategy == KoreaderSyncConflictStrategy.DISABLED && backwardStrategy == KoreaderSyncConflictStrategy.DISABLED) ||
+            (forwardStrategy == KoreaderSyncConflictStrategy.KEEP_LOCAL && backwardStrategy == KoreaderSyncConflictStrategy.KEEP_LOCAL)
+        ) {
+            return null
+        }

        val username = serverConfig.koreaderSyncUsername.value
        val userkey = serverConfig.koreaderSyncUserkey.value
@@ -417,19 +431,14 @@ object KoreaderSyncService {
                            return null
                        }

-                        when (strategy) {
-                            KoreaderSyncStrategy.RECEIVE -> {
-                                return SyncResult(pageRead, timestamp, device, shouldUpdate = true)
-                            }
-                            KoreaderSyncStrategy.SILENT -> {
-                                if (timestamp > (localProgress?.lastReadAt ?: 0L)) {
-                                    return SyncResult(pageRead, timestamp, device, shouldUpdate = true)
-                                }
-                            }
-                            KoreaderSyncStrategy.PROMPT -> {
-                                return SyncResult(pageRead, timestamp, device, isConflict = true)
-                            }
-                            else -> {} // SEND and DISABLED already handled at the start of the function
+                        val localTimestamp = localProgress?.lastReadAt ?: 0L
+                        val isRemoteNewer = timestamp > localTimestamp
+                        val strategy = if (isRemoteNewer) forwardStrategy else backwardStrategy
+
+                        return when (strategy) {
+                            KoreaderSyncConflictStrategy.PROMPT -> SyncResult(pageRead, timestamp, device, isConflict = true)
+                            KoreaderSyncConflictStrategy.KEEP_REMOTE -> SyncResult(pageRead, timestamp, device, shouldUpdate = true)
+                            KoreaderSyncConflictStrategy.KEEP_LOCAL, KoreaderSyncConflictStrategy.DISABLED -> null
                        }
                    }
                } else {
--- a/server/src/main/kotlin/suwayomi/tachidesk/server/ServerSetup.kt
+++ b/server/src/main/kotlin/suwayomi/tachidesk/server/ServerSetup.kt
@@ -339,6 +339,31 @@ fun applicationSetup() {
                        "server.authPassword",
                        toType = { it.unwrapped() as? String },
                    )
+
+                // Migrate KOReader sync strategy from single to forward/backward strategies
+                try {
+                    val oldStrategy = config.getString("server.koreaderSyncStrategy")
+                    val (forward, backward) =
+                        when (oldStrategy.uppercase()) {
+                            "PROMPT" -> "PROMPT" to "PROMPT"
+                            "SILENT" -> "KEEP_REMOTE" to "KEEP_LOCAL"
+                            "SEND" -> "KEEP_LOCAL" to "KEEP_LOCAL"
+                            "RECEIVE" -> "KEEP_REMOTE" to "KEEP_REMOTE"
+                            "DISABLED" -> "DISABLED" to "DISABLED"
+                            else -> null to null
+                        }
+
+                    if (forward != null && backward != null) {
+                        updatedConfig =
+                            updatedConfig
+                                .withValue("server.koreaderSyncStrategyForward", forward.toConfig("internal").getValue("internal"))
+                                .withValue("server.koreaderSyncStrategyBackward", backward.toConfig("internal").getValue("internal"))
+                                .withoutPath("server.koreaderSyncStrategy")
+                    }
+                } catch (_: ConfigException.Missing) {
+                    // Key doesn't exist, no migration needed
+                }
+
                updatedConfig
            }
        }