feat(webdav): complete RFC 3253 core DeltaV contracts - #31
Conversation
|
Warning Review limit reached
Next review available in: 43 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughPR 完成 RFC 3253 核心 DeltaV 契约：新增版本能力规划、类型化 REPORT、VERSION-CONTROL、受限 ChangesDeltaV 核心协议
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aster_forge_webdav/src/put.rs (1)
146-155: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win既然引入了
snapshot，那两条 405 路径就应该都带上Allowheader。这次改动给
put_plan_error_response加了snapshot参数。但MethodNotAllowed和CollectionTarget仍走no_store_empty_response(StatusCode::METHOD_NOT_ALLOWED)，它只设Cache-Control: no-store，不设Allow。而新增的
Versioning分支（172-173 行）经versioning_precondition_response走到crate::method_not_allowed_response(snapshot)，那条路径会带Allow（tests/deltav.rs 1081 行断言了这一点）。结果是同一个 PUT 端点在两种拒绝情况下返回形状不同的 405。RFC 7231 section 6.5.5 要求 405 必须携带
Allow。snapshot 现在就在手边，直接用它。
🐛 建议改动
match error { DavPutPlanError::MethodNotAllowed | DavPutPlanError::CollectionTarget => { - no_store_empty_response(StatusCode::METHOD_NOT_ALLOWED) + crate::method_not_allowed_response(snapshot) }
method_not_allowed_response是否已包含Cache-Control: no-store需要确认。若未包含，请在返回前补上，以保持既有的缓存语义。#!/bin/bash ast-grep run --pattern 'pub fn method_not_allowed_response($$$) -> $RET { $$$ }' --lang rust crates/aster_forge_webdav/src🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_webdav/src/put.rs` around lines 146 - 155, Update put_plan_error_response so the MethodNotAllowed and CollectionTarget branches use method_not_allowed_response(snapshot), ensuring both 405 responses include Allow from the capability snapshot. Preserve the existing no-store caching behavior by verifying that method_not_allowed_response provides Cache-Control: no-store and adding it there if necessary.
🧹 Nitpick comments (5)
crates/aster_forge_webdav/tests/capability.rs (1)
899-943: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win三个新错误变体没有失败路径断言。
这个测试覆盖了
AutoCheckoutLockNotApplicable、AutoCheckoutLockWithoutApplicableMode、VersionDeletePolicyNotApplicable，写得很干净。但validate_extensions这次一共加了六条规则，剩下三个变体全程没人管：
VersionControlWithoutTarget：声明了VersionControl包，但state留在Unsupported。VersioningTargetWithoutPackage：设了state，忘了加包。AutoVersionNotApplicable：auto_version != None配上Versionable或Version。前两条是整个版本事实一致性的门禁——
plan_capabilities后面所有的属性过滤、报告过滤、方法过滤都建立在"包与状态必须成对出现"这个前提上。这道门坏了没人会发现。编码规范要求高影响集成同时测试成功路径和失败路径。照你现有的模式补三段就行，成本很低。
✅ 建议补充的断言
assert_eq!( plan_capabilities(invalid_delete), Err(DavCapabilityPlanError::VersionDeletePolicyNotApplicable) ); + + let mut package_without_state = declaration(DavResourceState::File, &[DavMethod::Options]); + package_without_state.extensions = + DavExtensionSet::from_packages(&[DavExtensionPackage::VersionControl]); + assert_eq!( + plan_capabilities(package_without_state), + Err(DavCapabilityPlanError::VersionControlWithoutTarget) + ); + + let mut state_without_package = declaration(DavResourceState::File, &[DavMethod::Options]); + state_without_package.versioning = DavVersioningCapabilities { + state: DavVersioningState::CheckedIn, + ..DavVersioningCapabilities::default() + }; + assert_eq!( + plan_capabilities(state_without_package), + Err(DavCapabilityPlanError::VersioningTargetWithoutPackage) + ); + + let mut auto_version_on_versionable = + declaration(DavResourceState::File, &[DavMethod::Options]); + auto_version_on_versionable.extensions = + DavExtensionSet::from_packages(&[DavExtensionPackage::VersionControl]); + auto_version_on_versionable.versioning = DavVersioningCapabilities { + state: DavVersioningState::Versionable, + auto_version: DavAutoVersion::CheckoutCheckin, + ..DavVersioningCapabilities::default() + }; + assert_eq!( + plan_capabilities(auto_version_on_versionable), + Err(DavCapabilityPlanError::AutoVersionNotApplicable) + ); }依据编码规范："High-impact integrations must test success and failure paths, error-mapping boundaries" 以及 "integration tests should cover the behavior required by the changed mechanism"。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_webdav/tests/capability.rs` around lines 899 - 943, 在 deltav_runtime_facts_reject_inapplicable_auto_checkout_and_delete_policy 中按现有测试模式补充三个失败路径断言：VersionControl 包配合 Unsupported 状态应返回 VersionControlWithoutTarget；设置版本化 state 但不包含 VersionControl 包应返回 VersioningTargetWithoutPackage；在 Versionable 或 Version 状态下设置非 None 的 auto_version 应返回 AutoVersionNotApplicable。确保每个场景都通过 plan_capabilities 验证对应错误变体。Source: Coding guidelines
crates/aster_forge_webdav/tests/deltav.rs (2)
807-807: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value两个测试的名字和它们实际断言的内容不符。
expand_property_enforces_cycle_limits_cancellation_and_backend_failure（807 行）没有任何 cancellation 断言，也没有 backend failure 断言。那两项在expand_property_checks_cancellation_before_and_between_backend_lookups和expand_property_preserves_backend_failure_classification里。
expand_property_enforces_depth_property_value_and_output_byte_boundaries（928 行）没有 property value 断言，也没有 output byte 断言。那两项在expand_property_rejects_non_href_nested_values_and_bounds_output_bytes里。名字撒谎的测试比没有测试更麻烦——下次有人 grep "cancellation" 会以为已经覆盖了。改成
expand_property_enforces_cycle_and_resource_limits和expand_property_enforces_depth_and_property_count_limits就行。♻️ 建议改名
-fn expand_property_enforces_cycle_limits_cancellation_and_backend_failure() { +fn expand_property_enforces_cycle_and_resource_limits() {-fn expand_property_enforces_depth_property_value_and_output_byte_boundaries() { +fn expand_property_enforces_depth_and_property_count_limits() {Also applies to: 928-928
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_webdav/tests/deltav.rs` at line 807, Rename the test at `expand_property_enforces_cycle_limits_cancellation_and_backend_failure` to `expand_property_enforces_cycle_and_resource_limits`, and rename `expand_property_enforces_depth_property_value_and_output_byte_boundaries` to `expand_property_enforces_depth_and_property_count_limits`. Change only these test function names; leave their assertions and implementations unchanged.
205-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win几个限制和错误映射没有测试覆盖。
覆盖面整体很扎实，取消时序那部分（863-901 行）尤其精确。但有几处漏了：
report_limits_cover_input_xml_depth_and_nested_selection_depth（205-246 行）：测试名说覆盖 xml depth，实际maximum_xml_depth一直固定为 8，没有精确边界用例。DavReportLimits::is_valid检查 5 个字段，测试只覆盖maximum_input_bytes = 0一种零值。report_errors_map_xml_unknown_and_unavailable_categories（248-284 行）：没测DavReportPlanError::InvalidLimits映射到 500。version_control_plans_versionable_and_already_controlled_targets（286-324 行）：只测了CheckedOut→AlreadyControlled，没测CheckedIn。也没测Version和Unsupported状态下 VERSION-CONTROL 被拒绝。docs/crates/aster_forge_webdav.md 的 569-570 行明确要求覆盖 "versionable/checked-in/checked-out/immutable/collection/unmapped planner"。expand_property_failures_map_before_response_start（1088-1120 行）：漏了ResourceLimitExceeded、PropertyLimitExceeded、InvalidLimits，以及MultiStatus的各个子类映射。immutable version 上 VERSION-CONTROL 必须被拒绝，这条是 RFC 3253 的硬要求，我建议优先补。
As per coding guidelines: "High-impact integrations must test success and failure paths, error-mapping boundaries, retry/degradation/cancellation behavior".
💚 建议补充的用例
#[test] fn version_control_is_rejected_for_immutable_and_unsupported_targets() { for state in [DavVersioningState::Version, DavVersioningState::Unsupported] { let snapshot = versioning_snapshot( DavResourceState::File, state, &[DavMethod::Options, DavMethod::VersionControl], ); assert_eq!( plan_version_control_request(&snapshot, b""), Err(DavVersionControlPlanError::MethodNotAllowed), "{state:?}" ); } let checked_in = versioning_snapshot( DavResourceState::File, DavVersioningState::CheckedIn, &[DavMethod::Options, DavMethod::VersionControl], ); assert_eq!( plan_version_control_request(&checked_in, b"") .expect("checked-in plan") .action, DavVersionControlAction::AlreadyControlled ); } #[test] fn report_limits_reject_every_zero_field_and_map_to_a_server_error() { let snapshot = report_snapshot(DavVersioningState::CheckedIn); let body = br#"<D:version-tree xmlns:D="DAV:"/>"#; for limits in [ DavReportLimits::new(0, 8, 2, 8, 8, DavMultiStatusLimits::default()), DavReportLimits::new(4096, 0, 2, 8, 8, DavMultiStatusLimits::default()), DavReportLimits::new(4096, 8, 0, 8, 8, DavMultiStatusLimits::default()), DavReportLimits::new(4096, 8, 2, 0, 8, DavMultiStatusLimits::default()), DavReportLimits::new(4096, 8, 2, 8, 0, DavMultiStatusLimits::default()), ] { assert_eq!( plan_report_request_with_limits(&snapshot, body, None, limits), Err(DavReportPlanError::InvalidLimits) ); } let response = report_plan_error_response( &DavReportPlanError::InvalidLimits, &ReportResponsePolicy, ) .expect("invalid limits response"); assert_eq!(response.status, StatusCode::INTERNAL_SERVER_ERROR); }Also applies to: 248-284, 286-324, 1088-1120
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_webdav/tests/deltav.rs` around lines 205 - 246, Expand the affected WebDAV tests to cover every zero-valued field validated by DavReportLimits::is_valid, including the maximum_xml_depth boundary, and verify InvalidLimits maps to INTERNAL_SERVER_ERROR through report_plan_error_response. Extend version-control planning tests around plan_version_control_request to reject Version and Unsupported immutable targets while preserving CheckedIn’s AlreadyControlled result. Add expand-property failure cases for ResourceLimitExceeded, PropertyLimitExceeded, InvalidLimits, and each MultiStatus error mapping in the existing response-mapping test.Source: Coding guidelines
crates/aster_forge_webdav/src/xml.rs (1)
484-486: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value零限制检查的错误分类不对，而且这条路径走不到。
plan_report_request_with_limits在 189-191 行已经用limits.is_valid()拦截了任何为零的限制，返回DavReportPlanError::InvalidLimits。所以这里的 484 行判断在实际调用链上不可达。即使假设未来有别的调用方，把"服务器配置了零限制"分类成
DavXmlError::InvalidGrammar也是错的——它会让xml_request_error_response生成 400，把服务器配置错误算到客户端头上。InvalidLimits对应的是 500。建议把这个判断改成
debug_assert!，或者直接删掉，让唯一的限制校验入口留在DavReportLimits::is_valid。♻️ 建议改动
fn parse_expand_property<S: AsRef<[u8]>>( root: ElementRef<'_, S>, maximum_depth: usize, maximum_properties: usize, ) -> Result<Vec<DavExpandPropertySelection>, DavXmlError> { - if maximum_depth == 0 || maximum_properties == 0 { - return Err(DavXmlError::InvalidGrammar); - } + debug_assert!( + maximum_depth != 0 && maximum_properties != 0, + "callers must validate limits through DavReportLimits::is_valid" + ); require_element_content(root)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_webdav/src/xml.rs` around lines 484 - 486, Remove the zero-limit check in the XML parsing path around maximum_depth and maximum_properties, or replace it with a debug_assert! that treats zero limits as an internal invariant violation rather than DavXmlError::InvalidGrammar. Keep limit validation exclusively in DavReportLimits::is_valid and preserve InvalidLimits handling in plan_report_request_with_limits.crates/aster_forge_webdav/tests/xml_response.rs (1)
5-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win删掉的 DeltaV 转义测试没有在新位置补回来。
随导入一起删除的
deltav_multistatus_escapes_values_and_keeps_protocol_property_order覆盖了两件事：值转义和协议属性顺序。顺序那部分现在由 tests/deltav.rs 的
version_tree_response_groups_success_missing_unknown_and_backend_errors间接覆盖。转义那部分没有替代——新测试断言的是xml.contains("V1")和xml.contains("Z:custom")这类普通字符串，没有任何用例把<、&、"或控制字符喂给DavVersionProperty::text或DavVersionProperty::hrefs。产品返回的 version-name、comment、creator 都是任意文本，href 也可能带
&。这条路径现在没有转义测试。docs/crates/aster_forge_webdav.md 的 550 行把"异常旧值转义"列为 XML response 测试要求。
As per coding guidelines: "Test code may be direct, but must not hide real boundary problems; integration tests should cover the behavior required by the changed mechanism."
💚 建议在 tests/deltav.rs 补充
#[test] fn version_tree_response_escapes_product_text_and_hrefs() { let request = DavVersionTreeRequest { properties: Some(vec![dav_property("comment"), dav_property("predecessor-set")]), depth: Depth::Zero, }; let response = version_tree_response( &request, vec![DavVersionReportItem { href: "/versions/a&b".to_owned(), properties: vec![ DavVersionProperty::text(dav_property("comment"), r#"<script>&"'"#), DavVersionProperty::hrefs( dav_property("predecessor-set"), ["/versions/x?y=1&z=2".to_owned()], ), ], }], ) .expect("escaped version-tree response"); let xml = body_text(&response); assert!(!xml.contains("<script>"), "{xml}"); assert!(xml.contains("<script>"), "{xml}"); assert!(xml.contains("&"), "{xml}"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_webdav/tests/xml_response.rs` around lines 5 - 8, 在 tests/deltav.rs 的 version_tree_response 测试区域补回 XML 转义覆盖，新增针对 version_tree_response 的测试，向 DavVersionProperty::text 和 DavVersionProperty::hrefs 传入包含 <、&、引号及控制字符的文本与 href，并断言响应只包含对应的 XML 转义结果而不包含未转义内容。保留现有协议属性顺序测试，不重复改动其覆盖范围。Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aster_forge_webdav/src/capability.rs`:
- Around line 1188-1205: 将 VersionControlMethodNotApplicable 的状态适用性检查移到
extension_methods_for_declaration 与 extension-method 循环之前，并仅在声明包含 VersionControl
时执行；保留无包声明仍返回 ExtensionMethodWithoutPackage 的现有行为，同时确保 Version、Unsupported
等不适用状态返回 VersionControlMethodNotApplicable。
- Around line 1148-1163: 在 validate_compliance 和 validate_extensions 中补充
declaration.versioning.write_locked 与 declaration.locking 的一致性校验：write_locked 为
true 时必须具备启用的 DAV 写锁能力，禁止 locking 为
DavLockingCapability::Disabled；若该字段表示产品内部锁，则明确其语义和适用边界，并避免让 deltav.rs 将其误当作 DAV
锁使用。
In `@crates/aster_forge_webdav/src/deltav.rs`:
- Around line 238-241: Remove the hard-coded "Invalid REPORT limits" response
body from both InvalidLimits branches in the REPORT response handling, including
expand_property_error_response, and return the same empty-body response used by
the other branches. Preserve the existing INTERNAL_SERVER_ERROR status while
leaving product-facing messaging to the API layer.
- Around line 707-709: 统一 DavReportLimits 中解析阶段与执行阶段的预算语义：为请求 AST 深度/selection
数量和实际资源展开跳数/后端查询次数使用独立且明确命名的限制字段。同步更新
parse_expand_property_selection、plan_report_request_with_limits、execute_expand_property
及其调用方、测试和字段文档，确保各阶段只使用对应预算并保留超限错误行为。
- Around line 795-797: Update the expand-property handling around
DavExpandPropertyValue::Element so a client-requested non-href nested value is
reported in that property’s propstat with the appropriate 403 or 404 status
while processing of other selections continues. Remove InvalidPropertyValue from
the client-error path, or retain it only for genuine internal invariant
violations; update expand_property_error_response and
expand_property_rejects_non_href_nested_values_and_bounds_output_bytes to verify
the REPORT no longer becomes a 500.
In `@crates/aster_forge_webdav/src/lib.rs`:
- Around line 84-95: 更新 docs/crates/aster_forge_webdav.md，在 DeltaV 或版本树响应说明中补充
DavVersionXml 与 dav_version_multistatus_bytes 已移除的迁移说明，明确引导使用
version_tree_response 或 version_tree_response_with_limits，并保持现有 API 文档结构不变。
In `@crates/aster_forge_webdav/src/mutation.rs`:
- Around line 955-957: 在处理 DavMutationStop::Backend 的分支中，先将当前
frames[frame].failed 设为 true，再调用 propagate_frame_failure(frame,
frames)，确保当前帧及其父帧正确传播失败状态；同时补充或更新测试，断言该停止路径会标记当前帧失败并验证适用的父帧传播行为。
In `@crates/aster_forge_webdav/src/xml.rs`:
- Around line 530-537: 在解析 property 的 namespace 处补充 URI
语法校验，不能仅检查非空和首尾空白；应拒绝尖括号、引号及控制字符等无法作为 XMLNS 值的内容，并返回
DavXmlError::InvalidGrammar。优先复用现有 URI 校验能力；否则在 writer 的
validate_namespace_binding 中统一执行该校验，确保 deltav 与 xml_response 写入 namespaces
前不会透传非法值。
---
Outside diff comments:
In `@crates/aster_forge_webdav/src/put.rs`:
- Around line 146-155: Update put_plan_error_response so the MethodNotAllowed
and CollectionTarget branches use method_not_allowed_response(snapshot),
ensuring both 405 responses include Allow from the capability snapshot. Preserve
the existing no-store caching behavior by verifying that
method_not_allowed_response provides Cache-Control: no-store and adding it there
if necessary.
---
Nitpick comments:
In `@crates/aster_forge_webdav/src/xml.rs`:
- Around line 484-486: Remove the zero-limit check in the XML parsing path
around maximum_depth and maximum_properties, or replace it with a debug_assert!
that treats zero limits as an internal invariant violation rather than
DavXmlError::InvalidGrammar. Keep limit validation exclusively in
DavReportLimits::is_valid and preserve InvalidLimits handling in
plan_report_request_with_limits.
In `@crates/aster_forge_webdav/tests/capability.rs`:
- Around line 899-943: 在
deltav_runtime_facts_reject_inapplicable_auto_checkout_and_delete_policy
中按现有测试模式补充三个失败路径断言：VersionControl 包配合 Unsupported 状态应返回
VersionControlWithoutTarget；设置版本化 state 但不包含 VersionControl 包应返回
VersioningTargetWithoutPackage；在 Versionable 或 Version 状态下设置非 None 的
auto_version 应返回 AutoVersionNotApplicable。确保每个场景都通过 plan_capabilities 验证对应错误变体。
In `@crates/aster_forge_webdav/tests/deltav.rs`:
- Line 807: Rename the test at
`expand_property_enforces_cycle_limits_cancellation_and_backend_failure` to
`expand_property_enforces_cycle_and_resource_limits`, and rename
`expand_property_enforces_depth_property_value_and_output_byte_boundaries` to
`expand_property_enforces_depth_and_property_count_limits`. Change only these
test function names; leave their assertions and implementations unchanged.
- Around line 205-246: Expand the affected WebDAV tests to cover every
zero-valued field validated by DavReportLimits::is_valid, including the
maximum_xml_depth boundary, and verify InvalidLimits maps to
INTERNAL_SERVER_ERROR through report_plan_error_response. Extend version-control
planning tests around plan_version_control_request to reject Version and
Unsupported immutable targets while preserving CheckedIn’s AlreadyControlled
result. Add expand-property failure cases for ResourceLimitExceeded,
PropertyLimitExceeded, InvalidLimits, and each MultiStatus error mapping in the
existing response-mapping test.
In `@crates/aster_forge_webdav/tests/xml_response.rs`:
- Around line 5-8: 在 tests/deltav.rs 的 version_tree_response 测试区域补回 XML
转义覆盖，新增针对 version_tree_response 的测试，向 DavVersionProperty::text 和
DavVersionProperty::hrefs 传入包含 <、&、引号及控制字符的文本与 href，并断言响应只包含对应的 XML
转义结果而不包含未转义内容。保留现有协议属性顺序测试，不重复改动其覆盖范围。
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b0b7ab4-61f0-488b-aeea-7a2f90aaf419
📒 Files selected for processing (19)
crates/aster_forge_webdav/src/capability.rscrates/aster_forge_webdav/src/deltav.rscrates/aster_forge_webdav/src/lib.rscrates/aster_forge_webdav/src/mutation.rscrates/aster_forge_webdav/src/put.rscrates/aster_forge_webdav/src/request.rscrates/aster_forge_webdav/src/xml.rscrates/aster_forge_webdav/src/xml_response.rscrates/aster_forge_webdav/tests/actix.rscrates/aster_forge_webdav/tests/capability.rscrates/aster_forge_webdav/tests/deltav.rscrates/aster_forge_webdav/tests/mutation.rscrates/aster_forge_webdav/tests/preference.rscrates/aster_forge_webdav/tests/property.rscrates/aster_forge_webdav/tests/put.rscrates/aster_forge_webdav/tests/response.rscrates/aster_forge_webdav/tests/xml.rscrates/aster_forge_webdav/tests/xml_response.rsdocs/crates/aster_forge_webdav.md
Add typed REPORT parsing, bounded version-tree and expand-property composition, versioning resource facts, VERSION-CONTROL transaction planning, mutation preconditions, one-snapshot discovery consistency, Actix coverage, and complete boundary tests.\n\nCloses #30
e6f5574 to
063fc6a
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
crates/aster_forge_webdav/tests/capability.rs (2)
933-947: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
AutoCheckoutLockWithoutApplicableMode少测了DavAutoVersion::None这条路。现在只测了
CheckoutCheckin。但None同样会落到capability.rs第 1159 行的分支：state == CheckedOut时第 1142 行的auto_version != None为假，不会提前拦截，于是auto_checkout_lock校验放行到模式检查。这条路径是
auto_checkout_lock与auto_version之间最容易被误改的耦合点。补一个用例即可。💚 建议补充用例
assert_eq!( plan_capabilities(invalid_mode), Err(DavCapabilityPlanError::AutoCheckoutLockWithoutApplicableMode) ); + + let mut no_mode = declaration(DavResourceState::File, &locking_methods); + no_mode.locking = DavLockingCapability::Class2; + no_mode.extensions = DavExtensionSet::from_packages(&[DavExtensionPackage::VersionControl]); + no_mode.versioning = DavVersioningCapabilities { + state: DavVersioningState::CheckedOut, + auto_version: DavAutoVersion::None, + write_locked: true, + auto_checkout_lock: true, + allow_version_delete: false, + }; + assert_eq!( + plan_capabilities(no_mode), + Err(DavCapabilityPlanError::AutoCheckoutLockWithoutApplicableMode) + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_webdav/tests/capability.rs` around lines 933 - 947, 在 capability 测试中补充覆盖 DavAutoVersion::None 的用例，使用现有 invalid_mode 配置并将 auto_version 设为 None，保持 state 为 CheckedOut、auto_checkout_lock 为 true 以及无适用模式的条件不变。断言 plan_capabilities(invalid_mode) 仍返回 AutoCheckoutLockWithoutApplicableMode，并保留现有 CheckoutCheckin 用例。
438-453: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
versioning_resource的硬编码资源列表会与 descriptor 脱钩。第 438-441 行手写了
File | Collection | MountRoot。第 445 行已经用descriptor().resources.contains(resource)过滤过一次。这两处描述同一件事，但一个是常量、一个是数据源。后果不是现在坏，是以后坏：
DavVersionControlExtension的resources一旦加进Principal，第 445 行会放行，第 446 行会拦下，于是这个资源状态的 VersionControl 组合永远不进测试。你小子改了 descriptor 还以为测试覆盖了。直接从 descriptor 推导即可。
♻️ 可选重构：从 descriptor 推导
- let versioning_resource = matches!( - resource, - DavResourceState::File | DavResourceState::Collection | DavResourceState::MountRoot - ); + let versioning_resource = DavExtensionPackage::VersionControl + .descriptor() + .resources + .contains(resource);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aster_forge_webdav/tests/capability.rs` around lines 438 - 453, Remove the hard-coded resource list used to compute versioning_resource and derive it from the VersionControl package descriptor instead. Update the filtering around DavExtensionPackage::VersionControl so any resource declared by that descriptor receives the version-control combination, keeping the existing prerequisite and package filtering behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aster_forge_xml/src/syntax.rs`:
- Around line 77-83: Update is_valid_xml_namespace_name and
validate_namespace_binding to perform complete URI-reference validation,
rejecting incomplete or invalid percent-encoded sequences such as urn:bad%,
urn:bad%2, urn:bad%ZZ, and urn:%0G rather than only applying character
blacklists. Preserve the existing empty-value exception used to remove the
default namespace, and add regression coverage for at least urn:bad% and
urn:bad%2 so reader and writer paths inherit the corrected behavior.
In `@docs/crates/aster_forge_webdav.md`:
- Line 385: 补充该 DeltaV 语义说明中的 RFC 3253 引用，覆盖 PUT、PROPPATCH、DELETE、COPY、MOVE 和
UNLOCK 对应的 Section 3.9、3.10–3.16，确保引用范围覆盖 planner 合同；或者将表述收窄为仅涵盖
VERSION-CONTROL、version-tree 和 expand-property。
- Around line 117-119: 更新文档中 `expand-property` 的描述：将“只接受 DTD 声明的 nested
`DAV:property`”改为“只接受符合 RFC grammar 的 nested `DAV:property`”，明确 RFC grammar
描述的是请求结构而非客户端必须携带 DTD，并保留后文对 DTD/ENTITY 的拒绝行为。
---
Nitpick comments:
In `@crates/aster_forge_webdav/tests/capability.rs`:
- Around line 933-947: 在 capability 测试中补充覆盖 DavAutoVersion::None 的用例，使用现有
invalid_mode 配置并将 auto_version 设为 None，保持 state 为 CheckedOut、auto_checkout_lock
为 true 以及无适用模式的条件不变。断言 plan_capabilities(invalid_mode) 仍返回
AutoCheckoutLockWithoutApplicableMode，并保留现有 CheckoutCheckin 用例。
- Around line 438-453: Remove the hard-coded resource list used to compute
versioning_resource and derive it from the VersionControl package descriptor
instead. Update the filtering around DavExtensionPackage::VersionControl so any
resource declared by that descriptor receives the version-control combination,
keeping the existing prerequisite and package filtering behavior unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c74a5020-4499-4f71-bcab-316d7997429d
📒 Files selected for processing (22)
crates/aster_forge_webdav/src/capability.rscrates/aster_forge_webdav/src/deltav.rscrates/aster_forge_webdav/src/lib.rscrates/aster_forge_webdav/src/put.rscrates/aster_forge_webdav/src/request.rscrates/aster_forge_webdav/src/xml.rscrates/aster_forge_webdav/src/xml_response.rscrates/aster_forge_webdav/tests/actix.rscrates/aster_forge_webdav/tests/capability.rscrates/aster_forge_webdav/tests/deltav.rscrates/aster_forge_webdav/tests/preference.rscrates/aster_forge_webdav/tests/property.rscrates/aster_forge_webdav/tests/put.rscrates/aster_forge_webdav/tests/response.rscrates/aster_forge_webdav/tests/xml.rscrates/aster_forge_webdav/tests/xml_response.rscrates/aster_forge_xml/src/lib.rscrates/aster_forge_xml/src/syntax.rscrates/aster_forge_xml/src/writer.rscrates/aster_forge_xml/tests/writer.rscrates/aster_forge_xml/tests/xml.rsdocs/crates/aster_forge_webdav.md
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/aster_forge_webdav/tests/actix.rs
- crates/aster_forge_webdav/tests/property.rs
- crates/aster_forge_webdav/tests/xml_response.rs
- crates/aster_forge_webdav/src/request.rs
- crates/aster_forge_webdav/tests/preference.rs
- crates/aster_forge_webdav/src/put.rs
- crates/aster_forge_webdav/tests/put.rs
- crates/aster_forge_webdav/tests/response.rs
- crates/aster_forge_webdav/tests/xml.rs
- crates/aster_forge_webdav/src/lib.rs
- crates/aster_forge_webdav/src/xml.rs
- crates/aster_forge_webdav/src/deltav.rs
|
Latest review nitpicks are also addressed in 574207d:
Focused tests, both WebDAV feature sets, strict Clippy, workspace check, and workspace tests all pass. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Summary
version-treeand nestedexpand-propertyrequest ASTspropstatcomposition and bounded recursive expand-property executionContract details
version-treepropstatresponsesAllowremains consistentRemoved obsolete API
This intentionally removes the fixed or duplicate DeltaV paths rather than retaining compatibility facades:
validate_version_control_requestversion_control_request_error_responseDavVersionXmldav_version_multistatus_bytesversion_tree_non_file_responseValidation
cargo fmt --allcargo test -p aster_forge_webdavcargo test -p aster_forge_webdav --features actixcargo clippy -p aster_forge_webdav --all-targets --all-features -- -D warningscargo check --workspacecargo test --workspacegit diff --checkCloses #30
Summary by CodeRabbit
新功能
VERSION-CONTROL、版本树和expand-property报告支持。改进