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 02d849e..d8e538b 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 @@ -655,7 +655,22 @@ class CollectionSyncer( // `apply`, so nothing counts it — without this it is re-requested // on every sync for ever, which is the loop quarantine exists to // break. - fetched.missing.forEach { fail(it.toString(), "listed but not returned") } + // ⚠️ When the server both omitted hrefs we asked for and + // volunteered hrefs we did not, "omitted" and "we failed to + // recognise its spelling" are indistinguishable — and only one + // of the two verdicts is reversible. A download-side count has + // no refund: at THRESHOLD the href is stripped before the fetch, + // so `apply` can never run to clear it. A wasteful re-request is + // recoverable; a permanent silent drop of a good task is not. + val unmatched = fetched.unsolicited.isNotEmpty() + fetched.missing.forEach { + val href = it.toString() + if (unmatched) { + skip(href, "listed but not returned, among hrefs we did not ask for") + } else { + fail(href, "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 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 497a278..91dda04 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 @@ -516,6 +516,50 @@ class CollectionSyncerTest { assertThat(report.quarantined.single().failures).isEqualTo(0) } + @Test fun `a missing href is not counted when the server also sent strays`() { + remote.put("one.ics", vtodo("a", "Fine")) + remote.onFetch = { hrefs -> + Result.success( + FetchResult( + resources = emptyList(), + missing = hrefs, + failed = emptyList(), + // The shape an href we failed to recognise takes: one + // resource in both buckets. + unsolicited = listOf("http://server/dav/tasks/one.ics".toHttpUrl()), + ), + ) + } + + val report = sync() + + // ⚠️ Counting here is irreversible — at THRESHOLD the href is stripped + // before the fetch, so nothing can ever clear it again. + assertThat(quarantine).isEmpty() + assertThat(report.quarantined.single().failures).isEqualTo(0) + } + + @Test fun `a missing href is still counted when nothing was unmatched`() { + remote.put("one.ics", vtodo("a", "Fine")) + remote.onFetch = { hrefs -> + Result.success( + FetchResult( + resources = emptyList(), + missing = hrefs, + failed = emptyList(), + unsolicited = emptyList(), + ), + ) + } + + val report = sync() + + // A plain omission repeats identically every run and nothing else breaks + // the loop, which is what the counter is for. + assertThat(quarantine.values.single()).isEqualTo(1) + assertThat(report.quarantined.single().failures).isEqualTo(1) + } + @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) } diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt index 25f7cb9..501c8b8 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt @@ -178,6 +178,26 @@ class CalendarCollection( */ override fun fetch(hrefs: List): Result = fetch(hrefs, MULTIGET_BATCH) + /** + * What makes two hrefs the same resource. + * + * ⚠️ Not the raw `encodedPath`. The request href is *ours*, written into the + * REPORT body verbatim, and the response href is the server's own spelling + * of it — a server that answers `/my@dav.ics` for a requested `/my%40dav.ics` + * is the reason `UrlUtils.equals` exists at all. Compared raw, that resource + * lands in `unsolicited` *and* in `missing`, its intact body is dropped, and + * three such runs quarantine it out of the download for good, since nothing + * on this side can refund the count. + * + * `pathSegments` is decoded, so both spellings agree. Keeping it a list, not + * a joined string, stops `/a%2Fb` colliding with `/a/b`. Scheme, host and + * port ride along because a path-only match would accept a body from another + * host entirely. The trailing slash still distinguishes, exactly as + * `UrlUtils.equals` refuses to normalise it. + */ + private fun identityOf(url: HttpUrl): List = + listOf(url.scheme, url.host, url.port.toString()) + url.pathSegments + /** [batchSize] is a seam for tests; production always uses [MULTIGET_BATCH]. */ internal fun fetch(hrefs: List, batchSize: Int): Result = runCatching { @@ -187,10 +207,10 @@ class CalendarCollection( val seen = mutableSetOf() hrefs.chunked(batchSize).forEach { batch -> - val wanted = batch.associateBy { it.encodedPath } + val wanted = batch.associateBy { identityOf(it) } dav.multiget(batch, MIME_ICALENDAR, ICALENDAR_VERSION) { response, relation -> if (relation == Response.HrefRelation.SELF) return@multiget - val asked = wanted[response.href.encodedPath] + val asked = wanted[identityOf(response.href)] if (asked == null) { unsolicited += response.href return@multiget diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt index 93bf23c..04f46ef 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt @@ -101,6 +101,106 @@ END:VCALENDAR assertThat(result.missing.map { it.encodedPath }).containsExactly("/dav/tasks/two.ics") } + @Test fun `a server that respells the path is still matched`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/my%40dav.ics + + "e1" + BEGIN:VCALENDAR +END:VCALENDAR + HTTP/1.1 200 OK + + """, + ), + ) + + val asked = href("my@dav.ics") + val result = collection.fetch(listOf(asked)).getOrThrow() + + // ⚠️ Compared raw, one resource lands in unsolicited *and* missing, its + // body is dropped, and three such runs quarantine it out of the download + // for good — nothing on that side can refund the count. + assertThat(result.resources).hasSize(1) + assertThat(result.missing).isEmpty() + assertThat(result.unsolicited).isEmpty() + assertThat(result.failed).isEmpty() + // The href we asked for, not the server's spelling: apply is keyed on + // the local row's href. + assertThat(result.resources.single().href).isEqualTo(asked) + } + + @Test fun `the same path on another host is not our resource`() { + server.enqueue( + multistatus( + """ + + https://evil.example.com/dav/tasks/one.ics + + "e1" + BEGIN:VCALENDAR +END:VCALENDAR + HTTP/1.1 200 OK + + """, + ), + ) + + val result = collection.fetch(listOf(href("one.ics"))).getOrThrow() + + // Matching on the path alone would apply one host's body to another + // host's row. + assertThat(result.resources).isEmpty() + assertThat(result.unsolicited.map { it.host }).containsExactly("evil.example.com") + assertThat(result.missing).hasSize(1) + } + + @Test fun `a trailing slash is still a different resource`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/one.ics/ + "e1"HTTP/1.1 200 OK + + """, + ), + ) + + val result = collection.fetch(listOf(href("one.ics"))).getOrThrow() + + // Deliberately not normalised, as UrlUtils.equals also refuses to. + assertThat(result.resources).isEmpty() + assertThat(result.missing).hasSize(1) + } + + @Test fun `an encoded slash is not the same as a real one`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/a/b.ics + + "e1" + BEGIN:VCALENDAR +END:VCALENDAR + HTTP/1.1 200 OK + + """, + ), + ) + + val asked = server.url("/dav/tasks/a%2Fb.ics") + val result = collection.fetch(listOf(asked)).getOrThrow() + + // One segment "a/b" is not two segments "a" and "b" — which is why the + // key stays a list rather than a joined string. + assertThat(result.resources).isEmpty() + assertThat(result.missing).containsExactly(asked) + } + @Test fun `a refused resource is reported as failed, not as missing`() { server.enqueue( multistatus(