fix: correct geofence window handling and protect user data on upgrade - #71
Merged
Merged
Conversation
Breaks that could reach users, all verified against a real device holding a v3 database with existing data: - Database: `fallbackToDestructiveMigration` was active with no registered migrations, so every app update wiped all locations and reminders. Adds the full 1->5 chain; each step is validated by MigrationTestHelper against the exported schema. - Notifications: `POST_NOTIFICATIONS` was checked without an SDK guard. The permission does not exist below API 33, so the check always reported denied and no notification was ever posted across the whole supported range (minSdk 24). - Master geofence: registered with only `GEOFENCE_TRANSITION_EXIT` while requesting `INITIAL_TRIGGER_ENTER`, a combination GMS silently ignores. - Spatial search: the bounding-box query only handled the antimeridian when minLon > maxLon, so a window near +/-180 discarded every point on the far side. The window is now tested at +/-360 degrees. - Worker: `ExistingWorkPolicy.REPLACE` could cancel a running recalculation between removing and re-registering geofences, leaving none registered. Expedited and debounced work now use separate names and policies. - Worker: derived state was persisted before GMS accepted the new window, so the UI could claim geofences that did not exist. - Errors: `catch (e: Exception)` swallowed `CancellationException` in the repositories, ViewModels, worker and broadcast receiver. - Messages: `MutableSharedFlow(replay = 0)` discarded error events emitted while nothing was collecting, which is exactly when they matter. Replaced with a buffered channel. - `SettingsManager` treated 0.0 as "unset" for the last recalculation centre, making the valid coordinate (0.0, 0.0) unrepresentable, and wrote the two coordinates in separate transactions that could be read torn. - Aliases were stored untrimmed while the unique index is `COLLATE NOCASE`, so the UI reported "alias available" and SQLite then rejected the insert. New writes are trimmed; a v4->v5 migration fixes existing rows, skipping any that would collide on the unique index or become blank. - App Functions mutations never triggered a recalculation, so reminders created by the assistant had no effect until the app was next launched. - Notifications shared the reminder id as a `PendingIntent` request code, so colliding ids overwrote each other's intent extras. Cleanups: remove the unused navigation and Hilt-navigation dependencies, two dead DI providers, two dead `GeofenceManager` methods, a dead DAO query, an uncollected ViewModel flow, and 14 unused string resources across all 21 locales. Extract `BaseViewModel`, `ReminderStatusChips` and `ThemeSetting.resolve` to remove repeated ViewModel and Compose logic, and deduplicate the three geofence builders. Compose: remember the dynamic color scheme, replace the `as Activity` cast with a safe one, restore the navigation-bar icon appearance, make the tab switch exhaustive, add a back handler, keep form state across rotation, derive map overlay colours from the theme, and call `MapView.onDetach()` when a map leaves composition so its tile thread pool is not leaked. Tests: 142 unit tests and 8 instrumented tests pass, lint is clean. Adds MigrationTest coverage for the alias-trimming SQL, since a correlated subquery that forgets to qualify its outer reference makes the guard permanently true and the migration silently trims nothing.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resumen
Corrección del manejo de la ventana deslizante de geofences y protección de los datos del usuario al actualizar la base de datos.
Todos los fallos de la sección "Correctness" se verificaron contra un dispositivo real con una base de datos en v3 con datos existentes, no solo por lectura de código.
Correctness
Pérdida de datos al actualizar
fallbackToDestructiveMigrationestaba activo sin ninguna migración registrada, así que cada actualización de la app borraba todas las ubicaciones y recordatorios. Se declara la cadena completa 1→5, y cada paso se valida conMigrationTestHelpercontra el schema exportado.Verificado en dispositivo: BD v3 con 5 ubicaciones y 2 recordatorios → tras instalar, v5, integridad correcta, los 7 registros intactos.
Notificaciones nunca mostradas
NotificationHelpercomprobabaPOST_NOTIFICATIONSsin guard de versión de SDK. Ese permiso no existe por debajo de API 33, así quecheckSelfPermissiondevolvía siempreDENIEDynotify()nunca se alcanzaba en todo el rango soportado (minSdk 24).Geofence principal inerte
Se registraba con
setTransitionTypes(GEOFENCE_TRANSITION_EXIT)pidiendosetInitialTrigger(INITIAL_TRIGGER_ENTER). GMS ignora un trigger inicial cuyo tipo no está en la máscara de transiciones de la geofence.POIs perdidos en el antimeridiano
El bounding box solo contemplaba el cruce cuando
minLon > maxLon. Cerca de ±180 la ventana queda en174..184, la rama normal se activa y se descartan las longitudes negativas:-179.5nunca se consultaba. La ventana se prueba ahora desplazada ±360 grados.Verificado con SQLite: antes devolvía
a, c; ahora también devuelveb, d.Worker de recálculo
ExistingWorkPolicy.REPLACEpodía cancelar un worker en ejecución entreremoveAllGeofences()y el registro, dejando cero geofences. Ahora expedited y debounced usan nombres y políticas separadas (KEEPpara no interrumpir un en curso,REPLACEpara colapsar rafagas de edición).Result.retry()sin límite generaba cadenas de reintento eternas en dispositivos sin fix.Errores y mensajes
catch (e: Exception)tragándoseCancellationExceptionen repositorios, ViewModels, worker y receiver.MutableSharedFlow(replay = 0)descartaba los mensajes de error emitidos sin collectors, justo cuando importan. Sustituido por unChannelcon búfer."Error: null"enRemindersViewModelcuando la excepción no tenía mensaje.Alias con espacios (bug presente en datos reales)
Los alias se guardaban sin
trimmientras el índice único esCOLLATE NOCASE, que ignora mayúsculas pero no espacios. La UI decía "alias disponible" y SQLite rechazaba el insert conUNIQUE constraint failed.Las escrituras nuevas se normalizan, y la migración v4→v5 limpia las existentes. rows que colisionarían en el índice único o quedarían en blanco se conservan intactas en vez de romper la app.
Otros
SettingsManagertrataba0.0como "sin valor", haciendo la coordenada válida(0.0, 0.0)irrepresentable, y escribía lat/lng en transacciones separadas que podían leerse descuadradas.PendingIntent, así que ids en colisión sobrescribían los extras del intent del otro.Limpieza
Código muerto eliminado: dependencias
navigation-composeyhilt-navigation-composesin uso, 2 providers DI, 2 métodos deGeofenceManager, 1 query del DAO, un flow de ViewModel sin consumir y 14 recursos de string en los 21 idiomas.Duplicación extraída a
BaseViewModel,ReminderStatusChipsyThemeSetting.resolve. Los tres constructores de geofence estaban duplicados.Compose:
rememberen elColorSchemedinámico, castas Activityreemplazado por uno seguro, iconos de la barra de navegación,whenexhaustivo en pestañas,BackHandler, estado de formularios persistente al rotar, colores de overlays derivados del tema, yMapView.onDetach()al salir de composición.onDetach()se都不用 antes porque el comentario que lo justificaba era falso: verificado en el bytecode de osmdroid 6.1.20 queSqlTileWriter.onDetach()es un no-op y que el executor de tiles sí se filtra por instancia deMapView.Verificación
Los tests de migración cubren el SQL de recorte de alias porque un subquery correlacionado que olvide cualificar su referencia externa hace que la guarda sea siempre cierta y la migración recorte nada en silencio, mientras la app abre con normalidad.
Nota
androidx.room:room-testingrequierekotlinx-serialization1.8.1 pero la consistent resolution de Gradle la fijaba a 1.7.3, provocandoAbstractMethodError. Se fija con un constraint documentado.