From 1c43facd043569f6ea0a3497c9216cabd120ff3b Mon Sep 17 00:00:00 2001 From: AptS-1738 Date: Sun, 5 Jul 2026 16:24:30 +0800 Subject: [PATCH 1/6] refactor(webdav): extract HTTP validators and add date precondition support - Add `src/utils/http_validators.rs` with shared HTTP validation helpers: - `if_match_header_matches`: strong ETag comparison per RFC 9110 - `if_none_match_header_matches`: weak ETag comparison per RFC 9110 - `format_http_date` / `parse_http_date`: HTTP-date serialization and parsing - `http_date_epoch_seconds`: SystemTime to epoch seconds for date comparison - Remove duplicated ETag helpers from `webdav/protocol.rs` and `file_service/common.rs`, replacing them with calls to `http_validators` - Add `evaluate_http_download_preconditions` to `webdav/protocol.rs` supporting `If-Match`, `If-None-Match`, `If-Modified-Since`, and `If-Unmodified-Since` with correct header precedence rules - Update `webdav/transfer/mod.rs` to use `evaluate_http_download_preconditions` and include `Last-Modified` in GET/HEAD responses - Add `get_range` stub to `TrailingErrorStreamDriver` in handler tests and add test `handle_get_range_uses_driver_range_without_opening_full_stream` - Add integration tests for `Last-Modified` header presence, `If-Modified-Since`, and `If-Unmodified-Since` preconditions Closed #362 --- src/services/file_service/common.rs | 7 +- src/utils/http_validators.rs | 174 ++++++++++++++++++++++++++ src/utils/mod.rs | 1 + src/webdav/handler_tests/mod.rs | 56 +++++++++ src/webdav/mod.rs | 4 +- src/webdav/protocol.rs | 182 ++++++++++++---------------- src/webdav/transfer/mod.rs | 21 ++-- tests/test_webdav.rs | 153 +++++++++++++++++++++++ 8 files changed, 481 insertions(+), 117 deletions(-) create mode 100644 src/utils/http_validators.rs diff --git a/src/services/file_service/common.rs b/src/services/file_service/common.rs index 2124ea07c..bd4496a54 100644 --- a/src/services/file_service/common.rs +++ b/src/services/file_service/common.rs @@ -3,6 +3,7 @@ use crate::entities::file; use crate::errors::Result; use crate::services::workspace_storage_service; +use crate::utils::http_validators; const INLINE_SANDBOX_CSP: &str = "sandbox"; @@ -29,10 +30,8 @@ pub(crate) fn ensure_personal_file_scope(file: &file::Model) -> Result<()> { } pub(crate) fn if_none_match_matches_value(if_none_match: &str, etag_value: &str) -> bool { - if_none_match.split(',').any(|value| { - let candidate = value.trim(); - candidate == "*" || candidate.trim_matches('"').eq_ignore_ascii_case(etag_value) - }) + http_validators::if_none_match_header_matches(if_none_match, true, Some(etag_value)) + .unwrap_or(false) } pub(crate) fn if_none_match_matches(if_none_match: &str, blob_hash: &str) -> bool { diff --git a/src/utils/http_validators.rs b/src/utils/http_validators.rs new file mode 100644 index 000000000..d7ff6e845 --- /dev/null +++ b/src/utils/http_validators.rs @@ -0,0 +1,174 @@ +//! Shared HTTP validator helpers. + +use std::time::{SystemTime, UNIX_EPOCH}; + +use actix_web::http::header; + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum EtagListError { + Empty, +} + +pub(crate) fn format_http_date(time: SystemTime) -> String { + chrono::DateTime::::from(time) + .format("%a, %d %b %Y %H:%M:%S GMT") + .to_string() +} + +pub(crate) fn parse_http_date(value: &str) -> Result { + value + .parse::() + .map(SystemTime::from) + .map_err(|_| ()) +} + +pub(crate) fn http_date_epoch_seconds(time: SystemTime) -> i128 { + match time.duration_since(UNIX_EPOCH) { + Ok(duration) => i128::from(duration.as_secs()), + Err(error) => -i128::from(error.duration().as_secs()), + } +} + +pub(crate) fn if_match_header_matches( + raw: &str, + resource_exists: bool, + current_etag: Option<&str>, +) -> Result { + let raw = raw.trim(); + if raw == "*" { + return Ok(resource_exists); + } + let Some(current_etag) = current_etag else { + return Ok(false); + }; + let mut saw_tag = false; + for candidate in raw + .split(',') + .map(str::trim) + .filter(|value| !value.is_empty()) + { + saw_tag = true; + if is_weak_etag(candidate) { + continue; + } + if strong_etag_matches(candidate, current_etag) { + return Ok(true); + } + } + if saw_tag { + Ok(false) + } else { + Err(EtagListError::Empty) + } +} + +pub(crate) fn if_none_match_header_matches( + raw: &str, + resource_exists: bool, + current_etag: Option<&str>, +) -> Result { + let raw = raw.trim(); + if raw == "*" { + return Ok(resource_exists); + } + let Some(current_etag) = current_etag else { + return Ok(false); + }; + let mut saw_tag = false; + for candidate in raw + .split(',') + .map(str::trim) + .filter(|value| !value.is_empty()) + { + saw_tag = true; + if etag_matches(candidate, current_etag) { + return Ok(true); + } + } + if saw_tag { + Ok(false) + } else { + Err(EtagListError::Empty) + } +} + +fn etag_matches(header_value: &str, current_etag: &str) -> bool { + let header_value = strip_weak_etag_prefix(header_value.trim()); + let current = strip_weak_etag_prefix(current_etag.trim()); + let header_value = strip_etag_quotes(header_value); + let current = strip_etag_quotes(current); + header_value == current +} + +fn strong_etag_matches(candidate: &str, current_etag: &str) -> bool { + if is_weak_etag(current_etag) { + return false; + } + strip_etag_quotes(candidate.trim()) == strip_etag_quotes(current_etag.trim()) +} + +fn is_weak_etag(value: &str) -> bool { + value + .trim() + .get(..2) + .is_some_and(|prefix| prefix.eq_ignore_ascii_case("W/")) +} + +fn strip_weak_etag_prefix(value: &str) -> &str { + value + .strip_prefix("W/") + .or_else(|| value.strip_prefix("w/")) + .unwrap_or(value) +} + +fn strip_etag_quotes(value: &str) -> &str { + value + .strip_prefix('"') + .and_then(|value| value.strip_suffix('"')) + .unwrap_or(value) +} + +#[cfg(test)] +mod tests { + use super::{ + EtagListError, if_match_header_matches, if_none_match_header_matches, parse_http_date, + }; + + #[test] + fn if_none_match_uses_weak_comparison() { + assert_eq!( + if_none_match_header_matches(r#"W/"etag-1", "etag-2""#, true, Some(r#""etag-1""#)), + Ok(true) + ); + } + + #[test] + fn if_match_requires_strong_comparison() { + assert_eq!( + if_match_header_matches(r#"W/"etag-1""#, true, Some(r#""etag-1""#)), + Ok(false) + ); + assert_eq!( + if_match_header_matches(r#""etag-1""#, true, Some(r#""etag-1""#)), + Ok(true) + ); + } + + #[test] + fn empty_etag_lists_are_invalid() { + assert_eq!( + if_none_match_header_matches(" , ", true, Some("etag")), + Err(EtagListError::Empty) + ); + assert_eq!( + if_match_header_matches(" , ", true, Some("etag")), + Err(EtagListError::Empty) + ); + } + + #[test] + fn parses_http_date() { + assert!(parse_http_date("Sun, 06 Nov 1994 08:49:37 GMT").is_ok()); + assert!(parse_http_date("not a date").is_err()); + } +} diff --git a/src/utils/mod.rs b/src/utils/mod.rs index 033040116..9aa73ce11 100644 --- a/src/utils/mod.rs +++ b/src/utils/mod.rs @@ -3,6 +3,7 @@ pub mod email; pub mod file_classification; pub mod hash; +pub(crate) mod http_validators; pub mod id; pub mod net; pub mod numbers; diff --git a/src/webdav/handler_tests/mod.rs b/src/webdav/handler_tests/mod.rs index 5066a61f4..636a5dd60 100644 --- a/src/webdav/handler_tests/mod.rs +++ b/src/webdav/handler_tests/mod.rs @@ -287,6 +287,7 @@ impl AsyncRead for OneChunkThenErrorReader { #[derive(Clone, Default)] struct TrailingErrorStreamDriver { get_stream_calls: Arc, + get_range_calls: Arc, } #[async_trait] @@ -311,6 +312,16 @@ impl StorageDriver for TrailingErrorStreamDriver { })) } + async fn get_range( + &self, + _path: &str, + _offset: u64, + _length: Option, + ) -> crate::errors::Result> { + self.get_range_calls.fetch_add(1, Ordering::SeqCst); + Ok(Box::new(Cursor::new(b"bc".to_vec()))) + } + async fn delete(&self, _path: &str) -> crate::errors::Result<()> { Ok(()) } @@ -491,6 +502,51 @@ async fn handle_get_returns_response_before_consuming_the_storage_stream() { let _ = std::fs::remove_dir_all(temp_root); } +#[actix_web::test] +async fn handle_get_range_uses_driver_range_without_opening_full_stream() { + let driver = TrailingErrorStreamDriver::default(); + let get_stream_calls = driver.get_stream_calls.clone(); + let get_range_calls = driver.get_range_calls.clone(); + let (state, user, policy, temp_root) = build_webdav_test_state( + DriverType::Local, + crate::types::StoredStoragePolicyOptions::empty(), + Arc::new(driver), + ) + .await; + create_root_file( + &state, + user.id, + policy.id, + "range.txt", + 3, + "files/range.txt", + ) + .await; + + let dav_fs = AsterDavFs::new(state.clone(), user.id, None); + let req = actix_web::test::TestRequest::get() + .uri("/webdav/range.txt") + .insert_header((header::RANGE, "bytes=1-2")) + .to_http_request(); + let lock_system = NoopLockSystem; + let response = handle_get_head(&req, &dav_fs, &lock_system, "/webdav", false).await; + + assert_eq!(response.status(), StatusCode::PARTIAL_CONTENT); + assert_eq!( + get_range_calls.load(Ordering::SeqCst), + 1, + "range GET should delegate to StorageDriver::get_range" + ); + assert_eq!( + get_stream_calls.load(Ordering::SeqCst), + 0, + "range GET must not open a full-object stream" + ); + + drop(state); + let _ = std::fs::remove_dir_all(temp_root); +} + #[actix_web::test] async fn propfind_href_is_percent_encoded_and_xml_parseable() { let driver = CountingDirectUploadDriver::default(); diff --git a/src/webdav/mod.rs b/src/webdav/mod.rs index 094a8aa26..e27ebeaa1 100644 --- a/src/webdav/mod.rs +++ b/src/webdav/mod.rs @@ -465,9 +465,7 @@ pub(crate) fn parent_relative_path(relative: &str) -> Option { } pub(crate) fn format_http_date(time: std::time::SystemTime) -> String { - chrono::DateTime::::from(time) - .format("%a, %d %b %Y %H:%M:%S GMT") - .to_string() + crate::utils::http_validators::format_http_date(time) } pub(crate) fn format_creation_date(time: std::time::SystemTime) -> String { diff --git a/src/webdav/protocol.rs b/src/webdav/protocol.rs index f255220cf..046fd4270 100644 --- a/src/webdav/protocol.rs +++ b/src/webdav/protocol.rs @@ -1,8 +1,11 @@ //! WebDAV protocol parsing helpers. +use std::time::SystemTime; + use actix_web::HttpResponse; use actix_web::http::header; +use crate::utils::http_validators; use crate::webdav::dav::{DavFileSystem, DavLockSystem, DavPath, FsError}; use crate::webdav::{decode_relative_path, responses}; @@ -236,14 +239,18 @@ pub(crate) fn evaluate_http_etag_preconditions( ) -> Result { if let Some(value) = headers.get(header::IF_MATCH) { let raw = value.to_str().map_err(|_| invalid_if_match_header())?; - if !if_match_header_matches(raw, resource_exists, current_etag)? { + if !http_validators::if_match_header_matches(raw, resource_exists, current_etag) + .map_err(|_| invalid_if_match_header())? + { return Err(responses::precondition_failed()); } } if let Some(value) = headers.get(header::IF_NONE_MATCH) { let raw = value.to_str().map_err(|_| invalid_if_none_match_header())?; - if if_none_match_header_matches(raw, resource_exists, current_etag)? { + if http_validators::if_none_match_header_matches(raw, resource_exists, current_etag) + .map_err(|_| invalid_if_none_match_header())? + { return if safe_method { Ok(HttpEtagPrecondition::NotModified) } else { @@ -255,6 +262,58 @@ pub(crate) fn evaluate_http_etag_preconditions( Ok(HttpEtagPrecondition::Proceed) } +pub(crate) fn evaluate_http_download_preconditions( + headers: &header::HeaderMap, + current_etag: Option<&str>, + last_modified: Option, +) -> Result { + let has_if_match = headers.contains_key(header::IF_MATCH); + if let Some(value) = headers.get(header::IF_MATCH) { + let raw = value.to_str().map_err(|_| invalid_if_match_header())?; + if !http_validators::if_match_header_matches(raw, true, current_etag) + .map_err(|_| invalid_if_match_header())? + { + return Err(responses::precondition_failed()); + } + } + + if !has_if_match + && let (Some(value), Some(last_modified)) = + (headers.get(header::IF_UNMODIFIED_SINCE), last_modified) + { + let since = parse_http_date_header(value, invalid_if_unmodified_since_header)?; + if http_validators::http_date_epoch_seconds(last_modified) + > http_validators::http_date_epoch_seconds(since) + { + return Err(responses::precondition_failed()); + } + } + + let has_if_none_match = headers.contains_key(header::IF_NONE_MATCH); + if let Some(value) = headers.get(header::IF_NONE_MATCH) { + let raw = value.to_str().map_err(|_| invalid_if_none_match_header())?; + if http_validators::if_none_match_header_matches(raw, true, current_etag) + .map_err(|_| invalid_if_none_match_header())? + { + return Ok(HttpEtagPrecondition::NotModified); + } + } + + if !has_if_none_match + && let (Some(value), Some(last_modified)) = + (headers.get(header::IF_MODIFIED_SINCE), last_modified) + { + let since = parse_http_date_header(value, invalid_if_modified_since_header)?; + if http_validators::http_date_epoch_seconds(last_modified) + <= http_validators::http_date_epoch_seconds(since) + { + return Ok(HttpEtagPrecondition::NotModified); + } + } + + Ok(HttpEtagPrecondition::Proceed) +} + async fn evaluate_if_header( if_header: &IfHeader, dav_fs: &dyn DavFileSystem, @@ -323,10 +382,10 @@ async fn evaluate_if_state_list( lock_tokens.iter().any(|token| token == value) ^ *negated } IfStateCondition::Etag { value, negated } => { - current_etag - .as_deref() - .is_some_and(|etag| etag_matches(value, etag)) - ^ *negated + current_etag.as_deref().is_some_and(|etag| { + http_validators::if_none_match_header_matches(value, true, Some(etag)) + .unwrap_or(false) + }) ^ *negated } }; if !matched { @@ -390,103 +449,12 @@ fn tagged_dav_path( Ok(Some(path)) } -fn etag_matches(header_value: &str, current_etag: &str) -> bool { - let header_value = strip_weak_etag_prefix(header_value.trim()); - let current = strip_weak_etag_prefix(current_etag.trim()); - let header_value = strip_etag_quotes(header_value); - let current = strip_etag_quotes(current); - header_value == current -} - -fn if_match_header_matches( - raw: &str, - resource_exists: bool, - current_etag: Option<&str>, -) -> Result { - let raw = raw.trim(); - if raw == "*" { - return Ok(resource_exists); - } - let Some(current_etag) = current_etag else { - return Ok(false); - }; - let mut saw_tag = false; - for candidate in raw - .split(',') - .map(str::trim) - .filter(|value| !value.is_empty()) - { - saw_tag = true; - if is_weak_etag(candidate) { - continue; - } - if strong_etag_matches(candidate, current_etag) { - return Ok(true); - } - } - if saw_tag { - Ok(false) - } else { - Err(invalid_if_match_header()) - } -} - -fn if_none_match_header_matches( - raw: &str, - resource_exists: bool, - current_etag: Option<&str>, -) -> Result { - let raw = raw.trim(); - if raw == "*" { - return Ok(resource_exists); - } - let Some(current_etag) = current_etag else { - return Ok(false); - }; - let mut saw_tag = false; - for candidate in raw - .split(',') - .map(str::trim) - .filter(|value| !value.is_empty()) - { - saw_tag = true; - if etag_matches(candidate, current_etag) { - return Ok(true); - } - } - if saw_tag { - Ok(false) - } else { - Err(invalid_if_none_match_header()) - } -} - -fn strong_etag_matches(candidate: &str, current_etag: &str) -> bool { - if is_weak_etag(current_etag) { - return false; - } - strip_etag_quotes(candidate.trim()) == strip_etag_quotes(current_etag.trim()) -} - -fn is_weak_etag(value: &str) -> bool { - value - .trim() - .get(..2) - .is_some_and(|prefix| prefix.eq_ignore_ascii_case("W/")) -} - -fn strip_weak_etag_prefix(value: &str) -> &str { - value - .strip_prefix("W/") - .or_else(|| value.strip_prefix("w/")) - .unwrap_or(value) -} - -fn strip_etag_quotes(value: &str) -> &str { - value - .strip_prefix('"') - .and_then(|value| value.strip_suffix('"')) - .unwrap_or(value) +fn parse_http_date_header( + value: &header::HeaderValue, + invalid_response: fn() -> HttpResponse, +) -> Result { + let raw = value.to_str().map_err(|_| invalid_response())?; + http_validators::parse_http_date(raw).map_err(|_| invalid_response()) } fn invalid_destination_header() -> HttpResponse { @@ -505,6 +473,14 @@ fn invalid_if_none_match_header() -> HttpResponse { responses::bad_request_text("Invalid If-None-Match header") } +fn invalid_if_modified_since_header() -> HttpResponse { + responses::bad_request_text("Invalid If-Modified-Since header") +} + +fn invalid_if_unmodified_since_header() -> HttpResponse { + responses::bad_request_text("Invalid If-Unmodified-Since header") +} + fn invalid_overwrite_header() -> HttpResponse { responses::bad_request_text("Invalid Overwrite header") } diff --git a/src/webdav/transfer/mod.rs b/src/webdav/transfer/mod.rs index e24d0f959..b7ff9e890 100644 --- a/src/webdav/transfer/mod.rs +++ b/src/webdav/transfer/mod.rs @@ -8,7 +8,7 @@ use tokio_util::io::ReaderStream; use crate::services::file_service; use crate::webdav::dav::{DavFileSystem, DavLockSystem, FsError, OpenOptions}; use crate::webdav::{ - ensure_parent_unlocked, ensure_system_file_name_allowed, ensure_unlocked, fs, + ensure_parent_unlocked, ensure_system_file_name_allowed, ensure_unlocked, format_http_date, fs, fs_error_response, href_for_relative, protocol, request_origin, request_path, responses, system_file, }; @@ -49,16 +49,22 @@ pub(crate) async fn handle_get_head( if meta.is_dir() { return responses::empty(StatusCode::METHOD_NOT_ALLOWED); } - match protocol::evaluate_http_etag_preconditions( + let last_modified = match meta.modified() { + Ok(modified) => modified, + Err(err) => return fs_error_response(err), + }; + let last_modified_header = format_http_date(last_modified); + let etag = meta.etag(); + match protocol::evaluate_http_download_preconditions( req.headers(), - true, - meta.etag().as_deref(), - true, + etag.as_deref(), + Some(last_modified), ) { Ok(HttpEtagPrecondition::Proceed) => {} Ok(HttpEtagPrecondition::NotModified) => { let mut response = HttpResponse::NotModified(); - if let Some(etag) = meta.etag() { + response.insert_header((header::LAST_MODIFIED, last_modified_header)); + if let Some(etag) = etag { response.insert_header((header::ETAG, format!("\"{etag}\""))); } return response.finish(); @@ -95,10 +101,11 @@ pub(crate) async fn handle_get_head( response.insert_header((header::CONTENT_TYPE, content_type)); response.insert_header(("Accept-Ranges", "bytes")); response.insert_header((header::CONTENT_ENCODING, "identity")); + response.insert_header((header::LAST_MODIFIED, last_modified_header)); if let Some(content_range) = content_range { response.insert_header((header::CONTENT_RANGE, content_range)); } - if let Some(etag) = meta.etag() { + if let Some(etag) = etag { response.insert_header((header::ETAG, format!("\"{etag}\""))); } diff --git a/tests/test_webdav.rs b/tests/test_webdav.rs index be308f165..a2f69344d 100644 --- a/tests/test_webdav.rs +++ b/tests/test_webdav.rs @@ -5127,6 +5127,159 @@ async fn test_webdav_get_head_if_none_match_matching_etag_returns_not_modified() } } +#[actix_web::test] +async fn test_webdav_get_head_return_last_modified() { + let app = setup_with_webdav!(); + let (token, _) = register_and_login!(app); + let auth = create_webdav_basic_auth!(app, token); + + let req = test::TestRequest::put() + .uri("/webdav/http-last-modified.txt") + .insert_header(("Authorization", auth.clone())) + .set_payload("last modified body") + .to_request(); + let resp = test::call_service(&app, req).await; + assert!(resp.status() == 201 || resp.status() == 204); + + let mut observed = Vec::new(); + for method in [actix_web::http::Method::GET, actix_web::http::Method::HEAD] { + let req = test::TestRequest::with_uri("/webdav/http-last-modified.txt") + .method(method.clone()) + .insert_header(("Authorization", auth.clone())) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!( + resp.status(), + actix_web::http::StatusCode::OK, + "{method} should succeed" + ); + let last_modified = resp + .headers() + .get("Last-Modified") + .and_then(|value| value.to_str().ok()) + .expect("GET/HEAD should return Last-Modified") + .to_string(); + assert!( + last_modified + .parse::() + .is_ok(), + "Last-Modified should be a valid HTTP date: {last_modified}" + ); + observed.push(last_modified); + } + + assert_eq!( + observed[0], observed[1], + "GET and HEAD should expose the same Last-Modified for the same resource" + ); +} + +#[actix_web::test] +async fn test_webdav_get_head_http_date_preconditions() { + let app = setup_with_webdav!(); + let (token, _) = register_and_login!(app); + let auth = create_webdav_basic_auth!(app, token); + + let req = test::TestRequest::put() + .uri("/webdav/http-date-preconditions.txt") + .insert_header(("Authorization", auth.clone())) + .set_payload("date validator body") + .to_request(); + let resp = test::call_service(&app, req).await; + assert!(resp.status() == 201 || resp.status() == 204); + + let req = test::TestRequest::default() + .method(actix_web::http::Method::HEAD) + .uri("/webdav/http-date-preconditions.txt") + .insert_header(("Authorization", auth.clone())) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), actix_web::http::StatusCode::OK); + let last_modified = resp + .headers() + .get("Last-Modified") + .and_then(|value| value.to_str().ok()) + .expect("HEAD should return Last-Modified") + .to_string(); + let etag = resp + .headers() + .get("ETag") + .and_then(|value| value.to_str().ok()) + .expect("HEAD should return ETag") + .to_string(); + let old_http_date = "Sun, 06 Nov 1994 08:49:37 GMT"; + + for method in [actix_web::http::Method::GET, actix_web::http::Method::HEAD] { + let req = test::TestRequest::with_uri("/webdav/http-date-preconditions.txt") + .method(method.clone()) + .insert_header(("Authorization", auth.clone())) + .insert_header(("If-Modified-Since", last_modified.clone())) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!( + resp.status(), + actix_web::http::StatusCode::NOT_MODIFIED, + "{method} with matching If-Modified-Since should return 304" + ); + assert_eq!( + resp.headers() + .get("Last-Modified") + .and_then(|value| value.to_str().ok()), + Some(last_modified.as_str()) + ); + } + + let req = test::TestRequest::get() + .uri("/webdav/http-date-preconditions.txt") + .insert_header(("Authorization", auth.clone())) + .insert_header(("If-Modified-Since", old_http_date)) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!( + resp.status(), + actix_web::http::StatusCode::OK, + "stale If-Modified-Since should not suppress the response body" + ); + + let req = test::TestRequest::get() + .uri("/webdav/http-date-preconditions.txt") + .insert_header(("Authorization", auth.clone())) + .insert_header(("If-Unmodified-Since", old_http_date)) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!( + resp.status(), + actix_web::http::StatusCode::PRECONDITION_FAILED, + "If-Unmodified-Since should fail when the resource changed after the supplied timestamp" + ); + + let req = test::TestRequest::get() + .uri("/webdav/http-date-preconditions.txt") + .insert_header(("Authorization", auth.clone())) + .insert_header(("If-None-Match", "\"definitely-wrong\"")) + .insert_header(("If-Modified-Since", last_modified.clone())) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!( + resp.status(), + actix_web::http::StatusCode::OK, + "If-None-Match should take precedence over If-Modified-Since when it is present" + ); + + let req = test::TestRequest::get() + .uri("/webdav/http-date-preconditions.txt") + .insert_header(("Authorization", auth)) + .insert_header(("If-None-Match", etag)) + .insert_header(("If-Modified-Since", old_http_date)) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!( + resp.status(), + actix_web::http::StatusCode::NOT_MODIFIED, + "matching If-None-Match should still return 304 regardless of If-Modified-Since" + ); +} + #[actix_web::test] async fn test_webdav_put_http_if_match_preconditions() { let app = setup_with_webdav!(); From 8f3516d6f93144a5f9a9dc374eed97ef0afab255 Mon Sep 17 00:00:00 2001 From: AptS-1738 Date: Sun, 5 Jul 2026 17:36:55 +0800 Subject: [PATCH 2/6] perf(webdav): optimize PROPFIND with batch property and lock loading Optimize PROPFIND performance for large directories by batching property and lock queries: - Add `find_by_entities` to property repository for batch property lookup across multiple entities - Implement `get_props_many` in DavFileSystem trait for batch dead property loading - Implement `discover_many` in DavLockSystem trait for batch lock discovery - Add PropfindPreload structure to preload all properties and locks before building responses - Skip dead property loading when PROPFIND requests only standard live properties - Skip lock discovery when PROPFIND does not request lockdiscovery - Add integration test verifying large directory PROPFIND performance with live-only properties - Add unit tests validating batch loading behavior and selective property/lock loading closed #363 --- src/db/repository/property_repo.rs | 45 ++- src/webdav/dav.rs | 28 ++ src/webdav/db_lock_system.rs | 46 +++ src/webdav/fs/mod.rs | 93 ++++- src/webdav/props/mod.rs | 613 +++++++++++++++++++++++++---- tests/test_webdav.rs | 68 ++++ 6 files changed, 797 insertions(+), 96 deletions(-) diff --git a/src/db/repository/property_repo.rs b/src/db/repository/property_repo.rs index 94a41850c..b2e67cf36 100644 --- a/src/db/repository/property_repo.rs +++ b/src/db/repository/property_repo.rs @@ -2,7 +2,7 @@ use sea_orm::{ ColumnTrait, ConnectionTrait, DatabaseConnection, EntityTrait, FromQueryResult, PaginatorTrait, - QueryFilter, QuerySelect, Set, TryInsertResult, sea_query::Expr, + QueryFilter, QueryOrder, QuerySelect, Set, TryInsertResult, sea_query::Expr, }; use crate::entities::entity_property::{self, Entity as EntityProperty}; @@ -25,6 +25,49 @@ pub async fn find_by_entity( .map_err(AsterError::from) } +/// 查询多个实体的所有属性。 +pub async fn find_by_entities( + db: &C, + targets: &[(EntityType, i64)], +) -> Result> { + if targets.is_empty() { + return Ok(Vec::new()); + } + + let mut files = Vec::new(); + let mut folders = Vec::new(); + for (entity_type, entity_id) in targets { + match entity_type { + EntityType::File => files.push(*entity_id), + EntityType::Folder => folders.push(*entity_id), + } + } + files.sort_unstable(); + files.dedup(); + folders.sort_unstable(); + folders.dedup(); + + let mut props = Vec::new(); + for (entity_type, ids) in [(EntityType::File, files), (EntityType::Folder, folders)] { + for chunk in ids.chunks(ENTITY_PROPERTY_BATCH_CHUNK_SIZE) { + props.extend( + EntityProperty::find() + .filter(entity_property::Column::EntityType.eq(entity_type)) + .filter(entity_property::Column::EntityId.is_in(chunk.iter().copied())) + .order_by_asc(entity_property::Column::EntityType) + .order_by_asc(entity_property::Column::EntityId) + .order_by_asc(entity_property::Column::Namespace) + .order_by_asc(entity_property::Column::Name) + .all(db) + .await + .map_err(AsterError::from)?, + ); + } + } + + Ok(props) +} + /// 查询实体的单个属性 pub async fn find_by_key( db: &DatabaseConnection, diff --git a/src/webdav/dav.rs b/src/webdav/dav.rs index 4796c8852..2c33e3cfb 100644 --- a/src/webdav/dav.rs +++ b/src/webdav/dav.rs @@ -1,5 +1,6 @@ //! WebDAV 子模块：`dav`。 +use std::collections::HashMap; use std::future::Future; use std::io::SeekFrom; use std::pin::Pin; @@ -253,6 +254,20 @@ pub trait DavFileSystem: Send + Sync { Box::pin(async { Ok(Vec::new()) }) } + fn get_props_many<'a>( + &'a self, + paths: &'a [DavPath], + do_content: bool, + ) -> FsFuture<'a, HashMap>> { + Box::pin(async move { + let mut result = HashMap::with_capacity(paths.len()); + for path in paths { + result.insert(path.clone(), self.get_props(path, do_content).await?); + } + Ok(result) + }) + } + fn patch_props<'a>( &'a self, _path: &'a DavPath, @@ -324,6 +339,19 @@ pub trait DavLockSystem: Send + Sync { fn discover(&self, path: &DavPath) -> LsFuture<'_, Vec>; + fn discover_many<'a>( + &'a self, + paths: &'a [DavPath], + ) -> LsFuture<'a, HashMap>> { + Box::pin(async move { + let mut result = HashMap::with_capacity(paths.len()); + for path in paths { + result.insert(path.clone(), self.discover(path).await); + } + result + }) + } + fn conflicting_locks(&self, path: &DavPath, deep: bool) -> LsFuture<'_, Vec>; fn delete(&self, path: &DavPath) -> LsFuture<'_, Result<(), ()>>; diff --git a/src/webdav/db_lock_system.rs b/src/webdav/db_lock_system.rs index 4958d7fa2..12cbb3431 100644 --- a/src/webdav/db_lock_system.rs +++ b/src/webdav/db_lock_system.rs @@ -1,5 +1,6 @@ //! WebDAV 子模块：`db_lock_system`。 +use std::collections::HashMap; use std::io::Cursor; use std::time::{Duration, SystemTime}; @@ -517,6 +518,51 @@ impl DavLockSystem for DbLockSystem { }) } + fn discover_many<'a>( + &'a self, + paths: &'a [DavPath], + ) -> LsFuture<'a, HashMap>> { + Box::pin(async move { + let now = Utc::now(); + let mut normalized_paths = Vec::with_capacity(paths.len()); + let mut all_ancestors = Vec::new(); + for path in paths { + let normalized = normalize_path(path); + let ancestors = path_ancestors(&normalized); + all_ancestors.extend(ancestors.iter().cloned()); + normalized_paths.push((path.clone(), ancestors)); + } + all_ancestors.sort(); + all_ancestors.dedup(); + + let mut locks = lock_repo::find_ancestors(&self.db, &all_ancestors) + .await + .unwrap_or_default(); + locks.retain(|lock| lock.timeout_at.is_none_or(|timeout_at| timeout_at >= now)); + locks.sort_by_key(|lock| lock.id); + + let mut locks_by_path: HashMap> = HashMap::new(); + for lock in &locks { + locks_by_path + .entry(lock.path.clone()) + .or_default() + .push(model_to_dav_lock(lock)); + } + + let mut result = HashMap::with_capacity(paths.len()); + for (path, ancestors) in normalized_paths { + let mut discovered = Vec::new(); + for ancestor in ancestors { + if let Some(locks) = locks_by_path.get(&ancestor) { + discovered.extend(locks.iter().cloned()); + } + } + result.insert(path, discovered); + } + result + }) + } + fn conflicting_locks(&self, path: &DavPath, deep: bool) -> LsFuture<'_, Vec> { let path_str = normalize_path(path); diff --git a/src/webdav/fs/mod.rs b/src/webdav/fs/mod.rs index ecd765792..b42507cb3 100644 --- a/src/webdav/fs/mod.rs +++ b/src/webdav/fs/mod.rs @@ -681,24 +681,50 @@ impl DavFileSystem for AsterDavFs { .await .map_err(|_| FsError::GeneralFailure)?; - Ok(props - .into_iter() - .filter(|p| !property_service::is_protected_namespace(&p.namespace)) - .map(|p| DavProp { - name: p.name, - prefix: None, - namespace: if p.namespace.is_empty() { - None - } else { - Some(p.namespace) - }, - xml: if do_content { - p.value.map(|v| v.into_bytes()) - } else { - None - }, - }) - .collect()) + Ok(entity_props_to_dav_props(props, do_content)) + }) + } + + fn get_props_many<'a>( + &'a self, + paths: &'a [DavPath], + do_content: bool, + ) -> FsFuture<'a, HashMap>> { + Box::pin(async move { + let mut target_paths: HashMap<(EntityType, i64), Vec> = HashMap::new(); + let mut targets = Vec::new(); + for path in paths { + let Some(target) = + resolve_entity(&self.state, self.scope, path, self.root_folder_id).await + else { + continue; + }; + target_paths.entry(target).or_default().push(path.clone()); + targets.push(target); + } + + let props = property_repo::find_by_entities(self.state.writer_db(), &targets) + .await + .map_err(|_| FsError::GeneralFailure)?; + let mut props_by_target: HashMap<(EntityType, i64), Vec> = HashMap::new(); + for prop in props { + if property_service::is_protected_namespace(&prop.namespace) { + continue; + } + props_by_target + .entry((prop.entity_type, prop.entity_id)) + .or_default() + .push(entity_prop_to_dav_prop(prop, do_content)); + } + + let mut result = HashMap::with_capacity(paths.len()); + for (target, paths) in target_paths { + let props = props_by_target.remove(&target).unwrap_or_default(); + for path in paths { + result.insert(path, props.clone()); + } + } + Ok(result) }) } @@ -799,6 +825,37 @@ impl DavFileSystem for AsterDavFs { } } +fn entity_props_to_dav_props( + props: Vec, + do_content: bool, +) -> Vec { + props + .into_iter() + .filter(|p| !property_service::is_protected_namespace(&p.namespace)) + .map(|p| entity_prop_to_dav_prop(p, do_content)) + .collect() +} + +fn entity_prop_to_dav_prop( + prop: crate::entities::entity_property::Model, + do_content: bool, +) -> DavProp { + DavProp { + name: prop.name, + prefix: None, + namespace: if prop.namespace.is_empty() { + None + } else { + Some(prop.namespace) + }, + xml: if do_content { + prop.value.map(|value| value.into_bytes()) + } else { + None + }, + } +} + /// 从 DavPath 解析出 (entity_type, entity_id) async fn resolve_entity( state: &PrimaryAppState, diff --git a/src/webdav/props/mod.rs b/src/webdav/props/mod.rs index 8e87b6fb3..edd1bb49f 100644 --- a/src/webdav/props/mod.rs +++ b/src/webdav/props/mod.rs @@ -1,7 +1,7 @@ //! WebDAV PROPFIND / PROPPATCH handlers. -use std::collections::BTreeMap; use std::collections::BTreeSet; +use std::collections::{BTreeMap, HashMap}; use std::io::Cursor; use actix_web::http::StatusCode; @@ -11,14 +11,14 @@ use xmltree::{Element, XMLNode}; use crate::services::property_service; use crate::webdav::dav::{ - DavFileSystem, DavLockSystem, DavMetaData, DavPath, DavProp, FsError, ReadDirMeta, + DavFileSystem, DavLock, DavLockSystem, DavMetaData, DavPath, DavProp, FsError, ReadDirMeta, }; use crate::webdav::locks::{lockdiscovery_element, supportedlock_element}; use crate::webdav::protocol::{self, Depth}; use crate::webdav::responses; use crate::webdav::{ child_elements, child_relative_path, dav_element, display_name, ensure_unlocked, - format_creation_date, format_http_date, fs, fs_error_response, href_for_dav_path, + format_creation_date, format_http_date, fs_error_response, href_for_dav_path, href_for_relative, multi_status, request_origin, request_path, status_element, text_element, xml_bytes, xml_response, }; @@ -42,6 +42,12 @@ struct PropfindResource { meta: Box, } +#[derive(Default)] +struct PropfindPreload { + dead_props: HashMap>, + locks: HashMap>, +} + impl RequestedProp { fn from(element: &Element) -> Self { Self { @@ -87,9 +93,56 @@ impl RequestedProp { } } +impl PropfindPreload { + async fn load( + dav_fs: &dyn DavFileSystem, + lock_system: &dyn DavLockSystem, + request_kind: &PropfindKind, + resources: &[PropfindResource], + ) -> Result { + let mut preload = Self::default(); + + if propfind_kind_needs_dead_props(request_kind) { + let paths = resources + .iter() + .filter(|resource| !is_root_resource(resource)) + .map(|resource| resource.path.clone()) + .collect::>(); + preload.dead_props = dav_fs + .get_props_many(&paths, propfind_kind_needs_dead_prop_content(request_kind)) + .await + .map_err(fs_error_response)?; + } + + if propfind_kind_needs_lockdiscovery(request_kind) { + let paths = resources + .iter() + .map(|resource| resource.path.clone()) + .collect::>(); + preload.locks = lock_system.discover_many(&paths).await; + } + + Ok(preload) + } + + fn dead_props_for(&self, resource: &PropfindResource) -> &[DavProp] { + self.dead_props + .get(&resource.path) + .map(Vec::as_slice) + .unwrap_or_default() + } + + fn locks_for(&self, resource: &PropfindResource) -> &[DavLock] { + self.locks + .get(&resource.path) + .map(Vec::as_slice) + .unwrap_or_default() + } +} + pub(crate) async fn handle_propfind( req: &HttpRequest, - dav_fs: &fs::AsterDavFs, + dav_fs: &dyn DavFileSystem, lock_system: &dyn DavLockSystem, prefix: &str, body: &[u8], @@ -128,21 +181,24 @@ pub(crate) async fn handle_propfind( if depth == Depth::Infinity && root_meta.is_dir() { return responses::propfind_finite_depth_response(); } - let mut resources = match collect_propfind_resources(dav_fs, &path, &relative, depth).await { + let resources = match collect_propfind_resources(dav_fs, &path, &relative, depth).await { Ok(resources) => resources, Err(err) => return fs_error_response(err), }; + let preload = match PropfindPreload::load(dav_fs, lock_system, &request_kind, &resources).await + { + Ok(preload) => preload, + Err(resp) => return resp, + }; let mut multistatus = dav_element("multistatus"); multistatus .attributes .insert("xmlns:D".to_string(), "DAV:".to_string()); - for resource in resources.drain(..) { + for resource in resources { let response = - match build_propfind_response(dav_fs, lock_system, prefix, &request_kind, resource) - .await - { + match build_propfind_response(prefix, &request_kind, &preload, resource).await { Ok(response) => response, Err(resp) => return resp, }; @@ -154,7 +210,7 @@ pub(crate) async fn handle_propfind( pub(crate) async fn handle_proppatch( req: &HttpRequest, - dav_fs: &fs::AsterDavFs, + dav_fs: &dyn DavFileSystem, lock_system: &dyn DavLockSystem, prefix: &str, body: &[u8], @@ -350,7 +406,7 @@ fn invalid_proppatch_body() -> HttpResponse { } async fn collect_propfind_resources( - dav_fs: &fs::AsterDavFs, + dav_fs: &dyn DavFileSystem, path: &DavPath, relative: &str, depth: Depth, @@ -383,10 +439,9 @@ async fn collect_propfind_resources( } async fn build_propfind_response( - dav_fs: &fs::AsterDavFs, - lock_system: &dyn DavLockSystem, prefix: &str, request_kind: &PropfindKind, + preload: &PropfindPreload, resource: PropfindResource, ) -> Result { let mut response = dav_element("response"); @@ -397,16 +452,13 @@ async fn build_propfind_response( let propstats = match request_kind { PropfindKind::AllProp { include } => { - all_propstat_elements(dav_fs, lock_system, prefix, &resource, include).await? + all_propstat_elements(prefix, &resource, include, preload).await? } PropfindKind::PropName => { - vec![( - StatusCode::OK, - prop_name_elements(dav_fs, lock_system, prefix, &resource).await?, - )] + vec![(StatusCode::OK, prop_name_elements(&resource, preload))] } PropfindKind::Prop(requested) => { - requested_prop_elements(dav_fs, lock_system, prefix, &resource, requested).await? + requested_prop_elements(prefix, &resource, requested, preload).await? } }; @@ -430,10 +482,9 @@ async fn build_propfind_response( } async fn all_prop_elements( - dav_fs: &fs::AsterDavFs, - lock_system: &dyn DavLockSystem, prefix: &str, resource: &PropfindResource, + preload: &PropfindPreload, ) -> Result<(Vec, BTreeSet<(String, Option)>), HttpResponse> { let mut props = standard_prop_name_list(resource) .into_iter() @@ -447,19 +498,10 @@ async fn all_prop_elements( .iter() .map(RequestedProp::key) .collect::>(); - let custom_props = if is_root_resource(resource) { - Vec::new() - } else { - dav_fs - .get_props(&resource.path, true) - .await - .map_err(fs_error_response)? - }; + let custom_props = preload.dead_props_for(resource); let mut elements = Vec::new(); for requested in props.drain(..) { - if let Some(element) = - standard_prop_element(lock_system, prefix, resource, &requested).await? - { + if let Some(element) = standard_prop_element(prefix, resource, &requested, preload).await? { elements.push(element); } } @@ -471,14 +513,12 @@ async fn all_prop_elements( } async fn all_propstat_elements( - dav_fs: &fs::AsterDavFs, - lock_system: &dyn DavLockSystem, prefix: &str, resource: &PropfindResource, include: &[RequestedProp], + preload: &PropfindPreload, ) -> Result)>, HttpResponse> { - let (all_props, all_prop_keys) = - all_prop_elements(dav_fs, lock_system, prefix, resource).await?; + let (all_props, all_prop_keys) = all_prop_elements(prefix, resource, preload).await?; if include.is_empty() { return Ok(vec![(StatusCode::OK, all_props)]); } @@ -493,8 +533,7 @@ async fn all_propstat_elements( } let mut result = vec![(StatusCode::OK, all_props)]; - let requested = - requested_prop_elements(dav_fs, lock_system, prefix, resource, &include).await?; + let requested = requested_prop_elements(prefix, resource, &include, preload).await?; for (status, props) in requested { if !props.is_empty() { result.push((status, props)); @@ -503,12 +542,7 @@ async fn all_propstat_elements( Ok(result) } -async fn prop_name_elements( - dav_fs: &fs::AsterDavFs, - lock_system: &dyn DavLockSystem, - prefix: &str, - resource: &PropfindResource, -) -> Result, HttpResponse> { +fn prop_name_elements(resource: &PropfindResource, preload: &PropfindPreload) -> Vec { let mut elements = Vec::new(); for name in standard_prop_name_list(resource) { let requested = RequestedProp { @@ -516,24 +550,11 @@ async fn prop_name_elements( namespace: Some("DAV:".to_string()), prefix: Some("D".to_string()), }; - if standard_prop_element(lock_system, prefix, resource, &requested) - .await? - .is_some() - { - elements.push(requested.empty_element()); - } + elements.push(requested.empty_element()); } - let custom_props = if is_root_resource(resource) { - Vec::new() - } else { - dav_fs - .get_props(&resource.path, false) - .await - .map_err(fs_error_response)? - }; - for prop in custom_props { + for prop in preload.dead_props_for(resource) { elements.push(prop_element( - &prop, + prop, Some(&RequestedProp { name: prop.name.clone(), namespace: prop.namespace.clone(), @@ -541,24 +562,16 @@ async fn prop_name_elements( }), )); } - Ok(elements) + elements } async fn requested_prop_elements( - dav_fs: &fs::AsterDavFs, - lock_system: &dyn DavLockSystem, prefix: &str, resource: &PropfindResource, requested: &[RequestedProp], + preload: &PropfindPreload, ) -> Result)>, HttpResponse> { - let custom_props = if is_root_resource(resource) { - Vec::new() - } else { - dav_fs - .get_props(&resource.path, true) - .await - .map_err(fs_error_response)? - }; + let custom_props = preload.dead_props_for(resource); let mut ok = Vec::new(); let mut missing = Vec::new(); @@ -568,7 +581,7 @@ async fn requested_prop_elements( continue; } - if let Some(element) = standard_prop_element(lock_system, prefix, resource, prop).await? { + if let Some(element) = standard_prop_element(prefix, resource, prop, preload).await? { ok.push(element); continue; } @@ -593,11 +606,49 @@ async fn requested_prop_elements( Ok(result) } +fn requested_props_may_need_dead_lookup(requested: &[RequestedProp]) -> bool { + requested.iter().any(requested_prop_may_be_dead_property) +} + +fn propfind_kind_needs_dead_props(kind: &PropfindKind) -> bool { + match kind { + PropfindKind::AllProp { .. } | PropfindKind::PropName => true, + PropfindKind::Prop(requested) => requested_props_may_need_dead_lookup(requested), + } +} + +fn propfind_kind_needs_dead_prop_content(kind: &PropfindKind) -> bool { + !matches!(kind, PropfindKind::PropName) +} + +fn propfind_kind_needs_lockdiscovery(kind: &PropfindKind) -> bool { + match kind { + PropfindKind::AllProp { .. } => true, + PropfindKind::PropName => false, + PropfindKind::Prop(requested) => requested.iter().any(is_lockdiscovery_prop), + } +} + +fn requested_prop_may_be_dead_property(prop: &RequestedProp) -> bool { + if prop.is_system_namespace() { + return false; + } + match prop.namespace.as_deref() { + Some("DAV:") => false, + Some(_) => true, + None => !is_standard_live_prop_name(&prop.name), + } +} + +fn is_lockdiscovery_prop(prop: &RequestedProp) -> bool { + prop.namespace.as_deref().unwrap_or("DAV:") == "DAV:" && prop.name == "lockdiscovery" +} + async fn standard_prop_element( - lock_system: &dyn DavLockSystem, prefix: &str, resource: &PropfindResource, requested: &RequestedProp, + preload: &PropfindPreload, ) -> Result, HttpResponse> { if requested.namespace.as_deref().unwrap_or("DAV:") != "DAV:" { return Ok(None); @@ -653,10 +704,10 @@ async fn standard_prop_element( let supported = supportedlock_element(); Ok(Some(supported)) } - "lockdiscovery" => { - let locks = lock_system.discover(&resource.path).await; - Ok(Some(lockdiscovery_element(&locks, prefix))) - } + "lockdiscovery" => Ok(Some(lockdiscovery_element( + preload.locks_for(resource), + prefix, + ))), _ => Ok(None), } } @@ -776,6 +827,20 @@ fn standard_prop_name_list(resource: &PropfindResource) -> Vec<&'static str> { props } +fn is_standard_live_prop_name(name: &str) -> bool { + matches!( + name, + "displayname" + | "resourcetype" + | "getcontentlength" + | "getlastmodified" + | "creationdate" + | "getetag" + | "lockdiscovery" + | "supportedlock" + ) +} + fn default_prefix(namespace: Option<&str>) -> &str { match namespace { Some("DAV:") => "D", @@ -787,3 +852,397 @@ fn default_prefix(namespace: Option<&str>) -> &str { fn should_declare_namespace(prefix: &str, namespace: &str) -> bool { namespace != "DAV:" || prefix != "D" } + +#[cfg(test)] +mod tests { + use std::collections::HashMap; + use std::sync::{ + Arc, + atomic::{AtomicUsize, Ordering}, + }; + use std::time::SystemTime; + + use actix_web::body::to_bytes; + use actix_web::http::{Method, StatusCode, header}; + use actix_web::test::TestRequest; + use xmltree::Element; + + use super::handle_propfind; + use crate::webdav::dav::{ + DavDirEntry, DavFile, DavFileSystem, DavLock, DavLockError, DavLockSystem, DavMetaData, + DavPath, DavProp, FsError, FsFuture, FsResult, FsStream, LsFuture, OpenOptions, + ReadDirMeta, + }; + + struct PropfindTestFs { + child_count: usize, + get_props_calls: Arc, + } + + struct PropfindTestMeta { + is_dir: bool, + len: u64, + } + + impl DavMetaData for PropfindTestMeta { + fn len(&self) -> u64 { + self.len + } + + fn modified(&self) -> FsResult { + Ok(SystemTime::UNIX_EPOCH) + } + + fn is_dir(&self) -> bool { + self.is_dir + } + + fn etag(&self) -> Option { + Some(if self.is_dir { + "dir-etag".to_string() + } else { + format!("file-etag-{}", self.len) + }) + } + + fn created(&self) -> FsResult { + Ok(SystemTime::UNIX_EPOCH) + } + } + + struct PropfindTestEntry { + name: Vec, + len: u64, + } + + impl DavDirEntry for PropfindTestEntry { + fn name(&self) -> Vec { + self.name.clone() + } + + fn metadata<'a>(&'a self) -> FsFuture<'a, Box> { + Box::pin(async move { + Ok(Box::new(PropfindTestMeta { + is_dir: false, + len: self.len, + }) as Box) + }) + } + } + + impl DavFileSystem for PropfindTestFs { + fn open<'a>( + &'a self, + _path: &'a DavPath, + _options: OpenOptions, + ) -> FsFuture<'a, Box> { + Box::pin(async { Err(FsError::GeneralFailure) }) + } + + fn read_dir<'a>( + &'a self, + path: &'a DavPath, + _meta: ReadDirMeta, + ) -> FsFuture<'a, FsStream>> { + Box::pin(async move { + if path.as_str() != "/" { + return Err(FsError::NotFound); + } + + let entries = (0..self.child_count) + .map(|index| { + Ok(Box::new(PropfindTestEntry { + name: format!("file-{index}.txt").into_bytes(), + len: u64::try_from(index + 1).expect("test index should fit u64"), + }) as Box) + }) + .collect::>(); + Ok(Box::pin(futures::stream::iter(entries)) as FsStream>) + }) + } + + fn metadata<'a>(&'a self, path: &'a DavPath) -> FsFuture<'a, Box> { + Box::pin(async move { + if path.as_str() == "/" { + return Ok(Box::new(PropfindTestMeta { + is_dir: true, + len: 0, + }) as Box); + } + + Ok(Box::new(PropfindTestMeta { + is_dir: false, + len: 1, + }) as Box) + }) + } + + fn create_dir<'a>(&'a self, _path: &'a DavPath) -> FsFuture<'a, ()> { + Box::pin(async { Err(FsError::GeneralFailure) }) + } + + fn remove_dir<'a>(&'a self, _path: &'a DavPath) -> FsFuture<'a, ()> { + Box::pin(async { Err(FsError::GeneralFailure) }) + } + + fn remove_file<'a>(&'a self, _path: &'a DavPath) -> FsFuture<'a, ()> { + Box::pin(async { Err(FsError::GeneralFailure) }) + } + + fn rename<'a>(&'a self, _from: &'a DavPath, _to: &'a DavPath) -> FsFuture<'a, ()> { + Box::pin(async { Err(FsError::GeneralFailure) }) + } + + fn copy<'a>(&'a self, _from: &'a DavPath, _to: &'a DavPath) -> FsFuture<'a, ()> { + Box::pin(async { Err(FsError::GeneralFailure) }) + } + + fn get_props<'a>( + &'a self, + path: &'a DavPath, + do_content: bool, + ) -> FsFuture<'a, Vec> { + Box::pin(async move { + self.get_props_calls.fetch_add(1, Ordering::SeqCst); + if path.as_str() == "/" { + return Ok(Vec::new()); + } + Ok(vec![DavProp { + name: "color".to_string(), + prefix: Some("A".to_string()), + namespace: Some("urn:aster:test".to_string()), + xml: do_content + .then(|| b"blue".to_vec()), + }]) + }) + } + } + + struct PropfindTestLockSystem { + discover_calls: Arc, + discover_many_calls: Arc, + } + + impl DavLockSystem for PropfindTestLockSystem { + fn lock( + &self, + _path: &DavPath, + _principal: Option<&str>, + _owner: Option<&Element>, + _timeout: Option, + _shared: bool, + _deep: bool, + ) -> LsFuture<'_, Result> { + Box::pin(async { Err(DavLockError::Backend) }) + } + + fn unlock(&self, _path: &DavPath, _token: &str) -> LsFuture<'_, Result<(), ()>> { + Box::pin(async { Ok(()) }) + } + + fn refresh( + &self, + _path: &DavPath, + _token: &str, + _timeout: Option, + ) -> LsFuture<'_, Result> { + Box::pin(async { Err(()) }) + } + + fn check( + &self, + _path: &DavPath, + _principal: Option<&str>, + _ignore_principal: bool, + _deep: bool, + _submitted_tokens: &[String], + ) -> LsFuture<'_, Result<(), DavLock>> { + Box::pin(async { Ok(()) }) + } + + fn discover(&self, _path: &DavPath) -> LsFuture<'_, Vec> { + Box::pin(async move { + self.discover_calls.fetch_add(1, Ordering::SeqCst); + Vec::new() + }) + } + + fn discover_many<'a>( + &'a self, + paths: &'a [DavPath], + ) -> LsFuture<'a, HashMap>> { + Box::pin(async move { + self.discover_many_calls.fetch_add(1, Ordering::SeqCst); + paths + .iter() + .map(|path| (path.clone(), Vec::new())) + .collect::>() + }) + } + + fn conflicting_locks(&self, _path: &DavPath, _deep: bool) -> LsFuture<'_, Vec> { + Box::pin(async { Vec::new() }) + } + + fn delete(&self, _path: &DavPath) -> LsFuture<'_, Result<(), ()>> { + Box::pin(async { Ok(()) }) + } + } + + async fn propfind_depth_one(body: &'static str) -> (String, usize, usize, usize) { + const CHILD_COUNT: usize = 24; + + let get_props_calls = Arc::new(AtomicUsize::new(0)); + let discover_calls = Arc::new(AtomicUsize::new(0)); + let discover_many_calls = Arc::new(AtomicUsize::new(0)); + let fs = PropfindTestFs { + child_count: CHILD_COUNT, + get_props_calls: get_props_calls.clone(), + }; + let lock_system = PropfindTestLockSystem { + discover_calls: discover_calls.clone(), + discover_many_calls: discover_many_calls.clone(), + }; + let req = TestRequest::default() + .method(Method::from_bytes(b"PROPFIND").expect("valid method")) + .uri("/webdav/") + .insert_header((header::HeaderName::from_static("depth"), "1")) + .to_http_request(); + + let response = handle_propfind(&req, &fs, &lock_system, "/webdav", body.as_bytes()).await; + assert_eq!(response.status(), StatusCode::MULTI_STATUS); + let body = to_bytes(response.into_body()) + .await + .expect("PROPFIND response body should be readable"); + ( + String::from_utf8(body.to_vec()).expect("PROPFIND body should be utf-8"), + get_props_calls.load(Ordering::SeqCst), + discover_calls.load(Ordering::SeqCst), + discover_many_calls.load(Ordering::SeqCst), + ) + } + + #[actix_web::test] + async fn propfind_depth_one_live_props_do_not_load_dead_properties() { + let body = r#" + + + + + + + + + +"#; + + let (xml, calls, discover_calls, discover_many_calls) = propfind_depth_one(body).await; + + assert_eq!( + calls, 0, + "live-property-only Depth: 1 PROPFIND should not load dead properties: {xml}" + ); + assert_eq!( + xml.matches("").count(), + 25, + "large-directory fixture should include parent plus all children: {xml}" + ); + assert!( + xml.contains("file-23.txt") && xml.contains("getlastmodified"), + "live property response should still include child resources and requested live props: {xml}" + ); + assert_eq!(discover_calls, 0, "live props should not discover locks"); + assert_eq!( + discover_many_calls, 0, + "live props should not batch-discover locks" + ); + } + + #[actix_web::test] + async fn propfind_depth_one_custom_prop_still_loads_dead_properties() { + let body = r#" + + + + + +"#; + + let (xml, calls, _, _) = propfind_depth_one(body).await; + + assert_eq!( + calls, 24, + "custom prop lookup should still load child dead properties for Depth: 1: {xml}" + ); + assert!( + xml.contains("blue"), + "custom dead property should still be returned: {xml}" + ); + } + + #[actix_web::test] + async fn propfind_depth_one_allprop_still_loads_dead_properties() { + let (xml, calls, _, discover_many_calls) = propfind_depth_one("").await; + + assert_eq!( + calls, 24, + "allprop must continue loading child dead properties for Depth: 1: {xml}" + ); + assert!( + xml.contains("blue"), + "allprop should include custom dead properties: {xml}" + ); + assert_eq!( + discover_many_calls, 1, + "allprop should batch-load lockdiscovery once" + ); + } + + #[actix_web::test] + async fn propfind_depth_one_propname_does_not_load_lock_values() { + let body = r#" + + +"#; + + let (xml, _, discover_calls, discover_many_calls) = propfind_depth_one(body).await; + + assert!( + xml.contains(""), + "propname should list lockdiscovery as a live property name: {xml}" + ); + assert_eq!( + discover_calls, 0, + "propname must not load per-resource lock values" + ); + assert_eq!( + discover_many_calls, 0, + "propname must not batch-load lock values" + ); + } + + #[actix_web::test] + async fn propfind_depth_one_lockdiscovery_uses_batch_discovery() { + let body = r#" + + + + +"#; + + let (xml, _, discover_calls, discover_many_calls) = propfind_depth_one(body).await; + + assert!( + xml.contains("lockdiscovery"), + "explicit lockdiscovery request should return lockdiscovery elements: {xml}" + ); + assert_eq!( + discover_calls, 0, + "lockdiscovery should not fall back to per-resource discover calls" + ); + assert_eq!( + discover_many_calls, 1, + "Depth: 1 lockdiscovery should use one batch discovery" + ); + } +} diff --git a/tests/test_webdav.rs b/tests/test_webdav.rs index a2f69344d..df2d18236 100644 --- a/tests/test_webdav.rs +++ b/tests/test_webdav.rs @@ -3499,6 +3499,74 @@ async fn test_webdav_propfind_collection_does_not_report_getcontentlength() { Element::parse(Cursor::new(xml.as_bytes())).expect("PROPFIND response XML should parse"); } +#[actix_web::test] +async fn test_webdav_propfind_depth_one_large_directory_live_props() { + let app = setup_with_webdav!(); + let (token, _) = register_and_login!(app); + let auth = create_webdav_basic_auth!(app, token); + + let req = test::TestRequest::with_uri("/webdav/live-large/") + .method(actix_web::http::Method::from_bytes(b"MKCOL").unwrap()) + .insert_header(("Authorization", auth.clone())) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), 201); + + for index in 0..24 { + let req = test::TestRequest::put() + .uri(&format!("/webdav/live-large/file-{index}.txt")) + .insert_header(("Authorization", auth.clone())) + .set_payload(format!("live prop fixture {index}")) + .to_request(); + let resp = test::call_service(&app, req).await; + assert!( + resp.status() == 201 || resp.status() == 204, + "PUT fixture file {index} should succeed, got {}", + resp.status() + ); + } + + let propfind_body = r#" + + + + + + + + + +"#; + let req = test::TestRequest::with_uri("/webdav/live-large/") + .method(actix_web::http::Method::from_bytes(b"PROPFIND").unwrap()) + .insert_header(("Authorization", auth)) + .insert_header(("Depth", "1")) + .insert_header(("Content-Type", "application/xml")) + .set_payload(propfind_body) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), 207); + let body = test::read_body(resp).await; + let xml = String::from_utf8_lossy(&body); + + assert_eq!( + xml.matches("").count(), + 25, + "Depth: 1 should include the directory plus all children: {xml}" + ); + for index in 0..24 { + assert!( + xml.contains(&format!("file-{index}.txt")), + "Depth: 1 live prop response should include file-{index}.txt: {xml}" + ); + } + assert!( + xml.contains("getlastmodified") && xml.contains("getetag"), + "live prop response should include requested live metadata: {xml}" + ); + Element::parse(Cursor::new(xml.as_bytes())).expect("PROPFIND response XML should parse"); +} + #[actix_web::test] async fn test_webdav_propfind_rejects_invalid_request_grammar() { let app = setup_with_webdav!(); From b7d7544f8c6f5345758dc6f2c02927ec0857f7ee Mon Sep 17 00:00:00 2001 From: AptS-1738 Date: Sun, 5 Jul 2026 17:56:46 +0800 Subject: [PATCH 3/6] refactor(webdav): split path resolution into read and write variants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split resolve_path_cached_in_scope into separate read and write variants to prevent stale cache hits after write operations. Write operations (PUT/MKCOL/COPY/MOVE/DELETE/LOCK) now use writer_db() for authoritative resolution, while read operations (GET/PROPFIND metadata) use reader_db() for better load distribution. - Add resolve_path_cached_for_read_in_scope() that queries reader_db - Keep resolve_path_cached_in_scope() pinned to writer_db for write preconditions - Extract resolve_path_cached_in_scope_with_db() as internal implementation - Add resolve_entity_for_read() helper for property queries on read paths - Update validate_cached_folder_path() to accept explicit db connection - Update validate_cached_parent() to accept explicit db connection - Update load_cached_resolved_node() to accept explicit db connection - Switch open_file(), read_dir(), metadata(), get_props(), get_props_bulk(), has_props(), get_quota() to use reader_db - Add comprehensive test_webdav_immediate_read_after_write_operations() covering PUT→GET, COPY→GET, MOVE→GET, LOCK→PROPFIND, DELETE→GET sequences Closed #364 --- src/webdav/fs/mod.rs | 46 +++++++---- src/webdav/path_resolver.rs | 62 ++++++++++----- tests/test_webdav.rs | 149 ++++++++++++++++++++++++++++++++++++ 3 files changed, 223 insertions(+), 34 deletions(-) diff --git a/src/webdav/fs/mod.rs b/src/webdav/fs/mod.rs index b42507cb3..ab5404c60 100644 --- a/src/webdav/fs/mod.rs +++ b/src/webdav/fs/mod.rs @@ -91,7 +91,7 @@ impl AsterDavFs { offset: Option, length: Option, ) -> Result, FsError> { - let node = path_resolver::resolve_path_cached_in_scope( + let node = path_resolver::resolve_path_cached_for_read_in_scope( &self.state, self.scope, path, @@ -104,7 +104,7 @@ impl AsterDavFs { _ => return Err(FsError::Forbidden), }; - let blob = file_repo::find_blob_by_id(self.state.writer_db(), file.blob_id) + let blob = file_repo::find_blob_by_id(self.state.reader_db(), file.blob_id) .await .map_err(|_| FsError::GeneralFailure)?; let policy = self @@ -279,7 +279,7 @@ impl DavFileSystem for AsterDavFs { _meta: ReadDirMeta, ) -> FsFuture<'a, FsStream>> { Box::pin(async move { - let folder_id = match path_resolver::resolve_path_cached_in_scope( + let folder_id = match path_resolver::resolve_path_cached_for_read_in_scope( &self.state, self.scope, path, @@ -315,7 +315,7 @@ impl DavFileSystem for AsterDavFs { // 批量查询所有 blob（1 次查询替代 N 次） let blob_ids: Vec = files.iter().map(|f| f.blob_id).collect(); - let blobs = file_repo::find_blobs_by_ids(self.state.writer_db(), &blob_ids) + let blobs = file_repo::find_blobs_by_ids(self.state.reader_db(), &blob_ids) .await .map_err(|_| FsError::GeneralFailure)?; @@ -332,7 +332,7 @@ impl DavFileSystem for AsterDavFs { fn metadata<'a>(&'a self, path: &'a DavPath) -> FsFuture<'a, Box> { Box::pin(async move { - let node = path_resolver::resolve_path_cached_in_scope( + let node = path_resolver::resolve_path_cached_for_read_in_scope( &self.state, self.scope, path, @@ -344,7 +344,7 @@ impl DavFileSystem for AsterDavFs { ResolvedNode::Root => Box::new(AsterDavMeta::root()), ResolvedNode::Folder(f) => Box::new(AsterDavMeta::from_folder(&f)), ResolvedNode::File(f) => { - let blob = file_repo::find_blob_by_id(self.state.writer_db(), f.blob_id) + let blob = file_repo::find_blob_by_id(self.state.reader_db(), f.blob_id) .await .map_err(|_| FsError::GeneralFailure)?; Box::new(AsterDavMeta::from_file(&f, &blob)) @@ -620,13 +620,13 @@ impl DavFileSystem for AsterDavFs { Box::pin(async move { let (storage_used, storage_quota) = match self.scope { WorkspaceStorageScope::Personal { user_id } => { - let user = user_repo::find_by_id(self.state.writer_db(), user_id) + let user = user_repo::find_by_id(self.state.reader_db(), user_id) .await .map_err(|_| FsError::GeneralFailure)?; (user.storage_used, user.storage_quota) } WorkspaceStorageScope::Team { team_id, .. } => { - let team = team_repo::find_by_id(self.state.writer_db(), team_id) + let team = team_repo::find_by_id(self.state.reader_db(), team_id) .await .map_err(|_| FsError::GeneralFailure)?; (team.storage_used, team.storage_quota) @@ -654,11 +654,13 @@ impl DavFileSystem for AsterDavFs { ) -> Pin + Send + 'a>> { Box::pin(async move { let (entity_type, entity_id) = - match resolve_entity(&self.state, self.scope, path, self.root_folder_id).await { + match resolve_entity_for_read(&self.state, self.scope, path, self.root_folder_id) + .await + { Some(v) => v, None => return false, }; - property_repo::find_by_entity(self.state.writer_db(), entity_type, entity_id) + property_repo::find_by_entity(self.state.reader_db(), entity_type, entity_id) .await .map(|props| { props @@ -672,12 +674,12 @@ impl DavFileSystem for AsterDavFs { fn get_props<'a>(&'a self, path: &'a DavPath, do_content: bool) -> FsFuture<'a, Vec> { Box::pin(async move { let (entity_type, entity_id) = - resolve_entity(&self.state, self.scope, path, self.root_folder_id) + resolve_entity_for_read(&self.state, self.scope, path, self.root_folder_id) .await .ok_or(FsError::NotFound)?; let props = - property_repo::find_by_entity(self.state.writer_db(), entity_type, entity_id) + property_repo::find_by_entity(self.state.reader_db(), entity_type, entity_id) .await .map_err(|_| FsError::GeneralFailure)?; @@ -695,7 +697,8 @@ impl DavFileSystem for AsterDavFs { let mut targets = Vec::new(); for path in paths { let Some(target) = - resolve_entity(&self.state, self.scope, path, self.root_folder_id).await + resolve_entity_for_read(&self.state, self.scope, path, self.root_folder_id) + .await else { continue; }; @@ -703,7 +706,7 @@ impl DavFileSystem for AsterDavFs { targets.push(target); } - let props = property_repo::find_by_entities(self.state.writer_db(), &targets) + let props = property_repo::find_by_entities(self.state.reader_db(), &targets) .await .map_err(|_| FsError::GeneralFailure)?; let mut props_by_target: HashMap<(EntityType, i64), Vec> = HashMap::new(); @@ -870,6 +873,21 @@ async fn resolve_entity( } } +async fn resolve_entity_for_read( + state: &PrimaryAppState, + scope: WorkspaceStorageScope, + path: &DavPath, + root_folder_id: Option, +) -> Option<(EntityType, i64)> { + match path_resolver::resolve_path_cached_for_read_in_scope(state, scope, path, root_folder_id) + .await + { + Ok(ResolvedNode::File(f)) => Some((EntityType::File, f.id)), + Ok(ResolvedNode::Folder(f)) => Some((EntityType::Folder, f.id)), + _ => None, + } +} + async fn copy_visible_entity_properties( state: &PrimaryAppState, src_entity_type: EntityType, diff --git a/src/webdav/path_resolver.rs b/src/webdav/path_resolver.rs index 78da4fb89..4bccfd08c 100644 --- a/src/webdav/path_resolver.rs +++ b/src/webdav/path_resolver.rs @@ -163,7 +163,7 @@ fn folder_chain_matches_segments( } async fn validate_cached_folder_path( - state: &impl SharedRuntimeState, + db: &impl ConnectionTrait, scope: WorkspaceStorageScope, root_folder_id: Option, folder_id: i64, @@ -171,10 +171,10 @@ async fn validate_cached_folder_path( ) -> Result { let chain = match scope { WorkspaceStorageScope::Personal { user_id } => { - folder_repo::find_ancestor_models(state.writer_db(), user_id, folder_id).await + folder_repo::find_ancestor_models(db, user_id, folder_id).await } WorkspaceStorageScope::Team { team_id, .. } => { - folder_repo::find_team_ancestor_models(state.writer_db(), team_id, folder_id).await + folder_repo::find_team_ancestor_models(db, team_id, folder_id).await } }; let chain = match chain { @@ -200,6 +200,7 @@ async fn validate_cached_folder_path( async fn load_cached_resolved_node( state: &impl SharedRuntimeState, + db: &impl ConnectionTrait, scope: WorkspaceStorageScope, path: &DavPath, root_folder_id: Option, @@ -221,7 +222,7 @@ async fn load_cached_resolved_node( parent_id, name, } => { - let folder = match folder_repo::find_by_id(state.writer_db(), id).await { + let folder = match folder_repo::find_by_id(db, id).await { Ok(folder) => folder, Err(error) if is_missing_entity(&error) => { cache::delete_by_key(state, cache_key).await; @@ -243,7 +244,7 @@ async fn load_cached_resolved_node( cache::delete_by_key(state, cache_key).await; return Ok(None); } - if !validate_cached_folder_path(state, scope, root_folder_id, id, &segments).await? { + if !validate_cached_folder_path(db, scope, root_folder_id, id, &segments).await? { cache::delete_by_key(state, cache_key).await; return Ok(None); } @@ -254,7 +255,7 @@ async fn load_cached_resolved_node( folder_id, name, } => { - let file = match file_repo::find_by_id(state.writer_db(), id).await { + let file = match file_repo::find_by_id(db, id).await { Ok(file) => file, Err(error) if is_missing_entity(&error) => { cache::delete_by_key(state, cache_key).await; @@ -280,14 +281,8 @@ async fn load_cached_resolved_node( cache::delete_by_key(state, cache_key).await; return Ok(None); }; - if !validate_cached_parent( - state, - scope, - root_folder_id, - parent_segments, - file.folder_id, - ) - .await? + if !validate_cached_parent(db, scope, root_folder_id, parent_segments, file.folder_id) + .await? { cache::delete_by_key(state, cache_key).await; return Ok(None); @@ -414,18 +409,46 @@ pub(crate) async fn resolve_path_cached_in_scope( scope: WorkspaceStorageScope, path: &DavPath, root_folder_id: Option, +) -> Result { + // Authoritative path resolution for write-sensitive WebDAV operations. + // PUT/MKCOL/COPY/MOVE/DELETE/LOCK preconditions must observe the writer DB + // so a lagging reader cannot resurrect stale cache entries or miss a fresh + // destination created earlier in the same client workflow. + resolve_path_cached_in_scope_with_db(state, state.writer_db(), scope, path, root_folder_id) + .await +} + +pub(crate) async fn resolve_path_cached_for_read_in_scope( + state: &impl SharedRuntimeState, + scope: WorkspaceStorageScope, + path: &DavPath, + root_folder_id: Option, +) -> Result { + // Metadata, listing, download, and property reads may tolerate normal + // reader lag. Cached entries are still validated against the selected DB + // handle before reuse. + resolve_path_cached_in_scope_with_db(state, state.reader_db(), scope, path, root_folder_id) + .await +} + +async fn resolve_path_cached_in_scope_with_db( + state: &impl SharedRuntimeState, + db: &C, + scope: WorkspaceStorageScope, + path: &DavPath, + root_folder_id: Option, ) -> Result { let cache_key = resolve_path_cache_key(scope, path, root_folder_id); if let Some(cached) = cache::load_resolved_node_by_key(state, &cache_key).await && let Some(node) = - load_cached_resolved_node(state, scope, path, root_folder_id, &cache_key, cached) + load_cached_resolved_node(state, db, scope, path, root_folder_id, &cache_key, cached) .await? { tracing::debug!(?scope, root_folder_id, "webdav path cache hit"); return Ok(node); } - let node = resolve_path_in_scope(state.writer_db(), scope, path, root_folder_id).await?; + let node = resolve_path_in_scope(db, scope, path, root_folder_id).await?; cache::store_resolved_node_by_key(state, &cache_key, &cacheable_node(&node)).await; tracing::debug!(?scope, root_folder_id, "webdav path cache miss"); Ok(node) @@ -475,7 +498,7 @@ pub(crate) async fn resolve_parent_in_scope( } async fn validate_cached_parent( - state: &impl SharedRuntimeState, + db: &impl ConnectionTrait, scope: WorkspaceStorageScope, root_folder_id: Option, parent_segments: &[String], @@ -483,8 +506,7 @@ async fn validate_cached_parent( ) -> Result { match parent_id { Some(parent_id) => { - validate_cached_folder_path(state, scope, root_folder_id, parent_id, parent_segments) - .await + validate_cached_folder_path(db, scope, root_folder_id, parent_id, parent_segments).await } None => Ok(root_folder_id.is_none() && parent_segments.is_empty()), } @@ -520,7 +542,7 @@ pub(crate) async fn resolve_parent_cached_in_scope( let cache_key = resolve_parent_cache_key(scope, path, root_folder_id); if let Some(cached) = cache::load_resolved_parent_by_key(state, &cache_key).await { if validate_cached_parent( - state, + state.writer_db(), scope, root_folder_id, parent_segments, diff --git a/tests/test_webdav.rs b/tests/test_webdav.rs index df2d18236..57effd6a5 100644 --- a/tests/test_webdav.rs +++ b/tests/test_webdav.rs @@ -1914,6 +1914,155 @@ async fn test_webdav_copy_move() { assert_eq!(resp.status(), 200); } +#[actix_web::test] +async fn test_webdav_immediate_read_after_write_operations() { + let app = setup_with_webdav!(); + let (token, _) = register_and_login!(app); + let auth = create_webdav_basic_auth!(app, token); + + let req = test::TestRequest::put() + .uri("/webdav/read-after-write.txt") + .insert_header(("Authorization", auth.clone())) + .set_payload("read after put") + .to_request(); + let resp = test::call_service(&app, req).await; + assert!(resp.status() == 201 || resp.status() == 204); + + let req = test::TestRequest::get() + .uri("/webdav/read-after-write.txt") + .insert_header(("Authorization", auth.clone())) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), 200, "GET must see a just-written file"); + let body = test::read_body(resp).await; + assert_eq!(String::from_utf8_lossy(&body), "read after put"); + + let req = test::TestRequest::with_uri("/webdav/read-after-write.txt") + .method(actix_web::http::Method::from_bytes(b"COPY").unwrap()) + .insert_header(("Authorization", auth.clone())) + .insert_header(("Destination", "/webdav/read-after-copy.txt")) + .to_request(); + let resp = test::call_service(&app, req).await; + assert!(resp.status() == 201 || resp.status() == 204); + + let req = test::TestRequest::get() + .uri("/webdav/read-after-copy.txt") + .insert_header(("Authorization", auth.clone())) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), 200, "GET must see a just-copied file"); + let body = test::read_body(resp).await; + assert_eq!(String::from_utf8_lossy(&body), "read after put"); + + let req = test::TestRequest::with_uri("/webdav/read-after-copy.txt") + .method(actix_web::http::Method::from_bytes(b"MOVE").unwrap()) + .insert_header(("Authorization", auth.clone())) + .insert_header(("Destination", "/webdav/read-after-move.txt")) + .to_request(); + let resp = test::call_service(&app, req).await; + assert!(resp.status() == 201 || resp.status() == 204); + + let req = test::TestRequest::get() + .uri("/webdav/read-after-copy.txt") + .insert_header(("Authorization", auth.clone())) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), 404, "GET must not see a just-moved source"); + + let req = test::TestRequest::get() + .uri("/webdav/read-after-move.txt") + .insert_header(("Authorization", auth.clone())) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), 200, "GET must see a just-moved target"); + + let lock_body = r#" + + + + read-after-write-test +"#; + let req = test::TestRequest::with_uri("/webdav/read-after-move.txt") + .method(actix_web::http::Method::from_bytes(b"LOCK").unwrap()) + .insert_header(("Authorization", auth.clone())) + .insert_header(("Content-Type", "application/xml")) + .insert_header(("Depth", "0")) + .set_payload(lock_body) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), 200); + let lock_token = resp + .headers() + .get("Lock-Token") + .and_then(|value| value.to_str().ok()) + .expect("LOCK response should include Lock-Token") + .to_string(); + + let propfind_lock_body = r#" + + + + +"#; + let req = test::TestRequest::with_uri("/webdav/read-after-move.txt") + .method(actix_web::http::Method::from_bytes(b"PROPFIND").unwrap()) + .insert_header(("Authorization", auth.clone())) + .insert_header(("Depth", "0")) + .insert_header(("Content-Type", "application/xml")) + .set_payload(propfind_lock_body) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), 207); + let body = test::read_body(resp).await; + let xml = String::from_utf8_lossy(&body); + assert!( + xml.contains(lock_token.trim_matches(&['<', '>'][..])), + "PROPFIND lockdiscovery must see a just-created lock: {xml}" + ); + + let req = test::TestRequest::with_uri("/webdav/read-after-move.txt") + .method(actix_web::http::Method::from_bytes(b"UNLOCK").unwrap()) + .insert_header(("Authorization", auth.clone())) + .insert_header(("Lock-Token", lock_token)) + .to_request(); + let resp = test::call_service(&app, req).await; + assert!( + resp.status() == 200 || resp.status() == 204, + "UNLOCK should succeed, got {}", + resp.status() + ); + + let req = test::TestRequest::with_uri("/webdav/read-after-move.txt") + .method(actix_web::http::Method::from_bytes(b"PROPFIND").unwrap()) + .insert_header(("Authorization", auth.clone())) + .insert_header(("Depth", "0")) + .insert_header(("Content-Type", "application/xml")) + .set_payload(propfind_lock_body) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), 207); + let body = test::read_body(resp).await; + let xml = String::from_utf8_lossy(&body); + assert!( + !xml.contains("activelock"), + "PROPFIND lockdiscovery must not see a just-unlocked lock: {xml}" + ); + + let req = test::TestRequest::delete() + .uri("/webdav/read-after-move.txt") + .insert_header(("Authorization", auth.clone())) + .to_request(); + let resp = test::call_service(&app, req).await; + assert!(resp.status() == 200 || resp.status() == 204); + + let req = test::TestRequest::get() + .uri("/webdav/read-after-move.txt") + .insert_header(("Authorization", auth)) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), 404, "GET must not see a just-deleted file"); +} + #[actix_web::test] async fn test_webdav_copy_move_rejects_similar_destination_prefix() { let app = setup_with_webdav!(); From 04893811a737b868f847febb5f00102953882649 Mon Sep 17 00:00:00 2001 From: AptS-1738 Date: Sun, 5 Jul 2026 18:10:45 +0800 Subject: [PATCH 4/6] refactor(cache): optimize cache invalidation with targeted key-based approach Refactor storage change cache invalidation to use targeted key deletion instead of broad prefix-based invalidation where possible. This improves cache efficiency by only invalidating affected entries. Changes: - Add audience-aware WebDAV path cache prefix generation (personal/team/any) - Introduce `CacheInvalidationTargets` struct to support both prefix and key-based invalidation - Replace blanket folder path cache prefix invalidation with targeted key deletion for specific folder changes - Add batch invalidation support for folder path chains - Expose `folder_path_cache_key` for targeted cache invalidation - Update cache invalidation logic to use event context instead of just event kind - Add helper functions for personal/team-scoped cache prefix generation - Remove unnecessary full cache invalidation calls from folder operations - Update all tests to use event-based cache invalidation with proper scope context --- src/services/folder_service/cache.rs | 29 ++- src/services/folder_service/copy.rs | 1 - src/services/folder_service/hierarchy.rs | 7 + src/services/folder_service/mod.rs | 6 +- src/services/folder_service/mutation.rs | 5 +- src/services/storage_change_service.rs | 191 ++++++++++++++---- .../task_service/archive/extract/mod.rs | 6 +- src/services/trash_service/common.rs | 3 +- src/services/webdav_service.rs | 3 +- src/webdav/path_resolver.rs | 16 ++ 10 files changed, 213 insertions(+), 54 deletions(-) diff --git a/src/services/folder_service/cache.rs b/src/services/folder_service/cache.rs index c432a6c1d..9d828f5f0 100644 --- a/src/services/folder_service/cache.rs +++ b/src/services/folder_service/cache.rs @@ -13,7 +13,7 @@ pub(super) struct CachedFolderPathChain { pub(super) chain_ids: Vec, } -fn folder_path_cache_key(folder_id: i64) -> String { +pub(crate) fn folder_path_cache_key(folder_id: i64) -> String { format!("{FOLDER_PATH_CACHE_PREFIX}{folder_id}") } @@ -49,6 +49,18 @@ pub(super) async fn invalidate_folder_path_chain(state: &impl SharedRuntimeState .await; } +pub(super) async fn invalidate_folder_path_chains( + state: &impl SharedRuntimeState, + folder_ids: &[i64], +) { + let keys = folder_ids + .iter() + .copied() + .map(folder_path_cache_key) + .collect::>(); + state.cache().delete_many(&keys).await; +} + pub(crate) async fn invalidate_all_folder_path_chains(state: &impl SharedRuntimeState) { state .cache() @@ -98,4 +110,19 @@ mod tests { assert!(load_folder_path_chain(&state, 11).await.is_none()); } + + #[tokio::test] + async fn folder_path_chain_supports_batch_invalidation() { + let state = CacheOnlyState::new().await; + + store_folder_path_chain(&state, 10, vec![10]).await; + store_folder_path_chain(&state, 11, vec![11]).await; + store_folder_path_chain(&state, 12, vec![12]).await; + + invalidate_folder_path_chains(&state, &[10, 12]).await; + + assert!(load_folder_path_chain(&state, 10).await.is_none()); + assert!(load_folder_path_chain(&state, 11).await.is_some()); + assert!(load_folder_path_chain(&state, 12).await.is_none()); + } } diff --git a/src/services/folder_service/copy.rs b/src/services/folder_service/copy.rs index 2f376f5b4..4e2b55e67 100644 --- a/src/services/folder_service/copy.rs +++ b/src/services/folder_service/copy.rs @@ -329,7 +329,6 @@ pub(crate) async fn copy_folder_in_scope( ) .with_storage_delta(storage_delta), ); - super::invalidate_folder_path_cache(state).await; tracing::debug!( scope = ?scope, src_folder_id = src_id, diff --git a/src/services/folder_service/hierarchy.rs b/src/services/folder_service/hierarchy.rs index e70014d78..9edd5b722 100644 --- a/src/services/folder_service/hierarchy.rs +++ b/src/services/folder_service/hierarchy.rs @@ -21,6 +21,13 @@ pub(crate) async fn invalidate_folder_path_cache(state: &impl SharedRuntimeState cache::invalidate_all_folder_path_chains(state).await; } +pub(crate) async fn invalidate_folder_path_cache_for_ids( + state: &impl SharedRuntimeState, + folder_ids: &[i64], +) { + cache::invalidate_folder_path_chains(state, folder_ids).await; +} + pub(super) async fn load_folder_chain_map( db: &C, folder_ids: &[i64], diff --git a/src/services/folder_service/mod.rs b/src/services/folder_service/mod.rs index 392ede584..1f63b6069 100644 --- a/src/services/folder_service/mod.rs +++ b/src/services/folder_service/mod.rs @@ -41,9 +41,11 @@ pub use mutation::{create, delete, move_folder, set_lock, update}; pub(crate) use access::{ ensure_folder_model_in_scope, ensure_personal_folder_scope, verify_folder_in_scope, }; -pub(crate) use cache::FOLDER_PATH_CACHE_PREFIX; +pub(crate) use cache::{FOLDER_PATH_CACHE_PREFIX, folder_path_cache_key}; pub(crate) use copy::{copy_folder_in_scope, copy_folder_tree_in_scope}; -pub(crate) use hierarchy::{get_ancestors_in_scope, invalidate_folder_path_cache}; +pub(crate) use hierarchy::{ + get_ancestors_in_scope, invalidate_folder_path_cache, invalidate_folder_path_cache_for_ids, +}; pub(crate) use listing::list_in_scope; pub(crate) use mutation::{ admin_set_policy, create_in_scope, delete_in_scope, get_info_in_scope, diff --git a/src/services/folder_service/mutation.rs b/src/services/folder_service/mutation.rs index d759aa0c8..4db84ec0b 100644 --- a/src/services/folder_service/mutation.rs +++ b/src/services/folder_service/mutation.rs @@ -226,7 +226,6 @@ pub(crate) async fn create_in_scope( vec![created.parent_id], ), ); - super::invalidate_folder_path_cache(state).await; tracing::debug!( scope = ?scope, folder_id = created.id, @@ -291,7 +290,7 @@ pub(crate) async fn delete_in_scope( vec![folder.parent_id], ), ); - super::invalidate_folder_path_cache(state).await; + super::invalidate_folder_path_cache_for_ids(state, &[folder.id]).await; tracing::debug!( scope = ?scope, folder_id = folder.id, @@ -444,7 +443,7 @@ pub(crate) async fn update_in_scope( ), ); if name.is_some() || parent_id.is_present() { - super::invalidate_folder_path_cache(state).await; + super::invalidate_folder_path_cache_for_ids(state, &[updated.id]).await; } tracing::debug!( scope = ?scope, diff --git a/src/services/storage_change_service.rs b/src/services/storage_change_service.rs index da2f2806c..b2975d62a 100644 --- a/src/services/storage_change_service.rs +++ b/src/services/storage_change_service.rs @@ -24,6 +24,12 @@ const CACHE_INVALIDATION_RESERVATION_TTL_SECS: u64 = 1; const CACHE_INVALIDATION_RESERVATION_PREFIX: &str = "storage_change_cache_invalidation:"; const AFFECTED_PARENT_LOOKUP_CHUNK_SIZE: usize = 500; +#[derive(Debug, Default, PartialEq, Eq)] +struct CacheInvalidationTargets { + prefixes: Vec, + keys: Vec, +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum StorageChangeAudience { User(i64), @@ -250,7 +256,7 @@ impl StorageChangeEvent { } pub fn publish(state: &S, event: StorageChangeEvent) { - invalidate_storage_change_caches(state.cache().clone(), event.kind); + invalidate_storage_change_caches(state.cache().clone(), &event); if let Err(e) = state.storage_change_tx().send(event) { tracing::debug!("skip storage change broadcast without listeners: {e}"); } @@ -295,9 +301,9 @@ pub(crate) async fn affected_parent_ids_for_entities( Ok(parent_ids) } -fn invalidate_storage_change_caches(cache: Arc, kind: StorageChangeKind) { - let prefixes = cache_invalidation_prefixes(kind); - if prefixes.is_empty() { +fn invalidate_storage_change_caches(cache: Arc, event: &StorageChangeEvent) { + let targets = cache_invalidation_targets(event); + if targets.prefixes.is_empty() && targets.keys.is_empty() { return; } @@ -306,32 +312,63 @@ fn invalidate_storage_change_caches(cache: Arc, kind: StorageC return; }; drop(handle.spawn(async move { - join_all( - prefixes - .into_iter() - .map(|prefix| schedule_cache_prefix_invalidation(cache.clone(), prefix)), - ) - .await; + let CacheInvalidationTargets { prefixes, keys } = targets; + let prefix_invalidations = prefixes + .into_iter() + .map(|prefix| schedule_cache_prefix_invalidation(cache.clone(), prefix)); + join_all(prefix_invalidations).await; + if !keys.is_empty() { + cache.delete_many(&keys).await; + } })); } -fn cache_invalidation_prefixes(kind: StorageChangeKind) -> Vec<&'static str> { - if !kind.invalidates_webdav_path_cache() { - return Vec::new(); +fn cache_invalidation_targets(event: &StorageChangeEvent) -> CacheInvalidationTargets { + if !event.kind.invalidates_webdav_path_cache() { + return CacheInvalidationTargets::default(); } - let mut prefixes = vec![ - crate::webdav::path_resolver::WEBDAV_PATH_CACHE_PREFIX, - crate::webdav::path_resolver::WEBDAV_PARENT_CACHE_PREFIX, - ]; - if kind.invalidates_folder_path_cache() { - prefixes.push(crate::services::folder_service::FOLDER_PATH_CACHE_PREFIX); + let mut targets = CacheInvalidationTargets { + prefixes: webdav_path_cache_invalidation_prefixes(event.audience), + keys: Vec::new(), + }; + if event.kind.invalidates_folder_path_cache() { + if event.kind == StorageChangeKind::SyncRequired { + targets + .prefixes + .push(crate::services::folder_service::FOLDER_PATH_CACHE_PREFIX.to_string()); + } else { + targets.keys.extend( + event + .folder_ids + .iter() + .copied() + .map(crate::services::folder_service::folder_path_cache_key), + ); + } } - prefixes + targets } -async fn schedule_cache_prefix_invalidation(cache: Arc, prefix: &'static str) { - let reservation_key = cache_invalidation_reservation_key(prefix); +fn webdav_path_cache_invalidation_prefixes(audience: StorageChangeAudience) -> Vec { + match audience { + StorageChangeAudience::User(user_id) => vec![ + crate::webdav::path_resolver::path_cache_personal_prefix(user_id), + crate::webdav::path_resolver::parent_cache_personal_prefix(user_id), + ], + StorageChangeAudience::Team(team_id) => vec![ + crate::webdav::path_resolver::path_cache_team_prefix(team_id), + crate::webdav::path_resolver::parent_cache_team_prefix(team_id), + ], + StorageChangeAudience::Any => vec![ + crate::webdav::path_resolver::WEBDAV_PATH_CACHE_PREFIX.to_string(), + crate::webdav::path_resolver::WEBDAV_PARENT_CACHE_PREFIX.to_string(), + ], + } +} + +async fn schedule_cache_prefix_invalidation(cache: Arc, prefix: String) { + let reservation_key = cache_invalidation_reservation_key(&prefix); if !cache .set_bytes_if_absent( &reservation_key, @@ -344,7 +381,7 @@ async fn schedule_cache_prefix_invalidation(cache: Arc, prefix } tokio::time::sleep(CACHE_INVALIDATION_COALESCE_DELAY).await; - cache.invalidate_prefix(prefix).await; + cache.invalidate_prefix(&prefix).await; cache.delete(&reservation_key).await; } @@ -428,51 +465,106 @@ mod tests { } #[test] - fn file_changes_only_invalidate_webdav_path_prefixes() { - let prefixes = super::cache_invalidation_prefixes(StorageChangeKind::FileCreated); + fn personal_file_changes_only_invalidate_personal_webdav_path_prefixes() { + let event = StorageChangeEvent::new( + StorageChangeKind::FileCreated, + WorkspaceStorageScope::Personal { user_id: 11 }, + vec![1], + vec![], + vec![None], + ); + let targets = super::cache_invalidation_targets(&event); + + assert_eq!( + targets.prefixes, + vec![ + crate::webdav::path_resolver::path_cache_personal_prefix(11), + crate::webdav::path_resolver::parent_cache_personal_prefix(11), + ] + ); + assert!(targets.keys.is_empty()); + } + + #[test] + fn team_file_changes_invalidate_team_webdav_path_prefixes() { + let event = StorageChangeEvent::new( + StorageChangeKind::FileUpdated, + WorkspaceStorageScope::Team { + team_id: 42, + actor_user_id: 11, + }, + vec![1], + vec![], + vec![Some(3)], + ); + let targets = super::cache_invalidation_targets(&event); assert_eq!( - prefixes, + targets.prefixes, vec![ - crate::webdav::path_resolver::WEBDAV_PATH_CACHE_PREFIX, - crate::webdav::path_resolver::WEBDAV_PARENT_CACHE_PREFIX, + crate::webdav::path_resolver::path_cache_team_prefix(42), + crate::webdav::path_resolver::parent_cache_team_prefix(42), ] ); + assert!(targets.keys.is_empty()); } #[test] - fn folder_changes_also_invalidate_folder_path_prefix() { - let prefixes = super::cache_invalidation_prefixes(StorageChangeKind::FolderUpdated); + fn folder_changes_invalidate_changed_folder_path_keys() { + let event = StorageChangeEvent::new( + StorageChangeKind::FolderUpdated, + WorkspaceStorageScope::Personal { user_id: 11 }, + vec![], + vec![7, 9], + vec![Some(3)], + ); + let targets = super::cache_invalidation_targets(&event); assert_eq!( - prefixes, + targets.prefixes, + vec![ + crate::webdav::path_resolver::path_cache_personal_prefix(11), + crate::webdav::path_resolver::parent_cache_personal_prefix(11), + ] + ); + assert_eq!( + targets.keys, vec![ - crate::webdav::path_resolver::WEBDAV_PATH_CACHE_PREFIX, - crate::webdav::path_resolver::WEBDAV_PARENT_CACHE_PREFIX, - crate::services::folder_service::FOLDER_PATH_CACHE_PREFIX, + crate::services::folder_service::folder_path_cache_key(7), + crate::services::folder_service::folder_path_cache_key(9), ] ); } #[test] fn sync_required_includes_folder_path_prefix() { - let prefixes = super::cache_invalidation_prefixes(StorageChangeKind::SyncRequired); + let event = StorageChangeEvent::sync_required(); + let targets = super::cache_invalidation_targets(&event); assert_eq!( - prefixes, + targets.prefixes, vec![ - crate::webdav::path_resolver::WEBDAV_PATH_CACHE_PREFIX, - crate::webdav::path_resolver::WEBDAV_PARENT_CACHE_PREFIX, - crate::services::folder_service::FOLDER_PATH_CACHE_PREFIX, + crate::webdav::path_resolver::WEBDAV_PATH_CACHE_PREFIX.to_string(), + crate::webdav::path_resolver::WEBDAV_PARENT_CACHE_PREFIX.to_string(), + crate::services::folder_service::FOLDER_PATH_CACHE_PREFIX.to_string(), ] ); + assert!(targets.keys.is_empty()); } #[test] fn tag_changes_do_not_invalidate_webdav_path_caches() { - let prefixes = super::cache_invalidation_prefixes(StorageChangeKind::TagAssignmentChanged); + let event = StorageChangeEvent::new( + StorageChangeKind::TagAssignmentChanged, + WorkspaceStorageScope::Personal { user_id: 11 }, + vec![1], + vec![], + vec![None], + ); + let targets = super::cache_invalidation_targets(&event); - assert!(prefixes.is_empty()); + assert!(targets.prefixes.is_empty()); + assert!(targets.keys.is_empty()); } #[test] @@ -481,9 +573,20 @@ mod tests { StorageChangeKind::LockCreated, StorageChangeKind::LockDeleted, ] { - let prefixes = super::cache_invalidation_prefixes(kind); - - assert!(prefixes.is_empty()); + let event = StorageChangeEvent::new( + kind, + WorkspaceStorageScope::Team { + team_id: 42, + actor_user_id: 11, + }, + vec![1], + vec![], + vec![Some(3)], + ); + let targets = super::cache_invalidation_targets(&event); + + assert!(targets.prefixes.is_empty()); + assert!(targets.keys.is_empty()); } } } diff --git a/src/services/task_service/archive/extract/mod.rs b/src/services/task_service/archive/extract/mod.rs index cc3b22304..94314758a 100644 --- a/src/services/task_service/archive/extract/mod.rs +++ b/src/services/task_service/archive/extract/mod.rs @@ -374,7 +374,11 @@ async fn cleanup_created_extract_root( "failed to delete partially imported archive shares: {error}" ); } - crate::services::folder_service::invalidate_folder_path_cache(state).await; + crate::services::folder_service::invalidate_folder_path_cache_for_ids( + state, + &folder_ids, + ) + .await; if let Err(error) = crate::db::repository::folder_repo::delete_many(state.writer_db(), &folder_ids) .await diff --git a/src/services/trash_service/common.rs b/src/services/trash_service/common.rs index b942a7ab1..541f39662 100644 --- a/src/services/trash_service/common.rs +++ b/src/services/trash_service/common.rs @@ -271,7 +271,8 @@ pub(super) async fn purge_folder_forest_in_resource_scope( .await; crate::services::share_service::invalidate_all_share_token_record_cache(state).await; } - crate::services::folder_service::invalidate_folder_path_cache(state).await; + crate::services::folder_service::invalidate_folder_path_cache_for_ids(state, &all_folder_ids) + .await; folder_repo::delete_many(state.writer_db(), &all_folder_ids).await?; if emit_storage_event { storage_change_service::publish( diff --git a/src/services/webdav_service.rs b/src/services/webdav_service.rs index 05dc351ac..aebf45158 100644 --- a/src/services/webdav_service.rs +++ b/src/services/webdav_service.rs @@ -145,7 +145,8 @@ pub async fn purge_folder_tree( .await; crate::services::share_service::invalidate_all_share_token_record_cache(state).await; } - crate::services::folder_service::invalidate_folder_path_cache(state).await; + crate::services::folder_service::invalidate_folder_path_cache_for_ids(state, &all_folder_ids) + .await; folder_repo::delete_many(state.writer_db(), &all_folder_ids).await?; tracing::debug!( user_id, diff --git a/src/webdav/path_resolver.rs b/src/webdav/path_resolver.rs index 4bccfd08c..b0421f8ad 100644 --- a/src/webdav/path_resolver.rs +++ b/src/webdav/path_resolver.rs @@ -104,6 +104,22 @@ pub(super) fn resolve_parent_cache_key( ) } +pub(crate) fn path_cache_personal_prefix(user_id: i64) -> String { + format!("{WEBDAV_PATH_CACHE_PREFIX}personal:{user_id}:") +} + +pub(crate) fn parent_cache_personal_prefix(user_id: i64) -> String { + format!("{WEBDAV_PARENT_CACHE_PREFIX}personal:{user_id}:") +} + +pub(crate) fn path_cache_team_prefix(team_id: i64) -> String { + format!("{WEBDAV_PATH_CACHE_PREFIX}team:{team_id}:") +} + +pub(crate) fn parent_cache_team_prefix(team_id: i64) -> String { + format!("{WEBDAV_PARENT_CACHE_PREFIX}team:{team_id}:") +} + fn cacheable_node(node: &ResolvedNode) -> CachedResolvedNode { match node { ResolvedNode::Root => CachedResolvedNode::Root, From f8af50c6bb52ddec7b4b9e755b5b31b5dafb8ab9 Mon Sep 17 00:00:00 2001 From: AptS-1738 Date: Sun, 5 Jul 2026 19:08:07 +0800 Subject: [PATCH 5/6] feat(webdav): add download audit coalescing to reduce log volume Add configurable coalescing window for WebDAV download audit records to prevent log flooding from repeated reads by the same client. **Features:** - Add `webdav_download_audit_coalesce_window_secs` configuration (default 30s) - Coalesce repeated downloads by account, file, request type (full/ranged), and client fingerprint - Track WebDAV account ID through authentication and filesystem layers - Separate coalescing for full vs ranged reads - Support disabling coalescing by setting window to 0 **Implementation:** - New `download_audit` module with cache-based deduplication logic - Client fingerprint derived from IP address and User-Agent SHA256 hash - Integrate audit identity (account_id, scope, root_folder_id) into download recording - Add i18n labels for English and Chinese admin settings **Testing:** - Verify default 30-second coalescing reduces multiple range requests to single audit row - Confirm full and ranged reads tracked independently - Validate disabling coalescing (window=0) records every read - Update existing auth tests to handle new account_id field Closed #365 --- .../locales/en/admin/settings-common.json | 2 + .../locales/zh/admin/settings-common.json | 2 + src/config/definitions.rs | 14 ++ src/webdav/auth.rs | 21 +- src/webdav/download_audit.rs | 206 ++++++++++++++++++ src/webdav/fs/mod.rs | 26 ++- src/webdav/mod.rs | 2 + tests/test_webdav.rs | 134 +++++++++++- 8 files changed, 389 insertions(+), 18 deletions(-) create mode 100644 src/webdav/download_audit.rs diff --git a/frontend-panel/src/i18n/locales/en/admin/settings-common.json b/frontend-panel/src/i18n/locales/en/admin/settings-common.json index 02578afa0..bd008e5c4 100644 --- a/frontend-panel/src/i18n/locales/en/admin/settings-common.json +++ b/frontend-panel/src/i18n/locales/en/admin/settings-common.json @@ -64,6 +64,8 @@ "settings_item_webdav_enabled_desc": "Controls whether the WebDAV entrypoint is exposed. When disabled, clients can no longer access files through WebDAV.", "settings_item_webdav_max_active_locks_per_user_label": "WebDAV active lock limit", "settings_item_webdav_max_active_locks_per_user_desc": "Maximum active locks each WebDAV user can hold. New LOCK requests are rejected after this limit to prevent resource abuse.", + "settings_item_webdav_download_audit_coalesce_window_secs_label": "WebDAV download audit window", + "settings_item_webdav_download_audit_coalesce_window_secs_desc": "Seconds to coalesce repeated WebDAV download audit records for the same account, file, request type, and client. Set to 0 to record every read.", "settings_item_webdav_block_system_files_enabled_label": "Block WebDAV system files", "settings_item_webdav_block_system_files_enabled_desc": "Prevents WebDAV clients from creating common operating-system metadata files and folders such as .DS_Store, Thumbs.db, and desktop.ini.", "settings_item_webdav_block_system_file_patterns_label": "Blocked WebDAV system-file patterns", diff --git a/frontend-panel/src/i18n/locales/zh/admin/settings-common.json b/frontend-panel/src/i18n/locales/zh/admin/settings-common.json index 14171848e..8194471fe 100644 --- a/frontend-panel/src/i18n/locales/zh/admin/settings-common.json +++ b/frontend-panel/src/i18n/locales/zh/admin/settings-common.json @@ -64,6 +64,8 @@ "settings_item_webdav_enabled_desc": "控制 WebDAV 入口是否对外提供服务。关闭后客户端将无法通过 WebDAV 访问文件。", "settings_item_webdav_max_active_locks_per_user_label": "WebDAV 活跃锁上限", "settings_item_webdav_max_active_locks_per_user_desc": "每个 WebDAV 用户可持有的最大活跃锁数量。超过后拒绝新的 LOCK 请求，以防止资源滥用。", + "settings_item_webdav_download_audit_coalesce_window_secs_label": "WebDAV 下载审计合并窗口", + "settings_item_webdav_download_audit_coalesce_window_secs_desc": "同一账号、文件、请求类型和客户端在该秒数内的重复 WebDAV 下载审计会合并记录。设为 0 则每次读取都记录。", "settings_item_webdav_block_system_files_enabled_label": "阻止 WebDAV 系统文件", "settings_item_webdav_block_system_files_enabled_desc": "阻止 WebDAV 客户端创建常见操作系统元数据文件和目录，例如 .DS_Store、Thumbs.db、desktop.ini。", "settings_item_webdav_block_system_file_patterns_label": "WebDAV 系统文件拦截规则", diff --git a/src/config/definitions.rs b/src/config/definitions.rs index 90c1353d5..71546bdcd 100644 --- a/src/config/definitions.rs +++ b/src/config/definitions.rs @@ -231,6 +231,9 @@ pub const AUDIT_LOG_RECORDED_ACTIONS_KEY: &str = "audit_log_recorded_actions"; pub const WEBDAV_ENABLED_KEY: &str = "webdav_enabled"; pub const WEBDAV_MAX_ACTIVE_LOCKS_PER_USER_KEY: &str = "webdav_max_active_locks_per_user"; pub const DEFAULT_WEBDAV_MAX_ACTIVE_LOCKS_PER_USER: u64 = 1024; +pub const WEBDAV_DOWNLOAD_AUDIT_COALESCE_WINDOW_SECS_KEY: &str = + "webdav_download_audit_coalesce_window_secs"; +pub const DEFAULT_WEBDAV_DOWNLOAD_AUDIT_COALESCE_WINDOW_SECS: u64 = 30; pub const WEBDAV_BLOCK_SYSTEM_FILES_ENABLED_KEY: &str = "webdav_block_system_files_enabled"; pub const WEBDAV_BLOCK_SYSTEM_FILE_PATTERNS_KEY: &str = "webdav_block_system_file_patterns"; pub const DEFAULT_WEBDAV_SYSTEM_FILE_PATTERNS: &[&str] = &[ @@ -447,6 +450,17 @@ pub static ALL_CONFIGS: &[ConfigDef] = &[ category: CONFIG_CATEGORY_WEBDAV, description: "Maximum active WebDAV locks a single user can hold before new LOCK requests are rejected", }, + ConfigDef { + key: WEBDAV_DOWNLOAD_AUDIT_COALESCE_WINDOW_SECS_KEY, + label_i18n_key: "settings_item_webdav_download_audit_coalesce_window_secs_label", + description_i18n_key: "settings_item_webdav_download_audit_coalesce_window_secs_desc", + value_type: SystemConfigValueType::Number, + default_fn: || DEFAULT_WEBDAV_DOWNLOAD_AUDIT_COALESCE_WINDOW_SECS.to_string(), + requires_restart: false, + is_sensitive: false, + category: CONFIG_CATEGORY_WEBDAV, + description: "Seconds to coalesce repeated WebDAV download audit records for the same account, file, request type, and client fingerprint; 0 records every read", + }, ConfigDef { key: WEBDAV_BLOCK_SYSTEM_FILES_ENABLED_KEY, label_i18n_key: "settings_item_webdav_block_system_files_enabled_label", diff --git a/src/webdav/auth.rs b/src/webdav/auth.rs index 3998e6212..3e4db2d84 100644 --- a/src/webdav/auth.rs +++ b/src/webdav/auth.rs @@ -16,6 +16,7 @@ use crate::utils::hash; /// WebDAV 认证结果 #[derive(Debug)] pub(crate) struct WebdavAuthResult { + pub(crate) account_id: i64, pub(crate) scope: WorkspaceStorageScope, /// 限制访问范围：None = 全部，Some(folder_id) = 只能访问该文件夹及子目录 pub(crate) root_folder_id: Option, @@ -101,6 +102,7 @@ async fn authenticate_basic( validate_cached_account(state, username, password, &cached).await?; tracing::debug!(username_hash = %cache::username_cache_component(username), "webdav auth cache hit"); return Ok(WebdavAuthResult { + account_id: cached.account_id, scope: cached.scope(), root_folder_id: cached.root_folder_id, }); @@ -163,6 +165,7 @@ async fn authenticate_basic( .await; tracing::debug!(username_hash = %cache::username_cache_component(username), "webdav auth cache miss"); Ok(WebdavAuthResult { + account_id: account.id, scope, root_folder_id: account.root_folder_id, }) @@ -277,7 +280,9 @@ mod tests { } } - async fn seed_webdav_account(state: &PrimaryAppState) -> (String, String, i64, Option) { + async fn seed_webdav_account( + state: &PrimaryAppState, + ) -> (String, String, i64, i64, Option) { let now = Utc::now(); let user = user::ActiveModel { username: Set("webdav-auth-user".to_string()), @@ -304,7 +309,7 @@ mod tests { let password = "webdav-pass".to_string(); let root_folder_id = Some(123); - webdav_account::ActiveModel { + let account = webdav_account::ActiveModel { user_id: Set(user.id), username: Set(username.clone()), password_hash: Set( @@ -320,7 +325,7 @@ mod tests { .await .expect("webdav auth test account should be inserted"); - (username, password, user.id, root_folder_id) + (username, password, user.id, account.id, root_folder_id) } fn basic_headers(username: &str, password: &str) -> HeaderMap { @@ -348,12 +353,14 @@ mod tests { #[actix_web::test] async fn basic_auth_succeeds() { let state = build_auth_test_state().await; - let (username, password, user_id, root_folder_id) = seed_webdav_account(&state).await; + let (username, password, user_id, account_id, root_folder_id) = + seed_webdav_account(&state).await; let result = authenticate_webdav(&basic_headers(&username, &password), &state) .await .expect("basic auth should succeed"); + assert_eq!(result.account_id, account_id); assert_eq!(result.scope.actor_user_id(), user_id); assert_eq!(result.root_folder_id, root_folder_id); } @@ -361,7 +368,7 @@ mod tests { #[actix_web::test] async fn basic_auth_wrong_password_returns_invalid_credentials() { let state = build_auth_test_state().await; - let (username, _, _, _) = seed_webdav_account(&state).await; + let (username, _, _, _, _) = seed_webdav_account(&state).await; let err = authenticate_webdav(&basic_headers(&username, "wrong-password"), &state) .await @@ -390,7 +397,7 @@ mod tests { #[actix_web::test] async fn cached_basic_auth_rechecks_account_active_state() { let state = build_auth_test_state().await; - let (username, password, _, _) = seed_webdav_account(&state).await; + let (username, password, _, _, _) = seed_webdav_account(&state).await; authenticate_webdav(&basic_headers(&username, &password), &state) .await @@ -420,7 +427,7 @@ mod tests { #[actix_web::test] async fn cached_basic_auth_rechecks_current_password_hash() { let state = build_auth_test_state().await; - let (username, password, _, _) = seed_webdav_account(&state).await; + let (username, password, _, _, _) = seed_webdav_account(&state).await; authenticate_webdav(&basic_headers(&username, &password), &state) .await diff --git a/src/webdav/download_audit.rs b/src/webdav/download_audit.rs new file mode 100644 index 000000000..549d8c91f --- /dev/null +++ b/src/webdav/download_audit.rs @@ -0,0 +1,206 @@ +//! WebDAV 下载审计合并。 + +use crate::entities::file; +use crate::runtime::SharedRuntimeState; +use crate::services::{ + audit_service::{self, AuditContext, AuditEntityType}, + file_service, + workspace_storage_service::WorkspaceStorageScope, +}; +use crate::utils::hash; + +const DOWNLOAD_AUDIT_CACHE_PREFIX: &str = "webdav_download_audit:"; + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum WebdavDownloadRequestKind { + Full, + Ranged, +} + +impl WebdavDownloadRequestKind { + const fn as_str(self) -> &'static str { + match self { + Self::Full => "full", + Self::Ranged => "ranged", + } + } +} + +#[derive(Clone, Copy, Debug)] +pub(crate) struct WebdavDownloadAuditIdentity { + pub(crate) account_id: Option, + pub(crate) scope: WorkspaceStorageScope, + pub(crate) root_folder_id: Option, +} + +pub(crate) async fn record_download( + state: &S, + audit_ctx: &AuditContext, + identity: WebdavDownloadAuditIdentity, + file: &file::Model, + request_kind: WebdavDownloadRequestKind, +) where + S: SharedRuntimeState, +{ + if !audit_service::should_record(state, audit_service::AuditAction::FileDownload) { + return; + } + + if !reserve_download_audit_slot(state, audit_ctx, identity, file.id, request_kind).await { + return; + } + + let details = file_service::audit_location_details_for_model(state, identity.scope, file).await; + audit_service::log_with_details( + state, + audit_ctx, + audit_service::AuditAction::FileDownload, + AuditEntityType::File, + Some(file.id), + Some(&file.name), + || details.clone(), + ) + .await; +} + +async fn reserve_download_audit_slot( + state: &S, + audit_ctx: &AuditContext, + identity: WebdavDownloadAuditIdentity, + file_id: i64, + request_kind: WebdavDownloadRequestKind, +) -> bool +where + S: SharedRuntimeState, +{ + let ttl_secs = coalesce_window_secs(state); + if ttl_secs == 0 { + return true; + } + + let key = download_audit_cache_key(audit_ctx, identity, file_id, request_kind); + state + .cache() + .set_bytes_if_absent(&key, Vec::new(), Some(ttl_secs)) + .await +} + +fn coalesce_window_secs(state: &impl SharedRuntimeState) -> u64 { + state.runtime_config().get_u64_or( + crate::config::definitions::WEBDAV_DOWNLOAD_AUDIT_COALESCE_WINDOW_SECS_KEY, + crate::config::definitions::DEFAULT_WEBDAV_DOWNLOAD_AUDIT_COALESCE_WINDOW_SECS, + ) +} + +fn download_audit_cache_key( + audit_ctx: &AuditContext, + identity: WebdavDownloadAuditIdentity, + file_id: i64, + request_kind: WebdavDownloadRequestKind, +) -> String { + format!( + "{DOWNLOAD_AUDIT_CACHE_PREFIX}{}:{}:{}:{}:{}", + download_audit_principal(identity), + root_folder_component(identity.root_folder_id), + file_id, + request_kind.as_str(), + request_fingerprint(audit_ctx) + ) +} + +fn download_audit_principal(identity: WebdavDownloadAuditIdentity) -> String { + match identity.account_id { + Some(account_id) => format!("account:{account_id}"), + None => match identity.scope { + WorkspaceStorageScope::Personal { user_id } => format!("personal:{user_id}"), + WorkspaceStorageScope::Team { + team_id, + actor_user_id, + } => format!("team:{team_id}:actor:{actor_user_id}"), + }, + } +} + +fn root_folder_component(root_folder_id: Option) -> String { + match root_folder_id { + Some(root_folder_id) => root_folder_id.to_string(), + None => "root".to_string(), + } +} + +fn request_fingerprint(audit_ctx: &AuditContext) -> String { + let raw = format!( + "{}\n{}", + audit_ctx.ip_address.as_deref().unwrap_or_default(), + audit_ctx.user_agent.as_deref().unwrap_or_default() + ); + hash::sha256_hex(raw.as_bytes()) +} + +#[cfg(test)] +mod tests { + use super::{ + WebdavDownloadAuditIdentity, WebdavDownloadRequestKind, download_audit_cache_key, + request_fingerprint, + }; + use crate::services::{ + audit_service::AuditContext, workspace_storage_service::WorkspaceStorageScope, + }; + + fn audit_ctx() -> AuditContext { + AuditContext { + user_id: 7, + ip_address: Some("192.0.2.10".to_string()), + user_agent: Some("range-client/1.0".to_string()), + } + } + + #[test] + fn cache_key_uses_webdav_account_when_available() { + let identity = WebdavDownloadAuditIdentity { + account_id: Some(42), + scope: WorkspaceStorageScope::Personal { user_id: 7 }, + root_folder_id: None, + }; + + let key = download_audit_cache_key( + &audit_ctx(), + identity, + 99, + WebdavDownloadRequestKind::Ranged, + ); + + assert!(key.contains("account:42")); + assert!(key.contains(":99:ranged:")); + assert!(!key.contains("192.0.2.10")); + assert!(!key.contains("range-client")); + } + + #[test] + fn cache_key_separates_full_and_ranged_reads() { + let identity = WebdavDownloadAuditIdentity { + account_id: Some(42), + scope: WorkspaceStorageScope::Personal { user_id: 7 }, + root_folder_id: None, + }; + + let full = + download_audit_cache_key(&audit_ctx(), identity, 99, WebdavDownloadRequestKind::Full); + let ranged = download_audit_cache_key( + &audit_ctx(), + identity, + 99, + WebdavDownloadRequestKind::Ranged, + ); + + assert_ne!(full, ranged); + } + + #[test] + fn request_fingerprint_hashes_request_metadata() { + let fingerprint = request_fingerprint(&audit_ctx()); + + assert_eq!(fingerprint.len(), 64); + assert!(!fingerprint.contains("192.0.2.10")); + } +} diff --git a/src/webdav/fs/mod.rs b/src/webdav/fs/mod.rs index ab5404c60..b3691845c 100644 --- a/src/webdav/fs/mod.rs +++ b/src/webdav/fs/mod.rs @@ -19,6 +19,9 @@ use crate::webdav::dav::{ FsStream, OpenOptions, ReadDirMeta, }; use crate::webdav::dir_entry::AsterDavDirEntry; +use crate::webdav::download_audit::{ + WebdavDownloadAuditIdentity, WebdavDownloadRequestKind, record_download, +}; use crate::webdav::file::AsterDavFile; use crate::webdav::metadata::AsterDavMeta; use crate::webdav::path_resolver::{self, ResolvedNode}; @@ -27,6 +30,7 @@ use crate::webdav::path_resolver::{self, ResolvedNode}; #[derive(Clone)] pub struct AsterDavFs { state: PrimaryAppState, + webdav_account_id: Option, scope: WorkspaceStorageScope, /// 限制访问范围：None = 用户全部文件，Some(id) = 只能访问该文件夹及子目录 root_folder_id: Option, @@ -46,6 +50,7 @@ impl AsterDavFs { pub fn new(state: PrimaryAppState, user_id: i64, root_folder_id: Option) -> Self { Self::new_with_audit( state, + None, WorkspaceStorageScope::Personal { user_id }, root_folder_id, AuditContext { @@ -58,12 +63,14 @@ impl AsterDavFs { pub(crate) fn new_with_audit( state: PrimaryAppState, + webdav_account_id: Option, scope: WorkspaceStorageScope, root_folder_id: Option, audit_ctx: AuditContext, ) -> Self { Self { state, + webdav_account_id, scope, root_folder_id, audit_ctx, @@ -128,16 +135,19 @@ impl AsterDavFs { .await .map_err(|_| FsError::NotFound)?, }; - let details = - file_service::audit_location_details_for_model(&self.state, self.scope, &file).await; - audit_service::log_with_details( + record_download( &self.state, &self.audit_ctx, - audit_service::AuditAction::FileDownload, - crate::services::audit_service::AuditEntityType::File, - Some(file.id), - Some(&file.name), - || details.clone(), + WebdavDownloadAuditIdentity { + account_id: self.webdav_account_id, + scope: self.scope, + root_folder_id: self.root_folder_id, + }, + &file, + match offset { + Some(_) => WebdavDownloadRequestKind::Ranged, + None => WebdavDownloadRequestKind::Full, + }, ) .await; Ok(stream) diff --git a/src/webdav/mod.rs b/src/webdav/mod.rs index e27ebeaa1..ee3b2d5be 100644 --- a/src/webdav/mod.rs +++ b/src/webdav/mod.rs @@ -5,6 +5,7 @@ pub mod dav; pub mod db_lock_system; pub mod deltav; pub mod dir_entry; +mod download_audit; pub mod file; pub mod fs; mod locks; @@ -110,6 +111,7 @@ pub async fn webdav_handler( let dav_fs = fs::AsterDavFs::new_with_audit( state.get_ref().clone(), + Some(auth_result.account_id), auth_result.scope, auth_result.root_folder_id, audit_ctx.clone(), diff --git a/tests/test_webdav.rs b/tests/test_webdav.rs index 57effd6a5..3ad4f4abc 100644 --- a/tests/test_webdav.rs +++ b/tests/test_webdav.rs @@ -7,12 +7,18 @@ use actix_web::test; use actix_web::{App, HttpServer, web}; use aster_drive::config::{RateLimitConfig, WebDavConfig}; use aster_drive::db::repository::{file_repo, property_repo}; -use aster_drive::entities::{team, team_member, user, webdav_account}; +use aster_drive::entities::{audit_log, system_config, team, team_member, user, webdav_account}; use aster_drive::runtime::{PrimaryAppState, SharedRuntimeState}; -use aster_drive::types::{EntityType, TeamMemberRole, UserRole, UserStatus}; +use aster_drive::services::audit_service; +use aster_drive::types::{ + AuditAction, EntityType, SystemConfigSource, SystemConfigValueType, SystemConfigVisibility, + TeamMemberRole, UserRole, UserStatus, +}; use base64::Engine; use chrono::Utc; -use sea_orm::{ActiveModelTrait, IntoActiveModel, Set}; +use sea_orm::{ + ActiveModelTrait, ColumnTrait, EntityTrait, IntoActiveModel, PaginatorTrait, QueryFilter, Set, +}; use std::io::Cursor; use tokio::task::JoinHandle; use xmltree::Element; @@ -32,6 +38,34 @@ fn webdav_test_password(label: &str) -> String { format!("TEST_PASSWORD_{label}_{}", uuid::Uuid::new_v4().simple()) } +async fn count_file_download_audit_rows(state: &PrimaryAppState, entity_name: &str) -> u64 { + audit_service::flush_global_audit_log_manager().await; + audit_log::Entity::find() + .filter(audit_log::Column::Action.eq(AuditAction::FileDownload)) + .filter(audit_log::Column::EntityName.eq(entity_name)) + .count(state.writer_db()) + .await + .expect("file download audit count should query successfully") +} + +fn runtime_number_config(key: &str, value: &str) -> system_config::Model { + system_config::Model { + id: 0, + key: key.to_string(), + value: value.to_string(), + value_type: SystemConfigValueType::Number, + requires_restart: false, + is_sensitive: false, + source: SystemConfigSource::System, + visibility: SystemConfigVisibility::Private, + namespace: String::new(), + category: "webdav".to_string(), + description: "test runtime config".to_string(), + updated_at: Utc::now(), + updated_by: None, + } +} + struct RunningWebdavServer { base_url: String, handle: actix_web::dev::ServerHandle, @@ -910,6 +944,100 @@ async fn test_webdav_get_supports_binary_range_requests() { assert_eq!(body.as_ref(), data.as_slice()); } +#[actix_web::test] +async fn test_webdav_range_download_audit_is_coalesced_by_default() { + let state = common::setup().await; + let db1 = state.writer_db().clone(); + let db2 = state.writer_db().clone(); + let webdav_config = WebDavConfig::default(); + let app = test::init_service( + App::new() + .wrap(aster_drive::api::middleware::security_headers::default_headers()) + .app_data(web::PayloadConfig::new(10 * 1024 * 1024)) + .app_data(web::JsonConfig::default().limit(1024 * 1024)) + .app_data(web::Data::new(state.clone())) + .configure(move |cfg| { + aster_drive::webdav::configure(cfg, &webdav_config, &db2); + aster_drive::api::configure_primary(cfg, &db1); + }), + ) + .await; + + let (token, _) = register_and_login!(app); + let auth = create_webdav_basic_auth!(app, token); + let file_name = "range-audit-coalesced.bin"; + let data: Vec = (0..=255).cycle().take(4096).collect(); + + let req = test::TestRequest::put() + .uri(&format!("/webdav/{file_name}")) + .insert_header(("Authorization", auth.clone())) + .insert_header(("Content-Type", "application/octet-stream")) + .insert_header(("Content-Length", data.len().to_string())) + .set_payload(data.clone()) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), actix_web::http::StatusCode::CREATED); + + let before = count_file_download_audit_rows(&state, file_name).await; + for range in ["bytes=0-127", "bytes=128-255", "bytes=256-383"] { + let req = test::TestRequest::get() + .uri(&format!("/webdav/{file_name}")) + .insert_header(("Authorization", auth.clone())) + .insert_header(("Range", range)) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), actix_web::http::StatusCode::PARTIAL_CONTENT); + let _ = test::read_body(resp).await; + } + + let after_ranges = count_file_download_audit_rows(&state, file_name).await; + assert_eq!( + after_ranges - before, + 1, + "repeated WebDAV range reads should be coalesced into one file_download audit row" + ); + + for _ in 0..2 { + let req = test::TestRequest::get() + .uri(&format!("/webdav/{file_name}")) + .insert_header(("Authorization", auth.clone())) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), actix_web::http::StatusCode::OK); + let _ = test::read_body(resp).await; + } + + let after_full_reads = count_file_download_audit_rows(&state, file_name).await; + assert_eq!( + after_full_reads - after_ranges, + 1, + "full and ranged WebDAV reads should be coalesced independently" + ); + + state.runtime_config().apply(runtime_number_config( + aster_drive::config::definitions::WEBDAV_DOWNLOAD_AUDIT_COALESCE_WINDOW_SECS_KEY, + "0", + )); + + for range in ["bytes=512-639", "bytes=640-767"] { + let req = test::TestRequest::get() + .uri(&format!("/webdav/{file_name}")) + .insert_header(("Authorization", auth.clone())) + .insert_header(("Range", range)) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), actix_web::http::StatusCode::PARTIAL_CONTENT); + let _ = test::read_body(resp).await; + } + + let after_disabled_coalescing = count_file_download_audit_rows(&state, file_name).await; + assert_eq!( + after_disabled_coalescing - after_full_reads, + 2, + "setting the WebDAV download audit coalescing window to 0 should record every read" + ); +} + #[actix_web::test] async fn test_webdav_real_http_put_with_content_length_persists_bytes() { let state = common::setup().await; From c1616f40fb91db235d7d673fcad329b82606ed19 Mon Sep 17 00:00:00 2001 From: AptS-1738 Date: Sun, 5 Jul 2026 20:36:01 +0800 Subject: [PATCH 6/6] fix(webdav): address review feedback --- src/services/file_service/common.rs | 24 +++++++- src/webdav/dav.rs | 17 ++++++ src/webdav/db_lock_system.rs | 13 +++- src/webdav/fs/mod.rs | 39 ++++++++++++ src/webdav/metadata.rs | 9 +++ src/webdav/props/mod.rs | 12 +++- tests/test_webdav.rs | 94 ++++++++++++++++++++++++++++- 7 files changed, 200 insertions(+), 8 deletions(-) diff --git a/src/services/file_service/common.rs b/src/services/file_service/common.rs index bd4496a54..e91365ffc 100644 --- a/src/services/file_service/common.rs +++ b/src/services/file_service/common.rs @@ -32,6 +32,12 @@ pub(crate) fn ensure_personal_file_scope(file: &file::Model) -> Result<()> { pub(crate) fn if_none_match_matches_value(if_none_match: &str, etag_value: &str) -> bool { http_validators::if_none_match_header_matches(if_none_match, true, Some(etag_value)) .unwrap_or(false) + || if_none_match.split(',').any(|candidate| { + candidate + .trim() + .trim_matches('"') + .eq_ignore_ascii_case(etag_value) + }) } pub(crate) fn if_none_match_matches(if_none_match: &str, blob_hash: &str) -> bool { @@ -40,7 +46,7 @@ pub(crate) fn if_none_match_matches(if_none_match: &str, blob_hash: &str) -> boo #[cfg(test)] mod tests { - use super::requires_inline_sandbox; + use super::{if_none_match_matches_value, requires_inline_sandbox}; #[test] fn dangerous_same_origin_inline_mime_types_require_sandbox() { @@ -51,4 +57,20 @@ mod tests { assert!(!requires_inline_sandbox("text/plain")); assert!(!requires_inline_sandbox("application/pdf")); } + + #[test] + fn if_none_match_matches_value_preserves_case_insensitive_blob_hash_matching() { + assert!(if_none_match_matches_value( + r#""ABCDEF123456""#, + "abcdef123456" + )); + assert!(if_none_match_matches_value( + r#""other", "ABCDEF123456""#, + "abcdef123456" + )); + assert!(!if_none_match_matches_value( + r#"W/"ABCDEF123456""#, + "abcdef123456" + )); + } } diff --git a/src/webdav/dav.rs b/src/webdav/dav.rs index 2c33e3cfb..1c34af92d 100644 --- a/src/webdav/dav.rs +++ b/src/webdav/dav.rs @@ -185,6 +185,9 @@ pub trait DavMetaData: Send + Sync { fn is_dir(&self) -> bool; fn etag(&self) -> Option; fn created(&self) -> FsResult; + fn property_entity(&self) -> Option<(crate::types::EntityType, i64)> { + None + } fn is_empty(&self) -> bool { self.len() == 0 @@ -268,6 +271,20 @@ pub trait DavFileSystem: Send + Sync { }) } + fn get_props_many_for_entities<'a>( + &'a self, + targets: &'a [(DavPath, crate::types::EntityType, i64)], + do_content: bool, + ) -> FsFuture<'a, HashMap>> { + Box::pin(async move { + let paths = targets + .iter() + .map(|(path, _, _)| path.clone()) + .collect::>(); + self.get_props_many(&paths, do_content).await + }) + } + fn patch_props<'a>( &'a self, _path: &'a DavPath, diff --git a/src/webdav/db_lock_system.rs b/src/webdav/db_lock_system.rs index 12cbb3431..4d14d24d1 100644 --- a/src/webdav/db_lock_system.rs +++ b/src/webdav/db_lock_system.rs @@ -20,6 +20,8 @@ use crate::webdav::dav::{ }; use crate::webdav::path_resolver::{self, ResolvedNode}; +const DISCOVER_MANY_ANCESTOR_CHUNK_SIZE: usize = 500; + /// 数据库支持的 WebDAV 锁系统 /// /// Per-request 创建（需要 user_id 做 path → entity_id 解析） @@ -535,9 +537,14 @@ impl DavLockSystem for DbLockSystem { all_ancestors.sort(); all_ancestors.dedup(); - let mut locks = lock_repo::find_ancestors(&self.db, &all_ancestors) - .await - .unwrap_or_default(); + let mut locks = Vec::new(); + for chunk in all_ancestors.chunks(DISCOVER_MANY_ANCESTOR_CHUNK_SIZE) { + locks.extend( + lock_repo::find_ancestors(&self.db, chunk) + .await + .unwrap_or_default(), + ); + } locks.retain(|lock| lock.timeout_at.is_none_or(|timeout_at| timeout_at >= now)); locks.sort_by_key(|lock| lock.id); diff --git a/src/webdav/fs/mod.rs b/src/webdav/fs/mod.rs index b3691845c..4baa4579d 100644 --- a/src/webdav/fs/mod.rs +++ b/src/webdav/fs/mod.rs @@ -741,6 +741,45 @@ impl DavFileSystem for AsterDavFs { }) } + fn get_props_many_for_entities<'a>( + &'a self, + targets: &'a [(DavPath, EntityType, i64)], + do_content: bool, + ) -> FsFuture<'a, HashMap>> { + Box::pin(async move { + let mut target_paths: HashMap<(EntityType, i64), Vec> = HashMap::new(); + let mut entity_targets = Vec::with_capacity(targets.len()); + for (path, entity_type, entity_id) in targets { + let target = (*entity_type, *entity_id); + target_paths.entry(target).or_default().push(path.clone()); + entity_targets.push(target); + } + + let props = property_repo::find_by_entities(self.state.reader_db(), &entity_targets) + .await + .map_err(|_| FsError::GeneralFailure)?; + let mut props_by_target: HashMap<(EntityType, i64), Vec> = HashMap::new(); + for prop in props { + if property_service::is_protected_namespace(&prop.namespace) { + continue; + } + props_by_target + .entry((prop.entity_type, prop.entity_id)) + .or_default() + .push(entity_prop_to_dav_prop(prop, do_content)); + } + + let mut result = HashMap::with_capacity(targets.len()); + for (target, paths) in target_paths { + let props = props_by_target.remove(&target).unwrap_or_default(); + for path in paths { + result.insert(path, props.clone()); + } + } + Ok(result) + }) + } + fn patch_props<'a>( &'a self, path: &'a DavPath, diff --git a/src/webdav/metadata.rs b/src/webdav/metadata.rs index a430c6c70..381a2dd60 100644 --- a/src/webdav/metadata.rs +++ b/src/webdav/metadata.rs @@ -3,6 +3,7 @@ use std::time::SystemTime; use crate::entities::{file, file_blob, folder}; +use crate::types::EntityType; use crate::webdav::dav::{DavMetaData, FsResult}; /// 将 chrono DateTimeUtc 转换为 SystemTime @@ -21,6 +22,7 @@ pub struct AsterDavMeta { modified: SystemTime, created: SystemTime, etag: Option, + property_entity: Option<(EntityType, i64)>, } impl AsterDavMeta { @@ -31,6 +33,7 @@ impl AsterDavMeta { modified: SystemTime::UNIX_EPOCH, created: SystemTime::UNIX_EPOCH, etag: None, + property_entity: None, } } @@ -41,6 +44,7 @@ impl AsterDavMeta { modified: to_system_time(folder.updated_at), created: to_system_time(folder.created_at), etag: Some(format!("dir-{}", folder.updated_at.timestamp())), + property_entity: Some((EntityType::Folder, folder.id)), } } @@ -51,6 +55,7 @@ impl AsterDavMeta { modified: to_system_time(file.updated_at), created: to_system_time(file.created_at), etag: Some(blob.hash.clone()), + property_entity: Some((EntityType::File, file.id)), } } } @@ -75,4 +80,8 @@ impl DavMetaData for AsterDavMeta { fn created(&self) -> FsResult { Ok(self.created) } + + fn property_entity(&self) -> Option<(EntityType, i64)> { + self.property_entity + } } diff --git a/src/webdav/props/mod.rs b/src/webdav/props/mod.rs index edd1bb49f..3907f74b5 100644 --- a/src/webdav/props/mod.rs +++ b/src/webdav/props/mod.rs @@ -103,13 +103,19 @@ impl PropfindPreload { let mut preload = Self::default(); if propfind_kind_needs_dead_props(request_kind) { - let paths = resources + let targets = resources .iter() .filter(|resource| !is_root_resource(resource)) - .map(|resource| resource.path.clone()) + .filter_map(|resource| { + let (entity_type, entity_id) = resource.meta.property_entity()?; + Some((resource.path.clone(), entity_type, entity_id)) + }) .collect::>(); preload.dead_props = dav_fs - .get_props_many(&paths, propfind_kind_needs_dead_prop_content(request_kind)) + .get_props_many_for_entities( + &targets, + propfind_kind_needs_dead_prop_content(request_kind), + ) .await .map_err(fs_error_response)?; } diff --git a/tests/test_webdav.rs b/tests/test_webdav.rs index 3ad4f4abc..5ab7a4add 100644 --- a/tests/test_webdav.rs +++ b/tests/test_webdav.rs @@ -7,7 +7,9 @@ use actix_web::test; use actix_web::{App, HttpServer, web}; use aster_drive::config::{RateLimitConfig, WebDavConfig}; use aster_drive::db::repository::{file_repo, property_repo}; -use aster_drive::entities::{audit_log, system_config, team, team_member, user, webdav_account}; +use aster_drive::entities::{ + audit_log, folder, system_config, team, team_member, user, webdav_account, +}; use aster_drive::runtime::{PrimaryAppState, SharedRuntimeState}; use aster_drive::services::audit_service; use aster_drive::types::{ @@ -1038,6 +1040,96 @@ async fn test_webdav_range_download_audit_is_coalesced_by_default() { ); } +#[actix_web::test] +async fn test_webdav_propfind_lockdiscovery_chunks_large_depth_one_directories() { + let state = common::setup().await; + let db1 = state.writer_db().clone(); + let db2 = state.writer_db().clone(); + let webdav_config = WebDavConfig::default(); + let app = test::init_service( + App::new() + .wrap(aster_drive::api::middleware::security_headers::default_headers()) + .app_data(web::PayloadConfig::new(10 * 1024 * 1024)) + .app_data(web::JsonConfig::default().limit(1024 * 1024)) + .app_data(web::Data::new(state.clone())) + .configure(move |cfg| { + aster_drive::webdav::configure(cfg, &webdav_config, &db2); + aster_drive::api::configure_primary(cfg, &db1); + }), + ) + .await; + + let (token, _) = register_and_login!(app); + let auth = create_webdav_basic_auth!(app, token); + let owner = user::Entity::find() + .filter(user::Column::Username.eq("testuser")) + .one(state.writer_db()) + .await + .expect("test user query should succeed") + .expect("test user should exist"); + let now = Utc::now(); + let parent = folder::ActiveModel { + name: Set("large-lockdiscovery".to_string()), + parent_id: Set(None), + team_id: Set(None), + owner_user_id: Set(Some(owner.id)), + created_by_user_id: Set(Some(owner.id)), + created_by_username: Set(owner.username.clone()), + policy_id: Set(None), + created_at: Set(now), + updated_at: Set(now), + deleted_at: Set(None), + is_locked: Set(false), + ..Default::default() + } + .insert(state.writer_db()) + .await + .expect("large lockdiscovery parent folder should insert"); + + let children = (0..520) + .map(|index| folder::ActiveModel { + name: Set(format!("child-{index:04}")), + parent_id: Set(Some(parent.id)), + team_id: Set(None), + owner_user_id: Set(Some(owner.id)), + created_by_user_id: Set(Some(owner.id)), + created_by_username: Set(owner.username.clone()), + policy_id: Set(None), + created_at: Set(now), + updated_at: Set(now), + deleted_at: Set(None), + is_locked: Set(false), + ..Default::default() + }) + .collect::>(); + folder::Entity::insert_many(children) + .exec(state.writer_db()) + .await + .expect("large lockdiscovery child folders should insert"); + + let body = r#" + + + + +"#; + let req = test::TestRequest::with_uri("/webdav/large-lockdiscovery/") + .method(actix_web::http::Method::from_bytes(b"PROPFIND").unwrap()) + .insert_header(("Authorization", auth)) + .insert_header(("Depth", "1")) + .insert_header(("Content-Type", "application/xml")) + .set_payload(body) + .to_request(); + let resp = test::call_service(&app, req).await; + assert_eq!(resp.status(), actix_web::http::StatusCode::MULTI_STATUS); + let body = test::read_body(resp).await; + let xml = String::from_utf8_lossy(&body); + assert!( + xml.contains("/webdav/large-lockdiscovery/child-0519/"), + "Depth:1 PROPFIND should include the last seeded child: {xml}" + ); +} + #[actix_web::test] async fn test_webdav_real_http_put_with_content_length_persists_bytes() { let state = common::setup().await;