Skip to content

Store filename#101

Open
esposito wants to merge 2 commits into
mainfrom
store_filename
Open

Store filename#101
esposito wants to merge 2 commits into
mainfrom
store_filename

Conversation

@esposito
Copy link
Copy Markdown

@esposito esposito commented May 21, 2026

Description:

In this PR we ensure the filename is always passed when uploading a file to S3. That way, the filename is stored in the metadata, which makes it easier to host the S3 bucket behind a CDN.


Note

Medium Risk
Moderate risk because it changes the upload call chain to always propagate filename into Service#upload (affecting S3 metadata/content-disposition behavior) and adjusts tempfile naming for downloads; could impact integrations that relied on previous defaults.

Overview
Ensures uploaded blobs consistently pass a filename through to service.upload, wiring the value from all supported attachables (uploaded files, Pathname/File, Hash, ActiveStorage::Blob) via StorageTables::Blob.create_and_upload!, upload_without_unfurling, and the attachable upload helpers.

Makes download tempfiles safe for checksums containing / and + by generating the default tempfile prefix from a refactored checksum, and exposes Service#refactored_checksum for reuse. Adds targeted tests for filename propagation and downloader tempfile naming, and tweaks test/devcontainer setup (non-root runner, workspace path /app, and quieter missing-config logging via Rails.logger.debug).

Reviewed by Cursor Bugbot for commit 7a7e252. Bugbot is set up for automated code reviews on this repo. Configure here.

@esposito esposito self-assigned this May 21, 2026
Copilot AI review requested due to automatic review settings May 21, 2026 13:27
Comment thread app/models/storage_tables/blob.rb
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR threads an explicit filename: keyword through the attachment/upload pipeline so storage backends (notably S3) can receive the original filename during uploads, enabling downstream metadata/header behavior that’s helpful when serving via a CDN.

Changes:

  • Add filename: propagation through StorageTables::Blob.create_and_upload!, Blob#upload_without_unfurling, and attachable upload helpers.
  • Update one-attachment attach/upload flow to forward filename: into upload_without_unfurling.
  • Add tests asserting service.upload receives the expected filename: for multiple attachable types, plus update devcontainer post-create setup.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/models/attached/one_test.rb Adds assertions that service.upload receives filename: across attachable types and setter-based assignment.
lib/storage_tables/attachable/one.rb Passes filename: through to the upload helper during attach.
lib/storage_tables/attachable/changes/helper.rb Extends helper upload to accept/forward filename: into blob uploads.
lib/storage_tables/attachable/changes/create_one.rb Passes filename: into Blob.create_and_upload! for supported attachables and hash inputs.
app/models/storage_tables/blob.rb Extends create_and_upload!/upload_without_unfurling to accept and forward filename: to service.upload.
.devcontainer/devcontainer.json Adjusts post-create command to ensure gems are installed and reset the test DB.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/storage_tables/attachable/changes/helper.rb
@esposito esposito force-pushed the store_filename branch 2 times, most recently from 71c25f0 to d937b4c Compare May 22, 2026 07:00
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit d937b4c. Configure here.

raise StorageTables::ActiveRecordError, "Cannot upload a blob inside a transaction"
end

filename ||= extract_filename(attachable)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing filename forwarding in many-attachment upload path

Medium Severity

upload_many calls upload(uploadable.attachable, uploadable.blob) without passing uploadable.filename. Because Helper#upload now falls back to filename ||= extract_filename(attachable), and CreateOne#find_or_build_blob also passes filename: to create_and_upload!, two uploads happen with potentially different filenames. The first upload uses the correct custom filename from CreateOneOfMany, but the second (from upload_many) re-extracts it from the attachable, which may differ for Array-form attachables like [file, "custom.jpg"]. On S3, the second upload overwrites the first's content_disposition, losing the intended filename.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit d937b4c. Configure here.

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.

2 participants