feat(OpenGraph): Add findings CRUD BED-6868 #2148
Conversation
WalkthroughAdds OpenGraph schema entities and migrations, new GORM models, six CRUD DB methods and two duplicate-error constants, a RawFirst SQL helper, mocks for the new methods, and integration tests covering environments and relationship findings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/api/src/database/db.go(3 hunks)cmd/api/src/database/graphschema.go(5 hunks)cmd/api/src/database/graphschema_test.go(1 hunks)cmd/api/src/database/migration/migrations/v8.5.0.sql(1 hunks)cmd/api/src/database/mocks/db.go(6 hunks)cmd/api/src/model/graphschema.go(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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/graphschema_test.gocmd/api/src/database/graphschema.gocmd/api/src/database/mocks/db.gocmd/api/src/database/migration/migrations/v8.5.0.sql
📚 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_test.gocmd/api/src/database/graphschema.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/graphschema_test.go
📚 Learning: 2025-09-02T16:46:30.895Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: cmd/api/src/api/v2/auth/auth.go:559-573
Timestamp: 2025-09-02T16:46:30.895Z
Learning: In the BloodHound codebase, when updating user ETAC (Environment Access Control) lists in the UpdateUser function, the approach is to let GORM handle the creation/persistence of the environment access records through model associations rather than using explicit database helper methods like UpdateEnvironmentListForUser.
Applied to files:
cmd/api/src/database/graphschema.go
🧬 Code graph analysis (3)
cmd/api/src/model/graphschema.go (2)
packages/go/schemagen/tsgen/tsgen.go (1)
ID(216-220)packages/go/graphschema/common/common.go (1)
DisplayName(54-54)
cmd/api/src/database/db.go (1)
cmd/api/src/database/helper.go (1)
CheckError(26-32)
cmd/api/src/database/mocks/db.go (1)
cmd/api/src/model/graphschema.go (4)
SchemaEnvironment(88-93)SchemaEnvironment(95-97)SchemaRelationshipFinding(100-107)SchemaRelationshipFinding(109-111)
⏰ 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-tests
- GitHub Check: run-analysis
🔇 Additional comments (9)
cmd/api/src/database/graphschema_test.go (2)
435-519: LGTM! Comprehensive test coverage.Both test functions provide excellent coverage of CRUD operations, including:
- Successful create, read, and delete operations
- Duplicate constraint violation handling
- Not-found error handling for deleted and invalid IDs
The tests follow the existing patterns in the file and properly use
t.Parallel()at the top level with unique test data to avoid conflicts.Also applies to: 521-615
446-453: RawFirst is the standard pattern for FK setup in this test suite.The
RawFirstmethod is properly used here for inserting prerequisite records into thekindtable. This approach is consistent throughout the integration tests in this file (also used at lines 532-539) and is the standard way to set up foreign key dependencies in this codebase.cmd/api/src/database/db.go (2)
248-251: LGTM! Useful utility method.The
RawFirstmethod provides a clean wrapper for executing raw SQL queries that return a single result, following the existing error handling patterns. This complements the existing GORM raw query capabilities.
60-61: LGTM! Error variables follow existing patterns.The new error variables are well-named, descriptive, and follow the established naming convention for duplicate constraint errors.
cmd/api/src/database/migration/migrations/v8.5.0.sql (3)
91-98: Verify cascade delete behavior for schema_environments.The FK from
schema_environmentstoschema_extensions(id)doesn't haveON DELETE CASCADE, whileschema_relationship_findingsdoes (line 103). This means:
- Deleting an extension will cascade-delete relationship findings
- But deleting an extension will fail if environments still reference it
Is this intentional? If not, consider adding
ON DELETE CASCADEfor consistency:- schema_extension_id INTEGER NOT NULL REFERENCES schema_extensions(id), + schema_extension_id INTEGER NOT NULL REFERENCES schema_extensions(id) ON DELETE CASCADE,
110-110: Consider scoping finding names to extensions.The
UNIQUE(name)constraint makes finding names globally unique across all extensions. This means different extensions cannot define findings with the same name (e.g., two extensions can't both have "WriteOwner").If findings should be scoped to their extension, consider:
- UNIQUE(name) + UNIQUE(schema_extension_id, name)If global uniqueness is intentional, this is fine.
131-131: Verify principal_kind uniqueness constraint.The
UNIQUE(principal_kind)constraint means each kind can only be associated with one environment. This seems restrictive - couldn't the same principal kind (e.g., "User") be valid in multiple environments?If this is intentional (e.g., for a one-to-one mapping), this is fine. Otherwise, consider whether the constraint should be:
- UNIQUE(principal_kind) + UNIQUE(environment_id, principal_kind)cmd/api/src/database/graphschema.go (1)
282-354: LGTM! CRUD implementations follow established patterns.All six new methods correctly implement the CRUD operations:
- Proper error handling with specific error types for duplicates and not-found cases
- Consistent use of raw SQL with RETURNING clauses for inserts
- Row count checks for deletes
- Pattern matches existing methods in the file
Based on learnings, the absence of
AuditableTransactionis intentional to avoid audit log noise, consistent withCreateSchemaEdgeKind.cmd/api/src/database/mocks/db.go (1)
543-586: LGTM! Generated mock code.This file is auto-generated by mockgen (as indicated by the header comment). The mock methods correctly implement the new interface additions. No manual review needed.
Also applies to: 904-945, 1993-2037
| // SchemaRelationshipFinding - represents a finding (e.g., T0WriteOwner, T0ADCSESC1, T0DCSync) | ||
| type SchemaRelationshipFinding struct { | ||
| ID int32 `json:"id" gorm:"primaryKey"` | ||
| SchemaExtensionId int32 `json:"schema_extension_id"` | ||
| RelationshipKindId int32 `json:"relationship_kind_id"` | ||
| EnvironmentId int32 `json:"environment_id"` | ||
| Name string `json:"name"` | ||
| DisplayName string `json:"display_name"` | ||
| } | ||
|
|
||
| func (SchemaRelationshipFinding) TableName() string { | ||
| return "schema_relationship_findings" | ||
| } |
There was a problem hiding this comment.
Model field mismatch with database schema.
The SchemaRelationshipFinding model is missing the created_at timestamp field that exists in the database schema (see migration line 108). This could cause issues if:
- Code attempts to read or sort by
created_at - The field is unexpectedly omitted when querying
Similarly, consider whether these models should embed Serial (which includes timestamps) for consistency with other models in this file (e.g., GraphSchemaExtension, SchemaNodeKind).
Suggested fix to add the missing field:
type SchemaRelationshipFinding struct {
ID int32 `json:"id" gorm:"primaryKey"`
SchemaExtensionId int32 `json:"schema_extension_id"`
RelationshipKindId int32 `json:"relationship_kind_id"`
EnvironmentId int32 `json:"environment_id"`
Name string `json:"name"`
DisplayName string `json:"display_name"`
+ CreatedAt time.Time `json:"created_at"`
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SchemaRelationshipFinding - represents a finding (e.g., T0WriteOwner, T0ADCSESC1, T0DCSync) | |
| type SchemaRelationshipFinding struct { | |
| ID int32 `json:"id" gorm:"primaryKey"` | |
| SchemaExtensionId int32 `json:"schema_extension_id"` | |
| RelationshipKindId int32 `json:"relationship_kind_id"` | |
| EnvironmentId int32 `json:"environment_id"` | |
| Name string `json:"name"` | |
| DisplayName string `json:"display_name"` | |
| } | |
| func (SchemaRelationshipFinding) TableName() string { | |
| return "schema_relationship_findings" | |
| } | |
| // SchemaRelationshipFinding - represents a finding (e.g., T0WriteOwner, T0ADCSESC1, T0DCSync) | |
| type SchemaRelationshipFinding struct { | |
| ID int32 `json:"id" gorm:"primaryKey"` | |
| SchemaExtensionId int32 `json:"schema_extension_id"` | |
| RelationshipKindId int32 `json:"relationship_kind_id"` | |
| EnvironmentId int32 `json:"environment_id"` | |
| Name string `json:"name"` | |
| DisplayName string `json:"display_name"` | |
| CreatedAt time.Time `json:"created_at"` | |
| } | |
| func (SchemaRelationshipFinding) TableName() string { | |
| return "schema_relationship_findings" | |
| } |
🤖 Prompt for AI Agents
In cmd/api/src/model/graphschema.go around lines 99 to 111, the
SchemaRelationshipFinding model lacks the created_at timestamp field present in
the DB migration; add a CreatedAt time.Time field (with JSON tag "created_at"
and appropriate GORM tag if needed) to the struct or embed the existing Serial
struct used by other models to include created_at/updated_at consistently;
update imports if embedding requires time or serial type, and run model-related
tests to ensure queries/sorts that reference created_at work correctly.
cweidenkeller
force-pushed
the
BED-6868
branch
from
6ec5e26 to
03a9786
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
92-99: Consider adding timestamps to schema_environments for consistency.The
schema_environmentstable does not havecreated_atorupdated_attimestamps, whileschema_relationship_findings(lines 111) and most other schema tables in this migration include them. Unless this is intentional, adding timestamps would improve consistency and provide audit trails.CREATE TABLE IF NOT EXISTS schema_environments ( id SERIAL, schema_extension_id INTEGER NOT NULL REFERENCES schema_extensions(id) ON DELETE CASCADE, environment_kind_id INTEGER NOT NULL REFERENCES kind(id), source_kind_id INTEGER NOT NULL REFERENCES kind(id), + created_at TIMESTAMP WITH TIME ZONE DEFAULT current_timestamp, PRIMARY KEY (id), UNIQUE(environment_kind_id,source_kind_id) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmd/api/src/database/db.go(3 hunks)cmd/api/src/database/graphschema.go(5 hunks)cmd/api/src/database/graphschema_test.go(1 hunks)cmd/api/src/database/migration/migrations/v8.5.0.sql(2 hunks)cmd/api/src/database/mocks/db.go(6 hunks)cmd/api/src/model/graphschema.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/database/db.go
- cmd/api/src/model/graphschema.go
🧰 Additional context used
🧠 Learnings (3)
📚 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/graphschema_test.gocmd/api/src/database/migration/migrations/v8.5.0.sqlcmd/api/src/database/graphschema.gocmd/api/src/database/mocks/db.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_test.gocmd/api/src/database/graphschema.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/graphschema_test.go
🧬 Code graph analysis (3)
cmd/api/src/database/graphschema_test.go (2)
cmd/api/src/model/graphschema.go (4)
SchemaEnvironment(88-93)SchemaEnvironment(95-97)SchemaRelationshipFinding(100-107)SchemaRelationshipFinding(109-111)cmd/api/src/database/db.go (2)
ErrDuplicateSchemaEnvironment(60-60)ErrNotFound(42-42)
cmd/api/src/database/graphschema.go (3)
cmd/api/src/model/graphschema.go (4)
SchemaEnvironment(88-93)SchemaEnvironment(95-97)SchemaRelationshipFinding(100-107)SchemaRelationshipFinding(109-111)cmd/api/src/database/db.go (4)
BloodhoundDB(194-197)ErrDuplicateSchemaEnvironment(60-60)ErrNotFound(42-42)ErrDuplicateSchemaRelationshipFindingName(61-61)cmd/api/src/database/helper.go (1)
CheckError(26-32)
cmd/api/src/database/mocks/db.go (1)
cmd/api/src/model/graphschema.go (4)
SchemaEnvironment(88-93)SchemaEnvironment(95-97)SchemaRelationshipFinding(100-107)SchemaRelationshipFinding(109-111)
⏰ 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 (5)
cmd/api/src/database/graphschema_test.go (1)
435-615: LGTM! Comprehensive CRUD test coverage.Both test suites follow the established patterns in this file and provide thorough coverage of:
- Successful create and retrieve operations
- Duplicate constraint violations
- Delete operations
- Not-found error handling for deleted and invalid IDs
The tests correctly verify field values and error conditions.
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
91-142: LGTM! Well-structured schema additions.The table definitions are well-designed with:
- Appropriate foreign key constraints with CASCADE delete behavior
- UNIQUE constraints matching the duplicate error handling in the code
- Indexes on foreign key columns for query optimization
- Proper referential integrity across the schema
cmd/api/src/database/graphschema.go (2)
47-53: Consider whether Update methods are needed for completeness.The new interface declares Create, Read (Get), and Delete methods but omits Update operations for both
SchemaEnvironmentandSchemaRelationshipFinding. While this may be intentional (e.g., if these entities are immutable after creation), it's worth confirming whether Update functionality will be needed in the future. The PR title mentions "CRUD" but only CRD is implemented.
282-354: LGTM! Consistent CRUD implementations.The Create, Read (Get), and Delete implementations follow the established patterns in this file:
- Proper error handling with duplicate detection and not-found errors
- Use of RETURNING clause to populate created entities
- RowsAffected checks in Delete methods
- Consistent SQL structure and parameter passing
cmd/api/src/database/mocks/db.go (1)
543-586: LGTM! Auto-generated mocks are correct.The mock methods for the six new CRUD operations (CreateSchemaEnvironment, GetSchemaEnvironmentById, DeleteSchemaEnvironment, CreateSchemaRelationshipFinding, GetSchemaRelationshipFindingById, DeleteSchemaRelationshipFinding) follow the proper gomock patterns and match the interface signatures.
Also applies to: 904-944, 1993-2036
Description
Added CRUD for findings table
Motivation and Context
Resolves BED-6868
Why is this change required? What problem does it solve?
This is required for the expansion of opengraph schema functionality.
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 test suite.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Database
Tests
✏️ Tip: You can customize this high-level summary in your review settings.