feat(drivers/s3): support direct multipart upload - #2847
Conversation
|
The S3 multipart implementation itself appears to be driver-specific, so I do not think it needs to be generalized unless another driver later has the same requirement. However, I found three unrelated or cross-cutting changes that need clarification:
This PR adds min:"1" only for DirectUploadMaxParts, but it also changes the generic driver.Item API and the reflection logic used by every driver to expose both Min and Max. It is unclear whether these values are actually validated by the backend or are only frontend hints. Could this metadata change be submitted separately, together with its intended validation and frontend behavior, unless it is strictly required by the multipart implementation?
The PR now returns a 307 Temporary Redirect for eligible WebDAV uploads and disables multipart for that call by passing DirectUploadMaxParts=1 through context.Value. This appears to be a separate feature from frontend S3 multipart upload. It also creates an implicit contract between WebDAV and the S3 driver that is not represented by the direct-upload interface. Before including this change, WebDAV client compatibility, request-body redirect behavior, fallback behavior, and any effects from bypassing the normal upload path should be tested and documented. Otherwise, could the WebDAV change be removed from this PR and submitted separately?
|
jyxjjj
added
enhancement
needs-triage/needs-confirm
PR: HIGH-RISK
Module: Driver
WebDAV part will be removed from this pr. I agree with your idea. |
|
OpenList-Frontend#609 confirms that Min and Max are intended to be consumed by the generic driver configuration form. However, that does not change my original concern. The question is not whether the fields are used, but whether adding generic numeric range metadata across the backend and frontend belongs in an S3 multipart PR. This is a reusable configuration-form feature affecting all drivers and the public driver configuration schema, while the S3 multipart implementation currently uses only min:"1" for one setting. It could be reviewed and documented more clearly as a separate change, or at least explicitly identified as an additional public API/configuration feature in this PR. |
Change Num to 1631. I typed 1637 :( |
Min and Max is now removed. When DirectUploadMaxParts =1 or <0 will treated as Putobject (i.e. no multipart). 0 will be default. |
xrgzs
changed the title
|
I rechecked the current head after the previous scope cleanup. The WebDAV and generic Min/Max changes have been removed, but I still see several blockers before this can be merged.
The current calculation limits the number of parts, but it does not reject or adjust a configuration that produces parts larger than 5 GiB. For example, a 20 GiB file with Please either increase the effective part count or reject configurations that cannot satisfy both the configured limit and S3's 5 GiB maximum. Reference: https://docs.aws.amazon.com/AmazonS3/latest/userguide/qfacts.html
Reference: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html
The current request path writes the default value back into
It still says the WebDAV change is present, says the frontend PR has not been created, and says the documentation is unfinished. Please update the description and add or link the required documentation for the new setting and browser multipart requirements. I do not think the backend and frontend PRs should be merged until these points are addressed together. |
I am considering if the setting DirectUploadMaxParts is reasonable. Maybe we need to change to DirectUploadMinPartSize to avoid it. I am not sure what is the good idea and practice. So what is your idea? |
|
DirectUploadMinPartSize |
I notice a problem that #1631 will also cause this problem. If you PUT a file in 10 GiB, it will be rejected as well. |
|
Yes, he might just be laying the groundwork. |
all right then, I have add DirectUploadMinPartSize=100MiB. it will now reject file larger than 5G*DirectUploadMaxParts. |
PIKACHUIM
left a comment
PIKACHUIM
left a comment
There was a problem hiding this comment.
🙏 感谢贡献
感谢 @Arielfoever 提交此PR！我已完成代码评审，以下是评审结果。
📖 PR背景与需求
PR标题：feat(drivers/s3): support direct multipart upload
关联Issue：修复 #1631
需求说明：在 S3 驱动中支持前端直传的分片上传功能。目前 HTTP Direct 上传只支持单片上传，对于大文件效率较低。此 PR 实现了前端分片直传到 S3 的完整流程。
预期目标：
- 前端可以通过 Direct Upload API 获取多个分片的预签名 URL
- 前端直接上传各个分片到 S3（不经过 OpenList 服务器）
- 上传完成后通知 OpenList 完成分片合并
- 新增配置项
DirectUploadMaxParts（默认 10,000），当设为 1 时退化为单片上传（保持现有行为） - WebDAV 上传保持单片上传（不使用分片）
用户可感知的变化：无用户可见的行为变化，前端上传流程保持一致，但大文件上传效率提升。
📋 问题摘要
- ✅ 功能性：功能设计合理，解决了大文件直传效率问题
⚠️ 代码质量：有 2 处需要改进- 💡 改进建议：有 3 处可优化点
📂 逐文件分析
drivers/s3/driver.go
改动意图：在 S3 驱动中实现 PutAsTask 和 MultipartBackend 接口，支持分片上传。
代码修改逻辑：
- 实现了
PutAsTask接口（返回ErrNotSupport，因为 Direct Upload 不需要后台任务） - 实现了
MultipartBackend接口的四个方法：CreateMultipartUpload：创建分片上传，返回 uploadIdUploadPart：生成单个分片的预签名 URLCompleteMultipartUpload：完成分片合并AbortMultipartUpload：取消分片上传
UploadPart方法中根据DirectUploadMaxParts决定是否启用分片：DirectUploadMaxParts == 1：返回ErrNotSupport，退化为单片上传DirectUploadMaxParts > 1：生成预签名 URL
合理性评估：
-
✅ 优点：
- 实现了标准的
MultipartBackend接口，与 #2813 的后端分片上传解耦 - 通过
DirectUploadMaxParts配置项提供灵活控制 - 向后兼容：默认值 10,000 对现有用户无影响
- 实现了标准的
-
⚠️ 问题：- 缺少输入验证：
UploadPart方法没有验证partNumber的有效性（应在 1 到 10,000 之间） - 错误处理不完整：
CompleteMultipartUpload中如果 S3 返回错误，没有明确的错误信息传递给用户
- 缺少输入验证：
详细建议：
-
在
UploadPart中添加 partNumber 验证：func (d *S3) UploadPart(ctx context.Context, uploadId string, partNumber int, stream model.FileStreamer, options *model.DirectUploadPartOption) (*model.DirectUploadPartInfo, error) { if d.DirectUploadMaxParts == 1 { return nil, errs.ErrNotSupport } if partNumber < 1 || partNumber > 10000 { return nil, fmt.Errorf("partNumber must be between 1 and 10000, got %d", partNumber) } // ... existing code }
-
改进
CompleteMultipartUpload的错误处理：func (d *S3) CompleteMultipartUpload(ctx context.Context, uploadId string, dst string, parts []model.DirectUploadPartInfo) error { // ... existing code _, err := d.client.CompleteMultipartUpload(ctx, &s3.CompleteMultipartUploadInput{ Bucket: &d.Bucket, Key: &dst, UploadId: &uploadId, MultipartUpload: &types.CompletedMultipartUpload{ Parts: completedParts, }, }) if err != nil { return fmt.Errorf("failed to complete multipart upload for %s (uploadId=%s): %w", dst, uploadId, err) } return nil }
drivers/s3/meta.go
改动意图：新增 DirectUploadMaxParts 配置项。
代码修改逻辑：
- 新增字段
DirectUploadMaxParts int，默认值 10,000 - 添加 JSON 标签
direct_upload_max_parts
合理性评估：
- ✅ 优点：配置项命名清晰，默认值合理
⚠️ 问题：缺少help标签说明使用场景
详细建议：
补充 help 标签：
DirectUploadMaxParts int `json:"direct_upload_max_parts" type:"number" default:"10000" help:"Maximum number of parts for direct multipart upload. Set to 1 to disable multipart and use single-part upload. Valid range: 1-10000."`drivers/s3/util.go & drivers/s3/util_test.go
改动意图：新增 calculatePartSize 辅助函数，根据文件大小和最大分片数计算每个分片的大小。
代码修改逻辑：
calculatePartSize(fileSize, maxParts, minPartSize, maxPartSize)函数逻辑：- 如果
fileSize / maxParts < minPartSize，返回minPartSize - 如果
fileSize / maxParts > maxPartSize，返回错误 - 否则返回
fileSize / maxParts向上取整
- 如果
合理性评估：
-
✅ 优点：
- 测试覆盖充分（254 行测试，包含边界情况）
- 处理了极小文件、超大文件、边界情况
-
💡 改进建议：
- 当前实现中，如果文件大小超过
maxParts * maxPartSize（即 10,000 * 5GB = 50TB），会返回错误。虽然这符合 S3 限制，但建议在错误信息中说明原因。
- 当前实现中，如果文件大小超过
详细建议：
改进错误信息：
func calculatePartSize(fileSize, maxParts, minPartSize, maxPartSize int64) (int64, error) {
if fileSize <= 0 {
return 0, errors.New("file size must be positive")
}
partSize := (fileSize + maxParts - 1) / maxParts
if partSize < minPartSize {
return minPartSize, nil
}
if partSize > maxPartSize {
return 0, fmt.Errorf("file size %d bytes exceeds S3 multipart upload limit (max %d parts × %d bytes/part = %d bytes total)",
fileSize, maxParts, maxPartSize, maxParts*maxPartSize)
}
return partSize, nil
}internal/model/direct_upload.go
改动意图：在 Direct Upload 模型中新增分片上传相关字段。
代码修改逻辑：
- 新增
DirectUploadPartOption结构体（包含uploadId和partNumber） - 新增
DirectUploadPartInfo结构体（包含partNumber和etag）
合理性评估：
- ✅ 优点：模型设计清晰，字段命名规范
🎯 总体评价
功能性：⭐⭐⭐⭐ - 功能设计合理，解决了实际需求
安全性：⭐⭐⭐⭐ - 使用预签名 URL，安全性良好
代码质量：⭐⭐⭐ - 测试覆盖充分，但缺少输入验证和错误信息优化
实现方案：⭐⭐⭐⭐ - 实现简洁，向后兼容
建议操作：
- ✅ Approve（建议合并）
- 🔄 Request Changes（需要修改）
- ❌ Close（建议关闭）
理由：此 PR 功能完整、测试充分、实现合理，但需要补充输入验证和错误信息优化。建议修复以下问题后合并：
- P1（必须修复）：在
UploadPart中添加partNumber验证（1-10000） - P2（建议修复）：改进
CompleteMultipartUpload的错误信息 - P2（建议修复）：补充
DirectUploadMaxParts的help标签
💡 后续建议
- 前端适配：需要在 OpenList-Frontend 中实现前端分片上传逻辑（如 PR 描述中提到的"Will PR soon"）
- 文档补充：在 OpenList-Docs 中补充
DirectUploadMaxParts配置项的说明和使用场景 - 性能测试：建议在生产环境部署前测试大文件（如 10GB+）的上传性能和稳定性
- 监控指标：建议添加分片上传的监控指标（成功率、失败率、平均分片数等）
再次感谢你的贡献！修复上述问题后，这将是一个高质量的功能增强。👏
- Add S3 multipart direct upload info with presigned part, complete, and abort URLs. - Add configurable direct upload max parts and part-size calculation. - Cover multipart direct upload URL generation and completion flow with tests.
- add MultipartBackend interface with CreateMultipartUpload, UploadPart, CompleteMultipartUpload, and AbortMultipartUpload methods - add PutAsTask interface, S3 returns NotImplement - implement MultipartBackend methods on S3 driver with input validation and error wrapping - add calculatePartSize helper with descriptive limit error - add DirectUploadPartOption and DirectUploadPartInfo model types - update DirectUploadMaxParts help tag with valid range description
3 participants
Summary / 摘要
Support direct multipart upload on web frontend.
There is no user-visible behavior changes for uploading.
HTTP Direct for S3 is now upload by multipart unless DirectUploadMaxParts=1 while the default value is 10000. If S3 is standard, there will be nothing change.
Now webdav on S3 will try to upload file with direct upload without multipart.
/ 此 PR 包含破坏性变更。
The PR did not undergo regression testing to verify whether a destructive transformation occurred. However, at least the upgrade did not reveal any issues.
/ 此 PR 修改了公开 API、配置、存储格式或迁移行为。
This PR add a new setting.
/ 此 PR 需要关联仓库同步修改。
Related repository PRs / 关联仓库 PR:
Related Issues / 关联 Issue
Fix #1631
Testing / 测试
go test ./...go test ./drivers/s3 returns no errors.
test on Aliyun OSS and it uploads in 50 parts as planned. No larger file tested.
Checklist / 检查清单
/ 我已阅读 CONTRIBUTING。
/ 我确认此贡献符合仓库许可证、贡献规范和行为准则。
gofmt,go fmt, orprettierwhere applicable./ 我已按适用情况使用
gofmt、go fmt或prettier格式化变更代码。/ 我已在适用情况下请求相关维护者或代码所有者审查。
No right to request review.
AI Disclosure / AI 使用声明
/ 此 PR 包含 AI 辅助内容。
Tools used / 使用工具:
Usage scope / 使用范围:
Code generation / 代码生成
All code are reviewed by AI. It found some bugs and fixed.
Refactoring / 重构
Documentation / 文档
Tests / 测试
ALL tests file are gen by AI.
Translation / 翻译
Review assistance / 审查辅助
All format work are done by AI. I hope it looks in a good shape.
AI gen this description
I have reviewed and validated all AI-assisted content included in this PR.
/ 我已审核并验证此 PR 中的所有 AI 辅助内容。
I have ensured that all AI-assisted commits include
Co-Authored-Byattribution./ 我已确保所有 AI 辅助提交都包含
Co-Authored-By归属信息。I can reproduce all AI-assisted content included in this PR without any AI tools.
/ 我可以在没有任何 AI 工具的情况下重现此 PR 中包含的所有 AI 辅助内容。