diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/UpdateInfo.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/UpdateInfo.kt index bf6d052a..5e3ace0b 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/UpdateInfo.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/UpdateInfo.kt @@ -1,15 +1,26 @@ package com.fabledsword.minstrel.models /** - * Wire shape returned by `GET /api/client/version`. Mirrors - * the Flutter client's `UpdateInfo`. + * The server-bundled APK, as reported by `GET /api/client/version`. * - * `version` is the server-bundled APK version (may have a leading - * "v" from the git tag); `apkUrl` is server-relative (e.g. - * `/api/client/apk`); `sizeBytes` is the download size. + * Three values that are deliberately kept apart: + * + * - [version] is a LABEL for people — "YYYY.MM.DD.HHMM", derived from the + * build's commit, so two channels carrying the same code read the same. + * Display this; never decide on it when [code] is present. + * - [code] is the ORDERING KEY, and is the same value Android itself + * installs by. It answers "may this be installed over that?", which the + * name cannot. Null when the server predates the field. + * - [channel] is a SIBLING FIELD, never a suffix inside the name. Reported + * verbatim rather than validated, so an unexpected value is shown rather + * than dropped. + * + * [apkUrl] is server-relative (e.g. `/api/client/apk`). */ data class UpdateInfo( val version: String, + val code: Long?, + val channel: String?, val apkUrl: String, val sizeBytes: Long, ) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/UpdateInfoWire.kt b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/UpdateInfoWire.kt index fe772cd6..bf24db3a 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/models/wire/UpdateInfoWire.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/models/wire/UpdateInfoWire.kt @@ -4,12 +4,26 @@ import kotlinx.serialization.SerialName import kotlinx.serialization.Serializable /** - * Wire shape for `GET /api/client/version`. Defaults match Flutter: - * apk_url falls back to `/api/client/apk` if the server omits it. + * Wire shape for `GET /api/client/version`. + * + * `apkUrl` falls back to `/api/client/apk` if the server omits it. + * + * [code] MUST stay nullable, and this is not a style preference. The app's + * Json is configured with `coerceInputValues = true`, which replaces a JSON + * null with the declared default for a NON-nullable property — so writing + * `val code: Long = 0` would turn "this server reports no ordering key" into + * "this build's ordering key is 0", silently, with no error anywhere. A + * nullable type is what keeps absent distinguishable from zero, and the + * distinction is the whole reason the field exists. + * + * A server predating the ordering key sends neither [code] nor [channel]; + * both arrive null and the caller falls back to comparing names. */ @Serializable data class UpdateInfoWire( val version: String = "", + val code: Long? = null, + val channel: String? = null, @SerialName("apk_url") val apkUrl: String = "/api/client/apk", @SerialName("size_bytes") val sizeBytes: Long = 0, ) diff --git a/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/AboutCardViewModel.kt b/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/AboutCardViewModel.kt index cd5abac1..2ccd36ce 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/AboutCardViewModel.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/settings/ui/AboutCardViewModel.kt @@ -9,7 +9,7 @@ import com.fabledsword.minstrel.update.data.ApkInstaller import com.fabledsword.minstrel.update.data.InstallStage import com.fabledsword.minstrel.update.data.UpdateRepository import com.fabledsword.minstrel.update.data.isBusy -import com.fabledsword.minstrel.update.data.isVersionNewer +import com.fabledsword.minstrel.update.data.isUpdateAvailable import com.fabledsword.minstrel.update.data.message import com.fabledsword.minstrel.update.data.stage import dagger.hilt.android.lifecycle.HiltViewModel @@ -37,6 +37,10 @@ sealed interface UpdateCheckResult { data class AboutUiState( val installedVersion: String = BuildConfig.VERSION_NAME, + // The value the platform installs by, and therefore the one the update + // check must decide on. Held in state rather than read inline so a test + // can drive the comparison without a BuildConfig. + val installedCode: Long = BuildConfig.VERSION_CODE.toLong(), val isChecking: Boolean = false, val installStage: InstallStage = InstallStage.IDLE, val installMessage: String? = null, @@ -45,8 +49,9 @@ data class AboutUiState( /** * Backs the About card's update controls. "Check for updates" calls - * [UpdateRepository.getLatest], compares versus the build's - * VERSION_NAME via [isVersionNewer], and reports the terminal state. + * [UpdateRepository.getLatest], compares versus this build via + * [isUpdateAvailable] — on the ordering key where the server reports one, + * on the name otherwise — and reports the terminal state. * When an update is available, [install] downloads the APK via * [ApkInstaller] and installs it — routing the user to the "install * unknown apps" settings page first when that permission hasn't been @@ -66,9 +71,17 @@ class AboutCardViewModel @Inject constructor( viewModelScope.launch { internal.update { it.copy(isChecking = true, installMessage = null) } val installed = internal.value.installedVersion + val installedCode = internal.value.installedCode val result = runCatching { repository.getLatest() } .map { latest -> - if (isVersionNewer(latest.version, installed)) { + if ( + isUpdateAvailable( + serverCode = latest.code, + serverName = latest.version, + installedCode = installedCode, + installedName = installed, + ) + ) { UpdateCheckResult.UpdateAvailable(latest) } else { UpdateCheckResult.Latest diff --git a/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateBannerController.kt b/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateBannerController.kt index 188bfff6..9d92a247 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateBannerController.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateBannerController.kt @@ -19,7 +19,8 @@ private const val POLL_INTERVAL_MS = 24 * 60 * 60 * 1000L /** * Drives the shell's soft "update available" banner. Polls * `/api/client/version` at launch + every 24h and, when the bundled - * APK is strictly newer than this build, exposes its [UpdateInfo] so + * APK outranks this build — by ordering key where the server reports one, + * by name otherwise — exposes its [UpdateInfo] so * [com.fabledsword.minstrel.update.ui.UpdateBanner] can nudge an * install. Mirrors Flutter's `ClientUpdateController`. * @@ -58,6 +59,13 @@ class UpdateBannerController @Inject constructor( private suspend fun runOnce() { val info = runCatching { repository.getLatest() }.getOrNull() ?: return - latest.value = info.takeIf { isVersionNewer(it.version, BuildConfig.VERSION_NAME) } + latest.value = info.takeIf { + isUpdateAvailable( + serverCode = it.code, + serverName = it.version, + installedCode = BuildConfig.VERSION_CODE.toLong(), + installedName = BuildConfig.VERSION_NAME, + ) + } } } diff --git a/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateRepository.kt b/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateRepository.kt index 7a25ef45..6eddfd73 100644 --- a/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateRepository.kt +++ b/android/app/src/main/java/com/fabledsword/minstrel/update/data/UpdateRepository.kt @@ -21,10 +21,39 @@ class UpdateRepository @Inject constructor(retrofit: Retrofit) { private fun UpdateInfoWire.toDomain(): UpdateInfo = UpdateInfo( version = version, + code = code, + channel = channel, apkUrl = apkUrl, sizeBytes = sizeBytes, ) +/** + * True when [server] should be offered over the installed build. + * + * **Decide on the ordering key whenever the server sends one.** That is the + * same value Android's package installer compares, so an offer made this way + * implies an install the platform will actually accept. The app used to + * compare NAMES while the platform installed by `versionCode`, with nothing + * keeping the two orderings consistent — so it could offer a build Android + * then refused as a downgrade, or stay quiet about one it would have taken. + * + * Name comparison survives only as the fallback for a server that predates + * the field. A null code means "this server cannot tell me" — never "zero" — + * because treating absent as zero would rank every such server as infinitely + * old and offer its build to everyone, forever. + */ +fun isUpdateAvailable( + serverCode: Long?, + serverName: String, + installedCode: Long, + installedName: String, +): Boolean = + if (serverCode != null) { + serverCode > installedCode + } else { + isVersionNewer(serverName, installedName) + } + /** * True when [server] is strictly newer than [installed]. Mirrors * Flutter's `isVersionNewer` — splits both strings on `.`, parses diff --git a/android/app/src/test/java/com/fabledsword/minstrel/update/data/UpdateVersioningTest.kt b/android/app/src/test/java/com/fabledsword/minstrel/update/data/UpdateVersioningTest.kt new file mode 100644 index 00000000..130c8b6a --- /dev/null +++ b/android/app/src/test/java/com/fabledsword/minstrel/update/data/UpdateVersioningTest.kt @@ -0,0 +1,163 @@ +package com.fabledsword.minstrel.update.data + +import org.junit.jupiter.api.Test +import kotlin.test.assertFalse +import kotlin.test.assertTrue + +/** + * The update channel had no tests at all before this. That is worth saying + * out loud, because the thing it decides — whether anyone is ever offered an + * update — fails silently in both directions: an update nobody is offered + * looks exactly like being up to date, and nobody files a bug about a prompt + * they never saw. + */ +class UpdateVersioningTest { + @Test + fun `decides on the ordering key when the server reports one`() { + assertTrue( + isUpdateAvailable( + serverCode = 3523847, serverName = "2026.09.10.1432", + installedCode = 3519456, installedName = "2026.09.09.1828", + ), + ) + assertFalse( + isUpdateAvailable( + serverCode = 3519456, serverName = "2026.09.09.1828", + installedCode = 3523847, installedName = "2026.09.10.1432", + ), + ) + } + + @Test + fun `an equal ordering key is not an update`() { + assertFalse( + isUpdateAvailable( + serverCode = 3523847, serverName = "2026.09.10.1432", + installedCode = 3523847, installedName = "2026.09.10.1432", + ), + ) + } + + /** + * The property the whole rework exists for: the offer must agree with what + * the platform will actually install. Where the two disagree, the ordering + * key wins, because that is the value Android compares. + */ + @Test + fun `the ordering key wins even when the name disagrees`() { + // Name looks older, key is newer — e.g. an older commit rebuilt later. + assertTrue( + isUpdateAvailable( + serverCode = 9_000_000, serverName = "2020.01.01.0000", + installedCode = 1, installedName = "2099.12.31.2359", + ), + ) + // Name looks newer, key is not. Offering this would be offering an + // install the platform then refuses as a downgrade. + assertFalse( + isUpdateAvailable( + serverCode = 1, serverName = "2099.12.31.2359", + installedCode = 9_000_000, installedName = "2020.01.01.0000", + ), + ) + } + + @Test + fun `falls back to the name when the server reports no ordering key`() { + assertTrue( + isUpdateAvailable( + serverCode = null, serverName = "2026.09.10.1432", + installedCode = 3519456, installedName = "2026.09.09.1828", + ), + ) + assertFalse( + isUpdateAvailable( + serverCode = null, serverName = "2026.09.09.1828", + installedCode = 3519456, installedName = "2026.09.10.1432", + ), + ) + } + + /** + * A null code must never be read as zero. Zero would rank every + * older server as infinitely behind and offer its build to everyone, + * forever — so this asserts the fallback runs instead of a comparison + * against 0 succeeding by accident. + */ + @Test + fun `a null ordering key is absent, not zero`() { + // installedCode is 0 here: if null coerced to 0, "0 > 0" would be + // false and this would wrongly report no update despite a newer name. + assertTrue( + isUpdateAvailable( + serverCode = null, serverName = "2026.09.10.1432", + installedCode = 0, installedName = "2026.09.09.1828", + ), + ) + } + + /** + * The recorded migration constraint, pinned so it cannot be forgotten: + * the old scheme's fourth segment was a commit count (~1895), the new + * one is HHMM. Across a day boundary the date decides and all is well. + */ + @Test + fun `new-scheme name outranks an old-scheme name on a later day`() { + assertTrue(isVersionNewer("2026.09.10.1432", "2026.09.09.1895")) + } + + /** + * ...but on the SAME day the comparison comes down to HHMM against a + * commit count, and any build before ~19:00 UTC reads as older. This is + * why the first new-scheme release had to be cut on a later calendar day. + * Asserting the trap so nobody "fixes" it by accident. + */ + @Test + fun `same-day new-scheme name can read older than an old-scheme name`() { + assertFalse(isVersionNewer("2026.09.09.1828", "2026.09.09.1895")) + } + + @Test + fun `name comparison degrades per segment rather than discarding`() { + // The string is still compared rather than rejected outright: an + // earlier segment decides and the unparseable tail never matters. + assertTrue(isVersionNewer("2026.09.10.1432-dev", "2026.09.09.1828")) + + // A shorter name pads with zeros instead of being refused. + assertTrue(isVersionNewer("2026.09.10", "2026.09.09.9999")) + assertFalse(isVersionNewer("2026.09.10", "2026.09.10.0")) + } + + /** + * What "costs that segment's precision" actually means, and it is worth + * pinning because it is a real edge rather than a nicety: when the + * unparseable segment is the DECIDING one, it reads as 0 and loses. So a + * `-dev` suffixed build compares as older than an unsuffixed one from the + * same minute. + * + * That is the correct behaviour for a degrading parser — it is bounded + * loss rather than a discarded string — but it is exactly why the channel + * belongs in its own field and never in the name. + */ + @Test + fun `an unparseable deciding segment reads as zero and loses`() { + assertFalse(isVersionNewer("2026.09.10.1432-dev", "2026.09.10.1000")) + } + + /** + * Both sides unparseable (branch-name builds) falls back to string + * inequality, so a dev build still surfaces rather than comparing equal + * and going silent. + */ + @Test + fun `two unparseable names fall back to string inequality`() { + assertTrue(isVersionNewer("main", "dev")) + assertFalse(isVersionNewer("dev", "dev")) + } + + @Test + fun `a leading v is ignored on either side`() { + assertTrue(isVersionNewer("v2026.09.10.1432", "2026.09.09.1828")) + assertFalse(isVersionNewer("v2026.09.10.1432", "v2026.09.10.1432")) + } +}