Download logic#4
Conversation
sumanthd032
commented
Feb 12, 2026
sumanthd032
commented
- Added CreateDropRequest/Response models
- Implemented POST /api/v1/drop to initialize upload session
- Implemented POST /api/v1/drop/{id}/chunk to handle binary chunk upload
- Added DB insertion for drops and chunks
- Added S3 upload logic integration"
There was a problem hiding this comment.
Pull request overview
Adds initial download support and completes core “drop” upload flow by introducing metadata models, upload session initialization, chunk upload handling, and new download endpoints backed by Postgres + S3/MinIO.
Changes:
- Added
GetDropMetadataResponsemodel for download metadata. - Implemented upload handlers for creating drops and uploading binary chunks (DB + S3).
- Added download routes + handlers for drop metadata and chunk retrieval.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| internal/api/router.go | Registers new download endpoints alongside existing upload routes. |
| internal/api/models.go | Adds response model for drop metadata used by CLI before downloading. |
| internal/api/handlers_upload.go | Implements drop creation and chunk upload (S3 upload + DB inserts). |
| internal/api/handlers.download.go | Implements metadata fetch (DB) and chunk download (S3) endpoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| http.Error(w, "Drop not found", http.StatusNotFound) | ||
| return | ||
| } |
There was a problem hiding this comment.
QueryRow(...).Scan(...) errors are all mapped to 404. This will incorrectly return "Drop not found" on transient DB errors. Handle sql.ErrNoRows as 404, but return 500 (and ideally log) for other errors.
| // 3. Get chunk count (to know how many pieces to expect) | ||
| err = s.DB.QueryRow("SELECT COUNT(*) FROM chunks WHERE drop_id = $1", dropID).Scan(&resp.ChunkCount) | ||
| if err != nil { | ||
| http.Error(w, "Database error", http.StatusInternalServerError) | ||
| return |
There was a problem hiding this comment.
COUNT(*) in Postgres returns BIGINT; scanning into resp.ChunkCount (int) can cause conversion issues and is platform-dependent. Scan into an int64 variable and then convert (with bounds check) before assigning to the response.
| // 1. CLI sends CreateDropRequest to /api/v1/drops (POST) | ||
| // 2. Server responds with CreateDropResponse (drop ID and expiration time) | ||
| // 3. CLI uploads file chunks to /api/v1/drops/{drop_id}/chunks (POST) with ChunkUploadResponse confirming each chunk | ||
|
|
||
| // GetDropMetadataResponse is what the server sends back when the CLI requests metadata about a drop |
There was a problem hiding this comment.
The upload flow comments still reference /api/v1/drops and /api/v1/drops/{drop_id}/chunks, but the router defines /api/v1/drop and /api/v1/drop/{id}/chunk. Update these comments (or the routes) so the documented API matches the actual endpoints.
| dropID := chi.URLParam(r, "id") | ||
| chunkIndex := chi.URLParam(r, "chunkIndex") // We'll add this param to router | ||
|
|
||
| // 1. Construct S3 Key | ||
| key := fmt.Sprintf("drops/%s/%s", dropID, chunkIndex) |
There was a problem hiding this comment.
Download endpoint builds the S3 key directly from the URL chunkIndex without validating it or checking the drop/chunk state in Postgres (expiry, deletion, max/current downloads, chunk existence). Add validation (e.g. numeric index) and enforce drop expiry/download limits before serving data.
| func (s *Server) handleDownloadChunk() http.HandlerFunc { | ||
| return func(w http.ResponseWriter, r *http.Request) { | ||
| dropID := chi.URLParam(r, "id") | ||
| chunkIndex := chi.URLParam(r, "chunkIndex") // We'll add this param to router |
There was a problem hiding this comment.
The comment says "We'll add this param to router" but the router already defines {chunkIndex}. Update/remove this comment to avoid misleading future changes.
| chunkIndex := chi.URLParam(r, "chunkIndex") // We'll add this param to router | |
| chunkIndex := chi.URLParam(r, "chunkIndex") // chunkIndex is provided as a URL parameter |
| data, err := s.Store.DownloadChunk(key) | ||
| if err != nil { | ||
| http.Error(w, "Chunk not found or storage error", http.StatusNotFound) | ||
| return |
There was a problem hiding this comment.
This maps any DownloadChunk error to 404, which can hide genuine storage/permission/outage errors. Consider returning 404 only for missing objects and 500 for other errors (and/or log the underlying error).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
@sumanthd032 I've opened a new pull request, #5, to work on those changes. Once the pull request is ready, I'll request review from you. |
[WIP] Update download logic implementation based on feedback
