fix(oscal-import): upsert existing documents and fix nested-dir recursion#449
Conversation
…sion 'oscal import' used FirstOrCreate, which silently skips any document whose uuid already exists (while logging 'Successfully Created'). Content changes - new catalog controls, new implemented-requirements, edited statements - never reached the database on re-import, forcing API-side workarounds. Import now upserts: new uuids are created as before; existing documents are updated from the file with FullSaveAssociations, so nested rows (groups, controls, requirements, statements, by-components) are inserted or updated by primary key. Updates are additive - zero-value fields are skipped, so DB-managed columns absent from OSCAL (e.g. catalog Active) are preserved, and rows removed from a file are not deleted. Also fixes directory recursion: children were joined against the directory's base name instead of its opened path, so importing a directory containing a subdirectory panicked on the first nested file. Logs now say Created vs Updated, and log failures instead of unconditionally claiming success. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRefactors OSCAL import processing to use shared upsert and logging helpers, aggregate errors instead of stopping early, recover from panics during document import, and continue recursive directory traversal while reporting failures. ChangesOSCAL Import Refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant importOscal
participant importFile
participant importDocuments
participant upsertDocument
participant GORM
importOscal->>importFile: open and process paths
importFile->>importDocuments: recurse into nested entries
importDocuments->>upsertDocument: document UUID + OSCAL payload
upsertDocument->>GORM: lookup by UUID
alt record not found
upsertDocument->>GORM: create entity
else record found
upsertDocument->>GORM: FullSaveAssociations update
end
upsertDocument-->>importDocuments: importResult or error
importDocuments->>importDocuments: recover panic into error
importOscal->>importOscal: join all import/open errors
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the oscal import CLI to correctly handle re-importing OSCAL documents by updating existing DB rows (rather than silently skipping on UUID collisions) and fixes directory recursion so nested directories resolve using the opened directory path.
Changes:
- Replace
FirstOrCreatewith an upsert-style flow that creates new UUIDs and updates existing documents. - Fix recursive directory import to join children against
f.Name()(opened path) rather thaninfo.Name()(base name). - Improve import logging to distinguish
CreatedvsUpdatedand to log failures as errors.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmd/oscal/import.go`:
- Around line 112-122: `importFile` is swallowing upsert failures because
`logImport` only logs `res.err` while the caller still marks the import as
successful. Update the `logImport`/`importFile` flow so a failed upsert returns
the error instead of setting `imported = true`, and make the `Profile` branch
skip the sync path when the upsert result contains an error. Use the `logImport`
helper and the `Profile` branch in `importFile` to locate the fix.
🪄 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: 07fe02c5-8ff2-457a-8804-5f326a016ec4
📒 Files selected for processing (1)
cmd/oscal/import.go
… exit status Review follow-ups: - Updates+FullSaveAssociations does upsert nested rows (verified: edited by-component descriptions/statuses and new requirements land on re-import), but association rows without a stable OSCAL identifier - metadata and back-matter, whose ids are DB-generated - were duplicated on every re-import. reuseSingletonAssociations now points them at the existing rows so updates modify in place. - Failures now reach the exit status. logImport returns the error, importFile joins errors across documents and directory entries (continuing past bad files instead of aborting the batch), and importOscal exits non-zero when any document failed. A panic while unmarshalling a malformed file (the relational layer uses uuid.MustParse) is recovered per file and converted into an error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed both review comments in f820574: `Updates` + `FullSaveAssociations` and nested rows — tested against a live database rather than argued from docs: nested rows are upserted through `Updates` in `FullSaveAssociations` mode (this is GORM's documented pattern for updating association data). Edited by-component descriptions/statuses and newly added requirements all land on re-import. However, the test surfaced an adjacent real bug: association rows with no stable OSCAL identifier (metadata, back-matter — their ids are DB-generated) were duplicated on every re-import, since each unmarshal mints a fresh row. `reuseSingletonAssociations` now points them at the existing rows so updates modify in place. Verified: repeated re-imports keep exactly one metadata row, updated. Exit status on failure — agreed, fixed. `logImport` returns the error; `importFile` joins errors across documents and directory entries (continuing past bad files instead of aborting the batch — important for bulk `-f ` imports); `importOscal` exits non-zero when anything failed. Also: a malformed file used to panic the whole run (the relational layer uses `uuid.MustParse`) — that's now recovered per file and converted into an error. Verified: a directory with one bad + one good file imports the good one and exits 1; an all-good run exits 0.🤖 Generated with Claude Code |
| if errs != nil { | ||
| sugar.Fatalw("Import finished with errors", "error", errs) | ||
| } |
| systemFile, err := os.Open(f) | ||
| if err != nil { | ||
| panic(err) | ||
| errs = errors.Join(errs, err) | ||
| sugar.Errorw("Failed to open import path", "path", f, "error", err) | ||
| continue |
| @@ -104,16 +217,31 @@ func importFile(db *gorm.DB, sugar *zap.SugaredLogger, f *os.File) error { | |||
| } | |||
| if err := json.NewDecoder(f).Decode(input); err != nil { | ||
| sugar.Error(err) | ||
| } |
What
Two fixes to the
oscal importCLI:Re-imports now update existing documents. Import used
FirstOrCreate, which silently skips any document whose uuid already exists — while logging "Successfully Created". Content changes (new catalog controls, new implemented-requirements, edited statements/by-components) never reached the database on re-import. Import now upserts: new uuids are created as before; existing documents are updated from the file withFullSaveAssociations, so nested rows are inserted or updated by primary key.Directory recursion no longer panics on subdirectories. Children were joined against the directory's base name (
info.Name()) instead of the path it was opened with (f.Name()), so importing a directory that contains a subdirectory panicked on the first nested file unless the CWD happened to contain that subdirectory.Logging now distinguishes
CreatedvsUpdated, and failures are logged as errors instead of unconditionally claiming success.Semantics
Updates are additive:
Updatesskips zero-value fields, so DB-managed columns that OSCAL doesn't carry (e.g.catalogs.active, which isNOT NULLwith a nil-pointer model field by design) are left untouched. ASave-based approach was tried first and violates that column's constraint — see the comment onrelational.Catalog.Active.How this was found
Building a demo environment in local-dev: catalogs extended with new controls and SSPs extended with new implemented-requirements were silently ignored on re-import, forcing API-side workaround scripts (
ensure-showcase-catalogs.sh,sync-showcase-statements.shin local-dev). Those become no-ops once this ships. The subdirectory panic is why local-dev's showcase content lives in a top-leveloscal-content-showcase/directory instead ofoscal-content/showcase/.Testing
Verified against a live local-dev database with a locally built binary:
Created; v2 with edited remarks, a flipped implementation-status, and a brand-new requirement →Updated, all three changes present in the DB (requirements, statements, by-components).Updated, control count 1→2,activepreserved astrue.go build,go vet,go test ./cmd/...green.🤖 Generated with Claude Code
Summary by CodeRabbit