refactor(webdav): migrate protocol logic to aster_forge_webdav crate#431
Closed
AptS-1547 wants to merge 1 commit into
Closed
refactor(webdav): migrate protocol logic to aster_forge_webdav crate#431AptS-1547 wants to merge 1 commit into
AptS-1547 wants to merge 1 commit into
Conversation
- Remove local implementations of `DavPath`, `DavFileSystem`, `DavLockSystem`, and related traits from `src/webdav/dav.rs`; replace all imports with `aster_forge_webdav::dav` - Delete in-tree WebDAV protocol modules (`props`, `protocol`, `responses`, `resources`, `locks`, `transfer`, `deltav`) and delegate the full protocol exchange to `aster_forge_webdav::handler::WebDavHandler` - Add `aster_forge_webdav` dependency to `Cargo.toml` - Replace local `is_system_namespace`, `is_dav_namespace`, and `is_protected_namespace` helpers in `property.rs` with imports from `aster_forge_webdav` - Replace local `normalize_path`, `path_ancestors`, `lock_paths_overlap`, `lock_path_is_under`, and `unlock_request_targets_lock_scope` helpers in `db_lock_system.rs` with imports from `aster_forge_webdav::lock_paths` - Replace local `parse_basic_authorization` / base64 decoding in `auth.rs` with `aster_forge_webdav::auth::parse_basic_authorization` - Introduce `DavPropertyTarget` and `DavResourceKind` from Forge in `metadata.rs`, replacing the local `(EntityType, i64)` tuple - Implement `DavNamePolicy` for `SystemFileBlockPolicy` to satisfy the Forge handler interface - Add `AsterDavVersionProvider` in `version_provider.rs` as a persistence adapter implementing `aster_forge_webdav::deltav::DavVersionProvider` - Implement `open_download_stream` and `copy_dir_shallow` on `AsterDavFs` to satisfy the updated `DavFileSystem` contract from Forge - Update all test imports (`tests/webdav/`) to reference types from `aster_forge_webdav` directly
AptS-1547
added
Enhancement
📝 WalkthroughWalkthrough本次变更引入 ChangesWebDAV 协议抽取与适配
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
3 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/webdav/mod.rs (1)
70-71: 🚀 Performance & Scalability | 🔵 Trivial版本历史是纯读路径，塞给 writer_db 有点浪费。
AsterDavVersionProvider只做 PROPFIND/REPORT 的版本历史读取，属于允许短暂滞后的纯读路径；旁边AsterDavFs::metadata走的是reader_db。这里用writer_db会把只读流量都压到主库上。规模上来后要疼的是你小子，不是我。若确无读后写一致性要求，建议改用reader_db。依据 coding guidelines：「reader_db 仅用于允许短暂滞后的纯读路径」。
🤖 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 `@src/webdav/mod.rs` around lines 70 - 71, Update the version_provider initialization in the WebDAV request flow to pass state.get_ref().reader_db() instead of writer_db(). Keep AsterDavVersionProvider::new and the surrounding authentication flow unchanged, preserving this version-history read path as a replica-eligible operation.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 `@src/webdav/fs/mod.rs`:
- Around line 401-419: 为 open_download_stream 和 copy_dir_shallow
补充单元或集成测试：验证文件全量读取、范围读取及底层读取失败时的错误映射；同时验证 copy_dir_shallow
成功复制目录内容，以及目标已存在时覆盖失败。复用现有普通下载和 range 测试的测试工具与断言方式，确保覆盖这两个方法对外暴露的新契约。
- Around line 690-692: 将 WebDAV 的 copy_dir_shallow 流程收敛到同一个 service/repository
写入边界，使用 writer transaction 统一编排目标删除、目录落库和属性迁移，确保任一步失败都回滚而不破坏旧目标。调整
AsterDavFs::copy_dir_shallow 及其调用的 folder::create_in_scope_with_audit，审计改为同一事务内的
outbox 或采用明确的失败处理策略。
---
Nitpick comments:
In `@src/webdav/mod.rs`:
- Around line 70-71: Update the version_provider initialization in the WebDAV
request flow to pass state.get_ref().reader_db() instead of writer_db(). Keep
AsterDavVersionProvider::new and the surrounding authentication flow unchanged,
preserving this version-history read path as a replica-eligible operation.
🪄 Autofix (Beta)
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: f500c326-afeb-40b8-b5a2-59cbb94b9e5a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
Cargo.tomlsrc/services/content/property.rssrc/webdav/auth.rssrc/webdav/dav.rssrc/webdav/db_lock_system.rssrc/webdav/deltav.rssrc/webdav/dir_entry.rssrc/webdav/file/mod.rssrc/webdav/file/tests.rssrc/webdav/fs/mod.rssrc/webdav/handler_tests/mod.rssrc/webdav/locks/mod.rssrc/webdav/metadata.rssrc/webdav/mod.rssrc/webdav/path_resolver.rssrc/webdav/props/mod.rssrc/webdav/protocol.rssrc/webdav/resources/mod.rssrc/webdav/responses.rssrc/webdav/system_file.rssrc/webdav/transfer/mod.rssrc/webdav/version_provider.rstests/webdav/file.rstests/webdav/lock_system.rstests/webdav/path_resolver.rs
💤 Files with no reviewable changes (8)
- src/webdav/props/mod.rs
- src/webdav/locks/mod.rs
- src/webdav/transfer/mod.rs
- src/webdav/protocol.rs
- src/webdav/resources/mod.rs
- src/webdav/responses.rs
- src/webdav/deltav.rs
- src/webdav/dav.rs
Comment on lines
+401
to
+419
| fn open_download_stream<'a>( | ||
| &'a self, | ||
| path: &'a DavPath, | ||
| offset: Option<u64>, | ||
| length: Option<u64>, | ||
| ) -> FsFuture<'a, FsStream<Bytes>> { | ||
| Box::pin(async move { | ||
| let target = self | ||
| .resolve_download_target(path) | ||
| .await? | ||
| .ok_or(FsError::Forbidden)?; | ||
| let reader = self | ||
| .open_download_stream_for_file(&target.file, &target.blob, offset, length) | ||
| .await?; | ||
| let stream = ReaderStream::with_capacity(reader, DOWNLOAD_CHUNK_SIZE) | ||
| .map(|result| result.map_err(|_| FsError::GeneralFailure)); | ||
| Ok(Box::pin(stream) as FsStream<Bytes>) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 检查新增文件系统 trait 方法是否已有对应测试。
rg -nP -C3 '\b(open_download_stream|copy_dir_shallow)\s*\(' src tests
rg -nP -C3 '(range|offset|overwrite|shallow copy|download stream)' tests src/webdavRepository: AsterCommunity/AsterDrive
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files mentioning new Forge filesystem trait methods:"
rg -n -C2 'open_download_stream|copy_dir_shallow|FsFileSystem|file_system' src/webdav tests \
--glob '*.rs' || true
echo
echo "Outline of src/webdav/fs/mod.rs around trait and implementations:"
ast-grep outline src/webdav/fs/mod.rs --view compact 2>/dev/null || true
sed -n '1,240p' src/webdav/fs/mod.rs
echo "----"
sed -n '620,720p' src/webdav/fs/mod.rsRepository: AsterCommunity/AsterDrive
Length of output: 12802
为 Forge 文件系统新增方法补齐测试。
当前只看到普通下载 / range 的现存测试覆盖，但 open_download_stream 和 copy_dir_shallow 的新契约缺少单元或集成测试：需要覆盖全量读取、范围读取、驱动读取失败，以及浅拷贝成功和覆盖失败的边界，否则外部 handler 容易回归。
🤖 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 `@src/webdav/fs/mod.rs` around lines 401 - 419, 为 open_download_stream 和
copy_dir_shallow 补充单元或集成测试：验证文件全量读取、范围读取及底层读取失败时的错误映射；同时验证 copy_dir_shallow
成功复制目录内容，以及目标已存在时覆盖失败。复用现有普通下载和 range 测试的测试工具与断言方式，确保覆盖这两个方法对外暴露的新契约。
Source: Coding guidelines
Comment on lines
+690
to
+692
| fn copy_dir_shallow<'a>(&'a self, from: &'a DavPath, to: &'a DavPath) -> FsFuture<'a, ()> { | ||
| Box::pin(async move { AsterDavFs::copy_dir_shallow(self, from, to).await }) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 定位浅复制调用链及覆盖删除、属性复制的边界。
ast-grep outline src/webdav/fs/mod.rs --items all --type function
rg -nP -C3 '\b(copy_dir_shallow|delete_existing_destination_for_overwrite|copy_visible_entity_properties)\s*\(' src testsRepository: AsterCommunity/AsterDrive
Length of output: 6319
把 WebDAV 浅复制收敛为原子用例。
copy_dir_shallow 当前先在 copy_dir_shallow 里删除目标，再调用 folder::create_in_scope_with_audit 复制属性并记录 FolderCopy 审计；任一步失败都会让旧目标已丢、新对象不完整。把这组写操作放在同一个 service/repository 边界并用 writer transaction 编排，销毁/落库/属性迁移全部回滚；审计按 outbox 或明确策略处理。
🤖 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 `@src/webdav/fs/mod.rs` around lines 690 - 692, 将 WebDAV 的 copy_dir_shallow
流程收敛到同一个 service/repository 写入边界，使用 writer transaction
统一编排目标删除、目录落库和属性迁移，确保任一步失败都回滚而不破坏旧目标。调整 AsterDavFs::copy_dir_shallow 及其调用的
folder::create_in_scope_with_audit，审计改为同一事务内的 outbox 或采用明确的失败处理策略。
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DavPath,DavFileSystem,DavLockSystem, and related traits fromsrc/webdav/dav.rs; replace all imports withaster_forge_webdav::davprops,protocol,responses,resources,locks,transfer,deltav) and delegate the full protocol exchange toaster_forge_webdav::handler::WebDavHandleraster_forge_webdavdependency toCargo.tomlis_system_namespace,is_dav_namespace, andis_protected_namespacehelpers inproperty.rswith imports fromaster_forge_webdavnormalize_path,path_ancestors,lock_paths_overlap,lock_path_is_under, andunlock_request_targets_lock_scopehelpers indb_lock_system.rswith imports fromaster_forge_webdav::lock_pathsparse_basic_authorization/ base64 decoding inauth.rswithaster_forge_webdav::auth::parse_basic_authorizationDavPropertyTargetandDavResourceKindfrom Forge inmetadata.rs, replacing the local(EntityType, i64)tupleDavNamePolicyforSystemFileBlockPolicyto satisfy the Forge handler interfaceAsterDavVersionProviderinversion_provider.rsas a persistence adapter implementingaster_forge_webdav::deltav::DavVersionProvideropen_download_streamandcopy_dir_shallowonAsterDavFsto satisfy the updatedDavFileSystemcontract from Forgetests/webdav/) to reference types fromaster_forge_webdavdirectlySummary by CodeRabbit
新功能
安全与稳定性