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 1d2bb01..95dab2b 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 @@ -162,7 +162,7 @@ class CollectionSyncer( // The write phase asked for fresh copies the change log may never // mention — a server does not have to echo our own writes back to // us — so they are fetched explicitly rather than hoped for. - fetchAndApply(refetch.filterNot { isQuarantined(it) }) + fetchAndApply(refetch.filterNot { isQuarantined(it) || it in deferred }) } resolveParents() @@ -315,6 +315,11 @@ class CollectionSyncer( val byHref = localResources().filter { it.href != null }.associateBy { it.href!! } val wanted = changed.filterNot { ref -> val href = ref.href.toString() + // The write phase gave up on it this run. Checked before the + // `byHref` lookup, because the case it exists for is a row whose + // href is still null — invisible to that map, and so past the + // dirty-row guard below. + if (href in deferred) return@filterNot true val local = byHref[href] ?: return@filterNot false // ⚠️ A row still dirty after the write phase is an edit that never // reached the server — the collection was demoted to read-only, or @@ -491,8 +496,25 @@ class CollectionSyncer( if (eTag == null) { // No usable validator on record. Ask for one before writing, // rather than writing blind. - eTag = remote.fetch(listOf(url)).getOrNull() - ?.resources?.firstOrNull() + // + // ⚠️ A fetch that *failed* says we do not know, not that the + // server has none to offer. Falling through would PUT + // unconditionally over whatever a concurrent edit had left + // there, and book it under `unconditionalWrites` — whose whole + // meaning is that the server offered no validator. A success + // that yields nothing usable does mean that, and still writes. + // + // `deferred` is redundant on the direct call, where the row + // keeps its href and the dirty-row guards can see it. It is + // load-bearing on the delegated one from `createResource`, where + // the row's href is still null and only this suppresses a + // download that would overwrite the edit by UID. + val fetched = remote.fetch(listOf(url)).getOrElse { + deferred += href + skip(local.key, "update validator fetch failed: $it") + return + } + eTag = fetched.resources.firstOrNull() ?.eTag?.takeIf { it.usable }?.value if (eTag == null) { report = report.copy(unconditionalWrites = report.unconditionalWrites + 1) 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 27c3eb9..0112bef 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 @@ -139,6 +139,59 @@ class CollectionSyncerTest { assertThat(remote.resources.values.single().body).contains("SUMMARY:New") } + @Test fun `a failed validator fetch defers the update instead of writing blind`() { + remote.put("one.ics", vtodo("a", "Server")) + // No local etag: what StoredNeedsRefetch, and any server that serves none, + // leave behind. + store.rows += task(id = 1, uid = "a", title = "Mine") + .copy(href = "http://server/dav/tasks/one.ics", isDirty = true) + var failNext = true + remote.onFetch = { + if (failNext) { + failNext = false + Result.failure(IllegalStateException("timeout")) + } else { + null + } + } + + val report = sync() + + // Not knowing the validator is not the same as the server having none. + assertThat(remote.log.none { it.startsWith("UPDATE") }).isTrue() + assertThat(remote.resources.values.single().body).contains("SUMMARY:Server") + assertThat(report.unconditionalWrites).isEqualTo(0) + val row = store.rows.single() + assertThat(row.title).isEqualTo("Mine") + assertThat(row.isDirty).isTrue() + assertThat(quarantine).isEmpty() + assertThat(report.quarantined.single().failures).isEqualTo(0) + assertThat(report.quarantined.single().reason).contains("validator fetch failed") + + remote.onFetch = null + sync() + + assertThat(remote.log).contains("UPDATE one.ics if-match=e-one.ics") + assertThat(remote.resources.values.single().body).contains("SUMMARY:Mine") + assertThat(store.rows.single().isDirty).isFalse() + assertThat(quarantine).isEmpty() + } + + @Test fun `a server with no validator to offer is still written to`() { + remote.put("one.ics", vtodo("a", "Server"), eTag = null) + store.rows += task(id = 1, uid = "a", title = "Mine") + .copy(href = "http://server/dav/tasks/one.ics", isDirty = true) + + val report = sync() + + // The fetch succeeded and there was simply no validator — the case + // unconditionalWrites exists to count. Deferring here instead would stop + // syncing to every etag-less server. + assertThat(remote.log).contains("UPDATE one.ics if-match=null") + assertThat(report.unconditionalWrites).isEqualTo(1) + assertThat(remote.resources.values.single().body).contains("SUMMARY:Mine") + } + @Test fun `a PUT that returns no usable etag triggers a refetch`() { store.rows += task(id = 1, uid = "a", title = "New").copy(isDirty = true) remote.onPut = { href -> PutOutcome.StoredNeedsRefetch(href) } @@ -532,6 +585,30 @@ class CollectionSyncerTest { assertThat(quarantine).isEmpty() } + @Test fun `an adoption whose validator fetch fails keeps the local edit`() { + // Create 412s, the first fetch proves the resource is ours, and the + // validator fetch inside the adoption then fails. + remote.put("a.ics", vtodo("a", "Uploaded last time")) + store.rows += task(id = 1, uid = "a", title = "Edited after the lost PUT") + .copy(isDirty = true) + var fetches = 0 + remote.onFetch = { + fetches += 1 + if (fetches == 2) Result.failure(IllegalStateException("timeout")) else null + } + + sync() + + // The row still has no href here, so downloadPhase cannot match it and + // only `deferred` keeps apply from overwriting the edit by UID. This is + // the call site that makes updateResource's `deferred` load-bearing. + assertThat(remote.resources).hasSize(1) + val row = store.rows.single() + assertThat(row.title).isEqualTo("Edited after the lost PUT") + assertThat(row.isDirty).isTrue() + assertThat(row.href).isNull() + } + // ------------------------------------------------------------ protection @Test fun `a read-only collection is never written to`() { diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/IncrementalSyncTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/IncrementalSyncTest.kt index 9cb39ee..dad3709 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/IncrementalSyncTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/IncrementalSyncTest.kt @@ -291,6 +291,34 @@ class IncrementalSyncTest { // --------------------------------------------------------------- setup + @Test fun `a deferred create is not overwritten by the change log`() { + // A previous run's PUT landed but its answer was lost; the user has since + // edited the task, so the row is dirty with no href. + remote.put("a.ics", vtodo("a", "On the server")) + store.rows += task(id = 1, uid = "a", title = "Edited after the lost PUT") + .copy(isDirty = true) + remote.changePages += page(changed = listOf("a.ics"), token = "urn:x:2") + var failNext = true + remote.onFetch = { + if (failNext) { + failNext = false + Result.failure(IllegalStateException("timeout")) + } else { + null + } + } + + sync() + + // downloadChanged matches by href too, so a row with none is past its + // dirty-row guard: without honouring `deferred` here, apply matches by + // UID and the edit is gone with nothing in discardedEdits. + val row = store.rows.single() + assertThat(row.title).isEqualTo("Edited after the lost PUT") + assertThat(row.isDirty).isTrue() + assertThat(row.href).isNull() + } + private fun page( changed: List = emptyList(), removed: List = emptyList(),