From 80ed63484bf62b916ceda17fd76b1092261fda77 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Thu, 18 Jun 2026 06:34:30 +0800 Subject: [PATCH] refactor: move object precondition contracts (#3547) --- crates/ecstore/src/store_api/types.rs | 89 +++------- crates/storage-api/src/lib.rs | 1 + crates/storage-api/src/object.rs | 160 ++++++++++++++++++ docs/architecture/crate-boundaries.md | 1 + docs/architecture/migration-progress.md | 74 +++++--- scripts/check_architecture_migration_rules.sh | 6 +- 6 files changed, 239 insertions(+), 92 deletions(-) diff --git a/crates/ecstore/src/store_api/types.rs b/crates/ecstore/src/store_api/types.rs index ca2e6ab50..a54bcd461 100644 --- a/crates/ecstore/src/store_api/types.rs +++ b/crates/ecstore/src/store_api/types.rs @@ -1,5 +1,8 @@ use super::*; -use rustfs_storage_api::{HTTPPreconditions, ObjectLockRetentionOptions, VersionMarker, WalkVersionsSortOrder}; +use rustfs_storage_api::{ + HTTPPreconditions, ObjectLockRetentionOptions, ObjectPreconditionError, ObjectPreconditionPart, ObjectPreconditionState, + VersionMarker, WalkVersionsSortOrder, +}; #[derive(Debug, Default, Clone)] pub struct ObjectOptions { @@ -102,76 +105,30 @@ impl ObjectOptions { } pub fn precondition_check(&self, obj_info: &ObjectInfo) -> Result<()> { - let has_valid_mod_time = obj_info.mod_time.is_some_and(|t| t != OffsetDateTime::UNIX_EPOCH); - - if let Some(part_number) = self.part_number - && part_number > 1 - && !obj_info.parts.is_empty() - { - let part_found = obj_info.parts.iter().any(|pi| pi.number == part_number); - if !part_found { - return Err(Error::InvalidPartNumber(part_number)); + let requested_part = self.part_number.and_then(|part_number| { + if part_number > 1 && !obj_info.parts.is_empty() { + Some(ObjectPreconditionPart { + number: part_number, + exists: obj_info.parts.iter().any(|pi| pi.number == part_number), + }) + } else { + None } - } + }); + let state = ObjectPreconditionState { + etag: obj_info.etag.as_deref(), + mod_time: obj_info.mod_time, + requested_part, + }; - if let Some(pre) = &self.http_preconditions { - let if_none_match = pre.if_none_match_value(); - let if_match = pre.if_match_value(); - - if let Some(if_none_match) = if_none_match - && let Some(etag) = &obj_info.etag - && is_etag_equal(etag, if_none_match) - { - return Err(Error::NotModified); - } - - if has_valid_mod_time - && let Some(if_modified_since) = &pre.if_modified_since - && let Some(mod_time) = &obj_info.mod_time - && !is_modified_since(mod_time, if_modified_since) - { - return Err(Error::NotModified); - } - - if let Some(if_match) = if_match { - if let Some(etag) = &obj_info.etag { - if !is_etag_equal(etag, if_match) { - return Err(Error::PreconditionFailed); - } - } else { - return Err(Error::PreconditionFailed); - } - } - if has_valid_mod_time - && if_match.is_none() - && let Some(if_unmodified_since) = &pre.if_unmodified_since - && let Some(mod_time) = &obj_info.mod_time - && is_modified_since(mod_time, if_unmodified_since) - { - return Err(Error::PreconditionFailed); - } - } - - Ok(()) + state.check(self.http_preconditions.as_ref()).map_err(|err| match err { + ObjectPreconditionError::InvalidPartNumber(part_number) => Error::InvalidPartNumber(part_number), + ObjectPreconditionError::NotModified => Error::NotModified, + ObjectPreconditionError::PreconditionFailed => Error::PreconditionFailed, + }) } } -fn is_etag_equal(etag1: &str, etag2: &str) -> bool { - let e1 = etag1.trim_matches('"'); - let e2 = etag2.trim_matches('"'); - // Handle wildcard "*" - matches any ETag (per HTTP/1.1 RFC 7232) - if e2 == "*" { - return true; - } - e1 == e2 -} - -fn is_modified_since(mod_time: &OffsetDateTime, given_time: &OffsetDateTime) -> bool { - let mod_secs = mod_time.unix_timestamp(); - let given_secs = given_time.unix_timestamp(); - mod_secs > given_secs -} - #[derive(Debug, Default)] pub struct ObjectInfo { pub bucket: String, diff --git a/crates/storage-api/src/lib.rs b/crates/storage-api/src/lib.rs index 591feddd8..2e037065b 100644 --- a/crates/storage-api/src/lib.rs +++ b/crates/storage-api/src/lib.rs @@ -25,4 +25,5 @@ pub use bucket::{BucketInfo, BucketOperations, BucketOptions, DeleteBucketOption pub use error::{StorageErrorCode, StorageResult}; pub use multipart::{CompletePart, ListMultipartsInfo, ListPartsInfo, MultipartInfo, MultipartUploadResult, PartInfo}; pub use object::{HTTPPreconditions, HTTPRangeError, HTTPRangeSpec, ObjectLockRetentionOptions}; +pub use object::{ObjectPreconditionError, ObjectPreconditionPart, ObjectPreconditionState}; pub use object::{VersionMarker, WalkVersionsSortOrder}; diff --git a/crates/storage-api/src/object.rs b/crates/storage-api/src/object.rs index cd7757068..7b41e9a1c 100644 --- a/crates/storage-api/src/object.rs +++ b/crates/storage-api/src/object.rs @@ -43,6 +43,93 @@ pub struct ObjectLockRetentionOptions { pub bypass_governance: bool, } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct ObjectPreconditionPart { + pub number: usize, + pub exists: bool, +} + +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub struct ObjectPreconditionState<'a> { + pub etag: Option<&'a str>, + pub mod_time: Option, + pub requested_part: Option, +} + +impl ObjectPreconditionState<'_> { + pub fn check(self, preconditions: Option<&HTTPPreconditions>) -> Result<(), ObjectPreconditionError> { + if let Some(part) = self.requested_part + && part.number > 1 + && !part.exists + { + return Err(ObjectPreconditionError::InvalidPartNumber(part.number)); + } + + let Some(preconditions) = preconditions else { + return Ok(()); + }; + + let has_valid_mod_time = self.mod_time.is_some_and(|t| t != OffsetDateTime::UNIX_EPOCH); + let if_none_match = preconditions.if_none_match_value(); + let if_match = preconditions.if_match_value(); + + if let Some(if_none_match) = if_none_match + && let Some(etag) = self.etag + && etag_matches(etag, if_none_match) + { + return Err(ObjectPreconditionError::NotModified); + } + + if has_valid_mod_time + && let Some(if_modified_since) = &preconditions.if_modified_since + && let Some(mod_time) = &self.mod_time + && !is_modified_since(mod_time, if_modified_since) + { + return Err(ObjectPreconditionError::NotModified); + } + + if let Some(if_match) = if_match { + if let Some(etag) = self.etag { + if !etag_matches(etag, if_match) { + return Err(ObjectPreconditionError::PreconditionFailed); + } + } else { + return Err(ObjectPreconditionError::PreconditionFailed); + } + } + + if has_valid_mod_time + && if_match.is_none() + && let Some(if_unmodified_since) = &preconditions.if_unmodified_since + && let Some(mod_time) = &self.mod_time + && is_modified_since(mod_time, if_unmodified_since) + { + return Err(ObjectPreconditionError::PreconditionFailed); + } + + Ok(()) + } +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ObjectPreconditionError { + InvalidPartNumber(usize), + NotModified, + PreconditionFailed, +} + +impl fmt::Display for ObjectPreconditionError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::InvalidPartNumber(part_number) => write!(f, "invalid part number {part_number}"), + Self::NotModified => f.write_str("object not modified"), + Self::PreconditionFailed => f.write_str("object precondition failed"), + } + } +} + +impl std::error::Error for ObjectPreconditionError {} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum VersionMarker { Null, @@ -185,6 +272,16 @@ fn non_empty_condition_value(value: Option<&str>) -> Option<&str> { value.map(str::trim).filter(|value| !value.is_empty()) } +fn etag_matches(object_etag: &str, condition_etag: &str) -> bool { + let object_etag = object_etag.trim_matches('"'); + let condition_etag = condition_etag.trim_matches('"'); + condition_etag == "*" || object_etag == condition_etag +} + +fn is_modified_since(mod_time: &OffsetDateTime, given_time: &OffsetDateTime) -> bool { + mod_time.unix_timestamp() > given_time.unix_timestamp() +} + #[cfg(test)] mod tests { use super::*; @@ -210,6 +307,69 @@ mod tests { assert!(!opts.bypass_governance); } + #[test] + fn object_precondition_state_rejects_missing_requested_part() { + let state = ObjectPreconditionState { + requested_part: Some(ObjectPreconditionPart { + number: 3, + exists: false, + }), + ..Default::default() + }; + + assert_eq!(state.check(None), Err(ObjectPreconditionError::InvalidPartNumber(3))); + } + + #[test] + fn object_precondition_state_keeps_existing_etag_priority() { + let preconditions = HTTPPreconditions { + if_match: Some("\"other\"".to_owned()), + if_none_match: Some("\"abc\"".to_owned()), + ..Default::default() + }; + let state = ObjectPreconditionState { + etag: Some("\"abc\""), + ..Default::default() + }; + + assert_eq!(state.check(Some(&preconditions)), Err(ObjectPreconditionError::NotModified)); + } + + #[test] + fn object_precondition_state_requires_etag_for_if_match() { + let preconditions = HTTPPreconditions { + if_match: Some("\"abc\"".to_owned()), + ..Default::default() + }; + + assert_eq!( + ObjectPreconditionState::default().check(Some(&preconditions)), + Err(ObjectPreconditionError::PreconditionFailed) + ); + } + + #[test] + fn object_precondition_state_checks_modification_dates() { + let mod_time = OffsetDateTime::UNIX_EPOCH + time::Duration::seconds(100); + let preconditions = HTTPPreconditions { + if_modified_since: Some(OffsetDateTime::UNIX_EPOCH + time::Duration::seconds(100)), + ..Default::default() + }; + let state = ObjectPreconditionState { + mod_time: Some(mod_time), + ..Default::default() + }; + + assert_eq!(state.check(Some(&preconditions)), Err(ObjectPreconditionError::NotModified)); + + let preconditions = HTTPPreconditions { + if_unmodified_since: Some(OffsetDateTime::UNIX_EPOCH + time::Duration::seconds(99)), + ..Default::default() + }; + + assert_eq!(state.check(Some(&preconditions)), Err(ObjectPreconditionError::PreconditionFailed)); + } + #[test] fn version_marker_parses_null_and_uuid_markers() { assert_eq!(VersionMarker::parse("null").expect("null marker should parse"), VersionMarker::Null); diff --git a/docs/architecture/crate-boundaries.md b/docs/architecture/crate-boundaries.md index ba7a1bf74..6e1dbff66 100644 --- a/docs/architecture/crate-boundaries.md +++ b/docs/architecture/crate-boundaries.md @@ -95,6 +95,7 @@ Required `rustfs-storage-api` public re-exports: - `pub use error::{StorageErrorCode, StorageResult};` - `pub use multipart::{CompletePart, ListMultipartsInfo, ListPartsInfo, MultipartInfo, MultipartUploadResult, PartInfo};` - `pub use object::{HTTPPreconditions, HTTPRangeError, HTTPRangeSpec, ObjectLockRetentionOptions};` +- `pub use object::{ObjectPreconditionError, ObjectPreconditionPart, ObjectPreconditionState};` - `pub use object::{VersionMarker, WalkVersionsSortOrder};` ECStore must keep compile-time coverage for both `StorageAdminApi` and the diff --git a/docs/architecture/migration-progress.md b/docs/architecture/migration-progress.md index fc8c7c793..3598ded28 100644 --- a/docs/architecture/migration-progress.md +++ b/docs/architecture/migration-progress.md @@ -5,19 +5,18 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block ## Current Context - Issue: [`rustfs/backlog#660`](https://github.com/rustfs/backlog/issues/660) -- Branch: `overtrue/arch-object-list-helper-contracts` -- Baseline: `overtrue/arch-range-contracts` at `829bc62095f0fd853747cd6d7d77f8558baf24a2` - over `origin/main` at `a9aba323c6512e6b99c3137258b97b6058075ce9` +- Branch: `overtrue/arch-object-precondition-contracts` +- Baseline: `overtrue/arch-object-list-helper-contracts` at `65b56e1fb4fa4c65c167e2f434bf5fcb2a0b0cd4f` + over `overtrue/arch-range-contracts` at `829bc62095f0fd853747cd6d7d77f8558baf24a2` - PR type for this branch: `api-extraction` - Runtime behavior changes: no external behavior change expected. -- Rust code changes: move the pure `VersionMarker` and - `WalkVersionsSortOrder` list helper contracts into `rustfs-storage-api`, - then keep ECStore list/walk implementations and filemeta adaptation at the - ECStore boundary. -- CI/script changes: extend migration guards for list helper public re-exports - and reject restoring the old ECStore-owned list helper definitions or +- Rust code changes: move pure object HTTP precondition evaluation into + `rustfs-storage-api` as `ObjectPreconditionState`, + `ObjectPreconditionPart`, and `ObjectPreconditionError`, then keep + `ObjectOptions`, `ObjectInfo`, and ECStore error mapping in ECStore. +- CI/script changes: extend migration guards for object precondition contract public re-exports. -- Docs changes: record the list helper contract extraction slice. +- Docs changes: record the object precondition contract extraction slice. ## Phase 0 Tasks @@ -599,6 +598,27 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block migration/layer guards, formatting, diff hygiene, Rust risk scan, and required three-expert review passed. +- [x] `API-018` Move object precondition helper contracts. + - Completed slice: add `ObjectPreconditionState`, + `ObjectPreconditionPart`, and `ObjectPreconditionError` to + `rustfs-storage-api`; make ECStore `ObjectOptions::precondition_check` + adapt `ObjectInfo` into the shared pure contract and map the contract + result back to the existing ECStore errors. + - Acceptance: `rustfs-storage-api` exports the precondition helper contracts, + ECStore keeps `ObjectOptions` and `ObjectInfo`, and migration guards reject + dropping the public precondition contract re-export. + - Must preserve: requested-part validation, empty condition handling, + `If-None-Match`/`If-Modified-Since` `NotModified` behavior, + `If-Match`/`If-Unmodified-Since` `PreconditionFailed` behavior, wildcard + ETag matching, and ECStore error mapping. + - Risk defense: only pure precondition decision state and result contracts + cross into `rustfs-storage-api`; ECStore keeps object metadata adaptation, + storage error types, `ObjectOptions`, `ObjectInfo`, readers, + lifecycle/replication coupling, and storage implementation bodies. + - Verification: focused storage-api tests, ECStore/RustFS/downstream compile + checks, migration/layer guards, formatting, diff hygiene, Rust risk scan, + and required three-expert review passed. + ## Phase 8 Background Controller Tasks - [x] `BGC-001` Inventory background services. @@ -875,39 +895,43 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block | Expert | Status | Notes | |---|---|---| -| Quality/architecture | passed | Pure object list helper contracts now live in `rustfs-storage-api`; implementation-heavy list/walk contracts and ECStore behavior stay in ECStore. | -| Migration preservation | passed | The slice changes helper type ownership and import paths only; null version marker parsing, first-entry version marker behavior, walk sort default, and ECStore filemeta/list behavior are unchanged. | -| Testing/verification | passed | Focused storage-api/ECStore/RustFS/downstream compile checks, migration/layer guards, formatting, diff hygiene, Rust risk scan, and full `make pre-commit` passed. | +| Quality/architecture | passed | Pure precondition decision state and result contracts now live in `rustfs-storage-api`; ECStore still owns object metadata adaptation and storage error mapping. | +| Migration preservation | passed | Requested-part validation, ETag/date precondition priority, empty condition handling, and existing ECStore `Error` mapping are preserved. | +| Testing/verification | passed | Focused storage-api/ECStore tests, compile checks, migration/layer guards, formatting, diff hygiene, Rust risk scan, and full `make pre-commit` passed. | ## Verification Notes Passed before push: - `cargo test -p rustfs-storage-api`: passed. +- `cargo test -p rustfs-ecstore precondition_check_ignores_empty_etag_conditions`: passed. - `cargo check --tests -p rustfs-storage-api -p rustfs-ecstore -p rustfs -p rustfs-iam -p rustfs-scanner`: passed. - `./scripts/check_architecture_migration_rules.sh`: passed. - `./scripts/check_layer_dependencies.sh`: passed. - `cargo fmt --all --check`: passed. - `git diff --check`: passed. -- Rust risk scan: no new production `unwrap`/`expect`, panic/todo markers, - `unsafe`, process-spawning calls, lossy casts, println/eprintln, or relaxed - ordering in added Rust lines; new `expect` calls are test-only assertions. -- `make pre-commit`: passed. +- Rust risk scan: no new `unwrap`/`expect`, panic/todo markers, `unsafe`, + process-spawning calls, lossy casts, println/eprintln, or relaxed ordering in + added Rust lines. +- `make pre-commit`: passed; nextest reported 6164 tests passed and 111 + skipped, and doctests passed. Notes: - This slice follows the range helper contract branch and keeps the old aggregate facade, bucket DTO, multipart DTO, bucket operation contract, object - helper, range helper, and list helper contract guards active. -- The shared list marker/sort helper contracts are now owned by - `rustfs-storage-api`; ECStore keeps implementation-heavy object/list result - DTOs, walk options with filemeta filters, readers, lifecycle/replication, rio, - filemeta, and storage error contracts. -- The slice does not alter list, walk, object, multipart, or bucket runtime - behavior. + helper, range helper, list helper, and object precondition contract guards + active. +- The shared precondition helper contract is now owned by + `rustfs-storage-api`; ECStore keeps `ObjectOptions`, `ObjectInfo`, object + metadata adaptation, storage error mapping, readers, lifecycle/replication, + rio, filemeta, and implementation behavior. +- The slice does not alter object, list, walk, multipart, bucket, or reader + runtime behavior. ## Handoff Notes -- List helper contract cleanup is stacked on the range helper contract branch. +- Object precondition contract cleanup is stacked on the list helper contract + branch. - After this lands, remaining storage work can continue by extracting larger low-coupling DTO slices or by narrowing remaining operation-group consumers. diff --git a/scripts/check_architecture_migration_rules.sh b/scripts/check_architecture_migration_rules.sh index 9c05ec30a..9b32e64a9 100755 --- a/scripts/check_architecture_migration_rules.sh +++ b/scripts/check_architecture_migration_rules.sh @@ -196,6 +196,10 @@ require_source_line \ "crates/storage-api/src/lib.rs" \ "pub use object::{HTTPPreconditions, HTTPRangeError, HTTPRangeSpec, ObjectLockRetentionOptions};" \ "storage-api public object helper contract re-export" +require_source_line \ + "crates/storage-api/src/lib.rs" \ + "pub use object::{ObjectPreconditionError, ObjectPreconditionPart, ObjectPreconditionState};" \ + "storage-api public object precondition contract re-export" require_source_line \ "crates/storage-api/src/lib.rs" \ "pub use object::{VersionMarker, WalkVersionsSortOrder};" \ @@ -246,7 +250,7 @@ fi ( cd "$ROOT_DIR" - rg -n --no-heading 'pub(?:\(crate\))? use rustfs_storage_api(?:::\{[^}]*\b(?:HTTPPreconditions|ObjectLockRetentionOptions)\b|::(?:HTTPPreconditions|ObjectLockRetentionOptions)\b)|pub struct (?:HTTPPreconditions|ObjectLockRetentionOptions)\b' \ + rg -n --no-heading 'pub(?:\(crate\))? use rustfs_storage_api(?:::\{[^}]*\b(?:HTTPPreconditions|ObjectLockRetentionOptions|ObjectPreconditionError|ObjectPreconditionPart|ObjectPreconditionState)\b|::(?:HTTPPreconditions|ObjectLockRetentionOptions|ObjectPreconditionError|ObjectPreconditionPart|ObjectPreconditionState)\b)|pub (?:enum ObjectPreconditionError|struct (?:HTTPPreconditions|ObjectLockRetentionOptions|ObjectPreconditionPart|ObjectPreconditionState))\b' \ crates/ecstore/src/store_api.rs crates/ecstore/src/store_api/types.rs || true ) >"$STORE_API_OBJECT_HELPER_REEXPORTS_FILE"