From c2e9ebf62f954c6fe77272323de08343baee1016 Mon Sep 17 00:00:00 2001 From: Shankar Date: Sat, 18 Apr 2026 01:20:46 +0000 Subject: [PATCH] fix(m2-pr-d): thread ctx through Job/Notification/Audit services Collapse CancelJobWithContext into CancelJob; eliminate 10 context.Background() hits across the Job+Notification+Audit service cluster by threading ctx through their handler-facing service interfaces. Services (ctx-first): - service/job.go: ListJobs, GetJob, CancelJob, ApproveJob, RejectJob now accept ctx; the CancelJobWithContext wrapper is removed (handler callers continue to invoke CancelJob, now ctx-aware). - service/notification.go: ListNotifications, GetNotification, MarkAsRead accept ctx. - service/audit.go: ListAuditEvents, GetAuditEvent accept ctx. Handlers (interface + callsites): - handler/jobs.go, handler/notifications.go, handler/audit.go: local service interfaces updated, r.Context() threaded at every callsite. Tests: - Mock services updated to match the new interfaces (ctx accepted and ignored via '_ context.Context' first parameter; Fn closure fields unchanged). - job_test.go / notification_test.go callsites thread context.Background() to match production shape. Verification: go build ./... ok go vet ./... ok go test -short ./... ok go test -race -short ./... ok golangci-lint run ./... 0 issues Locked decisions from the M-2 plan: D-1 ctx-only signatures (no dual forms) D-4 preserve handler method names facing the router D-5 domain types stay ctx-free Audit complete. Commit: 855124a9d9c784da85c6467ac56afa5931e25106. Sections: 12. Findings: 2/7/10/4/6. --- internal/api/handler/audit.go | 9 ++++---- internal/api/handler/audit_handler_test.go | 4 ++-- internal/api/handler/job_handler_test.go | 11 +++++---- internal/api/handler/jobs.go | 21 +++++++++-------- .../api/handler/notification_handler_test.go | 7 +++--- internal/api/handler/notifications.go | 13 ++++++----- internal/service/audit.go | 8 +++---- internal/service/job.go | 23 +++++++------------ internal/service/job_test.go | 16 +++++++++---- internal/service/notification.go | 12 +++++----- internal/service/notification_test.go | 6 ++--- 11 files changed, 67 insertions(+), 63 deletions(-) diff --git a/internal/api/handler/audit.go b/internal/api/handler/audit.go index 60a89d3..63ef4dd 100644 --- a/internal/api/handler/audit.go +++ b/internal/api/handler/audit.go @@ -1,6 +1,7 @@ package handler import ( + "context" "net/http" "strconv" "strings" @@ -11,8 +12,8 @@ import ( // AuditService defines the service interface for audit event operations. type AuditService interface { - ListAuditEvents(page, perPage int) ([]domain.AuditEvent, int64, error) - GetAuditEvent(id string) (*domain.AuditEvent, error) + ListAuditEvents(ctx context.Context, page, perPage int) ([]domain.AuditEvent, int64, error) + GetAuditEvent(ctx context.Context, id string) (*domain.AuditEvent, error) } // AuditHandler handles HTTP requests for audit event operations. @@ -49,7 +50,7 @@ func (h AuditHandler) ListAuditEvents(w http.ResponseWriter, r *http.Request) { } } - events, total, err := h.svc.ListAuditEvents(page, perPage) + events, total, err := h.svc.ListAuditEvents(r.Context(), page, perPage) if err != nil { ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to list audit events", requestID) return @@ -83,7 +84,7 @@ func (h AuditHandler) GetAuditEvent(w http.ResponseWriter, r *http.Request) { } id = parts[0] - event, err := h.svc.GetAuditEvent(id) + event, err := h.svc.GetAuditEvent(r.Context(), id) if err != nil { ErrorWithRequestID(w, http.StatusNotFound, "Audit event not found", requestID) return diff --git a/internal/api/handler/audit_handler_test.go b/internal/api/handler/audit_handler_test.go index f11525e..7e3afb5 100644 --- a/internal/api/handler/audit_handler_test.go +++ b/internal/api/handler/audit_handler_test.go @@ -19,14 +19,14 @@ type mockAuditService struct { getFunc func(id string) (*domain.AuditEvent, error) } -func (m *mockAuditService) ListAuditEvents(page, perPage int) ([]domain.AuditEvent, int64, error) { +func (m *mockAuditService) ListAuditEvents(_ context.Context, page, perPage int) ([]domain.AuditEvent, int64, error) { if m.listFunc != nil { return m.listFunc(page, perPage) } return nil, 0, nil } -func (m *mockAuditService) GetAuditEvent(id string) (*domain.AuditEvent, error) { +func (m *mockAuditService) GetAuditEvent(_ context.Context, id string) (*domain.AuditEvent, error) { if m.getFunc != nil { return m.getFunc(id) } diff --git a/internal/api/handler/job_handler_test.go b/internal/api/handler/job_handler_test.go index f1ab8bc..b52fef0 100644 --- a/internal/api/handler/job_handler_test.go +++ b/internal/api/handler/job_handler_test.go @@ -1,6 +1,7 @@ package handler import ( + "context" "encoding/json" "fmt" "net/http" @@ -21,35 +22,35 @@ type MockJobService struct { RejectJobFn func(id string, reason string) error } -func (m *MockJobService) ListJobs(status, jobType string, page, perPage int) ([]domain.Job, int64, error) { +func (m *MockJobService) ListJobs(_ context.Context, status, jobType string, page, perPage int) ([]domain.Job, int64, error) { if m.ListJobsFn != nil { return m.ListJobsFn(status, jobType, page, perPage) } return nil, 0, nil } -func (m *MockJobService) GetJob(id string) (*domain.Job, error) { +func (m *MockJobService) GetJob(_ context.Context, id string) (*domain.Job, error) { if m.GetJobFn != nil { return m.GetJobFn(id) } return nil, nil } -func (m *MockJobService) CancelJob(id string) error { +func (m *MockJobService) CancelJob(_ context.Context, id string) error { if m.CancelJobFn != nil { return m.CancelJobFn(id) } return nil } -func (m *MockJobService) ApproveJob(id string) error { +func (m *MockJobService) ApproveJob(_ context.Context, id string) error { if m.ApproveJobFn != nil { return m.ApproveJobFn(id) } return nil } -func (m *MockJobService) RejectJob(id string, reason string) error { +func (m *MockJobService) RejectJob(_ context.Context, id string, reason string) error { if m.RejectJobFn != nil { return m.RejectJobFn(id, reason) } diff --git a/internal/api/handler/jobs.go b/internal/api/handler/jobs.go index 787c78f..4c78935 100644 --- a/internal/api/handler/jobs.go +++ b/internal/api/handler/jobs.go @@ -1,6 +1,7 @@ package handler import ( + "context" "encoding/json" "io" "net/http" @@ -13,11 +14,11 @@ import ( // JobService defines the service interface for job operations. type JobService interface { - ListJobs(status, jobType string, page, perPage int) ([]domain.Job, int64, error) - GetJob(id string) (*domain.Job, error) - CancelJob(id string) error - ApproveJob(id string) error - RejectJob(id string, reason string) error + ListJobs(ctx context.Context, status, jobType string, page, perPage int) ([]domain.Job, int64, error) + GetJob(ctx context.Context, id string) (*domain.Job, error) + CancelJob(ctx context.Context, id string) error + ApproveJob(ctx context.Context, id string) error + RejectJob(ctx context.Context, id string, reason string) error } // JobHandler handles HTTP requests for job operations. @@ -57,7 +58,7 @@ func (h JobHandler) ListJobs(w http.ResponseWriter, r *http.Request) { } } - jobs, total, err := h.svc.ListJobs(status, jobType, page, perPage) + jobs, total, err := h.svc.ListJobs(r.Context(), status, jobType, page, perPage) if err != nil { ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to list jobs", requestID) return @@ -91,7 +92,7 @@ func (h JobHandler) GetJob(w http.ResponseWriter, r *http.Request) { } id = parts[0] - job, err := h.svc.GetJob(id) + job, err := h.svc.GetJob(r.Context(), id) if err != nil { ErrorWithRequestID(w, http.StatusNotFound, "Job not found", requestID) return @@ -119,7 +120,7 @@ func (h JobHandler) CancelJob(w http.ResponseWriter, r *http.Request) { } jobID := parts[0] - if err := h.svc.CancelJob(jobID); err != nil { + if err := h.svc.CancelJob(r.Context(), jobID); err != nil { ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to cancel job", requestID) return } @@ -149,7 +150,7 @@ func (h JobHandler) ApproveJob(w http.ResponseWriter, r *http.Request) { } jobID := parts[0] - if err := h.svc.ApproveJob(jobID); err != nil { + if err := h.svc.ApproveJob(r.Context(), jobID); err != nil { if strings.Contains(err.Error(), "not found") { ErrorWithRequestID(w, http.StatusNotFound, "Job not found", requestID) return @@ -193,7 +194,7 @@ func (h JobHandler) RejectJob(w http.ResponseWriter, r *http.Request) { } } - if err := h.svc.RejectJob(jobID, body.Reason); err != nil { + if err := h.svc.RejectJob(r.Context(), jobID, body.Reason); err != nil { if strings.Contains(err.Error(), "not found") { ErrorWithRequestID(w, http.StatusNotFound, "Job not found", requestID) return diff --git a/internal/api/handler/notification_handler_test.go b/internal/api/handler/notification_handler_test.go index f50fd39..083f025 100644 --- a/internal/api/handler/notification_handler_test.go +++ b/internal/api/handler/notification_handler_test.go @@ -1,6 +1,7 @@ package handler import ( + "context" "encoding/json" "net/http" "net/http/httptest" @@ -17,21 +18,21 @@ type MockNotificationService struct { MarkAsReadFn func(id string) error } -func (m *MockNotificationService) ListNotifications(page, perPage int) ([]domain.NotificationEvent, int64, error) { +func (m *MockNotificationService) ListNotifications(_ context.Context, page, perPage int) ([]domain.NotificationEvent, int64, error) { if m.ListNotificationsFn != nil { return m.ListNotificationsFn(page, perPage) } return nil, 0, nil } -func (m *MockNotificationService) GetNotification(id string) (*domain.NotificationEvent, error) { +func (m *MockNotificationService) GetNotification(_ context.Context, id string) (*domain.NotificationEvent, error) { if m.GetNotificationFn != nil { return m.GetNotificationFn(id) } return nil, nil } -func (m *MockNotificationService) MarkAsRead(id string) error { +func (m *MockNotificationService) MarkAsRead(_ context.Context, id string) error { if m.MarkAsReadFn != nil { return m.MarkAsReadFn(id) } diff --git a/internal/api/handler/notifications.go b/internal/api/handler/notifications.go index abcc51f..3426240 100644 --- a/internal/api/handler/notifications.go +++ b/internal/api/handler/notifications.go @@ -1,6 +1,7 @@ package handler import ( + "context" "net/http" "strconv" "strings" @@ -11,9 +12,9 @@ import ( // NotificationService defines the service interface for notification operations. type NotificationService interface { - ListNotifications(page, perPage int) ([]domain.NotificationEvent, int64, error) - GetNotification(id string) (*domain.NotificationEvent, error) - MarkAsRead(id string) error + ListNotifications(ctx context.Context, page, perPage int) ([]domain.NotificationEvent, int64, error) + GetNotification(ctx context.Context, id string) (*domain.NotificationEvent, error) + MarkAsRead(ctx context.Context, id string) error } // NotificationHandler handles HTTP requests for notification operations. @@ -50,7 +51,7 @@ func (h NotificationHandler) ListNotifications(w http.ResponseWriter, r *http.Re } } - notifications, total, err := h.svc.ListNotifications(page, perPage) + notifications, total, err := h.svc.ListNotifications(r.Context(), page, perPage) if err != nil { ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to list notifications", requestID) return @@ -84,7 +85,7 @@ func (h NotificationHandler) GetNotification(w http.ResponseWriter, r *http.Requ } id = parts[0] - notification, err := h.svc.GetNotification(id) + notification, err := h.svc.GetNotification(r.Context(), id) if err != nil { ErrorWithRequestID(w, http.StatusNotFound, "Notification not found", requestID) return @@ -112,7 +113,7 @@ func (h NotificationHandler) MarkAsRead(w http.ResponseWriter, r *http.Request) } notificationID := parts[0] - if err := h.svc.MarkAsRead(notificationID); err != nil { + if err := h.svc.MarkAsRead(r.Context(), notificationID); err != nil { ErrorWithRequestID(w, http.StatusInternalServerError, "Failed to mark notification as read", requestID) return } diff --git a/internal/service/audit.go b/internal/service/audit.go index 86357c6..8798896 100644 --- a/internal/service/audit.go +++ b/internal/service/audit.go @@ -110,7 +110,7 @@ func (s *AuditService) ListByAction(ctx context.Context, action string, from, to } // ListAuditEvents returns paginated audit events (handler interface method). -func (s *AuditService) ListAuditEvents(page, perPage int) ([]domain.AuditEvent, int64, error) { +func (s *AuditService) ListAuditEvents(ctx context.Context, page, perPage int) ([]domain.AuditEvent, int64, error) { if page < 1 { page = 1 } @@ -123,7 +123,7 @@ func (s *AuditService) ListAuditEvents(page, perPage int) ([]domain.AuditEvent, PerPage: perPage, } - events, err := s.auditRepo.List(context.Background(), filter) + events, err := s.auditRepo.List(ctx, filter) if err != nil { return nil, 0, fmt.Errorf("failed to list audit events: %w", err) } @@ -143,13 +143,13 @@ func (s *AuditService) ListAuditEvents(page, perPage int) ([]domain.AuditEvent, } // GetAuditEvent returns a single audit event (handler interface method). -func (s *AuditService) GetAuditEvent(id string) (*domain.AuditEvent, error) { +func (s *AuditService) GetAuditEvent(ctx context.Context, id string) (*domain.AuditEvent, error) { filter := &repository.AuditFilter{ ResourceID: id, PerPage: 1, } - events, err := s.auditRepo.List(context.Background(), filter) + events, err := s.auditRepo.List(ctx, filter) if err != nil { return nil, fmt.Errorf("failed to get audit event: %w", err) } diff --git a/internal/service/job.go b/internal/service/job.go index 89fc2d1..0c1124a 100644 --- a/internal/service/job.go +++ b/internal/service/job.go @@ -189,8 +189,8 @@ func (s *JobService) GetJobStatus(ctx context.Context, jobID string) (*domain.Jo return job, nil } -// CancelJobWithContext cancels a pending or running job. -func (s *JobService) CancelJobWithContext(ctx context.Context, jobID string) error { +// CancelJob cancels a pending or running job (handler interface method). +func (s *JobService) CancelJob(ctx context.Context, jobID string) error { job, err := s.jobRepo.Get(ctx, jobID) if err != nil { return fmt.Errorf("failed to fetch job: %w", err) @@ -208,13 +208,8 @@ func (s *JobService) CancelJobWithContext(ctx context.Context, jobID string) err return nil } -// CancelJob cancels a job (handler interface method). -func (s *JobService) CancelJob(id string) error { - return s.CancelJobWithContext(context.Background(), id) -} - // ListJobs returns paginated jobs with optional filtering (handler interface method). -func (s *JobService) ListJobs(status, jobType string, page, perPage int) ([]domain.Job, int64, error) { +func (s *JobService) ListJobs(ctx context.Context, status, jobType string, page, perPage int) ([]domain.Job, int64, error) { if page < 1 { page = 1 } @@ -222,7 +217,7 @@ func (s *JobService) ListJobs(status, jobType string, page, perPage int) ([]doma perPage = 50 } - allJobs, err := s.jobRepo.List(context.Background()) + allJobs, err := s.jobRepo.List(ctx) if err != nil { return nil, 0, fmt.Errorf("failed to list jobs: %w", err) } @@ -263,14 +258,13 @@ func (s *JobService) ListJobs(status, jobType string, page, perPage int) ([]doma } // GetJob returns a single job (handler interface method). -func (s *JobService) GetJob(id string) (*domain.Job, error) { - return s.jobRepo.Get(context.Background(), id) +func (s *JobService) GetJob(ctx context.Context, id string) (*domain.Job, error) { + return s.jobRepo.Get(ctx, id) } // ApproveJob approves a renewal job that is awaiting approval. // Transitions the job from AwaitingApproval to Pending so the scheduler picks it up. -func (s *JobService) ApproveJob(id string) error { - ctx := context.Background() +func (s *JobService) ApproveJob(ctx context.Context, id string) error { job, err := s.jobRepo.Get(ctx, id) if err != nil { return fmt.Errorf("job not found: %w", err) @@ -290,8 +284,7 @@ func (s *JobService) ApproveJob(id string) error { // RejectJob rejects a renewal job that is awaiting approval. // Transitions the job to Cancelled with a rejection reason. -func (s *JobService) RejectJob(id string, reason string) error { - ctx := context.Background() +func (s *JobService) RejectJob(ctx context.Context, id string, reason string) error { job, err := s.jobRepo.Get(ctx, id) if err != nil { return fmt.Errorf("job not found: %w", err) diff --git a/internal/service/job_test.go b/internal/service/job_test.go index 8339897..931f78c 100644 --- a/internal/service/job_test.go +++ b/internal/service/job_test.go @@ -99,7 +99,7 @@ func TestCancelJob(t *testing.T) { jobService := newTestJobService(jobRepo) - err := jobService.CancelJobWithContext(ctx, "job-001") + err := jobService.CancelJob(ctx, "job-001") if err != nil { t.Fatalf("CancelJob failed: %v", err) } @@ -129,13 +129,15 @@ func TestCancelJob_AlreadyCompleted(t *testing.T) { jobService := newTestJobService(jobRepo) - err := jobService.CancelJobWithContext(ctx, "job-001") + err := jobService.CancelJob(ctx, "job-001") if err == nil { t.Fatal("expected error for completed job") } } func TestGetJob(t *testing.T) { + ctx := context.Background() + now := time.Now() job := &domain.Job{ ID: "job-001", @@ -153,7 +155,7 @@ func TestGetJob(t *testing.T) { jobService := newTestJobService(jobRepo) - retrieved, err := jobService.GetJob("job-001") + retrieved, err := jobService.GetJob(ctx, "job-001") if err != nil { t.Fatalf("GetJob failed: %v", err) } @@ -167,6 +169,8 @@ func TestGetJob(t *testing.T) { } func TestListJobs(t *testing.T) { + ctx := context.Background() + now := time.Now() job1 := &domain.Job{ ID: "job-001", @@ -192,7 +196,7 @@ func TestListJobs(t *testing.T) { jobService := newTestJobService(jobRepo) - jobs, total, err := jobService.ListJobs("", "", 1, 50) + jobs, total, err := jobService.ListJobs(ctx, "", "", 1, 50) if err != nil { t.Fatalf("ListJobs failed: %v", err) } @@ -206,6 +210,8 @@ func TestListJobs(t *testing.T) { } func TestListJobs_FilterByStatus(t *testing.T) { + ctx := context.Background() + now := time.Now() job1 := &domain.Job{ ID: "job-001", @@ -231,7 +237,7 @@ func TestListJobs_FilterByStatus(t *testing.T) { jobService := newTestJobService(jobRepo) - jobs, total, err := jobService.ListJobs(string(domain.JobStatusPending), "", 1, 50) + jobs, total, err := jobService.ListJobs(ctx, string(domain.JobStatusPending), "", 1, 50) if err != nil { t.Fatalf("ListJobs failed: %v", err) } diff --git a/internal/service/notification.go b/internal/service/notification.go index d1f08fe..7934b51 100644 --- a/internal/service/notification.go +++ b/internal/service/notification.go @@ -319,7 +319,7 @@ func (s *NotificationService) GetNotificationHistory(ctx context.Context, certID } // ListNotifications returns paginated notifications (handler interface method). -func (s *NotificationService) ListNotifications(page, perPage int) ([]domain.NotificationEvent, int64, error) { +func (s *NotificationService) ListNotifications(ctx context.Context, page, perPage int) ([]domain.NotificationEvent, int64, error) { if page < 1 { page = 1 } @@ -332,7 +332,7 @@ func (s *NotificationService) ListNotifications(page, perPage int) ([]domain.Not PerPage: perPage, } - notifications, err := s.notifRepo.List(context.Background(), filter) + notifications, err := s.notifRepo.List(ctx, filter) if err != nil { return nil, 0, fmt.Errorf("failed to list notifications: %w", err) } @@ -349,12 +349,12 @@ func (s *NotificationService) ListNotifications(page, perPage int) ([]domain.Not } // GetNotification returns a single notification (handler interface method). -func (s *NotificationService) GetNotification(id string) (*domain.NotificationEvent, error) { +func (s *NotificationService) GetNotification(ctx context.Context, id string) (*domain.NotificationEvent, error) { filter := &repository.NotificationFilter{ PerPage: 1, } - notifications, err := s.notifRepo.List(context.Background(), filter) + notifications, err := s.notifRepo.List(ctx, filter) if err != nil { return nil, fmt.Errorf("failed to get notification: %w", err) } @@ -370,6 +370,6 @@ func (s *NotificationService) GetNotification(id string) (*domain.NotificationEv } // MarkAsRead marks a notification as read (handler interface method). -func (s *NotificationService) MarkAsRead(id string) error { - return s.notifRepo.UpdateStatus(context.Background(), id, "read", time.Now()) +func (s *NotificationService) MarkAsRead(ctx context.Context, id string) error { + return s.notifRepo.UpdateStatus(ctx, id, "read", time.Now()) } diff --git a/internal/service/notification_test.go b/internal/service/notification_test.go index 870ea77..5d4273b 100644 --- a/internal/service/notification_test.go +++ b/internal/service/notification_test.go @@ -370,7 +370,7 @@ func TestListNotifications(t *testing.T) { } // List with pagination - notifs, total, err := svc.ListNotifications(1, 3) + notifs, total, err := svc.ListNotifications(context.Background(), 1, 3) if err != nil { t.Fatalf("ListNotifications failed: %v", err) } @@ -404,7 +404,7 @@ func TestMarkAsRead(t *testing.T) { notifRepo.AddNotification(notif) // Mark as read - err := svc.MarkAsRead(notif.ID) + err := svc.MarkAsRead(context.Background(), notif.ID) if err != nil { t.Fatalf("MarkAsRead failed: %v", err) } @@ -434,7 +434,7 @@ func TestGetNotification(t *testing.T) { notifRepo.AddNotification(notif) // Get the notification - retrieved, err := svc.GetNotification(notif.ID) + retrieved, err := svc.GetNotification(context.Background(), notif.ID) if err != nil { t.Fatalf("GetNotification failed: %v", err) }