Add attachments module with S3-backed signed upload/download flow#8
Merged
capcom6 merged 1 commit intoApr 23, 2026
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 17 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (28)
📝 WalkthroughWalkthroughAdds an attachments feature: new attachments domain, repository, service and HTTP endpoints (init, confirm, download, delete); MinIO-backed storage module and miniofx client; DB migration for attachments table; config and DI wiring updates; DTOs, OpenAPI updates, and example HTTP requests. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler
participant AttachmentsSvc
participant TasksSvc
participant AttachmentRepo
participant StorageSvc
participant MinIO
participant Database
rect rgba(100,150,200,0.5)
Note over Client,Database: Upload Initialization
Client->>Handler: POST /tasks/{id}/attachments
Handler->>AttachmentsSvc: InitUpload(input, uploaderID)
AttachmentsSvc->>TasksSvc: Exists(taskID)
TasksSvc-->>AttachmentsSvc: true/false
alt task missing
AttachmentsSvc-->>Handler: ErrTaskNotFound
Handler-->>Client: 404
else
AttachmentsSvc->>AttachmentRepo: Create(input, storageKey)
AttachmentRepo->>Database: INSERT attachments (status=pending)
Database-->>AttachmentRepo: record
AttachmentRepo-->>AttachmentsSvc: Attachment
AttachmentsSvc->>StorageSvc: PresignedPutObject(storageKey)
StorageSvc->>MinIO: presign PUT URL
MinIO-->>StorageSvc: URL
StorageSvc-->>AttachmentsSvc: URL
AttachmentsSvc-->>Handler: UploadResult
Handler-->>Client: 201 {id, upload_url}
end
end
rect rgba(150,200,100,0.5)
Note over Client,Database: Confirm Upload
Client->>Handler: PUT /attachments/{id}/confirm
Handler->>AttachmentsSvc: ConfirmUpload(id, uploaderID)
AttachmentsSvc->>AttachmentRepo: GetByID(id)
AttachmentRepo->>Database: SELECT *
Database-->>AttachmentRepo: record
AttachmentRepo-->>AttachmentsSvc: Attachment
AttachmentsSvc->>AttachmentRepo: Confirm(id)
AttachmentRepo->>Database: UPDATE status=uploaded
Database-->>AttachmentRepo: rows_affected
AttachmentsSvc->>StorageSvc: PresignedGetObject(storageKey)
StorageSvc->>MinIO: presign GET URL
MinIO-->>StorageSvc: URL
StorageSvc-->>AttachmentsSvc: URL
AttachmentsSvc-->>Handler: Attachment + downloadURL
Handler-->>Client: 200 {download_url,...}
end
rect rgba(200,150,100,0.5)
Note over Client,Database: Delete Attachment
Client->>Handler: DELETE /attachments/{id}
Handler->>AttachmentsSvc: Delete(user, id)
AttachmentsSvc->>AttachmentRepo: GetByID(id)
AttachmentRepo-->>AttachmentsSvc: Attachment
alt unauthorized
AttachmentsSvc-->>Handler: ErrUnauthorized
Handler-->>Client: 403
else
AttachmentsSvc->>StorageSvc: Delete(storageKey)
StorageSvc->>MinIO: removeObject
MinIO-->>StorageSvc: success
StorageSvc-->>AttachmentsSvc: nil
AttachmentsSvc->>AttachmentRepo: Delete(id)
AttachmentRepo->>Database: DELETE FROM attachments
Database-->>AttachmentRepo: success
AttachmentsSvc-->>Handler: nil
Handler-->>Client: 204
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
internal/attachments/service.go (1)
147-148: Consider deleting the S3 object during attachment delete.The current delete flow only soft-deletes DB metadata. Without object removal, storage costs and retained file data can grow indefinitely.
Suggested change
- return s.attachments.Delete(ctx, id) + if err := s.storage.RemoveObject(ctx, s.config.BucketName, attachment.StorageKey); err != nil { + return fmt.Errorf("failed to remove attachment object: %w", err) + } + return s.attachments.Delete(ctx, id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/attachments/service.go` around lines 147 - 148, The Delete method currently only calls s.attachments.Delete(ctx, id) and leaves the S3 object behind; update it to first load the attachment metadata (e.g., via s.attachments.Get / GetByID or whatever method returns the stored object key), call the storage/S3 client to remove the object (e.g., s.storage.DeleteObject or s.s3Client.DeleteObject with the bucket/key), handle and log any object-deletion errors, and then perform or coordinate the DB delete (or perform DB delete and attempt object deletion with retries/compensation) so metadata and the underlying S3 object are removed consistently; reference the existing Delete function on the attachments repository and the storage/S3 client methods in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@go.mod`:
- Line 77: Run go mod tidy to update module metadata so
github.com/minio/minio-go/v7 is recorded as a direct dependency (remove the //
indirect marker); ensure the change reflects that this module is directly
imported by internal/s3/client.go and internal/s3/module.go by re-running go mod
tidy (or manually edit go.mod to remove the // indirect comment and run go mod
tidy to validate).
In `@internal/attachments/domain.go`:
- Around line 39-50: AttachmentInput.Validate currently sanitizes the filename
for checking but doesn't persist it, and uses a value receiver; change the
receiver to a pointer on AttachmentInput.Validate, assign the sanitized filename
back to i.FileName after checking, and keep the existing validation of TaskID
and UploaderID (ErrValidationFailed) intact so the struct carries the normalized
filename; this will remove divergence with buildStorageKey and models.Create and
ensure the validated/sanitized FileName is persisted.
In `@internal/attachments/repository.go`:
- Around line 67-84: When rows == 0 in Repository.Confirm (the NewUpdate on
attachmentModel setting StatusUploaded where id and StatusPending), don't assume
it's always ErrAlreadyConfirmed: run a quick existence check for that id (e.g.,
SELECT id,status FROM attachments WHERE id = ?) using the same DB interface; if
no row is returned, return a not-found error, otherwise return
ErrAlreadyConfirmed. Update Confirm to perform this existence lookup and map the
two cases accordingly.
In `@internal/attachments/service.go`:
- Around line 80-89: InitUpload currently calls s.attachments.Create (creating a
pending DB row) before calling s.storage.PresignedPutObject, which can leave
orphaned records if presign fails; change the flow to compute storageKey via
s.buildStorageKey, call s.storage.PresignedPutObject first to obtain uploadURL,
and only then persist the attachment with s.attachments.Create (using the same
storageKey) — alternatively, if you prefer to keep Create first, ensure you
delete/rollback the created attachment on any PresignedPutObject error; update
InitUpload to use either "presign then create" or "create then delete on error"
so no orphaned DB rows remain.
- Around line 157-161: ListByTask is currently presigning URLs for every item
without checking item.Status; update ListByTask to enforce the same status gate
used by GetDownloadURL by either (a) calling s.GetDownloadURL(ctx, item.ID) and
using its result for each item so the existing status checks are reused, or (b)
explicitly skip/omit presigning for items whose item.Status is not the allowed
state (e.g., AttachmentStatusUploaded) before invoking
s.storage.PresignedGetObject; reference ListByTask, GetDownloadURL, item.Status,
and s.storage.PresignedGetObject when making the change so the logic matches the
single-item behavior.
In `@internal/config/config.go`:
- Around line 84-85: The config currently sets S3 credentials to the well-known
"minioadmin" defaults (AccessKey, SecretKey); change these runtime defaults to
empty strings and add startup validation in the config load/constructor (e.g.,
in the function that initializes the config or a Validate() method) to fail fast
if S3 is enabled but AccessKey/SecretKey are not provided, or gate these
defaults behind an explicit dev-only flag so they are never used in production;
update any references to AccessKey/SecretKey to rely on the environment-provided
values and emit a clear error message when validation fails.
In `@internal/db/migrations/20260422000000_create_attachments.sql`:
- Line 22: Remove the stray standalone '---' separator that appears between the
goose Up and Down blocks in the listed migration files; locate the offending
line in each file (e.g.,
internal/db/migrations/20260422000000_create_attachments.sql) and delete it so
the next annotation appears immediately after the '-- +goose StatementEnd' line
(annotations must be exactly '-- +goose [directive]' with no extra separator or
whitespace). Ensure this fix is applied to all six migrations mentioned so the
Up/Down directives follow goose syntax precisely.
In `@internal/server/attachments/dto.go`:
- Line 12: The SizeBytes field currently hard-codes max=104857600 which can
diverge from the runtime Config.MaxFileSizeBytes; remove the hard-coded max
constraint from the SizeBytes tag and enforce the limit at runtime using the
actual config (either by adding a Validate method on the DTO that checks
SizeBytes <= Config.MaxFileSizeBytes or by registering a custom validator that
reads Config.MaxFileSizeBytes and applying that validator to SizeBytes). Update
the request/handler validation path (where the DTO is validated) to use this
runtime check so API-layer validation always matches the service configuration.
In `@internal/server/attachments/handler.go`:
- Around line 71-79: ConfirmUpload currently mutates state then GetDownloadURL
can fail, causing a 500 after a successful confirm; make ConfirmUpload
idempotent and update the handler to treat "already confirmed" as success.
Change the attachments service ConfirmUpload implementation to return no error
when the record is already in the confirmed state (or return a sentinel error
like ErrAlreadyConfirmed that callers can treat as success), and update the
handler code around attachmentsSvc.ConfirmUpload and
attachmentsSvc.GetDownloadURL to: call ConfirmUpload, treat
ErrAlreadyConfirmed/no-error as a successful confirm, then attempt
GetDownloadURL but if URL generation fails log the error and return a
success/partial response (or a response without download_url) instead of
returning a 500. Ensure references: ConfirmUpload, GetDownloadURL,
attachmentsSvc, and the handler method in handler.go are updated accordingly.
---
Nitpick comments:
In `@internal/attachments/service.go`:
- Around line 147-148: The Delete method currently only calls
s.attachments.Delete(ctx, id) and leaves the S3 object behind; update it to
first load the attachment metadata (e.g., via s.attachments.Get / GetByID or
whatever method returns the stored object key), call the storage/S3 client to
remove the object (e.g., s.storage.DeleteObject or s.s3Client.DeleteObject with
the bucket/key), handle and log any object-deletion errors, and then perform or
coordinate the DB delete (or perform DB delete and attempt object deletion with
retries/compensation) so metadata and the underlying S3 object are removed
consistently; reference the existing Delete function on the attachments
repository and the storage/S3 client methods in your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 446445ea-2feb-4f00-b9c2-207a6d5fc6b7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
go.modinternal/app.gointernal/attachments/doc.gointernal/attachments/domain.gointernal/attachments/errors.gointernal/attachments/models.gointernal/attachments/module.gointernal/attachments/repository.gointernal/attachments/service.gointernal/config/config.gointernal/db/migrations/20260422000000_create_attachments.sqlinternal/s3/client.gointernal/s3/module.gointernal/server/attachments/dto.gointernal/server/attachments/handler.gointernal/server/module.gointernal/server/tasks/dto.gointernal/server/tasks/handler.go
capcom6
force-pushed
the
codex/design-attachments-module-with-abstracted-fields
branch
from
3178555 to
a92cd9b
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/server/tasks/handler.go (1)
297-312:⚠️ Potential issue | 🟠 MajorMap attachment domain errors to HTTP responses.
New attachment handlers wrap
attachments.ErrNotFound,ErrUnauthorized,ErrValidationFailed,ErrFileTooLarge,ErrTaskNotFound,ErrNotUploaded, andErrAlreadyConfirmed, but this middleware currently falls through todefault, turning expected client/auth errors into 500s.🚦 Proposed mapping
case errors.Is(err, comments.ErrValidationFailed): return fiber.NewError(fiber.StatusBadRequest, err.Error()) + + case errors.Is(err, attachments.ErrNotFound): + return fiber.NewError(fiber.StatusNotFound, err.Error()) + case errors.Is(err, attachments.ErrTaskNotFound): + return fiber.NewError(fiber.StatusNotFound, err.Error()) + case errors.Is(err, attachments.ErrUnauthorized): + return fiber.NewError(fiber.StatusForbidden, err.Error()) + case errors.Is(err, attachments.ErrValidationFailed): + return fiber.NewError(fiber.StatusBadRequest, err.Error()) + case errors.Is(err, attachments.ErrFileTooLarge): + return fiber.NewError(fiber.StatusRequestEntityTooLarge, err.Error()) + case errors.Is(err, attachments.ErrNotUploaded): + return fiber.NewError(fiber.StatusConflict, err.Error()) + case errors.Is(err, attachments.ErrAlreadyConfirmed): + return fiber.NewError(fiber.StatusConflict, err.Error()) default: return err //nolint:wrapcheck // err is already wrapped🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/handler.go` around lines 297 - 312, The error switch in the request error handler currently omits attachment-related errors so attachment handlers' expected client/auth errors become 500s; add errors.Is(...) cases for attachments.ErrNotFound -> return 404, attachments.ErrTaskNotFound -> return 404, attachments.ErrUnauthorized -> return 403, attachments.ErrValidationFailed -> return 400, attachments.ErrFileTooLarge -> return 413 (Payload Too Large), attachments.ErrNotUploaded -> return 400 (Bad Request), and attachments.ErrAlreadyConfirmed -> return 409 (Conflict), using the same pattern as the existing cases (errors.Is(err, ... ) -> fiber.NewError(..., err.Error())) to keep behavior consistent with the tasks/comments mappings.
🧹 Nitpick comments (1)
internal/storage/config.go (1)
5-8: Consider splitting endpoint/credentials/bucket into explicit fields rather than packing into a singleURL.Encoding credentials into a URL (
scheme://key:secret@host/bucket) is common but forces URL-escaping of secrets, leaks secrets into logs if the URL is ever printed, and makes rotation awkward. Given the PR summary lists endpoint, credentials, bucket, TTLs, and max file size as separate settings, consider surfacing them as distinct fields onConfig(e.g.,Endpoint,Region,AccessKey,SecretKey,Bucket,Secure) — this also cleans upminiofx/client.go(see separate comment).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/config.go` around lines 5 - 8, The Config struct currently packs storage connection info into a single URL which force-encodes secrets and leaks them; refactor the internal/storage.Config type to expose explicit fields (e.g., Endpoint, Region, AccessKey, SecretKey, Bucket, Secure, LinksTTL, MaxFileSize) instead of URL, update any code that constructs or reads Config (notably miniofx/client.go and any callers) to consume the new fields, and remove URL parsing/encoding logic there so credentials are passed separately and securely; ensure Config marshaling/unmarshaling and any config-loading code are updated to accept the new keys and add tests to verify behavior remains the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/attachments/service.go`:
- Around line 135-137: The Delete call in internal/attachments/service.go
shadows an existing err variable; inside the block where you call
s.storageSvc.Delete(ctx, attachment.StorageKey) do not redeclare err — rename
the inner variable (e.g., deleteErr or errDel) and return fmt.Errorf("failed to
delete attachment: %w", deleteErr) so you no longer shadow the outer err and
govet warning is resolved; update the variable name consistently in the
enclosing function that handles the Delete result.
- Around line 52-99: Before calling attachments.Confirm in ConfirmUpload, call
s.storageSvc.StatObject(ctx, attachment.StorageKey) to retrieve the actual
object size and compare it to attachment.SizeBytes; if StatObject returns an
error propagate it (wrap with context like "failed to stat object"), and if
sizes differ return a descriptive error (e.g., ErrInvalidUploadSize or a wrapped
fmt.Errorf indicating size mismatch) instead of confirming. Place this check
after retrieving attachment via s.attachments.GetByID and before
s.attachments.Confirm(ctx, id), using the fields/methods: ConfirmUpload,
attachment.StorageKey, attachment.SizeBytes, s.storageSvc.StatObject, and
s.attachments.Confirm.
In `@internal/server/tasks/handler_attachments.go`:
- Around line 60-75: The handler attachmentGetDownloadURL ignores :task_id and
the authenticated user, allowing any auth'd caller to get a presigned URL for
any attachment ID; parse task_id from c.Params("task_id"), validate it
(strconv.ParseInt) and retrieve the user via jwtauth.GetUser(c) (return 400/401
on parse/missing), then call the attachments service with both taskID and
attachmentID (e.g., change or add a method like
attachmentsSvc.GetDownloadURLForTask(ctx, taskID, attachmentID, user) or pass
the user ID to GetDownloadURL) so the service verifies the attachment belongs to
that task and the caller is authorized before returning a presigned URL; keep
the final response shape as AttachmentDownloadResponse and return appropriate
errors (BadRequest/Unauthorized/Forbidden) when validation/authorization fails.
In `@internal/storage/module.go`:
- Around line 16-25: The current fx.Provide(func(c Config) (miniofx.Config,
error)) builds miniofx.Config from u.Query().Get("endpoint") and
u.Query().Get("region") but does not validate them; update this provider to
check that u.Query().Get("endpoint") and u.Query().Get("region") are non-empty
and return a descriptive error (e.g., "invalid storage URL: missing endpoint" /
"missing region") if either is empty so wiring fails fast rather than causing
downstream presign/client errors; keep the existing url.Parse error handling and
return the validated miniofx.Config only when both values are present.
In `@internal/storage/service.go`:
- Around line 41-72: Centralize and normalize object key construction to avoid
leading or double slashes by adding a helper (e.g., normalizeObjectKey or
joinObjectKey) and use it from PresignedPutObject, PresignedGetObject, and
Delete; the helper should trim trailing slashes from s.keyPrefix, trim leading
slashes from the provided key, and concatenate them with a single "/" (or return
the key alone when keyPrefix is empty), then replace the inline
s.keyPrefix+"/"+key uses with calls to that helper throughout the service.
In `@internal/tasks/repository.go`:
- Around line 70-76: Exists currently checks only id and ignores soft-deleted
rows, allowing attachments.Service.InitUpload to attach to deleted tasks; update
Repository.Exists to mirror GetByID by adding the same soft-delete filter (e.g.,
include "deleted_at IS NULL" in the query/Where clause used by
Repository.Exists) so it only returns true for non-deleted tasks while keeping
the existing return/error semantics.
In `@pkg/miniofx/client.go`:
- Around line 10-23: NewClient currently hardcodes Secure: true and uses
credentials.NewEnvAWS(), preventing HTTP/local usage and static credentials;
update the miniofx.Config (and map from storage.Config.URL) to include a Secure
bool (derived from URL scheme or explicit field) and add optional
AccessKey/SecretKey (or a credentials struct) fields, then modify NewClient to
set Secure from Config.Secure and build credentials using
credentials.NewChainCredentials with a static provider when AccessKey/SecretKey
are present (falling back to environment/IAM otherwise) instead of calling
credentials.NewEnvAWS() directly.
In `@requests.http`:
- Around line 329-341: The presigned upload example in requests.http includes a
stale hardcoded Content-Length header (1048576) that contradicts the declared
size_bytes (11358) and the actual ./LICENSE file; remove the explicit
"Content-Length" header from the PUT example (or replace it by programmatically
setting it to the actual file size using the initAttachmentUpload response
size_bytes) so the HTTP client computes the correct length or the header matches
the file being uploaded.
---
Outside diff comments:
In `@internal/server/tasks/handler.go`:
- Around line 297-312: The error switch in the request error handler currently
omits attachment-related errors so attachment handlers' expected client/auth
errors become 500s; add errors.Is(...) cases for attachments.ErrNotFound ->
return 404, attachments.ErrTaskNotFound -> return 404,
attachments.ErrUnauthorized -> return 403, attachments.ErrValidationFailed ->
return 400, attachments.ErrFileTooLarge -> return 413 (Payload Too Large),
attachments.ErrNotUploaded -> return 400 (Bad Request), and
attachments.ErrAlreadyConfirmed -> return 409 (Conflict), using the same pattern
as the existing cases (errors.Is(err, ... ) -> fiber.NewError(..., err.Error()))
to keep behavior consistent with the tasks/comments mappings.
---
Nitpick comments:
In `@internal/storage/config.go`:
- Around line 5-8: The Config struct currently packs storage connection info
into a single URL which force-encodes secrets and leaks them; refactor the
internal/storage.Config type to expose explicit fields (e.g., Endpoint, Region,
AccessKey, SecretKey, Bucket, Secure, LinksTTL, MaxFileSize) instead of URL,
update any code that constructs or reads Config (notably miniofx/client.go and
any callers) to consume the new fields, and remove URL parsing/encoding logic
there so credentials are passed separately and securely; ensure Config
marshaling/unmarshaling and any config-loading code are updated to accept the
new keys and add tests to verify behavior remains the same.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b932370-0311-4ccb-a966-978c23c714f6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
.golangci.ymlgo.modinternal/app.gointernal/attachments/config.gointernal/attachments/doc.gointernal/attachments/domain.gointernal/attachments/errors.gointernal/attachments/models.gointernal/attachments/module.gointernal/attachments/repository.gointernal/attachments/service.gointernal/config/config.gointernal/config/module.gointernal/db/migrations/20260422000000_create_attachments.sqlinternal/server/docs/docs.gointernal/server/tasks/dto.gointernal/server/tasks/dto_attachments.gointernal/server/tasks/handler.gointernal/server/tasks/handler_attachments.gointernal/storage/config.gointernal/storage/module.gointernal/storage/service.gointernal/tasks/repository.gointernal/tasks/service.gopkg/miniofx/client.gopkg/miniofx/config.gopkg/miniofx/module.gorequests.http
✅ Files skipped from review due to trivial changes (7)
- internal/attachments/config.go
- pkg/miniofx/config.go
- internal/attachments/module.go
- internal/attachments/models.go
- internal/attachments/errors.go
- go.mod
- internal/attachments/doc.go
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/app.go
- internal/db/migrations/20260422000000_create_attachments.sql
- internal/attachments/repository.go
- internal/server/tasks/dto.go
- internal/attachments/domain.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
internal/attachments/repository.go (1)
64-80:⚠️ Potential issue | 🟠 MajorMake confirmation conditional and distinguish missing IDs.
Line 78 still maps every zero-row update to
ErrAlreadyConfirmed, and the update is not scoped toStatusPending. Add the pending-status predicate, then check existence/status when no row was updated so missing/deleted attachments returnErrNotFound.Suggested fix
res, err := r.db.NewUpdate(). Model((*attachmentModel)(nil)). Set("status = ?", StatusUploaded). Where("id = ?", id). + Where("status = ?", StatusPending). Exec(ctx) if err != nil { return fmt.Errorf("failed to confirm attachment: %w", err) } @@ if rows == 0 { - return ErrAlreadyConfirmed + var existing attachmentModel + err := r.db.NewSelect(). + Model(&existing). + Column("id", "status"). + Where("id = ?", id). + Scan(ctx) + if errors.Is(err, sql.ErrNoRows) { + return ErrNotFound + } + if err != nil { + return fmt.Errorf("failed to check attachment status: %w", err) + } + if existing.Status == string(StatusUploaded) { + return ErrAlreadyConfirmed + } + return ErrNotFound }Verify affected-row behavior and DSN settings:
#!/bin/bash # Description: Inspect confirmation update semantics and MySQL DSN flags. # Expectation: Confirm uses status-gated updates and differentiates missing vs already-uploaded records. rg -n -C4 '\b(clientFoundRows|RowsAffected|Confirm\s*\(|ErrAlreadyConfirmed|NewUpdate\(\))\b' --type=goBased on learnings,
RowsAffected()returns 0 both when no row matches and when a matching row's values are unchanged by the UPDATE in this MySQL repo.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/attachments/repository.go` around lines 64 - 80, In Confirm, make the UPDATE scoped to pending records by adding the StatusPending predicate to the r.db.NewUpdate() Where clause (so you're updating only rows with StatusPending to StatusUploaded); after executing and calling res.RowsAffected(), if rows == 0 perform a follow-up query against attachmentModel (e.g., SELECT id,status WHERE id = ?) to determine whether the record does not exist (return ErrNotFound) or exists but is already uploaded (return ErrAlreadyConfirmed), rather than always returning ErrAlreadyConfirmed; keep use of r.db.NewUpdate(), Confirm, StatusPending, StatusUploaded, ErrNotFound and ErrAlreadyConfirmed to locate and change the logic.internal/attachments/service.go (1)
81-95:⚠️ Potential issue | 🟠 MajorVerify the uploaded object before marking it uploaded.
SizeBytesis client-declared during init, butConfirmUploadmarks the attachment uploaded without checking that the object exists or that its actual size matches the recorded value. Stat the object beforeConfirm.#!/bin/bash # Description: Check whether upload confirmation verifies object existence/size before Repository.Confirm. # Expectation: ConfirmUpload calls a storage StatObject/metadata method and compares actual size to Attachment.SizeBytes. rg -n -C4 '\b(ConfirmUpload|StatObject|SizeBytes|PresignedPutObject|Confirm\s*\()' --type=go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/attachments/service.go` around lines 81 - 95, ConfirmUpload currently marks an attachment uploaded without verifying the object; modify Service.ConfirmUpload to call the storage layer's stat/metadata method (e.g., s.storage.StatObject or equivalent) after loading the Attachment via s.attachments.GetByID and after the uploaderID check, verify the object exists and that the returned size equals attachment.SizeBytes, and return a clear error (do not call s.attachments.Confirm) if the stat fails or sizes mismatch; if verification passes then call s.attachments.Confirm(ctx, id) and finally return the updated attachment via s.attachments.GetByID.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/attachments/service.go`:
- Around line 98-113: GetDownloadURL currently fetches an attachment and returns
a presigned URL without verifying task binding or user authorization; update the
handler to extract the route task_id and current user and pass both into
Service.GetDownloadURL, then modify Service.GetDownloadURL(signature: include
taskID int64, user *User) to: (1) call s.attachments.GetByID(ctx, id) as before,
(2) check attachment.TaskID equals the passed taskID and return a
not-found/forbidden error if it doesn’t, (3) enforce access policy allowing
admins, the uploader (compare user.ID to attachment.UploaderID), or the task
author (lookup task owner via s.tasks.GetByID or equivalent) and return
ErrForbidden if unauthorized, (4) keep the StatusUploaded check
(StatusUploaded/ErrNotUploaded) and finally call
s.storageSvc.PresignedGetObject(ctx, attachment.StorageKey) to return the URL;
ensure all new error cases return clear, wrapped errors.
In `@internal/storage/service.go`:
- Around line 24-38: NewService must validate the parsed URL and TTL: after
url.Parse and before constructing Service, verify u.Hostname() is not empty and
return a descriptive error if it is, ensure the derived keyPrefix
(strings.TrimPrefix(u.Path, "/")) is acceptable if needed, and validate
config.LinksTTL (or LinksTTL field) is > 0 and return an error if not; update
the NewService constructor to perform these checks and populate presignExpiry
only when valid so that caller receives an immediate error instead of runtime
failures in presigned URL generation.
---
Duplicate comments:
In `@internal/attachments/repository.go`:
- Around line 64-80: In Confirm, make the UPDATE scoped to pending records by
adding the StatusPending predicate to the r.db.NewUpdate() Where clause (so
you're updating only rows with StatusPending to StatusUploaded); after executing
and calling res.RowsAffected(), if rows == 0 perform a follow-up query against
attachmentModel (e.g., SELECT id,status WHERE id = ?) to determine whether the
record does not exist (return ErrNotFound) or exists but is already uploaded
(return ErrAlreadyConfirmed), rather than always returning ErrAlreadyConfirmed;
keep use of r.db.NewUpdate(), Confirm, StatusPending, StatusUploaded,
ErrNotFound and ErrAlreadyConfirmed to locate and change the logic.
In `@internal/attachments/service.go`:
- Around line 81-95: ConfirmUpload currently marks an attachment uploaded
without verifying the object; modify Service.ConfirmUpload to call the storage
layer's stat/metadata method (e.g., s.storage.StatObject or equivalent) after
loading the Attachment via s.attachments.GetByID and after the uploaderID check,
verify the object exists and that the returned size equals attachment.SizeBytes,
and return a clear error (do not call s.attachments.Confirm) if the stat fails
or sizes mismatch; if verification passes then call s.attachments.Confirm(ctx,
id) and finally return the updated attachment via s.attachments.GetByID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ee563be-462a-4c2d-af1a-e28de28f7807
📒 Files selected for processing (7)
internal/attachments/repository.gointernal/attachments/service.gointernal/server/tasks/dto_attachments.gointernal/server/tasks/handler.gointernal/storage/service.gointernal/tasks/repository.gorequests.http
✅ Files skipped from review due to trivial changes (1)
- internal/server/tasks/dto_attachments.go
🚧 Files skipped from review as they are similar to previous changes (2)
- requests.http
- internal/tasks/repository.go
capcom6
force-pushed
the
codex/design-attachments-module-with-abstracted-fields
branch
from
59576d8 to
cc75d74
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
internal/attachments/service.go (2)
81-95:⚠️ Potential issue | 🟠 MajorVerify the uploaded object before marking it uploaded.
SizeBytesis client-declared during init, andConfirmUploadnever checks that the object exists or matches the recorded size. Add a storage metadata/stat call beforeConfirm.Expected flow:
- Load attachment.
- Check uploader.
StatObject(ctx, attachment.StorageKey).- Compare actual size with
attachment.SizeBytes.- Only then call
Repository.Confirm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/attachments/service.go` around lines 81 - 95, ConfirmUpload currently trusts client-reported SizeBytes and marks the DB record without verifying the uploaded object; modify Service.ConfirmUpload to after loading attachment and verifying uploader call StatObject(ctx, attachment.StorageKey) on the storage layer, compare the returned size to attachment.SizeBytes, return an error if they mismatch or the object is missing, and only then call s.attachments.Confirm (Repository.Confirm) and finally reload via s.attachments.GetByID; ensure you use the storage StatObject result for size comparison and propagate any storage errors.
98-113:⚠️ Potential issue | 🔴 CriticalBind download URLs to the task and caller.
GetDownloadURLuses only the attachment ID, so an authenticated caller who knows or guesses an ID can receive a signed URL without task binding or authorization. Mirror the authorization shape used byDeleteand verify the routetaskID.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/attachments/service.go` around lines 98 - 113, GetDownloadURL currently returns a presigned URL based only on attachment ID, allowing unauthorized callers to fetch URLs; change it to mirror Delete by binding to the task and caller: add a taskID parameter (or obtain route taskID), after attachments.GetByID(ctx, id) verify attachment.TaskID == taskID, then call the same authorization check used by Delete (e.g., s.authorizer / s.canAccessTask or whatever method Delete uses) to ensure the caller is allowed for that task, and only then call storageSvc.PresignedGetObject; if the task mismatch or authorization fails, return the same error type Delete returns (e.g., ErrNotFound or ErrForbidden) rather than issuing the URL.internal/storage/service.go (1)
24-38:⚠️ Potential issue | 🟠 MajorValidate storage URL and TTL before constructing the service.
url.Parsecan succeed with an empty hostname, andLinksTTL <= 0leaves every presigned URL call vulnerable to runtime failure. Fail fast inNewService.Proposed fix
func NewService(config Config, client *minio.Client, logger *zap.Logger) (*Service, error) { u, err := url.Parse(config.URL) if err != nil { return nil, fmt.Errorf("failed to parse storage URL: %w", err) } + if u.Hostname() == "" { + return nil, fmt.Errorf("storage URL must include a bucket name") + } + if config.LinksTTL <= 0 { + return nil, fmt.Errorf("storage links TTL must be positive") + } return &Service{ bucketName: u.Hostname(), - keyPrefix: strings.TrimPrefix(u.Path, "/"), + keyPrefix: strings.Trim(u.Path, "/"), presignExpiry: config.LinksTTL,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/storage/service.go` around lines 24 - 38, NewService currently accepts empty/invalid storage URLs and non-positive TTLs; update NewService to validate the parsed URL and TTL: after url.Parse(check u := url.Parse(config.URL)) ensure u.Hostname() is non-empty and u.Path is present (or handle as needed) and verify config.LinksTTL (used for presignExpiry) is > 0, returning a descriptive error if either check fails; mention the symbols NewService, Config, url.Parse, u.Hostname(), LinksTTL, presignExpiry and bucketName so reviewers can find and update the constructor validation.internal/attachments/repository.go (1)
64-80:⚠️ Potential issue | 🟠 MajorMake confirmation a pending→uploaded state transition.
The update only matches
id, so confirming an already-uploaded attachment can still succeed. Addstatus = pending, then disambiguaterows == 0with an existence check.Based on learnings:
RowsAffected()is unreliable by itself for distinguishing missing rows from unchanged updates in this MySQL repository.Proposed fix
res, err := r.db.NewUpdate(). Model((*attachmentModel)(nil)). Set("status = ?", StatusUploaded). Where("id = ?", id). + Where("status = ?", StatusPending). Exec(ctx) @@ if rows == 0 { + exists, existsErr := r.db.NewSelect(). + Model((*attachmentModel)(nil)). + Where("id = ?", id). + Exists(ctx) + if existsErr != nil { + return fmt.Errorf("failed to check attachment existence: %w", existsErr) + } + if !exists { + return ErrNotFound + } return ErrAlreadyConfirmed }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/attachments/repository.go` around lines 64 - 80, Change the update in Repository.Confirm to only match pending rows by adding a WHERE clause for status = StatusPending (so the Set remains "status = ?", StatusUploaded but Where becomes "id = ? AND status = ?", id, StatusPending); after Exec, if RowsAffected() == 0 perform a separate existence check (e.g., SELECT 1 FROM attachments WHERE id = ?) to distinguish "no such attachment" from "already uploaded" and return ErrNotFound when the row doesn't exist and ErrAlreadyConfirmed when it exists but wasn't pending; keep using attachmentModel and the same error wrapping as in the current method.internal/db/migrations/20260422000000_create_attachments.sql (1)
22-22:⚠️ Potential issue | 🔴 CriticalRemove the standalone
---separator.Goose SQL migrations are split by
-- +gooseannotations, not a raw---line. This can make the migration fail to parse.Proposed fix
-- +goose StatementEnd ---- -- +goose DownVerify no migration still contains this separator:
#!/bin/bash # Description: Detect standalone markdown-style separators in SQL migrations. # Expected: No matches. rg -n '^\s*---\s*$' internal/db/migrations🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/db/migrations/20260422000000_create_attachments.sql` at line 22, Remove the standalone markdown-style separator lines consisting solely of '---' from the SQL migration(s), e.g., the migration named 20260422000000_create_attachments.sql; Goose uses '-- +goose' annotations to split sections so delete any '---' lines, ensure migration boundaries use the proper '-- +goose' markers, and run a repository-wide search in internal/db/migrations for lines that are only '---' to verify no other migrations contain that separator.internal/server/tasks/handler_attachments.go (2)
47-55:⚠️ Potential issue | 🟠 MajorAvoid returning failure after a successful confirm state change.
ConfirmUploadcan persist the uploaded state, thenGetDownloadURLcan fail and surface as an error. A retry may then hitErrAlreadyConfirmed/409 even though the original confirm actually succeeded. Consider making confirm idempotent, or return a successful confirm response without a download URL when only URL generation fails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/handler_attachments.go` around lines 47 - 55, ConfirmUpload currently persists the confirmed state then a failure from GetDownloadURL causes the whole request to error, which can leave callers retrying and hitting ErrAlreadyConfirmed; update the handler so ConfirmUpload is treated as the primary success and make the overall response succeed even if GetDownloadURL fails: call h.attachmentsSvc.ConfirmUpload(ctx, id, user.ID) first and on success either (a) make ConfirmUpload idempotent at the service layer (handle ErrAlreadyConfirmed and return success) or (b) return a successful confirm response to the client with a nil/empty download URL and log the GetDownloadURL error instead of returning it; reference the ConfirmUpload and GetDownloadURL calls and ErrAlreadyConfirmed handling when implementing this change.
60-75:⚠️ Potential issue | 🟠 MajorBind download authorization to both
task_idand the caller.This handler ignores
:task_idand discards the authenticated user before callingGetDownloadURL(ctx, id), so the service has no task path context or caller identity to verify before issuing a presigned URL. Parsetask_idand pass task/user context to a task-scoped service method before signing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/handler_attachments.go` around lines 60 - 75, The handler attachmentGetDownloadURL currently ignores the :task_id route param and the authenticated caller when requesting a presigned URL; update it to parse task_id from c.Params("task_id"), retrieve the authenticated user via jwtauth.GetUser(c) (extract user ID or principal), and call a task-scoped service method (e.g., attachmentsSvc.GetDownloadURLForTask or GetDownloadURLWithTaskContext) passing ctx, attachment id, taskID and caller identity so the service can authorize the request before signing; adjust error handling to return BadRequest for invalid task_id and Unauthorized if jwtauth.GetUser fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/attachments/service.go`:
- Around line 131-135: The current order calls s.storageSvc.Delete before
s.attachments.Delete, which can leave a DB record pointing to a missing object;
change the flow to update metadata first (use s.attachments.Delete or a
soft-delete/markDeleted method) and only then attempt s.storageSvc.Delete, and
if s.storageSvc.Delete fails ensure you log the error and enqueue a retry/outbox
job rather than returning a failure that would re-create the dangling-state;
reference s.attachments.Delete and s.storageSvc.Delete to locate and reorder the
operations and add retry/outbox handling for storage deletion.
- Around line 57-62: The current check around s.tasksSvc.Exists conflates a
storage error with a missing task because it returns ErrTaskNotFound when ok is
false even if err is non-nil; change the branching so you first check if err !=
nil and return the wrapped storage error (e.g., fmt.Errorf("failed to check if
task exists: %w", err)), and only if err == nil and ok == false return
ErrTaskNotFound; update the logic around s.tasksSvc.Exists and the
ErrTaskNotFound return accordingly.
---
Duplicate comments:
In `@internal/attachments/repository.go`:
- Around line 64-80: Change the update in Repository.Confirm to only match
pending rows by adding a WHERE clause for status = StatusPending (so the Set
remains "status = ?", StatusUploaded but Where becomes "id = ? AND status = ?",
id, StatusPending); after Exec, if RowsAffected() == 0 perform a separate
existence check (e.g., SELECT 1 FROM attachments WHERE id = ?) to distinguish
"no such attachment" from "already uploaded" and return ErrNotFound when the row
doesn't exist and ErrAlreadyConfirmed when it exists but wasn't pending; keep
using attachmentModel and the same error wrapping as in the current method.
In `@internal/attachments/service.go`:
- Around line 81-95: ConfirmUpload currently trusts client-reported SizeBytes
and marks the DB record without verifying the uploaded object; modify
Service.ConfirmUpload to after loading attachment and verifying uploader call
StatObject(ctx, attachment.StorageKey) on the storage layer, compare the
returned size to attachment.SizeBytes, return an error if they mismatch or the
object is missing, and only then call s.attachments.Confirm (Repository.Confirm)
and finally reload via s.attachments.GetByID; ensure you use the storage
StatObject result for size comparison and propagate any storage errors.
- Around line 98-113: GetDownloadURL currently returns a presigned URL based
only on attachment ID, allowing unauthorized callers to fetch URLs; change it to
mirror Delete by binding to the task and caller: add a taskID parameter (or
obtain route taskID), after attachments.GetByID(ctx, id) verify
attachment.TaskID == taskID, then call the same authorization check used by
Delete (e.g., s.authorizer / s.canAccessTask or whatever method Delete uses) to
ensure the caller is allowed for that task, and only then call
storageSvc.PresignedGetObject; if the task mismatch or authorization fails,
return the same error type Delete returns (e.g., ErrNotFound or ErrForbidden)
rather than issuing the URL.
In `@internal/db/migrations/20260422000000_create_attachments.sql`:
- Line 22: Remove the standalone markdown-style separator lines consisting
solely of '---' from the SQL migration(s), e.g., the migration named
20260422000000_create_attachments.sql; Goose uses '-- +goose' annotations to
split sections so delete any '---' lines, ensure migration boundaries use the
proper '-- +goose' markers, and run a repository-wide search in
internal/db/migrations for lines that are only '---' to verify no other
migrations contain that separator.
In `@internal/server/tasks/handler_attachments.go`:
- Around line 47-55: ConfirmUpload currently persists the confirmed state then a
failure from GetDownloadURL causes the whole request to error, which can leave
callers retrying and hitting ErrAlreadyConfirmed; update the handler so
ConfirmUpload is treated as the primary success and make the overall response
succeed even if GetDownloadURL fails: call h.attachmentsSvc.ConfirmUpload(ctx,
id, user.ID) first and on success either (a) make ConfirmUpload idempotent at
the service layer (handle ErrAlreadyConfirmed and return success) or (b) return
a successful confirm response to the client with a nil/empty download URL and
log the GetDownloadURL error instead of returning it; reference the
ConfirmUpload and GetDownloadURL calls and ErrAlreadyConfirmed handling when
implementing this change.
- Around line 60-75: The handler attachmentGetDownloadURL currently ignores the
:task_id route param and the authenticated caller when requesting a presigned
URL; update it to parse task_id from c.Params("task_id"), retrieve the
authenticated user via jwtauth.GetUser(c) (extract user ID or principal), and
call a task-scoped service method (e.g., attachmentsSvc.GetDownloadURLForTask or
GetDownloadURLWithTaskContext) passing ctx, attachment id, taskID and caller
identity so the service can authorize the request before signing; adjust error
handling to return BadRequest for invalid task_id and Unauthorized if
jwtauth.GetUser fails.
In `@internal/storage/service.go`:
- Around line 24-38: NewService currently accepts empty/invalid storage URLs and
non-positive TTLs; update NewService to validate the parsed URL and TTL: after
url.Parse(check u := url.Parse(config.URL)) ensure u.Hostname() is non-empty and
u.Path is present (or handle as needed) and verify config.LinksTTL (used for
presignExpiry) is > 0, returning a descriptive error if either check fails;
mention the symbols NewService, Config, url.Parse, u.Hostname(), LinksTTL,
presignExpiry and bucketName so reviewers can find and update the constructor
validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be9ee4bf-e130-49f8-b16e-f8c89c7b57cb
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
.golangci.ymlgo.modinternal/app.gointernal/attachments/config.gointernal/attachments/doc.gointernal/attachments/domain.gointernal/attachments/errors.gointernal/attachments/models.gointernal/attachments/module.gointernal/attachments/repository.gointernal/attachments/service.gointernal/config/config.gointernal/config/module.gointernal/db/migrations/20260422000000_create_attachments.sqlinternal/server/docs/docs.gointernal/server/tasks/dto.gointernal/server/tasks/dto_attachments.gointernal/server/tasks/handler.gointernal/server/tasks/handler_attachments.gointernal/storage/config.gointernal/storage/module.gointernal/storage/service.gointernal/tasks/repository.gointernal/tasks/service.gopkg/miniofx/client.gopkg/miniofx/config.gopkg/miniofx/module.gorequests.http
✅ Files skipped from review due to trivial changes (10)
- internal/attachments/config.go
- internal/storage/config.go
- pkg/miniofx/config.go
- internal/attachments/doc.go
- internal/attachments/errors.go
- internal/attachments/module.go
- requests.http
- internal/attachments/models.go
- internal/server/tasks/dto_attachments.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/tasks/service.go
- pkg/miniofx/module.go
- pkg/miniofx/client.go
- internal/app.go
- internal/config/module.go
- internal/server/docs/docs.go
- internal/storage/module.go
- .golangci.yml
capcom6
force-pushed
the
codex/design-attachments-module-with-abstracted-fields
branch
from
e9cd66c to
36e2e14
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
internal/attachments/domain.go (1)
64-66: Hard-coded 255 in error message can drift from theMaxFileNameLengthconstant.If
MaxFileNameLengthis ever changed, the error text will silently become stale. Consider formatting the constant into the message.♻️ Proposed refactor
- if len(fileName) > MaxFileNameLength { - return fmt.Errorf("%w: file_name too long (max 255 characters)", ErrValidationFailed) - } + if len(fileName) > MaxFileNameLength { + return fmt.Errorf("%w: file_name too long (max %d characters)", ErrValidationFailed, MaxFileNameLength) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/attachments/domain.go` around lines 64 - 66, The error message hardcodes "255" and can drift from the MaxFileNameLength constant; update the validation branch (the check using MaxFileNameLength and returning ErrValidationFailed) to include the actual MaxFileNameLength value in the formatted error string (e.g., use fmt.Errorf with %d and MaxFileNameLength) so the message stays consistent with the constant for the validation of fileName.internal/server/tasks/handler_attachments.go (1)
78-94:attachmentDeleteignores:task_idfrom the route path.The route is mounted under
/:task_id/attachments/:idbut the handler never parsestask_idor asserts thatattachment.TaskIDmatches the path. While the service's admin/uploader/task-author policy prevents cross-user abuse, it allows a caller to reference an attachment via a mismatched task path (e.g.,/tasks/42/attachments/99where attachment 99 belongs to task 7). Either validate the binding here (or in the service) or drop:task_idfrom the route if it's not meaningful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/handler_attachments.go` around lines 78 - 94, The handler attachmentDelete ignores the route param :task_id so a caller can delete an attachment through a mismatched task path; update attachmentDelete to parse taskID from c.Params("task_id"), fetch the attachment (or have attachmentsSvc provide a method like GetByID) and assert attachment.TaskID matches the parsed taskID before calling attachmentsSvc.Delete (or add a DeleteByTask( ctx, user, taskID, id ) variant on attachmentsSvc and call that) and return a 404 or 400 if the task IDs do not match.internal/attachments/service.go (2)
123-138:ConfirmUploadissues doubleGetByIDand has no pre-state guard.Two related concerns:
- The method performs two
GetByIDround-trips (Line 124 pre-check, Line 137 re-fetch) when a singleRETURNING *or reusing the already-loaded model and updating its status in memory would suffice.- There is no check that
attachment.Status == StatusPendingbefore callingConfirm. Combined with the repository not filtering by status in its UPDATE, a caller can re-confirm an already-uploaded attachment silently. Consider returningErrAlreadyConfirmedup-front whenattachment.Status != StatusPending.♻️ Proposed refactor
func (s *Service) ConfirmUpload(ctx context.Context, id int64, uploaderID int64) (*Attachment, error) { attachment, err := s.attachments.GetByID(ctx, id) if err != nil { return nil, err } if attachment.UploadedBy != uploaderID { return nil, ErrUnauthorized } + if attachment.Status != StatusPending { + return nil, ErrAlreadyConfirmed + } + if confirmErr := s.attachments.Confirm(ctx, id); confirmErr != nil { return nil, confirmErr } - return s.attachments.GetByID(ctx, id) + attachment.Status = StatusUploaded + return attachment, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/attachments/service.go` around lines 123 - 138, ConfirmUpload currently does two GetByID calls and lacks a pre-state guard; change it to fetch once (using the result from s.attachments.GetByID in ConfirmUpload), check that attachment.Status == StatusPending and return ErrAlreadyConfirmed if not, then either call s.attachments.Confirm implemented to RETURNING * (or, if Confirm does not return the updated record, update attachment.Status = StatusConfirmed in-memory) and return the single updated attachment; ensure you still call s.attachments.Confirm for the DB update but avoid the second GetByID round-trip and add the StatusPending guard before calling s.attachments.Confirm.
89-100: Status filter here is redundant —Repository.ListByTaskalready filters bystatus = uploaded.The repository's
ListByTaskquery hasWhere("status = ?", StatusUploaded), soitem.Status != StatusUploadedcan never be true here. Dead branch — either remove the filter here or drop the filter from the repository (depending on whether you want the service or the repo to enforce the policy).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/attachments/service.go` around lines 89 - 100, The loop in service.go redundantly checks attachment status even though Repository.ListByTask already filters by StatusUploaded; remove the dead branch `if item.Status != StatusUploaded { continue }` from the loop so you only call s.storageSvc.PresignedGetObject(ctx, item.StorageKey) and append AttachmentWithURL for each item, or if you prefer enforcing at service level instead, remove the `Where("status = ?", StatusUploaded)` from Repository.ListByTask—pick one place to enforce the uploaded-only policy (prefer removing the local check for minimal change).internal/server/tasks/handler.go (2)
320-321: Consider413 Payload Too LargeforErrFileTooLarge.
ErrFileTooLargesemantically maps more precisely tofiber.StatusRequestEntityTooLarge(413) than to 400. Minor, but it lets clients distinguish size-limit rejections from generic validation failures.♻️ Proposed fix
case errors.Is(err, attachments.ErrFileTooLarge): - return fiber.NewError(fiber.StatusBadRequest, err.Error()) + return fiber.NewError(fiber.StatusRequestEntityTooLarge, err.Error())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/handler.go` around lines 320 - 321, The handler currently maps attachments.ErrFileTooLarge to a 400 response; change the error mapping in the switch case that checks errors.Is(err, attachments.ErrFileTooLarge) (in internal/server/tasks/handler.go) to return fiber.StatusRequestEntityTooLarge (413) instead of fiber.StatusBadRequest so clients can distinguish payload-too-large errors from generic validation errors.
77-81: RedundanterrorsHandleron POST; missing validation middleware pattern consistency.Line 78 re-registers
h.errorsHandleron the POST route even though it's already attached to the parenttasksgroup at Line 54 (and inherited by this subgroup). It will run twice per request. The siblingcommentsgroup (Line 68) doesn't do this — drop it for consistency.♻️ Proposed fix
- attachments.Post("/", h.errorsHandler, validation.DecorateWithBodyEx(h.Validator, h.attachmentInitUpload)) + attachments.Post("/", validation.DecorateWithBodyEx(h.Validator, h.attachmentInitUpload))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/server/tasks/handler.go` around lines 77 - 81, Remove the redundant middleware registration: the attachments subgroup currently adds h.errorsHandler again on attachments.Post which is already inherited from the parent tasks group, causing it to run twice; modify the POST route so it uses only the validation middleware (validation.DecorateWithBodyEx(h.Validator, h.attachmentInitUpload)) and other handlers (h.attachmentInitUpload) but not h.errorsHandler, matching the pattern used by the comments group and avoiding duplicate middleware execution.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/attachments/repository.go`:
- Around line 64-83: The Confirm method currently updates by id only and
misinterprets RowsAffected() == 0 as ErrAlreadyConfirmed; change Confirm to
update only rows with status = StatusPending (e.g., add Where("status = ?",
StatusPending) in the same query), then read RowsAffected(): if 0, distinguish
whether the row exists (query by id) and return ErrNotFound when the record
doesn't exist or ErrAlreadyConfirmed when the record exists but status is not
pending; keep error wrapping for DB errors and retain use of ErrAlreadyConfirmed
and ErrNotFound as appropriate (also account for clientFoundRows=true behavior).
- Around line 20-28: The Create method uses Returning("*") which is incompatible
with the registered mysqldialect; either remove Returning("*") and explicitly
load the generated fields after insert, or switch the DB dialect to bun's
mariadb implementation so RETURNING is supported. Best fix: change DB
registration from mysqldialect.New() to mariadb.New() (so Repository.Create can
keep Returning("*") and newAttachmentModel will have ID/UploadedAt filled), or
if you prefer to keep mysqldialect, remove Returning("*") from Repository.Create
and after r.db.NewInsert().Model(model).Exec(ctx) perform a follow-up query to
populate generated fields (e.g., select by unique storageKey or by last-insert
id) before calling model.toDomain().
In `@internal/server/tasks/handler_attachments.go`:
- Around line 12-34: attachmentInitUpload currently skips task-level
authorization: before calling h.attachmentsSvc.InitUpload you must verify the
caller has access to the task (same policy used in the Delete handler — e.g.,
author, assignee, project member or admin). After extracting taskID and user
from jwtauth.GetUser, call the existing task-access check (reuse the
authorization helper or service used by Delete, or call the task lookup +
permission check) and return a 403/Unauthorized fiber error if the user is not
permitted; only then call h.attachmentsSvc.InitUpload and proceed to return the
created response.
---
Nitpick comments:
In `@internal/attachments/domain.go`:
- Around line 64-66: The error message hardcodes "255" and can drift from the
MaxFileNameLength constant; update the validation branch (the check using
MaxFileNameLength and returning ErrValidationFailed) to include the actual
MaxFileNameLength value in the formatted error string (e.g., use fmt.Errorf with
%d and MaxFileNameLength) so the message stays consistent with the constant for
the validation of fileName.
In `@internal/attachments/service.go`:
- Around line 123-138: ConfirmUpload currently does two GetByID calls and lacks
a pre-state guard; change it to fetch once (using the result from
s.attachments.GetByID in ConfirmUpload), check that attachment.Status ==
StatusPending and return ErrAlreadyConfirmed if not, then either call
s.attachments.Confirm implemented to RETURNING * (or, if Confirm does not return
the updated record, update attachment.Status = StatusConfirmed in-memory) and
return the single updated attachment; ensure you still call
s.attachments.Confirm for the DB update but avoid the second GetByID round-trip
and add the StatusPending guard before calling s.attachments.Confirm.
- Around line 89-100: The loop in service.go redundantly checks attachment
status even though Repository.ListByTask already filters by StatusUploaded;
remove the dead branch `if item.Status != StatusUploaded { continue }` from the
loop so you only call s.storageSvc.PresignedGetObject(ctx, item.StorageKey) and
append AttachmentWithURL for each item, or if you prefer enforcing at service
level instead, remove the `Where("status = ?", StatusUploaded)` from
Repository.ListByTask—pick one place to enforce the uploaded-only policy (prefer
removing the local check for minimal change).
In `@internal/server/tasks/handler_attachments.go`:
- Around line 78-94: The handler attachmentDelete ignores the route param
:task_id so a caller can delete an attachment through a mismatched task path;
update attachmentDelete to parse taskID from c.Params("task_id"), fetch the
attachment (or have attachmentsSvc provide a method like GetByID) and assert
attachment.TaskID matches the parsed taskID before calling attachmentsSvc.Delete
(or add a DeleteByTask( ctx, user, taskID, id ) variant on attachmentsSvc and
call that) and return a 404 or 400 if the task IDs do not match.
In `@internal/server/tasks/handler.go`:
- Around line 320-321: The handler currently maps attachments.ErrFileTooLarge to
a 400 response; change the error mapping in the switch case that checks
errors.Is(err, attachments.ErrFileTooLarge) (in
internal/server/tasks/handler.go) to return fiber.StatusRequestEntityTooLarge
(413) instead of fiber.StatusBadRequest so clients can distinguish
payload-too-large errors from generic validation errors.
- Around line 77-81: Remove the redundant middleware registration: the
attachments subgroup currently adds h.errorsHandler again on attachments.Post
which is already inherited from the parent tasks group, causing it to run twice;
modify the POST route so it uses only the validation middleware
(validation.DecorateWithBodyEx(h.Validator, h.attachmentInitUpload)) and other
handlers (h.attachmentInitUpload) but not h.errorsHandler, matching the pattern
used by the comments group and avoiding duplicate middleware execution.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05e72b93-88e9-4630-8f75-e72bee09e0f9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
.golangci.ymlgo.modinternal/app.gointernal/attachments/config.gointernal/attachments/doc.gointernal/attachments/domain.gointernal/attachments/errors.gointernal/attachments/models.gointernal/attachments/module.gointernal/attachments/repository.gointernal/attachments/service.gointernal/config/config.gointernal/config/module.gointernal/db/migrations/20260422000000_create_attachments.sqlinternal/server/docs/docs.gointernal/server/tasks/dto.gointernal/server/tasks/dto_attachments.gointernal/server/tasks/handler.gointernal/server/tasks/handler_attachments.gointernal/storage/config.gointernal/storage/module.gointernal/storage/service.gointernal/tasks/repository.gointernal/tasks/service.gopkg/miniofx/client.gopkg/miniofx/config.gopkg/miniofx/module.gorequests.http
✅ Files skipped from review due to trivial changes (13)
- internal/attachments/doc.go
- internal/attachments/config.go
- pkg/miniofx/config.go
- internal/storage/config.go
- internal/attachments/module.go
- internal/server/docs/docs.go
- pkg/miniofx/client.go
- internal/attachments/errors.go
- requests.http
- internal/db/migrations/20260422000000_create_attachments.sql
- go.mod
- internal/server/tasks/dto_attachments.go
- .golangci.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/tasks/service.go
- pkg/miniofx/module.go
- internal/app.go
- internal/config/module.go
- internal/attachments/models.go
- internal/tasks/repository.go
capcom6
force-pushed
the
codex/design-attachments-module-with-abstracted-fields
branch
from
198d31b to
aee82de
Compare
capcom6
deleted the
codex/design-attachments-module-with-abstracted-fields
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
statusto prevent downloads of pending uploads.StorageKey) while preserving DB columns3_keyfor compatibility.Description
internal/attachmentsproviding domain types, validation, error definitions, Bun ORM model, repository (Create, GetByID, ListByTask, Confirm, Delete), and service (InitUpload, ConfirmUpload, GetDownloadURL, Delete, ListByTask). (files:internal/attachments/*.go).internal/s3wrapping MinIO SDK with interface methodsPresignedPutObject,PresignedGetObject,RemoveObject, and FX provider wiring. (files:internal/s3/*.go).internal/server/attachmentswith DTOs and handlers for the 3-step upload flow plus download and delete endpoints, and errors->HTTP mapping. (files:internal/server/attachments/*.go).internal/db/migrations/20260422000000_create_attachments.sqlcreatingattachmentstable withstatus ENUM('pending','uploaded'), soft-delete and FK constraints.s3.Module()andattachments.Module()to startup and injectedattachments.Serviceinto the tasks handler soGET /tasks/{id}now returns attachments with per-request signed download URLs. (files modified:internal/app.go,internal/server/module.go,internal/server/tasks/*,internal/config/config.go).StorageKey(storage-agnostic) while DB columns3_keyremains for compatibility. (file:internal/config/config.go).go.mod/go.sumto support the S3 client implementation.Testing
github.com/minio/minio-go/v7; after thatgo build/go testsetup succeeded.go test ./... -run TestNonExistent -count=0 -timeout 20sto validate packages compile in this environment and observed no test files to run (all packages reported[no test files]).go testagainst new packages) which returned successfully (no failing unit tests present in repository for the new packages in this run).Codex Task
Summary by CodeRabbit
New Features
Database
Configuration
Storage
Tasks
Documentation