fix: auto-create diagram groupings on first diagram POST#415
Merged
Conversation
Creating a diagram under network-architecture, data-flow, or authorization-boundary returned 404 when the SSP's system characteristics lacked that grouping, and no endpoint could create it (PUT /system-characteristics deliberately omits the three groupings). This made 'Add Diagram' permanently fail in the UI for SSPs imported without the groupings (e.g. the ToDo demo SSP). The create-diagram handlers now create the missing grouping row, attached to the existing system characteristics, before inserting the diagram. A missing system-characteristics row still 404s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThree diagram creation endpoints (Authorization Boundary, Data Flow, Network Architecture) now auto-create missing OSCAL grouping records in the database instead of returning 404. Implementation, tests, and API documentation are updated consistently to reflect the new behavior. ChangesAuto-Create OSCAL Groupings for Diagram Endpoints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/api/handler/oscal/system_security_plans_test.go (1)
2483-2528: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a negative-path test for Authorization Boundary missing-grouping auto-create parity.
TestCreateNetworkArchitectureDiagram_NegativeandTestCreateDataFlowDiagram_Negativenow verify auto-creation when the grouping row is missing, butTestCreateAuthorizationBoundaryDiagram_Negativestill only checks invalid IDs/body. Please add the equivalent “missing grouping → POST returns 201 → GET returns grouping with created diagram” case for AB to lock in the same contract and prevent endpoint drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler/oscal/system_security_plans_test.go` around lines 2483 - 2528, Add a negative-path case in TestCreateAuthorizationBoundaryDiagram_Negative mirroring TestCreateNetworkArchitectureDiagram_Negative and TestCreateDataFlowDiagram_Negative: create an SSP via suite.createBasicSSP() that does NOT include SystemCharacteristics.AuthorizationBoundary, POST a Diagram (oscalTypes_1_1_3.Diagram) to the "/api/oscal/system-security-plans/{ssp.UUID}/system-characteristics/authorization-boundary/diagrams" endpoint using suite.createRequest() and server.E().ServeHTTP and assert http.StatusCreated (201), then perform a GET of the SSP and assert that SystemCharacteristics.AuthorizationBoundary was auto-created and contains the posted Diagram (matching UUID/Description); use the same request helpers (suite.createRequest, server.E().ServeHTTP) and assertion style as the other negative tests to keep parity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/swagger.json`:
- Line 16628: The swagger YAML is out of sync: docs/swagger.yaml is missing the
updated descriptions for Authorization Boundary and Network Architecture that
appear in docs/swagger.json and docs/docs.go (e.g., the “grouping if it does not
exist yet” text around the Authorization Boundary description at the
Authorization Boundary, Data Flow, and Network Architecture endpoints). Re-run
your Swagger generator (e.g., run swag init) to regenerate docs/swagger.yaml so
its description strings match docs/swagger.json and docs/docs.go, then verify
the Authorization Boundary, Data Flow, and Network Architecture operation
descriptions all include the “grouping if it does not exist yet” phrasing.
In `@internal/api/handler/oscal/system_security_plans.go`:
- Around line 603-610: The code is creating relational.NetworkArchitecture (na)
before validating the incoming request and is doing grouping + diagram creation
as separate writes; move the request binding and UUID validation (ctx.Bind and
any ID parsing) to occur before any DB mutation, then wrap the "ensure grouping
+ create diagram" sequence in a single DB transaction using h.db.Transaction so
both the grouping create and the na create use the transactional tx (e.g.,
replace h.db.Create calls with tx.Create inside the transaction closure), check
for existing grouping within the transaction and only create if missing to avoid
race/duplicate errors, and convert DB errors into appropriate HTTP responses
(handle unique-constraint conflicts vs internal errors); apply the same change
to the other two handlers referenced (around the other blocks at 848-855 and
1089-1096).
---
Outside diff comments:
In `@internal/api/handler/oscal/system_security_plans_test.go`:
- Around line 2483-2528: Add a negative-path case in
TestCreateAuthorizationBoundaryDiagram_Negative mirroring
TestCreateNetworkArchitectureDiagram_Negative and
TestCreateDataFlowDiagram_Negative: create an SSP via suite.createBasicSSP()
that does NOT include SystemCharacteristics.AuthorizationBoundary, POST a
Diagram (oscalTypes_1_1_3.Diagram) to the
"/api/oscal/system-security-plans/{ssp.UUID}/system-characteristics/authorization-boundary/diagrams"
endpoint using suite.createRequest() and server.E().ServeHTTP and assert
http.StatusCreated (201), then perform a GET of the SSP and assert that
SystemCharacteristics.AuthorizationBoundary was auto-created and contains the
posted Diagram (matching UUID/Description); use the same request helpers
(suite.createRequest, server.E().ServeHTTP) and assertion style as the other
negative tests to keep parity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 231613cb-992a-477b-98b1-4ea99678be42
📒 Files selected for processing (5)
docs/docs.godocs/swagger.jsondocs/swagger.yamlinternal/api/handler/oscal/system_security_plans.gointernal/api/handler/oscal/system_security_plans_test.go
| "/oscal/system-security-plans/{id}/system-characteristics/authorization-boundary/diagrams": { | ||
| "post": { | ||
| "description": "Creates a new Diagram under the Authorization Boundary of a System Security Plan.", | ||
| "description": "Creates a new Diagram under the Authorization Boundary of a System Security Plan. Creates the Authorization Boundary grouping if it does not exist yet.", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify consistency of auto-create descriptions across all three generated swagger artifacts.
echo "=== Checking swagger.json ==="
rg -n "Creates the Authorization Boundary grouping if it does not exist yet" docs/swagger.json
rg -n "Creates the Data Flow grouping if it does not exist yet" docs/swagger.json
rg -n "Creates the Network Architecture grouping if it does not exist yet" docs/swagger.json
echo -e "\n=== Checking swagger.yaml ==="
rg -n "Creates the Authorization Boundary grouping if it does not exist yet" docs/swagger.yaml
rg -n "Creates the Data Flow grouping if it does not exist yet" docs/swagger.yaml
rg -n "Creates the Network Architecture grouping if it does not exist yet" docs/swagger.yaml
echo -e "\n=== Checking docs.go ==="
rg -n "Creates the Authorization Boundary grouping if it does not exist yet" docs/docs.go
rg -n "Creates the Data Flow grouping if it does not exist yet" docs/docs.go
rg -n "Creates the Network Architecture grouping if it does not exist yet" docs/docs.goRepository: compliance-framework/api
Length of output: 1324
Fix Swagger YAML description inconsistency across generated artifacts
docs/swagger.jsonanddocs/docs.gocontain the updated descriptions for Authorization Boundary (16628), Data Flow (16893), and Network Architecture (17158).docs/swagger.yamlcontains only the updated Data Flow description (but is missing the Authorization Boundary and Network Architecture “grouping if it does not exist yet” descriptions).- Re-run the swagger generation (e.g.,
swag init) and ensuredocs/swagger.yamlis regenerated to match the other artifacts.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/swagger.json` at line 16628, The swagger YAML is out of sync:
docs/swagger.yaml is missing the updated descriptions for Authorization Boundary
and Network Architecture that appear in docs/swagger.json and docs/docs.go
(e.g., the “grouping if it does not exist yet” text around the Authorization
Boundary description at the Authorization Boundary, Data Flow, and Network
Architecture endpoints). Re-run your Swagger generator (e.g., run swag init) to
regenerate docs/swagger.yaml so its description strings match docs/swagger.json
and docs/docs.go, then verify the Authorization Boundary, Data Flow, and Network
Architecture operation descriptions all include the “grouping if it does not
exist yet” phrasing.
Source: Learnings
Comment on lines
+603
to
+610
| if ssp.SystemCharacteristics.ID == nil { | ||
| return ctx.JSON(http.StatusNotFound, api.NewError(fmt.Errorf("no system characteristics for system security plan %s", idParam))) | ||
| } | ||
| na = &relational.NetworkArchitecture{SystemCharacteristicsId: *ssp.SystemCharacteristics.ID} | ||
| if err := h.db.Create(na).Error; err != nil { | ||
| h.sugar.Errorf("Failed to create network architecture: %v", err) | ||
| return ctx.JSON(http.StatusInternalServerError, api.NewError(err)) | ||
| } |
There was a problem hiding this comment.
Make grouping+diagram creation atomic and validate input before mutating DB.
All three handlers can persist a new grouping before ctx.Bind/UUID validation runs, so malformed requests can return 400 after mutating state. They also perform grouping create and diagram create as separate writes, so concurrent “first diagram” calls can race into duplicate-group or unique-constraint 500 paths.
Wrap “ensure grouping + create diagram” in one transaction, and move request bind/validation before any write.
Suggested pattern (apply to all three handlers)
- // ensure/create grouping first
- if group == nil || group.ID == nil {
- if ssp.SystemCharacteristics.ID == nil { ...404... }
- group = &relational.<Grouping>{SystemCharacteristicsId: *ssp.SystemCharacteristics.ID}
- if err := h.db.Create(group).Error; err != nil { ...500... }
- }
-
- // then bind/validate diagram
+ // bind/validate first (no side effects on 400)
+ var oscalDiag oscalTypes_1_1_3.Diagram
+ if err := ctx.Bind(&oscalDiag); err != nil { ...400... }
+ if oscalDiag.UUID == "" { ...400... }
+ if _, err := uuid.Parse(oscalDiag.UUID); err != nil { ...400... }
+
+ err = h.db.Transaction(func(tx *gorm.DB) error {
+ // idempotent ensure-grouping (conflict-safe)
+ // then create diagram under resolved grouping ID
+ // return error to rollback both on any failure
+ return nil
+ })
+ if err != nil { ...500... }Also applies to: 848-855, 1089-1096
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/handler/oscal/system_security_plans.go` around lines 603 - 610,
The code is creating relational.NetworkArchitecture (na) before validating the
incoming request and is doing grouping + diagram creation as separate writes;
move the request binding and UUID validation (ctx.Bind and any ID parsing) to
occur before any DB mutation, then wrap the "ensure grouping + create diagram"
sequence in a single DB transaction using h.db.Transaction so both the grouping
create and the na create use the transactional tx (e.g., replace h.db.Create
calls with tx.Create inside the transaction closure), check for existing
grouping within the transaction and only create if missing to avoid
race/duplicate errors, and convert DB errors into appropriate HTTP responses
(handle unique-constraint conflicts vs internal errors); apply the same change
to the other two handlers referenced (around the other blocks at 848-855 and
1089-1096).
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
1 participant
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.
Problem
POST /api/oscal/system-security-plans/{id}/system-characteristics/{network-architecture|data-flow|authorization-boundary}/diagramsreturns 404 when the SSP's system characteristics lack that grouping — and nothing can create the grouping:PUT /system-characteristicsdeliberatelyOmits all three. So for SSPs imported without a grouping (e.g. the ToDo demo SSP, which only has an authorization boundary), the UI's Add Diagram button can never succeed:Fix
The three create-diagram handlers now create the missing grouping row (attached to the existing system characteristics) before inserting the diagram. A missing system-characteristics row still 404s. Godoc/swagger updated.
Tests
TestCreateNetworkArchitectureDiagram_Negative/TestCreateDataFlowDiagram_Negative: the missing-grouping case now asserts 201 + a follow-up GET returns the grouping containing the diagram (previously asserted 404).make reviewablegreen locally (swag + lint + full integration suite).🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Bug Fixes