Scaffold: persist file uploads on plain multipart submit (zero JS)#1867
Conversation
Part of #1236. Before: a scaffold with an `Attachment` field generated a URL-encoded form plus a hidden presign-key input, handlers that took `body: Bytes`, and a header note instructing the user to hand-write a presigned direct-upload endpoint and JavaScript. Nothing persisted on a plain multipart submit — the file column could only be filled by writing JS first (a dead-end). After: attachment scaffolds persist files on an ordinary multipart form submit with ZERO JavaScript (progressive enhancement). Only the generator (`autumn-cli/src/generate/scaffold.rs`) changed; the `autumn-web` storage / Multipart API surface is untouched. - Handlers (create/update) now take `mut multipart: autumn_web::extract::Multipart` as the last param (dropping `body: Bytes`). They stream each file part to the blob store with `MultipartField::save_to_blob_store` (which enforces `security.upload.max_file_size_bytes` and returns 413 — propagated via `?`), collect the remaining text parts as urlencoded pairs, and reuse the existing `decode_form` to build/validate the `{Pascal}Form`. - The attachment is no longer a `{Pascal}Form` field (a file isn't urlencodable). `into_new` is sync again and seeds each blob column to `None`; the handler binds the streamed `Blob` afterwards (`new.x = x_blob;`). - Preserve-on-update: an edit with no new file keeps the stored blob (`new.x = x_blob.or(current.x);`), loading the current row through the same handle the update statement uses (repository on live, sharded repo on sharded, diesel otherwise). - The standard `form_for` path promotes the attachment to `FieldControl::File`, which auto-flips the form to `enctype="multipart/form-data"`, renders a plain `<input type=file>` (no hidden key), and keeps the inline-error/ARIA skeleton. The `--live-validation` raw-markup path sets the enctype on the `<form>` tag and drops the hidden key too. - Optional attachment with no file submitted inserts/updates as NULL (the save is skipped when the file part has an empty filename). - Key generation: no framework-side helper exists, and generated apps depend on `uuid` only with the `serde` feature (no `v4`), so keys are derived dependency-free from `chrono` (already a dep + re-exported): `format!("{table}/{field}/{}", chrono::Utc::now().timestamp_nanos_opt()...)`. - Header note flipped: the default documented path is now zero-JS multipart + `save_to_blob_store`; presigned direct-to-storage is demoted to an advanced opt-in pointing at `complete_direct_upload` / `direct_upload_input` and `examples/reddit-clone`. - Auto-enable `storage` + `multipart` on the `autumn-web` dependency when the scaffold has attachments (mirrors the existing `maud`/`htmx` feature wiring with matching `Revert::CargoAutumnWebFeature` bookkeeping). Tests: rewrote the inverted `execute_writes_edit_form_with_attachment_hidden_input` unit test into `execute_writes_zero_js_multipart_attachment_handlers`, and added an integration test plus an `#[ignore]`d cargo-check that compiles a freshly scaffolded attachment resource with only the auto-enabled features. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vWPhpxf9qNAWGpobW2N9W
…note (#1236) Closes two gaps from the adversarial review of the zero-JS multipart upload scaffold. AC6 — generated multipart write-path test Before: attachment scaffolds only got the generic #1127 urlencoded write-path test (`Form { name, witness }` against an in-memory Vec) plus autumn-cli-suite string-match tests. Nothing exercised a real multipart create or asserted blob-key binding. After: a scaffold with >=1 attachment field also emits a dedicated, non-ignored `<plural>_multipart_upload_binds_blob_key` test into the generated project's `tests/`. Following the #1127 in-process style (no database, so it runs green without Docker), it drives the real `Multipart` extractor and real `save_to_blob_store` against a real `LocalBlobStore` (installed via `TestApp::state_initializer` + `BlobStoreState::new`), builds the multipart body by hand (no `.multipart()` helper), binds the returned `Blob` on the record, then asserts the bound blob key is non-empty with the expected prefix AND that the uploaded bytes actually landed in the store at that key. Plain scaffolds are unchanged (still only the #1127 CRUD test). AC4 — CSRF body-scan ceiling in the header note Before: the note claimed the zero-JS path "enforces max_file_size_bytes and returns 413 on oversize" — misleading in prod-with-defaults, where CSRF is auto-enabled and the middleware buffers the body only up to the ~2 MiB `max_scan_bytes` cap while scanning for the token, so a 2-16 MiB upload 403s (token not found in the truncated body) before the handler's 413 can fire. After: the note documents the ~2 MiB practical ceiling under CSRF (the prod default) and points larger uploads at the advanced presigned direct-upload path. Generator note text only; no autumn-web code changed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vWPhpxf9qNAWGpobW2N9W
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements zero-JS multipart file uploads for scaffolds with attachment fields (Issue #1236). It updates the scaffold generator to automatically enable the required storage and multipart features, configures forms to use multipart/form-data, streams file uploads directly to the blob store within the generated handlers, and preserves existing attachments on update. Feedback on the changes suggests appending a UUID to the generated blob keys to prevent potential collisions under high concurrency.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let key = format!(\n \ | ||
| \"{plural}/{name}/{{}}\",\n \ | ||
| chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default()\n \ | ||
| );\n \ |
There was a problem hiding this comment.
Using only a nanosecond timestamp for the blob key prefix ({plural}/{name}/{timestamp}) introduces a potential collision risk under high concurrency. If multiple users upload files for the same field at the exact same nanosecond, their keys will collide, causing one file to overwrite the other in the blob store.
To guarantee uniqueness, consider appending a random suffix (such as a UUID) to the key.
| let key = format!(\n \ | |
| \"{plural}/{name}/{{}}\",\n \ | |
| chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default()\n \ | |
| );\n \ | |
| let key = format!( | |
| "{plural}/{name}/{}_{}", | |
| chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default(), | |
| uuid::Uuid::new_v4() | |
| ); |
A PR reviewer flagged that generated multipart attachment handlers minted
blob keys from a nanosecond timestamp alone. Two uploads for the same field
landing on the same nanosecond — including two attachment fields in the SAME
multipart request — would collide and overwrite one another.
Before: let key = format!("{plural}/{name}/{}", ...timestamp_nanos_opt()...);
After: let key = format!("{plural}/{name}/{}_{}", ...timestamp_nanos_opt()..., uuid::Uuid::new_v4());
The timestamp prefix is kept for time-ordering; the uuid v4 suffix guarantees
uniqueness. Generated attachment apps carried `uuid` with only the `serde`
feature, so `uuid::Uuid::new_v4()` would not resolve — enable its `v4` feature
for attachment scaffolds via a new `ensure_dependency_feature` helper (thin
wrapper over the existing `ensure_dep_feature_status_in_section`), mirroring the
storage/multipart autumn-web feature enablement already in the has_attachments
block. The whole `uuid` dep is still reverted at destroy time by plan_cargo_deps.
Verified end-to-end: a freshly scaffolded attachment app declares
`uuid = { version = "1", features = ["serde", "v4"] }`, `cargo check --tests`
passes against local autumn-web, and the emitted
`photos_multipart_upload_binds_blob_key` test binds a `photos/image/{nanos}_{uuid}`
key and reads the bytes back.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016vWPhpxf9qNAWGpobW2N9W
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c9a9b0bcc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| chrono::Utc::now().timestamp_nanos_opt().unwrap_or_default(),\n \ | ||
| uuid::Uuid::new_v4()\n \ | ||
| );\n \ | ||
| {name}_blob = Some(field.save_to_blob_store(&*store, key).await?);\n \ |
There was a problem hiding this comment.
When a multipart create/update includes a file but the subsequent changeset validation returns 422 (for example, a required caption is missing), the generated handler has already written the object to the blob store here and the 422 path never deletes it or binds it to a row. That leaks unreferenced blobs on ordinary invalid submissions; consider staging the upload or deleting any saved blobs before returning validation/DB errors.
Useful? React with 👍 / 👎.
| for feat in feats { | ||
| plan.push_revert(Revert::CargoAutumnWebFeature { | ||
| path: cargo_path.clone(), | ||
| feature: (*feat).to_owned(), | ||
| owner_dir: Some(models_dir.clone()), |
There was a problem hiding this comment.
This loop gives both storage and multipart the src/models owner directory. For the multipart feature specifically, the generated API usage lives in routes, and autumn_web_feature_markers has no marker for multipart, so destroying the last attachment model can remove autumn-web/multipart even when a hand-written route still uses autumn_web::extract::Multipart. Use a routes owner for multipart or add a project-wide marker so destroy does not break those routes.
Useful? React with 👍 / 👎.
`autumn destroy` of the last attachment model bookkeeps the `multipart` and `storage` autumn-web features via `Revert::CargoAutumnWebFeature` with an owner_dir under `src/models`. But `autumn_web_feature_markers` had no arm for either feature, so the whole-project retention scan never fired and destroy would strip the features from Cargo.toml even when a hand-written route still uses `autumn_web::extract::Multipart` or `autumn_web::storage::...`, breaking compilation. Add marker arms mirroring the existing ws/mail/oauth2 arms: - "multipart" => "extract::Multipart" (matches the full path and a `use autumn_web::extract::Multipart;` import) - "storage" => "autumn_web::storage::" (matches the model's Blob column type and route usage like BlobStoreState / save_to_blob_store call sites) Adding markers is strictly conservative — can only retain a feature more often, never wrongly strip one. Owner_dir in scaffold.rs is unchanged. Regression coverage: a unit test asserting the marker slices, a fixture test exercising `autumn_web_feature_still_needed_elsewhere`, and an end-to-end test that generates an attachment scaffold, adds a hand-written route using both APIs, destroys the scaffold, and asserts both features survive. Addresses PR review. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016vWPhpxf9qNAWGpobW2N9W
|
Thanks for the review — addressing both Codex findings: Finding 2 (feature markers / Finding 1 (orphaned blob on 422 / DB error) — confirmed, but deferred to #1872. The uploaded file is saved to the blob store during multipart parsing, before changeset validation and the DB write, so every early return (invalid changeset, unique-violation 422, generic DB error) leaks the blob. It's outside #1236's acceptance criteria, and the framework's direct-upload path deliberately tolerates orphans (bind-step promotion, cleanup via sweep — Generated by Claude Code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7eebaf9775
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // type (`autumn_web::extract::Multipart`) and a `use | ||
| // autumn_web::extract::Multipart;` import, without matching unrelated | ||
| // identifiers that merely contain `Multipart`. | ||
| "multipart" => &["extract::Multipart"], |
There was a problem hiding this comment.
When a hand-written route uses the idiomatic use autumn_web::prelude::*; and takes Multipart unqualified, this marker never matches even though autumn/src/prelude.rs re-exports Multipart. Destroying the last attachment scaffold will therefore still remove autumn-web/multipart in that common route shape, leaving the route unable to compile; include a marker that catches prelude/unqualified Multipart usage or make the scan understand imports.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch on the multipart half — the prelude does re-export Multipart (autumn/src/prelude.rs: #[cfg(feature = "multipart")] pub use crate::extract::Multipart;), so a use autumn_web::prelude::*; route naming Multipart unqualified would have been missed. Broadened the multipart marker from extract::Multipart to bare Multipart in c42a4b3, and updated the retention tests to the prelude/unqualified shape.
On storage: leaving that marker as-is. The prelude re-exports no storage types (no Blob/BlobStore/BlobStoreState/storage re-export in prelude.rs), so every reachable spelling of a hand-written storage user goes through an autumn_web::storage::… path (fully-qualified or a use autumn_web::storage::{…} import), which the existing autumn_web::storage:: marker already matches. If a future change adds storage types to the prelude, we'd broaden it the same way.
Generated by Claude Code
…ed usage The multipart feature-retention marker was `extract::Multipart`, which misses a hand-written route that imports the extractor through `use autumn_web::prelude::*;` and names `Multipart` UNQUALIFIED (the prelude re-exports it). `autumn destroy` of the last attachment model would then wrongly strip `autumn-web/multipart` and break that route. Broaden the marker to the bare `Multipart` substring, which still matches the scaffold's own `autumn_web::extract::Multipart` and any `use ...::Multipart;` import while also catching the prelude-unqualified spelling. Extra hits like `MultipartField`/`MultipartError` are part of the multipart API surface, so over-retaining on them is harmless. The storage marker is left as-is: the prelude re-exports no storage type, so a hand-written storage user must reach `Blob`/`BlobStore`/… via a `autumn_web::storage::…` path (qualified or a `use` import), which the existing marker already catches. Tests updated to drive the prelude-style unqualified-`Multipart` shape and to keep the still-removes-when-unused assertion. Addresses PR review follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c42a4b3ed7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let _ = write!( | ||
| match_arms, | ||
| " Some(\"{name}\") => {{\n \ | ||
| if field.file_name().is_some_and(|file_name| !file_name.is_empty()) {{\n \ |
There was a problem hiding this comment.
In projects that configure security.upload.allowed_mime_types or reject_on_content_type_mismatch, a standard multipart create/edit that leaves the optional file input empty can still include a part with filename="". autumn_web::extract::Multipart::next_field() treats any file_name().is_some() part as a file and runs MIME sniffing before this generated !file_name.is_empty() guard, so the request is rejected before the handler can skip the empty upload or preserve the current blob. Empty file parts need to be filtered before upload MIME validation, or the extractor needs to treat filename="" as non-file input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — and it's real, but the fix is in the extractor, not the generator, so I'm tracking it as #1873 rather than in this PR. Multipart::next_field() gates MIME sniffing on file_name().is_some() (autumn/src/extract.rs:199), which is true for an empty file input's filename="" part, so with a non-empty allowed_mime_types or reject_on_content_type_mismatch the 0-byte part is rejected inside next_field() (extract.rs:243-253) before the generated !file_name.is_empty() guard can skip it — the handler only ever receives the Err, so there's no generator-only mitigation. Default-config projects (empty allow-list, reject_on_content_type_mismatch=false, per config.rs:1403-1410) are unaffected. Keeping this PR within #1236's generator-only scope (no autumn-web Multipart changes); #1873 covers the extractor fix.
Generated by Claude Code
2 participants
Part of #1236.
Before / After
Before:
autumn generate scaffold post avatar:Attachmentrendered an<input type="file">but wired only the direct-browser-to-storage presign path — expecting a hiddenkeyfield that a presign endpoint and client JavaScript were supposed to populate, neither of which the scaffold generated. Submitting the scaffolded form stored nothing.After: a scaffolded
Attachmentfield works end-to-end on a plain form submit with zero hand-written JavaScript or presign endpoint. The generated form isenctype="multipart/form-data"with a plain file input; the generated create/update handlers take aMultipartextractor, stream each uploaded file to the configuredBlobStoreviasave_to_blob_store, and bind the returnedBlobto the model column. Updating without choosing a new file preserves the existing attachment; an optional attachment left empty storesNULL.How
Generator-only change in
autumn-cli/src/generate/scaffold.rs:mut multipart: autumn_web::extract::Multipart(kept as the last, body-consuming param); the loop streams file parts to the blob store and collects text parts, then runs the existing changeset/validation flow and binds the blob(s) ontoNew{Pascal}.form_forpath renders attachment columns asFieldControl::File(auto-sets the multipart enctype + plain file input, preserves error/ARIA); the--live-validationpath sets the enctype directly. The hidden presignkeyinput is removed from both paths.new.x = new_blob.or(current.x)).storage+multipartfeatures are auto-enabled onautumn-webfor attachment scaffolds.complete_direct_upload/direct_upload_input,examples/reddit-clone).No changes to
autumn-web, the workspace version,CHANGELOG.md,skills/, ordocs/.Testing
cargo fmt --all -- --checkclean;cargo clippy --workspace --all-targets -- -D warningsclean;cargo test -p autumn-cligreen.cargo check --testscompiles with onlystorage+multipart, and its generated write-path test runs green: a no-JSmultipart/form-dataPOST persists a blob and asserts the bound key (<plural>_multipart_upload_binds_blob_key).Known limitation (tracked in #1866)
With CSRF enforcement on (the prod-profile default), the middleware buffers the request body only up to ~2 MiB while scanning for the form token, so a zero-JS multipart upload larger than ~2 MiB is rejected with 403 before the handler's
max_file_size_bytes/413 check runs. The generated note documents this ceiling and routes larger files to the presigned path. Reconcilingmax_scan_byteswithmax_file_size_bytesis anautumn-webchange split out to #1866.