feat(rds): automated backups and snapshots for RDS#82
Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces database snapshot and restore functionality by integrating SnapshotService into DatabaseService, adding three new API endpoints for snapshot management and database restoration, implementing snapshot creation/listing logic, and extending the OpenAPI documentation with the new endpoints and request/response types. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as DatabaseHandler
participant Service as DatabaseService
participant SnapSvc as SnapshotService
participant VolSvc as VolumeService
participant Container as Container Orchestration
Client->>Handler: POST /databases/restore<br/>(RestoreDatabaseRequest)
Handler->>Service: RestoreDatabase(snapshotID, name, engine, ...)
Service->>SnapSvc: GetSnapshot(snapshotID)
SnapSvc-->>Service: snapshot details
Service->>VolSvc: RestoreVolume(snapshot)
VolSvc-->>Service: restored volume
Service->>Service: Create database record
Service->>Container: Launch container with restored volume
Container-->>Service: container created
Service->>Service: Update port/status
Service->>Service: Record events & audit
Service-->>Handler: database instance
Handler-->>Client: 201 Created<br/>(domain.Database)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Adds RDS snapshot + restore capabilities by integrating the existing SnapshotService into the managed database workflow and exposing new REST endpoints for snapshot lifecycle and restore.
Changes:
- Added database snapshot creation/listing endpoints and a database restore endpoint.
- Extended
DatabaseServiceto locate the underlying DB volume, create/list snapshots, and restore a DB from a snapshot. - Wired snapshot dependencies into API service initialization and registered new routes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/handlers/database_handler.go | Adds HTTP handlers for create/list snapshots and restore (currently contains a compile-breaking handler bug). |
| internal/core/services/database.go | Implements DB volume resolution + snapshot and restore workflows in the core service. |
| internal/core/ports/database.go | Extends the DatabaseService interface with snapshot/restore methods. |
| internal/api/setup/router.go | Registers /databases/:id/snapshots and /databases/restore routes. |
| internal/api/setup/dependencies.go | Injects SnapshotService/SnapshotRepository into DatabaseService construction. |
| internal/core/services/database_unit_test.go | Adds snapshot service/repo mocks for unit testing. |
| internal/core/services/database_test.go | Updates test service wiring to include new (nil) snapshot deps. |
| fix_deps.sh | Adds an awk-based helper script to patch dependencies.go. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+164
to
216
| func (h *DatabaseHandler) CreateSnapshot(c *gin.Context) { | ||
| databaseID, err := uuid.Parse(c.Param("id")) | ||
| if err != nil { | ||
| httputil.Error(c, errors.New(errors.InvalidInput, invalidDatabaseIDMsg)) | ||
| return | ||
| } | ||
|
|
||
| var req CreateSnapshotRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil && err.Error() != "EOF" { // Allow empty body | ||
| httputil.Error(c, errors.New(errors.InvalidInput, err.Error())) | ||
| return | ||
| } | ||
|
|
||
| snap, err := h.svc.CreateDatabaseSnapshot(c.Request.Context(), databaseID, req.Description) | ||
| if err != nil { | ||
| httputil.Error(c, err) | ||
| return | ||
| } | ||
|
|
||
| httputil.Success(c, http.StatusCreated, snap) | ||
| } | ||
|
|
||
| func (h *DatabaseHandler) ListSnapshots(c *gin.Context) { | ||
| databaseID, err := uuid.Parse(c.Param("id")) | ||
| if err != nil { | ||
| httputil.Error(c, errors.New(errors.InvalidInput, invalidDatabaseIDMsg)) | ||
| return | ||
| } | ||
|
|
||
| snaps, err := h.svc.ListDatabaseSnapshots(c.Request.Context(), databaseID) | ||
| if err != nil { | ||
| httputil.Error(c, err) | ||
| return | ||
| } | ||
|
|
||
| httputil.Success(c, http.StatusOK, snaps) | ||
| } | ||
|
|
||
| func (h *DatabaseHandler) Restore(c *gin.Context) { | ||
| var req RestoreDatabaseRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| httputil.Error(c, errors.New(errors.InvalidInput, err.Error())) | ||
| return | ||
| } | ||
|
|
||
| db, err := h.svc.RestoreDatabase(c.Request.Context(), req.SnapshotID, req.Name, req.Engine, req.Version, req.VpcID, req.AllocatedStorage, req.Parameters) | ||
| if err != nil { | ||
| httputil.Error(c, err) | ||
| return | ||
| } | ||
|
|
||
| httputil.Success(c, http.StatusCreated, db) | ||
| } |
There was a problem hiding this comment.
New snapshot/restore endpoints were added to this handler, and this repo already has comprehensive database_handler_test.go coverage for existing database routes. Please add handler tests for POST /databases/:id/snapshots, GET /databases/:id/snapshots, and POST /databases/restore (success + common error cases) to prevent regressions.
Comment on lines
+528
to
+530
| if allocatedStorage < snap.SizeGB { | ||
| return nil, errors.New(errors.InvalidInput, fmt.Sprintf("allocated storage (%dGB) must be at least the snapshot size (%dGB)", allocatedStorage, snap.SizeGB)) | ||
| } |
There was a problem hiding this comment.
RestoreDatabase validates allocatedStorage against the snapshot size, but later overwrites db.AllocatedStorage with vol.SizeGB. This makes the allocated_storage request parameter effectively ignored (except for validation) and the created DB may not match what the API accepted.
Comment on lines
+562
to
+566
| // If the user requested MORE storage than the snapshot, we might need to resize the volume here. | ||
| // For V1, we assume the restored volume inherits the snapshot size initially, and resizing is handled by volumeSvc. | ||
| // We'll update our internal tracking to what the volume actually is if we couldn't resize. | ||
| db.AllocatedStorage = vol.SizeGB // Ensure DB record matches actual volume | ||
|
|
There was a problem hiding this comment.
If volume resizing isn’t supported yet (there’s no resize API on ports.VolumeService), the restore flow should either (a) drop allocated_storage from the restore request and always restore at snapshot size, or (b) implement/perform a resize and keep db.AllocatedStorage consistent with the requested size. Leaving it as-is is user-visible and confusing.
Comment on lines
+43
to
49
| // CreateDatabaseSnapshot initiates a point-in-time copy of a database's underlying volume. | ||
| CreateDatabaseSnapshot(ctx context.Context, databaseID uuid.UUID, description string) (*domain.Snapshot, error) | ||
| // ListDatabaseSnapshots returns all snapshots belonging to a specific database. | ||
| ListDatabaseSnapshots(ctx context.Context, databaseID uuid.UUID) ([]*domain.Snapshot, error) | ||
| // RestoreDatabase creates a new database instance from an existing snapshot. | ||
| RestoreDatabase(ctx context.Context, snapshotID uuid.UUID, newName, engine, version string, vpcID *uuid.UUID, allocatedStorage int, parameters map[string]string) (*domain.Database, error) | ||
| } |
There was a problem hiding this comment.
Adding methods to ports.DatabaseService requires updating all existing implementations/mocks of this interface. For example, internal/handlers/database_handler_test.go and internal/workers/database_failover_worker_test.go define mockDatabaseService types that currently don’t implement the new snapshot/restore methods, so the test build will fail until they’re updated.
Comment on lines
262
to
272
| snapshotSvc := services.NewSnapshotService(c.Repos.Snapshot, c.Repos.Volume, c.Storage, eventSvc, auditSvc, c.Logger) | ||
| databaseSvc := services.NewDatabaseService(services.DatabaseServiceParams{ | ||
| Repo: c.Repos.Database, | ||
| Compute: c.Compute, | ||
| VpcRepo: c.Repos.Vpc, | ||
| VolumeSvc: volumeSvc, | ||
| SnapshotSvc: snapshotSvc, | ||
| SnapshotRepo: c.Repos.Snapshot, | ||
| EventSvc: eventSvc, | ||
| AuditSvc: auditSvc, | ||
| Logger: c.Logger, |
There was a problem hiding this comment.
This block has inconsistent indentation (spaces vs tabs) and will need gofmt to keep the file consistent and avoid noisy diffs. Please run gofmt on this file before merging.
Suggested change
| snapshotSvc := services.NewSnapshotService(c.Repos.Snapshot, c.Repos.Volume, c.Storage, eventSvc, auditSvc, c.Logger) | |
| databaseSvc := services.NewDatabaseService(services.DatabaseServiceParams{ | |
| Repo: c.Repos.Database, | |
| Compute: c.Compute, | |
| VpcRepo: c.Repos.Vpc, | |
| VolumeSvc: volumeSvc, | |
| SnapshotSvc: snapshotSvc, | |
| SnapshotRepo: c.Repos.Snapshot, | |
| EventSvc: eventSvc, | |
| AuditSvc: auditSvc, | |
| Logger: c.Logger, | |
| snapshotSvc := services.NewSnapshotService(c.Repos.Snapshot, c.Repos.Volume, c.Storage, eventSvc, auditSvc, c.Logger) | |
| databaseSvc := services.NewDatabaseService(services.DatabaseServiceParams{ | |
| Repo: c.Repos.Database, | |
| Compute: c.Compute, | |
| VpcRepo: c.Repos.Vpc, | |
| VolumeSvc: volumeSvc, | |
| SnapshotSvc: snapshotSvc, | |
| SnapshotRepo: c.Repos.Snapshot, | |
| EventSvc: eventSvc, | |
| AuditSvc: auditSvc, | |
| Logger: c.Logger, |
Comment on lines
+171
to
+173
| var req CreateSnapshotRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil && err.Error() != "EOF" { // Allow empty body | ||
| httputil.Error(c, errors.New(errors.InvalidInput, err.Error())) |
There was a problem hiding this comment.
Checking err.Error() != "EOF" to allow an empty body is brittle (error strings aren’t stable). Prefer checking errors.Is(err, io.EOF) (and import io) or use Gin’s binding behavior in a way that explicitly treats empty body as valid.
Comment on lines
+474
to
+623
| func (s *DatabaseService) CreateDatabaseSnapshot(ctx context.Context, databaseID uuid.UUID, description string) (*domain.Snapshot, error) { | ||
| db, err := s.repo.GetByID(ctx, databaseID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if db.Status != domain.DatabaseStatusRunning && db.Status != domain.DatabaseStatusStopped { | ||
| return nil, errors.New(errors.InvalidInput, "database must be running or stopped to create a snapshot") | ||
| } | ||
|
|
||
| vol, err := s.getVolumeForDatabase(ctx, db) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| snapshotName := fmt.Sprintf("db-snap-%s-%s", db.Name, time.Now().Format("20060102150405")) | ||
| if description != "" { | ||
| snapshotName = fmt.Sprintf("%s-%s", snapshotName, description) | ||
| } | ||
|
|
||
| snap, err := s.snapshotSvc.CreateSnapshot(ctx, vol.ID, snapshotName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| _ = s.auditSvc.Log(ctx, db.UserID, "database.snapshot.create", "database", db.ID.String(), map[string]interface{}{ | ||
| "snapshot_id": snap.ID, | ||
| }) | ||
|
|
||
| return snap, nil | ||
| } | ||
|
|
||
| func (s *DatabaseService) ListDatabaseSnapshots(ctx context.Context, databaseID uuid.UUID) ([]*domain.Snapshot, error) { | ||
| db, err := s.repo.GetByID(ctx, databaseID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| vol, err := s.getVolumeForDatabase(ctx, db) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return s.snapshotRepo.ListByVolumeID(ctx, vol.ID) | ||
| } | ||
|
|
||
| func (s *DatabaseService) RestoreDatabase(ctx context.Context, snapshotID uuid.UUID, newName, engine, version string, vpcID *uuid.UUID, allocatedStorage int, parameters map[string]string) (*domain.Database, error) { | ||
| userID := appcontext.UserIDFromContext(ctx) | ||
|
|
||
| snap, err := s.snapshotSvc.GetSnapshot(ctx, snapshotID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if allocatedStorage < snap.SizeGB { | ||
| return nil, errors.New(errors.InvalidInput, fmt.Sprintf("allocated storage (%dGB) must be at least the snapshot size (%dGB)", allocatedStorage, snap.SizeGB)) | ||
| } | ||
|
|
||
| dbEngine := domain.DatabaseEngine(engine) | ||
| if !s.isValidEngine(dbEngine) { | ||
| return nil, errors.New(errors.InvalidInput, "unsupported database engine") | ||
| } | ||
|
|
||
| password, err := util.GenerateRandomPassword(16) | ||
| if err != nil { | ||
| return nil, errors.Wrap(errors.Internal, "failed to generate password", err) | ||
| } | ||
|
|
||
| username := s.getDefaultUsername(dbEngine) | ||
| db := s.initialDatabaseRecord(userID, newName, dbEngine, version, username, password, vpcID) | ||
| db.Role = domain.RolePrimary | ||
| db.AllocatedStorage = allocatedStorage | ||
| db.Parameters = parameters | ||
|
|
||
| imageName, env, defaultPort := s.getEngineConfig(dbEngine, version, username, password, newName, db.Role, "") | ||
|
|
||
| networkID, err := s.resolveVpcNetwork(ctx, vpcID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Create persistent volume FOR THE NEW DATABASE from the snapshot | ||
| volName := fmt.Sprintf("db-vol-%s", db.ID.String()[:8]) | ||
| vol, err := s.snapshotSvc.RestoreSnapshot(ctx, snapshotID, volName) | ||
| if err != nil { | ||
| return nil, errors.Wrap(errors.Internal, "failed to restore volume from snapshot", err) | ||
| } | ||
|
|
||
| // If the user requested MORE storage than the snapshot, we might need to resize the volume here. | ||
| // For V1, we assume the restored volume inherits the snapshot size initially, and resizing is handled by volumeSvc. | ||
| // We'll update our internal tracking to what the volume actually is if we couldn't resize. | ||
| db.AllocatedStorage = vol.SizeGB // Ensure DB record matches actual volume | ||
|
|
||
| backendVolName := "thecloud-vol-" + vol.ID.String()[:8] | ||
| if vol.BackendPath != "" { | ||
| backendVolName = vol.BackendPath | ||
| } | ||
|
|
||
| mountPath := "/var/lib/postgresql/data" | ||
| if dbEngine == domain.EngineMySQL { | ||
| mountPath = "/var/lib/mysql" | ||
| } | ||
| volumeBinds := []string{fmt.Sprintf("%s:%s", backendVolName, mountPath)} | ||
|
|
||
| cmd := s.buildEngineCmd(dbEngine, parameters) | ||
|
|
||
| dockerName := fmt.Sprintf("cloud-db-%s-%s", newName, db.ID.String()[:8]) | ||
| containerID, allocatedPorts, err := s.compute.LaunchInstanceWithOptions(ctx, ports.CreateInstanceOptions{ | ||
| Name: dockerName, | ||
| ImageName: imageName, | ||
| Ports: []string{"0:" + defaultPort}, | ||
| NetworkID: networkID, | ||
| VolumeBinds: volumeBinds, | ||
| Env: env, | ||
| Cmd: cmd, | ||
| }) | ||
|
|
||
| if err != nil { | ||
| _ = s.volumeSvc.DeleteVolume(ctx, vol.ID.String()) | ||
| s.logger.Error("failed to launch restored database container", "error", err) | ||
| return nil, errors.Wrap(errors.Internal, "failed to launch restored database container", err) | ||
| } | ||
|
|
||
| hostPort, err := s.parseAllocatedPort(allocatedPorts, defaultPort) | ||
| if err != nil || hostPort == 0 { | ||
| hostPort, _ = s.compute.GetInstancePort(ctx, containerID, defaultPort) | ||
| } | ||
|
|
||
| db.ContainerID = containerID | ||
| db.Port = hostPort | ||
| db.Status = domain.DatabaseStatusRunning | ||
|
|
||
| if err := s.repo.Create(ctx, db); err != nil { | ||
| _ = s.compute.DeleteInstance(ctx, containerID) | ||
| _ = s.volumeSvc.DeleteVolume(ctx, vol.ID.String()) | ||
| return nil, err | ||
| } | ||
|
|
||
| _ = s.eventSvc.RecordEvent(ctx, "DATABASE_RESTORE", db.ID.String(), "DATABASE", map[string]interface{}{ | ||
| "snapshot_id": snapshotID, | ||
| }) | ||
|
|
||
| _ = s.auditSvc.Log(ctx, userID, "database.restore", "database", db.ID.String(), map[string]interface{}{ | ||
| "snapshot_id": snapshotID, | ||
| }) | ||
|
|
||
| platform.RDSInstancesTotal.WithLabelValues(engine, "running").Inc() | ||
|
|
||
| return db, nil | ||
| } |
There was a problem hiding this comment.
These new snapshot/restore service methods introduce substantial new behavior, but there are no unit tests exercising them (this repo already has database_unit_test.go for the existing DatabaseService methods). Please add tests for snapshot creation/listing and restore (including error/cleanup paths).
Comment on lines
+2
to
+17
| awk ' | ||
| /databaseSvc := services.NewDatabaseService/ { | ||
| print " snapshotSvc := services.NewSnapshotService(c.Repos.Snapshot, c.Repos.Volume, c.Storage, eventSvc, auditSvc, c.Logger)" | ||
| print $0 | ||
| next | ||
| } | ||
| /VolumeSvc: volumeSvc,/ { | ||
| print $0 | ||
| print " SnapshotSvc: snapshotSvc," | ||
| print " SnapshotRepo: c.Repos.Snapshot," | ||
| next | ||
| } | ||
| /snapshotSvc := services.NewSnapshotService/ { | ||
| # If we encounter the original snapshotSvc initialization (which is now after), skip it | ||
| if (NR > 280) next | ||
| } |
There was a problem hiding this comment.
fix_deps.sh looks like a one-off codemod helper for this specific refactor (it hard-codes patterns and even an NR > 280 line-number condition). This is brittle and likely to become stale; consider removing it from the repo (or moving it under scripts/ with a clearer purpose and without line-number assumptions).
| return args.Get(0).(*domain.Snapshot), args.Error(1) | ||
| } | ||
| func (m *mockSnapshotService) ListSnapshots(ctx context.Context) ([]*domain.Snapshot, error) { | ||
| args := m.Called(ctx) |
There was a problem hiding this comment.
In this mock, ListSnapshots will panic if the test sets a nil return (it does a direct type assertion without a nil guard). Other mocks in the repo typically return nil, err when args.Get(0) == nil; please match that pattern for safety.
Suggested change
| args := m.Called(ctx) | |
| args := m.Called(ctx) | |
| if args.Get(0) == nil { return nil, args.Error(1) } |
| return args.Get(0).(*domain.Snapshot), args.Error(1) | ||
| } | ||
| func (m *mockSnapshotRepository) ListByVolumeID(ctx context.Context, volumeID uuid.UUID) ([]*domain.Snapshot, error) { | ||
| args := m.Called(ctx, volumeID) |
There was a problem hiding this comment.
Same issue here: ListByVolumeID directly type-asserts args.Get(0) and will panic if the mock is set up to return nil. Add a nil guard (consistent with the other repository mocks in this file) to make the mock safer to use.
Suggested change
| args := m.Called(ctx, volumeID) | |
| args := m.Called(ctx, volumeID) | |
| if args.Get(0) == nil { | |
| return nil, args.Error(1) | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
internal/core/services/database_unit_test.go (1)
55-237:⚠️ Potential issue | 🟠 MajorAdd unit coverage for snapshot database operations.
You wired snapshot dependencies but there are no unit cases for
CreateDatabaseSnapshot,ListDatabaseSnapshots, orRestoreDatabasein this suite.As per coding guidelines, "Do not skip tests for new services".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/database_unit_test.go` around lines 55 - 237, The test suite is missing unit tests for snapshot-related methods; add table tests covering CreateDatabaseSnapshot, ListDatabaseSnapshots, and RestoreDatabase using the wired mocks (mockSnapshotService, mockSnapshotRepository) and existing mocks (mockRepo, mockVolumeSvc, mockCompute) to simulate success and failure flows: for CreateDatabaseSnapshot mock SnapshotSvc.CreateSnapshot (or method name used) to return a snapshot and assert returned value and event/audit calls and error handling; for ListDatabaseSnapshots mock SnapshotRepo.List (or SnapshotSvc.ListSnapshots) to return a slice and assert results; for RestoreDatabase mock SnapshotSvc.Restore (or RestoreSnapshot) plus repo/compute/volume interactions to assert successful restore and rollback on failures. Ensure each test sets expectations with mock.On(...).Return(...).Once(), invokes svc.CreateDatabaseSnapshot / svc.ListDatabaseSnapshots / svc.RestoreDatabase, and asserts results and that error paths trigger appropriate cleanup and event/audit calls.internal/handlers/database_handler.go (1)
135-164:⚠️ Potential issue | 🔴 CriticalClose
GetConnectionStringand add success response before declaring new types (build breaker).The
GetConnectionStringfunction (line 135) is missing its closing brace and success response. Type declarations at line 149 are placed inside the function body, preventing compilation.Required fix
connStr, err := h.svc.GetConnectionString(c.Request.Context(), id) if err != nil { httputil.Error(c, err) return } + httputil.Success(c, http.StatusOK, gin.H{"connection_string": connStr}) +} // CreateSnapshotRequest is the payload for creating a database snapshot. type CreateSnapshotRequest struct {Additionally, add required Swagger documentation to the
CreateSnapshothandler method, and addomitemptytags to optional JSON fields:RestoreDatabaseRequest.VpcID,RestoreDatabaseRequest.Parameters, andCreateSnapshotRequest.Description.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/database_handler.go` around lines 135 - 164, GetConnectionString is missing its closing brace and a success response: close the function after writing the success HTTP response (e.g., write connStr with httputil.Success or c.JSON) and then add the final `}` before the type declarations so they are not inside the function; add Swagger annotations/comments above the CreateSnapshot handler (function CreateSnapshot) per project conventions; and update the struct tags to mark optional fields as omitempty for CreateSnapshotRequest.Description and RestoreDatabaseRequest.VpcID and RestoreDatabaseRequest.Parameters so they become `json:"description,omitempty"`, `json:"vpc_id,omitempty"`, and `json:"parameters,omitempty"` respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fix_deps.sh`:
- Around line 3-17: The rewrite currently inserts snapshotSvc wiring
unconditionally and uses a line-number guard (NR > 280); change it to be
idempotent by removing the NR check and instead test for existing insertion
patterns before printing: detect existing "snapshotSvc :=
services.NewSnapshotService" and "SnapshotSvc: snapshotSvc" (or "SnapshotSvc:
snapshotSvc," exact text) and only perform the prints when those patterns are
not already present; keep the existing match anchors (databaseSvc :=
services.NewDatabaseService, /VolumeSvc: volumeSvc,/, /snapshotSvc :=
services.NewSnapshotService/) but add conditional checks in the awk script to
skip insertion if either the initialization or the struct wiring already exists
so repeated runs do not duplicate lines.
In `@internal/core/services/database_unit_test.go`:
- Around line 247-250: The mockSnapshotService.ListSnapshots currently does
args.Get(0).([]*domain.Snapshot) which panics when the mock returns (nil, err);
change it to check the interface returned by args.Get(0) for nil before casting
(e.g., v := args.Get(0); if v == nil { return nil, args.Error(1) }; return
v.([]*domain.Snapshot), args.Error(1)); apply the same nil-guard/cast pattern to
the other mockSnapshotService mock methods that return []*domain.Snapshot in
this file so they safely return (nil, err) without a panic.
In `@internal/core/services/database.go`:
- Around line 520-535: In RestoreDatabase, before accepting requested
engine/version, fetch the snapshot (snapshotSvc.GetSnapshot already used) and
validate that the requested engine and version match the snapshot's origin
(e.g., compare domain.DatabaseEngine(engine) and version against snap.Engine and
snap.Version or equivalent fields); if they differ, return an
errors.InvalidInput error explaining the mismatch; add the check after
retrieving snap and before calling s.isValidEngine so incompatible restores are
rejected (optionally add/rename a helper like
s.isCompatibleWithSnapshot(snapshot, dbEngine, version) if you prefer
encapsulation).
- Around line 499-501: The audit call currently discards errors ("_ =
s.auditSvc.Log(...)") in the snapshot/restore paths; capture the returned error
from s.auditSvc.Log in the snapshot creation/restore functions, check it, and
handle it explicitly (e.g., log with s.logger.Errorf including context like
database ID, snapshot ID and operation, and/or return or wrap the error when the
operation is critical). Apply the same pattern to other discarded calls in the
nearby ranges (e.g., any cleanup/event calls in the 592-618 region): replace
blank-identifier assignments with error variables, perform nil checks, and
handle/log/propagate the error with clear contextual messages instead of
silently ignoring it.
- Around line 562-566: The current restore path unconditionally sets
db.AllocatedStorage = vol.SizeGB, which silently overwrites a caller-requested
larger allocation; change the logic so that if db.AllocatedStorage (the
requested size) > vol.SizeGB we do NOT overwrite it with vol.SizeGB but instead
retain the requested value and either trigger a resize via volumeSvc (e.g., call
volumeSvc.ResizeVolume/EnqueueResize) or mark the DB record with a pending
resize flag; only overwrite db.AllocatedStorage with vol.SizeGB when the
requested value is nil/zero or <= vol.SizeGB. Ensure you adjust the restore flow
in the same function in database.go and touch volumeSvc integration points
(ResizeVolume or a pending resize marker) so the eventual resize is handled.
- Around line 597-600: The code currently falls back to compute.GetInstancePort
but swallows its error and allows hostPort to remain 0; update the logic in the
block around parseAllocatedPort(...) and compute.GetInstancePort(...) to check
and return/propagate the error when compute.GetInstancePort fails (or when the
resolved hostPort == 0) instead of ignoring it—specifically, after calling
s.parseAllocatedPort(...) handle the non-nil err case by calling
s.compute.GetInstancePort(ctx, containerID, defaultPort) and if that call
returns an error or a zero port, return or surface the error (do not assign
hostPort=0 and continue), so any caller creating the DB (using hostPort) cannot
persist an invalid port.
In `@internal/handlers/database_handler.go`:
- Around line 164-216: Each handler (CreateSnapshot, ListSnapshots, Restore) is
missing method-level Swagger comments; add a Go comment block immediately above
each function containing `@Summary`, `@Tags` (e.g., Databases), `@Security` (your API
auth scheme), and `@Router` with the correct path and HTTP verb used by your
router so the docs generate consistently; ensure the comments reference the
handler request/response models (e.g., CreateSnapshotRequest,
RestoreDatabaseRequest) and include status codes (e.g., 201 for
CreateSnapshot/Restore, 200 for ListSnapshots) as appropriate.
- Around line 149-161: The JSON tags for optional fields need to include
`omitempty`; update CreateSnapshotRequest.Description's tag to
`json:"description,omitempty"` and update RestoreDatabaseRequest.VpcID and
RestoreDatabaseRequest.Parameters to `json:"vpc_id,omitempty"` and
`json:"parameters,omitempty"` respectively so optional fields are omitted when
empty and conform to the API struct tagging convention.
---
Outside diff comments:
In `@internal/core/services/database_unit_test.go`:
- Around line 55-237: The test suite is missing unit tests for snapshot-related
methods; add table tests covering CreateDatabaseSnapshot, ListDatabaseSnapshots,
and RestoreDatabase using the wired mocks (mockSnapshotService,
mockSnapshotRepository) and existing mocks (mockRepo, mockVolumeSvc,
mockCompute) to simulate success and failure flows: for CreateDatabaseSnapshot
mock SnapshotSvc.CreateSnapshot (or method name used) to return a snapshot and
assert returned value and event/audit calls and error handling; for
ListDatabaseSnapshots mock SnapshotRepo.List (or SnapshotSvc.ListSnapshots) to
return a slice and assert results; for RestoreDatabase mock SnapshotSvc.Restore
(or RestoreSnapshot) plus repo/compute/volume interactions to assert successful
restore and rollback on failures. Ensure each test sets expectations with
mock.On(...).Return(...).Once(), invokes svc.CreateDatabaseSnapshot /
svc.ListDatabaseSnapshots / svc.RestoreDatabase, and asserts results and that
error paths trigger appropriate cleanup and event/audit calls.
In `@internal/handlers/database_handler.go`:
- Around line 135-164: GetConnectionString is missing its closing brace and a
success response: close the function after writing the success HTTP response
(e.g., write connStr with httputil.Success or c.JSON) and then add the final `}`
before the type declarations so they are not inside the function; add Swagger
annotations/comments above the CreateSnapshot handler (function CreateSnapshot)
per project conventions; and update the struct tags to mark optional fields as
omitempty for CreateSnapshotRequest.Description and RestoreDatabaseRequest.VpcID
and RestoreDatabaseRequest.Parameters so they become
`json:"description,omitempty"`, `json:"vpc_id,omitempty"`, and
`json:"parameters,omitempty"` respectively.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
fix_deps.shinternal/api/setup/dependencies.gointernal/api/setup/router.gointernal/core/ports/database.gointernal/core/services/database.gointernal/core/services/database_test.gointernal/core/services/database_unit_test.gointernal/handlers/database_handler.go
Comment on lines
+247
to
+250
| func (m *mockSnapshotService) ListSnapshots(ctx context.Context) ([]*domain.Snapshot, error) { | ||
| args := m.Called(ctx) | ||
| return args.Get(0).([]*domain.Snapshot), args.Error(1) | ||
| } |
There was a problem hiding this comment.
Guard nil returns in snapshot list mocks to avoid panic in error-path tests.
args.Get(0).([]*domain.Snapshot) will panic when a test returns (nil, err).
🧪 Safer mock return handling
func (m *mockSnapshotService) ListSnapshots(ctx context.Context) ([]*domain.Snapshot, error) {
args := m.Called(ctx)
- return args.Get(0).([]*domain.Snapshot), args.Error(1)
+ if args.Get(0) == nil {
+ return nil, args.Error(1)
+ }
+ return args.Get(0).([]*domain.Snapshot), args.Error(1)
}
@@
func (m *mockSnapshotRepository) ListByVolumeID(ctx context.Context, volumeID uuid.UUID) ([]*domain.Snapshot, error) {
args := m.Called(ctx, volumeID)
- return args.Get(0).([]*domain.Snapshot), args.Error(1)
+ if args.Get(0) == nil {
+ return nil, args.Error(1)
+ }
+ return args.Get(0).([]*domain.Snapshot), args.Error(1)
}
func (m *mockSnapshotRepository) ListByUserID(ctx context.Context, userID uuid.UUID) ([]*domain.Snapshot, error) {
args := m.Called(ctx, userID)
- return args.Get(0).([]*domain.Snapshot), args.Error(1)
+ if args.Get(0) == nil {
+ return nil, args.Error(1)
+ }
+ return args.Get(0).([]*domain.Snapshot), args.Error(1)
}Also applies to: 274-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/database_unit_test.go` around lines 247 - 250, The
mockSnapshotService.ListSnapshots currently does
args.Get(0).([]*domain.Snapshot) which panics when the mock returns (nil, err);
change it to check the interface returned by args.Get(0) for nil before casting
(e.g., v := args.Get(0); if v == nil { return nil, args.Error(1) }; return
v.([]*domain.Snapshot), args.Error(1)); apply the same nil-guard/cast pattern to
the other mockSnapshotService mock methods that return []*domain.Snapshot in
this file so they safely return (nil, err) without a panic.
Comment on lines
+499
to
+501
| _ = s.auditSvc.Log(ctx, db.UserID, "database.snapshot.create", "database", db.ID.String(), map[string]interface{}{ | ||
| "snapshot_id": snap.ID, | ||
| }) |
There was a problem hiding this comment.
Replace _ = ... drops with explicit error handling in snapshot/restore paths.
Ignoring cleanup/audit/event errors hides leaks and operational failures in critical lifecycle operations.
🧭 Suggested explicit handling pattern
- _ = s.auditSvc.Log(ctx, db.UserID, "database.snapshot.create", "database", db.ID.String(), map[string]interface{}{
+ if err := s.auditSvc.Log(ctx, db.UserID, "database.snapshot.create", "database", db.ID.String(), map[string]interface{}{
"snapshot_id": snap.ID,
- })
+ }); err != nil {
+ s.logger.Warn("failed to write snapshot audit log", "database_id", db.ID, "snapshot_id", snap.ID, "error", err)
+ }
@@
- _ = s.volumeSvc.DeleteVolume(ctx, vol.ID.String())
+ if delErr := s.volumeSvc.DeleteVolume(ctx, vol.ID.String()); delErr != nil {
+ s.logger.Warn("failed to clean up restored volume", "volume_id", vol.ID, "error", delErr)
+ }
@@
- _ = s.compute.DeleteInstance(ctx, containerID)
- _ = s.volumeSvc.DeleteVolume(ctx, vol.ID.String())
+ if delErr := s.compute.DeleteInstance(ctx, containerID); delErr != nil {
+ s.logger.Warn("failed to clean up restored container after repo failure", "container_id", containerID, "error", delErr)
+ }
+ if delErr := s.volumeSvc.DeleteVolume(ctx, vol.ID.String()); delErr != nil {
+ s.logger.Warn("failed to clean up restored volume after repo failure", "volume_id", vol.ID, "error", delErr)
+ }
return nil, err
}
- _ = s.eventSvc.RecordEvent(ctx, "DATABASE_RESTORE", db.ID.String(), "DATABASE", map[string]interface{}{
+ if err := s.eventSvc.RecordEvent(ctx, "DATABASE_RESTORE", db.ID.String(), "DATABASE", map[string]interface{}{
"snapshot_id": snapshotID,
- })
+ }); err != nil {
+ s.logger.Warn("failed to record restore event", "database_id", db.ID, "snapshot_id", snapshotID, "error", err)
+ }
- _ = s.auditSvc.Log(ctx, userID, "database.restore", "database", db.ID.String(), map[string]interface{}{
+ if err := s.auditSvc.Log(ctx, userID, "database.restore", "database", db.ID.String(), map[string]interface{}{
"snapshot_id": snapshotID,
- })
+ }); err != nil {
+ s.logger.Warn("failed to write restore audit log", "database_id", db.ID, "snapshot_id", snapshotID, "error", err)
+ }As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like _ = someFunc()".
Also applies to: 592-618
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/database.go` around lines 499 - 501, The audit call
currently discards errors ("_ = s.auditSvc.Log(...)") in the snapshot/restore
paths; capture the returned error from s.auditSvc.Log in the snapshot
creation/restore functions, check it, and handle it explicitly (e.g., log with
s.logger.Errorf including context like database ID, snapshot ID and operation,
and/or return or wrap the error when the operation is critical). Apply the same
pattern to other discarded calls in the nearby ranges (e.g., any cleanup/event
calls in the 592-618 region): replace blank-identifier assignments with error
variables, perform nil checks, and handle/log/propagate the error with clear
contextual messages instead of silently ignoring it.
Comment on lines
+520
to
+535
| func (s *DatabaseService) RestoreDatabase(ctx context.Context, snapshotID uuid.UUID, newName, engine, version string, vpcID *uuid.UUID, allocatedStorage int, parameters map[string]string) (*domain.Database, error) { | ||
| userID := appcontext.UserIDFromContext(ctx) | ||
|
|
||
| snap, err := s.snapshotSvc.GetSnapshot(ctx, snapshotID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if allocatedStorage < snap.SizeGB { | ||
| return nil, errors.New(errors.InvalidInput, fmt.Sprintf("allocated storage (%dGB) must be at least the snapshot size (%dGB)", allocatedStorage, snap.SizeGB)) | ||
| } | ||
|
|
||
| dbEngine := domain.DatabaseEngine(engine) | ||
| if !s.isValidEngine(dbEngine) { | ||
| return nil, errors.New(errors.InvalidInput, "unsupported database engine") | ||
| } |
There was a problem hiding this comment.
Validate snapshot compatibility with requested engine/version before restore.
Restore currently accepts any engine/version from request without cross-checking snapshot origin, which can start an incompatible container on restored data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/database.go` around lines 520 - 535, In
RestoreDatabase, before accepting requested engine/version, fetch the snapshot
(snapshotSvc.GetSnapshot already used) and validate that the requested engine
and version match the snapshot's origin (e.g., compare
domain.DatabaseEngine(engine) and version against snap.Engine and snap.Version
or equivalent fields); if they differ, return an errors.InvalidInput error
explaining the mismatch; add the check after retrieving snap and before calling
s.isValidEngine so incompatible restores are rejected (optionally add/rename a
helper like s.isCompatibleWithSnapshot(snapshot, dbEngine, version) if you
prefer encapsulation).
Comment on lines
+562
to
+566
| // If the user requested MORE storage than the snapshot, we might need to resize the volume here. | ||
| // For V1, we assume the restored volume inherits the snapshot size initially, and resizing is handled by volumeSvc. | ||
| // We'll update our internal tracking to what the volume actually is if we couldn't resize. | ||
| db.AllocatedStorage = vol.SizeGB // Ensure DB record matches actual volume | ||
|
|
There was a problem hiding this comment.
Do not silently override requested allocatedStorage during restore.
db.AllocatedStorage is reset to vol.SizeGB even when the caller requested a larger size, so the API accepts one value and persists another.
📏 Safer behavior until resize is implemented
- // If the user requested MORE storage than the snapshot, we might need to resize the volume here.
- // For V1, we assume the restored volume inherits the snapshot size initially, and resizing is handled by volumeSvc.
- // We'll update our internal tracking to what the volume actually is if we couldn't resize.
- db.AllocatedStorage = vol.SizeGB // Ensure DB record matches actual volume
+ // Resize is not implemented in this path yet; keep contract explicit.
+ if vol.SizeGB != allocatedStorage {
+ return nil, errors.New(errors.InvalidInput, "allocated storage resize during restore is not supported yet")
+ }
+ db.AllocatedStorage = allocatedStorage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/database.go` around lines 562 - 566, The current
restore path unconditionally sets db.AllocatedStorage = vol.SizeGB, which
silently overwrites a caller-requested larger allocation; change the logic so
that if db.AllocatedStorage (the requested size) > vol.SizeGB we do NOT
overwrite it with vol.SizeGB but instead retain the requested value and either
trigger a resize via volumeSvc (e.g., call volumeSvc.ResizeVolume/EnqueueResize)
or mark the DB record with a pending resize flag; only overwrite
db.AllocatedStorage with vol.SizeGB when the requested value is nil/zero or <=
vol.SizeGB. Ensure you adjust the restore flow in the same function in
database.go and touch volumeSvc integration points (ResizeVolume or a pending
resize marker) so the eventual resize is handled.
Comment on lines
+597
to
+600
| hostPort, err := s.parseAllocatedPort(allocatedPorts, defaultPort) | ||
| if err != nil || hostPort == 0 { | ||
| hostPort, _ = s.compute.GetInstancePort(ctx, containerID, defaultPort) | ||
| } |
There was a problem hiding this comment.
Handle restore port resolution failures instead of continuing with Port=0.
The fallback ignores GetInstancePort errors and can persist a running DB with an invalid port.
🔌 Suggested error-safe port resolution
hostPort, err := s.parseAllocatedPort(allocatedPorts, defaultPort)
if err != nil || hostPort == 0 {
- hostPort, _ = s.compute.GetInstancePort(ctx, containerID, defaultPort)
+ hostPort, err = s.compute.GetInstancePort(ctx, containerID, defaultPort)
+ if err != nil || hostPort == 0 {
+ if delErr := s.compute.DeleteInstance(ctx, containerID); delErr != nil {
+ s.logger.Warn("failed to clean up restored container after port resolution failure", "container_id", containerID, "error", delErr)
+ }
+ if delErr := s.volumeSvc.DeleteVolume(ctx, vol.ID.String()); delErr != nil {
+ s.logger.Warn("failed to clean up restored volume after port resolution failure", "volume_id", vol.ID, "error", delErr)
+ }
+ return nil, errors.Wrap(errors.Internal, "failed to resolve restored database port", err)
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| hostPort, err := s.parseAllocatedPort(allocatedPorts, defaultPort) | |
| if err != nil || hostPort == 0 { | |
| hostPort, _ = s.compute.GetInstancePort(ctx, containerID, defaultPort) | |
| } | |
| hostPort, err := s.parseAllocatedPort(allocatedPorts, defaultPort) | |
| if err != nil || hostPort == 0 { | |
| hostPort, err = s.compute.GetInstancePort(ctx, containerID, defaultPort) | |
| if err != nil || hostPort == 0 { | |
| if delErr := s.compute.DeleteInstance(ctx, containerID); delErr != nil { | |
| s.logger.Warn("failed to clean up restored container after port resolution failure", "container_id", containerID, "error", delErr) | |
| } | |
| if delErr := s.volumeSvc.DeleteVolume(ctx, vol.ID.String()); delErr != nil { | |
| s.logger.Warn("failed to clean up restored volume after port resolution failure", "volume_id", vol.ID, "error", delErr) | |
| } | |
| return nil, errors.Wrap(errors.Internal, "failed to resolve restored database port", err) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/database.go` around lines 597 - 600, The code
currently falls back to compute.GetInstancePort but swallows its error and
allows hostPort to remain 0; update the logic in the block around
parseAllocatedPort(...) and compute.GetInstancePort(...) to check and
return/propagate the error when compute.GetInstancePort fails (or when the
resolved hostPort == 0) instead of ignoring it—specifically, after calling
s.parseAllocatedPort(...) handle the non-nil err case by calling
s.compute.GetInstancePort(ctx, containerID, defaultPort) and if that call
returns an error or a zero port, return or surface the error (do not assign
hostPort=0 and continue), so any caller creating the DB (using hostPort) cannot
persist an invalid port.
Comment on lines
+149
to
+161
| type CreateSnapshotRequest struct { | ||
| Description string `json:"description"` | ||
| } | ||
|
|
||
| // RestoreDatabaseRequest is the payload for restoring a database from a snapshot. | ||
| type RestoreDatabaseRequest struct { | ||
| SnapshotID uuid.UUID `json:"snapshot_id" binding:"required"` | ||
| Name string `json:"name" binding:"required"` | ||
| Engine string `json:"engine" binding:"required"` | ||
| Version string `json:"version" binding:"required"` | ||
| VpcID *uuid.UUID `json:"vpc_id"` | ||
| AllocatedStorage int `json:"allocated_storage" binding:"required"` | ||
| Parameters map[string]string `json:"parameters"` |
There was a problem hiding this comment.
Use omitempty on optional request fields.
Optional fields in the new request payloads should include omitempty (Description, VpcID, Parameters) to follow API struct tagging conventions.
🏷️ Suggested tag adjustments
type CreateSnapshotRequest struct {
- Description string `json:"description"`
+ Description string `json:"description,omitempty"`
}
@@
type RestoreDatabaseRequest struct {
SnapshotID uuid.UUID `json:"snapshot_id" binding:"required"`
Name string `json:"name" binding:"required"`
Engine string `json:"engine" binding:"required"`
Version string `json:"version" binding:"required"`
- VpcID *uuid.UUID `json:"vpc_id"`
+ VpcID *uuid.UUID `json:"vpc_id,omitempty"`
AllocatedStorage int `json:"allocated_storage" binding:"required"`
- Parameters map[string]string `json:"parameters"`
+ Parameters map[string]string `json:"parameters,omitempty"`
}As per coding guidelines, "Include json tags on struct fields with omitempty for optional fields".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| type CreateSnapshotRequest struct { | |
| Description string `json:"description"` | |
| } | |
| // RestoreDatabaseRequest is the payload for restoring a database from a snapshot. | |
| type RestoreDatabaseRequest struct { | |
| SnapshotID uuid.UUID `json:"snapshot_id" binding:"required"` | |
| Name string `json:"name" binding:"required"` | |
| Engine string `json:"engine" binding:"required"` | |
| Version string `json:"version" binding:"required"` | |
| VpcID *uuid.UUID `json:"vpc_id"` | |
| AllocatedStorage int `json:"allocated_storage" binding:"required"` | |
| Parameters map[string]string `json:"parameters"` | |
| type CreateSnapshotRequest struct { | |
| Description string `json:"description,omitempty"` | |
| } | |
| // RestoreDatabaseRequest is the payload for restoring a database from a snapshot. | |
| type RestoreDatabaseRequest struct { | |
| SnapshotID uuid.UUID `json:"snapshot_id" binding:"required"` | |
| Name string `json:"name" binding:"required"` | |
| Engine string `json:"engine" binding:"required"` | |
| Version string `json:"version" binding:"required"` | |
| VpcID *uuid.UUID `json:"vpc_id,omitempty"` | |
| AllocatedStorage int `json:"allocated_storage" binding:"required"` | |
| Parameters map[string]string `json:"parameters,omitempty"` | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/database_handler.go` around lines 149 - 161, The JSON tags
for optional fields need to include `omitempty`; update
CreateSnapshotRequest.Description's tag to `json:"description,omitempty"` and
update RestoreDatabaseRequest.VpcID and RestoreDatabaseRequest.Parameters to
`json:"vpc_id,omitempty"` and `json:"parameters,omitempty"` respectively so
optional fields are omitted when empty and conform to the API struct tagging
convention.
poyrazK
force-pushed
the
feature/rds-backups
branch
from
984cd4a to
88ec9d8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 84 out of 118 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (2)
pkg/crypto/presign_test.go:10
- This import block isn't
gofmt-formatted (stdlib imports should be grouped together and separated from third-party imports, and ordering should be gofmt-sorted). Runninggofmton this file (or the full Go package) will fix the grouping/order and helps avoid CI/lint failures.
pkg/audit/simple_audit_test.go:15 - The import block is not
gofmt-formatted: a third-party import (testify/require) is mixed into the stdlib group. Please rungofmtso stdlib imports are grouped/sorted separately from external imports.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+185
to
+189
| var req CreateDatabaseSnapshotRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil && err.Error() != "EOF" { // Allow empty body | ||
| httputil.Error(c, errors.New(errors.InvalidInput, err.Error())) | ||
| return | ||
| } |
There was a problem hiding this comment.
CreateSnapshot allows an empty body by checking err.Error() != "EOF", which is brittle (string comparison) and may break if Gin/binding changes the error text. Prefer checking errors.Is(err, io.EOF) (or c.Request.ContentLength == 0) to reliably detect an empty body.
Comment on lines
+460
to
+466
| expectedPrefix := fmt.Sprintf("db-vol-%s", db.ID.String()[:8]) | ||
| if db.Role == domain.RoleReplica { | ||
| expectedPrefix = fmt.Sprintf("db-replica-vol-%s", db.ID.String()[:8]) | ||
| } | ||
|
|
||
| for _, v := range vols { | ||
| if strings.HasPrefix(v.Name, expectedPrefix) { |
There was a problem hiding this comment.
getVolumeForDatabase picks the first volume whose name starts with the expected DB prefix. Because DB volumes are created with exact names (db-vol-<id8> / db-replica-vol-<id8>), using HasPrefix can accidentally match unrelated user volumes (e.g., db-vol-<id8>-old) and snapshot/restore the wrong underlying data. Prefer an exact name match, or persist the volume ID on the database record and look it up directly.
Suggested change
| expectedPrefix := fmt.Sprintf("db-vol-%s", db.ID.String()[:8]) | |
| if db.Role == domain.RoleReplica { | |
| expectedPrefix = fmt.Sprintf("db-replica-vol-%s", db.ID.String()[:8]) | |
| } | |
| for _, v := range vols { | |
| if strings.HasPrefix(v.Name, expectedPrefix) { | |
| expectedName := fmt.Sprintf("db-vol-%s", db.ID.String()[:8]) | |
| if db.Role == domain.RoleReplica { | |
| expectedName = fmt.Sprintf("db-replica-vol-%s", db.ID.String()[:8]) | |
| } | |
| for _, v := range vols { | |
| if v.Name == expectedName { |
Comment on lines
+562
to
+565
| // If the user requested MORE storage than the snapshot, we might need to resize the volume here. | ||
| // For V1, we assume the restored volume inherits the snapshot size initially, and resizing is handled by volumeSvc. | ||
| // We'll update our internal tracking to what the volume actually is if we couldn't resize. | ||
| db.AllocatedStorage = vol.SizeGB // Ensure DB record matches actual volume |
There was a problem hiding this comment.
RestoreDatabase accepts allocatedStorage and validates it is >= snapshot size, but then immediately overwrites db.AllocatedStorage with vol.SizeGB (the snapshot size). This means callers cannot actually request a larger size even though the API requires the field. Either (a) implement volume expansion after restore, (b) reject requests where allocatedStorage != snapshot size, or (c) make allocated_storage optional/ignored and document that behavior explicitly.
Suggested change
| // If the user requested MORE storage than the snapshot, we might need to resize the volume here. | |
| // For V1, we assume the restored volume inherits the snapshot size initially, and resizing is handled by volumeSvc. | |
| // We'll update our internal tracking to what the volume actually is if we couldn't resize. | |
| db.AllocatedStorage = vol.SizeGB // Ensure DB record matches actual volume | |
| // For now, restoring from a snapshot requires that the requested allocated storage | |
| // matches the snapshot size exactly. We do not yet support expanding the volume | |
| // as part of the restore operation. | |
| if db.AllocatedStorage != 0 && db.AllocatedStorage != vol.SizeGB { | |
| return nil, errors.Wrap( | |
| errors.Validation, | |
| "allocated storage must equal snapshot size when restoring from a snapshot", | |
| fmt.Errorf("requested=%dGiB snapshot=%dGiB", db.AllocatedStorage, vol.SizeGB), | |
| ) | |
| } |
Comment on lines
47
to
59
| // NewDatabaseService constructs a DatabaseService with its dependencies. | ||
| func NewDatabaseService(params DatabaseServiceParams) *DatabaseService { | ||
| return &DatabaseService{ | ||
| repo: params.Repo, | ||
| compute: params.Compute, | ||
| vpcRepo: params.VpcRepo, | ||
| volumeSvc: params.VolumeSvc, | ||
| eventSvc: params.EventSvc, | ||
| auditSvc: params.AuditSvc, | ||
| logger: params.Logger, | ||
| repo: params.Repo, | ||
| compute: params.Compute, | ||
| vpcRepo: params.VpcRepo, | ||
| volumeSvc: params.VolumeSvc, | ||
| snapshotSvc: params.SnapshotSvc, | ||
| snapshotRepo: params.SnapshotRepo, | ||
| eventSvc: params.EventSvc, | ||
| auditSvc: params.AuditSvc, | ||
| logger: params.Logger, | ||
| } |
There was a problem hiding this comment.
NewDatabaseService now accepts SnapshotSvc/SnapshotRepo, but it doesn't validate they are non-nil. If the service is instantiated without these deps (e.g., existing benchmarks/integration setups already pass nil) and the new snapshot/restore methods are invoked, it will panic with a nil dereference. Consider enforcing non-nil in the constructor (fail fast) or returning a typed NotImplemented/Internal error from snapshot-related methods when deps are nil.
Comment on lines
63
to
73
| svc := services.NewDatabaseService(services.DatabaseServiceParams{ | ||
| Repo: mockRepo, | ||
| Compute: mockCompute, | ||
| VpcRepo: mockVpcRepo, | ||
| VolumeSvc: mockVolumeSvc, | ||
| EventSvc: mockEventSvc, | ||
| AuditSvc: mockAuditSvc, | ||
| Logger: slog.Default(), | ||
| Repo: mockRepo, | ||
| Compute: mockCompute, | ||
| VpcRepo: mockVpcRepo, | ||
| VolumeSvc: mockVolumeSvc, | ||
| SnapshotSvc: new(mockSnapshotService), | ||
| SnapshotRepo: new(mockSnapshotRepository), | ||
| EventSvc: mockEventSvc, | ||
| AuditSvc: mockAuditSvc, | ||
| Logger: slog.Default(), | ||
| }) |
There was a problem hiding this comment.
DatabaseService gained snapshot/restore functionality, but database_unit_test.go doesn't add any unit tests for CreateDatabaseSnapshot, ListDatabaseSnapshots, or RestoreDatabase (only new mocks were introduced). Adding tests around volume resolution, snapshot size validation, and cleanup-on-failure would help lock in the new behavior.
Comment on lines
+489
to
+500
| snapshotName := fmt.Sprintf("db-snap-%s-%s", db.Name, time.Now().Format("20060102150405")) | ||
| if description != "" { | ||
| snapshotName = fmt.Sprintf("%s-%s", snapshotName, description) | ||
| } | ||
|
|
||
| snap, err := s.snapshotSvc.CreateSnapshot(ctx, vol.ID, snapshotName) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| _ = s.auditSvc.Log(ctx, db.UserID, "database.snapshot.create", "database", db.ID.String(), map[string]interface{}{ | ||
| "snapshot_id": snap.ID, |
There was a problem hiding this comment.
CreateDatabaseSnapshot builds snapshotName from db name + timestamp + optional user description, then passes it to SnapshotService.CreateSnapshot as the snapshot description. This makes the stored description noisy and mixes system-generated metadata with user-provided text (and can become very long/unstructured). Consider passing just the user-provided description through to CreateSnapshot and, if you need a generated display name, store it in a dedicated field (or keep the timestamped name in audit/event metadata instead).
Suggested change
| snapshotName := fmt.Sprintf("db-snap-%s-%s", db.Name, time.Now().Format("20060102150405")) | |
| if description != "" { | |
| snapshotName = fmt.Sprintf("%s-%s", snapshotName, description) | |
| } | |
| snap, err := s.snapshotSvc.CreateSnapshot(ctx, vol.ID, snapshotName) | |
| if err != nil { | |
| return nil, err | |
| } | |
| _ = s.auditSvc.Log(ctx, db.UserID, "database.snapshot.create", "database", db.ID.String(), map[string]interface{}{ | |
| "snapshot_id": snap.ID, | |
| // Generate a structured name for auditing/metadata purposes, but | |
| // pass only the user-provided description to the snapshot service. | |
| generatedName := fmt.Sprintf("db-snap-%s-%s", db.Name, time.Now().Format("20060102150405")) | |
| if description != "" { | |
| generatedName = fmt.Sprintf("%s-%s", generatedName, description) | |
| } | |
| snap, err := s.snapshotSvc.CreateSnapshot(ctx, vol.ID, description) | |
| if err != nil { | |
| return nil, err | |
| } | |
| _ = s.auditSvc.Log(ctx, db.UserID, "database.snapshot.create", "database", db.ID.String(), map[string]interface{}{ | |
| "snapshot_id": snap.ID, | |
| "generated_name": generatedName, | |
| "user_description": description, |
Comment on lines
+167
to
+248
| // CreateSnapshot creates a point-in-time backup of the database. | ||
| // @Summary Create database snapshot | ||
| // @Description Creates a point-in-time backup of the database underlying volume | ||
| // @Tags databases | ||
| // @Accept json | ||
| // @Produce json | ||
| // @Security APIKeyAuth | ||
| // @Param id path string true "Database ID" | ||
| // @Param request body CreateDatabaseSnapshotRequest false "Snapshot description" | ||
| // @Success 201 {object} domain.Snapshot | ||
| // @Router /databases/{id}/snapshots [post] | ||
| func (h *DatabaseHandler) CreateSnapshot(c *gin.Context) { | ||
| databaseID, err := uuid.Parse(c.Param("id")) | ||
| if err != nil { | ||
| httputil.Error(c, errors.New(errors.InvalidInput, invalidDatabaseIDMsg)) | ||
| return | ||
| } | ||
|
|
||
| var req CreateDatabaseSnapshotRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil && err.Error() != "EOF" { // Allow empty body | ||
| httputil.Error(c, errors.New(errors.InvalidInput, err.Error())) | ||
| return | ||
| } | ||
|
|
||
| snap, err := h.svc.CreateDatabaseSnapshot(c.Request.Context(), databaseID, req.Description) | ||
| if err != nil { | ||
| httputil.Error(c, err) | ||
| return | ||
| } | ||
|
|
||
| httputil.Success(c, http.StatusCreated, snap) | ||
| } | ||
|
|
||
| // ListSnapshots returns all snapshots for a database. | ||
| // @Summary List database snapshots | ||
| // @Description Returns all snapshots belonging to a specific database | ||
| // @Tags databases | ||
| // @Produce json | ||
| // @Security APIKeyAuth | ||
| // @Param id path string true "Database ID" | ||
| // @Success 200 {array} domain.Snapshot | ||
| // @Router /databases/{id}/snapshots [get] | ||
| func (h *DatabaseHandler) ListSnapshots(c *gin.Context) { | ||
| databaseID, err := uuid.Parse(c.Param("id")) | ||
| if err != nil { | ||
| httputil.Error(c, errors.New(errors.InvalidInput, invalidDatabaseIDMsg)) | ||
| return | ||
| } | ||
|
|
||
| snaps, err := h.svc.ListDatabaseSnapshots(c.Request.Context(), databaseID) | ||
| if err != nil { | ||
| httputil.Error(c, err) | ||
| return | ||
| } | ||
|
|
||
| httputil.Success(c, http.StatusOK, snaps) | ||
| } | ||
|
|
||
| // Restore provisions a new database from a snapshot. | ||
| // @Summary Restore database from snapshot | ||
| // @Description Creates a new database instance from an existing volume snapshot | ||
| // @Tags databases | ||
| // @Accept json | ||
| // @Produce json | ||
| // @Security APIKeyAuth | ||
| // @Param request body RestoreDatabaseRequest true "Restore parameters" | ||
| // @Success 201 {object} domain.Database | ||
| // @Router /databases/restore [post] | ||
| func (h *DatabaseHandler) Restore(c *gin.Context) { | ||
| var req RestoreDatabaseRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| httputil.Error(c, errors.New(errors.InvalidInput, err.Error())) | ||
| return | ||
| } | ||
|
|
||
| db, err := h.svc.RestoreDatabase(c.Request.Context(), req.SnapshotID, req.Name, req.Engine, req.Version, req.VpcID, req.AllocatedStorage, req.Parameters) | ||
| if err != nil { | ||
| httputil.Error(c, err) | ||
| return | ||
| } | ||
|
|
||
| httputil.Success(c, http.StatusCreated, db) |
There was a problem hiding this comment.
New snapshot/restore endpoints are added here, but internal/handlers/database_handler_test.go only updates the mock interface and doesn't add coverage for: (1) snapshot creation with/without body, (2) listing snapshots, and (3) restore request validation + service invocation. Please add handler tests for these new routes/handlers to prevent regressions in request binding and error handling.
Comment on lines
3
to
9
| import ( | ||
| "github.com/stretchr/testify/require" | ||
| "os" | ||
| "testing" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| ) |
There was a problem hiding this comment.
Import ordering here is not gofmt-compliant (third-party testify/require should come after stdlib imports, separated by a blank line). Please run gofmt to keep import grouping consistent with the rest of the repo and avoid formatting/lint diffs.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/repositories/postgres/cluster_repo.go (1)
213-214:⚠️ Potential issue | 🟠 MajorReturn a NotFound error instead of (nil, nil) on missing cluster.
Line 214 returns
(nil, nil)when no row exists, which silently treats "not found" as success and creates ambiguous API semantics. Callers are forced to defensively check for nil (see cluster_worker.go:88), but this violates the coding guideline: "Do not use silent failures."This pattern is inconsistent with the codebase majority—most repositories (VPC, Volume, User, Subnet, etc.) properly return
errors.New(errors.NotFound, "..."). The same issue exists in pipeline_repo.go (lines 289, 336, 378, 405), queue_repo.go (line 66), and tenant_repo.go:135 (GetMembership).🔧 Proposed fix
if err != nil { if stdlib_errors.Is(err, pgx.ErrNoRows) { - return nil, nil + return nil, errors.New(errors.NotFound, "cluster not found") } return nil, errors.Wrap(errors.Internal, "failed to scan cluster", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repositories/postgres/cluster_repo.go` around lines 213 - 214, The branch that checks stdlib_errors.Is(err, pgx.ErrNoRows) should return a NotFound error instead of (nil, nil); replace the current return nil, nil in that conditional with a call that constructs and returns a NotFound error (e.g. errors.New(errors.NotFound, "cluster not found") or the codebase's equivalent) so callers don’t have to defensively treat nil as "not found"; apply the same change to the analogous stdlib_errors.Is(err, pgx.ErrNoRows) cases in pipeline_repo.go (lines noted), queue_repo.go, and tenant_repo.go:GetMembership to keep behavior consistent across repositories.internal/api/setup/router.go (1)
459-472:⚠️ Potential issue | 🟠 MajorAdd tenant middleware to the
/databasesroute group for consistency and proper tenant isolation.The
/databasesgroup uses onlyhttputil.Auth(svcs.Identity, svcs.Tenant)but is missinghttputil.RequireTenant()andhttputil.TenantMember(svcs.Tenant)middleware that are consistently applied to all similar tenant-scoped resource groups:
instanceGroup(line 262)volumeGroup(line 451)storageGroup(line 411)vpcGroup(line 330)sshKeyGroup(line 276)eipGroup(line 374)peeringGroup(line 385)logGroup(line 648)Database resources are tenant-owned and managed via RBAC permissions (
PermissionDBCreate,PermissionDBRead,PermissionDBDelete). Without the tenant-scoping middleware, database operations may not properly validate tenant membership, creating a potential security gap.Update line 460 to:
dbGroup.Use(httputil.Auth(svcs.Identity, svcs.Tenant), httputil.RequireTenant(), httputil.TenantMember(svcs.Tenant))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/setup/router.go` around lines 459 - 472, The dbGroup route is missing tenant-scoping middleware; update the dbGroup.Use call (where dbGroup is defined) to include httputil.RequireTenant() and httputil.TenantMember(svcs.Tenant) in addition to the existing httputil.Auth(svcs.Identity, svcs.Tenant) so that the line becomes a single dbGroup.Use invocation with httputil.Auth(...), httputil.RequireTenant(), httputil.TenantMember(svcs.Tenant) to ensure tenant isolation for handlers like handlers.Database.Create, Restore, List, Get, Delete, CreateReplica, Promote, CreateSnapshot and ListSnapshots.
♻️ Duplicate comments (5)
internal/handlers/database_handler.go (1)
152-165:⚠️ Potential issue | 🟡 MinorAdd
omitemptyto optional request fields.Line 153 (
Description), Line 162 (VpcID), and Line 164 (Parameters) are optional but currently always serialized.Suggested diff
type CreateDatabaseSnapshotRequest struct { - Description string `json:"description"` + Description string `json:"description,omitempty"` } @@ type RestoreDatabaseRequest struct { SnapshotID uuid.UUID `json:"snapshot_id" binding:"required"` Name string `json:"name" binding:"required"` Engine string `json:"engine" binding:"required"` Version string `json:"version" binding:"required"` - VpcID *uuid.UUID `json:"vpc_id"` + VpcID *uuid.UUID `json:"vpc_id,omitempty"` AllocatedStorage int `json:"allocated_storage" binding:"required"` - Parameters map[string]string `json:"parameters"` + Parameters map[string]string `json:"parameters,omitempty"` }As per coding guidelines, "Include
jsontags on struct fields withomitemptyfor optional fields".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/handlers/database_handler.go` around lines 152 - 165, The JSON tags for optional fields are missing omitempty: update the struct tags so CreateDatabaseSnapshotRequest.Description uses `json:"description,omitempty"`, and in RestoreDatabaseRequest add `omitempty` to VpcID and Parameters (i.e., `VpcID *uuid.UUID `json:"vpc_id,omitempty"` and `Parameters map[string]string `json:"parameters,omitempty"`); modify the tags on the identified fields (Description, VpcID, Parameters) in the CreateDatabaseSnapshotRequest and RestoreDatabaseRequest structs accordingly so optional fields are omitted when empty.internal/core/services/database.go (4)
597-600:⚠️ Potential issue | 🟠 MajorFail restore when fallback port resolution fails or returns zero.
Line 599 swallows
GetInstancePorterrors and can leavehostPortas0, then persists an invalid running DB state.Error-safe fallback handling
hostPort, err := s.parseAllocatedPort(allocatedPorts, defaultPort) if err != nil || hostPort == 0 { - hostPort, _ = s.compute.GetInstancePort(ctx, containerID, defaultPort) + hostPort, err = s.compute.GetInstancePort(ctx, containerID, defaultPort) + if err != nil || hostPort == 0 { + if delErr := s.compute.DeleteInstance(ctx, containerID); delErr != nil { + s.logger.Warn("failed to clean up restored container after port resolution failure", "container_id", containerID, "error", delErr) + } + if delErr := s.volumeSvc.DeleteVolume(ctx, vol.ID.String()); delErr != nil { + s.logger.Warn("failed to clean up restored volume after port resolution failure", "volume_id", vol.ID, "error", delErr) + } + return nil, errors.Wrap(errors.Internal, "failed to resolve restored database port", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/database.go` around lines 597 - 600, The fallback path that calls s.compute.GetInstancePort(ctx, containerID, defaultPort) must not swallow errors or accept a zero port; update the restore logic around parseAllocatedPort and GetInstancePort so that if parseAllocatedPort returns an error or zero, you call s.compute.GetInstancePort and then check its returned error and the hostPort value—if GetInstancePort returns an error or hostPort == 0 propagate/return that error (or abort the restore) instead of assigning hostPort = 0 and continuing; ensure you reference and protect variables hostPort, containerID, defaultPort, parseAllocatedPort and s.compute.GetInstancePort in the updated control flow.
499-501:⚠️ Potential issue | 🟠 MajorHandle ignored errors in snapshot/restore lifecycle paths.
These calls still drop errors via
_ = ...in critical paths (audit/event/cleanup), which can hide operational failures and leak resources.Suggested pattern
- _ = s.auditSvc.Log(ctx, db.UserID, "database.snapshot.create", "database", db.ID.String(), map[string]interface{}{ + if err := s.auditSvc.Log(ctx, db.UserID, "database.snapshot.create", "database", db.ID.String(), map[string]interface{}{ "snapshot_id": snap.ID, - }) + }); err != nil { + s.logger.Warn("failed to write snapshot audit log", "database_id", db.ID, "snapshot_id", snap.ID, "error", err) + }As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like
_ = someFunc()".Also applies to: 592-593, 607-608, 612-618
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/database.go` around lines 499 - 501, The call to s.auditSvc.Log in the snapshot lifecycle is currently discarding errors via `_ = ...`; change this to capture and handle the error (e.g., err := s.auditSvc.Log(ctx, db.UserID, "database.snapshot.create", "database", db.ID.String(), map[string]interface{}{"snapshot_id": snap.ID}) and then handle err by logging via the service logger or returning the error so failures are not silently ignored). Apply the same fix pattern for the other ignored calls in this file (the other `_ =` usages around the snapshot/restore paths noted in the review) so each call checks the returned error and performs appropriate cleanup/logging/returning instead of dropping it.
523-535:⚠️ Potential issue | 🟠 MajorValidate restore engine/version compatibility with snapshot origin.
RestoreDatabaseaccepts requested engine/version without cross-checking snapshot provenance, so incompatible restores can be attempted and boot with mismatched data format.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/database.go` around lines 523 - 535, RestoreDatabase currently only checks allocated storage and that the requested engine is supported; add a validation step that loads the snapshot via snapshotSvc.GetSnapshot (snap) and compares the requested engine/version (domain.DatabaseEngine(engine) and the provided version string) against the snapshot's recorded engine and version fields (e.g., snap.Engine and snap.EngineVersion or equivalent metadata on snap), returning an errors.New(errors.InvalidInput, "...") when they differ to prevent attempting to restore an incompatible snapshot; place this check after obtaining snap and before proceeding with allocation/boot, reusing s.isValidEngine for engine validity but enforcing exact engine+version compatibility.
562-566:⚠️ Potential issue | 🟠 MajorDo not silently overwrite requested
allocatedStorageduring restore.Line 565 replaces the requested value with restored volume size, so the API accepts one value and persists another. Reject unsupported resize (or perform explicit resize) instead of implicit overwrite.
Safer behavior until resize is implemented
- db.AllocatedStorage = vol.SizeGB // Ensure DB record matches actual volume + if vol.SizeGB != allocatedStorage { + return nil, errors.New(errors.InvalidInput, "allocated storage resize during restore is not supported yet") + } + db.AllocatedStorage = allocatedStorage🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/database.go` around lines 562 - 566, The current code unconditionally sets db.AllocatedStorage = vol.SizeGB which silently overwrites the user's requested storage; instead, compare vol.SizeGB to the record's requested value and reject or explicitly handle mismatches: if vol.SizeGB == db.AllocatedStorage (or the stored requested field) leave it as-is; if vol.SizeGB < db.AllocatedStorage return an error indicating resize-down during restore is unsupported; if vol.SizeGB > db.AllocatedStorage either return an error indicating resize-up must be performed explicitly (or call the explicit resize flow) — do not silently assign vol.SizeGB to db.AllocatedStorage; update only when an explicit resize has succeeded. Ensure checks reference db.AllocatedStorage and vol.SizeGB and return a clear error path instead of overwriting.
🧹 Nitpick comments (8)
internal/repositories/postgres/ssh_key_repo.go (1)
3-12: Minor: stdlib import could be grouped with other stdlib imports.The aliased
stdlib_errors "errors"is a standard library package but is currently placed after the blank line with external packages. For consistency with Go import conventions, it could be moved before line 6 withcontextandfmt.That said, this is a cosmetic nit and the change is an improvement over the previous ordering.
♻️ Optional: Group stdlib imports together
import ( "context" + stdlib_errors "errors" "fmt" - stdlib_errors "errors" "github.com/google/uuid" "github.com/jackc/pgx/v5" "github.com/poyrazk/thecloud/internal/core/domain" "github.com/poyrazk/thecloud/internal/errors" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repositories/postgres/ssh_key_repo.go` around lines 3 - 12, Move the aliased stdlib import stdlib_errors "errors" into the standard-library import group with context and fmt inside the import block so stdlib packages are grouped together; update the import ordering in the import block (refer to the current import block containing context, fmt, stdlib_errors "errors", and external packages like github.com/google/uuid) and keep the alias stdlib_errors if desired for clarity.internal/workers/database_failover_worker_test.go (1)
73-82: Mock methods lackmock.Called()support for test assertions.The new mock methods return hardcoded
nil, nilvalues, which is consistent with other unused methods in this file (e.g.,CreateReplica,GetDatabase,ListDatabases). However, this pattern prevents setting expectations or verifying calls when snapshot/restore tests are added later.Consider using
mock.Called()for future-proofing, especially since these represent core new functionality:♻️ Proposed refactor to support testable mocks
func (m *mockDatabaseService) CreateDatabaseSnapshot(ctx context.Context, databaseID uuid.UUID, description string) (*domain.Snapshot, error) { - return nil, nil + args := m.Called(ctx, databaseID, description) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*domain.Snapshot), args.Error(1) } func (m *mockDatabaseService) ListDatabaseSnapshots(ctx context.Context, databaseID uuid.UUID) ([]*domain.Snapshot, error) { - return nil, nil + args := m.Called(ctx, databaseID) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).([]*domain.Snapshot), args.Error(1) } func (m *mockDatabaseService) RestoreDatabase(ctx context.Context, snapshotID uuid.UUID, newName, engine, version string, vpcID *uuid.UUID, allocatedStorage int, parameters map[string]string) (*domain.Database, error) { - return nil, nil + args := m.Called(ctx, snapshotID, newName, engine, version, vpcID, allocatedStorage, parameters) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*domain.Database), args.Error(1) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/workers/database_failover_worker_test.go` around lines 73 - 82, The three mock methods on mockDatabaseService (CreateDatabaseSnapshot, ListDatabaseSnapshots, RestoreDatabase) should call m.Called(...) and return values from that call so tests can set expectations; replace the hardcoded nil,nil returns with using args := m.Called(ctx, databaseID, description) (or appropriate params), then return the typed results using args.Get(0) cast to *domain.Snapshot / []*domain.Snapshot / *domain.Database and args.Error(1), handling nils safely (e.g., nil-check before casting) so tests can assert calls and stub return values.internal/api/setup/router.go (1)
470-471: Consider using dedicated snapshot permissions instead of reusing database permissions.The new snapshot endpoints use
PermissionDBCreateandPermissionDBRead, while the existing/snapshotsroutes (lines 293-297) use dedicated permissions likePermissionSnapshotCreate,PermissionSnapshotRead, etc.For consistency and finer-grained access control, consider introducing or reusing snapshot-specific permissions for these database snapshot operations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/setup/router.go` around lines 470 - 471, The routes registering database-scoped snapshot endpoints are using generic DB permissions (PermissionDBCreate and PermissionDBRead); change them to use the snapshot-specific permissions used elsewhere (e.g., PermissionSnapshotCreate and PermissionSnapshotRead) or add those new constants in the domain RBAC set and wire them into the RBAC service; specifically update the two lines registering handlers.Database.CreateSnapshot and handlers.Database.ListSnapshots to call httputil.Permission(svcs.RBAC, domain.PermissionSnapshotCreate) and httputil.Permission(svcs.RBAC, domain.PermissionSnapshotRead) (or add/alias those permission names in domain and ensure svcs.RBAC recognizes them) so snapshot operations have dedicated, fine-grained permissions.docs/swagger/docs.go (2)
1382-1461: Add explicit non-2xx response contracts for snapshot endpoints.On Line 1406 and Line 1452, only success responses are documented. Please include common error responses (
400/401/404/500) with#/definitions/httputil.Responseso SDK/client generation and API consumers get full contract coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/swagger/docs.go` around lines 1382 - 1461, For the "/databases/{id}/snapshots" GET and POST operations add explicit non-2xx response entries (400, 401, 404, 500) that reference the httputil.Response schema so clients and SDKs receive full error contracts; update the "get" and "post" response objects under that path to include entries "400", "401", "404", and "500" each with "description" text and "schema": {"$ref": "#/definitions/httputil.Response"}—apply this to the existing get and post blocks in the docs for the snapshots endpoint.
9311-9346: Constrain restoreengineto the existing enum schema.On Line 9324,
engineis documented as a plain string even thoughdomain.DatabaseEnginealready exists. Referencing the enum improves validation and prevents unsupported values in generated clients.🔧 Suggested OpenAPI schema adjustment
"engine": { - "type": "string" + "$ref": "#/definitions/domain.DatabaseEngine" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/swagger/docs.go` around lines 9311 - 9346, The RestoreDatabaseRequest schema's engine property is currently a plain string; update the httphandlers.RestoreDatabaseRequest definition to constrain engine to the existing domain.DatabaseEngine enum (i.e., reference or inline the domain.DatabaseEngine definition instead of "type":"string") so generated clients validate allowed engine values; locate the "httphandlers.RestoreDatabaseRequest" schema and replace the engine property with a $ref to the domain.DatabaseEngine definition or the equivalent enum listing from domain.DatabaseEngine.docs/swagger/swagger.json (2)
9313-9336: Add basic schema bounds on restore inputs.
allocated_storage,name,snapshot_id, andversionare required but unconstrained. Adding minimal bounds improves request validation quality at the API contract layer.🔧 Suggested validation constraints
"allocated_storage": { - "type": "integer" + "type": "integer", + "minimum": 1 }, "engine": { "$ref": "#/definitions/domain.DatabaseEngine" }, "name": { - "type": "string" + "type": "string", + "minLength": 1 }, @@ "snapshot_id": { - "type": "string" + "type": "string", + "minLength": 1 }, "version": { - "type": "string" + "type": "string", + "minLength": 1 },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/swagger/swagger.json` around lines 9313 - 9336, The schema for the restore input is missing required markers and bounds: mark "allocated_storage", "name", "snapshot_id", and "version" as required and add minimal validation — e.g., set "allocated_storage" to an integer with a sensible "minimum" (>=1) and optional "maximum" if desired, and set "name", "snapshot_id", and "version" to strings with "minLength": 1 (or a small pattern if you prefer stricter format). Update the schema around the "allocated_storage", "name", "snapshot_id", and "version" properties and add a "required" array including those four fields so the OpenAPI validator enforces them.
1335-1453: Document non-2xx responses for the new database endpoints.The new
POST /databases/restoreandGET/POST /databases/{id}/snapshotsoperations currently describe only success responses. Adding standard error schemas (400/401/404/500) will make client behavior and generated SDKs more reliable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/swagger/swagger.json` around lines 1335 - 1453, The new operations "/databases/restore" (post) and "/databases/{id}/snapshots" (get, post) only list success responses; add standard error responses (400, 401, 404, 500) to each operation using the project's shared error schema (e.g. reference the existing error definition such as "#/definitions/httphandlers.ErrorResponse" or "#/definitions/ErrorResponse"), and ensure each response includes a brief description ("Bad Request", "Unauthorized", "Not Found", "Internal Server Error") and the error schema reference so generated clients can handle non-2xx cases consistently.docs/swagger/swagger.yaml (1)
1969-1971: Use the enum schema for restore engine instead of an unbounded string.
httphandlers.RestoreDatabaseRequest.engineis currentlytype: string; referencingdomain.DatabaseEnginehere would align request validation with the actual accepted values (postgres,mysql).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/swagger/swagger.yaml` around lines 1969 - 1971, Replace the open string schema for the RestoreDatabaseRequest.engine with the enum schema for domain.DatabaseEngine: update the swagger entry for httphandlers.RestoreDatabaseRequest.engine (currently "type: string") to reference the existing domain.DatabaseEngine schema (or inline the same enum values) so validation only allows "postgres", "mysql" (i.e. use $ref: '#/components/schemas/DatabaseEngine' or the equivalent component name).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/swagger/swagger.json`:
- Around line 9303-9338: Update the swagger schema for
httphandlers.RestoreDatabaseRequest so the "engine" property references the
existing domain.DatabaseEngine enum instead of a free-form string: replace the
"engine": { "type": "string" } entry with a $ref to the enum (or an "enum"
listing matching domain.DatabaseEngine) so validation enforces allowed engine
values; locate the "engine" property inside the RestoreDatabaseRequest object
and change its schema to reference domain.DatabaseEngine.
In `@internal/handlers/database_handler_test.go`:
- Around line 90-93: The mockDatabaseService.ListDatabaseSnapshots method can
panic when the test returns (nil, err) because it unconditionally casts
args.Get(0) to []*domain.Snapshot; add the same nil-guard pattern used in
mockDatabaseService.CreateDatabaseSnapshot: check if args.Get(0) == nil and
return nil, args.Error(1) in that case, otherwise cast and return the slice
along with args.Error(1).
In `@internal/handlers/database_handler.go`:
- Line 197: Replace the use of http.StatusCreated with http.StatusOK in the
successful response calls to httputil.Success in
internal/handlers/database_handler.go (i.e., change httputil.Success(c,
http.StatusCreated, ...) to httputil.Success(c, http.StatusOK, ...)); update
both occurrences where httputil.Success currently uses http.StatusCreated so
they follow the project convention of returning http.StatusOK for successful
handler responses.
- Line 186: Replace the brittle string comparison of the binding error with an
idiomatic errors.Is check: in the c.ShouldBindJSON(&req) error handling, use
errors.Is(err, io.EOF) to detect empty body instead of err.Error() == "EOF";
update imports to include the "errors" and "io" packages if missing and keep the
existing behavior of allowing an empty body for the request bound to req.
---
Outside diff comments:
In `@internal/api/setup/router.go`:
- Around line 459-472: The dbGroup route is missing tenant-scoping middleware;
update the dbGroup.Use call (where dbGroup is defined) to include
httputil.RequireTenant() and httputil.TenantMember(svcs.Tenant) in addition to
the existing httputil.Auth(svcs.Identity, svcs.Tenant) so that the line becomes
a single dbGroup.Use invocation with httputil.Auth(...),
httputil.RequireTenant(), httputil.TenantMember(svcs.Tenant) to ensure tenant
isolation for handlers like handlers.Database.Create, Restore, List, Get,
Delete, CreateReplica, Promote, CreateSnapshot and ListSnapshots.
In `@internal/repositories/postgres/cluster_repo.go`:
- Around line 213-214: The branch that checks stdlib_errors.Is(err,
pgx.ErrNoRows) should return a NotFound error instead of (nil, nil); replace the
current return nil, nil in that conditional with a call that constructs and
returns a NotFound error (e.g. errors.New(errors.NotFound, "cluster not found")
or the codebase's equivalent) so callers don’t have to defensively treat nil as
"not found"; apply the same change to the analogous stdlib_errors.Is(err,
pgx.ErrNoRows) cases in pipeline_repo.go (lines noted), queue_repo.go, and
tenant_repo.go:GetMembership to keep behavior consistent across repositories.
---
Duplicate comments:
In `@internal/core/services/database.go`:
- Around line 597-600: The fallback path that calls
s.compute.GetInstancePort(ctx, containerID, defaultPort) must not swallow errors
or accept a zero port; update the restore logic around parseAllocatedPort and
GetInstancePort so that if parseAllocatedPort returns an error or zero, you call
s.compute.GetInstancePort and then check its returned error and the hostPort
value—if GetInstancePort returns an error or hostPort == 0 propagate/return that
error (or abort the restore) instead of assigning hostPort = 0 and continuing;
ensure you reference and protect variables hostPort, containerID, defaultPort,
parseAllocatedPort and s.compute.GetInstancePort in the updated control flow.
- Around line 499-501: The call to s.auditSvc.Log in the snapshot lifecycle is
currently discarding errors via `_ = ...`; change this to capture and handle the
error (e.g., err := s.auditSvc.Log(ctx, db.UserID, "database.snapshot.create",
"database", db.ID.String(), map[string]interface{}{"snapshot_id": snap.ID}) and
then handle err by logging via the service logger or returning the error so
failures are not silently ignored). Apply the same fix pattern for the other
ignored calls in this file (the other `_ =` usages around the snapshot/restore
paths noted in the review) so each call checks the returned error and performs
appropriate cleanup/logging/returning instead of dropping it.
- Around line 523-535: RestoreDatabase currently only checks allocated storage
and that the requested engine is supported; add a validation step that loads the
snapshot via snapshotSvc.GetSnapshot (snap) and compares the requested
engine/version (domain.DatabaseEngine(engine) and the provided version string)
against the snapshot's recorded engine and version fields (e.g., snap.Engine and
snap.EngineVersion or equivalent metadata on snap), returning an
errors.New(errors.InvalidInput, "...") when they differ to prevent attempting to
restore an incompatible snapshot; place this check after obtaining snap and
before proceeding with allocation/boot, reusing s.isValidEngine for engine
validity but enforcing exact engine+version compatibility.
- Around line 562-566: The current code unconditionally sets db.AllocatedStorage
= vol.SizeGB which silently overwrites the user's requested storage; instead,
compare vol.SizeGB to the record's requested value and reject or explicitly
handle mismatches: if vol.SizeGB == db.AllocatedStorage (or the stored requested
field) leave it as-is; if vol.SizeGB < db.AllocatedStorage return an error
indicating resize-down during restore is unsupported; if vol.SizeGB >
db.AllocatedStorage either return an error indicating resize-up must be
performed explicitly (or call the explicit resize flow) — do not silently assign
vol.SizeGB to db.AllocatedStorage; update only when an explicit resize has
succeeded. Ensure checks reference db.AllocatedStorage and vol.SizeGB and return
a clear error path instead of overwriting.
In `@internal/handlers/database_handler.go`:
- Around line 152-165: The JSON tags for optional fields are missing omitempty:
update the struct tags so CreateDatabaseSnapshotRequest.Description uses
`json:"description,omitempty"`, and in RestoreDatabaseRequest add `omitempty` to
VpcID and Parameters (i.e., `VpcID *uuid.UUID `json:"vpc_id,omitempty"` and
`Parameters map[string]string `json:"parameters,omitempty"`); modify the tags on
the identified fields (Description, VpcID, Parameters) in the
CreateDatabaseSnapshotRequest and RestoreDatabaseRequest structs accordingly so
optional fields are omitted when empty.
---
Nitpick comments:
In `@docs/swagger/docs.go`:
- Around line 1382-1461: For the "/databases/{id}/snapshots" GET and POST
operations add explicit non-2xx response entries (400, 401, 404, 500) that
reference the httputil.Response schema so clients and SDKs receive full error
contracts; update the "get" and "post" response objects under that path to
include entries "400", "401", "404", and "500" each with "description" text and
"schema": {"$ref": "#/definitions/httputil.Response"}—apply this to the existing
get and post blocks in the docs for the snapshots endpoint.
- Around line 9311-9346: The RestoreDatabaseRequest schema's engine property is
currently a plain string; update the httphandlers.RestoreDatabaseRequest
definition to constrain engine to the existing domain.DatabaseEngine enum (i.e.,
reference or inline the domain.DatabaseEngine definition instead of
"type":"string") so generated clients validate allowed engine values; locate the
"httphandlers.RestoreDatabaseRequest" schema and replace the engine property
with a $ref to the domain.DatabaseEngine definition or the equivalent enum
listing from domain.DatabaseEngine.
In `@docs/swagger/swagger.json`:
- Around line 9313-9336: The schema for the restore input is missing required
markers and bounds: mark "allocated_storage", "name", "snapshot_id", and
"version" as required and add minimal validation — e.g., set "allocated_storage"
to an integer with a sensible "minimum" (>=1) and optional "maximum" if desired,
and set "name", "snapshot_id", and "version" to strings with "minLength": 1 (or
a small pattern if you prefer stricter format). Update the schema around the
"allocated_storage", "name", "snapshot_id", and "version" properties and add a
"required" array including those four fields so the OpenAPI validator enforces
them.
- Around line 1335-1453: The new operations "/databases/restore" (post) and
"/databases/{id}/snapshots" (get, post) only list success responses; add
standard error responses (400, 401, 404, 500) to each operation using the
project's shared error schema (e.g. reference the existing error definition such
as "#/definitions/httphandlers.ErrorResponse" or "#/definitions/ErrorResponse"),
and ensure each response includes a brief description ("Bad Request",
"Unauthorized", "Not Found", "Internal Server Error") and the error schema
reference so generated clients can handle non-2xx cases consistently.
In `@docs/swagger/swagger.yaml`:
- Around line 1969-1971: Replace the open string schema for the
RestoreDatabaseRequest.engine with the enum schema for domain.DatabaseEngine:
update the swagger entry for httphandlers.RestoreDatabaseRequest.engine
(currently "type: string") to reference the existing domain.DatabaseEngine
schema (or inline the same enum values) so validation only allows "postgres",
"mysql" (i.e. use $ref: '#/components/schemas/DatabaseEngine' or the equivalent
component name).
In `@internal/api/setup/router.go`:
- Around line 470-471: The routes registering database-scoped snapshot endpoints
are using generic DB permissions (PermissionDBCreate and PermissionDBRead);
change them to use the snapshot-specific permissions used elsewhere (e.g.,
PermissionSnapshotCreate and PermissionSnapshotRead) or add those new constants
in the domain RBAC set and wire them into the RBAC service; specifically update
the two lines registering handlers.Database.CreateSnapshot and
handlers.Database.ListSnapshots to call httputil.Permission(svcs.RBAC,
domain.PermissionSnapshotCreate) and httputil.Permission(svcs.RBAC,
domain.PermissionSnapshotRead) (or add/alias those permission names in domain
and ensure svcs.RBAC recognizes them) so snapshot operations have dedicated,
fine-grained permissions.
In `@internal/repositories/postgres/ssh_key_repo.go`:
- Around line 3-12: Move the aliased stdlib import stdlib_errors "errors" into
the standard-library import group with context and fmt inside the import block
so stdlib packages are grouped together; update the import ordering in the
import block (refer to the current import block containing context, fmt,
stdlib_errors "errors", and external packages like github.com/google/uuid) and
keep the alias stdlib_errors if desired for clarity.
In `@internal/workers/database_failover_worker_test.go`:
- Around line 73-82: The three mock methods on mockDatabaseService
(CreateDatabaseSnapshot, ListDatabaseSnapshots, RestoreDatabase) should call
m.Called(...) and return values from that call so tests can set expectations;
replace the hardcoded nil,nil returns with using args := m.Called(ctx,
databaseID, description) (or appropriate params), then return the typed results
using args.Get(0) cast to *domain.Snapshot / []*domain.Snapshot /
*domain.Database and args.Error(1), handling nils safely (e.g., nil-check before
casting) so tests can assert calls and stub return values.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (118)
cmd/cloud/dns.gocmd/cloud/kubernetes_test.gocmd/cloud/logs_test.godocs/adr/ADR-018-database-automated-backups.mddocs/database.mddocs/swagger/docs.godocs/swagger/swagger.jsondocs/swagger/swagger.yamlinternal/adapters/dns/powerdns_geo_test.gointernal/api/setup/dependencies.gointernal/api/setup/infrastructure_test.gointernal/api/setup/router.gointernal/core/domain/elastic_ip_test.gointernal/core/domain/iam.gointernal/core/domain/security_group_test.gointernal/core/domain/tenant.gointernal/core/ports/database.gointernal/core/ports/log.gointernal/core/services/accounting_internal_test.gointernal/core/services/audit_test.gointernal/core/services/autoscaling_test.gointernal/core/services/cache_unit_test.gointernal/core/services/cached_identity_test.gointernal/core/services/cloudlogs_unit_test.gointernal/core/services/cluster_lifecycle_test.gointernal/core/services/cluster_test.gointernal/core/services/container_unit_test.gointernal/core/services/dashboard_test.gointernal/core/services/database.gointernal/core/services/database_test.gointernal/core/services/database_unit_test.gointernal/core/services/dns_unit_test.gointernal/core/services/elastic_ip_unit_test.gointernal/core/services/event_test.gointernal/core/services/function.gointernal/core/services/iam_test.gointernal/core/services/iam_unit_test.gointernal/core/services/instance_internal_test.gointernal/core/services/instance_test.gointernal/core/services/lifecycle_svc_test.gointernal/core/services/loadbalancer_unit_test.gointernal/core/services/pipeline.gointernal/core/services/pipeline_unit_test.gointernal/core/services/rbac_iam_test.gointernal/core/services/setup_test.gointernal/core/services/stack_unit_test.gointernal/core/services/storage_unit_test.gointernal/core/services/vpc_peering_test.gointernal/handlers/cache_handler_test.gointernal/handlers/container_handler_test.gointernal/handlers/database_handler.gointernal/handlers/database_handler_test.gointernal/handlers/dns_handler_test.gointernal/handlers/function_handler_test.gointernal/handlers/identity_handler_test.gointernal/handlers/lb_handler_test.gointernal/handlers/log_handler.gointernal/handlers/log_handler_test.gointernal/handlers/notify_handler_test.gointernal/handlers/rbac_handler_test.gointernal/handlers/ssh_key_handler_test.gointernal/handlers/volume_handler_test.gointernal/platform/circuit_breaker_test.gointernal/platform/config_test.gointernal/platform/redis_test.gointernal/repositories/k8s/provisioner_unit_test.gointernal/repositories/libvirt/adapter.gointernal/repositories/libvirt/adapter_lifecycle_test.gointernal/repositories/libvirt/adapter_unit_test.gointernal/repositories/libvirt/lb_proxy_test.gointernal/repositories/libvirt/templates_test.gointernal/repositories/lvm/adapter_test.gointernal/repositories/noop/adapters.gointernal/repositories/noop/repos_extra_test.gointernal/repositories/postgres/autoscaling_repo_extra_test.gointernal/repositories/postgres/cache_repo.gointernal/repositories/postgres/cluster_repo.gointernal/repositories/postgres/database_repo.gointernal/repositories/postgres/database_repo_unit_test.gointernal/repositories/postgres/dns_repo.gointernal/repositories/postgres/iam_repo.gointernal/repositories/postgres/identity_repo.gointernal/repositories/postgres/image_repo.gointernal/repositories/postgres/instance_repo.gointernal/repositories/postgres/instance_type_repo.gointernal/repositories/postgres/lb_repo.gointernal/repositories/postgres/lifecycle_repo.gointernal/repositories/postgres/log_repo.gointernal/repositories/postgres/pipeline_repo.gointernal/repositories/postgres/queue_repo.gointernal/repositories/postgres/secret_repo.gointernal/repositories/postgres/security_group_repo.gointernal/repositories/postgres/ssh_key_repo.gointernal/repositories/postgres/storage_repo.gointernal/repositories/postgres/subnet_repo.gointernal/repositories/postgres/tenant_repo.gointernal/repositories/postgres/test_helpers.gointernal/repositories/postgres/vpc_repo.gointernal/repositories/redis/task_queue.gointernal/storage/coordinator/service_test.gointernal/workers/database_failover_worker_test.gointernal/workers/healing_worker_test.gointernal/workers/log_worker.gointernal/workers/storage_cleanup_worker.gointernal/workers/storage_cleanup_worker_internal_test.gointernal/workers/workers_unit_test.gopkg/audit/simple_audit_test.gopkg/crypto/presign_test.gopkg/ratelimit/limiter_unit_test.gopkg/sdk/compute.gopkg/sdk/iac_test.gopkg/sdk/kubernetes_test.gopkg/sshutil/client_test.gotests/database_advanced_e2e_test.gotests/firecracker_api_test.gotests/firecracker_e2e_test.gotests/gateway_e2e_test.gotests/logs_e2e_test.go
✅ Files skipped from review due to trivial changes (76)
- internal/core/services/lifecycle_svc_test.go
- pkg/sdk/iac_test.go
- internal/core/services/pipeline_unit_test.go
- internal/repositories/postgres/dns_repo.go
- internal/core/services/event_test.go
- internal/repositories/postgres/tenant_repo.go
- internal/repositories/redis/task_queue.go
- internal/repositories/postgres/instance_type_repo.go
- internal/repositories/lvm/adapter_test.go
- internal/handlers/ssh_key_handler_test.go
- internal/core/services/autoscaling_test.go
- internal/core/services/audit_test.go
- pkg/sdk/compute.go
- cmd/cloud/kubernetes_test.go
- internal/platform/circuit_breaker_test.go
- internal/handlers/function_handler_test.go
- internal/repositories/libvirt/lb_proxy_test.go
- internal/core/services/elastic_ip_unit_test.go
- internal/core/services/iam_test.go
- tests/gateway_e2e_test.go
- internal/core/services/instance_test.go
- internal/workers/healing_worker_test.go
- internal/storage/coordinator/service_test.go
- cmd/cloud/dns.go
- internal/core/services/container_unit_test.go
- internal/handlers/log_handler.go
- internal/repositories/libvirt/templates_test.go
- internal/repositories/postgres/log_repo.go
- internal/core/domain/elastic_ip_test.go
- internal/repositories/postgres/security_group_repo.go
- tests/database_advanced_e2e_test.go
- internal/repositories/postgres/lifecycle_repo.go
- internal/core/services/dashboard_test.go
- tests/firecracker_e2e_test.go
- internal/repositories/postgres/image_repo.go
- internal/core/services/iam_unit_test.go
- internal/repositories/libvirt/adapter_unit_test.go
- internal/core/services/setup_test.go
- internal/core/services/rbac_iam_test.go
- cmd/cloud/logs_test.go
- internal/core/domain/security_group_test.go
- pkg/ratelimit/limiter_unit_test.go
- internal/repositories/postgres/storage_repo.go
- internal/repositories/libvirt/adapter.go
- internal/repositories/postgres/lb_repo.go
- internal/core/services/cache_unit_test.go
- internal/repositories/postgres/test_helpers.go
- internal/repositories/k8s/provisioner_unit_test.go
- internal/core/services/loadbalancer_unit_test.go
- internal/handlers/cache_handler_test.go
- tests/firecracker_api_test.go
- pkg/sshutil/client_test.go
- internal/repositories/postgres/autoscaling_repo_extra_test.go
- internal/repositories/noop/adapters.go
- internal/handlers/volume_handler_test.go
- internal/handlers/rbac_handler_test.go
- internal/repositories/postgres/database_repo.go
- internal/workers/storage_cleanup_worker_internal_test.go
- internal/core/services/vpc_peering_test.go
- internal/repositories/postgres/queue_repo.go
- internal/repositories/postgres/pipeline_repo.go
- internal/repositories/postgres/cache_repo.go
- internal/workers/storage_cleanup_worker.go
- tests/logs_e2e_test.go
- internal/handlers/dns_handler_test.go
- internal/repositories/postgres/iam_repo.go
- internal/handlers/notify_handler_test.go
- internal/core/domain/tenant.go
- internal/repositories/postgres/subnet_repo.go
- internal/repositories/postgres/secret_repo.go
- internal/core/services/instance_internal_test.go
- internal/handlers/lb_handler_test.go
- internal/core/services/storage_unit_test.go
- internal/core/ports/log.go
- internal/core/services/dns_unit_test.go
- internal/adapters/dns/powerdns_geo_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/services/database_test.go
- internal/api/setup/dependencies.go
Comment on lines
+9303
to
+9338
| "httphandlers.RestoreDatabaseRequest": { | ||
| "type": "object", | ||
| "required": [ | ||
| "allocated_storage", | ||
| "engine", | ||
| "name", | ||
| "snapshot_id", | ||
| "version" | ||
| ], | ||
| "properties": { | ||
| "allocated_storage": { | ||
| "type": "integer" | ||
| }, | ||
| "engine": { | ||
| "type": "string" | ||
| }, | ||
| "name": { | ||
| "type": "string" | ||
| }, | ||
| "parameters": { | ||
| "type": "object", | ||
| "additionalProperties": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "snapshot_id": { | ||
| "type": "string" | ||
| }, | ||
| "version": { | ||
| "type": "string" | ||
| }, | ||
| "vpc_id": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Constrain restore engine to the declared enum.
Line 9316 currently defines engine as free-form string, but the spec already defines domain.DatabaseEngine enum. This weakens contract correctness and can allow invalid client payloads.
🔧 Proposed fix
"httphandlers.RestoreDatabaseRequest": {
"type": "object",
"required": [
"allocated_storage",
"engine",
"name",
"snapshot_id",
"version"
],
"properties": {
"allocated_storage": {
"type": "integer"
},
"engine": {
- "type": "string"
+ "$ref": "#/definitions/domain.DatabaseEngine"
},
"name": {
"type": "string"
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/swagger/swagger.json` around lines 9303 - 9338, Update the swagger
schema for httphandlers.RestoreDatabaseRequest so the "engine" property
references the existing domain.DatabaseEngine enum instead of a free-form
string: replace the "engine": { "type": "string" } entry with a $ref to the enum
(or an "enum" listing matching domain.DatabaseEngine) so validation enforces
allowed engine values; locate the "engine" property inside the
RestoreDatabaseRequest object and change its schema to reference
domain.DatabaseEngine.
Comment on lines
+90
to
+93
| func (m *mockDatabaseService) ListDatabaseSnapshots(ctx context.Context, databaseID uuid.UUID) ([]*domain.Snapshot, error) { | ||
| args := m.Called(ctx, databaseID) | ||
| return args.Get(0).([]*domain.Snapshot), args.Error(1) | ||
| } |
There was a problem hiding this comment.
Guard nil return in ListDatabaseSnapshots to avoid panic.
Unlike CreateDatabaseSnapshot (lines 85-87) which has a nil guard, ListDatabaseSnapshots will panic when a test sets up (nil, err) return values.
🧪 Add nil guard for consistency
func (m *mockDatabaseService) ListDatabaseSnapshots(ctx context.Context, databaseID uuid.UUID) ([]*domain.Snapshot, error) {
args := m.Called(ctx, databaseID)
+ if args.Get(0) == nil {
+ return nil, args.Error(1)
+ }
return args.Get(0).([]*domain.Snapshot), args.Error(1)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/database_handler_test.go` around lines 90 - 93, The
mockDatabaseService.ListDatabaseSnapshots method can panic when the test returns
(nil, err) because it unconditionally casts args.Get(0) to []*domain.Snapshot;
add the same nil-guard pattern used in
mockDatabaseService.CreateDatabaseSnapshot: check if args.Get(0) == nil and
return nil, args.Error(1) in that case, otherwise cast and return the slice
along with args.Error(1).
| } | ||
|
|
||
| var req CreateDatabaseSnapshotRequest | ||
| if err := c.ShouldBindJSON(&req); err != nil && err.Error() != "EOF" { // Allow empty body |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the database_handler.go file
fd -type f -name "database_handler.go" | head -5Repository: poyrazK/thecloud
Length of output: 230
🏁 Script executed:
# Check for brittle EOF string comparisons in Go files
rg -n --type go 'err\.Error\(\)\s*(!=|==)\s*"EOF"' internal/handlers/Repository: poyrazK/thecloud
Length of output: 193
🏁 Script executed:
# Look at ShouldBindJSON usage patterns in handlers
rg -n -A 2 'ShouldBindJSON\(' internal/handlers/ | head -40Repository: poyrazK/thecloud
Length of output: 2524
🏁 Script executed:
# Read database_handler.go around line 186
sed -n '180,195p' internal/handlers/database_handler.goRepository: poyrazK/thecloud
Length of output: 521
🏁 Script executed:
# Get more context - see the full function containing line 186
sed -n '170,200p' internal/handlers/database_handler.goRepository: poyrazK/thecloud
Length of output: 1019
🏁 Script executed:
# Check imports in database_handler.go to understand what's already available
head -30 internal/handlers/database_handler.goRepository: poyrazK/thecloud
Length of output: 997
🏁 Script executed:
# Search for how the error from ShouldBindJSON is handled elsewhere with empty bodies
rg -n "ShouldBindJSON.*EOF" --type go internal/Repository: poyrazK/thecloud
Length of output: 193
🏁 Script executed:
# Look for any comments or context about empty body handling in the codebase
rg -n -B 2 -A 2 "Allow empty body" --type goRepository: poyrazK/thecloud
Length of output: 476
🏁 Script executed:
# Check if errors.Is is already used in the codebase for error checking
rg -n "errors\.Is" --type go internal/ | head -20Repository: poyrazK/thecloud
Length of output: 2031
🏁 Script executed:
# Search for tests related to empty request bodies or ShouldBindJSON behavior
find . -name "*test.go" -o -name "*_test.go" | xargs rg -l "ShouldBindJSON\|empty.*body" 2>/dev/null | head -5Repository: poyrazK/thecloud
Length of output: 42
🏁 Script executed:
# Look for any test files for handlers
find internal/handlers -name "*test.go" -o -name "*_test.go" 2>/dev/nullRepository: poyrazK/thecloud
Length of output: 1915
🏁 Script executed:
# Check if there's documentation or comments about Gin error handling
rg -n "Gin\|ShouldBind" internal/handlers/database_handler.goRepository: poyrazK/thecloud
Length of output: 42
🌐 Web query:
Gin ShouldBindJSON empty request body error io.EOF
💡 Result:
In Gin, c.ShouldBindJSON(&dst) ultimately uses Go’s JSON decoder; it will return io.EOF when the request body is empty or when the body has already been read/consumed earlier (e.g., by logging middleware, c.GetRawData(), io.ReadAll(c.Request.Body), or a previous bind). Gin’s docs also note that normal binding consumes c.Request.Body and can’t be called multiple times. [2], [4]
Fixes
1) If an empty body is allowed, treat io.EOF as “no input”
var req MyReq
if err := c.ShouldBindJSON(&req); err != nil {
if errors.Is(err, io.EOF) {
// empty body: keep defaults / handle as optional
} else {
c.JSON(400, gin.H{"error": err.Error()})
return
}
}2) If something read the body before binding, don’t consume it (or restore it)
- Preferred: use
ShouldBindBodyWith, which caches the body so it can be rebound later. [3], [4]
var req MyReq
if err := c.ShouldBindBodyWith(&req, binding.JSON); err != nil { ... }- If you manually read it, you must reset the body afterward (same idea as typical middleware fixes): [1], [5]
b, _ := io.ReadAll(c.Request.Body)
c.Request.Body = io.NopCloser(bytes.NewReader(b)) // restore for later ShouldBindJSON3) If you bind twice in the same handler, the second bind will hit EOF
Use ShouldBindBodyWith for both binds, or bind once and route logic from that. Gin explicitly shows the “second bind gets EOF” pattern and recommends ShouldBindBodyWith. [2], [4]
Most common root cause: a middleware (or earlier code) read the body for logging/HMAC/etc., leaving nothing for ShouldBindJSON to read. [1], [3]
🏁 Script executed:
# Check the database_handler_test.go for tests related to CreateSnapshot
grep -n "CreateSnapshot\|empty.*body" internal/handlers/database_handler_test.go | head -20Repository: poyrazK/thecloud
Length of output: 42
🏁 Script executed:
# Look at the actual test implementation for CreateSnapshot
sed -n '1,50p' internal/handlers/database_handler_test.goRepository: poyrazK/thecloud
Length of output: 1419
🏁 Script executed:
# Search for CreateSnapshot test specifically
rg -n "CreateSnapshot" internal/handlers/database_handler_test.go -A 30 | head -80Repository: poyrazK/thecloud
Length of output: 42
🏁 Script executed:
# Look for any test cases with empty body or nil request
rg -n "ShouldBindJSON\|EOF\|empty" internal/handlers/database_handler_test.goRepository: poyrazK/thecloud
Length of output: 42
🏁 Script executed:
# Check what the request body looks like when testing empty body
rg -n -B 5 -A 10 "nil.*request\|empty.*body" internal/handlers/database_handler_test.goRepository: poyrazK/thecloud
Length of output: 42
🏁 Script executed:
# Get the full database_handler_test.go file to find CreateSnapshot tests
wc -l internal/handlers/database_handler_test.goRepository: poyrazK/thecloud
Length of output: 106
🏁 Script executed:
# Search for all test functions in database_handler_test.go
rg -n "func.*Test" internal/handlers/database_handler_test.go | head -30Repository: poyrazK/thecloud
Length of output: 770
🏁 Script executed:
# Look for any test that might test the snapshot functionality with empty body
rg -n "snapshot" -i internal/handlers/database_handler_test.go -B 2 -A 10 | head -50Repository: poyrazK/thecloud
Length of output: 1266
🏁 Script executed:
# Look at the entire test file to see if CreateSnapshot is tested
grep -n "CreateSnapshot\|Snapshot" internal/handlers/database_handler_test.goRepository: poyrazK/thecloud
Length of output: 450
🏁 Script executed:
# Check the actual database_handler.go to see all the snapshot-related methods
rg -n "func.*Snapshot" internal/handlers/database_handler.goRepository: poyrazK/thecloud
Length of output: 184
🏁 Script executed:
# Look at what tests exist around line 350+ in database_handler_test.go
sed -n '330,397p' internal/handlers/database_handler_test.goRepository: poyrazK/thecloud
Length of output: 2334
Use errors.Is(err, io.EOF) instead of string-matching err.Error().
String comparison is brittle and dependent on error message formatting. Gin's ShouldBindJSON returns io.EOF for empty request bodies, which should be checked using the idiomatic errors.Is(err, io.EOF) pattern.
Suggested diff
import (
+ stdErrors "errors"
+ "io"
"net/http"
@@
- if err := c.ShouldBindJSON(&req); err != nil && err.Error() != "EOF" { // Allow empty body
+ if err := c.ShouldBindJSON(&req); err != nil && !stdErrors.Is(err, io.EOF) { // Allow empty body
httputil.Error(c, errors.New(errors.InvalidInput, err.Error()))
return
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/database_handler.go` at line 186, Replace the brittle
string comparison of the binding error with an idiomatic errors.Is check: in the
c.ShouldBindJSON(&req) error handling, use errors.Is(err, io.EOF) to detect
empty body instead of err.Error() == "EOF"; update imports to include the
"errors" and "io" packages if missing and keep the existing behavior of allowing
an empty body for the request bound to req.
| return | ||
| } | ||
|
|
||
| httputil.Success(c, http.StatusCreated, snap) |
There was a problem hiding this comment.
Use http.StatusOK for successful handler responses per project convention.
Line 197 and Line 248 currently return http.StatusCreated with httputil.Success.
Suggested diff
- httputil.Success(c, http.StatusCreated, snap)
+ httputil.Success(c, http.StatusOK, snap)
@@
- httputil.Success(c, http.StatusCreated, db)
+ httputil.Success(c, http.StatusOK, db)As per coding guidelines, "Use httputil.Success(c, http.StatusOK, data) for successful handler responses".
Also applies to: 248-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/handlers/database_handler.go` at line 197, Replace the use of
http.StatusCreated with http.StatusOK in the successful response calls to
httputil.Success in internal/handlers/database_handler.go (i.e., change
httputil.Success(c, http.StatusCreated, ...) to httputil.Success(c,
http.StatusOK, ...)); update both occurrences where httputil.Success currently
uses http.StatusCreated so they follow the project convention of returning
http.StatusOK for successful handler responses.
This was referenced Mar 2, 2026
This was referenced Apr 15, 2026
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
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.
This PR implements point-in-time recovery and snapshot management for the Managed Database Service.
Key Features:
POST /databases/:id/snapshots.GET /databases/:id/snapshots.POST /databases/restore.SnapshotServicefor crash-consistent volume backups.Implementation Details:
DatabaseServiceto resolve volumes and interact withSnapshotService.dependencies.go.Summary by CodeRabbit
Release Notes
New Features
Documentation