feat(BCH-1106): local-definitions routes for POAM #315

gusfcarvalho · 2026-01-24T12:51:40Z

No description provided.

Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>

Copilot

Pull request overview

This pull request adds CRUD API routes for local definitions in POAM (Plan of Action and Milestones) documents. The changes refactor local definitions from a simple JSON field to a proper database table with foreign key relationships, enabling individual local definitions to be created, retrieved, updated, and deleted independently.

Changes:

Refactored PlanOfActionAndMilestonesLocalDefinitions from a JSONType field to a separate database table with foreign key relationship
Added four new API endpoints: POST, GET (single), PUT, and DELETE for local definitions
Removed AssessmentAssets field from local definitions structure
Added comprehensive integration tests for all CRUD operations
Updated Swagger documentation for the new endpoints

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 22 comments.

Show a summary per file

File	Description
internal/service/relational/plan_of_action_and_milestones.go	Refactored LocalDefinitions from JSONType to separate table; removed AssessmentAssets field; updated marshaling logic to merge multiple definitions
internal/service/relational/plan_of_action_and_milestones_test.go	Removed unit test for old LocalDefinitions implementation
internal/service/relational/tests/plan_of_action_and_milestones_integration_test.go	Updated test data to use array of LocalDefinitions instead of JSONType
internal/api/handler/oscal/plan_of_action_and_milestones.go	Added CRUD endpoints and validation for local definitions
internal/api/handler/oscal/plan_of_action_and_milestones_integration_test.go	Added comprehensive integration tests for all local definitions CRUD operations
docs/swagger.yaml, docs/swagger.json, docs/docs.go	Updated API documentation with new local definitions endpoints

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-01-24T12:58:36Z

internal/service/relational/plan_of_action_and_milestones.go

