feat: Schema Service: Findings and Remediation Functionality - BED-6853 #2249
Conversation
… of questions about validations
…uple of Lawson's changes to test the environment upsert with a http endpoint
…in and all is well in the world
…ayer + integration tests
WalkthroughAdds a v2 OpenGraph schema ingestion API and service plus DB upsert implementations for environments, findings, principal kinds, and remediations; includes unit/integration tests and mocks. (50 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API\n(OpenGraphSchemaIngest)
participant Service as OpenGraphSchemaService\n(UpsertGraphSchemaExtension)
participant DB as BloodhoundDB\n(Upsert Helpers)
participant Repo as Database\n(persist)
Client->>API: POST /api/v2/opengraphschema\nGraphSchemaExtension JSON
API->>Service: UpsertGraphSchemaExtension(ctx, payload)
Service->>DB: UpsertSchemaEnvironmentWithPrincipalKinds(envs...)
DB->>Repo: GetKindByName / GetEnvironmentByKinds / CreateEnvironment / DeleteEnvironment
Repo-->>DB: ids / results
DB->>Repo: DeletePrincipalKinds / CreatePrincipalKind(s)
Service->>DB: UpsertFinding(findings...)
DB->>Repo: GetSchemaRelationshipFindingByName / DeleteSchemaRelationshipFinding / CreateSchemaRelationshipFinding
Repo-->>DB: finding results
Service->>DB: UpsertRemediation(findingId,...)
DB->>Repo: GetRemediationByFindingId / UpdateRemediation / CreateRemediation
Repo-->>DB: remediation results
DB-->>Service: success
Service-->>API: success
API->>Client: 201 Created
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
kpowderly
added
the
api
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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.go (1)
806-818: Missing duplicate key error handling.Other
Create*methods (e.g.,CreateSchemaEnvironmentat line 586) check for duplicate key violations and wrap them with semantic error types. This method will return a raw database error if a duplicate(environment_id, principal_kind)is inserted.🔧 Suggested fix for consistency
func (s *BloodhoundDB) CreateSchemaEnvironmentPrincipalKind(ctx context.Context, environmentId int32, principalKind int32) (model.SchemaEnvironmentPrincipalKind, error) { var envPrincipalKind model.SchemaEnvironmentPrincipalKind if result := s.db.WithContext(ctx).Raw(` INSERT INTO schema_environments_principal_kinds (environment_id, principal_kind, created_at) VALUES (?, ?, NOW()) RETURNING environment_id, principal_kind, created_at`, environmentId, principalKind).Scan(&envPrincipalKind); result.Error != nil { + if strings.Contains(result.Error.Error(), DuplicateKeyValueErrorString) { + return model.SchemaEnvironmentPrincipalKind{}, fmt.Errorf("duplicate schema environment principal kind: %w", result.Error) + } return model.SchemaEnvironmentPrincipalKind{}, CheckError(result) } return envPrincipalKind, nil }
🤖 Fix all issues with AI agents
In `@cmd/api/src/api/registration/v2.go`:
- Around line 370-371: The PUT /api/v2/extensions route currently registers as
routerInst.PUT("/api/v2/extensions", resources.OpenGraphSchemaIngest) with no
auth; add an authentication/authorization requirement to this route (e.g., chain
.RequireAuth() or .RequirePermissions(permissions.GraphDBIngest) when
registering the route) so that OpenGraphSchemaIngest cannot be called by
unauthenticated users; ensure the chosen permission matches other ingestion
endpoints' pattern and update any tests or route registration order if needed.
In `@cmd/api/src/database/upsert_schema_environment.go`:
- Around line 117-135: The upsertSchemaEnvironment function deletes an existing
environment via GetSchemaEnvironmentByKinds/DeleteSchemaEnvironment before
calling CreateSchemaEnvironment, which is not atomic and can lose data or
cascade-delete principals if CreateSchemaEnvironment fails; fix by performing
the operation inside a database transaction (begin tx, use appropriate row-level
lock or SELECT ... FOR UPDATE on the environment/kind rows via
GetSchemaEnvironmentByKinds) and then either perform an atomic upsert/update
instead of separate delete/create or create the new environment first and only
swap/delete the old within the same transaction (so rollback preserves the
original on failure); ensure DeleteSchemaEnvironment and CreateSchemaEnvironment
are executed using the same transaction context or converted to transactional
variants to avoid intermediate-visible state and unintended cascade deletes.
In `@cmd/api/src/database/upsert_schema_finding.go`:
- Around line 68-69: The function comment for upsertFinding incorrectly
references "environment" instead of "finding"; update the docstring above
upsertFinding to say it deletes an existing finding with the given kinds (or
matching criteria) before creating the new finding, so it accurately describes
the behavior and mentions "finding" rather than "environment".
In `@cmd/api/src/services/opengraphschema/extension_test.go`:
- Around line 95-100: The service currently hardcodes extensionID = 1 in the
Upsert path (see the service implementation in extension.go where
UpsertGraphSchemaExtension is called with int32(1)), which forces all upserts to
extension ID 1; fix by either (A) adding an extensionID field to the request
type v2.GraphSchemaExtension and changing the service handler to pass
request.ExtensionID into mockOpenGraphSchema.UpsertGraphSchemaExtension (and
update related tests to use that field), or (B) if single-extension behavior is
intentional, add clear documentation/comments in the service and API types
explaining why extensionID is fixed at 1 and keep tests expecting int32(1).
Ensure you update any call sites and test expectations (extension_test.go) to
match the chosen approach.
🧹 Nitpick comments (14)
cmd/api/src/database/graphschema.go (1)
600-614: Consider using explicit column selection for consistency.Other methods like
GetSchemaEnvironmentById(line 621) explicitly list columns in the SELECT clause. UsingSELECT *here is inconsistent and could cause issues if the table schema changes.♻️ Suggested change for consistency
func (s *BloodhoundDB) GetSchemaEnvironmentByKinds(ctx context.Context, environmentKindId, sourceKindId int32) (model.SchemaEnvironment, error) { var env model.SchemaEnvironment if result := s.db.WithContext(ctx).Raw( - "SELECT * FROM schema_environments WHERE environment_kind_id = ? AND source_kind_id = ? AND deleted_at IS NULL", + "SELECT id, schema_extension_id, environment_kind_id, source_kind_id, created_at, updated_at, deleted_at FROM schema_environments WHERE environment_kind_id = ? AND source_kind_id = ? AND deleted_at IS NULL", environmentKindId, sourceKindId, ).Scan(&env); result.Error != nil {cmd/api/src/database/kind_integration_test.go (1)
44-59: Consider relaxing the ID assertion for migration resilience.The test hardcodes
ID: 1which assumesTag_Tier_Zerois the first kind inserted. While currently accurate based on the v7.3.0 migration, this could break if migration order changes or if the test database has a different state.♻️ Alternative: Assert only the Name field
want: want{ err: nil, - kind: model.Kind{ - ID: 1, - Name: "Tag_Tier_Zero", - }, + kind: model.Kind{ + Name: "Tag_Tier_Zero", + }, },Then update assertion:
- assert.Equal(t, testCase.want.kind, kind) + assert.Equal(t, testCase.want.kind.Name, kind.Name) + assert.NotZero(t, kind.ID)cmd/api/src/database/sourcekinds.go (1)
97-128: LGTM with a minor consistency note.The implementation correctly queries for an active source kind by name and handles the not-found case appropriately.
Consider using
?placeholders for consistency with other queries in this file (e.g.,RegisterSourceKindon line 46 andDeactivateSourceKindsByNameon line 142). GORM normalizes both, but consistent style aids readability.cmd/api/src/database/sourcekinds_integration_test.go (1)
209-257: Consider adding a test case for non-existent source kind.The test currently only covers the success path. Adding a case for a name that doesn't exist (e.g.,
"NonExistent") would verify that the method returns an appropriate error (likelyErrNotFound), improving coverage of theGetSourceKindByNameimplementation.💡 Suggested additional test case
{ name: "Error: Source Kind not found", args: args{ name: "NonExistent", }, setup: func() IntegrationTestSuite { return setupIntegrationTestSuite(t) }, want: want{ err: database.ErrNotFound, sourceKind: database.SourceKind{}, }, },cmd/api/src/database/upsert_schema_extension_integration_test.go (1)
182-199: Consider adding a minimal assertion for the empty inputs case.While the comment indicates this tests "no error occurred," adding a simple assertion (e.g., verifying no findings exist for the extension) would make the test more explicit and guard against regressions.
cmd/api/src/services/opengraphschema/opengraphschema.go (1)
26-29: Incomplete documentation comment.The comment
// OpenGraphSchemaRepository -appears to be a placeholder. Consider adding a meaningful description of the interface's purpose.📝 Suggested improvement
-// OpenGraphSchemaRepository - +// OpenGraphSchemaRepository defines the database operations for managing graph schema extensions. type OpenGraphSchemaRepository interface { UpsertGraphSchemaExtension(ctx context.Context, extensionID int32, environments []database.EnvironmentInput, findings []database.FindingInput) error }cmd/api/src/database/upsert_schema_environment_integration_test.go (1)
276-306: Consider adding a brief comment about test data requirements.The tests rely on predefined kinds like
Tag_Tier_Zero,Tag_Owned, andBasebeing present in the test database. A brief comment at the top of the test function documenting these dependencies would improve maintainability for future developers.cmd/api/src/services/opengraphschema/extension_test.go (1)
39-44: Consider adding edge case tests.The current tests cover the main scenarios well. For completeness, consider adding tests for edge cases like empty
environmentsorfindingsslices to verify the service handles them gracefully.cmd/api/src/database/upsert_schema_extension.go (1)
48-50: Minor: Inconsistent error message wording.The error message uses "upload" while the method is an "upsert" operation. Consider updating for consistency.
Suggested fix
if err := tx.UpsertSchemaEnvironmentWithPrincipalKinds(ctx, extensionID, env.EnvironmentKindName, env.SourceKindName, env.PrincipalKinds); err != nil { - return fmt.Errorf("failed to upload environment with principal kinds: %w", err) + return fmt.Errorf("failed to upsert environment with principal kinds: %w", err) }cmd/api/src/api/v2/opengraphschema.go (3)
59-63: Remove or update the outdated TODO comment.The implementation appears complete, but the TODO suggests it's a skeleton. If this is ready for use, remove the TODO; otherwise, clarify what remains to be implemented.
66-74: Use the already-assignedctxvariable consistently.The
ctxvariable is assigned on line 62 butrequest.Context()is called again on lines 67 and 72. Usectxthroughout for consistency.Suggested fix
var req GraphSchemaExtension if err := json.NewDecoder(request.Body).Decode(&req); err != nil { - api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusBadRequest, api.ErrorResponsePayloadUnmarshalError, request), response) + api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusBadRequest, api.ErrorResponsePayloadUnmarshalError, request), response) return } if err := s.openGraphSchemaService.UpsertGraphSchemaExtension(ctx, req); err != nil { - api.WriteErrorResponse(request.Context(), api.BuildErrorResponse(http.StatusInternalServerError, fmt.Sprintf("error upserting graph schema extension: %v", err), request), response) + api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusInternalServerError, fmt.Sprintf("error upserting graph schema extension: %v", err), request), response) return }
71-73: Consider limiting error details exposed to clients.The error message includes internal error details via
%v, which could leak implementation details or sensitive information. Consider using a generic message for the client while logging the full error server-side.Suggested approach
if err := s.openGraphSchemaService.UpsertGraphSchemaExtension(ctx, req); err != nil { - api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusInternalServerError, fmt.Sprintf("error upserting graph schema extension: %v", err), request), response) + slog.ErrorContext(ctx, "failed to upsert graph schema extension", "error", err) + api.WriteErrorResponse(ctx, api.BuildErrorResponse(http.StatusInternalServerError, "error upserting graph schema extension", request), response) return }This requires adding
log/slogto imports.cmd/api/src/database/upsert_schema_environment.go (2)
137-158: Principal kinds deletion loop is inefficient and non-atomic.The loop at lines 143-147 deletes principal kinds one-by-one. This has two concerns:
- Performance: Multiple round-trips to the database instead of a single bulk delete
- Atomicity: If deletion fails mid-loop, the environment is left in a partially deleted state
Consider bulk delete operation
If the database layer supports it, a single
DELETE WHERE environment_id = ?would be more efficient and atomic:func (s *BloodhoundDB) upsertPrincipalKinds(ctx context.Context, environmentID int32, principalKinds []model.SchemaEnvironmentPrincipalKind) error { // Bulk delete all existing principal kinds for this environment if err := s.DeleteAllPrincipalKindsForEnvironment(ctx, environmentID); err != nil { return fmt.Errorf("error deleting principal kinds for environment %d: %w", environmentID, err) } // Create the new principal kinds (consider bulk insert as well) for _, kind := range principalKinds { if _, err := s.CreateSchemaEnvironmentPrincipalKind(ctx, environmentID, kind.PrincipalKind); err != nil { return fmt.Errorf("error creating principal kind %d for environment %d: %w", kind.PrincipalKind, environmentID, err) } } return nil }
139-148: Condition at line 141 may not behave as intended.The condition
!errors.Is(err, ErrNotFound)at line 141 is checked aftererr != nil && !errors.Is(err, ErrNotFound)at line 139. When line 139's condition is false (meaning eithererr == nilORerr == ErrNotFound), line 141 enters the block iferr != ErrNotFound.This means:
err == nil: enters block (correct - existing kinds found)err == ErrNotFound: skips block (correct - no existing kinds)The logic works but is harder to follow. Consider explicit nil check for clarity.
Clearer conditional structure
- if existingKinds, err := s.GetSchemaEnvironmentPrincipalKindsByEnvironmentId(ctx, environmentID); err != nil && !errors.Is(err, ErrNotFound) { + existingKinds, err := s.GetSchemaEnvironmentPrincipalKindsByEnvironmentId(ctx, environmentID) + if err != nil && !errors.Is(err, ErrNotFound) { return fmt.Errorf("error retrieving existing principal kinds for environment %d: %w", environmentID, err) - } else if !errors.Is(err, ErrNotFound) { + } + if err == nil && len(existingKinds) > 0 { // Delete all existing principal kinds for _, kind := range existingKinds {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
cmd/api/src/api/registration/registration.gocmd/api/src/api/registration/v2.gocmd/api/src/api/v2/mocks/graphschemaextensions.gocmd/api/src/api/v2/model.gocmd/api/src/api/v2/opengraphschema.gocmd/api/src/database/db.gocmd/api/src/database/graphschema.gocmd/api/src/database/kind.gocmd/api/src/database/kind_integration_test.gocmd/api/src/database/migration/migrations/v8.5.0.sqlcmd/api/src/database/mocks/db.gocmd/api/src/database/sourcekinds.gocmd/api/src/database/sourcekinds_integration_test.gocmd/api/src/database/upsert_schema_environment.gocmd/api/src/database/upsert_schema_environment_integration_test.gocmd/api/src/database/upsert_schema_extension.gocmd/api/src/database/upsert_schema_extension_integration_test.gocmd/api/src/database/upsert_schema_finding.gocmd/api/src/database/upsert_schema_finding_integration_test.gocmd/api/src/database/upsert_schema_remediation.gocmd/api/src/database/upsert_schema_remediation_integration_test.gocmd/api/src/model/kind.gocmd/api/src/services/entrypoint.gocmd/api/src/services/opengraphschema/extension.gocmd/api/src/services/opengraphschema/extension_test.gocmd/api/src/services/opengraphschema/mocks/opengraphschema.gocmd/api/src/services/opengraphschema/opengraphschema.go
🧰 Additional context used
🧠 Learnings (8)
📚 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/upsert_schema_remediation.gocmd/api/src/api/v2/mocks/graphschemaextensions.gocmd/api/src/database/upsert_schema_extension.gocmd/api/src/services/opengraphschema/opengraphschema.gocmd/api/src/database/upsert_schema_environment.gocmd/api/src/database/upsert_schema_finding_integration_test.gocmd/api/src/database/upsert_schema_remediation_integration_test.gocmd/api/src/services/entrypoint.gocmd/api/src/database/graphschema.gocmd/api/src/api/v2/opengraphschema.gocmd/api/src/database/upsert_schema_extension_integration_test.go
📚 Learning: 2025-12-18T21:50:43.837Z
Learnt from: brandonshearin
Repo: SpecterOps/BloodHound PR: 2165
File: cmd/api/src/database/sourcekinds.go:64-69
Timestamp: 2025-12-18T21:50:43.837Z
Learning: In cmd/api/src/database/sourcekinds.go, the GetSourceKinds query should sort results by name ASC at the database layer to match the UI's alphanumeric sorting behavior and maintain consistency.
Applied to files:
cmd/api/src/database/kind_integration_test.gocmd/api/src/database/kind.gocmd/api/src/database/sourcekinds.gocmd/api/src/database/db.gocmd/api/src/database/mocks/db.gocmd/api/src/database/graphschema.gocmd/api/src/database/sourcekinds_integration_test.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/kind_integration_test.gocmd/api/src/database/upsert_schema_finding_integration_test.gocmd/api/src/services/opengraphschema/extension_test.gocmd/api/src/database/upsert_schema_environment_integration_test.gocmd/api/src/database/upsert_schema_remediation_integration_test.gocmd/api/src/database/sourcekinds_integration_test.gocmd/api/src/database/upsert_schema_extension_integration_test.go
📚 Learning: 2025-12-10T20:16:54.652Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 2105
File: cmd/api/src/database/migration/migrations/v8.5.0.sql:139-143
Timestamp: 2025-12-10T20:16:54.652Z
Learning: In the BloodHound schema_environments_principal_kinds table (cmd/api/src/database/migration/migrations/v8.5.0.sql), the PRIMARY KEY(principal_kind) design is intentional and enforces that each principal kind is globally unique and exclusively owned by a single environment. This creates a one-to-many relationship from environments to principal kinds where an environment can have multiple principal kinds, but a principal kind can never be shared between environments.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sqlcmd/api/src/database/upsert_schema_environment.gocmd/api/src/database/upsert_schema_environment_integration_test.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/upsert_schema_extension.gocmd/api/src/database/upsert_schema_environment.gocmd/api/src/database/upsert_schema_environment_integration_test.gocmd/api/src/database/graphschema.go
📚 Learning: 2025-05-27T16:58:33.295Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1503
File: cmd/api/src/services/job/jobs_test.go:19-143
Timestamp: 2025-05-27T16:58:33.295Z
Learning: Tests in cmd/api/src/services/job/jobs_test.go have been found to be flaky in the past and are due for rewrite. They should be skipped with t.Skip() until they can be properly rewritten.
Applied to files:
cmd/api/src/database/upsert_schema_finding_integration_test.gocmd/api/src/services/opengraphschema/extension_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/upsert_schema_remediation_integration_test.go
📚 Learning: 2025-08-28T16:43:43.961Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: packages/go/openapi/doc/openapi.json:18008-18029
Timestamp: 2025-08-28T16:43:43.961Z
Learning: In SpecterOps/BloodHound, packages/go/openapi/doc/openapi.json is generated from YAML under packages/go/openapi/src/schemas; edits must be made to the YAML and then the spec regenerated.
Applied to files:
cmd/api/src/services/entrypoint.gocmd/api/src/api/v2/opengraphschema.go
🧬 Code graph analysis (19)
cmd/api/src/model/kind.go (2)
packages/go/schemagen/tsgen/tsgen.go (1)
ID(216-220)packages/go/graphschema/common/common.go (1)
Name(53-53)
cmd/api/src/database/kind_integration_test.go (2)
cmd/api/src/model/kind.go (1)
Kind(18-21)cmd/api/src/database/kind.go (1)
Kind(24-26)
cmd/api/src/database/kind.go (2)
cmd/api/src/model/kind.go (1)
Kind(18-21)cmd/api/src/database/db.go (2)
BloodhoundDB(197-200)ErrNotFound(42-42)
cmd/api/src/api/v2/mocks/graphschemaextensions.go (2)
cmd/api/src/api/v2/opengraphschema.go (1)
GraphSchemaExtension(32-35)cmd/api/src/model/graphschema.go (2)
GraphSchemaExtension(23-30)GraphSchemaExtension(32-34)
cmd/api/src/services/opengraphschema/mocks/opengraphschema.go (1)
cmd/api/src/database/upsert_schema_extension.go (2)
EnvironmentInput(23-27)FindingInput(29-36)
cmd/api/src/database/db.go (1)
cmd/api/src/database/kind.go (1)
Kind(24-26)
cmd/api/src/database/upsert_schema_extension.go (2)
packages/go/graphschema/common/common.go (1)
DisplayName(54-54)cmd/api/src/database/db.go (1)
BloodhoundDB(197-200)
cmd/api/src/services/opengraphschema/opengraphschema.go (2)
cmd/api/src/database/upsert_schema_extension.go (2)
EnvironmentInput(23-27)FindingInput(29-36)cmd/api/src/api/v2/opengraphschema.go (1)
OpenGraphSchemaService(28-30)
cmd/api/src/database/upsert_schema_environment.go (2)
cmd/api/src/database/db.go (2)
BloodhoundDB(197-200)ErrNotFound(42-42)packages/go/schemagen/tsgen/tsgen.go (1)
ID(216-220)
cmd/api/src/database/upsert_schema_finding_integration_test.go (3)
cmd/api/src/database/db.go (1)
BloodhoundDB(197-200)packages/go/schemagen/tsgen/tsgen.go (1)
ID(216-220)packages/go/graphschema/common/common.go (1)
DisplayName(54-54)
cmd/api/src/database/mocks/db.go (4)
cmd/api/src/model/kind.go (1)
Kind(18-21)cmd/api/src/database/kind.go (1)
Kind(24-26)cmd/api/src/model/graphschema.go (4)
SchemaEnvironment(101-106)SchemaEnvironment(108-110)SchemaRelationshipFinding(113-121)SchemaRelationshipFinding(123-125)cmd/api/src/database/sourcekinds.go (1)
SourceKind(59-63)
cmd/api/src/database/upsert_schema_environment_integration_test.go (2)
cmd/api/src/database/db.go (1)
BloodhoundDB(197-200)packages/go/schemagen/tsgen/tsgen.go (1)
ID(216-220)
cmd/api/src/api/v2/model.go (2)
cmd/api/src/api/v2/opengraphschema.go (1)
OpenGraphSchemaService(28-30)cmd/api/src/services/opengraphschema/opengraphschema.go (1)
OpenGraphSchemaService(31-33)
cmd/api/src/database/upsert_schema_remediation_integration_test.go (2)
cmd/api/src/database/db.go (1)
BloodhoundDB(197-200)packages/go/schemagen/tsgen/tsgen.go (1)
ID(216-220)
cmd/api/src/services/entrypoint.go (1)
cmd/api/src/services/opengraphschema/opengraphschema.go (1)
NewOpenGraphSchemaService(35-39)
cmd/api/src/api/registration/registration.go (3)
cmd/api/src/api/v2/opengraphschema.go (1)
OpenGraphSchemaService(28-30)cmd/api/src/services/opengraphschema/opengraphschema.go (1)
OpenGraphSchemaService(31-33)cmd/api/src/api/v2/model.go (1)
NewResources(123-152)
cmd/api/src/api/v2/opengraphschema.go (4)
cmd/api/src/model/graphschema.go (2)
GraphSchemaExtension(23-30)GraphSchemaExtension(32-34)cmd/api/src/api/v2/model.go (1)
Resources(106-121)cmd/api/src/api/marshalling.go (1)
WriteErrorResponse(77-85)cmd/api/src/api/error.go (1)
BuildErrorResponse(134-145)
cmd/api/src/database/sourcekinds_integration_test.go (2)
cmd/api/src/database/sourcekinds.go (1)
SourceKind(59-63)cmd/api/src/daemons/datapipe/datapipe_integration_test.go (1)
IntegrationTestSuite(50-57)
cmd/api/src/database/upsert_schema_extension_integration_test.go (3)
cmd/api/src/database/upsert_schema_extension.go (3)
EnvironmentInput(23-27)FindingInput(29-36)RemediationInput(38-43)cmd/api/src/database/db.go (1)
BloodhoundDB(197-200)packages/go/graphschema/common/common.go (1)
DisplayName(54-54)
⏰ 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-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (46)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
95-95: LGTM! FK reference correction alignssource_kind_idwith the appropriate table.The foreign key now correctly references
source_kinds(id)instead ofkind(id), which semantically matches the column's purpose.cmd/api/src/database/graphschema.go (4)
56-73: LGTM! Interface additions are well-structured and consistent.The new methods follow the established patterns in the interface, with appropriate context parameters and return types.
681-696: LGTM! Implementation follows established patterns.The method correctly uses parameterized queries, explicit column selection, and consistent error handling with
ErrNotFoundfor missing records.
820-832: LGTM! Implementation correctly returns empty slice for no results.For list queries returning multiple records, returning an empty slice rather than
ErrNotFoundis the appropriate pattern, consistent withGetSchemaEnvironments.
834-845: LGTM! Delete implementation follows established patterns.cmd/api/src/model/kind.go (1)
18-21: LGTM! Simple and appropriate data model.The
Kindstruct correctly represents thekindtable with appropriate JSON tags for API serialization.cmd/api/src/database/kind_integration_test.go (1)
61-74: LGTM! Test execution logic is well-structured.The test properly sets up and tears down the test suite, handles both error and success cases, and uses appropriate testify assertions.
cmd/api/src/database/db.go (1)
192-194: LGTM!The embedding of the
Kindinterface follows the established pattern in this file and cleanly extends theDatabaseinterface with the newGetKindByNamecapability.cmd/api/src/services/opengraphschema/extension.go (2)
56-59: Verify the hardcodedextensionID = 1.The extension ID is hardcoded to
1, which means all upserted environments and findings will be associated with the same extension. If the system is intended to support multiple extensions in the future, this should be derived from the request or a lookup.
40-54: VerifyRemediationis always present in findings.The code accesses
finding.Remediationfields directly without a nil check. IfRemediationcan be nil or omitted in the API request, this will panic. Ensure the model enforcesRemediationas required, or add a nil guard.cmd/api/src/database/upsert_schema_remediation.go (1)
24-41: LGTM!The upsert logic correctly handles the three cases: unexpected error, existing remediation (update), and not found (create). Error wrapping provides good context for debugging.
cmd/api/src/api/v2/model.go (1)
119-119: LGTM!The
OpenGraphSchemaServiceis correctly wired into theResourcesstruct, following the established dependency injection pattern.Also applies to: 134-134, 150-150
cmd/api/src/database/sourcekinds_integration_test.go (3)
145-145: LGTM!Switching from
require.EqualErrortoassert.EqualErroris reasonable here since subsequent assertions would still provide context if the error check fails.
200-200: LGTM!Consistent with the same change pattern applied elsewhere in the file.
426-426: LGTM!Consistent assertion change pattern.
cmd/api/src/database/kind.go (2)
24-26: LGTM!Clean interface definition that follows the existing pattern in the codebase.
28-47: Implementation looks correct; the dual check on line 42 is a safe defensive pattern.The check
result.RowsAffected == 0 || kind.ID == 0is appropriate since GORM'sRaw().Scan()may not always populateRowsAffectedreliably depending on the database driver. Thekind.ID == 0fallback ensures correctness.Based on learnings, consider adding
ORDER BY name ASCif this query could ever return multiple matches, though for a single-name lookup this is not strictly necessary.cmd/api/src/database/upsert_schema_extension_integration_test.go (4)
30-40: Well-structured test setup.The test structure with
setupDatareturning theextensionIdand a separateassertfunction provides good separation of concerns for table-driven tests.
42-120: LGTM!Comprehensive test case that validates both findings and remediations are created correctly with all expected fields.
121-181: LGTM!Good coverage for the update scenario, verifying that existing findings and remediations are properly updated.
201-226: LGTM!The test runner correctly handles both success and error paths, with proper setup/teardown lifecycle management.
cmd/api/src/database/upsert_schema_remediation_integration_test.go (4)
30-40: LGTM!Clean test structure with appropriate type definitions for test arguments.
41-77: LGTM!Good coverage for the update scenario with comprehensive field validation.
78-112: LGTM!Properly tests the creation path when no remediation exists, validating all fields.
115-145: LGTM!The explicit transaction wrapping (lines 123-132) is appropriate for testing
UpsertRemediationsince the production code likely expects to run within a transaction context.cmd/api/src/database/upsert_schema_finding_integration_test.go (4)
31-41: LGTM!Well-structured test definition with clear argument types matching the
UpsertFindingmethod signature.
42-76: LGTM!Good coverage for the update (delete and re-create) scenario with appropriate assertions.
77-107: LGTM!Properly tests the creation path with comprehensive validation.
110-145: LGTM!The pattern of declaring
findingResponseoutside the transaction closure (line 117) and capturing it inside (line 129) is the correct approach for extracting results from a transaction callback that only returns an error.cmd/api/src/services/entrypoint.go (1)
42-42: LGTM! Clean service wiring following established patterns.The new
OpenGraphSchemaServiceis properly instantiated and wired into the route registration, consistent with how other services likedogtagsServiceare integrated.Also applies to: 130-130, 134-134
cmd/api/src/services/opengraphschema/opengraphschema.go (1)
31-38: LGTM!The service struct and constructor follow standard Go patterns for dependency injection.
cmd/api/src/database/upsert_schema_environment_integration_test.go (1)
31-43: Well-structured integration tests with good coverage.The test suite covers key scenarios including success cases, error propagation, and transaction rollback verification. The table-driven approach is clean and maintainable.
cmd/api/src/database/upsert_schema_finding.go (2)
28-55: LGTM! Clean validation flow.The function properly validates all kind references before performing the upsert operation, with clear error propagation at each step.
70-86: Delete-then-create upsert pattern is correctly wrapped in transactions.All call sites to
UpsertFinding()are properly wrapped within transaction contexts: the main production call inUpsertGraphSchemaExtension()(line 46, upsert_schema_extension.go) and the integration test (line 119, upsert_schema_finding_integration_test.go). The delete-then-create approach ensures safe upsert semantics under transaction isolation, and error handling is appropriate.cmd/api/src/services/opengraphschema/extension_test.go (1)
260-284: Good test structure with proper parallelization.The test follows best practices with isolated mock controllers per subtest, allowing safe parallel execution. Based on learnings from this codebase, this pattern is acceptable.
cmd/api/src/services/opengraphschema/mocks/opengraphschema.go (1)
1-72: LGTM!This is a correctly generated gomock file for the
OpenGraphSchemaRepositoryinterface. The mock signature aligns with the interface definition and follows standard gomock patterns.cmd/api/src/api/v2/mocks/graphschemaextensions.go (1)
1-72: LGTM!This is a correctly generated gomock file for the
OpenGraphSchemaServiceinterface. The mock properly usesv2.GraphSchemaExtensionfor the API layer.cmd/api/src/database/upsert_schema_extension.go (1)
45-65: LGTM - Good transactional structure.The method correctly wraps all operations in a transaction, ensuring atomicity for the upsert workflow. The error handling with
%wwrapping preserves the error chain for debugging.cmd/api/src/database/mocks/db.go (4)
1950-1963: LGTM!The
GetKindByNamemock method is correctly generated and follows standard gomock patterns.
2205-2218: LGTM!The
GetSchemaEnvironmentByKindsmock method is correctly generated with proper parameter types.
2265-2278: LGTM!The
GetSchemaRelationshipFindingByNamemock method is correctly generated.
2366-2379: LGTM!The
GetSourceKindByNamemock method is correctly generated and returns the expecteddatabase.SourceKindtype.cmd/api/src/database/upsert_schema_environment.go (3)
64-73: Approve validation logic with minor style note.The error handling correctly distinguishes between "not found" and other errors. However, the nested
elseafterelse ifwith early returns could be simplified for readability.Optional: Simplified structure
func (s *BloodhoundDB) validateAndTranslateEnvironmentKind(ctx context.Context, environmentKindName string) (int32, error) { - if envKind, err := s.GetKindByName(ctx, environmentKindName); err != nil && !errors.Is(err, ErrNotFound) { + envKind, err := s.GetKindByName(ctx, environmentKindName) + if err != nil && !errors.Is(err, ErrNotFound) { return 0, fmt.Errorf("error retrieving environment kind '%s': %w", environmentKindName, err) - } else if errors.Is(err, ErrNotFound) { + } + if errors.Is(err, ErrNotFound) { return 0, fmt.Errorf("environment kind '%s' not found", environmentKindName) - } else { - return envKind.ID, nil } + return envKind.ID, nil }
75-95: The TOCTOU race condition here is mitigated by upsert semantics.While a race window exists between
GetSourceKindByName(line 78-82) returningErrNotFoundandRegisterSourceKind(line 86) executing, it's not actually problematic.RegisterSourceKindusesON CONFLICT (name) DO UPDATE SET active = truefor idempotent registration, so concurrent attempts to register the same source kind will safely converge—the second registration will simply update the existing record. The subsequentGetSourceKindByNamecall will then succeed.
27-62: Transaction wrapper is correctly placed at the caller level, not within this method.
UpsertSchemaEnvironmentWithPrincipalKindsis designed to be called within a transaction context (viaUpsertGraphSchemaExtension), which correctly wraps the operation usings.Transaction(). The method is not responsible for managing transaction boundaries; this design follows proper separation of concerns and avoids nested transactions.cmd/api/src/api/registration/registration.go (1)
55-87: LGTM - Clean dependency injection wiring.The new
openGraphSchemaServiceparameter is correctly added toRegisterFossRoutesand properly forwarded tov2.NewResources. The typev2.OpenGraphSchemaServicematches the interface expected byNewResourcesas shown in the relevant code snippets.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/api/src/database/mocks/db.go`:
- Around line 494-507: The mock no longer matches the database.Database
interface because method names changed—update the mock to match the current
interface by regenerating gomock mocks from the authoritative database.Database
interface, or else make method names consistent across the codebase (rename
CreateRelationshipFinding/DeleteRelationshipFinding/GetRelationshipFinding* to
the original CreateSchemaRelationshipFinding/etc., or update the interface and
all callers to the new names); ensure MockDatabase contains the same method
signatures as the interface (notably CreateSchemaRelationshipFinding or the new
CreateRelationshipFinding, plus the related Schema* methods) and then re-run
mock generation so MockDatabase/MockDatabaseMockRecorder (and the methods
referenced) compile.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cmd/api/src/config/config.gocmd/api/src/config/default.gocmd/api/src/database/mocks/db.gocmd/api/src/database/upsert_schema_finding.go
✅ Files skipped from review due to trivial changes (2)
- cmd/api/src/config/default.go
- cmd/api/src/config/config.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kpowderly
Repo: SpecterOps/BloodHound PR: 2249
File: cmd/api/src/api/registration/v2.go:370-371
Timestamp: 2026-01-14T18:33:03.091Z
Learning: The PUT /api/v2/extensions endpoint for OpenGraphSchemaIngest in cmd/api/src/api/registration/v2.go is intentionally registered without authentication during development, with authentication to be added in a separate ticket.
📚 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/upsert_schema_finding.go
🧬 Code graph analysis (2)
cmd/api/src/database/upsert_schema_finding.go (1)
cmd/api/src/database/db.go (2)
BloodhoundDB(197-200)ErrNotFound(42-42)
cmd/api/src/database/mocks/db.go (2)
cmd/api/src/vendormocks/dawgs/graph/mock.go (2)
MockDatabase(38-42)MockDatabaseMockRecorder(45-47)cmd/api/src/model/graphschema.go (2)
SchemaRelationshipFinding(113-121)SchemaRelationshipFinding(123-125)
🪛 golangci-lint (2.5.0)
cmd/api/src/database/upsert_schema_finding.go
[major] 33-33: : # github.com/specterops/bloodhound/cmd/api/src/daemons/gc [github.com/specterops/bloodhound/cmd/api/src/daemons/gc.test]
cmd/api/src/daemons/gc/data_pruning_test.go:33:33: cannot use mocks.NewMockDatabase(mockCtrl) (value of type *mocks.MockDatabase) as database.Database value in argument to NewDataPruningDaemon: *mocks.MockDatabase does not implement database.Database (missing method CreateSchemaRelationshipFinding)
cmd/api/src/daemons/gc/data_pruning_test.go:41:33: cannot use mocks.NewMockDatabase(mockCtrl) (value of type *mocks.MockDatabase) as database.Database value in argument to NewDataPruningDaemon: *mocks.MockDatabase does not implement database.Database (missing method CreateSchemaRelationshipFinding)
cmd/api/src/daemons/gc/data_pruning_test.go:62:33: cannot use mockDB (variable of type *mocks.MockDatabase) as database.Database value in argument to NewDataPruningDaemon: *mocks.MockDatabase does not implement database.Database (missing method CreateSchemaRelationshipFinding)
(typecheck)
⏰ 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-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (3)
cmd/api/src/database/upsert_schema_finding.go (3)
26-56: Upsert flow is clean and readable.The validation sequence and error propagation are straightforward and easy to follow.
59-67: Relationship-kind validation is clear and robust.Nice separation of not-found vs retrieval errors with contextual messages.
70-83: No action required—nameis globally unique per database schema.The
schema_relationship_findingstable defines aUNIQUE(name)constraint (line 115 in v8.5.0.sql), ensuring that finding names are globally unique across all records, regardless of extension, environment, or relationship kind. ThereplaceFindingfunction correctly relies on this constraint:GetSchemaRelationshipFindingByNamequeries by name alone, finds at most one record, and deletes by ID. Scoping the lookup toextensionId/environmentId/relationshipKindIdis unnecessary and would contradict the database schema design.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Description
Findings and Remediations
Flow:
Motivation and Context
Resolves BED-6853
When an extension is uploaded, this will cover the findings and remediation upsert to the database.
How Has This Been Tested?
Unit tests/Integration tests
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.