From 3f6b0aa99598f4b03bd068853b4224f7bdd143e0 Mon Sep 17 00:00:00 2001 From: shankar0123 Date: Sun, 15 Mar 2026 14:52:48 -0400 Subject: [PATCH] Fix Create methods: respect user-provided IDs and set timestamps All service-layer Create methods (team, owner, target, issuer, certificate) were unconditionally overwriting user-provided IDs with auto-generated ones and leaving CreatedAt/UpdatedAt as zero values. This caused three user-visible bugs: - POST /api/v1/teams with {"id": "t-demo"} returned a generated ID like "team-1773601137949154216" instead of "t-demo" - POST /api/v1/owners referencing the user-provided team_id failed with Internal Server Error (FK constraint on non-existent generated ID) - created_at/updated_at came back as "0001-01-01T00:00:00Z" Fix: all 9 affected Create methods (both context-aware and handler interface variants) now check if ID is empty before generating, and set timestamps to time.Now() if zero-valued. Follows the existing correct pattern in policy.go CreateRule/CreatePolicy. Also removes two stale temp files (audit.go.* and issuer.go.*) that were accidentally committed to the repo. Co-Authored-By: Claude Opus 4.6 --- internal/service/audit.go.4445065566393902048 | 161 ------------------ internal/service/certificate.go | 12 +- internal/service/issuer.go | 23 ++- .../service/issuer.go.4749230034325506546 | 116 ------------- internal/service/owner.go | 23 ++- internal/service/target.go | 23 ++- internal/service/team.go | 23 ++- 7 files changed, 95 insertions(+), 286 deletions(-) delete mode 100644 internal/service/audit.go.4445065566393902048 delete mode 100644 internal/service/issuer.go.4749230034325506546 diff --git a/internal/service/audit.go.4445065566393902048 b/internal/service/audit.go.4445065566393902048 deleted file mode 100644 index 7ce55d4..0000000 --- a/internal/service/audit.go.4445065566393902048 +++ /dev/null @@ -1,161 +0,0 @@ -package service - -import ( - "context" - "encoding/json" - "fmt" - "time" - - "github.com/shankar0123/certctl/internal/domain" - "github.com/shankar0123/certctl/internal/repository" -) - -// AuditService provides business logic for recording and retrieving audit events. -type AuditService struct { - auditRepo repository.AuditRepository -} - -// NewAuditService creates a new audit service. -func NewAuditService(auditRepo repository.AuditRepository) *AuditService { - return &AuditService{ - auditRepo: auditRepo, - } -} - -// RecordEvent records an audit event with actor, action, and resource information. -func (s *AuditService) RecordEvent(ctx context.Context, actor string, actorType domain.ActorType, action string, resourceType string, resourceID string, details map[string]interface{}) error { - detailsJSON, err := json.Marshal(details) - if err != nil { - detailsJSON = []byte("{}") - } - - event := &domain.AuditEvent{ - ID: generateID("audit"), - Timestamp: time.Now(), - Actor: actor, - ActorType: actorType, - Action: action, - ResourceType: resourceType, - ResourceID: resourceID, - Details: json.RawMessage(detailsJSON), - } - - if err := s.auditRepo.Create(ctx, event); err != nil { - return fmt.Errorf("failed to record audit event: %w", err) - } - - return nil -} - -// List returns audit events matching filter criteria. -func (s *AuditService) List(ctx context.Context, filter *repository.AuditFilter) ([]*domain.AuditEvent, error) { - events, err := s.auditRepo.List(ctx, filter) - if err != nil { - return nil, fmt.Errorf("failed to list audit events: %w", err) - } - return events, nil -} - -// ListByResource returns all audit events for a specific resource. -func (s *AuditService) ListByResource(ctx context.Context, resourceType string, resourceID string) ([]*domain.AuditEvent, error) { - filter := &repository.AuditFilter{ - ResourceType: resourceType, - ResourceID: resourceID, - PerPage: 1000, // reasonable default for single resource - } - - events, err := s.auditRepo.List(ctx, filter) - if err != nil { - return nil, fmt.Errorf("failed to list audit events: %w", err) - } - return events, nil -} - -// ListByActor returns all audit events for a specific actor. -func (s *AuditService) ListByActor(ctx context.Context, actor string) ([]*domain.AuditEvent, error) { - filter := &repository.AuditFilter{ - Actor: actor, - PerPage: 1000, - } - - events, err := s.auditRepo.List(ctx, filter) - if err != nil { - return nil, fmt.Errorf("failed to list audit events: %w", err) - } - return events, nil -} - -// ListByAction returns all audit events for a specific action type. -func (s *AuditService) ListByAction(ctx context.Context, action string, from, to time.Time) ([]*domain.AuditEvent, error) { - filter := &repository.AuditFilter{ - From: from, - To: to, - PerPage: 1000, - } - - events, err := s.auditRepo.List(ctx, filter) - if err != nil { - return nil, fmt.Errorf("failed to list audit events: %w", err) - } - - // Filter by action on client side (repository may not filter by action directly) - var filtered []*domain.AuditEvent - for _, e := range events { - if e.Action == action { - filtered = append(filtered, e) - } - } - - return filtered, nil -} - -// ListAuditEvents returns paginated audit events (handler interface method). -func (s *AuditService) ListAuditEvents(page, perPage int) ([]domain.AuditEvent, int64, error) { - if page < 1 { - page = 1 - } - if perPage < 1 { - perPage = 50 - } - - filter := &repository.AuditFilter{ - Offset: int64((page - 1) * perPage), - PerPage: int64(perPage), - } - - events, err := s.auditRepo.List(context.Background(), filter) - if err != nil { - return nil, 0, fmt.Errorf("failed to list audit events: %w", err) - } - - // Convert pointers to values for the handler interface - var result []domain.AuditEvent - for _, e := range events { - if e != nil { - result = append(result, *e) - } - } - - // TODO: Get total count from repository - total := int64(len(result)) - - return result, total, nil -} - -// GetAuditEvent returns a single audit event (handler interface method). -func (s *AuditService) GetAuditEvent(id string) (*domain.AuditEvent, error) { - filter := &repository.AuditFilter{ - ID: id, - } - - events, err := s.auditRepo.List(context.Background(), filter) - if err != nil { - return nil, fmt.Errorf("failed to get audit event: %w", err) - } - - if len(events) == 0 { - return nil, fmt.Errorf("audit event not found") - } - - return events[0], nil -} diff --git a/internal/service/certificate.go b/internal/service/certificate.go index 5c3545a..0769a3a 100644 --- a/internal/service/certificate.go +++ b/internal/service/certificate.go @@ -3,6 +3,7 @@ package service import ( "context" "fmt" + "time" "github.com/shankar0123/certctl/internal/domain" "github.com/shankar0123/certctl/internal/repository" @@ -254,7 +255,16 @@ func (s *CertificateService) GetCertificate(id string) (*domain.ManagedCertifica // CreateCertificate creates a new certificate (handler interface method). func (s *CertificateService) CreateCertificate(cert domain.ManagedCertificate) (*domain.ManagedCertificate, error) { - cert.ID = generateID("cert") + if cert.ID == "" { + cert.ID = generateID("cert") + } + now := time.Now() + if cert.CreatedAt.IsZero() { + cert.CreatedAt = now + } + if cert.UpdatedAt.IsZero() { + cert.UpdatedAt = now + } if err := s.certRepo.Create(context.Background(), &cert); err != nil { return nil, fmt.Errorf("failed to create certificate: %w", err) } diff --git a/internal/service/issuer.go b/internal/service/issuer.go index cb6e8a8..677f853 100644 --- a/internal/service/issuer.go +++ b/internal/service/issuer.go @@ -3,6 +3,7 @@ package service import ( "context" "fmt" + "time" "github.com/shankar0123/certctl/internal/domain" "github.com/shankar0123/certctl/internal/repository" @@ -65,7 +66,16 @@ func (s *IssuerService) Create(ctx context.Context, issuer *domain.Issuer, actor return fmt.Errorf("issuer name is required") } - issuer.ID = generateID("issuer") + if issuer.ID == "" { + issuer.ID = generateID("issuer") + } + now := time.Now() + if issuer.CreatedAt.IsZero() { + issuer.CreatedAt = now + } + if issuer.UpdatedAt.IsZero() { + issuer.UpdatedAt = now + } if err := s.issuerRepo.Create(ctx, issuer); err != nil { return fmt.Errorf("failed to create issuer: %w", err) } @@ -160,7 +170,16 @@ func (s *IssuerService) GetIssuer(id string) (*domain.Issuer, error) { // CreateIssuer creates a new issuer (handler interface method). func (s *IssuerService) CreateIssuer(issuer domain.Issuer) (*domain.Issuer, error) { - issuer.ID = generateID("issuer") + if issuer.ID == "" { + issuer.ID = generateID("issuer") + } + now := time.Now() + if issuer.CreatedAt.IsZero() { + issuer.CreatedAt = now + } + if issuer.UpdatedAt.IsZero() { + issuer.UpdatedAt = now + } if err := s.issuerRepo.Create(context.Background(), &issuer); err != nil { return nil, fmt.Errorf("failed to create issuer: %w", err) } diff --git a/internal/service/issuer.go.4749230034325506546 b/internal/service/issuer.go.4749230034325506546 deleted file mode 100644 index b642265..0000000 --- a/internal/service/issuer.go.4749230034325506546 +++ /dev/null @@ -1,116 +0,0 @@ -package service - -import ( - "context" - "fmt" - - "github.com/shankar0123/certctl/internal/domain" - "github.com/shankar0123/certctl/internal/repository" -) - -// IssuerService provides business logic for certificate issuer management. -type IssuerService struct { - issuerRepo repository.IssuerRepository - auditService *AuditService -} - -// NewIssuerService creates a new issuer service. -func NewIssuerService( - issuerRepo repository.IssuerRepository, - auditService *AuditService, -) *IssuerService { - return &IssuerService{ - issuerRepo: issuerRepo, - auditService: auditService, - } -} - -// List returns a paginated list of issuers. -func (s *IssuerService) List(ctx context.Context, page, perPage int) ([]*domain.Issuer, int64, error) { - if page < 1 { - page = 1 - } - if perPage < 1 { - perPage = 50 - } - - offset := int64((page - 1) * perPage) - issuers, total, err := s.issuerRepo.List(ctx, offset, int64(perPage)) - if err != nil { - return nil, 0, fmt.Errorf("failed to list issuers: %w", err) - } - return issuers, total, nil -} - -// Get retrieves an issuer by ID. -func (s *IssuerService) Get(ctx context.Context, id string) (*domain.Issuer, error) { - issuer, err := s.issuerRepo.Get(ctx, id) - if err != nil { - return nil, fmt.Errorf("failed to get issuer %s: %w", id, err) - } - return issuer, nil -} - -// Create validates and stores a new issuer. -func (s *IssuerService) Create(ctx context.Context, issuer *domain.Issuer, actor string) error { - if issuer.Name == "" { - return fmt.Errorf("issuer name is required") - } - - issuer.ID = generateID("issuer") - if err := s.issuerRepo.Create(ctx, issuer); err != nil { - return fmt.Errorf("failed to create issuer: %w", err) - } - - if s.auditService != nil { - _ = s.auditService.RecordEvent(ctx, actor, domain.ActorTypeUser, "create_issuer", "issuer", issuer.ID, nil) - } - - return nil -} - -// Update modifies an existing issuer. -func (s *IssuerService) Update(ctx context.Context, id string, issuer *domain.Issuer, actor string) error { - if issuer.Name == "" { - return fmt.Errorf("issuer name is required") - } - - issuer.ID = id - if err := s.issuerRepo.Update(ctx, issuer); err != nil { - return fmt.Errorf("failed to update issuer %s: %w", id, err) - } - - if s.auditService != nil { - _ = s.auditService.RecordEvent(ctx, actor, domain.ActorTypeUser, "update_issuer", "issuer", id, nil) - } - - return nil -} - -// Delete removes an issuer. -func (s *IssuerService) Delete(ctx context.Context, id string, actor string) error { - if err := s.issuerRepo.Delete(ctx, id); err != nil { - return fmt.Errorf("failed to delete issuer %s: %w", id, err) - } - - if s.auditService != nil { - _ = s.auditService.RecordEvent(ctx, actor, domain.ActorTypeUser, "delete_issuer", "issuer", id, nil) - } - - return nil -} - -// TestConnection verifies the issuer connection. -func (s *IssuerService) TestConnection(ctx context.Context, id string) error { - issuer, err := s.issuerRepo.Get(ctx, id) - if err != nil { - return fmt.Errorf("issuer not found: %w", err) - } - - // TODO: Implement actual connection test based on issuer type - if issuer == nil { - return fmt.Errorf("issuer not found") - } - - return nil -} diff --git a/internal/service/owner.go b/internal/service/owner.go index 1ee1d5a..95c4d70 100644 --- a/internal/service/owner.go +++ b/internal/service/owner.go @@ -3,6 +3,7 @@ package service import ( "context" "fmt" + "time" "github.com/shankar0123/certctl/internal/domain" "github.com/shankar0123/certctl/internal/repository" @@ -65,7 +66,16 @@ func (s *OwnerService) Create(ctx context.Context, owner *domain.Owner, actor st return fmt.Errorf("owner name is required") } - owner.ID = generateID("owner") + if owner.ID == "" { + owner.ID = generateID("owner") + } + now := time.Now() + if owner.CreatedAt.IsZero() { + owner.CreatedAt = now + } + if owner.UpdatedAt.IsZero() { + owner.UpdatedAt = now + } if err := s.ownerRepo.Create(ctx, owner); err != nil { return fmt.Errorf("failed to create owner: %w", err) } @@ -140,7 +150,16 @@ func (s *OwnerService) GetOwner(id string) (*domain.Owner, error) { // CreateOwner creates a new owner (handler interface method). func (s *OwnerService) CreateOwner(owner domain.Owner) (*domain.Owner, error) { - owner.ID = generateID("owner") + if owner.ID == "" { + owner.ID = generateID("owner") + } + now := time.Now() + if owner.CreatedAt.IsZero() { + owner.CreatedAt = now + } + if owner.UpdatedAt.IsZero() { + owner.UpdatedAt = now + } if err := s.ownerRepo.Create(context.Background(), &owner); err != nil { return nil, fmt.Errorf("failed to create owner: %w", err) } diff --git a/internal/service/target.go b/internal/service/target.go index 881e0b6..79396d4 100644 --- a/internal/service/target.go +++ b/internal/service/target.go @@ -3,6 +3,7 @@ package service import ( "context" "fmt" + "time" "github.com/shankar0123/certctl/internal/domain" "github.com/shankar0123/certctl/internal/repository" @@ -65,7 +66,16 @@ func (s *TargetService) Create(ctx context.Context, target *domain.DeploymentTar return fmt.Errorf("target name is required") } - target.ID = generateID("target") + if target.ID == "" { + target.ID = generateID("target") + } + now := time.Now() + if target.CreatedAt.IsZero() { + target.CreatedAt = now + } + if target.UpdatedAt.IsZero() { + target.UpdatedAt = now + } if err := s.targetRepo.Create(ctx, target); err != nil { return fmt.Errorf("failed to create target: %w", err) } @@ -140,7 +150,16 @@ func (s *TargetService) GetTarget(id string) (*domain.DeploymentTarget, error) { // CreateTarget creates a new target (handler interface method). func (s *TargetService) CreateTarget(target domain.DeploymentTarget) (*domain.DeploymentTarget, error) { - target.ID = generateID("target") + if target.ID == "" { + target.ID = generateID("target") + } + now := time.Now() + if target.CreatedAt.IsZero() { + target.CreatedAt = now + } + if target.UpdatedAt.IsZero() { + target.UpdatedAt = now + } if err := s.targetRepo.Create(context.Background(), &target); err != nil { return nil, fmt.Errorf("failed to create target: %w", err) } diff --git a/internal/service/team.go b/internal/service/team.go index d8eed30..de4fd3b 100644 --- a/internal/service/team.go +++ b/internal/service/team.go @@ -3,6 +3,7 @@ package service import ( "context" "fmt" + "time" "github.com/shankar0123/certctl/internal/domain" "github.com/shankar0123/certctl/internal/repository" @@ -65,7 +66,16 @@ func (s *TeamService) Create(ctx context.Context, team *domain.Team, actor strin return fmt.Errorf("team name is required") } - team.ID = generateID("team") + if team.ID == "" { + team.ID = generateID("team") + } + now := time.Now() + if team.CreatedAt.IsZero() { + team.CreatedAt = now + } + if team.UpdatedAt.IsZero() { + team.UpdatedAt = now + } if err := s.teamRepo.Create(ctx, team); err != nil { return fmt.Errorf("failed to create team: %w", err) } @@ -140,7 +150,16 @@ func (s *TeamService) GetTeam(id string) (*domain.Team, error) { // CreateTeam creates a new team (handler interface method). func (s *TeamService) CreateTeam(team domain.Team) (*domain.Team, error) { - team.ID = generateID("team") + if team.ID == "" { + team.ID = generateID("team") + } + now := time.Now() + if team.CreatedAt.IsZero() { + team.CreatedAt = now + } + if team.UpdatedAt.IsZero() { + team.UpdatedAt = now + } if err := s.teamRepo.Create(context.Background(), &team); err != nil { return nil, fmt.Errorf("failed to create team: %w", err) }