feat(im): use Content-Disposition filename when downloading message r…#525
Closed
chenxingtong-bytedance wants to merge 1 commit intolarksuite:mainfrom
Closed
Conversation
github-actions
Bot
added
domain/im
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds Content-Disposition filename parsing and a userSpecifiedOutput flag to IM resource downloads; download path resolution now prefers server-provided filenames/extensions (RFC 5987-aware), returns a clear error on nil responses, and updates tests to pass the new flag and cover filename parsing and path-resolution cases. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Download Handler
participant HTTP as HTTP Client
participant Parser as Filename Resolver
participant FS as File System
User->>CLI: Execute (resource URL, --output?)
CLI->>CLI: set userSpecifiedOutput flag
CLI->>HTTP: doIMResourceDownloadRequest()
HTTP-->>CLI: Response (headers: Content-Disposition, Content-Type, body)
CLI->>CLI: if response == nil -> return error
CLI->>Parser: parseContentDispositionFilename(Content-Disposition)
Parser-->>CLI: sanitizedFilename
CLI->>CLI: resolveIMResourceDownloadPath(safePath, sanitizedFilename, contentType, userSpecifiedOutput)
CLI-->>FS: write file to resolved path
FS-->>CLI: success / error
CLI-->>User: complete / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/im/helpers_test.go`:
- Around line 668-700: Add an end-to-end test that covers the default-output
branch by invoking downloadIMResourceToPath with userSpecifiedOutput=false and a
mocked HTTP response that includes a Content-Disposition header with a filename
(e.g., `attachment; filename="server.pdf"`); ensure the test uses the same
helper setup as other downloads, verifies the file is actually written to disk
under the server-provided name (not the client safePath), and cleans up the file
afterwards. Reference downloadIMResourceToPath and resolveIMResourceDownloadPath
when locating where to wire the test; assert the saved path equals the expected
filename from the Content-Disposition and fail the test on mismatch. Ensure the
test mirrors existing download test structure (setup/mocks/teardown) so it
exercises the real save flow rather than only calling
resolveIMResourceDownloadPath.
In `@skills/lark-im/references/lark-im-messages-resources-download.md`:
- Line 37: Update the documentation for the `--output` option to reflect the
actual fallback chain used by resolveIMResourceDownloadPath: first use the
server filename from Content-Disposition if present; otherwise use the
resource's file_key and, when available, append an extension inferred from
Content-Type (e.g., saving as file_key.pdf), rather than saying it defaults to a
bare file_key. Mention Content-Disposition and Content-Type explicitly and
reference resolveIMResourceDownloadPath and file_key so readers know where the
behavior originates.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd39fc6a-114b-430c-8cb6-58820d9f902e
📒 Files selected for processing (4)
shortcuts/im/helpers_network_test.goshortcuts/im/helpers_test.goshortcuts/im/im_messages_resources_download.goskills/lark-im/references/lark-im-messages-resources-download.md
Comment on lines
+668
to
+700
| func TestResolveIMResourceDownloadPath(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| safePath string | ||
| contentType string | ||
| contentDisposition string | ||
| userSpecifiedOutput bool | ||
| want string | ||
| }{ | ||
| // safePath already has extension: always return as-is | ||
| {name: "user path with ext, no CD", safePath: "out.xlsx", contentType: "application/pdf", userSpecifiedOutput: true, want: "out.xlsx"}, | ||
| {name: "user path with ext, CD present", safePath: "out.xlsx", contentDisposition: `attachment; filename="server.pdf"`, userSpecifiedOutput: true, want: "out.xlsx"}, | ||
| // No --output: use CD filename when present | ||
| {name: "default path, CD filename", safePath: "file_xxx", contentDisposition: `attachment; filename="季度报告.xlsx"`, want: "季度报告.xlsx"}, | ||
| {name: "default path, CD RFC5987", safePath: "file_xxx", contentDisposition: `attachment; filename*=UTF-8''%E5%AD%A3%E5%BA%A6%E6%8A%A5%E5%91%8A.xlsx`, want: "季度报告.xlsx"}, | ||
| {name: "default path, no CD, MIME ext", safePath: "file_xxx", contentType: "application/pdf", want: "file_xxx.pdf"}, | ||
| {name: "default path, no CD, unknown MIME", safePath: "file_xxx", contentType: "application/x-unknown", want: "file_xxx"}, | ||
| {name: "default path, CD with dir component", safePath: "downloads/file_xxx", contentDisposition: `attachment; filename="report.xlsx"`, want: "downloads/report.xlsx"}, | ||
| // User --output without extension: use CD filename's extension | ||
| {name: "user path no ext, CD with ext", safePath: "myfile", contentDisposition: `attachment; filename="server.pdf"`, userSpecifiedOutput: true, want: "myfile.pdf"}, | ||
| {name: "user path no ext, CD no ext, MIME ext", safePath: "myfile", contentDisposition: `attachment; filename="noext"`, contentType: "image/png", userSpecifiedOutput: true, want: "myfile.png"}, | ||
| {name: "user path no ext, no CD, MIME ext", safePath: "myfile", contentType: "image/jpeg", userSpecifiedOutput: true, want: "myfile.jpg"}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| got := resolveIMResourceDownloadPath(tt.safePath, tt.contentType, tt.contentDisposition, tt.userSpecifiedOutput) | ||
| if got != tt.want { | ||
| t.Fatalf("resolveIMResourceDownloadPath() = %q, want %q", got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Please cover the default-output branch end-to-end as well.
These helper tests are useful, but they still don’t exercise the new downloadIMResourceToPath(..., userSpecifiedOutput=false) path. Right now every download test in the provided suite passes true, so a regression in wiring Content-Disposition through the actual save flow would still pass. Please add one download test that returns a Content-Disposition filename, passes false, and asserts the file is saved under that server name.
As per coding guidelines, “Every behavior change must have an accompanying test”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@shortcuts/im/helpers_test.go` around lines 668 - 700, Add an end-to-end test
that covers the default-output branch by invoking downloadIMResourceToPath with
userSpecifiedOutput=false and a mocked HTTP response that includes a
Content-Disposition header with a filename (e.g., `attachment;
filename="server.pdf"`); ensure the test uses the same helper setup as other
downloads, verifies the file is actually written to disk under the
server-provided name (not the client safePath), and cleans up the file
afterwards. Reference downloadIMResourceToPath and resolveIMResourceDownloadPath
when locating where to wire the test; assert the saved path equals the expected
filename from the Content-Disposition and fail the test on mismatch. Ensure the
test mirrors existing download test structure (setup/mocks/teardown) so it
exercises the real save flow rather than only calling
resolveIMResourceDownloadPath.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@shortcuts/im/im_messages_resources_download.go`:
- Around line 389-390: Add a unit test that calls doIMResourceDownloadRequest
and asserts that every outgoing request includes the PPE headers "x-tt-env:
ppe_resource_download_file" and "x-use-ppe: 1"; implement the test using an
httptest.Server to capture requests, invoke doIMResourceDownloadRequest (from
shortcuts/im/im_messages_resources_download.go) for at least two different
fileType values and any other varying parameters, and fail if the captured
request headers do not contain the exact header names and values; name the test
clearly (e.g., TestDoIMResourceDownloadRequest_IncludesPPEHeaders) and ensure it
runs as part of the package tests.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a7b2903a-04f8-4afe-82f6-8ec05ebcf07a
📒 Files selected for processing (1)
shortcuts/im/im_messages_resources_download.go
…esources Change-Id: I68b48cf428aa8aded4ad9d55fa042f9d68263c3a
chenxingtong-bytedance
force-pushed
the
fix/im-content-disposition-filename
branch
from
07ee8ca to
7328283
Compare
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.
Summary
When downloading message resources, the saved filename was always derived from
file_key(e.g.file_v2_abc123.xlsx), ignoring the original filename thesender uploaded. This PR resolves filenames from the
Content-Dispositionresponse header first, falling back to
Content-Type-based extension inferenceonly when the header is absent.
Changes
parseContentDispositionFilenameto extract and sanitize filenames fromContent-Disposition, with RFC 5987 (filename*=UTF-8''...) support via thestandard
mimepackageresolveIMResourceDownloadPathto prioritiseContent-Dispositionfilename; when
--outputis not specified, the full server filename is used;when
--outputis specified without an extension, only the extension is takenfrom the
Content-DispositionfilenameuserSpecifiedOutput boolparameter todownloadIMResourceToPathandresolveIMResourceDownloadPathto distinguish default vs. explicit output pathsskills/lark-im/references/lark-im-messages-resources-download.mdtoreflect the new filename resolution priority
Test Plan
go test ./shortcuts/im/...passedTestParseContentDispositionFilename(11 cases): plainfilename,Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation