Code Review: feat: add file upload & storage support (Roadmap 2.2)

This is a substantial, well-structured PR. The storage abstraction is clean, test coverage is thorough, and the golden file + integration test approach gives strong confidence in generated output. A few items to address before merging:

Security

Path traversal in LocalStore (medium severity)

LocalStore.Save, Open, and Delete all do:

path := filepath.Join(s.baseDir, filepath.FromSlash(key))

filepath.Join calls filepath.Clean internally, which resolves .. — so filepath.Join("/uploads", "../etc/passwd") yields /etc/passwd. If a key ever contains ../ sequences, this escapes the base directory.

In practice, the generated handler builds keys from server-controlled upload entry IDs, and S3Presigner.generateKey correctly calls filepath.Base(entry.ClientName) to strip path components. But LocalStore has no equivalent guard, so the interface contract is weaker than it should be.

Suggested fix (add to all three methods, or factor into a shared resolvePath):

func (s *LocalStore) resolvePath(key string) (string, error) {
    path := filepath.Join(s.baseDir, filepath.FromSlash(key))
    base := filepath.Clean(s.baseDir) + string(os.PathSeparator)
    if !strings.HasPrefix(path, base) {
        return "", fmt.Errorf("storage: key escapes base directory")
    }
    return path, nil
}

Bugs

S3Store.Delete doesn't strip URL prefixes (unlike LocalStore.Delete)

LocalStore.Delete accepts both raw keys and full URLs (via resolveKey), since the DB stores the URL returned by URL(). S3Store.Delete has no equivalent stripping. Calling it with a full S3 URL will silently attempt to delete a non-existent key (S3 returns 204 for missing keys), leaving the object orphaned.

The handler template calls c.Store.Delete(ctx, existing.Photo) where existing.Photo is the URL stored in the DB. This will silently fail for S3.

Fix: add key resolution to S3Store.Delete (strip CDN/endpoint/standard S3 prefix to recover the raw key), or document that callers must pass raw keys and have the template store raw keys rather than full URLs.

S3Store.Save doesn't set ContentType

PutObject is called without a ContentType:

_, err := s.client.PutObject(ctx, &s3.PutObjectInput{
    Bucket: aws.String(s.config.Bucket),
    Key:    aws.String(key),
    Body:   reader,
})

Objects will be stored with binary/octet-stream, breaking browser rendering for images served directly from S3. The Store.Save signature could accept contentType string as a parameter, or a separate SaveWithMeta variant.

Dependencies

golang.org/x/image is pinned to a 2019 snapshot

golang.org/x/image v0.0.0-20191009234506-e7c1f5e7dbb8

This is ~6 years old. The current stable tagged version is v0.27.0. The old version may have unpatched vulnerabilities in image decoding (the exact attack surface that pkg/imaging exposes). Update to a recent tagged release:

go get golang.org/x/image@latest

The disintegration/imaging library itself is also unmaintained (last release 2019, v1.6.2). Worth noting in docs/comments in case a future maintainer wants to migrate to golang.org/x/image/draw directly.

Minor / Nitpicks

Manual setup instructions hard-code LocalStore

The post-generation message in commands/gen.go prints:

store := storage.NewLocalStore("uploads", "/uploads")

This is helpful for quick-start, but users who have an S3 setup get no guidance. Could mention both options or link to docs.

Store.URL not on the interface

URL(key string) string is defined on both LocalStore and S3Store but is absent from the Store interface in storage.go. The generated handler calls c.Store.URL(key) which means it currently only works via the concrete type, not the interface. Either add URL(key string) string to the Store interface, or the template needs to cast the type. (This may cause a compile error in generated code if c.Store is typed as storage.Store.)

imaging package is not used in generated code

pkg/imaging is added as a dependency but the generated handler templates don't appear to call GenerateThumbnail. If thumbnail generation is deferred to a future milestone, the import in go.mod and the package itself shouldn't be included yet — it adds a dependency with no current consumer. If it's intentional for app developers to call it themselves, it should be mentioned in the post-gen output or docs.

What's well done

• The Store interface is minimal and composable — easy to test and swap implementations
• LocalStore.resolveKey accepting both URL and raw key is ergonomic
• S3Presigner correctly uses filepath.Base to prevent path traversal in key generation
• The img = nil GC hint in GenerateThumbnail before encoding is a nice touch
• Test coverage is excellent: parser, storage, imaging, presigner, golden files, content validation, and a full compile test
• Skipping route injection for upload resources and guiding the user through manual wiring is the right tradeoff

Summary

The two most important items: LocalStore path traversal (security) and Store interface missing URL (likely compile error in generated code). The S3Store.Delete URL-stripping issue will cause silent data leaks in production. Everything else is polish.

PR Review: feat: add file upload and storage support (Roadmap 2.2)\n\nThis is a solid, well-structured feature addition. The storage abstraction is clean, test coverage is thorough, and path traversal protection is handled correctly in both the S3 key generator and the LocalStore. The page-mode refactor from per-request handler cloning to a shared handler with query-param routing is a meaningful improvement. Notes below are roughly ordered by severity.\n\n---\n\n### Bugs / Correctness\n\n1. Old files not deleted on update (resource leak)\n\nIn the generated Update handler, when a new file is uploaded the old file stored at existing.Photo / existing.Doc is never removed from storage. Only ConfirmDelete cleans up files. Over time this will silently accumulate orphaned files in both LocalStore and S3.\n\n2. Stale golang.org/x/image dependency\n\nThe pinned commit is from October 2019 (~6 years old). Worth bumping to at least v0.18.0.\n\n---\n\n### Design / API Concerns\n\n3. pkg/imaging is not wired into generated code\n\nGenerateThumbnail is implemented and tested but the generated handler template never calls it.\n\n4. disintegration/imaging is an archived package\n\nThe upstream repo is archived (read-only, no new releases).\n\n5. No server-side MIME type validation\n\nThe generated handler trusts entry.ClientType (client-provided) for content type.\n\n---\n\n### Minor / Polish\n\n6. Silent file deletion errors in ConfirmDelete - a log.Printf would help operators detect orphaned files.\n\n7. queries.sql.tmpl UPDATE uses .Fields instead of .NonReferenceFields\n\n8. _resource_id query param: mutating r.URL directly - Passing via context.WithValue would be cleaner.\n\n---\n\n### Positives\n\n- Clean Store interface with consistent URL-vs-key semantics\n- Path traversal protection is correct in safePath and key sanitization\n- IsFile fields are correctly excluded from search, sort, and form input structs\n- Golden file tests for the upload handler are a nice addition\n- Full flow test (generate -> sqlc -> build) that catches schema/query/model alignment is excellent\n- The page-mode handler refactor is a real improvement over per-request template cloning

Code Review: feat: add file upload & storage support (Roadmap 2.2)

This is a well-scoped, well-tested feature addition. The architecture is clean -- Store interface with LocalStore/S3Store implementations, clear field flag propagation (IsFile/IsImage), and good coverage with unit + golden + integration tests. A few issues worth addressing before merge.

1. Old file not deleted on update (storage leak): In handler.go.tmpl, the Update handler reads the existing record and falls back to existing file values when no new upload is provided -- but it never deletes the old file from storage when a replacement is uploaded. Over time this leaks orphaned files in both LocalStore and S3. After saving the new file, the old URL should be passed to c.Store.Delete.

2. S3Store.Save() drops the Content-Type: pkg/storage/s3.go calls PutObject without setting ContentType. Files served directly from S3 will default to application/octet-stream, so images will not render inline in browsers. Compare with the presigner which correctly sets ContentType.

3. defer f.Close() inside a range loop: In the generated Add and Update handlers, each iteration over FileFields opens a temp file and defers its close. With multiple file fields the defers only fire when the function returns, holding OS file descriptors for the full handler duration. Prefer closing explicitly after Store.Save returns.

4. Client-supplied MIME type is not server-validated: The generated handler stores entry.ClientType directly in the database as ContentType. This value comes from the browser and can be spoofed. For image fields, consider validating actual bytes (http.DetectContentType) before saving.

5. Filename stored verbatim: entry.ClientName is stored as-is and rendered into the DOM. Go html/template auto-escapes in most contexts so this is likely safe, but worth a deliberate check -- especially the href rendering path.

6. 10 MB limit is hardcoded: MaxFileSize: 10 << 20 will be wrong for many use cases. Consider making this a generator argument or named constant.

7. pkg/imaging is shipped but not wired: pkg/imaging.GenerateThumbnail is added with tests but the generated handler never calls it. Either wire it in for image fields or document it as a manual utility.

8. S3Store.Delete() is not idempotent: LocalStore.Delete() silently ignores os.IsNotExist; S3Store.Delete() propagates any error. Aligning the two implementations is cleaner.

9. Redundant fieldTypeTable entries for file/image: These are also handled by the early-return branch in ParseFields that sets IsFile/IsImage. The table entries produce a Field without those flags set, creating a subtly different code path if MapType is called directly.

10. err2 variable in integration test: integration_test.go uses err2 when reading main.go. The prior err has already been checked and can be reused.

Test coverage that is good: parser tests for IsFile/IsImage flags, LocalStore round-trip, path traversal prevention in safePath and generateKey, golden file for file-upload handler, content-validity assertions in TestFileUploadResourceGeneration, full compile flow in TestFileUploadFullFlow.

Missing coverage to consider: S3Store.Delete with a non-existent key; verification that old file is cleaned up during Update (once issue 1 is fixed); Content-Type header on S3Store.Save.

Overall this is solid work. The critical items are 1 (storage leak on update) and 2 (missing Content-Type on S3 upload) -- the rest are improvements.

REPLACE_COMMENT_PLACEHOLDER

This is a solid, well-structured feature addition. The storage abstraction is clean, test coverage is thorough, and path traversal protection is handled correctly in both the S3 key generator and the LocalStore. The page-mode refactor is a meaningful improvement. Notes below are roughly ordered by severity.

Bug 1 - Old files not deleted on update: In the generated Update handler, when a new file is uploaded the old file at existing.Photo or existing.Doc is never removed from storage. Only ConfirmDelete cleans up files. Over time orphaned files accumulate in both LocalStore and S3. Fix: delete the old key before saving the new one, similar to the ConfirmDelete pattern.

Bug 2 - Stale golang.org/x/image dependency: v0.0.0-20191009234506-e7c1f5e7dbb8 is from October 2019 (~6 years old). The current release includes security and correctness fixes. Worth bumping to at least v0.18.0.

Design 1 - pkg/imaging is not wired into generated code: GenerateThumbnail is implemented and tested but the generated handler template never calls it. Files are saved as-is, so image resources are not actually thumbnailed. Either wire it into the handler template or clarify in the package doc that this is a utility callers opt into manually.

