feat(graphschema): Add Remediations CRUD BED-6869 #2208
Conversation
WalkthroughThis PR introduces CRUD operations for schema environments, relationship findings, and remediations in the database layer, along with transaction support. New model types are defined, database interface methods are added with SQL implementations, integration tests cover transaction and CRUD scenarios, and mocks are updated to support the new operations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/src/database/graphschema_integration_test.go (1)
1284-1307:TestGetSchemaEnvironmentsmultiple-env case has an inconsistent expectation on timestamp fieldsIn the
"Success: multiple schema environments"case, the second expected environment at Line 1294–Line 1305 includes a non-zeroBasic(CreatedAt/UpdatedAt/DeletedAt set to year 1). However, in the test body you explicitly zero outCreatedAt,UpdatedAt, andDeletedAton each element ofgotbefore callingassert.Equal.That means:
got[1].CreatedAt/UpdatedAt/DeletedAtare all zeroed.want.res[1].CreatedAt/UpdatedAt/DeletedAtremain non-zero.So the equality assertion will always fail for the second element.
Simplest fix is to keep the expected
Basicat its zero value so both sides match after zeroing:Proposed fix to expected value
- { - Serial: model.Serial{ - ID: 2, - Basic: model.Basic{ - CreatedAt: time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), - UpdatedAt: time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), - DeletedAt: sql.NullTime{Time: time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), Valid: false}, - }, - }, + { + Serial: model.Serial{ + ID: 2, + }, SchemaExtensionId: 1, EnvironmentKindId: 2, SourceKindId: 2, },
🧹 Nitpick comments (8)
local-harnesses/integration.config.json.template (1)
11-17: Graph driver switch topgmatches the new Postgres-backed integration flowChanging
"graph_driver"to"pg"is consistent with the Postgres connection string used by the harness and the new SQL-based integration tests. Keeping theneo4jsection around is fine, but consider updating any local test docs so it’s clear Postgres is now the primary driver for integration runs.cmd/api/src/database/database_integration_test.go (1)
23-34: Transaction integration tests give solid coverage; consider tightening one assertionThe
TestTransactionsubtests cover the main behaviors of the new transaction API (commit, rollback, nested calls, isolation options, read-only, and failure-on-write-in-read-only), which is exactly what you want here. One small improvement:
- In
"Fail: write in read-only transaction"you only assert thatTransactionreturns an error. For extra safety, you could also re-read the flag after the call and assert that itsEnabledstate is unchanged, mirroring what you do in the earlier commit/rollback tests.Overall the structure and use of
sql.TxOptionsand the callback pattern look correct.Please run the integration test suite that exercises
TestTransactionagainst your target Postgres version to confirm read-only transaction semantics behave as expected under that environment.Also applies to: 120-237
cmd/api/src/database/db.go (1)
23-39: Transaction wrapper onBloodhoundDBlooks correct; clarify usage in the doc commentThe
Transactionmethod is a clean wrapper around the underlying DB transaction, correctly:
- Binding a new
BloodhoundDBto the transactional*gorm.DB.- Threading through optional
*sql.TxOptions.- Committing/rolling back based on the callback’s error.
ErrDuplicateSchemaRelationshipFindingNamealso aligns with the new graph schema tests.One subtle usage point: within the callback, all DB operations should go through the provided
tx *BloodhoundDBto ensure they participate in the same transaction; calling methods on some outer*BloodhoundDBinstance would bypass it. That’s worth making explicit in the comment to avoid misuse later.Suggested comment tweak
-// Transaction executes the given function within a database transaction. +// Transaction executes the given function within a database transaction. // The function receives a new BloodhoundDB instance backed by the transaction, // allowing all existing methods to participate in the transaction. +// Callers must use the provided tx for all DB operations inside fn; using any +// other *BloodhoundDB will execute outside this transaction.Please sanity-check other call sites that use this helper to ensure they consistently call methods on the
tx *BloodhoundDBparameter rather than any outer DB handle.Also applies to: 45-62, 229-239
cmd/api/src/model/graphschema.go (1)
19-20: New schema models align with usage; consider minor modeling tweaks
SchemaRelationshipFindingandRemediationslook consistent with how they’re exercised in the integration tests (table names, fields, and JSON tags all match).Two optional refinements for future maintainability:
- If
finding_idis the logical primary key forschema_remediations, consider marking it explicitly withgorm:"primaryKey"so GORM behavior is unambiguous.- The struct name
Remediationsrepresents a single row, whereas most other models here use singular names for row types (with plural slice aliases). Renaming toRemediationand adding atype Remediations []Remediationalias would better match that pattern, if it’s not too disruptive.If you rely on GORM’s upsert/primary-key behavior for
schema_remediations, please confirm the current mapping behaves as expected with your version of GORM.Also applies to: 112-137
cmd/api/src/database/graphschema_integration_test.go (2)
1332-1583: New SchemaEnvironment get/delete/transaction tests are solid; avoid hard‑coded IDs where possibleThe added tests around schema environments and transactions (
TestGetSchemaEnvironmentById,TestDeleteSchemaEnvironment,TestTransaction_SchemaEnvironment) do a good job of covering:
- Success and not-found paths for get/delete.
- Commit and rollback behaviors (including constraint violations) in transactions.
- Create+delete operations within a single transaction.
The main fragility is the pervasive assumption that auto-incremented IDs start at 1 (extension ID 1, environment IDs 1 and 2). That’s true today with a fresh test DB, but could be invalidated if migrations ever seed these tables.
When convenient, consider capturing IDs from the create calls and reusing them rather than hard-coding
1/2. For example:ext, err := testSuite.BHDatabase.CreateGraphSchemaExtension(...) require.NoError(t, err) env, err := testSuite.BHDatabase.CreateSchemaEnvironment(testSuite.Context, ext.ID, 1, 1) // use env.ID instead of assuming 1This keeps the tests robust to future schema/seed changes while preserving current behavior.
Please confirm these tests pass when run repeatedly and in different orders (e.g., via
go testwith-runfilters) to ensure no hidden coupling to ID allocation beyond what pgtestdb guarantees.
1585-2163: SchemaRelationshipFinding and Remediations CRUD tests cover the key behaviors; same note on hard‑coded IDsThe new tests:
TestCreateSchemaRelationshipFindingandTestGetSchemaRelationshipFindingByIdvalidate creation, retrieval, and theErrDuplicateSchemaRelationshipFindingNamepath.TestDeleteSchemaRelationshipFindingexercises delete + not-found behavior.TestCreateRemediations,TestGetRemediationsByFindingId,TestUpdateRemediations, andTestDeleteRemediationscover create, get, update/upsert, delete, and their error paths.This gives good confidence in the new CRUD surface and its error contracts.
Two optional improvements:
- As with the environment tests, avoid assuming extension/environment/finding IDs are always
1by using the IDs returned from the relevant create calls instead of literal constants.- If the underlying schema enforces foreign keys (e.g., remediations referencing a non-existent finding), consider adding a negative test that ensures such violations propagate as errors in a predictable way.
If your migrations add or change seed data for these tables in the future, please re-run these tests to ensure the ID assumptions remain valid or have been updated to use returned IDs.
cmd/api/src/database/graphschema.go (2)
615-644: CTE-based insertion is clean and atomic.The use of a CTE with aggregation to insert and return remediation data is elegant. The query structure ensures atomicity of the multi-row insert.
Optional: Add defensive RowsAffected check
For consistency with
GetRemediationsByFindingId, consider adding a check after the Scan:findingId, shortRemediation, findingId, longRemediation).Scan(&remediations); result.Error != nil { return model.Remediations{}, CheckError(result) + } else if result.RowsAffected == 0 { + return model.Remediations{}, fmt.Errorf("no remediations created") }This is defensive programming but not strictly necessary since the INSERT should always succeed or error.
668-698: Upsert implementation is correct and atomic.The
ON CONFLICT DO UPDATEpattern properly handles both insert and update cases in a single atomic operation. The CTE structure ensures consistency.Optional: Add defensive RowsAffected check
For consistency with
GetRemediationsByFindingId, consider:findingId, shortRemediation, findingId, longRemediation).Scan(&remediations); result.Error != nil { return model.Remediations{}, CheckError(result) + } else if result.RowsAffected == 0 { + return model.Remediations{}, fmt.Errorf("no remediations updated") }Not strictly required since upsert should always affect rows or error, but adds defensive depth.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/api/src/database/database_integration_test.gocmd/api/src/database/db.gocmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/mocks/db.gocmd/api/src/model/appcfg/parameter_integration_test.gocmd/api/src/model/graphschema.golocal-harnesses/integration.config.json.template
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: bsheth711
Repo: SpecterOps/BloodHound PR: 1766
File: cmd/api/src/database/access_control_list.go:55-103
Timestamp: 2025-08-12T15:30:00.877Z
Learning: In BloodHound's database layer, s.UpdateUser uses nested transactions within GORM. When called inside an AuditableTransaction, error propagation from the nested transaction causes rollback of the parent transaction, maintaining atomicity across the entire operation.
📚 Learning: 2025-08-07T16:45:34.764Z
Learnt from: specterops-avongluck
Repo: SpecterOps/BloodHound PR: 1765
File: debian/bhapi.json.default:7-9
Timestamp: 2025-08-07T16:45:34.764Z
Learning: In BloodHound Debian packaging, the database connection string "postgresql://" in debian/bhapi.json.default is intentionally minimal because it uses PostgreSQL peer authentication. The bloodhound system user (created in postinst) can connect to the bloodhound PostgreSQL role without a password, similar to how the postgresql system user works.
Applied to files:
local-harnesses/integration.config.json.template
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/model/appcfg/parameter_integration_test.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/database_integration_test.go
📚 Learning: 2025-08-12T15:30:00.877Z
Learnt from: bsheth711
Repo: SpecterOps/BloodHound PR: 1766
File: cmd/api/src/database/access_control_list.go:55-103
Timestamp: 2025-08-12T15:30:00.877Z
Learning: In BloodHound's database layer, s.UpdateUser uses nested transactions within GORM. When called inside an AuditableTransaction, error propagation from the nested transaction causes rollback of the parent transaction, maintaining atomicity across the entire operation.
Applied to files:
cmd/api/src/database/db.gocmd/api/src/database/database_integration_test.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
cmd/api/src/database/db.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
cmd/api/src/database/db.go
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/database/db.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/model/graphschema.gocmd/api/src/database/graphschema.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/database/graphschema_integration_test.gocmd/api/src/model/graphschema.gocmd/api/src/database/graphschema.go
🧬 Code graph analysis (3)
cmd/api/src/database/graphschema_integration_test.go (3)
cmd/api/src/model/graphschema.go (6)
SchemaEnvironment(101-106)SchemaEnvironment(108-110)SchemaRelationshipFinding(113-121)SchemaRelationshipFinding(123-125)Remediations(127-133)Remediations(135-137)cmd/api/src/database/db.go (3)
ErrNotFound(42-42)BloodhoundDB(194-197)ErrDuplicateSchemaRelationshipFindingName(61-61)packages/go/graphschema/common/common.go (1)
DisplayName(54-54)
cmd/api/src/database/database_integration_test.go (2)
packages/go/graphschema/common/common.go (1)
Enabled(64-64)cmd/api/src/database/db.go (1)
BloodhoundDB(194-197)
cmd/api/src/database/mocks/db.go (1)
cmd/api/src/model/graphschema.go (4)
Remediations(127-133)Remediations(135-137)SchemaRelationshipFinding(113-121)SchemaRelationshipFinding(123-125)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (11)
cmd/api/src/model/appcfg/parameter_integration_test.go (1)
25-31: Citrix RDP support expectation flipped totrueThe assertion in
TestParameters_GetCitrixRDPSupportnow expectstrue, which is fine as long as the integration DB seeding (or default parameter behavior) has been updated accordingly. The added imports are used (neo4j.DefaultWriteFlushSizeandrequire) and look correct.Please confirm that the integration migrations/seed data now set Citrix RDP support to enabled so this test reflects the intended default.
Also applies to: 118-120
cmd/api/src/database/mocks/db.go (1)
464-477: Mocks updated consistently for new schema environment/finding/remediation APIsThe additional mock methods for:
- Schema environments (
GetSchemaEnvironmentById,DeleteSchemaEnvironment)- Schema relationship findings (
CreateSchemaRelationshipFinding,GetSchemaRelationshipFindingById,DeleteSchemaRelationshipFinding)- Remediations (
CreateRemediations,GetRemediationsByFindingId,UpdateRemediations,DeleteRemediations)all follow the existing gomock pattern and match the corresponding method signatures on the
Databaseinterface and model types. No issues spotted here; looks likemockgenwas re-run correctly after the interface change.If you evolve the
Databaseinterface further (e.g., add a transactional helper there), remember to regenerate this file withmockgento keep mocks in sync.Also applies to: 588-601, 914-927, 975-1001, 1981-1994, 2146-2159, 2176-2189, 2942-2955
cmd/api/src/database/graphschema.go (9)
55-67: Interface additions look well-structured.The new CRUD method signatures for SchemaEnvironment, SchemaRelationshipFinding, and Remediations follow established conventions in the codebase and provide comprehensive data access capabilities.
532-535: Good consistency improvement.Wrapping the return with
CheckErroraligns this method's error handling with other retrieval methods in the file.
537-552: Correct implementation with proper error handling.The method correctly retrieves schema environments by ID and returns
ErrNotFoundwhen no rows match. The use ofScan()with manualRowsAffectedchecking is valid, though slightly different from some other GetById methods in the file that useFirst().
554-565: Delete implementation follows established patterns.Consistent with other delete methods in the file, properly handling both errors and missing records.
567-583: Create operation properly handles duplicates and errors.The implementation follows established patterns for creation methods, including proper duplicate key detection and comprehensive field returns.
585-600: Retrieval logic is correct.Properly handles both errors and missing records with appropriate error returns.
602-613: Delete operation follows standard pattern.Correctly implemented with proper error and not-found handling.
646-666: Aggregation query correctly retrieves remediation data.The use of
MAXwithFILTERto pivot the normalized remediation rows into a single aggregated result is appropriate. Proper handling of missing records withErrNotFound.
700-708: Delete operation correctly removes all remediation content.Properly deletes all content types for the specified finding and handles missing records appropriately.
cweidenkeller
changed the title
cweidenkeller
force-pushed
the
BED-6869
branch
from
88aa158 to
77dbd22
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/src/database/graphschema_integration_test.go (1)
1216-1330: Inconsistent handling of timestamp fields inTestGetSchemaEnvironments“multiple schema environments” caseIn the
"Success: multiple schema environments"case, you zero outgot[i].CreatedAt/UpdatedAt/DeletedAtbefore comparison, but the second expected environment explicitly sets non-zeroBasictimestamps (time.Date(1, ...)):Serial: model.Serial{ ID: 2, Basic: model.Basic{ CreatedAt: time.Date(1, ...), UpdatedAt: time.Date(1, ...), DeletedAt: sql.NullTime{Time: time.Date(1, ...), Valid: false}, }, },This makes the equality assertion fail once the test actually executes, because
got[1].Basichas zero timestamps whilewant[1].Basicdoes not.A minimal fix is to remove the explicit
Basicinitialization from the expected value so both sides have zero timestamps:Proposed diff to simplify expected second environment
- { - Serial: model.Serial{ - ID: 2, - Basic: model.Basic{ - CreatedAt: time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), - UpdatedAt: time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), - DeletedAt: sql.NullTime{Time: time.Date(1, time.January, 1, 0, 0, 0, 0, time.UTC), Valid: false}, - }, - }, + { + Serial: model.Serial{ + ID: 2, + }, SchemaExtensionId: 1, EnvironmentKindId: 2, SourceKindId: 2, },
🧹 Nitpick comments (6)
cmd/api/src/database/database_integration_test.go (1)
21-34: Good coverage of transaction semantics; consider tightening a couple of detailsThe
TestTransactionsuite nicely exercises commit, rollback, nested calls, isolation level, and read-only behavior against the newTransactionhelper.Two minor suggestions:
- Inside the transaction lambdas, prefer using the
ctxparameter passed intoTransaction(or a derived child) instead oftestSuite.Contextto make the call graph’s context flow explicit and easier to refactor.- In
"Fail: write in read-only transaction", you only assert that an error is returned. Adding a follow-upGetFlagByKeyassertion that the flag state is unchanged would make the intent (read-only truly enforces no writes) explicit and guard against a driver/config that surfaces a warning instead of an error.Also applies to: 120-238
cmd/api/src/database/graphschema_integration_test.go (2)
1332-1583: SchemaEnvironment ID assumptions and transaction tests are solid but a bit brittleThe new environment tests (
TestGetSchemaEnvironmentById,TestDeleteSchemaEnvironment, andTestTransaction_SchemaEnvironment) correctly exercise:
- Success and not-found flows for single-environment reads and deletes.
- Multi-operation commit and rollback semantics when using
BloodhoundDB.Transaction.- Rollback behavior on both explicit errors and unique-constraint violations.
- Create/delete in the same transaction yielding no persisted environment.
Two things to consider tightening:
Hard-coded primary/foreign key IDs
Most of these tests assert exact numeric IDs (e.g., environment
ID: 1,SchemaExtensionId: 1) or pass literal1/2intoCreateSchemaEnvironment. That’s fine with the current migration/seed behavior, but it couples the tests to specific ID allocation and assumes no pre-existing rows.To make the tests robust to future migrations or seed data, prefer using IDs from the objects you just created:
Illustrative change using returned extension/env IDs
- _, err := testSuite.BHDatabase.CreateGraphSchemaExtension(testSuite.Context, "Extension1", "DisplayName", "v1.0.0") + ext, err := testSuite.BHDatabase.CreateGraphSchemaExtension(testSuite.Context, "Extension1", "DisplayName", "v1.0.0") require.NoError(t, err) - _, err = testSuite.BHDatabase.CreateSchemaEnvironment(testSuite.Context, defaultSchemaExtensionID, int32(1), int32(1)) + env, err := testSuite.BHDatabase.CreateSchemaEnvironment(testSuite.Context, ext.ID, int32(1), int32(1)) require.NoError(t, err) // and in expectations: - environmentId: 1, + environmentId: env.ID, ... - SchemaExtensionId: 1, + SchemaExtensionId: ext.ID,Same idea applies in the transaction tests: use
ext.ID/env.IDinstead of assuming 1/2.Verifying rollback on constraint errors
The
"Rollback: database constraint error causes rollback"test currently only checks that an error occurred and thatGetSchemaEnvironmentById(ctx, 1)returnsErrNotFound. That’s good, but you might also want to assert that the error is specifically a duplicate-environment error (e.g.,ErrDuplicateSchemaEnvironment) so you don’t mask other unexpected failures.Overall, the transaction behavior you’re asserting looks correct; these changes would mainly make the tests more resilient to schema and seed evolution.
1585-2163: SchemaRelationshipFinding and Remediations integration tests comprehensively cover the new CRUD surfaceThe new tests for
SchemaRelationshipFindingandRemediationsdo a good job of validating:
- Creation, not-found, and deletion semantics for findings.
- Duplicate-finding detection via
ErrDuplicateSchemaRelationshipFindingName.- End-to-end Remediations flows:
- Initial create and retrieval by finding ID.
- Update-as-upsert behavior (updating existing rows and creating new ones if absent).
- Delete and subsequent
ErrNotFound.A couple of minor, optional improvements:
- Similar to the environment tests, you’re hard-coding IDs (
extensionId: 1,environmentId: 1,relationshipKindId: 1,findingId: 1) in both the arguments and expectations. Using the IDs returned from the variousCreate*calls would decouple these tests from any future changes in seeding or PK allocation.- For the “Fail: remediations not found” and “Fail: schema relationship finding not found” cases, you already assert
ErrNotFound, which is great. You might want to also add a quickrequire.Zero(t, got)(or equivalent) to ensure the zero-value behavior of the returned struct stays intentional.Functionally, though, the scenarios you’re exercising align well with the new database methods and look correct.
cmd/api/src/model/graphschema.go (1)
19-20: New graphschema models align with usage; consider consistency with existing Serial/Basic pattern
SchemaRelationshipFindingandRemediationslook consistent with how you query them ingraphschema.go:
SchemaRelationshipFindingexposesIDandCreatedAtexplicitly and maps toschema_relationship_findings.Remediationsis a denormalized projection over theschema_remediationskey/value table and its fields match the column aliases in the CTE queries.One stylistic consideration: most of the surrounding types embed
Serial/Basicfor ID/timestamps. If you expectSchemaRelationshipFindingto participate in the same generic behaviors (e.g., common helpers onBasic), embeddingSerialinstead of spelling outID/CreatedAtby hand might improve consistency. If it’s intentionally narrower (noUpdatedAt/DeletedAt), the current shape is fine.Also applies to: 112-137
cmd/api/src/database/graphschema.go (2)
54-67: Environment CRUD implementation is consistent; small consistency nitsThe additions around
SchemaEnvironmentlook correct:
CreateSchemaEnvironmentinserts with explicit timestamps, detects duplicates viaErrDuplicateSchemaEnvironment, and returns the full row.GetSchemaEnvironmentsnow simply doesFind(&result)and wraps errors inCheckError, returning an empty slice on “no rows”.GetSchemaEnvironmentByIdandDeleteSchemaEnvironmentfollow the same pattern as other ID-based getters/deleters: parameterized SQL,CheckErroron DB failures, and explicitErrNotFoundwhenRowsAffected == 0.Two minor consistency nits you might consider for a future pass:
- Other ID-based getters (e.g.,
GetGraphSchemaPropertyById) tend to use...First(&model)and rely onCheckErrorto turnErrRecordNotFoundintoErrNotFound. Your new methods useScan+ manualRowsAffected == 0handling. Both are correct, but using one style throughout would make the codebase more uniform.GetSchemaEnvironmentsdoesn’t define an explicit ordering, so callers (and tests) should not rely on row order. If you want deterministic behavior, consider addingOrder("id ASC")beforeFind(&result).Functionally these changes look good as-is.
Also applies to: 513-565
59-66: Relationship-finding and remediations SQL is sound; a couple of optional robustness tweaksFor
SchemaRelationshipFinding:
CreateSchemaRelationshipFindinguses a parameterizedINSERT ... RETURNINGand correctly maps duplicate-key violations toErrDuplicateSchemaRelationshipFindingName.GetSchemaRelationshipFindingByIdandDeleteSchemaRelationshipFindingare consistent with other ID-based operations and surfaceErrNotFoundwhen appropriate.For remediations:
CreateRemediationsandUpdateRemediationsuse CTEs overschema_remediationswith four rows per finding (content_typediscriminator) and aggregate back into a singlemodel.Remediationsrow viaMAX(...) FILTER (WHERE content_type = ...). Given an enforced unique(finding_id, content_type)constraint, this is a clean, fully parameterized approach.GetRemediationsByFindingIdreuses the same aggregation against the base table and returnsErrNotFoundwhen no rows exist.DeleteRemediationsdeletes all rows for a finding and returnsErrNotFoundwhen nothing was deleted, which matches the tests.Optional improvements:
- You hard-code
"schema_remediations"in the SQL rather than usingmodel.Remediations{}.TableName(). UsingTableName()would keep the SQL resilient to future table-renaming without changing code.- If you ever introduce additional
content_typevalues, these queries will silently ignore them. That’s fine given current requirements, but you may want a comment noting that the projection is intentionally limited to those four content types.Overall, the SQL and error mapping here look correct and safe.
Also applies to: 567-707
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/api/src/database/database_integration_test.gocmd/api/src/database/db.gocmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/mocks/db.gocmd/api/src/model/graphschema.go
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-12T15:30:00.877Z
Learnt from: bsheth711
Repo: SpecterOps/BloodHound PR: 1766
File: cmd/api/src/database/access_control_list.go:55-103
Timestamp: 2025-08-12T15:30:00.877Z
Learning: In BloodHound's database layer, s.UpdateUser uses nested transactions within GORM. When called inside an AuditableTransaction, error propagation from the nested transaction causes rollback of the parent transaction, maintaining atomicity across the entire operation.
Applied to files:
cmd/api/src/database/database_integration_test.gocmd/api/src/database/db.go
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/database/database_integration_test.gocmd/api/src/database/graphschema_integration_test.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/model/graphschema.gocmd/api/src/database/graphschema.go
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/model/graphschema.gocmd/api/src/database/db.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/graphschema.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
cmd/api/src/database/db.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
cmd/api/src/database/db.go
🧬 Code graph analysis (2)
cmd/api/src/database/database_integration_test.go (2)
packages/go/graphschema/common/common.go (1)
Enabled(64-64)cmd/api/src/database/db.go (1)
BloodhoundDB(194-197)
cmd/api/src/database/mocks/db.go (1)
cmd/api/src/model/graphschema.go (6)
Remediations(127-133)Remediations(135-137)SchemaRelationshipFinding(113-121)SchemaRelationshipFinding(123-125)SchemaEnvironment(101-106)SchemaEnvironment(108-110)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
- GitHub Check: build-ui
- GitHub Check: run-analysis
🔇 Additional comments (2)
cmd/api/src/database/db.go (1)
23-24: Transaction wrapper and new duplicate-finding error look correct; minor API ergonomics nitThe
Transactionhelper correctly wrapsgorm.DB.Transaction, propagates context and*sql.TxOptions, and reusesNewBloodhoundDBso all existing methods can be called inside the transaction. The newErrDuplicateSchemaRelationshipFindingNamecleanly plugs into the graphschema error-handling pattern.If you expect callers to often need a context scoped specifically to the transaction body, you might consider a future variant like
func (s *BloodhoundDB) Transaction(ctx context.Context, fn func(ctx context.Context, tx *BloodhoundDB) error, ...)so the transaction context can differ from the outer call-site context, but that’s purely optional given current usage.Also applies to: 61-62, 229-239
cmd/api/src/database/mocks/db.go (1)
464-477: Mocks correctly reflect new graphschema and remediation methodsThe newly generated mock methods for schema environments, relationship findings, and remediations (
CreateSchemaRelationshipFinding,GetSchemaEnvironmentById,GetSchemaRelationshipFindingById,CreateRemediations,GetRemediationsByFindingId,UpdateRemediations,DeleteSchemaEnvironment,DeleteSchemaRelationshipFinding,DeleteRemediations) all match the correspondingDatabase/OpenGraphSchemamethod signatures and follow the standard gomock call/recorder pattern.No changes needed here; just ensure you continue to regenerate this file via
mockgenwhenever the interface evolves, rather than editing it by hand.Also applies to: 914-927, 975-1001, 1981-1994, 2146-2159, 2176-2189, 2942-2955
Description
Describe your changes in detail
Added CRUD operations for Remediation table
Motivation and Context
Resolves BED-6869
Why is this change required? What problem does it solve?
Allows for dynamic config of remediation.
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Added integration tests
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.