diff --git a/migrations/20260830000001_webdav_dead_properties_resource_id_rekey.sql b/migrations/20260830000001_webdav_dead_properties_resource_id_rekey.sql new file mode 100644 index 00000000..f5917ed4 --- /dev/null +++ b/migrations/20260830000001_webdav_dead_properties_resource_id_rekey.sql @@ -0,0 +1,112 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- WebDAV dead properties: rekey from (resource_path, user_id) to resource id +-- ════════════════════════════════════════════════════════════════════════════ +-- The original schema (20260825000000) keyed dead properties on +-- `(resource_path, user_id, namespace, local_name)`. That model was wrong on +-- two counts: +-- +-- 1. Dead properties are RESOURCE state per RFC 4918 §4.2 — not user +-- state. Two users on a shared drive PROPFIND'ing the same resource +-- must see the same dead-properties. The user_id key siloed them. +-- 2. Every non-WebDAV delete path (REST `DELETE /api/files/{id}`, bulk +-- delete, trash empty, folder cascade) operates on a resource id — +-- not a path. None of those code paths could cheaply call +-- `remove_resource(path, user_id)`, so they leaked dead-property +-- tombstones. WebDAV DELETE itself had a workaround explicit-cleanup +-- call, but the REST surface (which the SvelteKit web UI uses) is the +-- dominant delete path in practice. +-- +-- This migration switches the key to a polymorphic resource reference: +-- exactly one of `folder_id` / `file_id` is set, each with `ON DELETE +-- CASCADE` to its owning table. After this lands every existing +-- delete code path — REST, WebDAV, NextCloud DAV, trash, folder +-- cascade — automatically reaps dead-property rows when the underlying +-- file or folder is removed, with no service-layer changes. +-- +-- MOVE / RENAME also become no-ops at the dead-properties layer: a +-- folder's id is stable across renames, so its dead properties move +-- with it for free. The `rename_resource()` method on the store is +-- removed in the matching Rust change. +-- +-- ── Migration shape ───────────────────────────────────────────────────────── +-- 1. ADD COLUMN folder_id / file_id (NULL-able for now). +-- 2. Backfill folder_id from any row whose resource_path matches a +-- folder row's `path` + `user_id`. +-- 3. Backfill file_id for the rest by joining through the parent folder +-- and matching `parent.path || '/' || fi.name`. +-- 4. Reap rows that didn't resolve — they're tombstones from before +-- the FK-cascade fix, and there's no resource left to attach them to. +-- 5. Add the CHECK constraint that exactly one column is set. +-- 6. Add two partial unique indexes (one per kind). +-- 7. DROP the old columns; PG drops the inline UNIQUE constraint and +-- the explicit path/user index along with them. +-- +-- The migration runs in a single sqlx transaction. If any step fails +-- the schema rolls back to (20260825000000) intact. + +ALTER TABLE storage.webdav_dead_properties + ADD COLUMN folder_id UUID NULL REFERENCES storage.folders(id) ON DELETE CASCADE, + ADD COLUMN file_id UUID NULL REFERENCES storage.files(id) ON DELETE CASCADE; + +-- Backfill: every row whose resource_path matches an existing folder +-- row's `path` + `user_id` gets its folder_id stamped. `NOT is_trashed` +-- mirrors what the handler does at lookup time — trashed rows can't be +-- the live target of a PROPPATCH anyway, so any old row pointing at a +-- trashed folder is a tombstone (handled in step 4). +UPDATE storage.webdav_dead_properties d + SET folder_id = fo.id + FROM storage.folders fo + WHERE fo.path = d.resource_path + AND fo.user_id = d.user_id + AND NOT fo.is_trashed; + +-- Backfill: any remaining row must be a file's properties. Match the +-- same path-computation the resolver uses for files — +-- `parent.path || '/' || fi.name` — so the rewrite mirrors the +-- handler's runtime behaviour exactly. +UPDATE storage.webdav_dead_properties d + SET file_id = fi.id + FROM storage.files fi + JOIN storage.folders parent ON parent.id = fi.folder_id + WHERE d.folder_id IS NULL + AND fi.user_id = d.user_id + AND NOT fi.is_trashed + AND parent.path || '/' || fi.name = d.resource_path; + +-- Reap orphans. A row that didn't resolve to a folder or file is a +-- tombstone left by some pre-fix delete path: the resource is long +-- gone but the dead-property row was never reaped because the old +-- `(path, user_id)` key kept it disconnected from the resource's +-- lifecycle. The FK-cascade era makes this category structurally +-- impossible, so dropping them on migration is the right cleanup. +DELETE FROM storage.webdav_dead_properties + WHERE folder_id IS NULL AND file_id IS NULL; + +-- Exactly-one-is-set: defends against future code accidentally +-- writing both columns or neither. `<>` between two boolean +-- IS NULL probes is the idiomatic PG shape for XOR. +ALTER TABLE storage.webdav_dead_properties + ADD CONSTRAINT webdav_dead_properties_one_resource_chk + CHECK ((folder_id IS NULL) <> (file_id IS NULL)); + +-- Partial unique indexes — one per resource kind. PG's ON CONFLICT +-- can infer either via `(folder_id, namespace, local_name) +-- WHERE folder_id IS NOT NULL`, matching the partial index, so +-- upsert continues to work without quirky ON CONSTRAINT plumbing. +CREATE UNIQUE INDEX IF NOT EXISTS idx_webdav_dead_props_folder_unique + ON storage.webdav_dead_properties (folder_id, namespace, local_name) + WHERE folder_id IS NOT NULL; + +CREATE UNIQUE INDEX IF NOT EXISTS idx_webdav_dead_props_file_unique + ON storage.webdav_dead_properties (file_id, namespace, local_name) + WHERE file_id IS NOT NULL; + +-- Drop the old key columns. PG cascades the auto-named inline UNIQUE +-- constraint and the explicit `(resource_path, user_id)` lookup index +-- along with the columns (idx is on resource_path which is going away, +-- so CASCADE is required). +DROP INDEX IF EXISTS storage.idx_webdav_dead_properties_path_user; + +ALTER TABLE storage.webdav_dead_properties + DROP COLUMN resource_path CASCADE, + DROP COLUMN user_id CASCADE; diff --git a/src/infrastructure/services/webdav_dead_property_store.rs b/src/infrastructure/services/webdav_dead_property_store.rs index 50e1347f..50ed1201 100644 --- a/src/infrastructure/services/webdav_dead_property_store.rs +++ b/src/infrastructure/services/webdav_dead_property_store.rs @@ -4,13 +4,35 @@ //! server without interpreting their value. Properties are persisted to //! `storage.webdav_dead_properties` and survive server restarts. //! -//! Queries here use `sqlx::query()` (runtime-bound) rather than the -//! compile-time-checked `sqlx::query!()` macro. The macro would require either -//! a live DB at compile time OR committed `.sqlx/` offline metadata; the rest -//! of this codebase consistently uses the runtime variant (see -//! `user_pg_repository.rs` for the canonical style), so a fresh checkout -//! compiles without any DB connection. Trading the macro's compile-time column -//! check for that bootstrap-friendliness is the project's standing convention. +//! Keying contract (after migration 20260830000001): the row is keyed by +//! the underlying resource id — exactly one of `folder_id` / `file_id` is +//! set — not by the resource's current path. Three consequences: +//! +//! * Every delete code path (REST, WebDAV, NextCloud DAV, trash empty, +//! folder cascade) reaps dead-property rows for free via FK +//! `ON DELETE CASCADE`. The store has no `remove_resource()` method +//! because it isn't needed: deleting the file/folder row reaps the +//! attached dead properties as a database invariant. +//! * MOVE / RENAME never changes the resource id, so dead properties +//! follow the resource without any store-side bookkeeping. The store +//! has no `rename_resource()` method for the same reason. +//! * Dead properties are RESOURCE state (RFC 4918 §4.2), not user +//! state. Two users on a shared drive PROPFIND'ing the same resource +//! see the same dead properties. The `user_id` scope key from the +//! pre-rekey schema is gone; user-delete cleanup happens +//! transitively through `auth.users` → `storage.{folders,files}` → +//! this table. +//! +//! Queries use `sqlx::query()` (runtime-bound) rather than `sqlx::query!()` +//! to keep fresh checkouts compilable without a DB connection — the +//! codebase's standing convention. +//! +//! COPY semantics (RFC 4918 §8.8 — dead properties MUST be duplicated) +//! are NOT handled here. The COPY handler is responsible for explicitly +//! reading the source's dead properties via `get_all()` and writing them +//! against the new resource id via `set()`. Not done in this migration — +//! it was not handled by the path-based store either, so this is a +//! parity decision, not a regression. use std::sync::Arc; @@ -20,6 +42,19 @@ use uuid::Uuid; use crate::application::adapters::webdav_adapter::QualifiedName; use crate::domain::errors::DomainError; +/// Polymorphic reference to the resource a dead property hangs off. +/// +/// Exactly one variant — folder or file — is ever stored in a single +/// row. The CHECK constraint +/// `(folder_id IS NULL) <> (file_id IS NULL)` enforces this at the +/// database level so the application layer cannot accidentally write a +/// row that's both or neither. +#[derive(Clone, Copy, Debug)] +pub enum ResourceRef { + Folder(Uuid), + File(Uuid), +} + pub struct DeadPropertyStore { pool: Arc, } @@ -30,47 +65,78 @@ impl DeadPropertyStore { } /// Upsert a dead property. `value = None` means an empty XML element. + /// + /// The two SQL branches are deliberately kept separate so each + /// ON CONFLICT clause can target the matching partial unique + /// index (`idx_webdav_dead_props_folder_unique` / + /// `idx_webdav_dead_props_file_unique`). A combined upsert would + /// require a non-partial unique index that treats NULL as + /// distinct, which doesn't match the (folder XOR file) shape. pub async fn set( &self, - path: &str, - user_id: Uuid, + r: ResourceRef, name: QualifiedName, value: Option, ) -> Result<(), DomainError> { - sqlx::query( - r#" - INSERT INTO storage.webdav_dead_properties - (resource_path, user_id, namespace, local_name, value) - VALUES ($1, $2, $3, $4, $5) - ON CONFLICT (resource_path, user_id, namespace, local_name) - DO UPDATE SET value = EXCLUDED.value, updated_at = CURRENT_TIMESTAMP - "#, - ) - .bind(path) - .bind(user_id) - .bind(&name.namespace) - .bind(&name.name) - .bind(&value) - .execute(&*self.pool) - .await - .map_err(|e| DomainError::internal_error("DeadPropertyStore", format!("set: {e}")))?; + match r { + ResourceRef::Folder(folder_id) => { + sqlx::query( + r#" + INSERT INTO storage.webdav_dead_properties + (folder_id, namespace, local_name, value) + VALUES ($1, $2, $3, $4) + ON CONFLICT (folder_id, namespace, local_name) + WHERE folder_id IS NOT NULL + DO UPDATE SET value = EXCLUDED.value, updated_at = CURRENT_TIMESTAMP + "#, + ) + .bind(folder_id) + .bind(&name.namespace) + .bind(&name.name) + .bind(&value) + .execute(&*self.pool) + .await + .map_err(|e| { + DomainError::internal_error("DeadPropertyStore", format!("set folder: {e}")) + })?; + } + ResourceRef::File(file_id) => { + sqlx::query( + r#" + INSERT INTO storage.webdav_dead_properties + (file_id, namespace, local_name, value) + VALUES ($1, $2, $3, $4) + ON CONFLICT (file_id, namespace, local_name) + WHERE file_id IS NOT NULL + DO UPDATE SET value = EXCLUDED.value, updated_at = CURRENT_TIMESTAMP + "#, + ) + .bind(file_id) + .bind(&name.namespace) + .bind(&name.name) + .bind(&value) + .execute(&*self.pool) + .await + .map_err(|e| { + DomainError::internal_error("DeadPropertyStore", format!("set file: {e}")) + })?; + } + } Ok(()) } /// Delete a specific dead property. No-op if not present. - pub async fn remove( - &self, - path: &str, - user_id: Uuid, - name: &QualifiedName, - ) -> Result<(), DomainError> { + pub async fn remove(&self, r: ResourceRef, name: &QualifiedName) -> Result<(), DomainError> { + let (folder_id, file_id) = split_ref(r); sqlx::query( "DELETE FROM storage.webdav_dead_properties - WHERE resource_path = $1 AND user_id = $2 - AND namespace = $3 AND local_name = $4", + WHERE folder_id IS NOT DISTINCT FROM $1 + AND file_id IS NOT DISTINCT FROM $2 + AND namespace = $3 + AND local_name = $4", ) - .bind(path) - .bind(user_id) + .bind(folder_id) + .bind(file_id) .bind(&name.namespace) .bind(&name.name) .execute(&*self.pool) @@ -79,19 +145,20 @@ impl DeadPropertyStore { Ok(()) } - /// Return all dead properties for `path`. + /// Return all dead properties for the given resource. pub async fn get_all( &self, - path: &str, - user_id: Uuid, + r: ResourceRef, ) -> Result)>, DomainError> { + let (folder_id, file_id) = split_ref(r); let rows = sqlx::query( "SELECT namespace, local_name, value - FROM storage.webdav_dead_properties - WHERE resource_path = $1 AND user_id = $2", + FROM storage.webdav_dead_properties + WHERE folder_id IS NOT DISTINCT FROM $1 + AND file_id IS NOT DISTINCT FROM $2", ) - .bind(path) - .bind(user_id) + .bind(folder_id) + .bind(file_id) .fetch_all(&*self.pool) .await .map_err(|e| DomainError::internal_error("DeadPropertyStore", format!("get_all: {e}")))?; @@ -111,17 +178,19 @@ impl DeadPropertyStore { /// Returns `Some(None)` when the property exists with an empty value. pub async fn get( &self, - path: &str, - user_id: Uuid, + r: ResourceRef, name: &QualifiedName, ) -> Result>, DomainError> { + let (folder_id, file_id) = split_ref(r); let row = sqlx::query( "SELECT value FROM storage.webdav_dead_properties - WHERE resource_path = $1 AND user_id = $2 - AND namespace = $3 AND local_name = $4", + WHERE folder_id IS NOT DISTINCT FROM $1 + AND file_id IS NOT DISTINCT FROM $2 + AND namespace = $3 + AND local_name = $4", ) - .bind(path) - .bind(user_id) + .bind(folder_id) + .bind(file_id) .bind(&name.namespace) .bind(&name.name) .fetch_optional(&*self.pool) @@ -130,65 +199,15 @@ impl DeadPropertyStore { Ok(row.map(|r| r.get::, _>("value"))) } +} - /// Delete all dead properties for `path` (called on DELETE). - pub async fn remove_resource(&self, path: &str, user_id: Uuid) -> Result<(), DomainError> { - sqlx::query( - "DELETE FROM storage.webdav_dead_properties - WHERE resource_path = $1 AND user_id = $2", - ) - .bind(path) - .bind(user_id) - .execute(&*self.pool) - .await - .map_err(|e| { - DomainError::internal_error("DeadPropertyStore", format!("remove_resource: {e}")) - })?; - Ok(()) - } - - /// Move dead properties from `old_path` to `new_path` (called on MOVE). - /// Clears any stale properties at `new_path` first. - pub async fn rename_resource( - &self, - old_path: &str, - user_id: Uuid, - new_path: &str, - ) -> Result<(), DomainError> { - let mut tx = self.pool.begin().await.map_err(|e| { - DomainError::internal_error("DeadPropertyStore", format!("rename_resource tx: {e}")) - })?; - - sqlx::query( - "DELETE FROM storage.webdav_dead_properties - WHERE resource_path = $1 AND user_id = $2", - ) - .bind(new_path) - .bind(user_id) - .execute(&mut *tx) - .await - .map_err(|e| { - DomainError::internal_error("DeadPropertyStore", format!("rename_resource delete: {e}")) - })?; - - sqlx::query( - "UPDATE storage.webdav_dead_properties - SET resource_path = $2 - WHERE resource_path = $1 AND user_id = $3", - ) - .bind(old_path) - .bind(new_path) - .bind(user_id) - .execute(&mut *tx) - .await - .map_err(|e| { - DomainError::internal_error("DeadPropertyStore", format!("rename_resource update: {e}")) - })?; - - tx.commit().await.map_err(|e| { - DomainError::internal_error("DeadPropertyStore", format!("rename_resource commit: {e}")) - })?; - Ok(()) +/// Splits a `ResourceRef` into `(folder_id, file_id)` Option pairs for +/// binding into SQL. The unused slot is `None` so `IS NOT DISTINCT FROM` +/// matches the NULL stored in the unused column. +fn split_ref(r: ResourceRef) -> (Option, Option) { + match r { + ResourceRef::Folder(id) => (Some(id), None), + ResourceRef::File(id) => (None, Some(id)), } } diff --git a/src/interfaces/api/handlers/webdav_handler.rs b/src/interfaces/api/handlers/webdav_handler.rs index 6f223477..b612ffe0 100644 --- a/src/interfaces/api/handlers/webdav_handler.rs +++ b/src/interfaces/api/handlers/webdav_handler.rs @@ -31,6 +31,7 @@ use crate::application::services::folder_service::FolderService; use crate::common::di::AppState; use crate::domain::repositories::drive_repository::DriveRepository; use crate::infrastructure::services::path_resolver_service::ResolvedResource; +use crate::infrastructure::services::webdav_dead_property_store::{DeadPropertyStore, ResourceRef}; use crate::interfaces::errors::AppError; use crate::interfaces::middleware::auth::{AuthUser, CurrentUser}; use crate::interfaces::range_requests::{not_modified_response, range_response}; @@ -462,7 +463,6 @@ async fn handle_propfind( file_retrieval_service, user.id, state.webdav_dead_props.clone(), - path.clone(), ) .await; } @@ -482,16 +482,11 @@ async fn handle_propfind( file_retrieval_service, user.id, state.webdav_dead_props.clone(), - path.clone(), ) .await; } Ok(ResolvedResource::File(file)) => { - let dead_props = state - .webdav_dead_props - .get_all(&path, user.id) - .await - .unwrap_or_default(); + let dead_props = file_dead_props(&state, &file).await; let file_href = webdav_href(&client_path); let mut buf = Vec::with_capacity(1024); { @@ -535,7 +530,6 @@ async fn handle_propfind( file_retrieval_service, user.id, state.webdav_dead_props.clone(), - path.clone(), ) .await; } @@ -544,11 +538,7 @@ async fn handle_propfind( .await { assert_owner(file.owner_id.as_deref(), &user.id.to_string(), &path)?; - let dead_props = state - .webdav_dead_props - .get_all(&path, user.id) - .await - .unwrap_or_default(); + let dead_props = file_dead_props(&state, &file).await; let file_href = webdav_href(&client_path); let mut buf = Vec::with_capacity(1024); { @@ -593,10 +583,7 @@ async fn build_streaming_propfind_response( folder_service: std::sync::Arc, file_retrieval_service: std::sync::Arc, user_id: Uuid, - dead_props_store: Arc< - crate::infrastructure::services::webdav_dead_property_store::DeadPropertyStore, - >, - folder_internal_path: String, + dead_props_store: Arc, ) -> Result, AppError> { let depth = depth.to_string(); let base_href = base_href.to_string(); @@ -604,11 +591,17 @@ async fn build_streaming_propfind_response( let stream = async_stream::try_stream! { // ── XML header + + folder entry ────────── + // + // Dead-property lookups key on the resource's stable id, so we + // pass each FolderDto / FileDto to a small helper that parses + // its `id` field into a `ResourceRef` and queries the store. + // The synthetic root folder (id = "root") fails to parse and + // the helper returns an empty list — correct, since the root + // has no DB row to anchor properties on. + let folder_dead = folder_dead_props(&dead_props_store, &folder).await; let mut buf = Vec::with_capacity(4096); { let mut w = Writer::new(&mut buf); - let folder_dead = dead_props_store.get_all(&folder_internal_path, user_id).await - .map_err(|e| std::io::Error::other(e.to_string()))?; WebDavAdapter::write_multistatus_start(&mut w) .map_err(|e| std::io::Error::other(e.to_string()))?; WebDavAdapter::write_folder_entry_with_dead_props(&mut w, &folder, &propfind_request, &base_href, &folder_dead) @@ -640,15 +633,21 @@ async fn build_streaming_propfind_response( break; } + // Materialise dead-props for the whole page before + // we start writing — keeps the borrow checker happy + // (the writer borrows the FolderDto and the dead-props + // vec for the duration of write_folder_entry_*). + let mut subfolder_deads = Vec::with_capacity(result.items.len()); + for subfolder in &result.items { + subfolder_deads.push(folder_dead_props(&dead_props_store, subfolder).await); + } + let mut chunk = Vec::with_capacity(result.items.len() * 800); { let mut w = Writer::new(&mut chunk); - for subfolder in &result.items { + for (subfolder, child_dead) in result.items.iter().zip(subfolder_deads.iter()) { let href = format!("{}{}/", base_href, encode_path_segment(&subfolder.name)); - let child_path = format!("{}/{}", folder_internal_path, subfolder.name); - let child_dead = dead_props_store.get_all(&child_path, user_id).await - .map_err(|e| std::io::Error::other(e.to_string()))?; - WebDavAdapter::write_folder_entry_with_dead_props(&mut w, subfolder, &propfind_request, &href, &child_dead) + WebDavAdapter::write_folder_entry_with_dead_props(&mut w, subfolder, &propfind_request, &href, child_dead) .map_err(|e| std::io::Error::other(e.to_string()))?; } } @@ -674,15 +673,17 @@ async fn build_streaming_propfind_response( } let batch_len = batch.len(); + let mut file_deads = Vec::with_capacity(batch_len); + for file in &batch { + file_deads.push(streamed_file_dead_props(&dead_props_store, file).await); + } + let mut chunk = Vec::with_capacity(batch_len * 800); { let mut w = Writer::new(&mut chunk); - for file in &batch { + for (file, child_dead) in batch.iter().zip(file_deads.iter()) { let href = format!("{}{}", base_href, encode_path_segment(&file.name)); - let child_path = format!("{}/{}", folder_internal_path, file.name); - let child_dead = dead_props_store.get_all(&child_path, user_id).await - .map_err(|e| std::io::Error::other(e.to_string()))?; - WebDavAdapter::write_file_entry_with_dead_props(&mut w, file, &propfind_request, &href, &child_dead) + WebDavAdapter::write_file_entry_with_dead_props(&mut w, file, &propfind_request, &href, child_dead) .map_err(|e| std::io::Error::other(e.to_string()))?; } } @@ -752,29 +753,45 @@ async fn handle_proppatch( return Ok(resp); } - // Resolve the target resource type BEFORE consuming the body so - // we can pick the correct href shape in the multi-status - // response. RFC 4918 §5.2 + strict WebDAV-client parser rules - // require a trailing `/` for collection hrefs; emitting - // `/webdav/foo` for a folder breaks NC-desktop / Cyberduck / - // other multi-status consumers the same way the NC PROPFIND - // bug did. An empty / `/` path is the root, always a - // collection. A path that resolves to neither file nor folder - // (e.g. PROPPATCH on a resource that doesn't exist) defaults - // to non-collection — matches the request-line shape the - // client used, since collection paths conventionally arrive - // with trailing `/` already trimmed by routing. - let is_collection = if path.is_empty() || path == "/" { - true + // Resolve the target resource BEFORE consuming the body. We need + // the resolved kind for two reasons: + // + // 1. The store key is the resource id (folder_id XOR file_id) + // after migration 20260830000001; we need to know which one + // to bind into `ResourceRef`. + // 2. The href shape in the multi-status response differs for + // collections vs leaves — RFC 4918 §5.2 + strict WebDAV- + // client parser rules require a trailing `/` for collection + // hrefs, and emitting `/webdav/foo` for a folder breaks + // NC-desktop / Cyberduck / other multi-status consumers. + // + // PROPPATCH on a non-existent resource returns 404. This is a + // tighter contract than the pre-rekey code, which silently + // wrote a dead-prop row keyed by the ghost path — that was a + // foot-gun, not a feature. + let (resource_ref, is_collection) = if path.is_empty() || path == "/" { + // The synthetic root has no DB row to anchor properties on. + // Treat it as a collection for href shaping; reject the + // PROPPATCH itself below so we don't fabricate a target. + (None, true) } else { - let drive_id = resolve_drive_id_for_native_webdav(&state, user.id).await?; - state - .applications - .folder_service - .get_folder_by_path(&path, drive_id) - .await - .is_ok() + match resolve_or_legacy(&state, &path, user.id).await { + Some(ResolvedResource::Folder(folder)) => { + let id = Uuid::parse_str(&folder.id).map_err(|e| { + AppError::internal_error(format!("Folder id is not a UUID: {e}")) + })?; + (Some(ResourceRef::Folder(id)), true) + } + Some(ResolvedResource::File(file)) => { + let id = Uuid::parse_str(&file.id) + .map_err(|e| AppError::internal_error(format!("File id is not a UUID: {e}")))?; + (Some(ResourceRef::File(id)), false) + } + None => return Err(AppError::not_found(format!("Resource not found: {}", path))), + } }; + let resource_ref = resource_ref + .ok_or_else(|| AppError::forbidden("PROPPATCH on the WebDAV root is not supported"))?; // Read request body (XML — bounded to 1 MB) let body_bytes = body::to_bytes(req.into_body(), MAX_XML_BODY) @@ -792,7 +809,7 @@ async fn handle_proppatch( match op { PropPatchOp::Set(pv) => { dead_props - .set(&path, user.id, pv.name.clone(), pv.value.clone()) + .set(resource_ref, pv.name.clone(), pv.value.clone()) .await .map_err(|e| { AppError::internal_error(format!("Failed to store dead property: {e}")) @@ -800,7 +817,7 @@ async fn handle_proppatch( results.push((&pv.name, true)); } PropPatchOp::Remove(name) => { - dead_props.remove(&path, user.id, name).await.map_err(|e| { + dead_props.remove(resource_ref, name).await.map_err(|e| { AppError::internal_error(format!("Failed to remove dead property: {e}")) })?; results.push((name, true)); @@ -1068,6 +1085,58 @@ async fn resolve_or_legacy( None } +/// Fetch a file's dead properties for a PROPFIND response. +/// +/// Lenient on every failure mode: malformed id, DB error → empty list. +/// PROPFIND must still emit the resource's live properties even when +/// the dead-prop lookup is broken; surfacing a 500 here would mask the +/// resource entirely from sync clients. The legacy path-keyed lookup +/// behaved the same way (`.unwrap_or_default()`); we preserve it. +async fn file_dead_props( + state: &Arc, + file: &FileDto, +) -> Vec<(QualifiedName, Option)> { + let Ok(file_id) = Uuid::parse_str(&file.id) else { + return Vec::new(); + }; + state + .webdav_dead_props + .get_all(ResourceRef::File(file_id)) + .await + .unwrap_or_default() +} + +/// Same shape as `file_dead_props` but for folder rows. Used by the +/// streaming PROPFIND walker. +async fn folder_dead_props( + store: &DeadPropertyStore, + folder: &FolderDto, +) -> Vec<(QualifiedName, Option)> { + let Ok(folder_id) = Uuid::parse_str(&folder.id) else { + return Vec::new(); + }; + store + .get_all(ResourceRef::Folder(folder_id)) + .await + .unwrap_or_default() +} + +/// File-leaf variant for the streaming walker (takes a `&DeadPropertyStore` +/// rather than the full `&Arc` so it can be called from inside +/// the async-stream future without cloning state). +async fn streamed_file_dead_props( + store: &DeadPropertyStore, + file: &FileDto, +) -> Vec<(QualifiedName, Option)> { + let Ok(file_id) = Uuid::parse_str(&file.id) else { + return Vec::new(); + }; + store + .get_all(ResourceRef::File(file_id)) + .await + .unwrap_or_default() +} + /// Extract every `<...>` token from a WebDAV `If:` header value. /// /// RFC 4918 §10.4 defines a richer grammar (tagged-list / no-tag-list of @@ -1587,22 +1656,12 @@ async fn handle_delete( None => return Err(AppError::not_found(format!("Resource not found: {}", path))), } - // Reap dead properties so a future resource at the same path - // doesn't inherit tombstone metadata from the deleted one. Best- - // effort: a failure to clear leaves orphan rows but the user- - // facing DELETE has succeeded, so we don't propagate the error. - // Caught by tests/api/webdav_dead_properties.hurl Step 10. - if let Err(e) = state - .webdav_dead_props - .remove_resource(&path, user.id) - .await - { - tracing::warn!( - user_id = %user.id, - path = %path, - "dead-property cleanup on DELETE failed: {e}" - ); - } + // Dead-property rows attached to the deleted file/folder are reaped + // automatically by `storage.webdav_dead_properties.{folder,file}_id` + // ON DELETE CASCADE (migration 20260830000001). Same guarantee + // applies to every other delete code path — REST `DELETE + // /api/files/{id}`, bulk delete, trash empty, folder cascade — + // without any service-layer call. No explicit cleanup needed here. Ok(Response::builder() .status(StatusCode::NO_CONTENT) @@ -1860,12 +1919,13 @@ async fn handle_move( } } - // Migrate dead properties to the new path (RFC 4918 §9.9 — MOVE preserves properties). - state - .webdav_dead_props - .rename_resource(&source_path, user.id, &destination_path) - .await - .map_err(|e| AppError::internal_error(format!("Failed to migrate dead properties: {e}")))?; + // Dead properties follow the resource automatically across MOVE + // and RENAME: the rows in `storage.webdav_dead_properties` key on + // the underlying folder/file id, which is stable across both + // operations (BEFORE trigger rewrites path on the row, AFTER + // cascade rewrites descendants' path/lpath — but no id ever + // changes). RFC 4918 §9.9 "MOVE preserves properties" satisfied + // by the database invariant, no store call needed. // RFC 4918 §9.9.5: 201 Created when destination is new, 204 when overwritten. let status = if dest_existed { diff --git a/tests/api/webdav_dead_properties.hurl b/tests/api/webdav_dead_properties.hurl index 45a06f05..b78556a0 100644 --- a/tests/api/webdav_dead_properties.hurl +++ b/tests/api/webdav_dead_properties.hurl @@ -14,15 +14,29 @@ # server is broken; OxiCloud sees nothing wrong in its logs). # # Coverage: -# 1. Setup admin, capture JWT, PUT a probe file. -# 2. PROPPATCH set → 207 -# 3. PROPFIND get → value round-trips verbatim -# 4. PROPPATCH upsert (set same name → new value) → 207 -# 5. PROPFIND get → new value (upsert worked) -# 6. PROPPATCH remove → 207 -# 7. PROPFIND get → property absent -# 8. MOVE file → properties follow the path (rename_resource) -# 9. DELETE file → properties cleaned up (no orphan rows) +# 1. Setup admin, capture JWT, PUT a probe file. +# 2. PROPPATCH set → 207 +# 3. PROPFIND get → value round-trips verbatim +# 4. PROPPATCH upsert (set same name → new value) → 207 +# 5. PROPFIND get → new value (upsert worked) +# 6. PROPPATCH remove → 207 +# 7. PROPFIND get → property absent +# 8. MOVE file → properties follow the resource id automatically +# (no rename_resource() call; the row's file_id is stable +# across MOVE so dead-props travel with the resource). +# 9. PROPFIND on moved path returns the property. +# 10. DELETE via WebDAV → FK CASCADE reaps dead-prop rows. +# 11. PROPPATCH + REST DELETE `/api/files/{id}` → FK CASCADE +# reaps via the REST-side delete path too. This is the +# new coverage unlocked by migration 20260830000001 — the +# old path-keyed store had no way to clean up here, so +# the SvelteKit web UI (which deletes via REST) was +# silently leaking tombstones every time a user deleted +# a file that had ever carried dead properties. +# 12. Folder MOVE preserves dead properties (id-stable +# guarantee under rename). The Hurl suite had no folder- +# side coverage of this until 20260830000001; only the +# file MOVE case (step 9) was guarded. # # XPath assertions deliberately use `local-name()` so the test # is robust against the server's choice of namespace prefix — @@ -208,12 +222,15 @@ xpath "count(//*[local-name()='testlabel'])" == 0 # ───────────────────────────────────────────────────────────── -# Step 9 — Re-set a property, then MOVE the file. The -# rename_resource path in DeadPropertyStore must -# re-key the row to the new path so the property -# follows the file (a regression that leaves the row -# at the old path would silently break every client -# that does a MOVE then a PROPFIND). +# Step 9 — Re-set a property, then MOVE the file. Post-rekey +# (migration 20260830000001) the dead-property row +# keys on `file_id`, which never changes across MOVE +# or RENAME — so properties follow the resource by a +# database invariant, without any store-side call. +# A regression that broke this would be a regression +# on the id-stability guarantee in the move SQL itself +# (i.e. it would surface elsewhere too); this assertion +# locks it in for sync clients that do MOVE → PROPFIND. # ───────────────────────────────────────────────────────────── PROPPATCH {{base_url}}/webdav/dead-props-probe.txt Authorization: Bearer {{token}} @@ -260,12 +277,14 @@ xpath "string(//*[local-name()='testlabel'])" == "survives-move" # ───────────────────────────────────────────────────────────── -# Step 10 — DELETE the file; remove_resource() must reap the -# dead-property rows so they don't accumulate as -# tombstones the next time a file is created at the -# same path. We verify by recreating the same path -# and PROPFIND'ing — a leak would resurface the old -# "survives-move" value. +# Step 10 — DELETE the file via WebDAV; the FK +# `webdav_dead_properties.file_id → storage.files.id +# ON DELETE CASCADE` (migration 20260830000001) must +# reap the dead-property rows automatically, so they +# don't accumulate as tombstones the next time a file +# is created at the same path. We verify by recreating +# the same path and PROPFIND'ing — a leak would +# resurface the old "survives-move" value. # ───────────────────────────────────────────────────────────── DELETE {{base_url}}/webdav/dead-props-moved.txt Authorization: Bearer {{token}} @@ -302,6 +321,123 @@ HTTP 207 xpath "count(//*[local-name()='testlabel'])" == 0 +# ───────────────────────────────────────────────────────────── +# Step 11 — Same FK-cascade property test but via the REST API +# delete path. The path-based store would have leaked +# here forever (REST DELETE receives a file_id, not a +# path; the old store had no efficient way to clean +# up). The id-keyed schema reaps the dead-property +# row through the same FK CASCADE on `storage.files`, +# so this proves the new coverage end-to-end. +# +# Sequence: +# a. PROPPATCH a marker dead property on the file. +# b. PROPFIND — confirm it's stored. +# c. Resolve the file's id via REST listing of the +# home folder. +# d. DELETE via `/api/files/{id}` — pure REST, +# never touches the WebDAV surface. +# e. PUT a fresh file at the same WebDAV path. +# f. PROPFIND — must not see the marker. +# ───────────────────────────────────────────────────────────── + +# Step 11a — set a new marker dead property on the just-PUT file +PROPPATCH {{base_url}}/webdav/dead-props-moved.txt +Authorization: Bearer {{token}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + rest-delete-coverage + + + +``` + +HTTP 207 + + +# Step 11b — confirm the marker is stored +PROPFIND {{base_url}}/webdav/dead-props-moved.txt +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +xpath "string(//*[local-name()='restmarker'])" == "rest-delete-coverage" + + +# Step 11c — resolve the file id from the home folder listing. +# The home folder is whatever `GET /api/folders` returns as the +# first root-level entry for the admin user (Personal drive root, +# post drive-no-wrapper). +GET {{base_url}}/api/folders +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +home_folder_id: jsonpath "$[0].id" + + +GET {{base_url}}/api/files?folder_id={{home_folder_id}} +Authorization: Bearer {{token}} + +HTTP 200 +[Captures] +rest_file_id: jsonpath "$[?(@.name=='dead-props-moved.txt')].id" nth 0 + + +# Step 11d — REST DELETE. No webdav, no dead-prop API call — +# the cleanup must happen via the FK CASCADE on storage.files. +DELETE {{base_url}}/api/files/{{rest_file_id}} +Authorization: Bearer {{token}} + +# The REST delete handler returns 204 No Content on success. +HTTP 204 + + +# Step 11e — recreate the file at the same WebDAV path +PUT {{base_url}}/webdav/dead-props-moved.txt +Authorization: Bearer {{token}} +Content-Type: text/plain +``` +fresh file post REST DELETE +``` + +HTTP 201 + + +# Step 11f — PROPFIND must not surface the old marker +PROPFIND {{base_url}}/webdav/dead-props-moved.txt +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +# If REST DELETE failed to cascade, the marker would still be +# attached to the (recreated) path under the old `(path, user_id)` +# key — but the new schema keys by file_id, and the REST DELETE +# took the storage.files row with it. Asserting absence proves +# the cascade fired. +xpath "count(//*[local-name()='restmarker'])" == 0 + + # ───────────────────────────────────────────────────────────── # Cleanup # ───────────────────────────────────────────────────────────── @@ -309,3 +445,87 @@ DELETE {{base_url}}/webdav/dead-props-moved.txt Authorization: Bearer {{token}} HTTP 204 + + +# ───────────────────────────────────────────────────────────── +# Step 12 — Folder MOVE dead-property preservation. +# Same invariant as step 9 (id-stability under MOVE) +# but for folders. The Hurl suite had no folder-side +# coverage of this until now, so a regression that +# broke folder dead-property preservation could +# silently land — calendar / contacts / NextCloud +# clients that PROPPATCH per-folder sync state would +# lose it on every rename. +# +# Sequence: +# a. MKCOL a fresh test folder. +# b. PROPPATCH a dead property on it. +# c. MOVE / rename the folder. +# d. PROPFIND the new collection path; assert +# the property survived. +# e. Cleanup: DELETE the renamed folder. +# ───────────────────────────────────────────────────────────── + +# Step 12a — fresh collection (no prior state at this path) +MKCOL {{base_url}}/webdav/dead-props-folder/ +Authorization: Bearer {{token}} + +HTTP 201 + + +# Step 12b — attach a marker dead property to the FOLDER row +PROPPATCH {{base_url}}/webdav/dead-props-folder/ +Authorization: Bearer {{token}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + folder-keeps-this + + + +``` + +HTTP 207 + + +# Step 12c — rename the folder via MOVE. Same-parent rename +# (intra-collection name change) — the most common shape clients +# issue and the one that previously needed `rename_resource()` +# to keep dead properties attached. +MOVE {{base_url}}/webdav/dead-props-folder/ +Authorization: Bearer {{token}} +Destination: {{base_url}}/webdav/dead-props-folder-renamed/ + +HTTP 201 + + +# Step 12d — PROPFIND the new collection path; the dead property +# must still be attached. If the folder row's id had changed +# under MOVE (it doesn't), or if anything had reaped the +# webdav_dead_properties row, the property would be gone. +PROPFIND {{base_url}}/webdav/dead-props-folder-renamed/ +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +xpath "string(//*[local-name()='foldermark'])" == "folder-keeps-this" + + +# Step 12e — cleanup. The DELETE cascades the foldermark row +# away via FK ON DELETE CASCADE, leaving the schema clean for +# any subsequent test that touches this path. +DELETE {{base_url}}/webdav/dead-props-folder-renamed/ +Authorization: Bearer {{token}} + +HTTP 204