From f2dc567bcd00a3e8a4d61e10b5d89fc0454cb844 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 23:21:19 +0200 Subject: [PATCH 1/4] chore(justfile): api-test: add litmus webdav play listmus webdav test only if found locally --- justfile | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/justfile b/justfile index 89e59729..35e78e6d 100644 --- a/justfile +++ b/justfile @@ -172,9 +172,17 @@ front-design: # .github/workflows/ci.yml; keep the order in sync so a local pass means # CI passes. api-test: - bash tests/api/run.sh - bash tests/webdav/run.sh - bash tests/oidc/run.sh + #!/usr/bin/env bash + set -euo pipefail + ./tests/api/run.sh + ./tests/webdav/run.sh + ./tests/oidc/run.sh + if which litmus >/dev/null 2>/dev/null + then + ./tests/webdav/run-litmus.sh + else + echo "XXX litmus webdav not found, ignore test" + fi # --------------------------------------------------------------------------- # SvelteKit frontend (frontend/) — the only frontend. These `fe-*` recipes From cb7b653a15894a1d4f37149fbc7c00d9a7ba3bf5 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 23:33:17 +0200 Subject: [PATCH 2/4] feat(webdav): bind dead prop to res. id rather path --- ...bdav_dead_properties_resource_id_rekey.sql | 112 ++++++++ .../services/webdav_dead_property_store.rs | 233 +++++++++------- src/interfaces/api/handlers/webdav_handler.rs | 208 +++++++++----- tests/api/webdav_dead_properties.hurl | 262 ++++++++++++++++-- 4 files changed, 613 insertions(+), 202 deletions(-) create mode 100644 migrations/20260830000001_webdav_dead_properties_resource_id_rekey.sql 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 From d8adae3572b8a4c06fde498b447cb58879a5435f Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Tue, 30 Jun 2026 23:44:42 +0200 Subject: [PATCH 3/4] feat(dead-props): ensure replication on copy --- ...02_copy_dead_properties_on_folder_tree.sql | 223 +++++++++++++++ .../pg/file_blob_write_repository.rs | 27 +- tests/api/webdav_dead_properties.hurl | 264 +++++++++++++++++- 3 files changed, 509 insertions(+), 5 deletions(-) create mode 100644 migrations/20260830000002_copy_dead_properties_on_folder_tree.sql diff --git a/migrations/20260830000002_copy_dead_properties_on_folder_tree.sql b/migrations/20260830000002_copy_dead_properties_on_folder_tree.sql new file mode 100644 index 00000000..e33fa77f --- /dev/null +++ b/migrations/20260830000002_copy_dead_properties_on_folder_tree.sql @@ -0,0 +1,223 @@ +-- ════════════════════════════════════════════════════════════════════════════ +-- COPY: duplicate dead properties along with files and folders +-- ════════════════════════════════════════════════════════════════════════════ +-- RFC 4918 §8.8 — "If a property cannot be copied live, then its value +-- MUST be duplicated, exactly as it would be for a PROPPATCH SET +-- operation, in the copy." Dead properties are by definition not live +-- (the server stores them verbatim with no interpretation), so every +-- COPY MUST duplicate the source's dead properties onto the new +-- resource. +-- +-- The pre-rekey path-based store handled this by accident in some +-- cases and missed it in others; the id-keyed store (migration +-- 20260830000001) makes the requirement explicit — dead properties +-- key on `folder_id` / `file_id`, so a copy that doesn't insert new +-- rows for the destination's ids loses the properties entirely. +-- +-- This migration replaces `storage.copy_folder_tree` with a version +-- that: +-- +-- 1. Pre-allocates destination file ids in a new temp table +-- `_copy_file_map(old_id, new_id)` — analogous to the +-- pre-existing `_copy_map` that already does this for folders. +-- Previously, file ids were generated by the `gen_random_uuid()` +-- DEFAULT during the batch INSERT, leaving no way to relate src +-- and dst files afterward. +-- 2. Switches the batch file INSERT to use the explicit +-- pre-allocated id, so src→dst is bidirectionally known by +-- `_copy_file_map`. +-- 3. Adds two `INSERT INTO storage.webdav_dead_properties` SELECTs +-- at the end that duplicate dead-property rows for every copied +-- folder (via `_copy_map`) and every copied file (via +-- `_copy_file_map`). Each duplicated row carries the same +-- `(namespace, local_name, value)` triple as the source — the +-- definition of "duplicate" in RFC 4918 §8.8. +-- +-- Idempotent via CREATE OR REPLACE FUNCTION. No callers change (the +-- function signature and return shape are unchanged). +-- +-- COPY semantics out of scope for this migration: +-- * Cross-user permission handling on the copied resources is the +-- caller's responsibility (the `_with_perms` service variant +-- already enforces this on the source side). Dead properties +-- hitch a ride on the resource's ACL; nothing additional needed. +-- * Trash: trashed source rows are excluded by the existing +-- `NOT is_trashed` filter; dead-props on trashed rows live on +-- until the resource itself is hard-deleted, at which point +-- CASCADE handles them. Same model holds in the new COPY path. + +CREATE OR REPLACE FUNCTION storage.copy_folder_tree( + p_source_id UUID, + p_target_parent_id UUID, -- NULL = copy to root (keeps source drive) + p_dest_name TEXT DEFAULT NULL -- NULL = keep source folder name +) RETURNS TABLE(new_root_id TEXT, folders_copied BIGINT, files_copied BIGINT) AS $$ +DECLARE + v_root_lpath ltree; + v_root_depth INT; + v_max_depth INT; + v_level INT; + v_folders BIGINT := 0; + v_files BIGINT := 0; + v_inserted BIGINT; + v_new_root UUID; + v_dest_drive_id UUID; +BEGIN + -- Validate source exists + SELECT fo.lpath, nlevel(fo.lpath) + INTO v_root_lpath, v_root_depth + FROM storage.folders fo + WHERE fo.id = p_source_id AND NOT fo.is_trashed; + + IF v_root_lpath IS NULL THEN + RAISE EXCEPTION 'Source folder not found: %', p_source_id + USING ERRCODE = 'P0002'; -- no_data_found + END IF; + + -- Resolve the destination drive_id ONCE up front. The whole copied + -- subtree lands in this drive; pulling it per-row from `fo.drive_id` + -- (the previous body) was the cross-drive bug. + -- + -- When p_target_parent_id is NULL the caller asked for "copy to + -- root" — there is no global root in the multi-drive world, so we + -- preserve the source's drive_id (legacy behaviour, defensive). + -- Real API call sites always pass a concrete target folder. + IF p_target_parent_id IS NULL THEN + SELECT fo.drive_id INTO v_dest_drive_id + FROM storage.folders fo + WHERE fo.id = p_source_id; + ELSE + SELECT fo.drive_id INTO v_dest_drive_id + FROM storage.folders fo + WHERE fo.id = p_target_parent_id AND NOT fo.is_trashed; + IF v_dest_drive_id IS NULL THEN + RAISE EXCEPTION 'Target parent folder not found: %', p_target_parent_id + USING ERRCODE = 'P0002'; -- no_data_found + END IF; + END IF; + + -- Temp mapping: every folder in the subtree → new UUID + CREATE TEMP TABLE IF NOT EXISTS _copy_map( + old_id UUID PRIMARY KEY, + new_id UUID NOT NULL DEFAULT gen_random_uuid() + ) ON COMMIT DROP; + TRUNCATE _copy_map; + + INSERT INTO _copy_map(old_id) + SELECT fo.id + FROM storage.folders fo + WHERE NOT fo.is_trashed + AND fo.lpath <@ v_root_lpath; + + -- Remember new root ID + SELECT cm.new_id INTO v_new_root + FROM _copy_map cm WHERE cm.old_id = p_source_id; + + -- Max depth for level iteration + SELECT MAX(nlevel(fo.lpath)) + INTO v_max_depth + FROM storage.folders fo + JOIN _copy_map cm ON fo.id = cm.old_id; + + -- ── Insert folders level by level ── + -- Each level is a separate INSERT so the BEFORE INSERT trigger + -- (trg_folders_path) can resolve the parent's path/lpath from rows + -- inserted in the previous level. drive_id is the destination's + -- (resolved once above); user_id + provenance preserved from source. + FOR v_level IN v_root_depth .. v_max_depth LOOP + INSERT INTO storage.folders( + id, name, parent_id, user_id, + drive_id, created_by, updated_by + ) + SELECT cm.new_id, + CASE WHEN fo.id = p_source_id AND p_dest_name IS NOT NULL + THEN p_dest_name ELSE fo.name END, + CASE WHEN fo.id = p_source_id THEN p_target_parent_id + ELSE pm.new_id END, + fo.user_id, + v_dest_drive_id, + fo.created_by, + fo.updated_by + FROM storage.folders fo + JOIN _copy_map cm ON fo.id = cm.old_id + LEFT JOIN _copy_map pm ON fo.parent_id = pm.old_id + WHERE NOT fo.is_trashed + AND nlevel(fo.lpath) = v_level; + + GET DIAGNOSTICS v_inserted = ROW_COUNT; + v_folders := v_folders + v_inserted; + END LOOP; + + -- ── NEW: temp mapping for files src→dst ─────────────────────────── + -- Pre-allocate destination ids so we can: + -- (a) reference each dst file by id in the dead-property INSERT + -- below — a batched INSERT...RETURNING couldn't tell us which + -- new id corresponded to which source id, so the mapping + -- has to be stamped at planning time, not after the fact; + -- (b) batch the file INSERT with explicit ids exactly the same + -- way folders are batched. + CREATE TEMP TABLE IF NOT EXISTS _copy_file_map( + old_id UUID PRIMARY KEY, + new_id UUID NOT NULL DEFAULT gen_random_uuid() + ) ON COMMIT DROP; + TRUNCATE _copy_file_map; + + INSERT INTO _copy_file_map(old_id) + SELECT f.id + FROM storage.files f + JOIN _copy_map cm ON f.folder_id = cm.old_id + WHERE NOT f.is_trashed; + + -- ── Batch copy all files (zero-copy: same blob_hash) ── + -- drive_id from destination; everything else (user_id, created_by, + -- updated_by) preserved from source so authorship survives the copy. + -- `id` is the pre-allocated dst id from _copy_file_map. + INSERT INTO storage.files( + id, name, folder_id, user_id, blob_hash, size, mime_type, + media_sort_date, drive_id, created_by, updated_by + ) + SELECT fm.new_id, f.name, cm.new_id, f.user_id, f.blob_hash, f.size, + f.mime_type, f.media_sort_date, v_dest_drive_id, f.created_by, + f.updated_by + FROM storage.files f + JOIN _copy_map cm ON f.folder_id = cm.old_id + JOIN _copy_file_map fm ON fm.old_id = f.id + WHERE NOT f.is_trashed; + + GET DIAGNOSTICS v_files = ROW_COUNT; + + -- ── Batch increment blob ref_counts ── + IF v_files > 0 THEN + UPDATE storage.blobs b + SET ref_count = ref_count + hc.cnt + FROM ( + SELECT f.blob_hash, COUNT(*)::int AS cnt + FROM storage.files f + JOIN _copy_map cm ON f.folder_id = cm.new_id + WHERE NOT f.is_trashed + GROUP BY f.blob_hash + ) hc + WHERE b.hash = hc.blob_hash; + END IF; + + -- ── NEW: duplicate dead properties for every copied folder ──────── + -- RFC 4918 §8.8 — dead properties MUST be duplicated. The id-keyed + -- store (migration 20260830000001) keys on `folder_id`, so we + -- emit a new row per source dead-property pointing at the + -- destination folder id. `(namespace, local_name, value)` is + -- preserved verbatim — that's the "duplicate" definition. + INSERT INTO storage.webdav_dead_properties + (folder_id, namespace, local_name, value) + SELECT cm.new_id, dp.namespace, dp.local_name, dp.value + FROM storage.webdav_dead_properties dp + JOIN _copy_map cm ON dp.folder_id = cm.old_id; + + -- ── NEW: duplicate dead properties for every copied file ────────── + INSERT INTO storage.webdav_dead_properties + (file_id, namespace, local_name, value) + SELECT fm.new_id, dp.namespace, dp.local_name, dp.value + FROM storage.webdav_dead_properties dp + JOIN _copy_file_map fm ON dp.file_id = fm.old_id; + + RETURN QUERY SELECT v_new_root::text, v_folders, v_files; +END; +$$ LANGUAGE plpgsql; diff --git a/src/infrastructure/repositories/pg/file_blob_write_repository.rs b/src/infrastructure/repositories/pg/file_blob_write_repository.rs index cdb4aa8f..43063851 100644 --- a/src/infrastructure/repositories/pg/file_blob_write_repository.rs +++ b/src/infrastructure/repositories/pg/file_blob_write_repository.rs @@ -642,14 +642,33 @@ impl FileWritePort for FileBlobWriteRepository { $4, $4 FROM src, dest_folder - RETURNING id::text, name, folder_id::text, size, mime_type, - EXTRACT(EPOCH FROM created_at)::bigint, - EXTRACT(EPOCH FROM updated_at)::bigint, + RETURNING id, + id::text AS id_text, + name, folder_id::text, size, mime_type, + EXTRACT(EPOCH FROM created_at)::bigint AS created_at, + EXTRACT(EPOCH FROM updated_at)::bigint AS updated_at, blob_hash, created_by, updated_by + ), + -- RFC 4918 §8.8 — dead properties MUST be duplicated on + -- COPY. With the id-keyed store (migration + -- 20260830000001) this is a single batch INSERT keyed on + -- the new file's id. Runs in the same query as the file + -- INSERT so either both land or neither does — atomic + -- by virtue of being one statement. + dead_prop_copy AS ( + INSERT INTO storage.webdav_dead_properties + (file_id, namespace, local_name, value) + SELECT (SELECT id FROM new_file), + dp.namespace, dp.local_name, dp.value + FROM storage.webdav_dead_properties dp + WHERE dp.file_id = $1::uuid ) - SELECT * FROM new_file + SELECT id_text, name, folder_id, size, mime_type, + created_at, updated_at, + blob_hash, created_by, updated_by + FROM new_file "#, ) .bind(file_id) diff --git a/tests/api/webdav_dead_properties.hurl b/tests/api/webdav_dead_properties.hurl index b78556a0..a1bba249 100644 --- a/tests/api/webdav_dead_properties.hurl +++ b/tests/api/webdav_dead_properties.hurl @@ -37,6 +37,17 @@ # guarantee under rename). The Hurl suite had no folder- # side coverage of this until 20260830000001; only the # file MOVE case (step 9) was guarded. +# 13. Single-file COPY duplicates dead properties (RFC 4918 +# §8.8). Destination carries a copy of the source's +# marker; source retains its copy (COPY ≠ MOVE). +# Implementation: `dead_prop_copy` CTE branch in +# `copy_file` (migration 20260830000002). +# 14. Folder COPY (Depth: infinity) duplicates dead +# properties for every descendant — both folder and file +# dead-props. Implementation: the two INSERT...SELECT +# branches in `storage.copy_folder_tree` (migration +# 20260830000002) that walk `_copy_map` and the new +# `_copy_file_map` respectively. # # XPath assertions deliberately use `local-name()` so the test # is robust against the server's choice of namespace prefix — @@ -393,7 +404,13 @@ Authorization: Bearer {{token}} HTTP 200 [Captures] -rest_file_id: jsonpath "$[?(@.name=='dead-props-moved.txt')].id" nth 0 +# Hurl quirk: `$[?(...)]` collapses to a scalar (not a list) when the +# filter matches exactly one element, so `nth 0` fails with "invalid +# filter input type". The bare filter capture returns that scalar +# directly. Filename uniqueness across the home folder makes the +# single-match assumption safe — `dead-props-moved.txt` is created +# only by this test (no other Hurl test ever PUTs that name). +rest_file_id: jsonpath "$[?(@.name=='dead-props-moved.txt')].id" # Step 11d — REST DELETE. No webdav, no dead-prop API call — @@ -529,3 +546,248 @@ DELETE {{base_url}}/webdav/dead-props-folder-renamed/ Authorization: Bearer {{token}} HTTP 204 + + +# ───────────────────────────────────────────────────────────── +# Step 13 — Single-file COPY duplicates dead properties. +# RFC 4918 §8.8: dead properties MUST be duplicated. +# Implementation is the `dead_prop_copy` CTE branch in +# `file_blob_write_repository::copy_file` (inserts a +# new dead-prop row per source row, keyed on the new +# file's id). +# +# Sequence: +# a. PUT a source file. +# b. PROPPATCH a marker dead property. +# c. COPY (WebDAV) to a new path. +# d. PROPFIND the new path; marker must be present. +# e. PROPFIND the source path; marker still present +# on source too (COPY duplicates — it doesn't +# move). +# f. Cleanup both files. +# ───────────────────────────────────────────────────────────── + +# Step 13a — source file +PUT {{base_url}}/webdav/dead-props-copy-src.txt +Authorization: Bearer {{token}} +Content-Type: text/plain +``` +copy source +``` + +HTTP 201 + + +# Step 13b — set the marker dead property on the source +PROPPATCH {{base_url}}/webdav/dead-props-copy-src.txt +Authorization: Bearer {{token}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + survives-copy + + + +``` + +HTTP 207 + + +# Step 13c — COPY the file. Destination is fresh → 201 Created. +# §9.8.5: 201 when destination is new, 204 when overwriting. +COPY {{base_url}}/webdav/dead-props-copy-src.txt +Authorization: Bearer {{token}} +Destination: {{base_url}}/webdav/dead-props-copy-dst.txt + +HTTP 201 + + +# Step 13d — destination must carry the property (RFC 4918 §8.8) +PROPFIND {{base_url}}/webdav/dead-props-copy-dst.txt +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +xpath "string(//*[local-name()='copymark'])" == "survives-copy" + + +# Step 13e — source still has it too (COPY, not MOVE) +PROPFIND {{base_url}}/webdav/dead-props-copy-src.txt +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +xpath "string(//*[local-name()='copymark'])" == "survives-copy" + + +# Step 13f — cleanup both +DELETE {{base_url}}/webdav/dead-props-copy-src.txt +Authorization: Bearer {{token}} + +HTTP 204 + + +DELETE {{base_url}}/webdav/dead-props-copy-dst.txt +Authorization: Bearer {{token}} + +HTTP 204 + + +# ───────────────────────────────────────────────────────────── +# Step 14 — Folder COPY duplicates dead properties on every +# descendant. RFC 4918 §8.8 + §9.8.3 (Depth: infinity +# for collections). Implementation is the two +# INSERT...SELECT branches added to +# `storage.copy_folder_tree` in migration +# 20260830000002: +# - folders mapped via `_copy_map` +# - files mapped via the new `_copy_file_map` +# +# Test shape: +# a. MKCOL outer collection. +# b. MKCOL inner collection (descendant). +# c. PUT a leaf file inside inner. +# d. PROPPATCH a marker on the descendant FOLDER. +# e. PROPPATCH a different marker on the leaf FILE. +# f. COPY outer/ → outer-copy/ (Depth: infinity). +# g. PROPFIND descendant in copy; marker present. +# h. PROPFIND leaf in copy; marker present. +# i. Cleanup both trees. +# ───────────────────────────────────────────────────────────── + +# Step 14a/b/c — build the source subtree +MKCOL {{base_url}}/webdav/dead-props-copy-tree/ +Authorization: Bearer {{token}} + +HTTP 201 + + +MKCOL {{base_url}}/webdav/dead-props-copy-tree/inner/ +Authorization: Bearer {{token}} + +HTTP 201 + + +PUT {{base_url}}/webdav/dead-props-copy-tree/inner/leaf.txt +Authorization: Bearer {{token}} +Content-Type: text/plain +``` +leaf inside the copy tree +``` + +HTTP 201 + + +# Step 14d — marker on the descendant FOLDER +PROPPATCH {{base_url}}/webdav/dead-props-copy-tree/inner/ +Authorization: Bearer {{token}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + inner-folder-mark + + + +``` + +HTTP 207 + + +# Step 14e — marker on the leaf FILE +PROPPATCH {{base_url}}/webdav/dead-props-copy-tree/inner/leaf.txt +Authorization: Bearer {{token}} +Content-Type: application/xml; charset=utf-8 +``` + + + + + leaf-file-mark + + + +``` + +HTTP 207 + + +# Step 14f — recursive COPY (Depth: infinity is the default for +# collections per RFC 4918 §9.8.3). Destination is fresh → 201. +COPY {{base_url}}/webdav/dead-props-copy-tree/ +Authorization: Bearer {{token}} +Destination: {{base_url}}/webdav/dead-props-copy-tree-clone/ + +HTTP 201 + + +# Step 14g — descendant folder in the COPY carries the folder marker. +# The path resolves only if `storage.copy_folder_tree` correctly +# duplicated the descendant folder AND its dead-prop row. +PROPFIND {{base_url}}/webdav/dead-props-copy-tree-clone/inner/ +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +xpath "string(//*[local-name()='innermark'])" == "inner-folder-mark" + + +# Step 14h — leaf file in the COPY carries the file marker +PROPFIND {{base_url}}/webdav/dead-props-copy-tree-clone/inner/leaf.txt +Authorization: Bearer {{token}} +Depth: 0 +Content-Type: application/xml; charset=utf-8 +``` + + + + +``` + +HTTP 207 +[Asserts] +xpath "string(//*[local-name()='leafmark'])" == "leaf-file-mark" + + +# Step 14i — cleanup both trees. Recursive DELETE cascades each +# subtree's folder + file rows, and the FK ON DELETE CASCADE on +# webdav_dead_properties takes the dead-prop rows with them. +DELETE {{base_url}}/webdav/dead-props-copy-tree/ +Authorization: Bearer {{token}} + +HTTP 204 + + +DELETE {{base_url}}/webdav/dead-props-copy-tree-clone/ +Authorization: Bearer {{token}} + +HTTP 204 From d134fc889d2dbb1180759ab6b7b72473b0bbbd08 Mon Sep 17 00:00:00 2001 From: Edouard Vanbelle Date: Wed, 1 Jul 2026 00:07:13 +0200 Subject: [PATCH 4/4] doc(metadata): bride dead props to API metadata MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit what about exposing dead props into rest API let user to store preferences, labels on resources | Use case | What it looks like | Why dead-props help | |---|---|---| | Photo annotations | captions, ratings (1-5), notes per photo | already keyed by `file_id`; round-trips via WebDAV without re-implementing | | Web-UI tags / labels | `oxi:user:tag/project=alpha`, color flags, "archived" markers | per-resource user metadata without new tables | | Folder UI preferences | default sort, default view mode, "favourite" flag | persistent per-folder, shared across users on shared drives | | Cross-protocol bridge | Thunderbird sets `oxi:lastsync=...` via PROPPATCH → web UI reads it via REST | one store, two surfaces — visibility goes both ways | | Workflow / approval state | `reviewed_by=alice`, `due=2026-09-15` | ad-hoc state per resource without schema sprawl | | Third-party integrations | external apps store scratch space per resource | lower barrier than implementing WebDAV | --- docs/plan/extra-metadata.md | 364 ++++++++++++++++++++++++++++++++++++ 1 file changed, 364 insertions(+) create mode 100644 docs/plan/extra-metadata.md diff --git a/docs/plan/extra-metadata.md b/docs/plan/extra-metadata.md new file mode 100644 index 00000000..e8cf1a67 --- /dev/null +++ b/docs/plan/extra-metadata.md @@ -0,0 +1,364 @@ +# Plan — Expose dead properties as a REST metadata API + +## Context + +Migration `20260830000001` rekeyed `storage.webdav_dead_properties` from +`(resource_path, user_id)` to a polymorphic resource id +(`folder_id` XOR `file_id`) with `ON DELETE CASCADE`. The table is now a +clean per-resource key-value store: `(resource id, namespace, local_name) → value`, +shaped exactly like a generic metadata layer. + +Today only WebDAV (PROPPATCH / PROPFIND) reads and writes it. NextCloud DAV +sees the rows (id-keyed, no path coupling) but isn't yet wired up to +emit/consume them. No REST surface exists. + +We discussed the question "any interest in exposing this as a REST +metadata API?" on **2026-06-30** during the rekey landing and agreed it's +worth a follow-up plan but not part of the rekey itself. This document +captures the design we sketched so we can pick it up without +re-litigating. + +## Why this is worth doing now (and not before the rekey) + +| Pre-rekey schema | Post-rekey schema | +|---|---| +| `(resource_path, user_id)` key | `(folder_id XOR file_id, namespace, local_name)` key | +| Path-keyed → invalidated on rename / move | Id-keyed → stable across rename / move (DB invariant) | +| User-siloed → wrong for shared drives | Resource-state — correct under D1+ shared-drive semantics | +| Service-layer deletes leak tombstones | FK `ON DELETE CASCADE` reaps on every delete code path | + +Pre-rekey, exposing the store via REST would have been wrong: REST clients +operate on resource ids, but the store keyed on paths; cross-protocol +parity would have been a mess. Post-rekey, the store IS already shaped +like the API we'd want — a thin REST layer matches it 1:1. + +## Use cases + +| Use case | What it looks like | Why dead-props help | +|---|---|---| +| Photo annotations | captions, ratings (1-5), notes per photo | already keyed by `file_id`; round-trips via WebDAV without re-implementing | +| Web-UI tags / labels | `oxi:user:tag/project=alpha`, color flags, "archived" markers | per-resource user metadata without new tables | +| Folder UI preferences | default sort, default view mode, "favourite" flag | persistent per-folder, shared across users on shared drives | +| Cross-protocol bridge | Thunderbird sets `oxi:lastsync=...` via PROPPATCH → web UI reads it via REST | one store, two surfaces — visibility goes both ways | +| Workflow / approval state | `reviewed_by=alice`, `due=2026-09-15` | ad-hoc state per resource without schema sprawl | +| Third-party integrations | external apps store scratch space per resource | lower barrier than implementing WebDAV | + +Each use case is the same store; only the values differ. That's why +exposing it as a generic API is more leverage than adding ad-hoc columns +for any one of them. + +## API shape (decided) + +### Per-resource CRUD — nested under the resource + +``` +GET /api/files/{id}/metadata → list all keys +GET /api/files/{id}/metadata/{namespace}/{name} → fetch one value +PUT /api/files/{id}/metadata/{namespace}/{name} → upsert (body = value) +DELETE /api/files/{id}/metadata/{namespace}/{name} → remove one key + +GET /api/folders/{id}/metadata → list all keys +GET /api/folders/{id}/metadata/{namespace}/{name} → fetch one value +PUT /api/folders/{id}/metadata/{namespace}/{name} → upsert +DELETE /api/folders/{id}/metadata/{namespace}/{name} → remove one key +``` + +The `{kind}` is encoded in the URL prefix, so we don't carry a +discriminator field. `{namespace}` and `{name}` are passed verbatim to +the store; URL-encode the colon-containing namespaces +(`oxi:user:tag` → `oxi%3Auser%3Atag`). + +This shape matches the rest of the API — `/api/files/{id}/thumbnail`, +`/api/files/{id}/preview`, `/api/folders/{id}/contents` — and stays +discoverable as a sub-resource of the file/folder. + +AuthZ goes through the `_with_perms` service path: +- `Read` on the resource → GET allowed. +- `Update` on the resource → PUT / DELETE allowed. +- 404 on no-Read (anti-enumeration), 403 on Read-but-no-Update. + +GET (list) response shape: + +```json +{ + "properties": [ + { + "namespace": "oxi:user:tag", + "name": "project", + "value": "alpha", + "updated_by": "", + "updated_at": "2026-06-30T20:33:38Z" + }, + ... + ] +} +``` + +The resource itself is identified by the URL — no need to echo +`{ "kind": ..., "id": ... }` in the body. + +### Cross-resource lookup — separate search endpoint (deferred to phase 3) + +Cross-resource search ("which files have `oxi:user:tag/project=alpha`?") +is a fundamentally different operation from CRUD — it's a SEARCH, not a +fetch. Nesting it under a single resource URL would be wrong, and +overloading CRUD with `?filter=...` would muddy the shape. It lives at +its own endpoint: + +``` +GET /api/search/metadata?namespace=...&name=...&value=...&kind=file +``` + +This separation has three concrete payoffs: + +- **CRUD path stays simple**: per-resource fetch/upsert/remove with no + query-string filter logic. +- **AuthZ shape differs**: per-resource CRUD enforces permissions on + ONE resource; search must enumerate every resource the caller can + Read, then filter. That's expensive enough to need its own + rate-limit / pagination story. Isolating it keeps the CRUD path + cheap. +- **Search can grow** more filter syntax (multiple keys, value + patterns, `>` / `<` comparisons) without touching the CRUD shape. + +Search is **phase 3** — it's not required for the read-only or +read-write cases (phases 1 and 2). Don't build it until a UI feature +asks for it. + +Note this is the LOW-VOLUME lookup option. Genuine tag-based faceted +browse at scale needs a first-class tags table with indexes — not a +metadata-table scan. The search endpoint exists for debugging, small +instances, and occasional one-off queries. See "Out of scope" below. + +## Schema additions needed + +```sql +ALTER TABLE storage.webdav_dead_properties + ADD COLUMN updated_by UUID NULL REFERENCES auth.users(id) ON DELETE SET NULL; +``` + +The `updated_at` column already exists. `updated_by` is the new bit — +load-bearing if both WebDAV and REST are writing. Without it, "why did +this caption change overnight?" is blind. + +Set on every `set()` / `remove()` (the latter currently has no provenance +concept, but the audit value would be "who reaped it" — same column). +`ON DELETE SET NULL` so a user delete doesn't lose the property itself, +only the authorship — symmetric with how other audit columns in the +schema behave (`created_by` on folders/files is `ON DELETE SET NULL` for +the same reason). + +## Decisions to lock in before implementation + +### 1. Namespace policy — denylist or allowlist? + +Server-managed namespaces (`DAV:`, anything we want to use internally for +sync state, locks, etc.) should be REST-write-rejected so REST can't +poison live WebDAV behaviour. + +**Recommendation: denylist.** More permissive, less surprising, matches +the WebDAV side (which lets clients write any namespace they please). +Initial denylist: + +- `DAV:` — RFC 4918 live properties; server-managed. +- `oxi:internal:*` — reserved for future server-managed properties. + +REST read is unrestricted; only write is filtered. + +### 2. Size limits + +Today no cap. WebDAV is bounded by `MAX_XML_BODY` (1 MB) on the request, +but per-row there's no limit and no per-resource key-count limit. A +REST API in the wild needs both: + +- per-value: **64 KB** (enough for any human-authored caption, JSON blob, + or sync token; rejects "use the metadata table as a file store" + abuse). +- per-resource: **100 keys** (enough for any reasonable application; + rejects "use it as a directory listing"). + +Both as 413 Payload Too Large on the offending endpoint. + +WebDAV PROPPATCH should adopt the same per-key limit (currently bounded +only by the 1 MB body); per-resource count limit applies on the +incremental write. + +### 3. Value content type + +Stored as `TEXT` today. If REST PUTs JSON, WebDAV clients reading it +back via PROPFIND wrap it in their XML envelope and see `"{...}"` as a +literal string. Defensible (it's "just a string"); document the +convention. + +If we add a `content_type` column (RFC 4918 §15.5 `getcontenttype` on +properties is murky), REST can return the original `Content-Type` to +REST callers and WebDAV continues to see the literal value. Probably +**not worth it** until a real use case needs it — adds a column + a +write path branch for zero functional benefit today. + +### 4. Listing semantics + +Inlining child metadata into `GET /api/folders/{id}/contents` is +tempting — fewer round-trips for the UI — but PROPFIND already pays this +O(N) cost and it's expensive on big folders. + +**Recommendation: dedicated endpoint only.** No inlining. UIs that need +per-child metadata can batch via `GET /api/files/{id}/metadata` calls +in parallel (HTTP/2 multiplexing makes that cheap) until measured +demand justifies a bulk endpoint. + +### 5. WebDAV-write hygiene + +REST writes go through the same `DeadPropertyStore::set` as PROPPATCH — +no special branch. The denylist (above) gates which namespaces REST may +write; WebDAV stays unrestricted. + +## Scope: phase 1 / 2 / 3 + +### Phase 1 (read-only) + +GET endpoints only, nested under `/api/{files,folders}/{id}/metadata`: + +- `GET /api/files/{id}/metadata` → list +- `GET /api/files/{id}/metadata/{namespace}/{name}` → single +- `GET /api/folders/{id}/metadata` → list +- `GET /api/folders/{id}/metadata/{namespace}/{name}` → single + +Plus the `updated_by` schema migration (so phase 2 doesn't break wire +contracts). + +Use case unlocked: the SvelteKit UI can READ properties Thunderbird / +DAVx5 / Cyberduck have written. Cross-protocol visibility, one +direction. + +Cost: ~150 LOC handler + ~20 LOC migration. No new authz primitives — +`Read` permission already exists. + +### Phase 2 (write) + +PUT + DELETE. Namespace denylist. Size limits. + +Decide first what the primary REST writer is: + +- **Photo captions**: probably wants a dedicated `/api/photos/{id}/caption` + endpoint that stores under a fixed `oxi:photo:caption` key. Generic + API still useful but not the obvious surface. +- **Tags**: deserves a structured tags table (queryable, faceted search) + rather than k/v. Generic API is the wrong shape. +- **Workflow state**: generic API IS the right shape — exactly what k/v + was designed for. +- **Third-party integrations**: generic API is the right shape. + +If the primary writer turns out to be one of the structured-data cases, +phase 2 may never ship — the read API plus a dedicated write endpoint +per feature is the better factoring. The decision should be driven by +real demand, not speculative design. + +### Phase 3 (cross-resource search) + +`GET /api/search/metadata?namespace=...&name=...&value=...&kind=file` +with pagination. AuthZ enumerates resources the caller has Read on +and filters in-engine. + +Scope guidance: + +- Low-volume **only**. Implemented as a sequential scan with + permission filter. No new indexes (the (`namespace`, `local_name`, + `value`) shape doesn't index cheaply, and adding `value` to a + composite index changes the table's write pattern). +- Filter syntax stays minimal until a UI feature drives it. Start + with `namespace=` and `name=`; add `value=` literal match next; add + `value~=` pattern match only when needed. +- Hard rate-limit per caller — search is expensive enough that an + unbounded REST client could starve the database. + +If demand for tag-based browse at scale ever materialises, do NOT +extend this endpoint — build a dedicated tags table with an inverted +index. Search-on-metadata is the debug/scratch tool, not the +production tag system. + +## Out of scope + +- **Tags / faceted search** — deserves a first-class table with foreign + keys and a search index. The metadata API can hold tags as values, + but querying "all files tagged `alpha`" via the metadata table is a + full scan. Don't build features that need indexed tag queries on top + of this. +- **Live properties** (RFC 4918 §15) — `getcontentlength`, + `creationdate`, `getetag`, etc. are server-computed. The REST API + exposes only dead properties; live properties are derived from the + resource and surface through their existing endpoints. +- **Bulk write** — phase 2 could add a batch endpoint if needed, but + initial design ships one-at-a-time. Bulk read (list) is already + there. +- **Versioning / history** — `updated_at` + `updated_by` give an audit + timestamp but not history. If someone wants "show me the previous + caption", that's a separate append-only journal table. +- **WebDAV PROPPATCH size enforcement** mentioned above as "should + adopt the same limit" — actually applying the limit to PROPPATCH is + a separate small change, deferable. + +## Open questions left for implementation + +1. **Sub-resource name** — `/metadata` matches REST conventions and + reads naturally to API consumers; `/properties` would be more + WebDAV-faithful but users don't know what "dead properties" means. + Locked in: `/metadata`. +2. **Permission for empty list** — GET on a resource with zero metadata + returns `{ properties: [] }` and 200, OR 404? RFC 4918 §9.1 says + PROPFIND on a resource that exists but has no requested properties + is 207 with empty ``. REST should mirror: 200 with empty + array. 404 only when the underlying resource doesn't exist (or + anti-enum 404 on no-Read). +3. **Listing order** — alphabetical by `(namespace, name)`, or by + `updated_at DESC`? Probably the former (stable, deterministic). +4. **Caching** — `ETag` on the list response? Cheap if the underlying + resource already emits one; we could derive a sub-ETag from + `MAX(updated_at)` across the metadata rows. Defer until UI asks. +5. **NC DAV wiring** (see memory note + `project-nc-webdav-dead-props-unwired.md`) — the read API would + work over the unwired NC surface trivially since REST and NC DAV + read the same store; the unwired bit is just PROPPATCH/PROPFIND on + the NC URL prefix. Worth doing alongside phase 1 read so the cross- + protocol story is complete on day one. +6. **Search endpoint shape** (phase 3, not phase 1) — the cross- + resource lookup `/api/search/metadata?...` will need pagination, + sort, and resource-kind filter; design when the first concrete + use case lands. Don't speculatively build it. + +## Verification (phase 1) + +1. Migration adds `updated_by`; downgrade leaves data intact (no + `DROP COLUMN` on rollback — just leave it; harmless). +2. `cargo check` green; `cargo clippy --all-features --all-targets -D + warnings` green. +3. New Hurl scenario `tests/api/metadata_api.hurl`: + - PUT a file via WebDAV, PROPPATCH a marker → 207. + - GET `/api/files/{id}/metadata/oxi%3Atest/marker` → 200 with the + value. + - GET `/api/files/{id}/metadata` → 200 with list containing the + marker. + - Asserts `updated_by` field is the authenticated user id. + - GET against a foreign user's file → 404 (anti-enum). + - GET `/api/folders/{id}/metadata` round-trip on a folder PROPPATCH. +4. The existing `webdav_dead_properties.hurl` continues to pass — + read-only REST shouldn't perturb any existing write path. + +## Verification (phase 2) + +To be expanded when phase 2 starts. At minimum: + +- PUT writes round-trip via PROPFIND. +- PUT to denylisted namespace → 403. +- PUT exceeding per-value cap → 413. +- PUT exceeding per-resource cap → 413. +- DELETE removes via PROPFIND verification. +- PUT/DELETE without Update permission → 403. + +## Memory notes to write when this lands + +- `project_metadata_api.md` — phase shipped, what's still open. +- Update `project_webdav_dead_properties_drive_rekey.md` to mention + this API as the consumer that justified the id-rekey effort. +- If the namespace denylist is contentious, capture it as + `feedback_metadata_namespace_denylist.md`.