+	if len(p.LocalDefinitions) > 0 {
+		// Combine all local definitions into one OSCAL structure
+		var allComponents []oscalTypes_1_1_3.SystemComponent
+		var allInventoryItems []oscalTypes_1_1_3.InventoryItem
+		var remarks string
+
+		for _, ld := range p.LocalDefinitions {
+			if len(ld.Components) > 0 {
+				allComponents = append(allComponents, ld.Components...)
+			}
+			if len(ld.InventoryItems) > 0 {
+				allInventoryItems = append(allInventoryItems, ld.InventoryItems...)
+			}
+			if ld.Remarks != "" {
+				remarks = ld.Remarks
+			}
+		}


Potential data loss issue: When there are multiple LocalDefinitions with different Remarks, only the last one's Remarks will be preserved. Consider concatenating remarks from all definitions or choosing a more explicit merge strategy.

This same pattern appears in the GetLocalDefinitions handler (lines 891-906 in the handler file), which means the data loss happens during both marshaling and retrieval operations.

Copilot · 2026-01-24T12:58:36Z

internal/service/relational/plan_of_action_and_milestones.go

 type PlanOfActionAndMilestonesLocalDefinitions struct {
-	AssessmentAssets datatypes.JSONType[oscalTypes_1_1_3.AssessmentAssets] `json:"assessment-assets"`
-	Components       datatypes.JSONSlice[oscalTypes_1_1_3.SystemComponent] `json:"components" gorm:"type:json"`
-	InventoryItems   datatypes.JSONSlice[oscalTypes_1_1_3.InventoryItem]   `json:"inventory-items" gorm:"type:json"`
-	Remarks          string                                                `json:"remarks"`
+	UUIDModel
+	PlanOfActionAndMilestonesID uuid.UUID                                             `gorm:"index"`
+	Remarks                     string                                                `json:"remarks"`
+	Components                  datatypes.JSONSlice[oscalTypes_1_1_3.SystemComponent] `json:"components" gorm:"type:json"`
+	InventoryItems              datatypes.JSONSlice[oscalTypes_1_1_3.InventoryItem]   `json:"inventory-items" gorm:"type:json"`
 }


Missing field: The AssessmentAssets field has been removed from PlanOfActionAndMilestonesLocalDefinitions. The old implementation stored this field as AssessmentAssets datatypes.JSONType[oscalTypes_1_1_3.AssessmentAssets] and the removed unit test (TestPlanOfActionAndMilestonesLocalDefinitions_MarshalUnmarshalOscal) included test coverage for this field. If the OSCAL specification includes AssessmentAssets in PlanOfActionAndMilestonesLocalDefinitions, this field should be added back to avoid data loss when importing OSCAL documents that contain this field.

Copilot · 2026-01-24T12:58:36Z

docs/swagger.yaml

+        name: id
+        required: true
+        type: string
+      - description: Definition ID (placeholder for consistency, not used in implementation)


Misleading parameter description: The documentation states "Definition ID (placeholder for consistency, not used in implementation)" but the definitionId parameter is actually used in the implementation to identify which specific local definition to update. This description should be updated to accurately reflect that the parameter is actively used.

Copilot · 2026-01-24T12:58:37Z

internal/api/handler/oscal/plan_of_action_and_milestones.go

+//	@Accept			json
+//	@Produce		json
+//	@Param			id					path		string														true	"POA&M ID"
+//	@Param			definitionId		path		string														true	"Definition ID (placeholder for consistency, not used in implementation)"


Misleading parameter description: The documentation states "Definition ID (placeholder for consistency, not used in implementation)" but the definitionId parameter is actually used in the implementation at line 2300 and line 2325. The parameter is parsed and used to identify which specific local definition to update. This description should be updated to accurately reflect that the parameter is actively used to identify the local definition being updated.

Suggested change

// @Param definitionId path string true "Definition ID (placeholder for consistency, not used in implementation)"

// @Param definitionId path string true "Local definition ID to update"

Copilot · 2026-01-24T12:58:37Z

internal/api/handler/oscal/plan_of_action_and_milestones.go

+//	@Description	Deletes local definitions for a given POA&M.
+//	@Tags			Plan Of Action and Milestones
+//	@Param			id				path	string	true	"POA&M ID"
+//	@Param			definitionId	path	string	true	"Definition ID (placeholder for consistency, not used in implementation)"


Misleading parameter description: The documentation states "Definition ID (placeholder for consistency, not used in implementation)" but the definitionId parameter is actually used in the implementation at line 2412 and line 2424. The parameter is parsed and used to identify which specific local definition to delete. This description should be updated to accurately reflect that the parameter is actively used.

Copilot · 2026-01-24T12:58:40Z

internal/api/handler/oscal/plan_of_action_and_milestones.go

+	var poam relational.PlanOfActionAndMilestones
+	if err := h.db.First(&poam, "id = ?", id).Error; err != nil {
+		h.sugar.Errorw("failed to get poam", "error", err)
+		return ctx.JSON(http.StatusNotFound, api.NewError(err))
+	}
+


Unnecessary database query: The POAM entity retrieved at line 2257 is never used. Since verifyPoamExists at line 2240 already confirms the POAM exists, this query can be removed entirely.

Suggested change

var poam relational.PlanOfActionAndMilestones

if err := h.db.First(&poam, "id = ?", id).Error; err != nil {

h.sugar.Errorw("failed to get poam", "error", err)

return ctx.JSON(http.StatusNotFound, api.NewError(err))

}

Copilot · 2026-01-24T12:58:40Z

docs/swagger.json

+                    },
+                    {
+                        "type": "string",
+                        "description": "Definition ID (placeholder for consistency, not used in implementation)",


Misleading parameter description: The documentation states "Definition ID (placeholder for consistency, not used in implementation)" but the definitionId parameter is actually used in the implementation to identify which specific local definition to retrieve, update, or delete. This description should be updated to accurately reflect that the parameter is actively used.

Suggested change

"description": "Definition ID (placeholder for consistency, not used in implementation)",

"description": "Definition ID used to identify the specific local definition to update",

Copilot · 2026-01-24T12:58:40Z

docs/swagger.json

+                    },
+                    {
+                        "type": "string",
+                        "description": "Definition ID (placeholder for consistency, not used in implementation)",


Misleading parameter description: The documentation states "Definition ID (placeholder for consistency, not used in implementation)" but the definitionId parameter is actually used in the implementation to identify which specific local definition to retrieve, update, or delete. This description should be updated to accurately reflect that the parameter is actively used.

Copilot · 2026-01-24T12:58:40Z

internal/api/handler/oscal/plan_of_action_and_milestones_integration_test.go

+	// Try to get single local definitions that don't exist
+	getSingleRec, getSingleReq := suite.createRequest(http.MethodGet, fmt.Sprintf("/api/oscal/plan-of-action-and-milestones/%s/local-definitions/test-id", poamUUID), nil)
+	suite.server.E().ServeHTTP(getSingleRec, getSingleReq)
+	suite.Equal(http.StatusNotFound, getSingleRec.Code)
+
+	// Try to delete local definitions that don't exist
+	deleteRec, deleteReq := suite.createRequest(http.MethodDelete, fmt.Sprintf("/api/oscal/plan-of-action-and-milestones/%s/local-definitions/test-id", poamUUID), nil)
+	suite.server.E().ServeHTTP(deleteRec, deleteReq)
+	suite.Equal(http.StatusNotFound, deleteRec.Code)


Test expects incorrect HTTP status code: The test uses "test-id" (which is not a valid UUID) as the definitionId parameter and expects HTTP 404. However, the handler at lines 2368-2372 will return HTTP 400 (BadRequest) when UUID parsing fails, before it ever tries to find the definition in the database. This test will fail. The same issue exists on line 3905.

Copilot · 2026-01-24T12:58:40Z

docs/swagger.json

+        },
+        "/oscal/plan-of-action-and-milestones/{id}/local-definitions/{definitionId}": {
+            "get": {
+                "description": "Retrieves local definitions for a given POA\u0026M (same as GET /local-definitions but with consistent routing).",


Inaccurate endpoint description: The description states "Retrieves local definitions for a given POA&M (same as GET /local-definitions but with consistent routing)" but this is incorrect. This endpoint retrieves a SPECIFIC local definition by its ID, while the GET /local-definitions endpoint (without definitionId) retrieves ALL local definitions for the POAM and merges them into a single response. The description should be updated to clarify that this endpoint retrieves a single specific local definition.

gusfcarvalho · 2026-01-26T10:18:24Z

Copilot actually saved a very dumb PR to be merged. Thanks

feat: local-definitions routes for POAM

289f864

Signed-off-by: Gustavo Carvalho <gustavo.carvalho@container-solutions.com>

Copilot AI review requested due to automatic review settings January 24, 2026 12:51

Copilot started reviewing on behalf of gusfcarvalho January 24, 2026 12:51 View session

gusfcarvalho changed the title ~~feat: local-definitions routes for POAM~~ feat(BCH-1106): local-definitions routes for POAM Jan 24, 2026

Copilot AI reviewed Jan 24, 2026

View reviewed changes

gusfcarvalho closed this Jan 26, 2026

gusfcarvalho deleted the gc-feat-poam-local-def-routes branch January 26, 2026 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(BCH-1106): local-definitions routes for POAM #315

feat(BCH-1106): local-definitions routes for POAM #315

Uh oh!

gusfcarvalho commented Jan 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

Copilot AI Jan 24, 2026

Uh oh!

gusfcarvalho commented Jan 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	// @Param definitionId path string true "Definition ID (placeholder for consistency, not used in implementation)"
	// @Param definitionId path string true "Local definition ID to update"

	"description": "Definition ID (placeholder for consistency, not used in implementation)",
	"description": "Definition ID used to identify the specific local definition to update",

feat(BCH-1106): local-definitions routes for POAM #315

feat(BCH-1106): local-definitions routes for POAM #315

Uh oh!

Conversation

gusfcarvalho commented Jan 24, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

gusfcarvalho commented Jan 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants