Skip to content

Commit ac30f8a

Browse files
committed
fix: address PR review comments
- orderIndex in createMilestoneRequest changed to *int so explicit 0 is distinguishable from an omitted field; inline milestone loop in Create handler falls back to slice position when nil - overdueOnly filter now includes status='overdue' in the IN clause so items already persisted with that status are not silently excluded - removed blank identifier var _ = relational.SystemSecurityPlan{} and the relational import from models.go (import only needed in service.go) - added DB-level FK absence comments on all four link tables explaining the intentional cross-bounded-context design decision - added SSP-scoped routes /system-security-plans/:sspId/poam-items via RegisterSSPScoped; List and Create handlers inject the path param into filters/body automatically so clients don't repeat the SSP ID - regenerated Swagger docs
1 parent a099859 commit ac30f8a

File tree

8 files changed

+69
-13
lines changed

8 files changed

+69
-13
lines changed

docs/docs.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25776,6 +25776,7 @@ const docTemplate = `{
2577625776
"type": "string"
2577725777
},
2577825778
"orderIndex": {
25779+
"description": "OrderIndex is a pointer so that clients can explicitly set 0 without it\nbeing indistinguishable from an omitted field.",
2577925780
"type": "integer"
2578025781
},
2578125782
"scheduledCompletionDate": {

docs/swagger.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25770,6 +25770,7 @@
2577025770
"type": "string"
2577125771
},
2577225772
"orderIndex": {
25773+
"description": "OrderIndex is a pointer so that clients can explicitly set 0 without it\nbeing indistinguishable from an omitted field.",
2577325774
"type": "integer"
2577425775
},
2577525776
"scheduledCompletionDate": {

docs/swagger.yaml

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1522,6 +1522,9 @@ definitions:
15221522
description:
15231523
type: string
15241524
orderIndex:
1525+
description: |-
1526+
OrderIndex is a pointer so that clients can explicitly set 0 without it
1527+
being indistinguishable from an omitted field.
15251528
type: integer
15261529
scheduledCompletionDate:
15271530
type: string

internal/api/handler/api.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,9 +51,15 @@ func RegisterHandlers(server *api.Server, logger *zap.SugaredLogger, db *gorm.DB
5151

5252
poamService := poamsvc.NewPoamService(db)
5353
poamHandler := NewPoamItemsHandler(poamService, logger)
54+
// Flat route: /api/poam-items (supports ?sspId= query filter)
5455
poamGroup := server.API().Group("/poam-items")
5556
poamGroup.Use(middleware.JWTMiddleware(config.JWTPublicKey))
5657
poamHandler.Register(poamGroup)
58+
// SSP-scoped route: /api/system-security-plans/:sspId/poam-items
59+
// The :sspId path param is automatically injected into list/create filters.
60+
sspPoamGroup := server.API().Group("/system-security-plans/:sspId/poam-items")
61+
sspPoamGroup.Use(middleware.JWTMiddleware(config.JWTPublicKey))
62+
poamHandler.RegisterSSPScoped(sspPoamGroup)
5763

5864
riskHandler := NewRiskHandler(logger, db)
5965
riskGroup := server.API().Group("/risks")

internal/api/handler/poam_items.go

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,17 @@ func NewPoamItemsHandler(svc *poamsvc.PoamService, sugar *zap.SugaredLogger) *Po
3030
// Register mounts all POAM routes onto the given Echo group. JWT middleware
3131
// is applied at the group level in api.go.
3232
func (h *PoamItemsHandler) Register(g *echo.Group) {
33+
h.registerRoutes(g)
34+
}
35+
36+
// RegisterSSPScoped mounts all POAM routes under an SSP-scoped group
37+
// (e.g. /system-security-plans/:sspId/poam-items). The :sspId path param is
38+
// extracted and injected into list/create filters automatically.
39+
func (h *PoamItemsHandler) RegisterSSPScoped(g *echo.Group) {
40+
h.registerRoutes(g)
41+
}
42+
43+
func (h *PoamItemsHandler) registerRoutes(g *echo.Group) {
3344
g.GET("", h.List)
3445
g.POST("", h.Create)
3546
g.GET("/:id", h.Get)
@@ -102,7 +113,9 @@ type createMilestoneRequest struct {
102113
Description string `json:"description"`
103114
Status string `json:"status"`
104115
ScheduledCompletionDate *time.Time `json:"scheduledCompletionDate"`
105-
OrderIndex int `json:"orderIndex"`
116+
// OrderIndex is a pointer so that clients can explicitly set 0 without it
117+
// being indistinguishable from an omitted field.
118+
OrderIndex *int `json:"orderIndex"`
106119
}
107120

108121
type updateMilestoneRequest struct {
@@ -276,6 +289,15 @@ func (h *PoamItemsHandler) List(c echo.Context) error {
276289
if err != nil {
277290
return c.JSON(http.StatusBadRequest, api.NewError(err))
278291
}
292+
// When mounted under /system-security-plans/:sspId/poam-items, the sspId
293+
// path param takes precedence over the query parameter.
294+
if sspIDParam := c.Param("sspId"); sspIDParam != "" {
295+
parsed, err := uuid.Parse(sspIDParam)
296+
if err != nil {
297+
return c.JSON(http.StatusBadRequest, api.NewError(fmt.Errorf("sspId path param must be a valid UUID")))
298+
}
299+
filters.SspID = &parsed
300+
}
279301
items, err := h.poamService.List(filters)
280302
if err != nil {
281303
return h.internalError(c, "failed to list poam items", err)
@@ -305,10 +327,14 @@ func (h *PoamItemsHandler) Create(c echo.Context) error {
305327
if err := c.Bind(&in); err != nil {
306328
return c.JSON(http.StatusBadRequest, api.NewError(err))
307329
}
330+
// When mounted under /system-security-plans/:sspId/poam-items, the sspId
331+
// path param overrides the body field so the client doesn't have to repeat it.
332+
if sspIDParam := c.Param("sspId"); sspIDParam != "" {
333+
in.SspID = sspIDParam
334+
}
308335
if err := c.Validate(&in); err != nil {
309336
return c.JSON(http.StatusBadRequest, api.NewError(err))
310337
}
311-
312338
sspID, err := uuid.Parse(in.SspID)
313339
if err != nil {
314340
return c.JSON(http.StatusBadRequest, api.NewError(fmt.Errorf("sspId must be a valid UUID")))
@@ -376,19 +402,25 @@ func (h *PoamItemsHandler) Create(c echo.Context) error {
376402
}
377403
params.ControlRefs = controlRefs
378404

379-
for _, mr := range in.Milestones {
405+
for i, mr := range in.Milestones {
380406
if mr.Title == "" {
381407
return c.JSON(http.StatusBadRequest, api.NewError(fmt.Errorf("milestone title is required")))
382408
}
383409
if mr.Status != "" && !poamsvc.MilestoneStatus(mr.Status).IsValid() {
384410
return c.JSON(http.StatusBadRequest, api.NewError(fmt.Errorf("invalid milestone status: %s", mr.Status)))
385411
}
412+
// When orderIndex is omitted (nil), fall back to the slice position so
413+
// ordering is still deterministic without requiring the client to set it.
414+
msOrderIdx := i
415+
if mr.OrderIndex != nil {
416+
msOrderIdx = *mr.OrderIndex
417+
}
386418
params.Milestones = append(params.Milestones, poamsvc.CreateMilestoneParams{
387419
Title: mr.Title,
388420
Description: mr.Description,
389421
Status: mr.Status,
390422
ScheduledCompletionDate: mr.ScheduledCompletionDate,
391-
OrderIndex: mr.OrderIndex,
423+
OrderIndex: msOrderIdx,
392424
})
393425
}
394426

@@ -626,12 +658,16 @@ func (h *PoamItemsHandler) AddMilestone(c echo.Context) error {
626658
if in.Status != "" && !poamsvc.MilestoneStatus(in.Status).IsValid() {
627659
return c.JSON(http.StatusBadRequest, api.NewError(fmt.Errorf("invalid milestone status: %s", in.Status)))
628660
}
661+
var orderIdx int
662+
if in.OrderIndex != nil {
663+
orderIdx = *in.OrderIndex
664+
}
629665
m, err := h.poamService.AddMilestone(id, poamsvc.CreateMilestoneParams{
630666
Title: in.Title,
631667
Description: in.Description,
632668
Status: in.Status,
633669
ScheduledCompletionDate: in.ScheduledCompletionDate,
634-
OrderIndex: in.OrderIndex,
670+
OrderIndex: orderIdx,
635671
})
636672
if err != nil {
637673
return h.internalError(c, "failed to add milestone", err)

internal/api/handler/poam_items_integration_test.go

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,9 @@ func (suite *PoamItemsApiIntegrationSuite) authedReq(method, path string, body [
6464
return rec, req
6565
}
6666

67+
// intPtr is a convenience helper that returns a pointer to an int literal.
68+
func intPtr(i int) *int { return &i }
69+
6770
// ensureSSP seeds a SystemSecurityPlan row so that the Create handler's
6871
// EnsureSSPExists check passes. The SSP record only needs an ID.
6972
func (suite *PoamItemsApiIntegrationSuite) ensureSSP(id uuid.UUID) {
@@ -137,8 +140,8 @@ func (suite *PoamItemsApiIntegrationSuite) TestCreate_WithMilestonesAndLinks() {
137140
Status: "open",
138141
SourceType: "risk-promotion",
139142
Milestones: []createMilestoneRequest{
140-
{Title: "Patch staging", Status: "planned", ScheduledCompletionDate: &due, OrderIndex: 0},
141-
{Title: "Patch production", Status: "planned", OrderIndex: 1},
143+
{Title: "Patch staging", Status: "planned", ScheduledCompletionDate: &due, OrderIndex: intPtr(0)},
144+
{Title: "Patch production", Status: "planned", OrderIndex: intPtr(1)},
142145
},
143146
}
144147
raw, _ := json.Marshal(body)
@@ -578,7 +581,7 @@ func (suite *PoamItemsApiIntegrationSuite) TestAddMilestone() {
578581
Description: "Deploy patched version to staging",
579582
Status: "planned",
580583
ScheduledCompletionDate: &due,
581-
OrderIndex: 0,
584+
OrderIndex: intPtr(0),
582585
}
583586
raw, _ := json.Marshal(body)
584587
rec, req := suite.authedReq(http.MethodPost, fmt.Sprintf("/api/poam-items/%s/milestones", item.ID), raw)

internal/service/relational/poam/models.go

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,6 @@ import (
44
"fmt"
55
"time"
66

7-
"github.com/compliance-framework/api/internal/service/relational"
87
"github.com/google/uuid"
98
"gorm.io/gorm"
109
)
@@ -150,6 +149,11 @@ func (m *PoamItemMilestone) BeforeCreate(_ *gorm.DB) error {
150149
// PoamItemRiskLink is the join table linking PoamItems to Risks.
151150
// Uses a composite primary key and OnDelete:CASCADE to match the Risk service
152151
// link table pattern (e.g., risk_evidence_links).
152+
//
153+
// Note: only the PoamItem side carries a DB-level FK constraint. The RiskID
154+
// column intentionally has no FK back to the risks table because Risks live in
155+
// a separate bounded context. Referential integrity on the Risk side is
156+
// enforced at the application layer (EnsureExists checks before link creation).
153157
type PoamItemRiskLink struct {
154158
PoamItemID uuid.UUID `gorm:"type:uuid;primaryKey" json:"poamItemId"`
155159
RiskID uuid.UUID `gorm:"type:uuid;primaryKey;index" json:"riskId"`
@@ -161,6 +165,7 @@ type PoamItemRiskLink struct {
161165
func (PoamItemRiskLink) TableName() string { return "ccf_poam_item_risk_links" }
162166

163167
// PoamItemEvidenceLink is the join table linking PoamItems to Evidence records.
168+
// EvidenceID has no DB-level FK (same cross-context reasoning as PoamItemRiskLink).
164169
type PoamItemEvidenceLink struct {
165170
PoamItemID uuid.UUID `gorm:"type:uuid;primaryKey" json:"poamItemId"`
166171
EvidenceID uuid.UUID `gorm:"type:uuid;primaryKey;index" json:"evidenceId"`
@@ -172,6 +177,7 @@ type PoamItemEvidenceLink struct {
172177
func (PoamItemEvidenceLink) TableName() string { return "ccf_poam_item_evidence_links" }
173178

174179
// PoamItemControlLink is the join table linking PoamItems to Controls.
180+
// CatalogID/ControlID have no DB-level FK (same cross-context reasoning as PoamItemRiskLink).
175181
type PoamItemControlLink struct {
176182
PoamItemID uuid.UUID `gorm:"type:uuid;primaryKey" json:"poamItemId"`
177183
CatalogID uuid.UUID `gorm:"type:uuid;primaryKey;index" json:"catalogId"`
@@ -184,6 +190,7 @@ type PoamItemControlLink struct {
184190
func (PoamItemControlLink) TableName() string { return "ccf_poam_item_control_links" }
185191

186192
// PoamItemFindingLink is the join table linking PoamItems to Findings.
193+
// FindingID has no DB-level FK (same cross-context reasoning as PoamItemRiskLink).
187194
type PoamItemFindingLink struct {
188195
PoamItemID uuid.UUID `gorm:"type:uuid;primaryKey" json:"poamItemId"`
189196
FindingID uuid.UUID `gorm:"type:uuid;primaryKey;index" json:"findingId"`
@@ -199,6 +206,3 @@ type ControlRef struct {
199206
CatalogID uuid.UUID `json:"catalogId"`
200207
ControlID string `json:"controlId"`
201208
}
202-
203-
// Ensure the relational package is imported (used for SSP existence checks in the service).
204-
var _ = relational.SystemSecurityPlan{}

internal/service/relational/poam/queries.go

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,9 @@ func ApplyFilters(query *gorm.DB, filters ListFilters) *gorm.DB {
3939
if filters.OverdueOnly {
4040
now := time.Now().UTC()
4141
q = q.Where(
42-
"ccf_poam_items.status IN ('open','in-progress') AND ccf_poam_items.planned_completion_date IS NOT NULL AND ccf_poam_items.planned_completion_date < ?",
42+
// Include 'overdue' in the filter so that items already persisted with
43+
// that status (a valid PoamItemStatus) are not silently excluded.
44+
"ccf_poam_items.status IN ('open','in-progress','overdue') AND ccf_poam_items.planned_completion_date IS NOT NULL AND ccf_poam_items.planned_completion_date < ?",
4345
now,
4446
)
4547
}

0 commit comments

Comments
 (0)