Skip to content

Fix extensionless file serving from disk - #21

Merged
zachdaniel merged 4 commits into
ash-project:mainfrom
deep-c:feature/disk-filename-serve
May 11, 2026
Merged

Fix extensionless file serving from disk #21
zachdaniel merged 4 commits into
ash-project:mainfrom
deep-c:feature/disk-filename-serve

Conversation

@deep-c

@deep-c deep-c commented May 8, 2026

Copy link
Copy Markdown
Contributor

Contributor checklist

Leave anything that you believe does not apply unchecked.

  • [ x] I accept the AI Policy, or AI was not used in the creation of this PR.
  • Bug fixes include regression tests
  • [ x] Chores
  • Documentation changes
  • [ x] Features include unit/acceptance tests
  • Refactoring
  • Update dependencies

Tested in my project locally and it works as expected. Fixing #20

Changes:

  1. The original filename (e.g. photo.svg) is already stored in the database alongside the storage key. The fix threads that filename through to the URL so the serving plug can use it. URLs now look like /files/<hex-key>/photo.svg instead of /files/<hex-key>. The file on disk doesn't move — the hex key is still used to find it — the filename segment is just there so the plug can derive the correct content type from the extension without hitting the database on every request.

Old-format URLs (without the filename segment) continue to resolve to the correct file. They'll still get application/octet-stream since there's no extension to read, but nothing breaks.

  1. Special characters in filenames (including /) are percent-encoded in the URL to prevent them from corrupting the path structure.

Change I'm not 100% sure on:

The existing "serves nested files" test set up a file at root/sub/dir/nested.txt and requested it via /sub/dir/nested.txt. This worked under the old code because the entire path was joined and used as the storage key. Under the new code, only the first path segment is used as the storage key, so the same request would look for a file at root/sub and return a 404.

The test was replaced rather than updated because multi-segment storage keys don't actually exist in practice — the storage key is always a single 56-character hex string. The nested path test was accidentally passing by treating the whole URL path as the storage key, which was never a real use case. It's been replaced with a test that reflects how the disk service actually works: an extensionless hex key as the first segment, the original filename as the second.

This is technically a behaviour change for anyone using DiskServe to serve files stored under multi-segment keys manually (e.g. root: "priv/storage" with files at priv/storage/users/123/avatar). That pattern was never part of the documented API and doesn't occur when files are stored through AshStorage.Service.Disk itself

Not sure if you disagree but the change made sense to me. Happy to change with guidance

Thanks!

@zachdaniel

Copy link
Copy Markdown
Contributor

The mix audit is fine, but mind addressing credo?

@deep-c

deep-c commented May 8, 2026

Copy link
Copy Markdown
Contributor Author

Np, Should be fixed now @zachdaniel

@zachdaniel

Copy link
Copy Markdown
Contributor

Hmm...looks like some more failures.

@sodapopcan

sodapopcan commented May 11, 2026

Copy link
Copy Markdown
Contributor

I came here to report this issue and was excited to see a fix already in the works.

So I want to throw a lil' wrench into this and ask why can't the extension just be appended to the hash instead of exposing the original filename? Or even just grabbing the extension from the original filename instead of calling MIME.from_path? I appreciated that the default was to not expose the user's filename to the web, and for some kinds of apps this can be important. Think of a site like Reddit where privacy is a feature and you want to protect people from dumb slip-ups like uploading a photo called, my-name-bob-smith-and-i-live-at-123-real-st.jpg. I realize that is super contrived but you get what I mean. Now that I think of it it would be nice to have the option to not even store the original filename (though that is out of scope here).

EDIT: of course the reverse can also be true where perhaps for SEO reasons your users may want well-named images... I have lived experience of this working very, very well, though that was a different time so I don't know how relevant this still is.

@zachdaniel
zachdaniel merged commit a8c7354 into ash-project:main May 11, 2026
21 of 24 checks passed
@zachdaniel

Copy link
Copy Markdown
Contributor

@sodapopcan open to configuration to let the user opt into/out-of preserving the original file name 😄

@sodapopcan

sodapopcan commented May 11, 2026

Copy link
Copy Markdown
Contributor

It looks like I misunderstood how this PR works anyway and it doesn't show any file names sooooo I'm good, lol. Thanks, @deep-c!

EDIT: I misunderstood because I'm looking at DiskServe so ya, I may come back to this once I deploy.

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.

3 participants