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 d8e538b..a06cf32 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 @@ -662,11 +662,20 @@ class CollectionSyncer( // 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() + // ⚠️ Paired by path, not excused wholesale. Real servers answer + // with hrefs nobody asked for as a matter of course — a sibling, + // something from another collection — so "any stray at all" + // would mean a genuinely omitted resource is never counted and + // is re-requested for ever, which is the loop the count exists + // to break. Only a stray naming the *same path* is plausibly + // this href under a spelling we failed to recognise, and that is + // the one case where counting could permanently drop a resource + // that is really there. + val strayPaths = fetched.unsolicited.map { it.pathSegments }.toSet() fetched.missing.forEach { val href = it.toString() - if (unmatched) { - skip(href, "listed but not returned, among hrefs we did not ask for") + if (it.pathSegments in strayPaths) { + skip(href, "answered under a spelling we did not recognise") } else { fail(href, "listed but not returned") } 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 91dda04..f5a6067 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 @@ -524,9 +524,9 @@ class CollectionSyncerTest { 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()), + // The shape an href we failed to recognise takes: the same + // path, in both buckets. + unsolicited = listOf("http://elsewhere/dav/tasks/one.ics".toHttpUrl()), ), ) } @@ -539,6 +539,28 @@ class CollectionSyncerTest { assertThat(report.quarantined.single().failures).isEqualTo(0) } + @Test fun `a stray for another path does not excuse a missing one`() { + remote.put("one.ics", vtodo("a", "Fine")) + remote.onFetch = { hrefs -> + Result.success( + FetchResult( + resources = emptyList(), + missing = hrefs, + failed = emptyList(), + // Servers volunteer siblings routinely. Excusing every + // missing href on that basis would mean nothing is ever + // counted, and the loop never breaks. + unsolicited = listOf("http://server/dav/tasks/somebody-else.ics".toHttpUrl()), + ), + ) + } + + val report = sync() + + assertThat(quarantine.values.single()).isEqualTo(1) + assertThat(report.quarantined.single().failures).isEqualTo(1) + } + @Test fun `a missing href is still counted when nothing was unmatched`() { remote.put("one.ics", vtodo("a", "Fine")) remote.onFetch = { hrefs -> diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt index 501c8b8..ba0ae42 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt @@ -190,13 +190,31 @@ class CalendarCollection( * 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. + * a joined string, stops `/a%2Fb` colliding with `/a/b`. The trailing slash + * still distinguishes, exactly as `UrlUtils.equals` refuses to normalise it. + * + * ⚠️ The origin is deliberately *not* part of this key, and is checked + * separately by [answersFor]. The two sides do not share one by + * construction: the REPORT body carries only our paths, so the reply's href + * resolves against the collection's *post-redirect* location, while our + * stored hrefs still carry the origin we had before the redirect. Keying on + * the origin would put every resource of a redirected collection into both + * `unsolicited` and `missing`. */ - private fun identityOf(url: HttpUrl): List = - listOf(url.scheme, url.host, url.port.toString()) + url.pathSegments + private fun identityOf(url: HttpUrl): List = url.pathSegments + + /** + * Whether a reply about [answered] can be the resource we asked for at + * [asked]. + * + * Same host as the href we asked for, or as the collection we asked it of — + * the latter covering a redirect we followed. Anything else is a body from + * somewhere we never spoke to, and applying it would write one host's + * content into another host's row. + */ + private fun answersFor(answered: HttpUrl, asked: HttpUrl, collection: HttpUrl): Boolean = + answered.host.equals(asked.host, ignoreCase = true) || + answered.host.equals(collection.host, ignoreCase = true) /** [batchSize] is a seam for tests; production always uses [MULTIGET_BATCH]. */ internal fun fetch(hrefs: List, batchSize: Int): Result = @@ -207,10 +225,23 @@ class CalendarCollection( val seen = mutableSetOf() hrefs.chunked(batchSize).forEach { batch -> - val wanted = batch.associateBy { identityOf(it) } - dav.multiget(batch, MIME_ICALENDAR, ICALENDAR_VERSION) { response, relation -> + val byExactPath = batch.associateBy { it.encodedPath } + // ⚠️ Only identities that name exactly one request href. Two + // spellings of one path — `/my%40dav.ics` and `/my@dav.ics` — + // decode alike but are separate rows here, and silently keeping + // the last would report the other as missing and eventually + // quarantine it. Where we cannot tell them apart, the exact + // spelling is the only honest match. + val byPath = batch.groupBy { identityOf(it) } + .filterValues { it.size == 1 } + .mapValues { (_, matches) -> matches.single() } + val calendar = dav + calendar.multiget(batch, MIME_ICALENDAR, ICALENDAR_VERSION) { response, relation -> if (relation == Response.HrefRelation.SELF) return@multiget - val asked = wanted[identityOf(response.href)] + val candidate = byExactPath[response.href.encodedPath] + ?: byPath[identityOf(response.href)] + val asked = candidate + ?.takeIf { answersFor(response.href, it, calendar.location) } 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 04f46ef..583afc6 100644 --- a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt @@ -2,6 +2,7 @@ package de.jeanlucmakiola.caldav import com.google.common.truth.Truth.assertThat import okhttp3.HttpUrl +import okhttp3.HttpUrl.Companion.toHttpUrl import okhttp3.OkHttpClient import okhttp3.mockwebserver.MockResponse import okhttp3.mockwebserver.MockWebServer @@ -132,6 +133,34 @@ END:VCALENDAR assertThat(result.resources.single().href).isEqualTo(asked) } + @Test fun `an href stored before a redirect still matches the reply`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/one.ics + + "e1" + BEGIN:VCALENDAR +END:VCALENDAR + HTTP/1.1 200 OK + + """, + ), + ) + + // ⚠️ The REPORT body carries only paths, so the reply resolves against + // the collection we asked — while a stored href can still carry the + // origin we held before a redirect moved us. Keying on the origin would + // put every resource of a redirected collection into both buckets. + val stored = "https://old.example.com/dav/tasks/one.ics".toHttpUrl() + val result = collection.fetch(listOf(stored)).getOrThrow() + + assertThat(result.resources.map { it.href }).containsExactly(stored) + assertThat(result.missing).isEmpty() + assertThat(result.unsolicited).isEmpty() + } + @Test fun `the same path on another host is not our resource`() { server.enqueue( multistatus( @@ -157,6 +186,35 @@ END:VCALENDAR assertThat(result.missing).hasSize(1) } + @Test fun `an ambiguous spelling matches nothing rather than the wrong row`() { + // A third spelling: matches neither request exactly, decodes like both. + server.enqueue( + multistatus( + """ + + /dav/tasks/%6Dy@dav.ics + + "e1" + BEGIN:VCALENDAR +END:VCALENDAR + HTTP/1.1 200 OK + + """, + ), + ) + + val plain = href("my@dav.ics") + val encoded = server.url("/dav/tasks/my%40dav.ics") + val result = collection.fetch(listOf(plain, encoded)).getOrThrow() + + // ⚠️ Two rows decode alike, so no answer can say which it means. Guessing + // applies one row's body to the other; both are reported instead, and the + // syncer will not spend a quarantine count on a stray of the same path. + assertThat(result.resources).isEmpty() + assertThat(result.unsolicited).hasSize(1) + assertThat(result.missing).containsExactly(plain, encoded) + } + @Test fun `a trailing slash is still a different resource`() { server.enqueue( multistatus(