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 95dab2b..02d849e 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 @@ -464,6 +464,17 @@ class CollectionSyncer( skip(local.key, "create verification failed: $it") return } + // A refusal is not an answer either. It arrives as a + // *successful* multiget carrying a per-resource error, so + // without this the empty `resources` reads as "somebody + // else's" and we take a fresh name — the duplicate UID + // this branch exists to avoid. + if (fetched.failed.isNotEmpty()) { + deferred += outcome.href.toString() + val code = fetched.failed.first().code + skip(local.key, "create verification refused: $code") + return + } val existing = fetched.resources.firstOrNull() val sameTask = existing != null && uidOf(existing.iCalendar) == local.uid if (sameTask) { @@ -514,6 +525,12 @@ class CollectionSyncer( skip(local.key, "update validator fetch failed: $it") return } + // Same distinction: refused is not "has none to offer". + if (fetched.failed.isNotEmpty()) { + deferred += href + skip(local.key, "validator fetch refused: ${fetched.failed.first().code}") + return + } eTag = fetched.resources.firstOrNull() ?.eTag?.takeIf { it.usable }?.value if (eTag == null) { @@ -639,6 +656,29 @@ class CollectionSyncer( // on every sync for ever, which is the loop quarantine exists to // break. fetched.missing.forEach { fail(it.toString(), "listed but not returned") } + fetched.failed.forEach { failure -> + // ⚠️ Quarantining here also silences the upload phase and the + // sweep, because a row's quarantine key *is* its href. So the + // verdict has to distinguish what the server actually said. + val href = failure.href.toString() + when (failure.code) { + // Gone. The next listing omits it and the sweep purges the + // row — counting it would quarantine the resource out of + // the very sweep that would have cleaned it up. + HTTP_NOT_FOUND, HTTP_GONE -> + skip(href, "the server no longer has this resource: ${failure.code}") + // The server failing on its own stored object, or on the + // batch. A multiget carries no body of ours, so there is + // nothing here for a server to reject permanently. + 0, in 500..599 -> + skip(href, "the server could not return this resource: ${failure.code}") + // A judgement about this resource — a per-object ACL, or a + // share hiding one object. Deterministic, and nothing else + // breaks the loop. + else -> + fail(href, "the server refused this resource: ${failure.code}") + } + } } } @@ -940,6 +980,9 @@ class CollectionSyncer( } private companion object { + const val HTTP_NOT_FOUND = 404 + const val HTTP_GONE = 410 + /** RFC 5545 `CLASS:CONFIDENTIAL`, as `tasks.classification` stores it. */ const val CLASS_CONFIDENTIAL = 2 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 0112bef..497a278 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 @@ -3,7 +3,12 @@ package de.jeanlucmakiola.agendula.data.sync import com.google.common.truth.Truth.assertThat import de.jeanlucmakiola.agendula.data.tasks.room.TaskEntity import de.jeanlucmakiola.agendula.data.tasks.room.TaskListEntity +import de.jeanlucmakiola.caldav.FetchFailure +import de.jeanlucmakiola.caldav.FetchResult import de.jeanlucmakiola.caldav.PutOutcome +import de.jeanlucmakiola.caldav.RemoteResource +import okhttp3.HttpUrl +import okhttp3.HttpUrl.Companion.toHttpUrl import org.junit.jupiter.api.Test import kotlin.time.Instant @@ -471,6 +476,60 @@ class CollectionSyncerTest { assertThat(report.deletedLocally).isEqualTo(1) } + @Test fun `a resource the server refuses is counted, and the batch survives`() { + remote.put("one.ics", vtodo("a", "Fine")) + remote.put("two.ics", vtodo("b", "Refused")) + remote.onFetch = { hrefs -> refuseTwoIcs(hrefs, code = 403) } + + val report = sync() + + // A refusal on one member says nothing about the others. + assertThat(store.rows.map { it.uid }).containsExactly("a") + assertThat(quarantine.values.single()).isEqualTo(1) + assertThat(report.quarantined.single().failures).isEqualTo(1) + assertThat(report.quarantined.single().reason).contains("403") + } + + @Test fun `a resource the server says is gone is not counted`() { + remote.put("one.ics", vtodo("a", "Fine")) + remote.put("two.ics", vtodo("b", "Gone")) + remote.onFetch = { hrefs -> refuseTwoIcs(hrefs, code = 404) } + + val report = sync() + + // Counting it would quarantine the resource out of the sweep that is + // about to clean it up once the listing drops it. + assertThat(quarantine).isEmpty() + assertThat(report.quarantined.single().failures).isEqualTo(0) + } + + @Test fun `a resource the server fails on is not counted`() { + remote.put("one.ics", vtodo("a", "Fine")) + remote.put("two.ics", vtodo("b", "Broken")) + remote.onFetch = { hrefs -> refuseTwoIcs(hrefs, code = 500) } + + val report = sync() + + // A multiget carries no body of ours, so there is nothing here a server + // could be permanently right to reject. + assertThat(quarantine).isEmpty() + assertThat(report.quarantined.single().failures).isEqualTo(0) + } + + @Test fun `a resource the server keeps refusing stops being requested`() { + remote.put("two.ics", vtodo("b", "Refused")) + remote.onFetch = { hrefs -> refuseTwoIcs(hrefs, code = 403) } + + repeat(QuarantineStore.THRESHOLD) { sync() } + remote.log.clear() + sync() + + // The whole point. It is in the listing, so the sweep leaves it alone, + // and it never reaches apply, so without a count it is re-requested on + // every sync for ever. + assertThat(remote.log).containsExactly("LIST") + } + @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") } @@ -609,6 +668,55 @@ class CollectionSyncerTest { assertThat(row.href).isNull() } + @Test fun `a refused verification fetch does not rename the create`() { + 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) + // A per-resource refusal is a *successful* multiget. Without inspecting + // `failed`, the empty resources list reads as somebody else's resource. + remote.onFetch = { hrefs -> + Result.success( + FetchResult( + resources = emptyList(), + missing = emptyList(), + failed = hrefs.map { FetchFailure(it, 403) }, + unsolicited = emptyList(), + ), + ) + } + + sync() + + assertThat(remote.resources).hasSize(1) + assertThat(remote.log.count { it.startsWith("CREATE") }).isEqualTo(1) + val row = store.rows.single() + assertThat(row.title).isEqualTo("Edited after the lost PUT") + assertThat(row.isDirty).isTrue() + } + + @Test fun `a refused validator fetch does not write unconditionally`() { + remote.put("one.ics", vtodo("a", "Server")) + store.rows += task(id = 1, uid = "a", title = "Mine") + .copy(href = "http://server/dav/tasks/one.ics", isDirty = true) + remote.onFetch = { hrefs -> + Result.success( + FetchResult( + resources = emptyList(), + missing = emptyList(), + failed = hrefs.map { FetchFailure(it, 500) }, + unsolicited = emptyList(), + ), + ) + } + + val report = sync() + + assertThat(remote.log.none { it.startsWith("UPDATE") }).isTrue() + assertThat(report.unconditionalWrites).isEqualTo(0) + assertThat(remote.resources.values.single().body).contains("SUMMARY:Server") + assertThat(store.rows.single().isDirty).isTrue() + } + // ------------------------------------------------------------ protection @Test fun `a read-only collection is never written to`() { @@ -673,6 +781,27 @@ class CollectionSyncerTest { // ----------------------------------------------------------------- setup + /** A multiget in which `two.ics` alone comes back refused. */ + private fun refuseTwoIcs(hrefs: List, code: Int): Result { + val refused = hrefs.filter { it.pathSegments.last() == "two.ics" } + val rest = hrefs - refused.toSet() + val served = remote.resources.filterKeys { key -> rest.any { it.toString() == key } } + return Result.success( + FetchResult( + resources = served.map { (href, stored) -> + RemoteResource( + href = href.toHttpUrl(), + eTag = stored.eTag, + iCalendar = stored.body, + ) + }, + missing = emptyList(), + failed = refused.map { FetchFailure(it, code) }, + unsolicited = emptyList(), + ), + ) + } + private fun task( id: Long, uid: String, diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncFakes.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncFakes.kt index b197521..0ec5c77 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncFakes.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncFakes.kt @@ -156,6 +156,7 @@ class FakeRemote(override val url: HttpUrl = "http://server/dav/tasks/".toHttpUr FetchResult( resources = found, missing = hrefs.filterNot { it.toString() in resources }, + failed = emptyList(), unsolicited = emptyList(), ), ) diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt index 065b4e1..25f7cb9 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt @@ -183,6 +183,7 @@ class CalendarCollection( runCatching { val resources = mutableListOf() val unsolicited = mutableListOf() + val failed = mutableListOf() val seen = mutableSetOf() hrefs.chunked(batchSize).forEach { batch -> @@ -194,9 +195,26 @@ class CalendarCollection( unsolicited += response.href return@multiget } + // Before the checks below, and deliberately: the server did + // mention this href, so whatever it said, the href is not + // one the server omitted. seen += asked - if (!response.isSuccess()) return@multiget - val body = response[CalendarData::class.java]?.iCalendar ?: return@multiget + if (!response.isSuccess()) { + failed += FetchFailure(asked, response.status?.code ?: 0) + return@multiget + } + val body = response[CalendarData::class.java]?.iCalendar + if (body == null) { + // Success at the response level, no body. The usual shape + // is a per-property refusal — a `propstat` carrying 403 + // around `calendar-data` — and `Response.properties` drops + // non-2xx propstats silently, so the verdict has to be + // read back out of them or a deterministic refusal + // arrives looking like a transient blank. + val refusal = response.propstat.firstOrNull { !it.isSuccess() } + failed += FetchFailure(asked, refusal?.status?.code ?: 0) + return@multiget + } resources += RemoteResource( href = asked, // The tag from *this* response, paired with *this* body. @@ -209,6 +227,12 @@ class CalendarCollection( FetchResult( resources = resources, missing = hrefs.filterNot { it in seen }, + // A server that repeats a response element must not spend two + // thirds of the quarantine threshold in one run — nor count + // against a resource it also answered properly. + failed = failed + .filterNot { failure -> resources.any { it.href == failure.href } } + .distinctBy { it.href }, unsolicited = unsolicited, ) } diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarResources.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarResources.kt index d95b60d..b52938b 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarResources.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarResources.kt @@ -64,11 +64,28 @@ data class RemoteRef(val href: HttpUrl, val eTag: ETag?) */ data class RemoteResource(val href: HttpUrl, val eTag: ETag?, val iCalendar: String) +/** + * A resource the server answered for, but did not hand over. + * + * @param code the response-level status, or `0` when the server reported + * success and supplied no `calendar-data` — the same "no HTTP judgement to + * report" convention [PutOutcome.Rejected] uses. + */ +data class FetchFailure(val href: HttpUrl, val code: Int) + /** What a multiget actually returned, and what it did not. */ data class FetchResult( val resources: List, /** Asked for, not answered — the server simply omitted them. */ val missing: List, + /** + * Asked for, answered, and refused or empty. + * + * Separate from [missing] because "the server said 403" and "the server said + * nothing" are different facts with different right answers — merging them + * is the same mistake as merging the three meanings of a 412. + */ + val failed: List, /** * Answered without being asked for. * diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt index 4bccbbb..93bf23c 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt @@ -101,6 +101,107 @@ END:VCALENDAR assertThat(result.missing.map { it.encodedPath }).containsExactly("/dav/tasks/two.ics") } + @Test fun `a refused resource is reported as failed, not as missing`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/one.ics + + "e1" + BEGIN:VCALENDAR +END:VCALENDAR + HTTP/1.1 200 OK + + + /dav/tasks/two.ics + HTTP/1.1 403 Forbidden + + """, + ), + ) + + val result = collection.fetch(listOf(href("one.ics"), href("two.ics"))).getOrThrow() + + // The server did mention two.ics, so it is not missing. Merging the two + // costs the caller the difference between "refused" and "omitted", which + // are not the same fact and do not deserve the same answer. + assertThat(result.missing).isEmpty() + assertThat(result.failed.map { it.href.encodedPath to it.code }) + .containsExactly("/dav/tasks/two.ics" to 403) + // One bad member does not cost the batch its good ones. + assertThat(result.resources.map { it.href.encodedPath }) + .containsExactly("/dav/tasks/one.ics") + } + + @Test fun `a success carrying no calendar-data is reported as failed`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/one.ics + + "e1" + HTTP/1.1 200 OK + + """, + ), + ) + + val result = collection.fetch(listOf(href("one.ics"))).getOrThrow() + + // No HTTP judgement to report: the server said yes and sent nothing. + assertThat(result.failed.map { it.href.encodedPath to it.code }) + .containsExactly("/dav/tasks/one.ics" to 0) + assertThat(result.resources).isEmpty() + assertThat(result.missing).isEmpty() + } + + @Test fun `a propstat refusal carries its own status, not a blank`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/one.ics + + "e1" + HTTP/1.1 200 OK + + + HTTP/1.1 403 Forbidden + + """, + ), + ) + + val result = collection.fetch(listOf(href("one.ics"))).getOrThrow() + + // A per-property refusal is the usual shape for "you may not read this + // one object". Response.properties drops non-2xx propstats, so reporting + // 0 here would make a deterministic refusal look like a transient blank. + assertThat(result.failed.map { it.code }).containsExactly(403) + } + + @Test fun `a resource the server says is gone carries its status`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/one.ics + HTTP/1.1 404 Not Found + + """, + ), + ) + + val result = collection.fetch(listOf(href("one.ics"))).getOrThrow() + + // The code has to survive the trip: the engine treats 404 and 403 as + // opposite verdicts. + assertThat(result.failed.map { it.code }).containsExactly(404) + assertThat(result.missing).isEmpty() + } + @Test fun `fetch batches`() { repeat(2) { server.enqueue(multistatus("")) } collection.fetch((1..3).map { href("$it.ics") }, batchSize = 2).getOrThrow()