Skip to content

feat(drivers/s3): support direct multipart upload#2847

Open
Arielfoever wants to merge 1 commit into
OpenListTeam:mainfrom
Arielfoever:feat/s3
Open

feat(drivers/s3): support direct multipart upload#2847
Arielfoever wants to merge 1 commit into
OpenListTeam:mainfrom
Arielfoever:feat/s3

Conversation

@Arielfoever

@Arielfoever Arielfoever commented Jul 26, 2026

Copy link
Copy Markdown

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.

  • This PR has breaking changes.
    / 此 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.
  • This PR changes public API, config, storage format, or migration behavior.
    / 此 PR 修改了公开 API、配置、存储格式或迁移行为。
    This PR add a new setting.
  • This PR requires corresponding changes in related repositories.
    / 此 PR 需要关联仓库同步修改。

Related repository PRs / 关联仓库 PR:

  • OpenList-Frontend: Will PR soon
  • OpenList-Docs: it should have a commit for setting change but not finished yet.

Related Issues / 关联 Issue

Fix #1631

Testing / 测试

  • go test ./...
    go test ./drivers/s3 returns no errors.
  • Manual test / 手动测试:
    test on Aliyun OSS and it uploads in 50 parts as planned. No larger file tested.

Checklist / 检查清单

  • I have read CONTRIBUTING.
    / 我已阅读 CONTRIBUTING
  • I confirm this contribution follows the repository license, contribution policy, and code of conduct.
    / 我确认此贡献符合仓库许可证、贡献规范和行为准则。
  • I have formatted the changed code with gofmt, go fmt, or prettier where applicable.
    / 我已按适用情况使用 gofmtgo fmtprettier 格式化变更代码。
  • I have requested review from relevant maintainers or code owners where applicable.
    / 我已在适用情况下请求相关维护者或代码所有者审查。
    No right to request review.

AI Disclosure / AI 使用声明

  • This PR includes AI-assisted content.
    / 此 PR 包含 AI 辅助内容。

Tools used / 使用工具:

  • ChatGPT
  • Codex
  • [] GitHub Copilot
  • Claude
  • Gemini
  • Other (please specify) / 其他(请注明):

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-By attribution.
    / 我已确保所有 AI 辅助提交都包含 Co-Authored-By 归属信息。

  • I can reproduce all AI-assisted content included in this PR without any AI tools.
    / 我可以在没有任何 AI 工具的情况下重现此 PR 中包含的所有 AI 辅助内容。

@jyxjjj

jyxjjj commented Jul 26, 2026

Copy link
Copy Markdown
Member

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:

  1. Why does this PR extend the generic driver configuration metadata with Min and Max?

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?

  1. Why does this PR change WebDAV PUT behavior?

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?

  1. The PR description says Fix feat(onedrive): support create share link for download #1637, but feat(onedrive): support create share link for download #1637 is an unrelated OneDrive share-link PR. Please remove or correct this reference.

@Arielfoever

Copy link
Copy Markdown
Author
2. Why does this PR change WebDAV PUT behavior?

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.

@Arielfoever
Arielfoever marked this pull request as draft July 26, 2026 15:10
@jyxjjj

jyxjjj commented Jul 26, 2026

Copy link
Copy Markdown
Member

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.

@Arielfoever

Copy link
Copy Markdown
Author
  1. The PR description says Fix feat(onedrive): support create share link for download #1637, but feat(onedrive): support create share link for download #1637 is an unrelated OneDrive share-link PR. Please remove or correct this reference.

Change Num to 1631. I typed 1637 :(

@Arielfoever

Copy link
Copy Markdown
Author

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.

Min and Max is now removed. When DirectUploadMaxParts =1 or <0 will treated as Putobject (i.e. no multipart). 0 will be default.

@Arielfoever
Arielfoever marked this pull request as ready for review July 26, 2026 15:30
@xrgzs xrgzs changed the title feat(s3): support direct multipart upload feat(drivers/s3): support direct multipart upload Jul 26, 2026
@jyxjjj

jyxjjj commented Jul 27, 2026

Copy link
Copy Markdown
Member

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.

  1. Multipart part size is not bounded by S3's 5 GiB maximum.

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 DirectUploadMaxParts=2 produces approximately 10 GiB parts, which S3 rejects.

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

  1. The coordinated frontend PR treats every HTTP 2xx completion response as success.

CompleteMultipartUpload can return HTTP 200 with an <Error> response body. The frontend must parse the completion response and only report success when it contains a valid completion result; embedded errors should be surfaced and handled as failures.

Reference: https://docs.aws.amazon.com/AmazonS3/latest/API/API_CompleteMultipartUpload.html

  1. Please avoid mutating shared driver configuration from an upload request.

The current request path writes the default value back into DirectUploadMaxParts. Concurrent uploads can therefore access the same field unsafely. Please compute an effective value in a local variable instead.

  1. The PR description is stale.

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.

@Arielfoever

Arielfoever commented Jul 27, 2026

Copy link
Copy Markdown
Author
1. Multipart part size is not bounded by S3's 5 GiB maximum.

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 DirectUploadMaxParts=2 produces approximately 10 GiB parts, which S3 rejects.

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

3. Please avoid mutating shared driver configuration from an upload request.

The current request path writes the default value back into DirectUploadMaxParts. Concurrent uploads can therefore access the same field unsafely. Please compute an effective value in a local variable instead.

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?

@jyxjjj

jyxjjj commented Jul 27, 2026

Copy link
Copy Markdown
Member

DirectUploadMinPartSize

@Arielfoever

Arielfoever commented Jul 27, 2026

Copy link
Copy Markdown
Author
  1. Multipart part size is not bounded by S3's 5 GiB maximum.

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 DirectUploadMaxParts=2 produces approximately 10 GiB parts, which S3 rejects.

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

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.

@jyxjjj

jyxjjj commented Jul 27, 2026

Copy link
Copy Markdown
Member

Yes, he might just be laying the groundwork.
Files of such massive size don't typically appear in his workflow.
As this is a new feature submitted directly by members, we might not be quite as strict.

- 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.
@Arielfoever

Copy link
Copy Markdown
Author

Yes, he might just be laying the groundwork. Files of such massive size don't typically appear in his workflow. As this is a new feature submitted directly by members, we might not be quite as strict.

all right then, I have add DirectUploadMinPartSize=100MiB. it will now reject file larger than 5G*DirectUploadMaxParts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants