sync: check the reply's origin instead of keying on it
Three corrections to 7fac4bd, all found in review.
Folding the origin into the match key was wrong: the REPORT body carries
only our paths, so a reply's href resolves against the collection's
post-redirect location while our stored hrefs still hold the origin we
had before it. Every resource of a redirected collection would have
landed in both buckets -- the failure the commit was meant to remove.
The key is the decoded path; the origin is checked separately, and a
reply may come from the host we asked or from the collection's own.
Two request hrefs can decode to one identity, and keeping the last of
them silently reported the other as missing and applied one row's body
to the other. Only identities naming exactly one href are matched
loosely now; where we cannot tell two spellings apart, the exact one is
the only honest answer.
And the syncer's amnesty was excused by any stray at all, though servers
volunteer siblings as a matter of course -- so a genuinely omitted
resource would never have been counted and would be re-requested for
ever. Only a stray naming the same path can be this href under a
spelling we failed to read.
This commit is contained in:
@@ -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<String> =
|
||||
listOf(url.scheme, url.host, url.port.toString()) + url.pathSegments
|
||||
private fun identityOf(url: HttpUrl): List<String> = 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<HttpUrl>, batchSize: Int): Result<FetchResult> =
|
||||
@@ -207,10 +225,23 @@ class CalendarCollection(
|
||||
val seen = mutableSetOf<HttpUrl>()
|
||||
|
||||
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
|
||||
|
||||
@@ -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</C:calendar-data>
|
||||
assertThat(result.resources.single().href).isEqualTo(asked)
|
||||
}
|
||||
|
||||
@Test fun `an href stored before a redirect still matches the reply`() {
|
||||
server.enqueue(
|
||||
multistatus(
|
||||
"""
|
||||
<response>
|
||||
<href>/dav/tasks/one.ics</href>
|
||||
<propstat><prop>
|
||||
<getetag>"e1"</getetag>
|
||||
<C:calendar-data xmlns:C="urn:ietf:params:xml:ns:caldav">BEGIN:VCALENDAR
|
||||
END:VCALENDAR</C:calendar-data>
|
||||
</prop><status>HTTP/1.1 200 OK</status></propstat>
|
||||
</response>
|
||||
""",
|
||||
),
|
||||
)
|
||||
|
||||
// ⚠️ 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</C:calendar-data>
|
||||
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(
|
||||
"""
|
||||
<response>
|
||||
<href>/dav/tasks/%6Dy@dav.ics</href>
|
||||
<propstat><prop>
|
||||
<getetag>"e1"</getetag>
|
||||
<C:calendar-data xmlns:C="urn:ietf:params:xml:ns:caldav">BEGIN:VCALENDAR
|
||||
END:VCALENDAR</C:calendar-data>
|
||||
</prop><status>HTTP/1.1 200 OK</status></propstat>
|
||||
</response>
|
||||
""",
|
||||
),
|
||||
)
|
||||
|
||||
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(
|
||||
|
||||
Reference in New Issue
Block a user