feat(drivers): add Rakuten Drive support#2084
feat(drivers): add Rakuten Drive support#2084rufusu wants to merge 7 commits intoOpenListTeam:mainfrom
Conversation
| if srcRemote == "" { | ||
| srcRemote = d.toRemotePath(srcObj.GetPath(), srcObj.IsDir()) | ||
| } | ||
| prefix := path.Dir(srcRemote) |
There was a problem hiding this comment.
Maybe you can extract it as a function? It was used four times.
There was a problem hiding this comment.
Thanks for the suggestion. Will update this to reduce duplication as well.
| } | ||
| if strings.HasSuffix(itemPath, "/") { | ||
| isFolder = true | ||
| } |
There was a problem hiding this comment.
Would you mind moving this part into its own method? It would make the code easier to read and maintain.
There was a problem hiding this comment.
Thanks for the suggestion — I’ll update this to reduce duplication.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
refactor(rakuten_drive): extract repeated logic into helper methodsDescription / 描述
Motivation and Context / 背景
How Has This Been Tested? / 测试
Checklist / 检查清单
|
| } | ||
| if strings.HasSuffix(itemPath, "/") { | ||
| isFolder = true | ||
| } |
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| return time.Unix(exp, 0), nil | ||
| } | ||
|
|
||
| func (d *RakutenDrive) toRemotePath(p string, isDir bool) string { |
There was a problem hiding this comment.
Typically, we express conversions as "from A to B" (e.g., a2b), or retrievals as "get something" (e.g., getSomething). This method name fails to accurately convey its purpose.
toLocalPath has the same promble.
Suyunmeng
force-pushed
the
main
branch
4 times, most recently
from
a31fd53 to
7bea29c
Compare
…2RemotePath/remotePath2LocalPath Follow project naming conventions as per reviewer feedback
There was a problem hiding this comment.
Pull request overview
Adds a new Rakuten Drive storage driver to the OpenList drivers set, enabling basic file operations and multipart uploads against Rakuten Drive’s API, and registers it for runtime availability.
Changes:
- Introduces
drivers/rakuten_drivewith auth/token refresh, request helpers, path utilities, and API response types. - Implements core driver operations (list/link/mkdir/move/rename/copy/remove/upload/quota).
- Registers the new driver via
drivers/all.go.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| drivers/rakuten_drive/driver.go | Implements RakutenDrive driver operations, including S3 multipart upload and quota retrieval. |
| drivers/rakuten_drive/util.go | Adds token refresh/JWT exp parsing, request wrapper, and path/time normalization helpers. |
| drivers/rakuten_drive/types.go | Defines API response structs and a File object wrapper carrying extra metadata. |
| drivers/rakuten_drive/meta.go | Defines driver config/addition schema and registers the driver. |
| drivers/all.go | Imports the driver package to register it in the global driver set. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 4) check status | ||
| checkBody := base.Json{"key": initResp.UploadID} | ||
| for i := 0; i < 30; i++ { | ||
| var checkResp uploadCheckResp | ||
| _, err = d.newForestRequest(ctx, http.MethodPost, forestBase+"/v3/files/check", checkBody, &checkResp) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if strings.EqualFold(checkResp.State, "complete") { | ||
| break | ||
| } | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| case <-time.After(1 * time.Second): | ||
| } | ||
| } |
There was a problem hiding this comment.
In Put, the files/check polling loop breaks when state == complete, but if it never becomes complete within 30 tries the code still proceeds to complete/upload and returns success. Consider tracking whether completion was reached and returning a timeout/error (or handling explicit failure states) before continuing.
| if localDir != "" && !strings.HasSuffix(localDir, "/") && !strings.HasPrefix(localPath, "/") { | ||
| localPath = path.Join(localDir, name) | ||
| } |
There was a problem hiding this comment.
This localPath adjustment branch appears unreachable: remotePath2LocalPath always returns a string starting with "/" (either "/" or "/..."), so !strings.HasPrefix(localPath, "/") will never be true. Consider removing this conditional or reworking it if a different path format is expected.
| if localDir != "" && !strings.HasSuffix(localDir, "/") && !strings.HasPrefix(localPath, "/") { | |
| localPath = path.Join(localDir, name) | |
| } |
| if remoteDir == "/" { | ||
| remoteDir = "" | ||
| } |
There was a problem hiding this comment.
localPath2RemotePath(..., true) never returns "/" (it returns "" for root), so these remoteDir == "/" checks are dead code. Consider removing them to avoid confusion, or adjust localPath2RemotePath to return "/" for root consistently if that’s required by the API.
| if remoteDir == "/" { | |
| remoteDir = "" | |
| } |
| if remoteDir == "/" { | ||
| remoteDir = "" | ||
| } |
There was a problem hiding this comment.
Same as above: localPath2RemotePath(..., true) returns "" (not "/") for root, so this remoteDir == "/" branch is unreachable and can be removed (or make root handling consistent in one place).
| if remoteDir == "/" { | |
| remoteDir = "" | |
| } |
| if result != nil { | ||
| req.SetResult(result) | ||
| } | ||
| return req.Execute(method, url) |
There was a problem hiding this comment.
newForestRequest returns req.Execute results without checking HTTP status codes. Resty does not treat 4xx/5xx as an error, so callers like MakeDir/Move/Rename/Remove/Put can incorrectly report success on API failures. Consider explicitly checking resp.IsError() / resp.StatusCode() >= 400 and returning an error (ideally including status + response body) before returning the response.
| return req.Execute(method, url) | |
| resp, err := req.Execute(method, url) | |
| if err != nil { | |
| return resp, err | |
| } | |
| if resp != nil && resp.IsError() { | |
| return resp, fmt.Errorf("forest request %s %s failed with status %d: %s", method, url, resp.StatusCode(), strings.TrimSpace(resp.String())) | |
| } | |
| return resp, nil |
| if resp.Key != "" { | ||
| checkBody := base.Json{"key": resp.Key} | ||
| for i := 0; i < 30; i++ { | ||
| var checkResp uploadCheckResp | ||
| _, err = d.newForestRequest(ctx, http.MethodPost, forestBase+"/v3/files/check", checkBody, &checkResp) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if strings.EqualFold(checkResp.State, "complete") { | ||
| break | ||
| } | ||
| select { | ||
| case <-ctx.Done(): | ||
| return nil, ctx.Err() | ||
| case <-time.After(1 * time.Second): | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
In Copy, the async completion polling loop breaks after 30s but still returns a successful model.Object even if the operation never reaches state == complete. This can surface as phantom copies. After the loop, verify completion and return a timeout/error when the state is still not complete.
| if resp.Key == "" { | ||
| return nil | ||
| } | ||
| checkBody := base.Json{"key": resp.Key} | ||
| for i := 0; i < 30; i++ { | ||
| var checkResp uploadCheckResp | ||
| _, err = d.newForestRequest(ctx, http.MethodPost, forestBase+"/v3/files/check", checkBody, &checkResp) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if strings.EqualFold(checkResp.State, "complete") { | ||
| return nil | ||
| } | ||
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case <-time.After(1 * time.Second): | ||
| } | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Remove polls files/check up to 30 times but returns nil even if the operation never reaches state == complete (loop falls through). This can report a deletion as successful while it’s still pending/failed. Consider returning a timeout/error after the loop when completion wasn’t observed.
feat(rakuten_drive): add Rakuten Drive storage driver
Description / 描述
RakutenDrivedriver with list, link, mkdir, move, rename, copy, delete, upload (S3 multipart), and storage quota support.drivers/all.go.Motivation and Context / 背景
How Has This Been Tested? / 测试
Checklist / 检查清单
我已阅读 CONTRIBUTING 文档。
go fmtor prettier.我已使用
go fmt或 prettier 格式化提交的代码。我已为此 PR 添加了适当的标签（如无权限或需要的标签不存在，请在描述中说明，管理员将后续处理）。
我已在适当情况下使用"Request review"功能请求相关代码作者进行审查。
我已相应更新了相关仓库（若适用）。