Skip to content

fix: preserve URL extensions for generic downloads - #543

Merged
rickdijk merged 7 commits into
Baseflow:mainfrom
rahulshahDEV:fix/489-extension-fallback
Nov 2, 2026
Merged

rickdijk merged 7 commits into
Baseflow:mainfrom
rahulshahDEV:fix/489-extension-fallback

Conversation

@rahulshahDEV

@rahulshahDEV rahulshahDEV commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

✨ What kind of change does this PR introduce? (Bug fix, feature, docs update...)

Bug fix.

⤵️ What is the current behavior?

When a download response uses a missing or generic Content-Type, the cached file can get a generic extension such as .bin even when the URL ends in a meaningful file extension.

🆕 What is the new behavior (if this is a feature change)?

When the content type is missing or generic, a short alphanumeric extension containing a letter from the URL's final path segment is used when available; known dynamic endpoint suffixes are ignored. A specific response MIME type remains authoritative, query parameters do not affect the extension, and the legacy adapter follows the same rule. The exported HttpGetResponse(response) constructor remains supported and uses the response request URL when available.

💥 Does this PR introduce a breaking change?

No.

🐛 Recommendations for testing

dart format --set-exit-if-changed, flutter analyze, and flutter test --coverage passed locally (138 tests) with Flutter 3.47.4. Platform example builds passed on the previous head; CI needs to run on this head.

📝 Links to relevant issues/docs

Closes #489.

🤔 Checklist before submitting

  • All projects build (platform example builds await CI on this head).
  • Follows style guide lines (code style guide).
  • Relevant documentation was updated.
  • Rebased onto current main.
  • Version bumped and dated CHANGELOG entry added.

@rahulshahDEV
rahulshahDEV force-pushed the fix/489-extension-fallback branch from 8afb8d4 to 69f551a Compare October 27, 2026 10:59
@rickdijk

Copy link
Copy Markdown
Contributor

Re-reviewed at 69f551a: all earlier comments are addressed and covered by tests. Locally dart format --set-exit-if-changed, flutter analyze and flutter test (132 tests) pass on Flutter stable. Two small non-code things before merge:

  • PR description is out of date. "invalid path, control, or platform-reserved characters fall back" no longer describes the logic (it's now a short-alphanumeric allowlist, a list of generic MIME types, and ignored .php/.asp*/.cgi/.jsp). The testing section still cites 118 tests on a 3.49 pre-release SDK. Per AGENTS.md, please keep "Recommendations for testing" to one or two sentences about what ran.
  • CHANGELOG wording: "tolerates malformed legacy URLs" describes a regression introduced and fixed within this PR, so it means nothing to 3.4.6 users. Something like: Uses the URL's file extension when a download's content type is missing or generic (e.g. application/octet-stream, binary/octet-stream) (#489).

@rahulshahDEV
rahulshahDEV force-pushed the fix/489-extension-fallback branch from c87f751 to d9be08c Compare November 2, 2026 07:39
@rickdijk

rickdijk commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

Reviewed at d9be08c. On this head, dart format --set-exit-if-changed, flutter analyze and flutter test (136 tests) pass. The new tests fail on the base, as they should.

Criterion Verdict Reason
Real value pass Fixes #489: files served as application/octet-stream were always cached as .bin. This is common on S3 (binary/octet-stream).
Really fixes it concern Works with the default client. It is skipped when the HTTP client doesn't set response.request, and some URLs still produce wrong extensions.
Breaks something pass Only adds an optional constructor parameter, and a patch bump fits. Firebase download URLs (%2F paths) keep working.
Tests concern Three of the new tests pass without reaching the URL fallback.
Guidelines concern The CHANGELOG date is out of order, and CI is waiting for approval on this head.

What I'd like changed:

  1. Pass req.url in HttpFileService.get. HttpGetResponse(httpResponse, req.url). Right now the URL comes from _response.request?.url, which is null for any client whose send() doesn't set request. With a MockClient response that has no request, https://e.com/video.mp4 served as application/octet-stream came back as .bin. I asked earlier to drop this argument, but that assumed every client sets request, and they don't. Sorry about that.
  2. All-digit suffixes (the open thread): /api/v1.2 is still cached as .2. Please add the regex change and a test for it.
  3. Tests that skip the fallback: "Generic content type without URL extension remains .bin", "Specific content type takes precedence over URL extension" and "Legacy file service uses URL extension for generic content type" build responses without request:. For the first two, the URL never reaches HttpGetResponse. Pass request: request in their mock responses.
  4. CHANGELOG date: the 3.4.8 entry is dated 2026-10-27 but sits above 3.4.7 (2026-10-31). Please date it 2026-11-02.

Optional:

  • download.ashx?id=7 is cached as .ashx. The list of server-script suffixes to skip will keep growing (.do, .action, .cfm, …). A simpler rule is to accept a URL suffix only when it is a known extension in the mimeTypes map, compared case-insensitively. That also covers the all-digit case.
  • The URL extension keeps its case (VIDEO.MP4 becomes .MP4). Consider lowercasing it.
  • p.url.extension() from package:path, which is already a dependency, could replace the hand-written lastIndexOf parsing.

Once items 1–4 are in and CI is green on the new head, this looks good to merge.

@rickdijk
rickdijk merged commit 4e114a7 into Baseflow:main Nov 2, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Downloaded files are always stored as .bin instead of preserving the original extension

2 participants