diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt index d61742e..0c10e51 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt @@ -29,7 +29,8 @@ import kotlin.time.Instant * 3. **Downloads**, including everything the write phase decided we now need a * fresh copy of. * 4. **Sweep**, which is the only step that may delete a row it did not see fail - * — and so it runs last, on a listing taken before any of the writes. + * — and so it runs last, on a listing taken before any of the writes. Its + * local side, unlike that listing, is read *after* the download. * * ⚠️ **A failed resource must not fail the collection**, the twin of "a failed * collection must not fail the account". Every per-resource outcome below either @@ -179,9 +180,10 @@ class CollectionSyncer( ref.href.toString() to ref.eTag?.takeIf { it.usable }?.value } - val locals = localResources() - downloadPhase(locals, remoteETags) - sweepPhase(locals, remoteETags.keys) + downloadPhase(localResources(), remoteETags) + // ⚠️ Re-read. The download just ran and may have re-pointed a row's + // href; the pre-download list still names the vacated one. + sweepPhase(localResources(), remoteETags.keys) report = report.copy(reconciledInFull = true) // ⚠️ Adopted only now, and taken from the PROPFIND that *preceded* @@ -709,6 +711,12 @@ class CollectionSyncer( // ------------------------------------------------------------ phase 4 + /** + * @param locals must be read *after* [downloadPhase]. [apply] can re-point + * an existing row at a new href when the same UID reappears under a new + * filename, and [purge] deletes by row id — so a pre-download list sweeps + * away the row the download just repaired. + */ private fun sweepPhase(locals: List, remoteHrefs: Set) { // ⚠️ An empty listing never sweeps. The sweep is the one phase that // deletes rows it did not see fail, and its evidence is a diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt index 49669c8..3dd9da8 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt @@ -317,6 +317,43 @@ class CollectionSyncerTest { assertThat(report.deletedLocally).isEqualTo(1) } + @Test fun `a task recreated under a new href is repointed, not swept`() { + // Another client deleted one.ics and recreated the same UID as two.ics. + remote.put("two.ics", vtodo("a", "Buy milk")) + store.rows += task(id = 5, uid = "a", title = "Buy milk") + .copy( + href = "http://server/dav/tasks/one.ics", + etag = "e-one.ics", + sortOrder = 42, + color = 0xFF0000, + ) + + val report = sync() + + // The download re-points row 5; a sweep reading the pre-download list + // would purge it by id and lose the local-only columns for good. + val row = store.rows.single() + assertThat(row.id).isEqualTo(5) + assertThat(row.href).isEqualTo("http://server/dav/tasks/two.ics") + assertThat(row.sortOrder).isEqualTo(42) + assertThat(row.color).isEqualTo(0xFF0000) + assertThat(report.deletedLocally).isEqualTo(0) + assertThat(report.downloaded).isEqualTo(1) + } + + @Test fun `the post-download sweep still removes a stale row`() { + remote.put("one.ics", vtodo("a", "New")) + store.rows += task(id = 1, uid = "b") + .copy(href = "http://server/dav/tasks/gone.ics", etag = "e") + + val report = sync() + + // Reading locals after the download widens what the sweep sees; the row + // just inserted must survive it, the stale one must not. + assertThat(store.rows.map { it.uid }).containsExactly("a") + assertThat(report.deletedLocally).isEqualTo(1) + } + @Test fun `a create the server keeps refusing is eventually quarantined`() { store.rows += task(id = 1, uid = "a", title = "Never acceptable").copy(isDirty = true) remote.onPut = { PutOutcome.Rejected(415, "Unsupported Media Type") }