Design 2 - disintegration/imaging is an archived package: The upstream repo is archived (read-only, no new releases). For a code-generation tool shipping a new pkg/imaging package, this is a long-term maintenance concern. Alternatives include golang.org/x/image directly or stdlib image/* packages for basic resize.

Design 3 - No server-side MIME type validation: The generated handler trusts entry.ClientType (client-provided) for content type. For image fields there is no server-side check that the uploaded bytes are actually an image. Consider using net/http.DetectContentType on the first 512 bytes before saving.

Minor 1 - Silent file deletion errors in ConfirmDelete: Ignoring Store.Delete errors is reasonable since the DB row is deleted regardless, but a log.Printf would help operators detect orphaned files.

Minor 2 - queries.sql.tmpl UPDATE uses .Fields instead of .NonReferenceFields: Both queries.sql.tmpl and embedded_queries.sql.tmpl use .Fields for the UPDATE SET clause, meaning foreign key reference fields are included in UPDATE. Pre-existing behavior, but since this PR touches these templates it might be worth aligning with the handler use of NonReferenceFields.

Minor 3 - Mutating r.URL directly in page-mode handler: r.URL.RawQuery = q.Encode() mutates the incoming request URL in place. Passing the resource ID via context.WithValue would be cleaner and avoid potential edge cases with middleware that reads the URL.

Positives: Clean Store interface, correct path traversal protection in safePath and key sanitization, IsFile fields correctly excluded from search/sort/input structs, golden file tests for upload handler, excellent full flow test (generate->sqlc->build), and the page-mode refactor to a single shared handler is a real improvement over per-request template cloning.

PR Review: feat: add file upload and storage support (Roadmap 2.2)

This is a solid, well-structured feature addition. Good test coverage and clean architecture. Below are observations ranging from bugs to minor nits.

Bugs / Correctness Issues

1. S3Store.Save missing ContentType

pkg/storage/s3.go Save() does not set ContentType on PutObjectInput. Images uploaded to S3 will be served as application/octet-stream, breaking img-tag preview in browsers. The Store interface does not accept content type, so this needs either an interface change or inferring the type from the key extension via mime.TypeByExtension.

2. disintegration/imaging marked as indirect in go.mod

pkg/imaging/thumbnail.go imports this package directly, but go.mod marks it as // indirect. Running go mod tidy should fix this.

3. go.sum missing /go.mod entry for golang.org/x/tools v0.42.0

The diff adds the h1: hash but the corresponding /go.mod h1: line is absent. Other packages have both. This may cause go mod verify failures.

Security Concerns

4. No MIME type validation for image fields

The generated handler stores any uploaded file without verifying it is actually an image. An attacker could upload HTML, SVG, or an executable to an image field. Consider validating that entry.ClientType has prefix image/ in the generated upload processing code.

5. Client-reported file size stored verbatim

The generated code uses entry.ClientSize (client-reported) rather than actual bytes written. Consider tracking bytes via io.TeeReader in Store.Save, or document that this value is unverified client data.

6. LocalStore.FileServer() serves entire baseDir without auth

http.FileServer(http.Dir(baseDir)) exposes directory listings and all files under baseDir. A doc comment noting this is development-only (use a CDN or signed URLs in production) would set expectations.

Design / API Issues

7. Store.Delete accepting URLs is a surprising contract

The interface says "Delete also accepts URLs returned by URL()" but callers must remember this dual key/URL behavior. The generated handler passes a URL (the existing stored URL) to Delete. A comment at the generated call-site would reduce confusion for anyone reading the output code.

8. Inconsistent not-found behavior: LocalStore vs S3Store

LocalStore.Delete swallows os.IsNotExist errors gracefully. S3Store.Delete returns an error if the object does not exist. The generated handler masks this with _ = ... but it will surprise anyone using S3Store.Delete directly.

Minor Issues

• err2 in TestFileUploadFullFlow: unusual variable naming; restructuring the block to reuse err would be cleaner.
• GenerateThumbnail doc missing upscale note: imaging.Fit does not upscale small images; the test covers this but the doc comment does not mention it.
• Route skip message does not mention S3Store: the printed instructions only reference NewLocalStore; a mention of NewS3Store for production would improve discoverability.

What is Done Well

• safePath path traversal prevention in LocalStore is correct and well-tested
• 4-column expansion (url, filename, content_type, size) is a clean schema design
• Skipping auto-injection for upload resources with clear manual setup instructions is the right call
• NonFileFields() and FileFields() helpers are a clean separation of concerns in the template data layer
• Search and sort correctly skip file/image fields
• Type inference for common field names (avatar, photo, document, etc.) is user-friendly
• Golden file test for the file upload handler is a valuable regression guard

The most important items before merging: 1 (S3 ContentType, affects correctness of the primary use case), 3 (go.sum integrity), and 4 (MIME validation in generated code, since this lands in user apps).

Generated with Claude Code

Code Review: feat: add file upload & storage support (Roadmap 2.2)

Great work on this feature — a substantial, well-structured addition. The Store interface design, 4-column SQL expansion, UseUpload flag to skip auto-injection, and path-traversal protection are all sound. Test coverage across the new packages is notably thorough. Here is what I found that deserves attention before merging.

Security

[Medium] http.FileServer exposes directory listings on the uploads folder

pkg/storage/local.go, FileServer method:

http.FileServer(http.Dir(s.baseDir))

http.Dir-based file servers serve directory listings by default. If a user navigates to /uploads/ they will see all uploaded filenames — likely unintended information disclosure for most apps. Consider returning 403 on directory requests:

type noListFS struct{ http.FileSystem }
func (f noListFS) Open(name string) (http.File, error) {
    file, err := f.FileSystem.Open(name)
    if err != nil { return nil, err }
    if stat, _ := file.Stat(); stat.IsDir() { file.Close(); return nil, fs.ErrPermission }
    return file, nil
}

[Low] S3Store.resolveKey has an empty-prefix edge case

pkg/storage/s3.go:

for _, prefix := range []string{
    s.config.CDNPrefix + "/",               // becomes "/" when CDNPrefix is ""
    fmt.Sprintf("%s/%s/", s.config.Endpoint, s.config.Bucket), // becomes "/bucket/" when Endpoint is ""
    ...

The guard prefix != "/" prevents the first case from matching, but when Endpoint is empty the second entry becomes "/bucket/" — any key starting with /bucket/ would be incorrectly stripped, potentially targeting the wrong object. A test covering the empty-endpoint case would surface this.

Bugs / Correctness

[Medium] defer f.Close() inside a loop holds all file descriptors until the handler returns

handler.go.tmpl (generated Add/Update functions):

// for each file field:
f, err := os.Open(entry.TempPath)
...
defer f.Close()   // ← deferred until Add/Update returns, not until after Store.Save

With N file fields, N descriptors stay open simultaneously. The fix is to close explicitly after Store.Save:

f, err := os.Open(entry.TempPath)
...
_, err = c.Store.Save(ctx, key, f, contentType)
f.Close()  // explicit, not deferred

[Low] Store.Delete is called with a URL, not a key — undocumented dual contract

handler.go.tmpl, Update block: the existing field value (stored as a URL from c.Store.URL(key)) is passed directly to Store.Delete. LocalStore and S3Store both implement resolveKey to handle this, but the Store interface documentation does not mention it. A future store implementation that expects a bare key would silently fail to delete old files. Please document this on the interface.

[Low] pkg/imaging/thumbnail.go is never called from generated handlers

GenerateThumbnail is shipped and tested but the image field upload path does not invoke it — images are stored at full resolution. This should either be wired in (e.g., auto-thumbnail on image field save) or documented as an opt-in utility, otherwise it is dead code from day one.

Code Quality

[Low] Duplicated AWS config initialization in pkg/s3presigner and pkg/storage/s3.go

Both packages have nearly identical if cfg.AccessKeyID != "" && cfg.SecretAccessKey != "" blocks constructing aws.Config with static credentials (~20 lines each). A shared internal helper would reduce future maintenance cost.

[Low] Golden file struct alignment likely fails gofmt

testdata/golden/file_upload_handler.go.golden (and the corresponding template):

PhotoContentType: photoContentType,
PhotoSize:         photoSize,   // extra space here

The extra space in the template alignment means go fmt will reformat the generated output, breaking golden tests on any CI that runs gofmt. Please verify the template renders gofmt-clean output.

[Low] TestFileUploadFullFlow uses os.Chdir — add a non-parallelizable marker

integration_test.go: os.Chdir(tmpDir) changes the process-wide working directory. The test does not call t.Parallel(), which is correct, but there is no comment to warn future maintainers. Please add:

// This test must not be run in parallel — it calls os.Chdir which affects the whole process.

Test Coverage Gaps

• LocalStore.Delete is not tested with a URL containing .. after the prefix (path-traversal via Delete)
• S3Store.Delete / resolveKey with an empty Endpoint is not covered
• No test verifying GenerateThumbnail is exercised on image field upload (see dead-code note above)

Performance Notes

• S3 upload body content-length: S3Store.Save passes an io.Reader directly as PutObject.Body. The AWS SDK v2 may buffer the entire file in memory if it cannot determine content length from the reader. Consider setting ContentLength explicitly from the multipart file header size when available.
• The extra GetByID query in Update before updating file fields is necessary for "keep if not replaced" semantics — acceptable trade-off.

Summary

The medium issues should be addressed before merging. The low-priority items are mostly polish and documentation gaps that would be fine in a follow-up. Overall this is solid work — the architecture is clean and the test suite is comprehensive for a feature of this scope.