fix: modify resource download #267
Conversation
WalkthroughThe /resource/download/{name} endpoint now validates the name, fetches the Resource by name (fails if missing), selects base64Data (resourceData or thumbnailData), ensures non-empty data, decodes it to bytes, infers content type, sets unified headers (including Content-Disposition), and streams bytes with try-with-resources and an explicit flush. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant RC as ResourceController
participant RS as ResourceStore
C->>RC: GET /resource/download/{name}
RC->>RC: Validate name (non-null, non-blank)
RC->>RS: findByName(name)
RS-->>RC: Resource or null
alt resource missing
RC-->>C: 404 / throw exception
else resource found
alt isResource(name)
RC->>RC: base64Data = resourceData
else
RC->>RC: base64Data = thumbnailData
end
RC->>RC: Ensure base64Data non-empty, clean and decode -> imageBytes
RC->>RC: Infer contentType, encode filename, decide disposition (inline/attachment)
RC-->>C: 200 OK with headers (Content-Type, Content-Length, Cache-Control, Accept-Ranges, Content-Disposition)
RC->>RC: Stream imageBytes (try-with-resources) and flush
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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
🧹 Nitpick comments (5)
base/src/main/java/com/tinyengine/it/controller/ResourceController.java (5)
299-306: OpenAPI doc drift: remove unusedisResourceparameter or document the new rule.The endpoint no longer accepts
isResource, but it’s still listed in the OpenAPI annotations. This confuses clients.- @Parameter(name = "isResource", description = "isResource"),
316-318: Differentiate not-found vs bad-request.Returning the same CM009 for both invalid input and missing resource blurs client semantics; prefer a distinct code (or 404 mapping) for not-found.
338-342: Addfilenamefallback and sanitize for broader browser support.Some clients ignore
filename*. Provide bothfilenameandfilename*, and sanitize to prevent header injection.- String contentDisposition = Utils.isDownload(name) - ? "attachment; filename*=UTF-8''" + encodedFileName - : "inline; filename*=UTF-8''" + encodedFileName; + String type = Utils.isDownload(name) ? "attachment" : "inline"; + String safeBasicFileName = name + .replaceAll("[^\\x20-\\x7E]", "_") + .replaceAll("[\\\\/:*?\"<>|\\r\\n]", "_"); + String contentDisposition = type + + "; filename=\"" + safeBasicFileName + "\"" + + "; filename*=UTF-8''" + encodedFileName; response.setHeader("Content-Disposition", contentDisposition);
344-346: Redundant flush inside try-with-resources.
close()will flush; you can dropout.flush()to reduce noise.- out.flush();
334-334: Optional: add ETag/Last-Modified for better caching.With
Cache-Control: public, max-age=3600, consider strong ETag (e.g., SHA-256 of bytes) and/or Last-Modified to enable conditional requests.I can provide a small helper to compute an ETag and return 304 on
If-None-Match.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base/src/main/java/com/tinyengine/it/controller/ResourceController.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
base/src/main/java/com/tinyengine/it/controller/ResourceController.java (1)
base/src/main/java/com/tinyengine/it/common/utils/ImageThumbnailGenerator.java (1)
ImageThumbnailGenerator(42-515)
🔇 Additional comments (2)
base/src/main/java/com/tinyengine/it/controller/ResourceController.java (2)
311-313: Good null/blank guard onname.
329-329: Filename encoding LGTM.Using
URLEncoderwith space normalization is correct forfilename*.
base/src/main/java/com/tinyengine/it/controller/ResourceController.java
Outdated
Show resolved
Hide resolved
base/src/main/java/com/tinyengine/it/controller/ResourceController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
base/src/main/java/com/tinyengine/it/controller/ResourceController.java (1)
325-333: Harden Base64 decode and MIME detection; usesetContentLengthLong.Uncaught
Base64.Decoder.decode(...)and strictextractContentType(...)can 500 on malformed/no-prefix data; also prefersetContentLengthLongfor large payloads. This was raised earlier and still applies.Apply:
- byte[] imageBytes = Base64.getDecoder().decode(cleanBase64); + byte[] imageBytes; + try { + imageBytes = Base64.getDecoder().decode(cleanBase64); + } catch (IllegalArgumentException e) { + throw new ServiceException(ExceptionEnum.CM009.getResultCode(), "资源数据格式无效"); + } @@ - String detectedType = ImageThumbnailGenerator.extractContentType(base64Data); + String detectedType; + if (base64Data != null && base64Data.startsWith("data:")) { + detectedType = ImageThumbnailGenerator.extractContentType(base64Data); + } else { + String guessed = java.net.URLConnection.guessContentTypeFromName(name); + detectedType = (guessed != null) ? guessed : "application/octet-stream"; + } @@ - response.setContentLength(imageBytes.length); + response.setContentLengthLong(imageBytes.length);
🧹 Nitpick comments (2)
base/src/main/java/com/tinyengine/it/controller/ResourceController.java (2)
309-309: Remove unnecessarythrows Exceptionand align API doc.The method doesn’t throw checked exceptions; drop
throws Exception. Also, the@Operationlists anisResourceparameter that the method does not accept—update the API doc to avoid confusion.Apply this diff to tighten the signature:
- public void getResource(@PathVariable String name, HttpServletResponse response) throws Exception { + public void getResource(@PathVariable String name, HttpServletResponse response) {
329-340: Use Spring’sContentDispositionbuilder; safer encoding and better UA compatibility.Build
Content-Dispositionwith RFC 5987 support and auto ASCII fallback; also remove the now‑unused manualencodedFileName.Apply:
- String encodedFileName = URLEncoder.encode(name, StandardCharsets.UTF_8).replace("+", "%20"); @@ - String contentDisposition = Utils.isDownload(name) - ? "attachment; filename*=UTF-8''" + encodedFileName - : "inline; filename*=UTF-8''" + encodedFileName; - response.setHeader("Content-Disposition", contentDisposition); + org.springframework.http.ContentDisposition cd = org.springframework.http.ContentDisposition + .builder(Utils.isDownload(name) ? "attachment" : "inline") + .filename(name, StandardCharsets.UTF_8) + .build(); + response.setHeader(org.springframework.http.HttpHeaders.CONTENT_DISPOSITION, cd.toString());If you prefer imports instead of FQNs, add:
import org.springframework.http.ContentDisposition; import org.springframework.http.HttpHeaders;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
base/src/main/java/com/tinyengine/it/controller/ResourceController.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
base/src/main/java/com/tinyengine/it/controller/ResourceController.java (1)
base/src/main/java/com/tinyengine/it/common/utils/ImageThumbnailGenerator.java (1)
ImageThumbnailGenerator(42-515)
🔇 Additional comments (2)
base/src/main/java/com/tinyengine/it/controller/ResourceController.java (2)
311-323: LGTM: Solid input, existence, and empty‑data checks.Null/blank name, missing resource, and empty data are handled upfront. Good guard rails.
315-318: Verify tenant scoping on lookup to avoid cross‑app data exposure.Ensure
resourceService.queryResourceByName(name)restricts byloginUserContext.getAppId()(or equivalent) to prevent downloading other apps’ assets.Run:
Expected: the service/repository method either accepts appId or the underlying SQL includes
WHERE app_id = :appId(or equivalent).
* feat: add resource API * fix: modify source API * fix: modify source API * fix: modify source API * fix: modify source API * fix: modify source API * fix: modify source API * fix: modify source API * fix: modify source API * fix: modify source API * feat: add scheduled tasks * fix: modify resource download * fix: modify resource group update * fix: modify resource group update * fix: modify resource group update * fix: modify resource download * fix: modify block group * fix: modify AI chat * fix: modify AI chat * fix: modify AI chat * fix: modify AI chat * fix: modify AI chat * fix: modify AI chat * fix: modify AI chat * fix: modify resource upload * fix: modify resource upload * fix: modify resource upload * fix: modify resource upload * fix: modify resource upload * fix: modify resource upload * fix: modify resource upload * fix: modify resource download * fix: modify resource download * fix: modify resource download
2 participants
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Bug Fixes