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 new file mode 100644 index 0000000..eff3921 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncer.kt @@ -0,0 +1,630 @@ +package de.jeanlucmakiola.agendula.data.sync + +import de.jeanlucmakiola.agendula.data.tasks.ical.CalendarResource +import de.jeanlucmakiola.agendula.data.tasks.ical.ResourceValidator +import de.jeanlucmakiola.agendula.data.tasks.ical.VTodoMapper +import de.jeanlucmakiola.agendula.data.tasks.room.TaskEntity +import de.jeanlucmakiola.agendula.data.tasks.room.TaskListEntity +import de.jeanlucmakiola.caldav.DeleteOutcome +import de.jeanlucmakiola.caldav.PutOutcome +import de.jeanlucmakiola.caldav.RemoteCalendar +import de.jeanlucmakiola.caldav.ResourceNames +import okhttp3.HttpUrl +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull +import kotlin.time.Clock +import kotlin.time.Instant + +/** + * Reconciles one task list against one remote collection. + * + * The order is deliberate and is the whole design: + * + * 1. **Deletions**, so a tombstone never races the download of the resource it + * is about to remove. + * 2. **Uploads**, so local work reaches the server before anything can overwrite + * it, and so a conflict is discovered while the local edit still exists. + * 3. **Downloads**, including everything the write phase decided we now need a + * fresh copy of. + * 4. **Sweep**, which is the only step that may delete a row it did not see fail + * — and so it runs last, on a listing taken before any of the writes. + * + * ⚠️ **A failed resource must not fail the collection**, the twin of "a failed + * collection must not fail the account". Every per-resource outcome below either + * counts against [QuarantineStore.THRESHOLD] or is recorded in the report; none + * of them abandon the run. + */ +class CollectionSyncer( + private val store: SyncStore, + private val now: () -> Instant = { Clock.System.now() }, +) { + + /** + * @param quarantine failure counts keyed by [QuarantineStore.key], mutated in + * place so the caller can persist one map for the whole account. + */ + fun sync( + list: TaskListEntity, + remote: RemoteCalendar, + quarantine: MutableMap, + ): SyncReport { + val run = Run(list, remote, quarantine) + return try { + run.execute() + } catch (e: Exception) { + // The collection is lost, the account is not. + run.report.copy(failure = e.toString()) + } + } + + /** One collection's pass. Mutable state lives here rather than in the class. */ + private inner class Run( + val list: TaskListEntity, + val remote: RemoteCalendar, + val quarantine: MutableMap, + ) { + var report = SyncReport(listId = list.id, listName = list.name) + + /** Hrefs the write phase wants a fresh copy of. */ + val refetch = mutableSetOf() + + /** + * Edits that will be discarded **once the replacement actually arrives**. + * + * ⚠️ Reported from [apply], not from the write phase. Announcing the + * discard at the moment of the 412 would claim a loss that has not + * happened yet — and clearing `is_dirty` there would make it happen for + * real if the download then failed, with nothing left to retry. + */ + val pendingDiscard = mutableMapOf() + + /** Hrefs this run wrote, and which the sweep must therefore not remove. */ + val touched = mutableSetOf() + + /** + * `uid -> parent uid`, resolved to row ids once every row exists. + * + * The value is nullable and every downloaded resource records one: a + * *removed* `RELATED-TO` must clear the link, and a map that only holds + * present parents leaves the stale `parent_id` in place — which + * [parentUidOf] then resolves back into a `RELATED-TO` and re-uploads. + */ + val parents = mutableMapOf() + + var writable = true + var shared = false + + fun execute(): SyncReport { + val state = remote.state().getOrElse { + return report.copy(failure = "collection unavailable: $it") + } + writable = !state.collection.readOnly + shared = state.collection.isShared + persistCollectionState(state.collection.readOnly) + + val refs = remote.list().getOrElse { + return report.copy(failure = "listing failed: $it") + } + // ⚠️ Only strong tags are carried forward. A weak one cannot serve + // as `If-Match`, so recording it would make every later write look + // conditional while silently not being one. + val remoteETags: Map = refs.associate { ref -> + ref.href.toString() to ref.eTag?.takeIf { it.usable }?.value + } + + val locals = localResources() + + deletePhase(locals) + uploadPhase(locals) + downloadPhase(locals, remoteETags) + resolveParents() + sweepPhase(locals, remoteETags.keys) + + return report + } + + // ------------------------------------------------------------ phase 1 + + private fun deletePhase(locals: List) { + locals.filter { it.isDeleted }.forEach { local -> + val href = local.href + if (href == null) { + // ⚠️ Never uploaded, so there is nothing to DELETE. Sending + // one would 404 on every sync forever. + purge(local) + return@forEach + } + if (isQuarantined(local.key)) return@forEach + + when (val outcome = remote.delete(url(href), local.eTag)) { + DeleteOutcome.Deleted -> { + purge(local) + touched += href + report = report.copy(deletedRemotely = report.deletedRemotely + 1) + } + + DeleteOutcome.ServerNewer -> { + // The delete lost. Undo the tombstone and take the + // server's copy — decision 2, applied to a deletion. + clearTombstone(local) + refetch += href + touched += href + discard(local, DiscardedEdit.Cause.DELETE_LOST) + } + + is DeleteOutcome.Rejected -> + fail(href, "DELETE refused: ${outcome.code} ${outcome.message}") + + is DeleteOutcome.Failed -> + fail(href, "DELETE failed: ${outcome.reason}") + } + } + } + + // ------------------------------------------------------------ phase 2 + + private fun uploadPhase(locals: List) { + locals.filter { it.isDirty && !it.isDeleted }.forEach { local -> + val href = local.href + // ⚠️ Keyed by UID when there is no href yet. A create that the + // server permanently rejects has nothing else to key on, and a + // counter nothing ever reads is a resource re-PUT on every sync + // forever — the exact failure quarantine exists to prevent. + if (isQuarantined(local.key)) return@forEach + + if (!writable) { + skip(local.key, "the collection is read-only") + return@forEach + } + + // ⚠️ The Nextcloud shared-calendar landmine. `CalendarObject::get()` + // reduces a CLASS:CONFIDENTIAL object from a share to a + // VEVENT-shaped whitelist — deleting DUE, STATUS, COMPLETED, + // PERCENT-COMPLETE, PRIORITY and RELATED-TO — while leaving the + // ETag untouched. Writing that back destroys the owner's task, + // and the ETag matches, so nothing stops it but this. + if (shared && href != null && local.master.classification == CLASS_CONFIDENTIAL) { + skip(href, "confidential task in a shared collection: the server may have served a reduced copy") + return@forEach + } + + val body = serialize(local) ?: return@forEach + if (href == null) createResource(local, body) else updateResource(local, href, body) + } + } + + /** Null when the resource was rejected before it left the device. */ + private fun serialize(local: LocalResource): String? { + val todos = local.live.map { row -> + VTodoMapper.write(row, parentUidOf(row), now()) + } + val calendar = CalendarResource.build(todos) + + ResourceValidator.validate(calendar)?.let { rejection -> + // Retrying cannot help: the same bytes produce the same 415. + fail(local.key, "not uploadable — it ${rejection.reason}") + return null + } + return CalendarResource.serialize(todos) + } + + private fun createResource(local: LocalResource, body: String) { + var name = ResourceNames.forUid(local.uid) + repeat(CREATE_ATTEMPTS) { + when (val outcome = remote.create(name, body)) { + is PutOutcome.Stored -> { + stored(local, outcome.href, outcome.eTag.value) + return + } + + is PutOutcome.StoredNeedsRefetch -> { + stored(local, outcome.href, eTag = null) + refetch += outcome.href.toString() + return + } + + is PutOutcome.NameTaken -> { + // Either a previous run's PUT whose answer we never saw, + // or an unrelated resource squatting the name. Only the + // UID in the body can tell them apart. + val existing = remote.fetch(listOf(outcome.href)).getOrNull() + ?.resources?.firstOrNull() + val sameTask = existing != null && uidOf(existing.iCalendar) == local.uid + if (sameTask) { + updateResource(local, outcome.href.toString(), body) + return + } + name = ResourceNames.random() + } + + is PutOutcome.Rejected -> { + fail(local.key, "create refused: ${outcome.code} ${outcome.message}") + return + } + + is PutOutcome.Failed -> { + fail(local.key, "create failed: ${outcome.reason}") + return + } + + // Unreachable on create; If-None-Match cannot produce them. + PutOutcome.ServerNewer, PutOutcome.Vanished -> return + } + } + fail(local.key, "could not find a free name after $CREATE_ATTEMPTS attempts") + } + + private fun updateResource(local: LocalResource, href: String, body: String) { + val url = url(href) + var eTag = local.eTag + 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() + ?.eTag?.takeIf { it.usable }?.value + if (eTag == null) { + report = report.copy(unconditionalWrites = report.unconditionalWrites + 1) + } + } + + when (val outcome = remote.update(url, eTag, body)) { + is PutOutcome.Stored -> stored(local, outcome.href, outcome.eTag.value) + + is PutOutcome.StoredNeedsRefetch -> { + stored(local, outcome.href, eTag = null) + refetch += outcome.href.toString() + } + + PutOutcome.ServerNewer -> { + // Decision 2: the server wins and the local edit is thrown + // away — but only when its replacement is in hand. The row + // stays dirty until [apply] overwrites it, so a failed + // download costs a retry rather than the edit. + refetch += href + touched += href + pendingDiscard[href] = DiscardedEdit.Cause.SERVER_NEWER + } + + PutOutcome.Vanished -> { + purge(local) + touched += href + discard(local, DiscardedEdit.Cause.DELETED_ON_SERVER) + report = report.copy(deletedLocally = report.deletedLocally + 1) + } + + is PutOutcome.Rejected -> + fail(href, "upload refused: ${outcome.code} ${outcome.message}") + + is PutOutcome.Failed -> + fail(href, "upload failed: ${outcome.reason}") + + is PutOutcome.NameTaken -> + fail(href, "unexpected 412 on a conditional update") + } + } + + private fun stored(local: LocalResource, href: HttpUrl, eTag: String?) { + val key = href.toString() + store.markSynced(local.rows.map { it.id }, key, eTag) + // Both, because a resource that failed as a create was counted under + // its UID and is now counted under its href. Clearing one would leak + // the other into the store forever. + succeeded(key) + succeeded(local.key) + touched += key + report = report.copy(uploaded = report.uploaded + 1) + } + + // ------------------------------------------------------------ phase 3 + + private fun downloadPhase( + locals: List, + remoteETags: Map, + ) { + val byHref = locals.filter { it.href != null }.associateBy { it.href!! } + val wanted = linkedSetOf() + + remoteETags.forEach { (href, eTag) -> + val local = byHref[href] + when { + // Never seen it. + local == null -> wanted += href + // The write phase owns it this run. + local.isDirty || local.isDeleted -> Unit + // No validator to compare against, so we cannot know. + local.eTag == null || eTag == null -> wanted += href + eTag != local.eTag -> wanted += href + } + } + wanted += refetch + wanted.removeAll { isQuarantined(it) } + + // Built once. Re-reading the whole list per resource turns a first + // sync of a large collection into a quadratic scan on the sync thread. + val byUid = store.rowsIn(list.id).groupBy { it.uid }.toMutableMap() + + wanted.chunked(DOWNLOAD_BATCH).forEach { batch -> + val fetched = remote.fetch(batch.map(::url)).getOrElse { error -> + report = report.copy(failure = "download failed: $error") + return + } + fetched.resources.forEach { apply(it, byUid) } + } + } + + private fun apply( + resource: de.jeanlucmakiola.caldav.RemoteResource, + byUid: MutableMap>, + ) { + val href = resource.href.toString() + val todos = runCatching { + CalendarResource.todosIn(CalendarResource.parse(resource.iCalendar)) + }.getOrElse { + fail(href, "unreadable iCalendar: $it") + return + } + if (todos.isEmpty()) { + fail(href, "contains no VTODO") + return + } + + val mapped = todos.map { VTodoMapper.read(it, list.id) } + if (mapped.any { it.uidWasMissing }) { + fail(href, "a component has no UID") + return + } + // RFC 4791 §4.1: one resource, one UID. More than one is unmappable + // to rows without inventing an identity the server does not share. + val uid = mapped.map { it.entity.uid }.distinct().singleOrNull() ?: run { + fail(href, "holds more than one UID") + return + } + + // ⚠️ Only the ETag that arrived with *this* body, and only if strong. + // A tag from the listing paired with a body from here is not a + // matched pair, and a weak one cannot be used as `If-Match` at all. + val eTag = resource.eTag?.takeIf { it.usable }?.value + + val existing = byUid[uid].orEmpty().associateBy { it.recurrenceId } + + // Captured before the overwrite, because that is what the report is + // about: the version the user is losing. + val losing = existing.values.firstOrNull { it.isDirty } + + val master = mapped.firstOrNull { it.entity.recurrenceId == null } ?: mapped.first() + val masterRow = upsert(master.entity, existing[master.entity.recurrenceId], null, href, eTag) + val masterId = masterRow.id + parents[uid] = master.parentUid + + val written = mutableListOf(masterRow) + mapped.filter { it !== master }.forEach { override -> + written += upsert( + override.entity, existing[override.entity.recurrenceId], masterId, href, eTag, + ) + } + val kept = written.map { it.recurrenceId }.toSet() + + // Overrides the server no longer has. Cascade would take them with + // the master, but the master is still here. + val stale = existing.filterKeys { it !in kept }.values.map { it.id } + store.deleteAll(stale) + + // Keep the hoisted index honest for the resources still to come. + byUid[uid] = written + + pendingDiscard.remove(href)?.let { cause -> + report = report.copy( + discardedEdits = report.discardedEdits + + DiscardedEdit(uid, losing?.title, cause), + ) + } + + succeeded(href) + touched += href + report = report.copy(downloaded = report.downloaded + 1) + } + + /** @return the row as it now stands, so the caller can index it. */ + private fun upsert( + incoming: TaskEntity, + existing: TaskEntity?, + masterId: Long?, + href: String, + eTag: String?, + ): TaskEntity { + // Local-only columns the server has no opinion about. Taking the + // mapper's defaults here would silently reset the user's ordering and + // per-task colour on every download. + val row = incoming.copy( + id = existing?.id ?: 0L, + listId = list.id, + masterId = masterId, + parentId = existing?.parentId, + sortOrder = existing?.sortOrder ?: 0, + color = existing?.color, + href = href, + etag = eTag, + // Explicit, not defaulted: a downstream write that leaves this + // set uploads what was just downloaded. + isDirty = false, + isDeleted = false, + ) + return if (existing == null) { + row.copy(id = store.insert(row)) + } else { + store.update(row) + row + } + } + + /** + * Turns the `RELATED-TO` UIDs collected during the download into row ids. + * + * Deferred to the end because a parent may arrive in a later batch than + * its child, and a forward reference resolved eagerly is a lost + * hierarchy. + */ + private fun resolveParents() { + parents.forEach { (uid, parentUid) -> + val child = store.masterByUid(list.id, uid) ?: return@forEach + val parent = parentUid?.let { store.masterByUid(list.id, it) } + if (child.parentId != parent?.id) { + store.setParent(child.id, parent?.id) + } + } + } + + // ------------------------------------------------------------ phase 4 + + private fun sweepPhase(locals: List, remoteHrefs: Set) { + // ⚠️ An empty listing never sweeps. The sweep is the one phase that + // deletes rows it did not see fail, and its evidence is a + // `calendar-query` with a VTODO comp-filter — a filter some servers + // mishandle badly enough to answer with an empty *successful* + // multistatus, which is indistinguishable from an empty collection. + // Without this floor, one such answer hard-deletes every task the + // user has in that list, in a single pass, unrecoverably. + // + // Cost accepted: a collection genuinely emptied on the server keeps + // its local rows until one task reappears there. That is recoverable + // by hand. The other error is not. + if (remoteHrefs.isEmpty() && locals.any { it.href != null }) { + report = report.copy( + failure = "the server listed no tasks while ${locals.count { it.href != null }} " + + "are known here — nothing was deleted", + ) + return + } + + locals.forEach { local -> + val href = local.href ?: return@forEach + if (href in remoteHrefs || href in touched || isQuarantined(local.key)) return@forEach + if (local.isDeleted) return@forEach + + // Present locally, absent from a full listing: deleted on the + // server. A dirty row here is a local edit that lost to that + // deletion, which the user is told about rather than left to + // discover. + if (local.isDirty) discard(local, DiscardedEdit.Cause.DELETED_ON_SERVER) + purge(local) + report = report.copy(deletedLocally = report.deletedLocally + 1) + } + } + + // ------------------------------------------------------------- shared + + private fun localResources(): List = + store.rowsIn(list.id) + .groupBy { it.uid } + .map { (uid, rows) -> LocalResource(uid, rows) } + + private fun persistCollectionState(readOnly: Boolean) { + if (list.isReadOnly == readOnly) return + // ⚠️ ACL churn is silent: a share can be demoted to read-only with no + // notification, and a stale flag turns every upload into a 403 the + // user cannot act on. + // + // One column, not the whole row. `list` was captured before the sync + // started, so writing it back would silently revert a rename, a + // recolour or a visibility toggle the user made while it ran. + store.setListReadOnly(list.id, readOnly) + } + + private fun parentUidOf(row: TaskEntity): String? = + row.parentId?.let { store.row(it)?.uid } + + private fun uidOf(iCalendar: String): String? = runCatching { + CalendarResource.todosIn(CalendarResource.parse(iCalendar)) + .firstNotNullOfOrNull { it.property("UID")?.value?.trim() } + }.getOrNull() + + private fun url(href: String): HttpUrl = + href.toHttpUrlOrNull() ?: remote.url.resolve(href) ?: remote.url + + private fun purge(local: LocalResource) { + store.deleteAll(local.rows.map { it.id }) + // The rows are gone, so a counter about them is dead weight. + succeeded(local.key) + } + + private fun clearTombstone(local: LocalResource) { + local.rows.forEach { store.update(it.copy(isDeleted = false, isDirty = false)) } + } + + private fun discard(local: LocalResource, cause: DiscardedEdit.Cause) { + report = report.copy( + discardedEdits = report.discardedEdits + + DiscardedEdit(local.uid, local.master.title, cause), + ) + } + + /** A resource we cannot sync this run, but which is nobody's fault. */ + private fun skip(href: String, reason: String) { + report = report.copy( + quarantined = report.quarantined + QuarantinedResource(href, reason, failures = 0), + ) + } + + /** A resource that failed. Counts towards [QuarantineStore.THRESHOLD]. */ + private fun fail(href: String, reason: String) { + val key = QuarantineStore.key(list.id, href) + val failures = (quarantine[key] ?: 0) + 1 + quarantine[key] = failures + report = report.copy( + quarantined = report.quarantined + QuarantinedResource(href, reason, failures), + ) + } + + private fun succeeded(href: String) { + quarantine.remove(QuarantineStore.key(list.id, href)) + } + + private fun isQuarantined(href: String): Boolean = + (quarantine[QuarantineStore.key(list.id, href)] ?: 0) >= QuarantineStore.THRESHOLD + } + + /** + * The rows that make up one calendar resource. + * + * A recurring task and its `RECURRENCE-ID` overrides are separate rows and + * one file: RFC 4791 §4.1 requires everything in a resource to share a UID. + * So href and ETag belong to the group, never to a row. + */ + private data class LocalResource(val uid: String, val rows: List) { + val master: TaskEntity = rows.firstOrNull { it.recurrenceId == null } ?: rows.first() + val href: String? = rows.firstNotNullOfOrNull { it.href } + val eTag: String? = rows.firstNotNullOfOrNull { it.etag } + val isDirty: Boolean = rows.any { it.isDirty } + + /** + * What quarantine counts this resource under. + * + * The href once it has one, and the UID before that — a resource that has + * never been uploaded still has to be countable, or a body the server + * refuses forever is retried forever. + */ + val key: String = href ?: "uid:$uid" + + /** Only when the whole resource is gone; a deleted override is an edit. */ + val isDeleted: Boolean = rows.all { it.isDeleted } + + /** What gets serialised: a deleted override is simply absent. */ + val live: List = rows.filterNot { it.isDeleted } + } + + private companion object { + /** RFC 5545 `CLASS:CONFIDENTIAL`, as `tasks.classification` stores it. */ + const val CLASS_CONFIDENTIAL = 2 + + /** + * Fresh names tried before giving up on a create. + * + * Bounded because every 412 retry loop in this engine is bounded — an + * unbounded one against a server that 412s unconditionally is a sync that + * never finishes. + */ + const val CREATE_ATTEMPTS = 3 + + const val DOWNLOAD_BATCH = 30 + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/QuarantineStore.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/QuarantineStore.kt new file mode 100644 index 0000000..3c0f77f --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/QuarantineStore.kt @@ -0,0 +1,79 @@ +package de.jeanlucmakiola.agendula.data.sync + +import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.edit +import androidx.datastore.preferences.core.stringSetPreferencesKey +import kotlinx.coroutines.flow.first +import javax.inject.Inject +import javax.inject.Singleton + +/** + * How many times each resource has failed, and therefore which ones to skip. + * + * ⚠️ Deliberately **not** a backoff. A backoff assumes the failure is transient + * and asks "how long until I try again"; the failures that matter here are + * permanent — a body sabre answers 415 for, a contradictory `RRULE`/`EXDATE` + * pair Nextcloud answers 500 for forever, a 507 the spec forbids retrying at + * all. The question worth asking is "how many times before I leave this one + * alone and finish the collection", and the answer is [THRESHOLD]. + * + * Counts are cleared the moment a resource succeeds, so a genuinely transient + * failure costs nothing beyond the runs it actually failed in. + */ +@Singleton +class QuarantineStore @Inject constructor( + private val dataStore: DataStore, +) { + + /** Current failure counts, keyed by [key]. */ + suspend fun counts(): Map = decode(dataStore.data.first()[KEY].orEmpty()) + + private fun decode(entries: Set): Map = entries.mapNotNull { entry -> + val separator = entry.lastIndexOf(COUNT_SEPARATOR) + if (separator <= 0) return@mapNotNull null + val count = entry.substring(separator + 1).toIntOrNull() ?: return@mapNotNull null + entry.substring(0, separator) to count + }.toMap() + + /** + * Applies one account's changes without disturbing anyone else's. + * + * ⚠️ Not a whole-map replace. The counts are global — keyed by list, not by + * account — while `SyncWorker`'s uniqueness is only *per account*, so two + * accounts can sync at once. Each would snapshot the same global map and the + * later writer would discard the other's increments and resurrect the + * counters it had cleared. Re-reading inside `edit`, which DataStore + * serialises, keeps the read-modify-write atomic. + * + * @param updates counts to set, replacing any current value for those keys. + * @param cleared keys to remove outright, whatever they currently hold. + */ + suspend fun merge(updates: Map, cleared: Set) { + dataStore.edit { prefs -> + val current = decode(prefs[KEY].orEmpty()).toMutableMap() + current -= cleared + current += updates.filterValues { it > 0 } + prefs[KEY] = current + .map { (key, count) -> "$key$COUNT_SEPARATOR$count" } + .toSet() + } + } + + companion object { + /** + * Attempts before a resource is left alone. + * + * Three rather than one: a 502 from a reverse proxy mid-restart and a + * permanently malformed body arrive as the same outcome, and burning two + * extra runs is cheaper than quarantining a resource that would have + * worked. + */ + const val THRESHOLD = 3 + + fun key(listId: Long, href: String) = "$listId|$href" + + private const val COUNT_SEPARATOR = '#' + private val KEY = stringSetPreferencesKey("sync_quarantine") + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncAdapterService.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncAdapterService.kt index 81cfd78..868b4b3 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncAdapterService.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncAdapterService.kt @@ -9,9 +9,6 @@ import android.content.Intent import android.content.SyncResult import android.os.Bundle import android.os.IBinder -import androidx.work.Data -import androidx.work.ExistingWorkPolicy -import androidx.work.OneTimeWorkRequestBuilder import androidx.work.WorkInfo import androidx.work.WorkManager import kotlinx.coroutines.flow.first @@ -58,18 +55,7 @@ private class CalDavSyncAdapter(context: Context) : syncResult: SyncResult, ) { val workManager = WorkManager.getInstance(context) - val uniqueName = SyncWorker.uniqueNameFor(account.name) - val request = OneTimeWorkRequestBuilder() - .setInputData(Data.Builder().putString(SyncWorker.KEY_ACCOUNT_NAME, account.name).build()) - .build() - - workManager.enqueueUniqueWork( - uniqueName, - // KEEP, not REPLACE: a periodic trigger arriving while a manual sync - // is mid-flight must not cancel it and lose the cursor. - ExistingWorkPolicy.KEEP, - request, - ) + val uniqueName = SyncTrigger(context).enqueue(account.name) // Block this thread until the work reaches a terminal state. The framework // treats onPerformSync returning as "the sync is done", so returning early diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncEngine.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncEngine.kt new file mode 100644 index 0000000..45e0260 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncEngine.kt @@ -0,0 +1,126 @@ +package de.jeanlucmakiola.agendula.data.sync + +import de.jeanlucmakiola.agendula.data.di.IoDispatcher +import de.jeanlucmakiola.agendula.data.tasks.room.AccountEntity +import de.jeanlucmakiola.agendula.data.tasks.room.TasksDatabase +import de.jeanlucmakiola.caldav.CalDavHttp +import de.jeanlucmakiola.caldav.CalendarCollection +import de.jeanlucmakiola.caldav.RemoteCalendar +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.withContext +import okhttp3.HttpUrl +import okhttp3.HttpUrl.Companion.toHttpUrlOrNull +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Syncs one account: every list it owns, against the collection each points at. + * + * ⚠️ **A failed collection must not fail the account.** One revoked share, one + * calendar the server 500s on, must not stop the other four from syncing — so + * every collection's outcome is a [SyncReport] rather than an exception, and the + * account's own result is the list of them. + */ +@Singleton +class SyncEngine @Inject constructor( + private val database: TasksDatabase, + private val store: RoomSyncStore, + private val credentials: CredentialStore, + private val quarantine: QuarantineStore, + @IoDispatcher private val io: CoroutineDispatcher, +) { + + /** Why an account could not be synced at all, as opposed to one of its lists. */ + sealed interface Result { + data class Synced(val reports: List) : Result + + /** The credential is gone or undecryptable: only re-authentication helps. */ + data class NeedsSignIn(val reason: String) : Result + + data class Misconfigured(val reason: String) : Result + } + + suspend fun sync(accountName: String): Result = withContext(io) { + val account = database.accounts().all().firstOrNull { it.displayName == accountName } + ?: return@withContext Result.Misconfigured("no such account: $accountName") + + val username = account.username + ?: return@withContext Result.Misconfigured("account has no username") + val origin = account.principalUrl?.toHttpUrlOrNull() + ?: return@withContext Result.Misconfigured("account has no principal URL") + + val password = when (val secret = credentials.get(account.id)) { + is CredentialStore.Secret.Present -> secret.value + CredentialStore.Secret.Absent -> + return@withContext Result.NeedsSignIn("no stored password") + is CredentialStore.Secret.Unrecoverable -> + return@withContext Result.NeedsSignIn(secret.reason) + } + + val client = CalDavHttp.authenticated(USER_AGENT, username, password, origin) + val reports = syncCollections(account) { url -> CalendarCollection(client, url) } + Result.Synced(reports) + } + + /** + * Records why the account could not be synced at all. + * + * ⚠️ Without this the row keeps its old `lastSyncAt`, and the accounts screen + * goes on reporting "synced 5 minutes ago" for an account whose credential + * can no longer be decrypted — the silent failure the account layer exists to + * avoid. + */ + private fun fatal(accountId: Long, result: Result): Result { + val reason = when (result) { + is Result.NeedsSignIn -> result.reason + is Result.Misconfigured -> result.reason + is Result.Synced -> return result + } + database.accounts().recordSync(accountId, at = null, error = reason) + return result + } + + /** Split out from [sync] so the reconciliation can be driven without a network. */ + internal suspend fun syncCollections( + account: AccountEntity, + remoteFor: (HttpUrl) -> RemoteCalendar, + ): List { + val lists = database.taskLists().syncedForAccount(account.id) + val listIds = lists.map { it.id }.toSet() + + // ⚠️ Only this account's keys are written back. The counts are global + // while the worker's uniqueness is only per account, so replacing the + // whole map would discard a concurrently syncing account's increments and + // resurrect the counters it had cleared. + val before = quarantine.counts() + val counts = before.toMutableMap() + val syncer = CollectionSyncer(store) + + val reports = lists.map { list -> + val url = list.href?.toHttpUrlOrNull() + ?: return@map SyncReport(list.id, list.name, failure = "list has no collection URL") + syncer.sync(list, remoteFor(url), counts) + } + + fun mine(key: String) = key.substringBefore('|').toLongOrNull() in listIds + quarantine.merge( + updates = counts.filterKeys(::mine), + cleared = before.keys.filter(::mine).filterNot { it in counts }.toSet(), + ) + database.accounts().recordSync( + accountId = account.id, + at = kotlin.time.Clock.System.now(), + error = reports.mapNotNull { it.failure }.firstOrNull(), + ) + return reports + } + + private companion object { + /** + * Matches what the account-add flow signed in with, so Nextcloud's + * Settings → Security → Devices & sessions keeps naming the app password + * after the app rather than after OkHttp. + */ + const val USER_AGENT = "Agendula (Android)" + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncReport.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncReport.kt new file mode 100644 index 0000000..db74d3c --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncReport.kt @@ -0,0 +1,66 @@ +package de.jeanlucmakiola.agendula.data.sync + +/** + * What a sync did, and — the part that matters — what it destroyed. + * + * ⚠️ `docs/SYNC-PLAN.md` decision 2 is **server wins, local edit discarded**. + * That policy terminates, which is why it was chosen over forking under a new + * UID, but on its own it is indistinguishable from data loss: the user's edit is + * gone and nothing said so. The report is the other half of the decision, not a + * nice-to-have — [discardedEdits] is why this type exists. + */ +data class SyncReport( + val listId: Long, + val listName: String, + val downloaded: Int = 0, + val uploaded: Int = 0, + val deletedRemotely: Int = 0, + val deletedLocally: Int = 0, + /** Local edits thrown away because the server's copy was newer. */ + val discardedEdits: List = emptyList(), + /** Resources the collection gave up on, so the rest of it could finish. */ + val quarantined: List = emptyList(), + /** + * Writes sent without `If-Match` because the server offers no usable + * validator. Not an error, but the one case where a concurrent edit can be + * overwritten without us noticing, so it is said out loud. + */ + val unconditionalWrites: Int = 0, + /** Set when the collection failed as a whole. The account keeps going. */ + val failure: String? = null, +) { + val hadWork: Boolean + get() = downloaded > 0 || uploaded > 0 || deletedRemotely > 0 || deletedLocally > 0 +} + +/** One local edit that lost to the server. */ +data class DiscardedEdit( + val uid: String, + val title: String?, + val cause: Cause, +) { + enum class Cause { + /** The server's copy changed after we last read it. */ + SERVER_NEWER, + + /** The task was deleted on the server while it was edited here. */ + DELETED_ON_SERVER, + + /** Deleted here, but changed on the server after that. The delete lost. */ + DELETE_LOST, + } +} + +/** + * A resource the collection stopped trying. + * + * ⚠️ Quarantine is a **counter, not a backoff**. A single HTTP 400 on one + * resource has halted all of a user's calendar sync in DAVx5 for weeks; the + * failure has to be contained to the resource that caused it, and the rest of + * the collection has to complete. + */ +data class QuarantinedResource( + val href: String, + val reason: String, + val failures: Int, +) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncStore.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncStore.kt new file mode 100644 index 0000000..bc5dc5e --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncStore.kt @@ -0,0 +1,76 @@ +package de.jeanlucmakiola.agendula.data.sync + +import de.jeanlucmakiola.agendula.data.tasks.room.TaskEntity +import de.jeanlucmakiola.agendula.data.tasks.room.TasksDatabase +import javax.inject.Inject + +/** + * The database, as [CollectionSyncer] needs it. + * + * A seam, and the reason is the same one that put [CalDavGateway] in front of + * discovery: the reconciliation above this interface is where local edits are + * discarded, tombstones swept and conflicts resolved, and every one of those is + * a decision that should be provable without a device. Room's test double is + * Robolectric plus an in-memory database; this is eight methods. + */ +interface SyncStore { + + /** Every row in a list, **tombstones included**. */ + fun rowsIn(listId: Long): List + + fun insert(row: TaskEntity): Long + + fun update(row: TaskEntity) + + fun deleteAll(taskIds: List) + + /** Records href and ETag on a whole resource, clearing `is_dirty`. */ + fun markSynced(taskIds: List, href: String?, eTag: String?) + + fun setParent(taskId: Long, parentId: Long?) + + /** The master row for a UID — the one with no `RECURRENCE-ID`. */ + fun masterByUid(listId: Long, uid: String): TaskEntity? + + fun row(taskId: Long): TaskEntity? + + /** + * One column, deliberately. A whole-entity update would carry the row as it + * looked when the sync started and revert anything the user changed while it + * ran. + */ + fun setListReadOnly(listId: Long, readOnly: Boolean) +} + +class RoomSyncStore @Inject constructor( + private val database: TasksDatabase, +) : SyncStore { + + override fun rowsIn(listId: Long) = database.tasks().allIn(listId) + + override fun insert(row: TaskEntity) = database.tasks().insert(row) + + override fun update(row: TaskEntity) { + database.tasks().update(row) + } + + override fun deleteAll(taskIds: List) { + if (taskIds.isNotEmpty()) database.tasks().deleteAll(taskIds) + } + + override fun markSynced(taskIds: List, href: String?, eTag: String?) { + if (taskIds.isNotEmpty()) database.tasks().markSynced(taskIds, href, eTag) + } + + override fun setParent(taskId: Long, parentId: Long?) { + database.tasks().setParent(taskId, parentId) + } + + override fun masterByUid(listId: Long, uid: String) = database.tasks().byUid(listId, uid) + + override fun row(taskId: Long) = database.tasks().entity(taskId) + + override fun setListReadOnly(listId: Long, readOnly: Boolean) { + database.taskLists().setReadOnly(listId, readOnly) + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncTrigger.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncTrigger.kt new file mode 100644 index 0000000..096ad40 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncTrigger.kt @@ -0,0 +1,44 @@ +package de.jeanlucmakiola.agendula.data.sync + +import android.content.Context +import androidx.work.Data +import androidx.work.ExistingWorkPolicy +import androidx.work.OneTimeWorkRequestBuilder +import androidx.work.WorkManager +import dagger.hilt.android.qualifiers.ApplicationContext +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Starts a sync for one account. + * + * Shared by the sync adapter and by the app's own "sync now", because the app + * cannot rely on the system trigger: ⚠️ `ContentService.hasAuthorityAccess()` + * gates `requestSync` behind a compat change that is on at targetSdk ≥ 34, and + * our authority is `userVisible="false"`, so Settings greys "Sync now" out. The + * in-app button enqueues the work directly and is unaffected. + */ +@Singleton +class SyncTrigger @Inject constructor( + @ApplicationContext private val context: Context, +) { + + /** @return the unique work name, which the caller may wait on. */ + fun enqueue(accountName: String): String { + val uniqueName = SyncWorker.uniqueNameFor(accountName) + val request = OneTimeWorkRequestBuilder() + .setInputData( + Data.Builder().putString(SyncWorker.KEY_ACCOUNT_NAME, accountName).build(), + ) + .build() + + WorkManager.getInstance(context).enqueueUniqueWork( + uniqueName, + // KEEP, not REPLACE: a periodic trigger arriving while a manual sync + // is mid-flight must not cancel it and lose the cursor. + ExistingWorkPolicy.KEEP, + request, + ) + return uniqueName + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncWorker.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncWorker.kt index 6a08951..409d4b1 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncWorker.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/sync/SyncWorker.kt @@ -1,6 +1,7 @@ package de.jeanlucmakiola.agendula.data.sync import android.content.Context +import android.util.Log import androidx.hilt.work.HiltWorker import androidx.work.CoroutineWorker import androidx.work.WorkerParameters @@ -10,11 +11,8 @@ import dagger.assisted.AssistedInject /** * Where sync actually happens. * - * A stub until chunk 3 — the account plumbing has to exist and be provably wired - * before there is anywhere for an engine to run. - * - * Two things about this worker are decided already and are not chunk 3's to - * revisit. It is a **`CoroutineWorker` with no foreground service**: an ordinary + * Two things about this worker are decided already. It is a + * **`CoroutineWorker` with no foreground service**: an ordinary * worker is documented for under 10 minutes, and escalating to `setForeground` * pulls in `FOREGROUND_SERVICE_DATA_SYNC`, the Android 15 six-hours-per-24 * `dataSync` budget whose failure mode is a fatal `RemoteServiceException`, and @@ -27,14 +25,39 @@ import dagger.assisted.AssistedInject class SyncWorker @AssistedInject constructor( @Assisted context: Context, @Assisted parameters: WorkerParameters, + private val engine: SyncEngine, ) : CoroutineWorker(context, parameters) { - override suspend fun doWork(): Result = Result.success() + override suspend fun doWork(): Result { + val accountName = inputData.getString(KEY_ACCOUNT_NAME) ?: return Result.failure() + + return when (val outcome = engine.sync(accountName)) { + is SyncEngine.Result.Synced -> { + // ⚠️ Success even when collections failed. A retry re-runs the + // whole account, and WorkManager's backoff would then punish the + // four healthy collections for the one that 500s — while the + // failing one is already contained by its own quarantine counter. + outcome.reports.forEach { report -> + if (report.failure != null || report.hadWork) Log.i(TAG, report.toString()) + } + Result.success() + } + + // Only the user can fix this, and retrying costs them Nextcloud's + // per-IP brute-force throttle — which takes their *other* clients + // down with it. + is SyncEngine.Result.NeedsSignIn -> Result.failure() + + is SyncEngine.Result.Misconfigured -> Result.failure() + } + } companion object { /** One in-flight sync per account, so a manual trigger cannot pile up. */ fun uniqueNameFor(accountName: String) = "caldav-sync:$accountName" const val KEY_ACCOUNT_NAME = "accountName" + + private const val TAG = "SyncWorker" } } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/CalendarResource.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/CalendarResource.kt new file mode 100644 index 0000000..380ddcc --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/CalendarResource.kt @@ -0,0 +1,56 @@ +package de.jeanlucmakiola.agendula.data.tasks.ical + +import de.jeanlucmakiola.agendula.domain.ical.ICalComponent +import de.jeanlucmakiola.agendula.domain.ical.ICalParser +import de.jeanlucmakiola.agendula.domain.ical.ICalProperty +import de.jeanlucmakiola.agendula.domain.ical.ICalSerializer +import de.jeanlucmakiola.agendula.domain.ical.VTimeZones + +/** + * One CalDAV resource: the `VCALENDAR` wrapper around the `VTODO`s that share a + * `UID`. + * + * ⚠️ A resource is **not** a task. RFC 4791 §4.1 requires every component in one + * resource to share a UID, which makes a recurring task and all of its + * `RECURRENCE-ID` overrides exactly one resource — and makes two unrelated tasks + * in one resource unuploadable. The engine works in resources; the mapper works + * in components. + */ +object CalendarResource { + + const val PRODUCT_ID = "-//Jean-Luc Makiola//Agendula//EN" + const val VERSION = "2.0" + + /** Wraps [vtodos] with the `VTIMEZONE`s their `TZID` parameters reference. */ + fun build(vtodos: List): ICalComponent { + val zones = vtodos + .flatMap { VTimeZones.forComponent(it) } + .distinctBy { it.property("TZID")?.value } + return ICalComponent( + name = "VCALENDAR", + properties = listOf( + ICalProperty("VERSION", emptyList(), VERSION), + ICalProperty("PRODID", emptyList(), PRODUCT_ID), + ), + // Zones first: a server that streams the object as it parses has the + // definition before the reference. + components = zones + vtodos, + ) + } + + fun serialize(vtodos: List): String = + ICalSerializer.serialize(build(vtodos)) + + /** + * The `VCALENDAR`s in a downloaded body. + * + * More than one is malformed but does happen; the caller decides what to do + * about it rather than having the decision made here by a parser. + */ + fun parse(text: String): List = + ICalParser.parseAll(text).filter { it.name.equals("VCALENDAR", ignoreCase = true) } + + /** Every `VTODO` across [calendars], in document order. */ + fun todosIn(calendars: List): List = + calendars.flatMap { it.components("VTODO") } +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/ResourceValidator.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/ResourceValidator.kt new file mode 100644 index 0000000..457f504 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/ical/ResourceValidator.kt @@ -0,0 +1,88 @@ +package de.jeanlucmakiola.agendula.data.tasks.ical + +import de.jeanlucmakiola.agendula.domain.ical.ICalComponent + +/** + * Refuses a resource before it is uploaded. + * + * ⚠️ This exists because **sabre answers 415 for things ordinary UI actions + * produce**, and a 415 is not recoverable by retrying: the row stays dirty, the + * next sync sends the same bytes, and the user sees a task that never leaves the + * device with no explanation. Catching it here turns a permanent silent failure + * into one message naming the field. + * + * Every rule below is a real sabre rejection, not defensive tidiness. + */ +object ResourceValidator { + + /** Why a resource cannot be uploaded, in words that name the offending field. */ + @JvmInline + value class Rejection(val reason: String) + + /** Null when [calendar] may be uploaded. */ + fun validate(calendar: ICalComponent): Rejection? { + // An object carrying METHOD is an iTIP *message*, not calendar data. + // RFC 4791 §4.1 forbids it outright in a calendar object resource. + calendar.property("METHOD")?.let { + return Rejection("carries METHOD:${it.value}, which makes it a scheduling message") + } + + val todos = calendar.components("VTODO") + if (todos.isEmpty()) return Rejection("contains no VTODO") + + // ⚠️ Mixed component types in one resource. A VEVENT that arrived in the + // same body and was never modelled must not be re-emitted next to a + // VTODO — RFC 4791 §4.1 allows only one component type per resource. + val foreign = calendar.components + .map { it.name.uppercase() } + .filterNot { it == "VTODO" || it == "VTIMEZONE" } + .distinct() + if (foreign.isNotEmpty()) { + return Rejection("mixes VTODO with ${foreign.joinToString(", ")}") + } + + // One resource, one UID — the constraint that makes "fork the conflicting + // edit into the same resource" impossible, and the one servers enforce. + val uids = todos.map { it.property("UID")?.value.orEmpty() }.distinct() + if (uids.size > 1) return Rejection("holds ${uids.size} different UIDs") + if (uids.singleOrNull().isNullOrBlank()) return Rejection("has no UID") + + // ⚠️ A TZID without a leading solidus must reference a VTIMEZONE in the + // same object (RFC 5545 §3.2.19). We regenerate definitions from + // `java.time`, which cannot resolve a non-IANA id — a Windows zone name + // that arrived from another client and survives in the residue produces a + // reference with nothing behind it. `Prefer: handling=strict` turns that + // from a server-side repair into a rejection, so it is caught by name + // here instead of as an unexplained 415. + val defined = calendar.components("VTIMEZONE") + .mapNotNull { it.property("TZID")?.value } + .toSet() + val undefined = todos.flatMap(::tzidsIn).filterNot { it in defined }.distinct() + if (undefined.isNotEmpty()) { + return Rejection("references the unknown time zone ${undefined.first()}") + } + + return todos.firstNotNullOfOrNull(::validateTodo) + } + + /** + * ⚠️ The per-component rules are [VTodoMapper.validate]'s, not a second copy. + * + * Two value-type tests that disagree is worse than one: `ResourceValidator` + * originally tested only `VALUE=DATE`, while the mapper also treats a bare + * eight-digit value as a DATE, so a residue `DTSTART:20260101` beside an + * authored `DUE;VALUE=DATE:20260102` was rejected as a mismatch and never + * left the device. + */ + private fun validateTodo(todo: ICalComponent): Rejection? { + if (todo.property("DUE") != null && todo.property("DURATION") != null) { + // RFC 5545 §3.6.2: DUE and DURATION are mutually exclusive. + return Rejection("has both DUE and DURATION") + } + return VTodoMapper.validate(todo).firstOrNull()?.let(::Rejection) + } + + private fun tzidsIn(component: ICalComponent): List = + component.properties.mapNotNull { it.param("TZID")?.takeIf(String::isNotBlank) } + + component.components.flatMap(::tzidsIn) +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt index c5747c5..fb75cb3 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskDao.kt @@ -100,6 +100,16 @@ interface TaskDao { @Query("SELECT * FROM tasks WHERE is_dirty = 1") fun dirty(): List + /** + * Every row in a list, tombstones included. + * + * Sync needs the tombstones: a row with `is_deleted = 1` is a DELETE the + * server is still owed, and a query that filters them out is a client that + * resurrects deleted tasks on the next download. + */ + @Query("SELECT * FROM tasks WHERE list_id = :listId") + fun allIn(listId: Long): List + // --- writes --------------------------------------------------------------- @Insert @@ -131,4 +141,31 @@ interface TaskDao { /** Tombstone, for a row a server still knows about. */ @Query("UPDATE tasks SET is_deleted = 1, is_dirty = 1, last_modified = :at WHERE id = :taskId") fun markDeleted(taskId: Long, at: Instant?): Int + + /** + * Re-points a subtask at its parent. + * + * Separate from [update] because `RELATED-TO` arrives as a UID and can only + * be resolved to a row id once every row of the sync is present — and + * resolving it must not disturb `is_dirty`, which a whole-entity update + * would. + */ + @Query("UPDATE tasks SET parent_id = :parentId WHERE id = :taskId") + fun setParent(taskId: Long, parentId: Long?): Int + + /** Hard delete of a whole resource's rows — a master and its overrides. */ + @Query("DELETE FROM tasks WHERE id IN (:taskIds)") + fun deleteAll(taskIds: List): Int + + /** + * Records where a resource lives and which version we hold. + * + * Applied to every row of a resource at once, because a master and its + * `RECURRENCE-ID` overrides share one href and one ETag — they are one file + * on the server. `is_dirty` is cleared **explicitly** rather than left to a + * default: a downstream write that leaves the flag set uploads what was just + * downloaded, which is how a sync loop starts. + */ + @Query("UPDATE tasks SET href = :href, etag = :etag, is_dirty = 0 WHERE id IN (:taskIds)") + fun markSynced(taskIds: List, href: String?, etag: String?): Int } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt index b432a6d..04a1cc8 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/data/tasks/room/TaskListDao.kt @@ -59,4 +59,18 @@ interface TaskListDao { */ @Query("DELETE FROM task_lists WHERE account_id = :accountId") fun deleteForAccount(accountId: Long) + + /** + * Refreshes just the ACL flag. + * + * Not [update]: sync holds a snapshot of the row from before it started, and + * writing that whole entity back would revert a rename or recolour the user + * made while it ran. + */ + @Query("UPDATE task_lists SET is_read_only = :readOnly WHERE id = :listId") + fun setReadOnly(listId: Long, readOnly: Boolean) + + /** The synced collections of one account, in the order sync walks them. */ + @Query("SELECT * FROM task_lists WHERE account_id = :accountId AND is_synced = 1 ORDER BY id") + fun syncedForAccount(accountId: Long): List } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/domain/ical/VTimeZones.kt b/app/src/main/java/de/jeanlucmakiola/agendula/domain/ical/VTimeZones.kt new file mode 100644 index 0000000..c21e8b4 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/agendula/domain/ical/VTimeZones.kt @@ -0,0 +1,160 @@ +package de.jeanlucmakiola.agendula.domain.ical + +import java.time.DayOfWeek +import java.time.LocalDateTime +import java.time.ZoneId +import java.time.ZoneOffset +import java.time.zone.ZoneOffsetTransitionRule + +/** + * Builds a `VTIMEZONE` for an IANA zone id. + * + * ⚠️ This is not optional decoration. RFC 5545 §3.2.19: a `TZID` parameter + * without a leading solidus **must** reference a `VTIMEZONE` in the same + * calendar object. The mapper emits `DTSTART;TZID=Europe/Berlin:…` whenever a + * task carries a zone, so a resource that ships those without a definition is + * malformed — and `Prefer: handling=strict`, which we send precisely so servers + * stop silently repairing our bytes, turns "malformed" from a shrug into a + * rejection. + * + * The definition is generated from `java.time`'s own rules rather than carried + * from the server, which is a deliberate trade. A server's `VTIMEZONE` is opaque + * to us and lives at `VCALENDAR` level, outside the `VTODO` the mapper stores + * residue for; regenerating it means the zone we write always agrees with the + * zone we compute occurrences in. The round-trip corpus already treats + * `VTIMEZONE` bodies as opaque for exactly this reason. + * + * **Bounded on purpose:** only the currently-effective rules are emitted, not + * the full historical transition table. Tasks are dated now or later; reproducing + * a century of political time changes to place a due date is not a trade worth + * making. + */ +object VTimeZones { + + /** The `VTIMEZONE` for [zoneId], or null when the id is not resolvable. */ + fun forZone(zoneId: String): ICalComponent? { + val zone = runCatching { ZoneId.of(zoneId) }.getOrNull() ?: return null + val rules = zone.rules + + val observances = if (rules.transitionRules.isEmpty()) { + // Either a fixed-offset zone, or one whose DST was abolished and + // whose rules therefore ended. Both are a single standard offset from + // here on, which is the only period a task can fall in. + val offset = rules.getStandardOffset(java.time.Instant.now()) + listOf(fixedObservance(offset)) + } else { + rules.transitionRules.map(::observance) + } + + return ICalComponent( + name = "VTIMEZONE", + properties = listOf(ICalProperty("TZID", emptyList(), zoneId)), + components = observances, + ) + } + + /** The `VTIMEZONE`s referenced by any `TZID` parameter under [component]. */ + fun forComponent(component: ICalComponent): List = + tzids(component).sorted().mapNotNull(::forZone) + + private fun tzids(component: ICalComponent): Set { + val here = component.properties.mapNotNull { it.param("TZID") } + return (here + component.components.flatMap { tzids(it) }) + .filter { it.isNotBlank() } + .toSet() + } + + private fun fixedObservance(offset: ZoneOffset) = ICalComponent( + name = "STANDARD", + properties = listOf( + ICalProperty("DTSTART", emptyList(), "19700101T000000"), + ICalProperty("TZOFFSETFROM", emptyList(), format(offset)), + ICalProperty("TZOFFSETTO", emptyList(), format(offset)), + ), + components = emptyList(), + ) + + private fun observance(rule: ZoneOffsetTransitionRule): ICalComponent { + val daylight = rule.offsetAfter.totalSeconds > rule.standardOffset.totalSeconds + // The rule's own first transition, so DTSTART is a real instance of the + // recurrence rather than an arbitrary date that happens to share a month. + val start = rule.createTransition(ANCHOR_YEAR).dateTimeBefore + return ICalComponent( + name = if (daylight) "DAYLIGHT" else "STANDARD", + properties = listOfNotNull( + ICalProperty("DTSTART", emptyList(), formatLocal(start)), + ICalProperty("TZOFFSETFROM", emptyList(), format(rule.offsetBefore)), + ICalProperty("TZOFFSETTO", emptyList(), format(rule.offsetAfter)), + rrule(rule)?.let { ICalProperty("RRULE", emptyList(), it) }, + ), + components = emptyList(), + ) + } + + private fun rrule(rule: ZoneOffsetTransitionRule): String? { + val month = rule.month.value + val day = rule.dayOfMonthIndicator + val dow = rule.dayOfWeek ?: return "FREQ=YEARLY;BYMONTH=$month;BYMONTHDAY=$day" + + val byDay = when { + // ⚠️ java.time does not encode "the last " as -1. The EU rule + // arrives as dayOfMonthIndicator = 25 — "the first Sunday on or after + // the 25th" — and only the fact that a seven-day window ending on the + // last day of the month *is* the last occurrence identifies it. + // Missing this emits a seven-value BYMONTHDAY list where every other + // client writes BYDAY=-1SU. + day > 0 && day + 6 == rule.month.maxLength() -> "-1${abbreviate(dow)}" + day == -1 -> "-1${abbreviate(dow)}" + // "the nth ", which java.time encodes as "on or after day + // 1 / 8 / 15 / 22". + day > 0 && (day - 1) % 7 == 0 -> "${(day - 1) / 7 + 1}${abbreviate(dow)}" + else -> null + } + if (byDay != null) return "FREQ=YEARLY;BYMONTH=$month;BYDAY=$byDay" + + // Anything else is "the first on or after day N", which iCalendar + // can only say as a day-of-week plus the seven dates it could land on. + if (day <= 0) return null + val days = (day until day + 7).joinToString(",") + return "FREQ=YEARLY;BYMONTH=$month;BYDAY=${abbreviate(dow)};BYMONTHDAY=$days" + } + + private fun abbreviate(day: DayOfWeek) = when (day) { + DayOfWeek.MONDAY -> "MO" + DayOfWeek.TUESDAY -> "TU" + DayOfWeek.WEDNESDAY -> "WE" + DayOfWeek.THURSDAY -> "TH" + DayOfWeek.FRIDAY -> "FR" + DayOfWeek.SATURDAY -> "SA" + DayOfWeek.SUNDAY -> "SU" + } + + /** `+HHMM`, or `+HHMMSS` for the handful of zones with a sub-minute offset. */ + private fun format(offset: ZoneOffset): String { + val total = offset.totalSeconds + val sign = if (total < 0) "-" else "+" + val abs = kotlin.math.abs(total) + val hours = abs / 3600 + val minutes = (abs % 3600) / 60 + val seconds = abs % 60 + return buildString { + append(sign) + append("%02d%02d".format(hours, minutes)) + if (seconds != 0) append("%02d".format(seconds)) + } + } + + private fun formatLocal(time: LocalDateTime): String = + "%04d%02d%02dT%02d%02d%02d".format( + time.year, time.monthValue, time.dayOfMonth, + time.hour, time.minute, time.second, + ) + + /** + * The year the observance `DTSTART`s are anchored to. + * + * Any year the rule applies in produces an equivalent definition; 1970 is + * the conventional choice and keeps generated bodies stable across runs. + */ + private const val ANCHOR_YEAR = 1970 +} diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt index 098f104..42b5762 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsScreen.kt @@ -7,8 +7,10 @@ import androidx.compose.foundation.layout.padding import androidx.compose.material.icons.Icons import androidx.compose.material.icons.rounded.Add import androidx.compose.material.icons.rounded.CloudSync +import androidx.compose.material.icons.rounded.Sync import androidx.compose.material3.AlertDialog import androidx.compose.material3.Icon +import androidx.compose.material3.IconButton import androidx.compose.material3.MaterialTheme import androidx.compose.material3.Text import androidx.compose.material3.TextButton @@ -87,6 +89,14 @@ internal fun AccountsScreen( }, position = positionOf(index, loaded.size), modifier = Modifier.padding(horizontal = 16.dp), + trailing = { + IconButton(onClick = { viewModel.syncNow(account) }) { + Icon( + Icons.Rounded.Sync, + contentDescription = stringResource(R.string.accounts_sync_now), + ) + } + }, onClick = { pendingRemoval = account }, ) } diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt index dc38bee..ddc8c34 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/accounts/AccountsViewModel.kt @@ -4,6 +4,7 @@ import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel import de.jeanlucmakiola.agendula.data.sync.AccountRepository +import de.jeanlucmakiola.agendula.data.sync.SyncTrigger import de.jeanlucmakiola.agendula.data.tasks.room.AccountEntity import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow @@ -14,6 +15,7 @@ import javax.inject.Inject @HiltViewModel class AccountsViewModel @Inject constructor( private val repository: AccountRepository, + private val syncTrigger: SyncTrigger, ) : ViewModel() { private val _accounts = MutableStateFlow?>(null) @@ -29,6 +31,17 @@ class AccountsViewModel @Inject constructor( viewModelScope.launch { _accounts.value = repository.all() } } + /** + * The app's own sync trigger. + * + * Not `ContentResolver.requestSync`: that is gated behind + * `hasAuthorityAccess()` at our targetSdk and returns silently when it + * refuses, which would leave the user pressing a button that does nothing. + */ + fun syncNow(account: AccountEntity) { + syncTrigger.enqueue(account.displayName) + } + fun remove(account: AccountEntity) { viewModelScope.launch { repository.remove(account.id, account.displayName) diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index f38c2e2..517f61e 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -307,6 +307,7 @@ Remove Never synced Last sync didn\u2019t finish + Sync now Add an account Email address or server address 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 new file mode 100644 index 0000000..49669c8 --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/CollectionSyncerTest.kt @@ -0,0 +1,467 @@ +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.PutOutcome +import org.junit.jupiter.api.Test +import kotlin.time.Instant + +class CollectionSyncerTest { + + private val store = FakeStore() + private val remote = FakeRemote() + private val quarantine = mutableMapOf() + private val list = TaskListEntity( + id = 1, + name = "Tasks", + color = 0, + accountId = 7, + href = "http://server/dav/tasks/", + ) + + private fun sync() = CollectionSyncer(store) { NOW }.sync(list, remote, quarantine) + + // -------------------------------------------------------------- download + + @Test fun `a new remote task is downloaded and is not dirty`() { + remote.put("one.ics", vtodo("a", "Buy milk")) + + val report = sync() + + val row = store.rows.single() + assertThat(row.uid).isEqualTo("a") + assertThat(row.title).isEqualTo("Buy milk") + assertThat(row.href).isEqualTo("http://server/dav/tasks/one.ics") + assertThat(row.etag).isEqualTo("e-one.ics") + // ⚠️ Explicit, not defaulted. A downstream write that leaves this set + // uploads what was just downloaded, and that is how a sync loop starts. + assertThat(row.isDirty).isFalse() + assertThat(report.downloaded).isEqualTo(1) + } + + @Test fun `an unchanged etag is not downloaded again`() { + remote.put("one.ics", vtodo("a", "Buy milk")) + sync() + remote.log.clear() + + sync() + + assertThat(remote.log).containsExactly("LIST") + } + + @Test fun `a changed etag is downloaded`() { + remote.put("one.ics", vtodo("a", "Buy milk")) + sync() + remote.put("one.ics", vtodo("a", "Buy oat milk"), eTag = "e-2") + remote.log.clear() + + sync() + + assertThat(store.rows.single().title).isEqualTo("Buy oat milk") + assertThat(remote.log).contains("FETCH one.ics") + } + + @Test fun `a weak remote etag is never stored`() { + remote.put("one.ics", vtodo("a", "Buy milk")) + remote.resources.values.single().eTag = + de.jeanlucmakiola.caldav.ETag("weak", weak = true) + + sync() + + // Storing it would make the next conditional write look conditional + // while silently not being one. + assertThat(store.rows.single().etag).isNull() + } + + @Test fun `local-only columns survive a download`() { + store.rows += task(id = 5, uid = "a", href = "http://server/dav/tasks/one.ics") + .copy(etag = "old", sortOrder = 42, color = 0xFF00FF00.toInt()) + remote.put("one.ics", vtodo("a", "Renamed"), eTag = "new") + + sync() + + val row = store.rows.single() + assertThat(row.title).isEqualTo("Renamed") + // The server has no opinion about either, so taking the mapper's defaults + // would silently reset the user's ordering and colour on every sync. + assertThat(row.sortOrder).isEqualTo(42) + assertThat(row.color).isEqualTo(0xFF00FF00.toInt()) + assertThat(row.id).isEqualTo(5) + } + + @Test fun `RELATED-TO is resolved to a row id once both rows exist`() { + // The child arrives first, so an eager resolution would lose the link. + remote.put("child.ics", vtodo("child", "Sub", extra = "RELATED-TO;RELTYPE=PARENT:parent")) + remote.put("parent.ics", vtodo("parent", "Top")) + + sync() + + val parent = store.rows.single { it.uid == "parent" } + val child = store.rows.single { it.uid == "child" } + assertThat(child.parentId).isEqualTo(parent.id) + } + + @Test fun `an unreadable body quarantines just that resource`() { + remote.put("bad.ics", "this is not iCalendar") + remote.put("good.ics", vtodo("a", "Fine")) + + val report = sync() + + assertThat(store.rows.map { it.uid }).containsExactly("a") + assertThat(report.quarantined.single().href).endsWith("bad.ics") + assertThat(quarantine.values.single()).isEqualTo(1) + } + + // ---------------------------------------------------------------- upload + + @Test fun `a new local task is created and its href recorded`() { + store.rows += task(id = 1, uid = "a", title = "Write it down").copy(isDirty = true) + + val report = sync() + + assertThat(remote.resources.keys.single()).endsWith("/a.ics") + val row = store.rows.single() + assertThat(row.href).isEqualTo("http://server/dav/tasks/a.ics") + assertThat(row.etag).isEqualTo("e-a.ics") + assertThat(row.isDirty).isFalse() + assertThat(report.uploaded).isEqualTo(1) + } + + @Test fun `an edited task is updated with If-Match`() { + remote.put("one.ics", vtodo("a", "Old")) + store.rows += task(id = 1, uid = "a", title = "New") + .copy(href = "http://server/dav/tasks/one.ics", etag = "e-one.ics", isDirty = true) + + sync() + + assertThat(remote.log).contains("UPDATE one.ics if-match=e-one.ics") + assertThat(remote.resources.values.single().body).contains("SUMMARY:New") + } + + @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) } + + sync() + + // Not an error: the resource is on the server, but without a usable + // validator and possibly not as the bytes we sent. + assertThat(remote.log).contains("FETCH a.ics") + assertThat(store.rows.single().etag).isNull() + } + + @Test fun `a rejected upload quarantines and leaves the row dirty`() { + store.rows += task(id = 1, uid = "a", title = "Bad").copy(isDirty = true) + remote.onPut = { PutOutcome.Rejected(415, "Unsupported Media Type") } + + val report = sync() + + assertThat(report.quarantined.single().reason).contains("415") + // Retrying identical bytes cannot help, but the edit is not thrown away. + assertThat(store.rows.single().isDirty).isTrue() + } + + @Test fun `a quarantined resource is skipped once it hits the threshold`() { + remote.put("one.ics", vtodo("a", "Server")) + store.rows += task(id = 1, uid = "a", title = "Mine") + .copy(href = "http://server/dav/tasks/one.ics", etag = "e-one.ics", isDirty = true) + remote.onPut = { PutOutcome.Rejected(400, "Bad Request") } + + repeat(QuarantineStore.THRESHOLD) { sync() } + remote.log.clear() + sync() + + // ⚠️ A single 400 on one resource has halted all calendar sync in DAVx5 + // for weeks. After the threshold this one is left alone and the rest of + // the collection still runs. + assertThat(remote.log).containsExactly("LIST") + } + + @Test fun `an invalid body never leaves the device`() { + store.rows += task(id = 1, uid = "a", title = "Backwards").copy( + isDirty = true, + dtstart = Instant.parse("2026-03-01T10:00:00Z"), + due = Instant.parse("2026-03-01T09:00:00Z"), + ) + + val report = sync() + + assertThat(remote.log).containsExactly("LIST") + assertThat(report.quarantined.single().reason).contains("DUE precedes DTSTART") + } + + // -------------------------------------------------------------- conflict + + @Test fun `a 412 on update discards the local edit and reports it`() { + remote.put("one.ics", vtodo("a", "Server won"), eTag = "newer") + store.rows += task(id = 1, uid = "a", title = "Mine") + .copy(href = "http://server/dav/tasks/one.ics", etag = "stale", isDirty = true) + + val report = sync() + + // Decision 2: server wins. The report is the other half of that policy — + // without it this is indistinguishable from data loss. + assertThat(report.discardedEdits.single().cause) + .isEqualTo(DiscardedEdit.Cause.SERVER_NEWER) + assertThat(report.discardedEdits.single().title).isEqualTo("Mine") + assertThat(store.rows.single().title).isEqualTo("Server won") + assertThat(store.rows.single().isDirty).isFalse() + } + + @Test fun `a lost 412 conflict keeps the edit when the replacement never arrives`() { + remote.put("one.ics", vtodo("a", "Server won"), eTag = "newer") + store.rows += task(id = 1, uid = "a", title = "Mine") + .copy(href = "http://server/dav/tasks/one.ics", etag = "stale", isDirty = true) + // The 412 lands, then the replacement download dies. + remote.resources["http://server/dav/tasks/one.ics"]!!.body = "not iCalendar" + + val report = sync() + + // Nothing was replaced, so nothing was discarded — and the edit is still + // there to try again with. + assertThat(report.discardedEdits).isEmpty() + assertThat(store.rows.single().title).isEqualTo("Mine") + assertThat(store.rows.single().isDirty).isTrue() + } + + @Test fun `an edit to a task deleted on the server loses`() { + store.rows += task(id = 1, uid = "a", title = "Mine") + .copy(href = "http://server/dav/tasks/gone.ics", etag = "e", isDirty = true) + + val report = sync() + + assertThat(store.rows).isEmpty() + assertThat(report.discardedEdits.single().cause) + .isEqualTo(DiscardedEdit.Cause.DELETED_ON_SERVER) + } + + @Test fun `a delete that loses to a server edit is undone`() { + remote.put("one.ics", vtodo("a", "Server changed it"), eTag = "newer") + store.rows += task(id = 1, uid = "a", title = "Mine").copy( + href = "http://server/dav/tasks/one.ics", + etag = "stale", + isDeleted = true, + isDirty = true, + ) + + val report = sync() + + assertThat(report.discardedEdits.single().cause) + .isEqualTo(DiscardedEdit.Cause.DELETE_LOST) + val row = store.rows.single() + assertThat(row.isDeleted).isFalse() + assertThat(row.title).isEqualTo("Server changed it") + } + + // -------------------------------------------------------------- deletion + + @Test fun `a tombstone with an href is deleted on the server`() { + remote.put("one.ics", vtodo("a", "Gone")) + store.rows += task(id = 1, uid = "a") + .copy(href = "http://server/dav/tasks/one.ics", etag = "e-one.ics", isDeleted = true) + + val report = sync() + + assertThat(remote.resources).isEmpty() + assertThat(store.rows).isEmpty() + assertThat(report.deletedRemotely).isEqualTo(1) + } + + @Test fun `a task deleted before it was ever uploaded is never DELETEd`() { + store.rows += task(id = 1, uid = "a").copy(isDeleted = true, isDirty = true) + + sync() + + // A DELETE here would 404 on every sync, forever. + assertThat(remote.log).containsExactly("LIST") + assertThat(store.rows).isEmpty() + } + + @Test fun `the sweep does not remove what this run just created`() { + store.rows += task(id = 1, uid = "a", title = "Fresh").copy(isDirty = true) + + sync() + + // The listing was taken before the upload, so the new href is not in it. + assertThat(store.rows).hasSize(1) + assertThat(remote.resources).hasSize(1) + } + + @Test fun `an empty listing never sweeps`() { + store.rows += task(id = 1, uid = "a", title = "Mine") + .copy(href = "http://server/dav/tasks/one.ics", etag = "e") + store.rows += task(id = 2, uid = "b", title = "Also mine") + .copy(href = "http://server/dav/tasks/two.ics", etag = "e") + + val report = sync() + + // ⚠️ A server that mishandles the VTODO comp-filter answers with an empty + // *successful* multistatus, which is indistinguishable from an empty + // collection. Sweeping on that evidence hard-deletes every task at once. + assertThat(store.rows).hasSize(2) + assertThat(report.deletedLocally).isEqualTo(0) + assertThat(report.failure).contains("listed no tasks") + } + + @Test fun `a resource missing from a non-empty listing is swept`() { + remote.put("one.ics", vtodo("a", "Still there")) + store.rows += task(id = 1, uid = "a") + .copy(href = "http://server/dav/tasks/one.ics", etag = "e-one.ics") + store.rows += task(id = 2, uid = "b") + .copy(href = "http://server/dav/tasks/two.ics", etag = "e") + + val report = sync() + + assertThat(store.rows.map { it.uid }).containsExactly("a") + assertThat(report.deletedLocally).isEqualTo(1) + } + + @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") } + + repeat(QuarantineStore.THRESHOLD) { sync() } + remote.log.clear() + sync() + + // A resource with no href yet still has to be countable, or it is re-PUT + // on every sync forever. + assertThat(remote.log).containsExactly("LIST") + assertThat(quarantine.keys.single()).isEqualTo("${list.id}|uid:a") + } + + @Test fun `a successful create clears the counter it accrued as a uid`() { + store.rows += task(id = 1, uid = "a", title = "Flaky").copy(isDirty = true) + remote.onPut = { PutOutcome.Failed("network") } + sync() + assertThat(quarantine).isNotEmpty() + + remote.onPut = null + sync() + + // Both keys, or the uid-keyed one leaks into the store forever. + assertThat(quarantine).isEmpty() + } + + @Test fun `a RELATED-TO removed on the server unparents the task`() { + remote.put("parent.ics", vtodo("parent", "Top")) + remote.put("child.ics", vtodo("child", "Sub", extra = "RELATED-TO;RELTYPE=PARENT:parent")) + sync() + assertThat(store.rows.single { it.uid == "child" }.parentId).isNotNull() + + remote.put("child.ics", vtodo("child", "Sub"), eTag = "e-2") + sync() + + // Carrying the old parent_id forward would re-upload, on the next local + // edit, the relationship the user deleted elsewhere. + assertThat(store.rows.single { it.uid == "child" }.parentId).isNull() + } + + // ---------------------------------------------------------- name clashes + + @Test fun `a name taken by another task gets a fresh one`() { + remote.put("a.ics", vtodo("somebody-else", "Not ours")) + store.rows += task(id = 1, uid = "a", title = "Ours").copy(isDirty = true) + + sync() + + val ours = store.rows.single { it.uid == "a" } + assertThat(ours.href).isNotEqualTo("http://server/dav/tasks/a.ics") + assertThat(ours.href).endsWith(".ics") + assertThat(ours.isDirty).isFalse() + } + + @Test fun `a name taken by the same task is adopted`() { + // A previous run's PUT whose answer we never saw. + remote.put("a.ics", vtodo("a", "Uploaded last time")) + store.rows += task(id = 1, uid = "a", title = "Ours").copy(isDirty = true) + + sync() + + assertThat(remote.resources).hasSize(1) + assertThat(store.rows.single().href).isEqualTo("http://server/dav/tasks/a.ics") + } + + // ------------------------------------------------------------ protection + + @Test fun `a read-only collection is never written to`() { + remote.readOnly = true + store.rows += task(id = 1, uid = "a", title = "Mine").copy(isDirty = true) + + val report = sync() + + assertThat(remote.log).containsExactly("LIST") + assertThat(report.quarantined.single().reason).contains("read-only") + // ⚠️ ACL churn is silent, so the refreshed flag is written back. + assertThat(store.readOnlyWrites.single()).isEqualTo(list.id to true) + } + + @Test fun `a confidential task in a shared collection is not written back`() { + remote.shared = true + remote.put("one.ics", vtodo("a", "Secret")) + store.rows += task(id = 1, uid = "a", title = "Secret").copy( + href = "http://server/dav/tasks/one.ics", + etag = "e-one.ics", + classification = 2, + isDirty = true, + ) + + val report = sync() + + // ⚠️ Nextcloud's CalendarObject::get() serves a whitelist-reduced copy of + // a confidential object from a share while leaving the ETag untouched. + // Writing that back destroys the owner's task, and the ETag matches. + assertThat(remote.log).containsExactly("LIST") + assertThat(report.quarantined.single().reason).contains("reduced copy") + } + + @Test fun `a confidential task the user owns is written normally`() { + remote.shared = false + remote.put("one.ics", vtodo("a", "Secret")) + store.rows += task(id = 1, uid = "a", title = "Secret").copy( + href = "http://server/dav/tasks/one.ics", + etag = "e-one.ics", + classification = 2, + isDirty = true, + ) + + sync() + + assertThat(remote.log).contains("UPDATE one.ics if-match=e-one.ics") + } + + // -------------------------------------------------------------- failures + + @Test fun `a collection that cannot be listed reports rather than throws`() { + remote.listFailure = "boom" + val report = sync() + assertThat(report.failure).contains("boom") + } + + @Test fun `a collection that is no longer readable reports rather than throws`() { + remote.stateFailure = "revoked" + val report = sync() + assertThat(report.failure).contains("revoked") + } + + // ----------------------------------------------------------------- setup + + private fun task( + id: Long, + uid: String, + title: String? = null, + href: String? = null, + ) = TaskEntity(id = id, listId = list.id, uid = uid, title = title, href = href) + + private fun vtodo(uid: String, summary: String, extra: String? = null) = buildString { + append("BEGIN:VCALENDAR\r\nVERSION:2.0\r\nBEGIN:VTODO\r\n") + append("UID:$uid\r\nSUMMARY:$summary\r\n") + extra?.let { append("$it\r\n") } + append("END:VTODO\r\nEND:VCALENDAR\r\n") + } + + private companion object { + val NOW: Instant = Instant.parse("2026-03-01T12:00:00Z") + } +} 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 new file mode 100644 index 0000000..f5af1f8 --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/sync/SyncFakes.kt @@ -0,0 +1,157 @@ +package de.jeanlucmakiola.agendula.data.sync + +import de.jeanlucmakiola.agendula.data.tasks.room.TaskEntity +import de.jeanlucmakiola.caldav.CalendarCollection +import de.jeanlucmakiola.caldav.DeleteOutcome +import de.jeanlucmakiola.caldav.ETag +import de.jeanlucmakiola.caldav.FetchResult +import de.jeanlucmakiola.caldav.PutOutcome +import de.jeanlucmakiola.caldav.RemoteCalendar +import de.jeanlucmakiola.caldav.RemoteRef +import de.jeanlucmakiola.caldav.RemoteResource +import de.jeanlucmakiola.caldav.TaskCollection +import okhttp3.HttpUrl +import okhttp3.HttpUrl.Companion.toHttpUrl + +/** An in-memory [SyncStore], standing in for Room. */ +class FakeStore(rows: List = emptyList()) : SyncStore { + + val rows = rows.toMutableList() + val readOnlyWrites = mutableListOf>() + private var nextId = (rows.maxOfOrNull { it.id } ?: 0L) + 1 + + override fun rowsIn(listId: Long) = this.rows.filter { it.listId == listId } + + override fun insert(row: TaskEntity): Long { + val id = nextId++ + rows += row.copy(id = id) + return id + } + + override fun update(row: TaskEntity) { + val index = rows.indexOfFirst { it.id == row.id } + if (index >= 0) rows[index] = row + } + + override fun deleteAll(taskIds: List) { + rows.removeAll { it.id in taskIds } + } + + override fun markSynced(taskIds: List, href: String?, eTag: String?) { + taskIds.forEach { id -> + val index = rows.indexOfFirst { it.id == id } + if (index >= 0) { + rows[index] = rows[index].copy(href = href, etag = eTag, isDirty = false) + } + } + } + + override fun setParent(taskId: Long, parentId: Long?) { + val index = rows.indexOfFirst { it.id == taskId } + if (index >= 0) rows[index] = rows[index].copy(parentId = parentId) + } + + override fun masterByUid(listId: Long, uid: String) = + rows.firstOrNull { it.listId == listId && it.uid == uid && it.recurrenceId == null } + + override fun row(taskId: Long) = rows.firstOrNull { it.id == taskId } + + override fun setListReadOnly(listId: Long, readOnly: Boolean) { + readOnlyWrites += listId to readOnly + } +} + +/** An in-memory CalDAV collection that answers like a server rather than a mock. */ +class FakeRemote(override val url: HttpUrl = "http://server/dav/tasks/".toHttpUrl()) : + RemoteCalendar { + + data class Stored(var eTag: ETag?, var body: String) + + val resources = linkedMapOf() + val log = mutableListOf() + + var readOnly = false + var shared = false + var stateFailure: String? = null + var listFailure: String? = null + + /** Returns non-null to pre-empt the default behaviour for that href. */ + var onPut: ((HttpUrl) -> PutOutcome?)? = null + var onDelete: ((HttpUrl) -> DeleteOutcome?)? = null + + fun put(name: String, body: String, eTag: String? = "e-$name") { + resources[url.newBuilder().addPathSegment(name).build().toString()] = + Stored(eTag?.let { ETag(it, weak = false) }, body) + } + + override fun state(): Result { + stateFailure?.let { return Result.failure(IllegalStateException(it)) } + return Result.success( + CalendarCollection.State( + collection = TaskCollection( + url = url, + displayName = "Tasks", + color = null, + readOnly = readOnly, + isShared = shared, + supportsSyncCollection = false, + maxResourceSize = null, + ), + ctag = "ctag", + syncToken = null, + ), + ) + } + + override fun list(): Result> { + listFailure?.let { return Result.failure(IllegalStateException(it)) } + log += "LIST" + return Result.success( + resources.map { (href, stored) -> RemoteRef(href.toHttpUrl(), stored.eTag) }, + ) + } + + override fun fetch(hrefs: List): Result { + log += "FETCH ${hrefs.joinToString(",") { it.pathSegments.last() }}" + val found = hrefs.mapNotNull { href -> + resources[href.toString()]?.let { RemoteResource(href, it.eTag, it.body) } + } + return Result.success( + FetchResult( + resources = found, + missing = hrefs.filterNot { it.toString() in resources }, + unsolicited = emptyList(), + ), + ) + } + + override fun create(name: String, iCalendar: String): PutOutcome { + val href = url.newBuilder().addPathSegment(name).build() + log += "CREATE $name" + onPut?.invoke(href)?.let { return it } + if (href.toString() in resources) return PutOutcome.NameTaken(href) + val eTag = ETag("e-$name", weak = false) + resources[href.toString()] = Stored(eTag, iCalendar) + return PutOutcome.Stored(href, eTag) + } + + override fun update(href: HttpUrl, eTag: String?, iCalendar: String): PutOutcome { + log += "UPDATE ${href.pathSegments.last()} if-match=$eTag" + onPut?.invoke(href)?.let { return it } + val stored = resources[href.toString()] ?: return PutOutcome.Vanished + if (eTag != null && stored.eTag?.value != eTag) return PutOutcome.ServerNewer + val fresh = ETag("e-${resources.size}-${iCalendar.hashCode()}", weak = false) + stored.eTag = fresh + stored.body = iCalendar + return PutOutcome.Stored(href, fresh) + } + + override fun delete(href: HttpUrl, eTag: String?): DeleteOutcome { + log += "DELETE ${href.pathSegments.last()} if-match=$eTag" + onDelete?.invoke(href)?.let { return it } + val stored = resources[href.toString()] ?: return DeleteOutcome.Deleted + if (eTag != null && stored.eTag?.value != eTag) return DeleteOutcome.ServerNewer + resources.remove(href.toString()) + return DeleteOutcome.Deleted + } +} diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ical/CalendarResourceTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ical/CalendarResourceTest.kt new file mode 100644 index 0000000..28daab8 --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ical/CalendarResourceTest.kt @@ -0,0 +1,60 @@ +package de.jeanlucmakiola.agendula.data.tasks.ical + +import com.google.common.truth.Truth.assertThat +import de.jeanlucmakiola.agendula.domain.ical.ICalParam +import de.jeanlucmakiola.agendula.domain.ical.ICalComponent +import de.jeanlucmakiola.agendula.domain.ical.ICalProperty +import org.junit.jupiter.api.Test + +class CalendarResourceTest { + + @Test fun `a zoned task carries the VTIMEZONE it references`() { + val todo = todo(ICalParam("TZID", "Europe/Berlin")) + + val text = CalendarResource.serialize(listOf(todo)) + + // ⚠️ RFC 5545 §3.2.19: a TZID without a leading solidus must reference a + // VTIMEZONE in the same object. Omitting it makes the resource malformed, + // and `Prefer: handling=strict` turns malformed into rejected. + assertThat(text).contains("BEGIN:VTIMEZONE") + assertThat(text).contains("TZID:Europe/Berlin") + // Definition before reference, for a server that parses as it streams. + assertThat(text.indexOf("BEGIN:VTIMEZONE")).isLessThan(text.indexOf("BEGIN:VTODO")) + } + + @Test fun `a UTC task carries no timezone at all`() { + val text = CalendarResource.serialize(listOf(todo())) + assertThat(text).doesNotContain("VTIMEZONE") + } + + @Test fun `one definition serves every task that shares a zone`() { + val text = CalendarResource.serialize( + listOf(todo(ICalParam("TZID", "Europe/Berlin")), todo(ICalParam("TZID", "Europe/Berlin"))), + ) + assertThat(text.split("BEGIN:VTIMEZONE")).hasSize(2) + } + + @Test fun `the wrapper is a parseable VCALENDAR`() { + val text = CalendarResource.serialize(listOf(todo())) + val parsed = CalendarResource.parse(text) + + assertThat(parsed).hasSize(1) + assertThat(parsed.single().property("VERSION")!!.value).isEqualTo("2.0") + assertThat(parsed.single().property("PRODID")!!.value) + .isEqualTo(CalendarResource.PRODUCT_ID) + assertThat(CalendarResource.todosIn(parsed)).hasSize(1) + } + + @Test fun `a body that is not a calendar yields no calendars`() { + assertThat(CalendarResource.parse("BEGIN:VCARD\r\nEND:VCARD\r\n")).isEmpty() + } + + private fun todo(vararg params: ICalParam) = ICalComponent( + name = "VTODO", + properties = listOf( + ICalProperty("UID", emptyList(), "a"), + ICalProperty("DTSTART", params.toList(), "20260301T090000"), + ), + components = emptyList(), + ) +} diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ical/ResourceValidatorTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ical/ResourceValidatorTest.kt new file mode 100644 index 0000000..3fd6c21 --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/agendula/data/tasks/ical/ResourceValidatorTest.kt @@ -0,0 +1,207 @@ +package de.jeanlucmakiola.agendula.data.tasks.ical + +import com.google.common.truth.Truth.assertThat +import de.jeanlucmakiola.agendula.domain.ical.ICalParser +import org.junit.jupiter.api.Test + +/** + * Every case here is a real sabre rejection reachable from the UI, not a + * defensive check. A 415 is permanent: the row stays dirty and the next sync + * sends the same bytes forever. + */ +class ResourceValidatorTest { + + @Test fun `an ordinary task passes`() { + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + SUMMARY:Buy milk + END:VTODO + """))).isNull() + } + + @Test fun `DUE before DTSTART is refused`() { + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + DTSTART:20260301T100000Z + DUE:20260301T090000Z + END:VTODO + """))).contains("DUE precedes DTSTART") + } + + @Test fun `equal DUE and DTSTART is allowed`() { + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + DTSTART:20260301T100000Z + DUE:20260301T100000Z + END:VTODO + """))).isNull() + } + + @Test fun `a value-type mismatch is refused`() { + // Reachable: set an all-day due date on a task that already has a timed + // start. + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + DTSTART:20260301T100000Z + DUE;VALUE=DATE:20260302 + END:VTODO + """))).contains("disagree on value type") + } + + @Test fun `two DATE values are not a mismatch`() { + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + DTSTART;VALUE=DATE:20260301 + DUE;VALUE=DATE:20260302 + END:VTODO + """))).isNull() + } + + @Test fun `METHOD makes it a scheduling message`() { + assertThat(reject(""" + BEGIN:VCALENDAR + VERSION:2.0 + METHOD:REQUEST + BEGIN:VTODO + UID:a + END:VTODO + END:VCALENDAR + """)).contains("METHOD") + } + + @Test fun `DUE with DURATION is refused`() { + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + DTSTART:20260301T100000Z + DUE:20260301T110000Z + DURATION:PT1H + END:VTODO + """))).contains("DUE and DURATION") + } + + @Test fun `two UIDs in one resource are refused`() { + // RFC 4791 §4.1 — this is the constraint that makes forking a conflicting + // edit into the same resource impossible. + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + END:VTODO + BEGIN:VTODO + UID:b + END:VTODO + """))).contains("different UIDs") + } + + @Test fun `overrides sharing a UID are fine`() { + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + RRULE:FREQ=DAILY + END:VTODO + BEGIN:VTODO + UID:a + RECURRENCE-ID:20260302T090000Z + END:VTODO + """))).isNull() + } + + @Test fun `a mixed component type is refused`() { + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + END:VTODO + BEGIN:VEVENT + UID:a + END:VEVENT + """))).contains("VEVENT") + } + + @Test fun `a VTIMEZONE alongside the task is not foreign`() { + assertThat(reject(vcalendar(""" + BEGIN:VTIMEZONE + TZID:Europe/Berlin + END:VTIMEZONE + BEGIN:VTODO + UID:a + END:VTODO + """))).isNull() + } + + @Test fun `a missing UID is refused`() { + assertThat(reject(vcalendar(""" + BEGIN:VTODO + SUMMARY:no identity + END:VTODO + """))).contains("no UID") + } + + @Test fun `an implicit DATE does not falsely disagree`() { + // Two value-type tests that disagree block a legitimate PUT: the mapper + // reads a bare eight-digit value as a DATE, and so must this. + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + DTSTART:20260301 + DUE;VALUE=DATE:20260302 + END:VTODO + """))).isNull() + } + + @Test fun `an end-relative reminder with nothing to anchor it is refused`() { + // Reachable from the UI: clear the due date on a task that has an + // end-relative reminder. + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + BEGIN:VALARM + TRIGGER;RELATED=END:-PT15M + END:VALARM + END:VTODO + """))).contains("RELATED=END") + } + + @Test fun `a TZID with no definition is refused`() { + // A Windows zone name from another client survives in the residue, and + // `java.time` cannot regenerate a VTIMEZONE for it. + assertThat(reject(vcalendar(""" + BEGIN:VTODO + UID:a + DTSTART;TZID=W. Europe Standard Time:20260301T090000 + END:VTODO + """))).contains("unknown time zone") + } + + @Test fun `a TZID with its definition present is fine`() { + assertThat(reject(vcalendar(""" + BEGIN:VTIMEZONE + TZID:Europe/Berlin + END:VTIMEZONE + BEGIN:VTODO + UID:a + DTSTART;TZID=Europe/Berlin:20260301T090000 + END:VTODO + """))).isNull() + } + + @Test fun `a calendar with no task is refused`() { + assertThat(reject(vcalendar(""))).contains("no VTODO") + } + + private fun reject(text: String): String? = + ResourceValidator.validate(ICalParser.parse(crlf(text)))?.reason + + private fun vcalendar(body: String) = + "BEGIN:VCALENDAR\nVERSION:2.0\n$body\nEND:VCALENDAR" + + /** Indentation-insensitive, so the fixtures can be written inline. */ + private fun crlf(text: String) = text.lines() + .map { it.trim() } + .filter { it.isNotEmpty() } + .joinToString("\r\n") + "\r\n" +} diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/domain/ical/VTimeZonesTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/domain/ical/VTimeZonesTest.kt new file mode 100644 index 0000000..77748b0 --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/agendula/domain/ical/VTimeZonesTest.kt @@ -0,0 +1,99 @@ +package de.jeanlucmakiola.agendula.domain.ical + +import com.google.common.truth.Truth.assertThat +import org.junit.jupiter.api.Test + +class VTimeZonesTest { + + @Test fun `a DST zone gets both observances`() { + val zone = VTimeZones.forZone("Europe/Berlin")!! + + assertThat(zone.property("TZID")!!.value).isEqualTo("Europe/Berlin") + assertThat(zone.components.map { it.name }) + .containsExactly("STANDARD", "DAYLIGHT") + + val daylight = zone.components("DAYLIGHT").single() + assertThat(daylight.property("TZOFFSETFROM")!!.value).isEqualTo("+0100") + assertThat(daylight.property("TZOFFSETTO")!!.value).isEqualTo("+0200") + // The EU rule is "the last Sunday", which is what java.time encodes as + // dayOfMonthIndicator = -1. + assertThat(daylight.property("RRULE")!!.value) + .isEqualTo("FREQ=YEARLY;BYMONTH=3;BYDAY=-1SU") + } + + @Test fun `a nth-weekday rule becomes an ordinal BYDAY`() { + val zone = VTimeZones.forZone("America/New_York")!! + // US DST starts on the second Sunday in March, which java.time encodes as + // "on or after the 8th". + assertThat(zone.components("DAYLIGHT").single().property("RRULE")!!.value) + .isEqualTo("FREQ=YEARLY;BYMONTH=3;BYDAY=2SU") + } + + @Test fun `a fixed-offset zone gets one standard observance and no rule`() { + val zone = VTimeZones.forZone("Asia/Kolkata")!! + val standard = zone.components.single() + + assertThat(standard.name).isEqualTo("STANDARD") + assertThat(standard.property("TZOFFSETTO")!!.value).isEqualTo("+0530") + assertThat(standard.property("RRULE")).isNull() + } + + @Test fun `UTC is representable`() { + val zone = VTimeZones.forZone("UTC")!! + assertThat(zone.components.single().property("TZOFFSETTO")!!.value).isEqualTo("+0000") + } + + @Test fun `a negative offset keeps its sign`() { + val zone = VTimeZones.forZone("America/New_York")!! + assertThat(zone.components("STANDARD").single().property("TZOFFSETTO")!!.value) + .isEqualTo("-0500") + } + + @Test fun `an unknown zone is null rather than an exception`() { + assertThat(VTimeZones.forZone("Mars/Olympus_Mons")).isNull() + assertThat(VTimeZones.forZone("")).isNull() + } + + @Test fun `observance DTSTART is a real instance of its own rule`() { + val daylight = VTimeZones.forZone("Europe/Berlin")!!.components("DAYLIGHT").single() + val start = daylight.property("DTSTART")!!.value + // 1970-03-29 was a Sunday, and the last one in March. + assertThat(start).isEqualTo("19700329T020000") + } + + @Test fun `zones are collected from every TZID in the tree`() { + val todo = ICalComponent( + name = "VTODO", + properties = listOf( + ICalProperty("DTSTART", listOf(ICalParam("TZID", "Europe/Berlin")), "20260101T090000"), + ICalProperty("DUE", listOf(ICalParam("TZID", "Europe/Berlin")), "20260101T100000"), + ), + components = listOf( + ICalComponent( + name = "VALARM", + properties = listOf( + ICalProperty( + "TRIGGER", + listOf(ICalParam("TZID", "America/New_York")), + "20260101T080000", + ), + ), + components = emptyList(), + ), + ), + ) + + val zones = VTimeZones.forComponent(todo).map { it.property("TZID")!!.value } + // Deduplicated, and the nested one is not missed. + assertThat(zones).containsExactly("America/New_York", "Europe/Berlin").inOrder() + } + + @Test fun `a component with no TZID needs no definitions`() { + val todo = ICalComponent( + name = "VTODO", + properties = listOf(ICalProperty("DTSTART", emptyList(), "20260101T090000Z")), + components = emptyList(), + ) + assertThat(VTimeZones.forComponent(todo)).isEmpty() + } +} diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt index 522d615..8218edb 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavDiscovery.kt @@ -2,15 +2,8 @@ package de.jeanlucmakiola.caldav import at.bitfire.dav4jvm.DavResource import at.bitfire.dav4jvm.Response -import at.bitfire.dav4jvm.property.CalendarColor import at.bitfire.dav4jvm.property.CalendarHomeSet import at.bitfire.dav4jvm.property.CurrentUserPrincipal -import at.bitfire.dav4jvm.property.CurrentUserPrivilegeSet -import at.bitfire.dav4jvm.property.DisplayName -import at.bitfire.dav4jvm.property.MaxICalendarSize -import at.bitfire.dav4jvm.property.ResourceType -import at.bitfire.dav4jvm.property.SupportedCalendarComponentSet -import at.bitfire.dav4jvm.property.SupportedReportSet import okhttp3.HttpUrl import okhttp3.HttpUrl.Companion.toHttpUrlOrNull import okhttp3.OkHttpClient @@ -38,16 +31,13 @@ class CalDavDiscovery( private val allowCleartext: Boolean = false, ) { - /** Properties requested **by name**, because allprop legitimately omits them. */ - private val collectionProperties = arrayOf( - ResourceType.NAME, - DisplayName.NAME, - CalendarColor.NAME, - SupportedCalendarComponentSet.NAME, - SupportedReportSet.NAME, - CurrentUserPrivilegeSet.NAME, - MaxICalendarSize.NAME, - ) + /** + * Properties requested **by name**, because allprop legitimately omits them. + * + * Owned by [CollectionClassifier], which is what reads them back — a second + * copy here would drift the day one of them is added. + */ + private val collectionProperties = CollectionClassifier.PROPERTIES /** A home set that could not be listed. One failing must not hide the others. */ data class HomeSetFailure( diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt index feb43f1..ba520c5 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalDavHttp.kt @@ -3,6 +3,7 @@ package de.jeanlucmakiola.caldav import at.bitfire.dav4jvm.BasicDigestAuthHandler import at.bitfire.dav4jvm.UrlUtils import okhttp3.HttpUrl +import okhttp3.Interceptor import okhttp3.OkHttpClient import java.util.concurrent.TimeUnit @@ -81,9 +82,39 @@ object CalDavHttp { } private fun base(userAgent: String) = shared.newBuilder() + .addInterceptor(calendarHeaders) .addInterceptor { chain -> chain.proceed( chain.request().newBuilder().header("User-Agent", userAgent).build(), ) } + + /** + * The two headers that decide whether ETags are usable and whether our bytes + * survive the server. + * + * **`Accept-Encoding: identity`.** Setting it by hand disables OkHttp's + * transparent gzip, and that is the point: ⚠️ a compressing intermediary + * *weakens the ETag*. Any gzip-enabled nginx, Cloudflare or Traefik in front + * of an otherwise perfect server turns every strong validator into `W/"…"`, + * and a weak tag cannot be used with `If-Match` (RFC 9110 §13.1.1) — so + * conditional writes stop working for reasons that live in the user's reverse + * proxy rather than in their CalDAV server. Trading some bandwidth for a + * working conditional PUT is the right side of that deal. + * + * **`Prefer: handling=strict` on writes.** sabre-based servers (Baïkal, + * Nextcloud, ownCloud, SabreDAV itself) run vobject's `REPAIR` over anything + * uploaded unless this is set, and `Server::createFile` then deliberately + * withholds the ETag because the stored bytes are no longer ours. Strict + * handling keeps both. RFC 7240 §2 requires an unrecognised preference to be + * ignored, so sending it to every server costs nothing. + */ + private val calendarHeaders = Interceptor { chain -> + val request = chain.request() + val builder = request.newBuilder().header("Accept-Encoding", "identity") + if (request.method in WRITE_METHODS) builder.header("Prefer", "handling=strict") + chain.proceed(builder.build()) + } + + private val WRITE_METHODS = setOf("PUT", "POST", "PATCH") } diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt new file mode 100644 index 0000000..6530efc --- /dev/null +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarCollection.kt @@ -0,0 +1,288 @@ +package de.jeanlucmakiola.caldav + +import at.bitfire.dav4jvm.DavCalendar +import at.bitfire.dav4jvm.DavResource +import at.bitfire.dav4jvm.Response +import at.bitfire.dav4jvm.exception.DavException +import at.bitfire.dav4jvm.exception.HttpException +import at.bitfire.dav4jvm.property.CalendarData +import at.bitfire.dav4jvm.property.GetCTag +import at.bitfire.dav4jvm.property.GetETag +import at.bitfire.dav4jvm.property.SyncToken +import okhttp3.HttpUrl +import okhttp3.OkHttpClient +import okhttp3.RequestBody.Companion.toRequestBody +import java.io.IOException + +/** + * One remote task collection, as a set of operations rather than a set of HTTP + * calls. Every method here returns an outcome; none of them throw for anything a + * server can legitimately answer. + * + * This class holds **no task-domain type and no Android type**, per the module + * boundary — it deals in `String` bodies of `text/calendar`. Deciding what those + * bodies mean is the engine's job, one module up. + */ +class CalendarCollection( + private val httpClient: OkHttpClient, + override val url: HttpUrl, +) : RemoteCalendar { + + private val dav get() = DavCalendar(httpClient, url) + + /** What a Depth-0 PROPFIND says about the collection right now. */ + data class State( + val collection: TaskCollection, + val ctag: String?, + val syncToken: String?, + ) + + /** + * Re-reads the collection's own properties. + * + * Worth doing on every sync rather than trusting what account-add recorded: + * ⚠️ ACL churn is real — a calendar shared with the user can be revoked, or + * demoted from read-write to read-only, with no notification of any kind. A + * stale "writable" flag turns every upload into a 403 the user cannot act on, + * and a stale "not shared" flag disarms the guard that keeps us from writing + * a mutilated body back over the owner's task. + */ + override fun state(): Result = runCatching { + var found: State? = null + var fromSelf = false + DavResource(httpClient, url).propfind( + 0, + *CollectionClassifier.PROPERTIES, + GetCTag.NAME, + SyncToken.NAME, + ) { response, relation -> + val isSelf = relation == Response.HrefRelation.SELF + // ⚠️ SELF wins outright and nothing overwrites it. A Depth-0 PROPFIND + // that also reports a member — which some servers send — would + // otherwise have that member classified as the collection's own + // state. The non-SELF branch exists only because a server whose href + // differs from ours by a trailing slash is classified OTHER, and + // refusing those outright breaks it against real deployments. + if (!isSelf && (fromSelf || found != null)) return@propfind + + val collection = CollectionClassifier.classify(response) ?: return@propfind + found = State( + collection = collection, + ctag = response[GetCTag::class.java]?.cTag, + syncToken = response[SyncToken::class.java]?.token, + ) + if (isSelf) fromSelf = true + } + found ?: throw IOException("$url is no longer a task collection") + } + + /** + * Every VTODO resource in the collection, as href + ETag. + * + * ⚠️ **No time-range filter.** DAVx5 omits it for the same reason: some + * servers return nothing at all for a `VTODO` comp-filter that carries one, + * and a task without `DTSTART` or `DUE` is outside every range by + * construction — which is most tasks. + * + * ⚠️ **No `calendar-data` in this REPORT.** A `calendar-query` that asks for + * bodies downloads the whole collection on every listing; worse, the bodies + * would arrive without a matched ETag from the same response. Bodies come + * from [fetch] only. + */ + override fun list(): Result> = runCatching { + val refs = mutableListOf() + dav.calendarQuery(COMPONENT, null, null) { response, relation -> + if (relation != Response.HrefRelation.MEMBER) return@calendarQuery + if (!response.isSuccess()) return@calendarQuery + refs += RemoteRef(response.href, ETag.from(response[GetETag::class.java])) + } + refs + } + + /** + * Downloads [hrefs] with `calendar-multiget`, in batches. + * + * ⚠️ Responses are **matched against the request**. Real servers reply with + * responses for URLs that were never asked for — a collection's own href, a + * sibling, occasionally something from another collection entirely — and a + * client that indexes the response by position or trusts it wholesale will + * apply one resource's body to another's row. + */ + override fun fetch(hrefs: List): Result = fetch(hrefs, MULTIGET_BATCH) + + /** [batchSize] is a seam for tests; production always uses [MULTIGET_BATCH]. */ + internal fun fetch(hrefs: List, batchSize: Int): Result = + runCatching { + val resources = mutableListOf() + val unsolicited = mutableListOf() + val seen = mutableSetOf() + + hrefs.chunked(batchSize).forEach { batch -> + val wanted = batch.associateBy { it.encodedPath } + dav.multiget(batch, MIME_ICALENDAR, ICALENDAR_VERSION) { response, relation -> + if (relation == Response.HrefRelation.SELF) return@multiget + val asked = wanted[response.href.encodedPath] + if (asked == null) { + unsolicited += response.href + return@multiget + } + seen += asked + if (!response.isSuccess()) return@multiget + val body = response[CalendarData::class.java]?.iCalendar ?: return@multiget + resources += RemoteResource( + href = asked, + // The tag from *this* response, paired with *this* body. + eTag = ETag.from(response[GetETag::class.java]), + iCalendar = body, + ) + } + } + + FetchResult( + resources = resources, + missing = hrefs.filterNot { it in seen }, + unsolicited = unsolicited, + ) + } + + /** + * Creates a resource at [name] with `If-None-Match: *`. + * + * The conditional is what makes creation safe to retry: without it a + * re-running worker overwrites whatever a previous run put there. + */ + override fun create(name: String, iCalendar: String): PutOutcome { + val href = url.newBuilder().addPathSegment(name).build() + return put(href, iCalendar, ifETag = null, ifNoneMatch = true) + } + + /** + * Replaces [href], conditional on [eTag] when one is given. + * + * A null [eTag] means the server has never handed us a strong validator for + * this resource, and a conditional write is therefore impossible rather than + * merely inconvenient. The engine decides whether writing anyway is + * acceptable; this class only carries out the decision. + */ + override fun update(href: HttpUrl, eTag: String?, iCalendar: String): PutOutcome = + put(href, iCalendar, ifETag = eTag, ifNoneMatch = false) + + private fun put( + href: HttpUrl, + iCalendar: String, + ifETag: String?, + ifNoneMatch: Boolean, + ): PutOutcome = try { + var outcome: PutOutcome = PutOutcome.StoredNeedsRefetch(href) + DavResource(httpClient, href).put( + body = iCalendar.toRequestBody(DavCalendar.MIME_ICALENDAR_UTF8), + ifETag = ifETag, + ifNoneMatch = ifNoneMatch, + ) { response -> + // The server may have stored the resource somewhere else entirely. + val location = response.header("Location") + ?.let { href.resolve(it) } + ?: href + val eTag = ETag.parse(response.header("ETag")) + outcome = if (eTag != null && eTag.usable) { + PutOutcome.Stored(location, eTag) + } else { + PutOutcome.StoredNeedsRefetch(location) + } + } + outcome + } catch (e: HttpException) { + classifyPutFailure(href, e, ifNoneMatch) + } catch (e: DavException) { + // Not an IOException: a refused HTTPS->HTTP redirect arrives here, and + // it is a configuration problem rather than a transient one. + PutOutcome.Rejected(0, e.toString()) + } catch (e: IOException) { + PutOutcome.Failed(e.toString()) + } + + /** + * ⚠️ **412 means three different things** and merging any two of them is a + * data-loss bug. + */ + private fun classifyPutFailure( + href: HttpUrl, + e: HttpException, + wasCreate: Boolean, + ): PutOutcome = when { + e.code != PRECONDITION_FAILED -> PutOutcome.Rejected(e.code, e.message.orEmpty()) + + // On create the precondition was `If-None-Match: *`, so 412 says only + // that the name is occupied. It says nothing about by what — the engine + // re-fetches and adopts the resource if the UID matches. + wasCreate -> PutOutcome.NameTaken(href) + + // On update the precondition was `If-Match`, and a resource that is gone + // fails it just as one that changed does. RFC 9110 §13.1.1 requires 412 + // rather than 404 once the precondition is evaluated, so the two are + // indistinguishable without asking again. + !exists(href) -> PutOutcome.Vanished + + else -> PutOutcome.ServerNewer + } + + /** + * Deletes [href], conditional on [eTag] when we hold a usable one. + * + * ⚠️ 404 and 410 are **success**. The purpose of a DELETE is for the + * resource to be absent, and it is; treating "already gone" as a failure + * leaves a tombstone that is retried on every sync forever. + */ + override fun delete(href: HttpUrl, eTag: String?): DeleteOutcome = try { + DavResource(httpClient, href).delete(ifETag = eTag) { } + DeleteOutcome.Deleted + } catch (e: HttpException) { + when (e.code) { + NOT_FOUND, GONE -> DeleteOutcome.Deleted + PRECONDITION_FAILED -> if (exists(href)) { + DeleteOutcome.ServerNewer + } else { + DeleteOutcome.Deleted + } + else -> DeleteOutcome.Rejected(e.code, e.message.orEmpty()) + } + } catch (e: DavException) { + DeleteOutcome.Rejected(0, e.toString()) + } catch (e: IOException) { + DeleteOutcome.Failed(e.toString()) + } + + /** A HEAD, used only to tell "changed" from "gone" apart after a 412. */ + private fun exists(href: HttpUrl): Boolean = try { + DavResource(httpClient, href).head { } + true + } catch (e: HttpException) { + e.code != NOT_FOUND && e.code != GONE + } catch (_: DavException) { + true + } catch (_: IOException) { + // Unknown. Treat as still present: discarding a local edit is + // irreversible, and giving up on this resource until the next run is not. + true + } + + companion object { + const val COMPONENT = "VTODO" + + /** + * Resources per `calendar-multiget`. + * + * A whole collection in one REPORT is a request body and a response that + * a modest server will refuse outright, and a failure mid-way costs the + * entire batch. DAVx5 settled on the same order of magnitude. + */ + const val MULTIGET_BATCH = 30 + + private const val MIME_ICALENDAR = "text/calendar" + private const val ICALENDAR_VERSION = "2.0" + + private const val NOT_FOUND = 404 + private const val GONE = 410 + private const val PRECONDITION_FAILED = 412 + } +} diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarResources.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarResources.kt new file mode 100644 index 0000000..d95b60d --- /dev/null +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CalendarResources.kt @@ -0,0 +1,141 @@ +package de.jeanlucmakiola.caldav + +import at.bitfire.dav4jvm.property.GetETag +import okhttp3.HttpUrl + +/** + * An entity tag, plus the one bit of it that changes behaviour. + * + * ⚠️ A weak tag is not a slightly worse strong tag — it is unusable for what we + * need one for. RFC 9110 §13.1.1: *"A weak entity-tag cannot be used with + * If-Match."* So a weak tag must never be persisted as a validator; the resource + * is re-fetched instead. + * + * Weak tags are not exotic. On sabre they are the **default** path unless + * `Prefer: handling=strict` is sent, and any gzip-compressing nginx, Cloudflare + * or Traefik in front of the server weakens them regardless of the server + * software — which is why the calendar client asks for `Accept-Encoding: + * identity`. + */ +data class ETag(val value: String, val weak: Boolean) { + + /** Whether this tag may be persisted and later sent as `If-Match`. */ + val usable: Boolean get() = !weak && value.isNotBlank() + + override fun toString() = if (weak) "W/\"$value\"" else "\"$value\"" + + companion object { + /** + * The tag from a parsed `DAV:getetag` property. + * + * ⚠️ Not [parse]. `GetETag` has *already* stripped the `W/` marker and + * recorded it separately, so feeding its [GetETag.eTag] back through a + * parser reports every weak tag as strong — which is precisely the + * failure the weak flag exists to prevent. + */ + fun from(property: GetETag?): ETag? { + val value = property?.eTag?.takeIf { it.isNotBlank() } ?: return null + return ETag(value, property.weak == true) + } + + /** Parses a raw `ETag` **header**; null when absent. */ + fun parse(raw: String?): ETag? { + val trimmed = raw?.trim().orEmpty() + if (trimmed.isEmpty()) return null + val parsed = GetETag(trimmed) + val value = parsed.eTag?.takeIf { it.isNotBlank() } ?: return null + return ETag(value, parsed.weak == true) + } + } +} + +/** A resource as a listing reports it: an href and an ETag, with no body. */ +data class RemoteRef(val href: HttpUrl, val eTag: ETag?) + +/** + * A resource together with the body it was served with. + * + * ⚠️ [eTag] is the tag that arrived **in the same response as [iCalendar]**, and + * only such a pairing is ever stored. A tag taken from a listing and a body + * taken from a later fetch are not a matched pair, and treating them as one is + * how the Nextcloud shared-calendar landmine detonates: `CalendarObject::get()` + * hands back a body it has stripped while leaving the ETag alone, so an + * unmatched pair lets us `If-Match` a mutilated body over the real one. + */ +data class RemoteResource(val href: HttpUrl, val eTag: ETag?, val iCalendar: String) + +/** 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, + /** + * Answered without being asked for. + * + * Real servers do this, so responses are matched against the request rather + * than trusted, and the strays are counted rather than silently applied. + */ + val unsolicited: List, +) + +/** The outcome of a PUT. Every branch here is a different bug if merged with another. */ +sealed interface PutOutcome { + + /** Stored, with a strong ETag we can use as the next `If-Match`. */ + data class Stored(val href: HttpUrl, val eTag: ETag) : PutOutcome + + /** + * Stored, but the ETag was absent or weak. + * + * Not an error and not a conflict: the resource is on the server. It has to + * be re-fetched for a usable validator *and* for the server's canonical + * body, which may not be the bytes we sent. + */ + data class StoredNeedsRefetch(val href: HttpUrl) : PutOutcome + + /** 412 against `If-None-Match: *` — something already lives at this name. */ + data class NameTaken(val href: HttpUrl) : PutOutcome + + /** 412 against `If-Match`, and the resource is still there: the server is newer. */ + data object ServerNewer : PutOutcome + + /** + * 412 against `If-Match`, and the resource is gone. + * + * Spec-correct and not a conflict at all — a delete on the server racing an + * edit here. RFC 9110 requires 412 rather than 404 when the precondition is + * evaluated, so only a follow-up `HEAD` can tell the two apart. + */ + data object Vanished : PutOutcome + + /** + * The server refused this body, and sending it again will not help. + * + * Covers 4xx and 5xx alike: `507` MUST NOT be auto-retried (RFC 4918 §11.5) + * and a contradictory `RRULE`/`EXDATE` pair returns 500 from Nextcloud + * forever, so neither class earns a retry loop. + */ + data class Rejected(val code: Int, val message: String) : PutOutcome + + /** The request never got an answer. Transport, not judgement. */ + data class Failed(val reason: String) : PutOutcome +} + +/** The outcome of a DELETE. */ +sealed interface DeleteOutcome { + + /** + * Gone from the server. + * + * 404 and 410 count as success: the goal was for the resource not to exist, + * and it does not. + */ + data object Deleted : DeleteOutcome + + /** 412: the server's copy changed after we last saw it. */ + data object ServerNewer : DeleteOutcome + + data class Rejected(val code: Int, val message: String) : DeleteOutcome + + data class Failed(val reason: String) : DeleteOutcome +} diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CollectionClassifier.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CollectionClassifier.kt index 10e3705..7ab9783 100644 --- a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CollectionClassifier.kt +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/CollectionClassifier.kt @@ -2,7 +2,10 @@ package de.jeanlucmakiola.caldav import at.bitfire.dav4jvm.Property import at.bitfire.dav4jvm.Response +import at.bitfire.dav4jvm.property.CalendarColor import at.bitfire.dav4jvm.property.CurrentUserPrivilegeSet +import at.bitfire.dav4jvm.property.DisplayName +import at.bitfire.dav4jvm.property.MaxICalendarSize import at.bitfire.dav4jvm.property.ResourceType import at.bitfire.dav4jvm.property.SupportedCalendarComponentSet import at.bitfire.dav4jvm.property.SupportedReportSet @@ -38,6 +41,23 @@ data class TaskCollection( */ object CollectionClassifier { + /** + * The properties [classify] reads, requested **by name**. + * + * `allprop` legitimately omits every one of them (RFC 4791 §5.2.3 for the + * component set, RFC 3744 §3.7 for the privileges), so asking for them by + * name is not an optimisation — it is the only way to get them. + */ + val PROPERTIES = arrayOf( + ResourceType.NAME, + DisplayName.NAME, + CalendarColor.NAME, + SupportedCalendarComponentSet.NAME, + SupportedReportSet.NAME, + CurrentUserPrivilegeSet.NAME, + MaxICalendarSize.NAME, + ) + /** `CS:shared`, which Nextcloud adds alongside `caldav:calendar` on a share. */ private val SHARED = Property.Name("http://calendarserver.org/ns/", "shared") @@ -67,16 +87,16 @@ object CollectionClassifier { return TaskCollection( url = response.href, - displayName = response[at.bitfire.dav4jvm.property.DisplayName::class.java]?.displayName, + displayName = response[DisplayName::class.java]?.displayName, // CalendarColor.color is a packed ARGB Int. Calling toString() on it // yields "-65536" for red — an unparseable decimal, not a colour. - color = response[at.bitfire.dav4jvm.property.CalendarColor::class.java]?.color, + color = response[CalendarColor::class.java]?.color, readOnly = readOnly, isShared = SHARED in resourceType.types, // A hint, not a contract — Radicale advertised this for years without // implementing it, so the engine must still degrade gracefully. supportsSyncCollection = reports?.reports?.contains(SupportedReportSet.SYNC_COLLECTION) == true, - maxResourceSize = response[at.bitfire.dav4jvm.property.MaxICalendarSize::class.java]?.maxSize, + maxResourceSize = response[MaxICalendarSize::class.java]?.maxSize, ) } diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/RemoteCalendar.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/RemoteCalendar.kt new file mode 100644 index 0000000..f90ae12 --- /dev/null +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/RemoteCalendar.kt @@ -0,0 +1,33 @@ +package de.jeanlucmakiola.caldav + +import okhttp3.HttpUrl + +/** + * The remote side of one task collection, as the sync engine needs it. + * + * A seam, and a load-bearing one: the reconciliation above this interface is + * where local edits get discarded, tombstones get swept and conflicts get + * resolved, and none of that should need a server — or a network — to exercise. + * [CalendarCollection] is the only production implementation. + */ +interface RemoteCalendar { + + val url: HttpUrl + + fun state(): Result + + fun list(): Result> + + fun fetch(hrefs: List): Result + + fun create(name: String, iCalendar: String): PutOutcome + + /** + * Replaces [href]. A null [eTag] writes **unconditionally**, which is only + * ever correct when the server offers no usable validator at all — see + * [ETag]. + */ + fun update(href: HttpUrl, eTag: String?, iCalendar: String): PutOutcome + + fun delete(href: HttpUrl, eTag: String?): DeleteOutcome +} diff --git a/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ResourceNames.kt b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ResourceNames.kt new file mode 100644 index 0000000..11db21b --- /dev/null +++ b/caldav/src/main/kotlin/de/jeanlucmakiola/caldav/ResourceNames.kt @@ -0,0 +1,51 @@ +package de.jeanlucmakiola.caldav + +import java.util.UUID + +/** + * Filenames for calendar resources. + * + * ⚠️ **An href is not a UID.** A UID is opaque text chosen by whoever created + * the task — it may contain slashes, spaces, percent signs or nothing printable + * at all — while an href is a path segment on someone else's filesystem. Using + * one as the other is the classic CalDAV client bug: it works against the server + * you tested on and produces 400s, 403s or silently truncated names elsewhere. + * + * The character class is vdirsyncer's, and the exclusion of `@` is deliberate: + * plenty of UIDs are email-shaped, and `@` in a path segment is legal but + * routinely mishandled by proxies and by servers that re-parse their own URLs. + */ +object ResourceNames { + + /** + * Cap on the basename, in bytes. + * + * Well under the 255 that most filesystems allow, because the server appends + * to what we send — Nextcloud's trashbin renames a deleted resource to + * `-deleted.ics`, and a name that only just fit before deletion does + * not fit afterwards. + */ + const val MAX_BASENAME_BYTES = 200 + + const val EXTENSION = ".ics" + + private val UNSAFE = Regex("[^A-Za-z0-9_.+-]") + + /** + * A filename derived from [uid], or a random one when [uid] does not survive + * sanitisation. + * + * The derivation is a convenience for humans reading the collection over + * WebDAV, never an identity: nothing reads the UID back out of an href. + */ + fun forUid(uid: String): String { + val cleaned = UNSAFE.replace(uid, "-") + .trim('-', '.') + .take(MAX_BASENAME_BYTES) + .trimEnd('-', '.') + return if (cleaned.isEmpty() || cleaned.all { it == '-' }) random() else cleaned + EXTENSION + } + + /** A fresh name that cannot collide, for the fallback and the 412 retry. */ + fun random(): String = UUID.randomUUID().toString() + EXTENSION +} diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt new file mode 100644 index 0000000..4bccbbb --- /dev/null +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/CalendarCollectionTest.kt @@ -0,0 +1,314 @@ +package de.jeanlucmakiola.caldav + +import com.google.common.truth.Truth.assertThat +import okhttp3.HttpUrl +import okhttp3.OkHttpClient +import okhttp3.mockwebserver.MockResponse +import okhttp3.mockwebserver.MockWebServer +import org.junit.After +import org.junit.Before +import org.junit.Test + +class CalendarCollectionTest { + + private val server = MockWebServer() + private val httpClient = OkHttpClient.Builder().followRedirects(false).build() + private lateinit var collection: CalendarCollection + + @Before fun start() { + server.start() + collection = CalendarCollection(httpClient, server.url("/dav/tasks/")) + } + + @After fun stop() = server.shutdown() + + // ------------------------------------------------------------------ list + + @Test fun `list reads hrefs and strong etags`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/one.ics + "e1" + HTTP/1.1 200 OK + + + /dav/tasks/two.ics + W/"e2" + HTTP/1.1 200 OK + + """, + ), + ) + + val refs = collection.list().getOrThrow() + + assertThat(refs.map { it.href.encodedPath }) + .containsExactly("/dav/tasks/one.ics", "/dav/tasks/two.ics") + assertThat(refs[0].eTag).isEqualTo(ETag("e1", weak = false)) + assertThat(refs[1].eTag!!.usable).isFalse() + } + + @Test fun `list asks for no calendar-data and no time-range`() { + server.enqueue(multistatus("")) + collection.list().getOrThrow() + + val body = server.takeRequest().body.readUtf8() + // Bodies in a listing would download the whole collection every time, and + // would arrive without an ETag matched to them. + assertThat(body).doesNotContain("calendar-data") + // A VTODO without DTSTART or DUE is outside every range by construction, + // and some servers answer a ranged VTODO filter with nothing at all. + assertThat(body).doesNotContain("time-range") + assertThat(body).contains("VTODO") + } + + // ----------------------------------------------------------------- fetch + + @Test fun `fetch matches responses against the request`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/one.ics + + "e1" + BEGIN:VCALENDAR +END:VCALENDAR + HTTP/1.1 200 OK + + + /dav/tasks/somebody-elses.ics + + "e9" + BEGIN:VCALENDAR +END:VCALENDAR + HTTP/1.1 200 OK + + """, + ), + ) + + val result = collection.fetch(listOf(href("one.ics"), href("two.ics"))).getOrThrow() + + assertThat(result.resources.map { it.href.encodedPath }) + .containsExactly("/dav/tasks/one.ics") + // Real servers answer with URLs nobody asked about. Applying one of those + // to a row keyed by another href corrupts the wrong task. + assertThat(result.unsolicited.map { it.encodedPath }) + .containsExactly("/dav/tasks/somebody-elses.ics") + assertThat(result.missing.map { it.encodedPath }).containsExactly("/dav/tasks/two.ics") + } + + @Test fun `fetch batches`() { + repeat(2) { server.enqueue(multistatus("")) } + collection.fetch((1..3).map { href("$it.ics") }, batchSize = 2).getOrThrow() + assertThat(server.requestCount).isEqualTo(2) + } + + // ---------------------------------------------------------------- create + + @Test fun `create with a strong etag is stored`() { + server.enqueue(MockResponse().setResponseCode(201).setHeader("ETag", "\"new\"")) + + val outcome = collection.create("one.ics", "BEGIN:VCALENDAR\r\nEND:VCALENDAR\r\n") + + assertThat(outcome).isInstanceOf(PutOutcome.Stored::class.java) + assertThat((outcome as PutOutcome.Stored).eTag).isEqualTo(ETag("new", weak = false)) + assertThat(server.takeRequest().getHeader("If-None-Match")).isEqualTo("*") + } + + @Test fun `create with a weak etag needs a refetch`() { + server.enqueue(MockResponse().setResponseCode(201).setHeader("ETag", "W/\"new\"")) + // A weak tag is worse than none: storing it makes every later write look + // conditional while silently not being one. + assertThat(collection.create("one.ics", "x")) + .isInstanceOf(PutOutcome.StoredNeedsRefetch::class.java) + } + + @Test fun `create with no etag needs a refetch`() { + server.enqueue(MockResponse().setResponseCode(201)) + assertThat(collection.create("one.ics", "x")) + .isInstanceOf(PutOutcome.StoredNeedsRefetch::class.java) + } + + @Test fun `create answered 412 means the name is taken`() { + server.enqueue(MockResponse().setResponseCode(412)) + val outcome = collection.create("one.ics", "x") + assertThat(outcome).isInstanceOf(PutOutcome.NameTaken::class.java) + assertThat((outcome as PutOutcome.NameTaken).href.encodedPath) + .isEqualTo("/dav/tasks/one.ics") + } + + @Test fun `create answered 415 is rejected and not retried`() { + server.enqueue(MockResponse().setResponseCode(415)) + val outcome = collection.create("one.ics", "x") + assertThat(outcome).isInstanceOf(PutOutcome.Rejected::class.java) + assertThat((outcome as PutOutcome.Rejected).code).isEqualTo(415) + } + + // ---------------------------------------------------------------- update + + @Test fun `update answered 412 with the resource still there is a conflict`() { + server.enqueue(MockResponse().setResponseCode(412)) + server.enqueue(MockResponse().setResponseCode(200)) + + assertThat(collection.update(href("one.ics"), "old", "x")) + .isEqualTo(PutOutcome.ServerNewer) + assertThat(server.takeRequest().getHeader("If-Match")).isEqualTo("\"old\"") + assertThat(server.takeRequest().method).isEqualTo("HEAD") + } + + @Test fun `update answered 412 with the resource gone is not a conflict`() { + server.enqueue(MockResponse().setResponseCode(412)) + server.enqueue(MockResponse().setResponseCode(404)) + + // RFC 9110 §13.1.1 requires 412 once the precondition is evaluated, so a + // deleted resource and a changed one are the same status code. + assertThat(collection.update(href("one.ics"), "old", "x")) + .isEqualTo(PutOutcome.Vanished) + } + + @Test fun `update without an etag writes unconditionally`() { + server.enqueue(MockResponse().setResponseCode(204).setHeader("ETag", "\"new\"")) + collection.update(href("one.ics"), null, "x") + assertThat(server.takeRequest().getHeader("If-Match")).isNull() + } + + // ---------------------------------------------------------------- delete + + @Test fun `delete counts 404 and 410 as success`() { + server.enqueue(MockResponse().setResponseCode(404)) + assertThat(collection.delete(href("one.ics"), "e")).isEqualTo(DeleteOutcome.Deleted) + + server.enqueue(MockResponse().setResponseCode(410)) + assertThat(collection.delete(href("one.ics"), "e")).isEqualTo(DeleteOutcome.Deleted) + } + + @Test fun `delete sends If-Match and reports a conflict`() { + server.enqueue(MockResponse().setResponseCode(412)) + server.enqueue(MockResponse().setResponseCode(200)) + + assertThat(collection.delete(href("one.ics"), "old")) + .isEqualTo(DeleteOutcome.ServerNewer) + assertThat(server.takeRequest().getHeader("If-Match")).isEqualTo("\"old\"") + } + + @Test fun `delete answered 403 is rejected`() { + // Nextcloud's trashbin renames a deleted resource, so delete, recreate and + // delete again on the same href answers 403. + server.enqueue(MockResponse().setResponseCode(403)) + val outcome = collection.delete(href("one.ics"), null) + assertThat(outcome).isInstanceOf(DeleteOutcome.Rejected::class.java) + assertThat((outcome as DeleteOutcome.Rejected).code).isEqualTo(403) + } + + // --------------------------------------------------------------- headers + + @Test fun `writes carry Prefer strict and identity encoding`() { + val client = CalDavHttp.anonymous("Agendula test") + val withHeaders = CalendarCollection(client, server.url("/dav/tasks/")) + server.enqueue(MockResponse().setResponseCode(201).setHeader("ETag", "\"e\"")) + + withHeaders.create("one.ics", "x") + + val request = server.takeRequest() + // Without this, sabre runs vobject REPAIR over the body and then withholds + // the ETag because the stored bytes are no longer ours. + assertThat(request.getHeader("Prefer")).isEqualTo("handling=strict") + // Without this, a gzip-compressing proxy weakens every ETag it touches. + assertThat(request.getHeader("Accept-Encoding")).isEqualTo("identity") + } + + // ----------------------------------------------------------------- state + + @Test fun `state re-reads read-only and shared`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/ + + + + + + + + + + ct-1 + HTTP/1.1 200 OK + + """, + ), + ) + + val state = collection.state().getOrThrow() + + assertThat(state.collection.readOnly).isTrue() + assertThat(state.collection.isShared).isTrue() + assertThat(state.ctag).isEqualTo("ct-1") + } + + @Test fun `a member reported alongside self never becomes the collection state`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/member.ics + + + + + + + HTTP/1.1 200 OK + + + /dav/tasks/ + + + + + + mine + HTTP/1.1 200 OK + + """, + ), + ) + + // Some servers answer a Depth-0 PROPFIND with a member as well. Taking the + // first classifiable response would read that member's ACL as the + // collection's own — here, read-only when it is not. + val state = collection.state().getOrThrow() + assertThat(state.ctag).isEqualTo("mine") + assertThat(state.collection.readOnly).isFalse() + } + + @Test fun `state fails rather than guessing when the collection is no longer one`() { + server.enqueue( + multistatus( + """ + + /dav/tasks/ + + HTTP/1.1 200 OK + + """, + ), + ) + assertThat(collection.state().isFailure).isTrue() + } + + // ----------------------------------------------------------------- setup + + private fun href(name: String): HttpUrl = server.url("/dav/tasks/$name") + + private fun multistatus(responses: String) = MockResponse() + .setResponseCode(207) + .setHeader("Content-Type", "application/xml; charset=utf-8") + .setBody("$responses") +} diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ETagTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ETagTest.kt new file mode 100644 index 0000000..ff6e6ee --- /dev/null +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ETagTest.kt @@ -0,0 +1,45 @@ +package de.jeanlucmakiola.caldav + +import com.google.common.truth.Truth.assertThat +import org.junit.Test + +class ETagTest { + + @Test fun `strong tag is unquoted and usable`() { + val tag = ETag.parse("\"abc123\"") + assertThat(tag).isEqualTo(ETag("abc123", weak = false)) + assertThat(tag!!.usable).isTrue() + } + + @Test fun `weak tag keeps its value but is never usable`() { + val tag = ETag.parse("W/\"abc123\"") + assertThat(tag).isEqualTo(ETag("abc123", weak = true)) + // RFC 9110 §13.1.1 — a weak tag cannot be used with If-Match, so anything + // that treats this as a validator writes unconditionally without saying so. + assertThat(tag!!.usable).isFalse() + } + + @Test fun `unquoted tag is accepted`() { + assertThat(ETag.parse("abc123")).isEqualTo(ETag("abc123", weak = false)) + } + + @Test fun `absent and empty are both null`() { + assertThat(ETag.parse(null)).isNull() + assertThat(ETag.parse("")).isNull() + assertThat(ETag.parse(" ")).isNull() + assertThat(ETag.parse("\"\"")).isNull() + } + + @Test fun `a parsed property is not re-parsed`() { + // GetETag has already stripped the marker and recorded it separately, so + // running its value back through parse() reports every weak tag as strong. + val property = at.bitfire.dav4jvm.property.GetETag("W/\"abc\"") + assertThat(ETag.from(property)).isEqualTo(ETag("abc", weak = true)) + assertThat(ETag.parse(property.eTag)).isEqualTo(ETag("abc", weak = false)) + } + + @Test fun `a value of W is not a weak marker`() { + // "W/" needs something after it; the guard is length, not prefix alone. + assertThat(ETag.parse("\"W/\"")).isEqualTo(ETag("W/", weak = false)) + } +} diff --git a/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ResourceNamesTest.kt b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ResourceNamesTest.kt new file mode 100644 index 0000000..e918965 --- /dev/null +++ b/caldav/src/test/kotlin/de/jeanlucmakiola/caldav/ResourceNamesTest.kt @@ -0,0 +1,41 @@ +package de.jeanlucmakiola.caldav + +import com.google.common.truth.Truth.assertThat +import org.junit.Test + +class ResourceNamesTest { + + @Test fun `an ordinary uid becomes itself`() { + assertThat(ResourceNames.forUid("a1b2c3")).isEqualTo("a1b2c3.ics") + } + + @Test fun `path separators never survive`() { + // The whole point: a UID is opaque text, an href is a path segment. + assertThat(ResourceNames.forUid("../../etc/passwd")).doesNotContain("/") + assertThat(ResourceNames.forUid("a/b")).isEqualTo("a-b.ics") + } + + @Test fun `at signs are excluded even though they are legal in a path`() { + assertThat(ResourceNames.forUid("task@example.com")).isEqualTo("task-example.com.ics") + } + + @Test fun `the basename is capped`() { + val name = ResourceNames.forUid("x".repeat(500)) + assertThat(name.removeSuffix(".ics")).hasLength(ResourceNames.MAX_BASENAME_BYTES) + } + + @Test fun `a uid with nothing usable in it falls back to a uuid`() { + val name = ResourceNames.forUid("///") + assertThat(name).endsWith(".ics") + assertThat(name.removeSuffix(".ics")).matches("[0-9a-f-]{36}") + } + + @Test fun `dots alone do not become a relative path`() { + assertThat(ResourceNames.forUid("..")).doesNotContain("..") + assertThat(ResourceNames.forUid(".")).endsWith(".ics") + } + + @Test fun `random names do not repeat`() { + assertThat(ResourceNames.random()).isNotEqualTo(ResourceNames.random()) + } +} diff --git a/docs/SYNC-PLAN.md b/docs/SYNC-PLAN.md index d7dc8da..33d8fac 100644 --- a/docs/SYNC-PLAN.md +++ b/docs/SYNC-PLAN.md @@ -445,6 +445,102 @@ the columns already exist in the v1 schema. Every downstream insert writes a concurrent edit resolves server-wins with a visible report; a quarantined resource does not stop its collection. +### Settled while building it + +**⚠️ The mapper emitted `TZID` with nothing to resolve it.** Chunk 1 writes +`DTSTART;TZID=Europe/Berlin:…`, and RFC 5545 §3.2.19 requires a `TZID` without a +leading solidus to reference a `VTIMEZONE` **in the same object**. Nothing +generated one — `VTIMEZONE` sits at `VCALENDAR` level, outside the `VTODO` whose +residue the mapper stores, so it was outside the round-trip guarantee entirely. +Left alone, every zoned task would have been malformed, and `Prefer: +handling=strict` — which chunk 3 sends precisely so servers stop repairing our +bytes — turns malformed into rejected. `VTimeZones` now generates the definition +from `java.time`'s own rules, which also keeps the zone we *write* in agreement +with the zone we compute occurrences in. **Cost accepted: only the currently +effective rules are emitted, not the historical transition table.** Tasks are +dated now or later. + +**java.time does not encode "the last Sunday" as `-1`.** It arrives as +`dayOfMonthIndicator = 25` — "the first Sunday on or after the 25th" — and only +the fact that a seven-day window ending on the last day of the month *is* the +last occurrence identifies it. The obvious `-1` test never fires for any European +zone, and the fallback emits a seven-value `BYMONTHDAY` list where every other +client writes `BYDAY=-1SU`. + +**`GetETag.eTag` has already been parsed.** dav4jvm strips the `W/` marker and +records it in a separate field, so feeding its value back through an ETag parser +reports **every weak tag as strong** — the exact failure the weak flag exists to +prevent. `ETag.from(property)` and `ETag.parse(header)` are now different +functions for that reason, and a test pins the difference. + +**The discard is reported when the replacement arrives, not when the 412 does.** +Announcing it at the conflict — and clearing `is_dirty` there — makes the loss +real if the replacement download then fails, with nothing left to retry. The row +now stays dirty until the download overwrites it. + +**A `SyncStore` seam, not Robolectric.** The reconciliation is where local edits +are discarded, tombstones swept and conflicts resolved; proving that needs a test +double for eight methods, not an in-memory Room plus a new test runtime. + +**The mutilation guard is scoped to what is actually reachable.** Nextcloud +reduces `CLASS:CONFIDENTIAL` objects served from a *share* while leaving the ETag +intact; `VALARM` stripping happens on read-only shares, which are never written +to anyway. So the refusal is: shared collection + confidential + already on the +server. The collection's `read-only` and `shared` flags are re-read on every sync +rather than trusted from account-add, because ACL churn is silent. + +**Manual trigger.** `ContentResolver.requestSync` is gated behind +`hasAuthorityAccess()` at our targetSdk and returns silently when it refuses, so +the in-app button enqueues the WorkManager job directly via `SyncTrigger`. The +sync adapter uses the same path. + +### Found by the review, and worth naming + +**⚠️ An empty listing would have hard-deleted the whole list.** The sweep took +"absent from `remote.list()`" as proof of a server-side delete, and its evidence +is a `calendar-query` with a `VTODO` comp-filter — the filter whose mishandling +is the reason the query carries no time-range in the first place. A server +answering it with an empty *successful* multistatus is indistinguishable from an +empty collection, and one such answer destroyed every task in that list in one +pass. **An empty listing now never sweeps.** Cost accepted: a collection +genuinely emptied on the server keeps its local rows until one task reappears +there. That is recoverable by hand; the other error is not. + +**Quarantine did not cover creates.** Failures on a resource with no href yet +were counted under its UID and read back under its href, so nothing ever read +them: a new task the server rejects permanently was re-PUT on every sync forever +— the exact DAVx5 failure the counter exists to prevent — while the unread keys +grew in DataStore without bound. Resources are now keyed by href *or* `uid:…`, +and a success clears both. + +**A `RELATED-TO` deleted on the server never unparented anything.** Only present +parents were recorded, and `upsert` carried the old `parent_id` forward — which +the writer then resolved straight back into a `RELATED-TO`, re-uploading the link +the user had deleted elsewhere. + +**Two validators disagreed about what a DATE is.** `ResourceValidator` tested +only `VALUE=DATE` while the mapper also reads a bare eight-digit value as one, so +a residue `DTSTART:20260101` beside an authored `DUE;VALUE=DATE:20260102` was +rejected locally and never left the device — a case `VTodoMapperTest` already had +a test for. The per-component rules now delegate to `VTodoMapper.validate`, which +also restores the `TRIGGER;RELATED=END` check that the new validator had silently +dropped and left as dead code. + +**An unresolvable `TZID` produced a resource with no definition for it.** A +Windows zone name from another client survives in the residue, and `java.time` +cannot regenerate a `VTIMEZONE` for it — so with `Prefer: handling=strict` the +upload becomes an unexplained rejection. It is now refused by name. +**Open for chunk 5:** preserving the server's own `VCALENDAR`-level `VTIMEZONE` +would fix it properly, and needs somewhere to put residue that is not the +`VTODO`'s. + +**Three narrower ones, all fixed:** quarantine counts were a global +read-modify-write, so two accounts syncing at once discarded each other's +(now merged inside `edit`); `NeedsSignIn` and `Misconfigured` never called +`recordSync`, leaving the accounts screen reporting "synced 5 minutes ago" for an +account that cannot sync at all; and `apply()` re-read every row in the list once +per downloaded resource. + --- ## Chunk 4 — incremental sync and scheduling