fix(android): decide updates on the ordering key, not the version name
android / Build + lint + test (push) Successful in 3m55s
android / Build + lint + test (push) Successful in 3m55s
The app compared NAMES while Android installs by versionCode, with nothing keeping the two orderings consistent. So it could offer a build the platform then refused as a downgrade, or stay silent about one it would have accepted. The offer and the install were asking different questions. Both consumers — the shell banner and the About card — now route through one isUpdateAvailable(): decide on the ordering key whenever the server reports one, since that is the same value the package installer compares, so an offer implies an install that will actually be accepted. Name comparison survives only as the fallback for a server predating the field. isVersionNewer is deliberately untouched. It already degrades per segment and is not what was broken; rewriting it while nearby would have put the fallback path at risk for no gain. code is nullable on the wire, and that is load-bearing rather than stylistic. The app's Json sets coerceInputValues = true, which replaces a JSON null with the declared default on a NON-nullable property — so `val code: Long = 0` would have turned "this server reports no ordering key" into "its key is 0" silently, ranking every such server as infinitely behind and offering its build to everyone forever. Reading the field declaration alone would never show that; it lives in AppModule. A third caller turned up during the sweep and was deliberately left alone. NetworkStatusController compares the /healthz minClientVersion, which is a server-declared compatibility floor rather than the bundled APK — there is no ordering key on that wire at all, so names remain the only thing it can compare. Different question, correctly still using the old helper. The update channel had no tests whatsoever before this, which is worth stating: the thing deciding whether anyone is ever offered an update fails silently in both directions. The new suite pins that the key wins when it disagrees with the name, that a null key falls back rather than reading as zero, the recorded migration constraint (a new-scheme name outranks an old-scheme one across a day boundary but NOT within the same day), and the degradation cases — including that an unparseable DECIDING segment reads as zero and loses, which is why the channel must never live inside the name. Every assertion was checked against the real comparison by mirroring it, rather than from reading it: two of my first-draft comments described the wrong mechanism and were corrected on the evidence. Step 4 of 5 — Scribe task #3811, milestone #390. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SQ31KQpYbStyK5y58UmPLH
This commit is contained in:
@@ -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,
|
||||
)
|
||||
|
||||
@@ -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,
|
||||
)
|
||||
|
||||
+17
-4
@@ -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
|
||||
|
||||
+10
-2
@@ -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,
|
||||
)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user