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?
|
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. |
|
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. |
- 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.
all right then, I have add DirectUploadMinPartSize=100MiB. it will now reject file larger than 5G*DirectUploadMaxParts. |
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 辅助内容。