feat(OpenGraph): Add schema_environments crud BED-6851 #2146
Conversation
|
Warning Rate limit exceeded@cweidenkeller has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (6)
WalkthroughAdds a new SchemaEnvironment entity with model, CRUD DB methods, mocks, integration tests, a raw-query helper (RawFirst), a new public duplicate error, and SQL migrations introducing environment-related tables and relationships. 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)
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
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/migration/migrations/v8.5.0.sql (1)
59-70: Duplicate table definition forschema_extensions.The
schema_extensionstable is defined twice in this migration file (lines 29-40 and lines 60-70). WhileCREATE TABLE IF NOT EXISTSprevents an error, this duplication is likely unintentional and should be removed.-- OpenGraph graph schema - extensions (collectors) -CREATE TABLE IF NOT EXISTS schema_extensions ( - id SERIAL NOT NULL, - name TEXT UNIQUE NOT NULL, - display_name TEXT NOT NULL, - version TEXT NOT NULL, - is_builtin BOOLEAN DEFAULT FALSE, - created_at TIMESTAMP WITH TIME ZONE DEFAULT current_timestamp, - updated_at TIMESTAMP WITH TIME ZONE DEFAULT current_timestamp, - deleted_at TIMESTAMP WITH TIME ZONE DEFAULT NULL, - PRIMARY KEY (id) -); -
🧹 Nitpick comments (2)
cmd/api/src/model/graphschema.go (1)
87-97: Inconsistent struct pattern: missingSerialembedding and timestamps.Other schema types (
SchemaNodeKind,SchemaEdgeKind,GraphSchemaProperty) embedSerialwhich providesID,CreatedAt,UpdatedAt, andDeletedAtfields.SchemaEnvironmentdefines its ownIDfield without timestamps.If this is intentional (simpler entity without audit trail), consider adding a comment explaining the design decision. Otherwise, consider aligning with the existing pattern:
// SchemaEnvironment - represents an environment mapping for an extension type SchemaEnvironment struct { - ID int32 `json:"id" gorm:"primaryKey"` + Serial ExtensionId int32 `json:"extension_id"` EnvironmentKindId int32 `json:"environment_kind_id"` SourceKindId int32 `json:"source_kind_id"` }Note: This would also require adding timestamp columns to the
schema_environmentstable in the migration.cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
103-145: Consider adding indexes on foreign key columns.The new tables have foreign key columns (
extension_id,environment_kind_id,source_kind_id,environment_id,finding_id,relationship_kind_id) that may benefit from indexes for query performance, especially if these tables grow large. Other tables in this migration (e.g.,schema_node_kinds,schema_properties,schema_edge_kinds) have explicit indexes on their FK columns.
📜 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(3 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/database/graphschema_test.gocmd/api/src/model/graphschema.gocmd/api/src/database/mocks/db.gocmd/api/src/database/graphschema.gocmd/api/src/database/migration/migrations/v8.5.0.sql
📚 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
📚 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.go
🧬 Code graph analysis (4)
cmd/api/src/database/graphschema_test.go (2)
cmd/api/src/model/graphschema.go (2)
SchemaEnvironment(88-93)SchemaEnvironment(95-97)cmd/api/src/database/db.go (2)
ErrDuplicateSchemaEnvironment(60-60)ErrNotFound(42-42)
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 (2)
SchemaEnvironment(88-93)SchemaEnvironment(95-97)
cmd/api/src/database/graphschema.go (3)
cmd/api/src/model/graphschema.go (2)
SchemaEnvironment(88-93)SchemaEnvironment(95-97)cmd/api/src/database/db.go (3)
BloodhoundDB(193-196)ErrDuplicateSchemaEnvironment(60-60)ErrNotFound(42-42)cmd/api/src/database/helper.go (1)
CheckError(26-32)
⏰ 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 (12)
cmd/api/src/database/db.go (2)
60-60: LGTM!New error follows the established pattern for duplicate entity errors in this file.
247-250: TheRawFirstmethod is correctly implemented for its actual usage pattern.
RawFirstis only used forINSERT ... RETURNINGstatements in the codebase. WithINSERT ... RETURNING, the database guarantees a row is returned on success, so the difference betweenScanandTake/Firstis irrelevant. Empty result sets cannot occur with successfulINSERT ... RETURNING—the operation either succeeds with a row or fails at the INSERT level. The current implementation usingScanis appropriate andCheckErrorcorrectly handles any database-level errors.Likely an incorrect or invalid review comment.
cmd/api/src/database/graphschema_test.go (2)
435-519: LGTM! Comprehensive CRUD test coverage.The test follows the established patterns in this file and covers the key scenarios:
- Successful creation with field validation
- Duplicate detection for unique constraint
- Get by ID
- Delete and subsequent not-found verification
- Invalid ID handling
The use of unique kind names (
TestSchemaEnvEnvironmentKind1, etc.) prevents conflicts with parallel tests.
444-453: The test suite usespgtestdb.Custom()which provides isolated database instances per test. Each test receives a fresh database with migrations applied, and records inserted in one test do not persist across other tests. Explicit cleanup of kind records is unnecessary.Likely an incorrect or invalid review comment.
cmd/api/src/database/graphschema.go (4)
46-50: LGTM! Interface properly extended.The new SchemaEnvironment methods follow the established interface patterns. The absence of an
UpdateSchemaEnvironmentmethod appears intentional given the nature of environment mappings (typically immutable once created).
278-294: LGTM! Create implementation follows established patterns.The implementation correctly:
- Uses raw SQL with
RETURNINGfor atomic insert-and-fetch- Handles duplicate key constraint with appropriate error wrapping
- Uses
CheckErrorfor general error handling
296-302: LGTM! Get implementation is correct.Uses
First()which properly returnsgorm.ErrRecordNotFoundfor missing records, correctly translated toErrNotFoundviaCheckError.
304-313: LGTM! Delete implementation follows established patterns.Correctly checks
RowsAffectedto returnErrNotFoundwhen the record doesn't exist.cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
138-145: VerifyUNIQUE(principal_kind)constraint intent.The
UNIQUE(principal_kind)constraint means a principal kind can only belong to one environment globally. This seems restrictive—typically you'd wantUNIQUE(environment_id, principal_kind)to allow the same principal kind in different environments while preventing duplicates within an environment.Please confirm this is the intended behavior.
cmd/api/src/database/mocks/db.go (3)
543-556: CreateSchemaEnvironment mock wiring looks correctSignature, argument order, and return types match surrounding schema methods and
model.SchemaEnvironment; gomock call/record patterns are consistent with the rest of the file.
889-901: DeleteSchemaEnvironment mock matches existing delete patternsTakes
ctxand anint32ID, returns error, and has a corresponding recorder method wired identically toDeleteSchemaEdgeKind/DeleteSchemaNodeKind.
1964-1977: GetSchemaEnvironmentById mock is consistent with other gettersGetter uses
ctx+int32ID and returnsmodel.SchemaEnvironment, error; recorder method mirrorsGetSchemaEdgeKindByIdandGetSchemaNodeKindByIdpatterns.
cweidenkeller
force-pushed
the
BED-6851
branch
2 times, most recently
from
4cd8c46 to
9ded80d
Compare
cweidenkeller
force-pushed
the
BED-6851
branch
from
faafdf9 to
2081e3b
Compare
| RETURNING id, schema_extension_id, environment_kind_id, source_kind_id`, | ||
| schemaEnvironment.TableName()), | ||
| schemaExtensionId, environmentKindId, sourceKindId).Scan(&schemaEnvironment); result.Error != nil { | ||
| if strings.Contains(result.Error.Error(), "duplicate key value violates unique constraint") { |
There was a problem hiding this comment.
If you pull main, there's a const you can use defined for this error
const DuplicateKeyValueErrorString = "duplicate key value violates unique constraint"
Everything else looks good.
|
Gonna approve but can you resolve conflicts and run |
Description
Added schema environments basic crud operations
Motivation and Context
Resolves BED-6851
Why is this change required? What problem does it solve?
This is part of the OG environment expansion.
It allows the app to interact with this new table.
How Has This Been Tested?
Added full suite tests
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Migrations
Tests
✏️ Tip: You can customize this high-level summary in your review settings.