feat(upload): require explicit session_kind and remove legacy chunk a…#430
Merged
Merged
Conversation
…ssembly **Breaking Changes:** - `upload_sessions.session_kind` is now `NOT NULL` - Removed 0.4.x payload-per-chunk `chunk_N`, `assembled` assembly/relay, kind inference, and assembly limiter - Migration will fail if null or invalid `session_kind` values exist; deployments must clean up expired sessions before upgrading to 0.5.0 **Changes:** - Chunk PUT, Progress, Complete, Cancel/Cleanup now only accept persisted explicit `UploadSessionKind` - Still validate multipart, temp key, and provider session metadata invariants - OffsetStaging, StreamStaging, provider/remote relay multipart, presigned, and provider resumable paths remain unchanged - Upload service continues generating Init plan from connector-owned transport without introducing `DriverType` routing matrix - Removed `LegacyChunkFiles` kind, `assemble_legacy_local_chunks_to_temp_file`, `stream_legacy_local_chunks_into_writer`, compatibility classifier for null rows, and chunk assembly limiter - Removed `UploadRuntime` from `PrimaryAppState` (no longer needed without assembly serialization) - Simplified `resolve_upload_session_kind` to direct field read without fallback inference - Added migration `m20260723_000001_require_upload_session_kind` with pre-flight validation that rejects upgrades containing legacy rows - Updated tests to create sessions with explicit kinds and verify migration rollback/reapply behavior
AptS-1547
added
Enhancement
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthrough上传会话改为持久化合法且非空的 Changes上传会话契约与迁移
上传路径与运行时收窄
上传测试与契约同步
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: 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 |
![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 (2)
src/services/files/upload/kind.rs (1)
8-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
validate_persisted_kind的kind参数是多余的，留着迟早坑人。
resolve_upload_session_kind永远传session.session_kind，可validate_persisted_kind又单独收一个kind去跟 session 的字段比对。现在没事，但哪天有人手滑传进来一个跟持久化字段不一致的kind，它会安安静静按错的类型放行——而这个 kind 后面是要决定上传路由的。既然只有这么一个真实用法，就别留这个洞。♻️ 直接读持久化字段
pub(crate) fn resolve_upload_session_kind( session: &upload_session::Model, ) -> Result<UploadSessionKind> { - validate_persisted_kind(session, session.session_kind) + validate_persisted_kind(session) } -pub(crate) fn validate_persisted_kind( - session: &upload_session::Model, - kind: UploadSessionKind, -) -> Result<UploadSessionKind> { +pub(crate) fn validate_persisted_kind( + session: &upload_session::Model, +) -> Result<UploadSessionKind> { + let kind = session.session_kind;🤖 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/services/files/upload/kind.rs` around lines 8 - 17, 移除 validate_persisted_kind 的 kind 参数，改为在函数内部直接读取并校验 session.session_kind；同时更新 resolve_upload_session_kind 仅传入 session，确保上传类型始终来源于持久化字段。migration/src/m20260723_000001_require_upload_session_kind.rs (1)
164-260: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value泛型参数名不副实：外键始终硬编码
SQLITE_REBUILT_TABLE。
upload_sessions_table<T: IntoIden>接受任意table，但foreign_key().from()里写死了Alias::new(SQLITE_REBUILT_TABLE)而不是复用传入的table。目前唯一调用点恰好两者一致，凑巧没炸，但函数签名承诺的通用性和实现是脱节的，之后谁复用这个函数换个表名就会悄悄拿到错的外键定义。既然只有一处调用，不如把泛型去掉，直接接受具体表名或者复用table变量。♻️ 建议修复
-fn upload_sessions_table<T>(table: T, nullable: bool) -> TableCreateStatement -where - T: IntoIden, -{ +fn upload_sessions_table(table: impl IntoIden + Clone, nullable: bool) -> TableCreateStatement { let mut session_kind = ColumnDef::new(UploadSessions::SessionKind); session_kind.string_len(32); ... .foreign_key( ForeignKey::create() - .from(Alias::new(SQLITE_REBUILT_TABLE), UploadSessions::UserId) + .from(table.clone(), UploadSessions::UserId) .to(Users::Table, Users::Id) .on_delete(ForeignKeyAction::Cascade), ) .to_owned() }🤖 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 `@migration/src/m20260723_000001_require_upload_session_kind.rs` around lines 164 - 260, Update upload_sessions_table so the foreign key’s from-table uses the function’s table parameter instead of the hardcoded SQLITE_REBUILT_TABLE alias; preserve the existing table creation behavior and generic table support.
🤖 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 `@tests/test_upload.rs`:
- Around line 3326-3331: 增强该并发测试：不要仅依赖 tokio::join!，应在 part claim 或 staging
写入临界区注入可控的 barrier/failpoint，确保两个请求都进入关键区后再继续；随后断言并发进入确实发生，并验证最终 staging/part
仅登记一次。
- Line 177: 将 UploadSessionSpec::new 修改为强制接收 UploadSessionKind 参数，移除其中自动填充
OffsetStaging 的隐式默认值；同步更新所有调用方（包括仅调用 object_upload 的 fixture），显式传入正确的
session_kind，避免遗漏配置时生成错误组合。
---
Nitpick comments:
In `@migration/src/m20260723_000001_require_upload_session_kind.rs`:
- Around line 164-260: Update upload_sessions_table so the foreign key’s
from-table uses the function’s table parameter instead of the hardcoded
SQLITE_REBUILT_TABLE alias; preserve the existing table creation behavior and
generic table support.
In `@src/services/files/upload/kind.rs`:
- Around line 8-17: 移除 validate_persisted_kind 的 kind 参数，改为在函数内部直接读取并校验
session.session_kind；同时更新 resolve_upload_session_kind 仅传入
session，确保上传类型始终来源于持久化字段。
🪄 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: 24a48998-8ef1-4967-a580-36411dcaa503
⛔ Files ignored due to path filters (1)
frontend-panel/src/services/api.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (56)
CHANGELOG.mddeveloper-docs/zh-CN/api/files.mddeveloper-docs/zh-CN/design/upload-finalization-contracts.mdmigration/src/lib.rsmigration/src/m20260723_000001_require_upload_session_kind.rssrc/api/routes/files/access.rssrc/api/routes/frontend.rssrc/api/routes/health.rssrc/api/routes/share_public.rssrc/entities/upload_session.rssrc/runtime/mod.rssrc/runtime/startup/primary.rssrc/runtime/tasks.rssrc/services/files/file/deletion/tests.rssrc/services/files/file/download/tests.rssrc/services/files/file/resource_handle.rssrc/services/files/lock/tests.rssrc/services/files/upload/chunk.rssrc/services/files/upload/complete/chunked.rssrc/services/files/upload/complete/mod.rssrc/services/files/upload/complete/object_multipart.rssrc/services/files/upload/complete/plan.rssrc/services/files/upload/complete/tests.rssrc/services/files/upload/init/context.rssrc/services/files/upload/init/mod.rssrc/services/files/upload/kind.rssrc/services/files/upload/lifecycle.rssrc/services/files/upload/mod.rssrc/services/files/upload/progress.rssrc/services/files/upload/runtime.rssrc/services/files/upload/shared.rssrc/services/files/upload/staging.rssrc/services/media/metadata/tests.rssrc/services/ops/audit/tests.rssrc/services/ops/config/system.rssrc/services/storage_policy/credential/oauth/tests.rssrc/services/storage_policy/policy/policies.rssrc/services/task/dispatch/tests.rssrc/services/workspace/scope/mod.rssrc/services/workspace/storage/tests.rssrc/storage/connectors/mod.rssrc/storage/connectors/tests.rssrc/storage/connectors/upload.rssrc/types/upload_session.rssrc/webdav/auth.rssrc/webdav/file/tests.rssrc/webdav/handler_tests/mod.rstests/common/mod.rstests/test_auth.rstests/test_database_backends.rstests/test_maintenance.rstests/test_migrations.rstests/test_policies.rstests/test_remote_storage.rstests/test_services.rstests/test_upload.rs
💤 Files with no reviewable changes (28)
- src/services/storage_policy/credential/oauth/tests.rs
- src/services/task/dispatch/tests.rs
- src/webdav/handler_tests/mod.rs
- src/services/ops/audit/tests.rs
- src/services/files/upload/runtime.rs
- src/api/routes/health.rs
- src/services/files/upload/mod.rs
- src/services/files/file/download/tests.rs
- src/runtime/tasks.rs
- src/services/storage_policy/policy/policies.rs
- src/api/routes/frontend.rs
- src/api/routes/files/access.rs
- src/services/workspace/storage/tests.rs
- src/api/routes/share_public.rs
- src/services/files/lock/tests.rs
- src/types/upload_session.rs
- src/services/ops/config/system.rs
- src/webdav/auth.rs
- src/services/workspace/scope/mod.rs
- src/services/files/file/deletion/tests.rs
- src/runtime/startup/primary.rs
- tests/test_auth.rs
- src/storage/connectors/upload.rs
- src/services/media/metadata/tests.rs
- src/services/files/file/resource_handle.rs
- tests/common/mod.rs
- src/storage/connectors/tests.rs
- src/webdav/file/tests.rs
…arameters - Add rendezvous barrier before staging write lock acquisition for more reliable concurrency testing - Replace timeout-based overlap detection with entry barrier and counter - Remove redundant `kind` parameter from `validate_persisted_kind` function - Fix foreign key reference in migration to use correct table identifier - Update test helper to require explicit `session_kind` in constructor - Improve concurrent chunk upload test with proper synchronization verification - Add entry count assertion to duplicate chunk test - Update remote storage test assertion to check for transient error kind
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.
…ssembly
Breaking Changes:
upload_sessions.session_kindis nowNOT NULLchunk_N,assembledassembly/relay, kind inference, and assembly limitersession_kindvalues exist; deployments must clean up expired sessions before upgrading to 0.5.0Changes:
UploadSessionKindDriverTyperouting matrixLegacyChunkFileskind,assemble_legacy_local_chunks_to_temp_file,stream_legacy_local_chunks_into_writer, compatibility classifier for null rows, and chunk assembly limiterUploadRuntimefromPrimaryAppState(no longer needed without assembly serialization)resolve_upload_session_kindto direct field read without fallback inferencem20260723_000001_require_upload_session_kindwith pre-flight validation that rejects upgrades containing legacy rowshanges:
Summary by CodeRabbit
session_kind）现为必填且数据库侧不再允许为NULL；升级时若发现空值或非法类型，将中止升级并保留原数据。