feat: Schema Service: Environment Functionality - BED-6852 #2220
Conversation
… of questions about validations
WalkthroughThis PR adds OpenGraph schema extension ingestion functionality to the API. It introduces a new PUT Changes
Sequence DiagramsequenceDiagram
participant Client
participant Handler as HTTP Handler<br/>(OpenGraphSchemaIngest)
participant Service as OpenGraphSchemaService
participant DB as BloodhoundDB
participant TX as Transaction
Client->>Handler: PUT /api/v2/extensions<br/>GraphSchemaExtension
Handler->>Handler: Decode JSON request
Handler->>Service: UpsertGraphSchemaExtension(ctx, req)
Service->>Service: Map Environments to<br/>EnvironmentInput[]
Service->>DB: UpsertGraphSchemaExtension(ctx,<br/>extensionID, environments)
DB->>TX: Begin transaction
loop For each environment
DB->>DB: UpsertSchemaEnvironmentWithPrincipalKinds()
DB->>DB: validateAndTranslateEnvironmentKind()
DB->>DB: GetKindByName(environmentKind)
DB->>DB: validateAndTranslateSourceKind()
DB->>DB: GetSourceKindByName(sourceKind)
DB->>DB: RegisterSourceKind() if needed
DB->>DB: validateAndTranslatePrincipalKinds()
DB->>DB: replaceSchemaEnvironment()
DB->>DB: GetEnvironmentByKinds()
DB->>DB: DeleteEnvironment() if exists
DB->>DB: CreateEnvironment()
DB->>DB: replacePrincipalKinds()
DB->>DB: DeletePrincipalKind() old
DB->>DB: CreatePrincipalKind() new
end
DB->>TX: Commit
Service->>Handler: nil (success)
Handler->>Client: 201 Created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-12-10T20:16:54.652ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting 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 |
…uple of Lawson's changes to test the environment upsert with a http endpoint
kpowderly
changed the title
…in and all is well in the world
…ayer + integration tests
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/ui/src/rendering/utils/dagre/dagre.test.ts (1)
74-83: Don’t maskedgeDataparse failures with|| ''.
AssertedgeDatais defined (and hassource/target) before queryinggraphlibGraph.edge(...), otherwise this test can pass whilegetEdgeDataFromKeyis broken.
🤖 Fix all issues with AI agents
In @cmd/api/src/api/registration/v2.go:
- Line 368: The PUT route registration for "/api/v2/extensions" currently calls
resources.OpenGraphSchemaIngest without any auth; update the route to use the
same authentication and authorization middleware used by the file-upload
data-ingest endpoints (the pattern at lines ~127-132) so only authenticated
users with the appropriate ingest/schema-modification permission can call it;
specifically, modify the routerInst.PUT("/api/v2/extensions",
resources.OpenGraphSchemaIngest) line to include the project/app auth middleware
and the authorize middleware/scopes used by those endpoints (wrap before
resources.OpenGraphSchemaIngest) so the OpenGraphSchemaIngest handler is
protected.
In @cmd/api/src/database/kind_integration_test.go:
- Around line 67-68: When asserting expected errors in the test, avoid calling
err.Error() when err may be nil and fix the swapped arguments passed to
assert.EqualError: first check that err is non-nil (e.g., assert.Error or an
explicit nil-check/fatal if you must) and then call assert.EqualError(t, err,
testCase.want.err.Error()) (or compare against testCase.want.err.Error() as the
expected string); update the block that currently reads assert.EqualError(t,
testCase.want.err, err.Error()) to perform the nil-safe check and pass (err,
expectedString) in the correct order.
In @cmd/api/src/database/upsert_schema_environment.go:
- Around line 119-121: Update the function comment for upsertSchemaEnvironment
to accurately describe its behavior: state that it looks up and deletes any
existing schema environment by environment kind and source kind (via
GetSchemaEnvironmentByKinds) before creating the new one, rather than saying it
checks existence by ID; reference the function name upsertSchemaEnvironment and
the lookup method GetSchemaEnvironmentByKinds so the comment clearly matches the
implementation.
In @cmd/api/src/services/opengraphschema/extension.go:
- Around line 25-33: UpsertGraphSchemaExtension is using a hardcoded
schemaExtensionId of 1 when calling
openGraphSchemaRepository.UpsertSchemaEnvironmentWithPrincipalKinds; replace
that literal with the actual extension ID from the GraphSchemaExtension request
(e.g., use req.SchemaExtensionId or req.Id), or add a schemaExtensionId
parameter to UpsertGraphSchemaExtension if the request type lacks it, then pass
that variable into UpsertSchemaEnvironmentWithPrincipalKinds and update any call
sites and tests accordingly so each environment is associated with the correct
schema extension ID instead of 1.
In @cmd/api/src/services/opengraphschema/opengraphschema_test.go:
- Around line 34-37: The test struct field schemaExtensionId is defined but
never used when invoking service.UpsertGraphSchemaExtension; either remove
schemaExtensionId from the args struct and all test cases if the API doesn't
require an explicit ID, or wire it into the call by setting the
GraphSchemaExtension ID (or appropriate field) before calling
service.UpsertGraphSchemaExtension in the test (update the test helper that
constructs GraphSchemaExtension to include schemaExtensionId); locate references
to args.schemaExtensionId, the args struct definition, and the
service.UpsertGraphSchemaExtension call to implement the chosen fix.
🧹 Nitpick comments (10)
cmd/ui/src/rendering/utils/dagre/dagre.test.ts (2)
23-25: Avoid cross-test coupling:sigmaGraph/graphlibGraphare shared across tests.
Consider re-initializing them inbeforeEach()(or clearing them) so tests don’t depend on execution order or helper “reset” behavior.Also applies to: 29-59, 72-84
61-70: Restoreconsole.warnspy/mocks to prevent leakage into other tests.
UsewarnLogSpy.mockRestore()/vi.restoreAllMocks()inafterEach()(and avoid permanently overwritingconsole.warn).cmd/ui/src/rendering/utils/dagre/dagre.ts (1)
101-102: Note: This formatting change appears unrelated to the PR's stated objectives.This is a minor formatting adjustment with no behavioral impact. However, it seems unrelated to the Schema Service Environment functionality described in the PR objectives (BED-6852). Consider whether this change was included accidentally or should be in a separate PR for code formatting.
cmd/api/src/database/kind_integration_test.go (1)
28-75: Consider expanding test coverage to include error cases.The test currently only covers the success path where
Tag_Tier_Zeroexists. Consider adding test cases for error scenarios such as:
- Kind not found (non-existent kind name)
- Empty string as kind name
- Database connection errors (if applicable)
This would improve confidence in the error handling paths of
GetKindByName.cmd/api/src/services/opengraphschema/opengraphschema.go (1)
24-37: LGTM!Clean interface definition and service struct following Go conventions. The mock generation directive is correctly placed.
Minor: The trailing dash in the comment on Line 24 (
// OpenGraphSchemaRepository -) could be removed for consistency, but this is purely cosmetic.cmd/api/src/api/v2/opengraphschema.go (3)
28-40: Consider adding validation tags to the request structures.The
GraphSchemaExtensionandEnvironmentstructs lack validation tags (e.g.,validate:"required"). The PR objectives mention documented validation rules (bh-rfc-4.md), and the handler at line 54 calls the service directly without explicit validation.If validation is performed downstream in the service layer, this is acceptable. However, if these structures represent the API contract, consider adding validation tags for clarity and fail-fast behavior.
Example validation tags
type GraphSchemaExtension struct { - Environments []Environment `json:"environments"` + Environments []Environment `json:"environments" validate:"required,dive"` } type Environment struct { - EnvironmentKind string `json:"environmentKind"` - SourceKind string `json:"sourceKind"` - PrincipalKinds []string `json:"principalKinds"` + EnvironmentKind string `json:"environmentKind" validate:"required"` + SourceKind string `json:"sourceKind" validate:"required"` + PrincipalKinds []string `json:"principalKinds" validate:"required,dive,required"` }
50-50: Use the extracted context variable consistently.Lines 50 and 55 call
api.WriteErrorResponse(request.Context(), ...)instead of using thectxvariable extracted at line 45. This inconsistency can be confusing.Consistent context usage
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 }Also applies to: 55-55
59-59: Consider returning a response body on success.The handler returns
201 Createdwith no response body. While this is valid HTTP, it's common for creation endpoints to return the created resource or at least an acknowledgment message. This can improve API usability and debugging.If the service returns identifiers or confirmation data, consider including them in the response. Otherwise, this implementation is acceptable for a simple upsert operation.
cmd/api/src/database/graphschema.go (1)
731-743: Consider explicit error handling for duplicate principal kinds.Based on the learnings,
principal_kindis the PRIMARY KEY inschema_environments_principal_kinds, enforcing global uniqueness. If a principal kind is already associated with a different environment, this insert will fail with a constraint violation.While the current implementation allows the database error to propagate (via
CheckError), consider explicitly handlingDuplicateKeyValueErrorStringlike other methods in this file (e.g., lines 105-107, 212-214) to provide a more descriptive error message.Based on learnings, as principal kinds are exclusively owned by a single environment.
cmd/api/src/database/upsert_schema_environment.go (1)
99-117: Consider validating for duplicate principal kinds upfront.If
principalKindNamescontains duplicates, this will pass validation but fail later during insertion due to thePRIMARY KEY(principal_kind)constraint. Pre-checking for duplicates would provide a clearer error message to the caller. Based on learnings, each principal kind is globally unique and exclusively owned by a single environment.💡 Optional: Add duplicate detection
func (s *BloodhoundDB) validateAndTranslatePrincipalKinds(ctx context.Context, principalKindNames []string) ([]model.SchemaEnvironmentPrincipalKind, error) { principalKinds := make([]model.SchemaEnvironmentPrincipalKind, len(principalKindNames)) + seen := make(map[string]struct{}, len(principalKindNames)) for i, kindName := range principalKindNames { + if _, exists := seen[kindName]; exists { + return nil, fmt.Errorf("duplicate principal kind '%s'", kindName) + } + seen[kindName] = struct{}{} + if kind, err := s.GetKindByName(ctx, kindName); err != nil && !errors.Is(err, ErrNotFound) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
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/config/config.gocmd/api/src/config/default.gocmd/api/src/database/db.gocmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.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/model/graphschema.gocmd/api/src/model/kind.gocmd/api/src/services/entrypoint.gocmd/api/src/services/opengraphschema/extension.gocmd/api/src/services/opengraphschema/mocks/opengraphschema.gocmd/api/src/services/opengraphschema/opengraphschema.gocmd/api/src/services/opengraphschema/opengraphschema_test.gocmd/ui/src/rendering/utils/dagre/dagre.test.tscmd/ui/src/rendering/utils/dagre/dagre.tspackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/ObjectCountPanel.tsx
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
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.
📚 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.gocmd/api/src/database/db.gocmd/api/src/database/kind_integration_test.gocmd/api/src/database/sourcekinds_integration_test.gocmd/api/src/database/sourcekinds.gocmd/api/src/database/mocks/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/upsert_schema_environment.gocmd/api/src/database/graphschema.gocmd/api/src/database/upsert_schema_environment_integration_test.gocmd/api/src/database/mocks/db.go
📚 Learning: 2025-08-28T19:26:03.304Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1829
File: packages/javascript/bh-shared-ui/src/views/ZoneManagement/ZoneAnalysisIcon.tsx:26-26
Timestamp: 2025-08-28T19:26:03.304Z
Learning: In packages/javascript/bh-shared-ui/src/hooks/, useZonePathParams is exported through the useZoneParams barrel (useZoneParams/index.ts exports it via wildcard from useZonePathParams.tsx), and usePrivilegeZoneAnalysis is exported through useConfiguration.ts. Both are available via the main hooks barrel import.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/ObjectCountPanel.tsx
📚 Learning: 2026-01-08T22:00:44.868Z
Learnt from: TheNando
Repo: SpecterOps/BloodHound PR: 2230
File: packages/javascript/bh-shared-ui/src/utils/object.ts:17-19
Timestamp: 2026-01-08T22:00:44.868Z
Learning: In the bh-shared-ui package, ensure that public utilities, types, and components are exported from the package’s API surface so they can be consumed by parent projects. This applies broadly to all files under packages/javascript/bh-shared-ui/src, not just this specific file. Avoid exporting internal, implementation-only modules that aren’t meant for external consumption; document and surface stable APIs for downstream users.
Applied to files:
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/ObjectCountPanel.tsx
📚 Learning: 2025-07-30T15:02:05.134Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:02:05.134Z
Learning: BloodHound migrations consistently use `current_timestamp` for created_at and updated_at fields across all tables (feature_flags, permissions, roles, parameters, etc.). This is the established pattern from schema.sql through all version migrations (v6.1.0+).
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 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/graphschema_integration_test.gocmd/api/src/model/graphschema.gocmd/api/src/database/upsert_schema_environment.gocmd/api/src/database/graphschema.gocmd/api/src/database/upsert_schema_environment_integration_test.go
📚 Learning: 2025-07-30T15:01:15.324Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:01:15.324Z
Learning: BloodHound migration files consistently use `current_timestamp` for both `created_at` and `updated_at` fields in INSERT statements across all migration versions. This is the established pattern that should be maintained for consistency across the codebase.
Applied to files:
cmd/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/kind_integration_test.gocmd/api/src/services/opengraphschema/opengraphschema_test.gocmd/api/src/database/upsert_schema_environment_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/api/v2/mocks/graphschemaextensions.gocmd/api/src/database/upsert_schema_environment.gocmd/api/src/database/graphschema.gocmd/api/src/services/entrypoint.gocmd/api/src/database/upsert_schema_environment_integration_test.go
📚 Learning: 2025-06-17T22:37:36.389Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1595
File: cmd/api/src/api/v2/saved_queries_test.go:2594-2594
Timestamp: 2025-06-17T22:37:36.389Z
Learning: In Go table-driven tests, there's a distinction between main test function parallelism and subtest parallelism. Main test functions can safely use t.Parallel() for performance benefits, but individual subtests within table-driven tests may need to run sequentially to avoid race conditions with mocks, deferred functions, or shared resources.
Applied to files:
cmd/api/src/services/opengraphschema/opengraphschema_test.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/services/opengraphschema/opengraphschema_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.go
🧬 Code graph analysis (17)
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/database/db.go (1)
cmd/api/src/database/kind.go (1)
Kind(24-26)
cmd/api/src/api/registration/registration.go (3)
cmd/api/src/services/opengraphschema/opengraphschema.go (1)
OpenGraphSchemaService(29-31)cmd/api/src/api/v2/opengraphschema.go (1)
OpenGraphSchemaService(28-30)cmd/api/src/api/v2/model.go (1)
NewResources(121-148)
cmd/api/src/database/kind_integration_test.go (3)
cmd/api/src/database/kind.go (1)
Kind(24-26)cmd/api/src/model/kind.go (1)
Kind(18-21)packages/go/schemagen/tsgen/tsgen.go (1)
ID(216-220)
cmd/api/src/database/sourcekinds_integration_test.go (4)
cmd/api/src/database/sourcekinds.go (1)
SourceKind(59-63)cmd/api/src/daemons/datapipe/datapipe_integration_test.go (1)
IntegrationTestSuite(50-57)packages/go/schemagen/tsgen/tsgen.go (1)
ID(216-220)packages/go/graphify/graph/graph.go (1)
Name(54-54)
cmd/api/src/database/sourcekinds.go (1)
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/model/graphschema.go (2)
GraphSchemaExtension(23-30)GraphSchemaExtension(32-34)cmd/api/src/api/v2/opengraphschema.go (1)
GraphSchemaExtension(32-34)
cmd/api/src/api/v2/model.go (2)
cmd/api/src/services/opengraphschema/opengraphschema.go (1)
OpenGraphSchemaService(29-31)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)cmd/api/src/database/kind.go (1)
Kind(24-26)
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/services/opengraphschema/extension.go (2)
cmd/api/src/services/opengraphschema/opengraphschema.go (1)
OpenGraphSchemaService(29-31)cmd/api/src/api/v2/opengraphschema.go (2)
OpenGraphSchemaService(28-30)GraphSchemaExtension(32-34)
cmd/api/src/services/opengraphschema/opengraphschema_test.go (3)
cmd/api/src/services/opengraphschema/mocks/opengraphschema.go (2)
MockOpenGraphSchemaRepository(36-40)NewMockOpenGraphSchemaRepository(48-52)cmd/api/src/database/sourcekinds.go (1)
SourceKind(59-63)cmd/api/src/services/opengraphschema/opengraphschema.go (1)
NewOpenGraphSchemaService(33-37)
cmd/api/src/database/graphschema.go (3)
cmd/api/src/model/graphschema.go (5)
SchemaEnvironment(101-106)SchemaEnvironment(108-110)SchemaEnvironmentPrincipalKind(141-145)SchemaEnvironmentPrincipalKind(147-149)SchemaEnvironmentPrincipalKinds(139-139)cmd/api/src/database/db.go (2)
BloodhoundDB(197-200)ErrNotFound(42-42)cmd/api/src/database/helper.go (1)
CheckError(39-45)
cmd/api/src/api/v2/opengraphschema.go (3)
cmd/api/src/api/v2/model.go (1)
Resources(105-119)cmd/api/src/api/marshalling.go (1)
WriteErrorResponse(77-85)cmd/api/src/api/error.go (2)
BuildErrorResponse(134-145)ErrorResponsePayloadUnmarshalError(60-60)
cmd/api/src/services/entrypoint.go (1)
cmd/api/src/services/opengraphschema/opengraphschema.go (1)
NewOpenGraphSchemaService(33-37)
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/database/mocks/db.go (4)
cmd/api/src/model/graphschema.go (5)
SchemaEnvironmentPrincipalKind(141-145)SchemaEnvironmentPrincipalKind(147-149)SchemaEnvironment(101-106)SchemaEnvironment(108-110)SchemaEnvironmentPrincipalKinds(139-139)cmd/api/src/database/kind.go (1)
Kind(24-26)cmd/api/src/model/kind.go (1)
Kind(18-21)cmd/api/src/database/sourcekinds.go (1)
SourceKind(59-63)
🪛 GitHub Actions: Run Go Unit Tests
cmd/api/src/database/upsert_schema_environment_integration_test.go
[error] 275-275: undefined: setupIntegrationTestSuite
⏰ 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). (3)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (36)
packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/ObjectCountPanel.tsx (1)
23-23: Import reordering looks good.The reorganization of imports improves readability by grouping utility imports together, and the component logic remains correct and unchanged.
cmd/ui/src/rendering/utils/dagre/dagre.test.ts (1)
17-22: Import reorder is fine; no behavioral impact.cmd/api/src/config/config.go (1)
165-165: Formatting change only—no functional impact.The DisableTimeoutLimit field whitespace alignment is purely cosmetic.
cmd/api/src/config/default.go (1)
68-68: Formatting change only—no functional impact.The DisableTimeoutLimit field whitespace alignment is purely cosmetic with no change to the default value.
cmd/api/src/database/db.go (1)
193-194: LGTM! Proper interface composition.The Kind interface embedding follows the established pattern in this codebase and appropriately extends the Database interface with kind lookup capabilities needed for the environment functionality.
cmd/api/src/database/migration/migrations/v8.5.0.sql (2)
96-96: LGTM! Correct foreign key reference.The foreign key reference to
source_kinds(id)is semantically correct for a column namedsource_kind_id, and aligns with the broader Source Kinds integration in this PR.
151-151: LGTM! Timestamp follows established pattern.The
created_atcolumn withcurrent_timestampdefault follows the established pattern consistently used across BloodHound migrations.cmd/api/src/model/kind.go (1)
18-21: LGTM! Clean and minimal data model.The Kind struct is well-defined with appropriate field types and JSON tags for its intended use in kind lookups.
cmd/api/src/services/opengraphschema/opengraphschema_test.go (2)
476-478: LGTM: Safe parallel test execution.The use of
t.Parallel()is appropriate here since each subtest creates its own isolatedgomock.Controllerand mock expectations, with no shared state between subtests.Based on learnings, this pattern is acceptable for table-driven tests with isolated mock controllers.
44-473: LGTM: Comprehensive test coverage.The test suite provides excellent coverage of the upsert flow, including:
- Environment kind validation errors
- Source kind validation and registration flows
- Principal kind validation
- Schema environment upsert operations (create, delete, errors)
- Principal kinds management
- Multiple environment processing
The mock expectations correctly simulate the repository layer behavior across all scenarios.
cmd/api/src/database/sourcekinds_integration_test.go (1)
209-257: LGTM: Well-structured integration test.The
TestGetSourceKindByNameintegration test follows the established patterns in this file and correctly validates the retrieval of source kinds by name. The test setup, execution, and assertions are all appropriate.cmd/api/src/api/v2/model.go (1)
118-146: LGTM: Properly wired OpenGraphSchemaService dependency.The addition of the
openGraphSchemaServicefield to theResourcesstruct and its initialization inNewResourcesfollows the established pattern for dependency injection in this codebase. The unexported field visibility is consistent with other service dependencies.cmd/api/src/api/registration/registration.go (1)
65-84: LGTM: Correct dependency propagation.The addition of the
openGraphSchemaServiceparameter and its propagation tov2.NewResourcescorrectly wires the new service into the route registration flow.cmd/api/src/database/kind.go (1)
28-46: LGTM!Clean implementation following existing codebase patterns. The parameterized query prevents SQL injection, and the dual check for
RowsAffected == 0 || kind.ID == 0properly handles edge cases where GORM might not setRowsAffectedcorrectly withScan.cmd/api/src/services/entrypoint.go (2)
41-41: LGTM!Import added correctly for the new opengraphschema package.
121-125: LGTM!Service instantiation and route registration follow the established patterns in the codebase. The
openGraphSchemaServiceis correctly created with the RDMS connection and passed through to route registration.cmd/api/src/database/sourcekinds.go (1)
97-128: LGTM!Implementation is consistent with
GetKindByNameand follows established patterns. Theactive = truefilter appropriately ensures only active source kinds are returned, and the conversion fromrawSourceKindtoSourceKindwithgraph.StringKindis correctly applied.cmd/api/src/database/graphschema_integration_test.go (4)
2166-2222: LGTM!Test correctly verifies creation of
SchemaEnvironmentPrincipalKindwith proper setup of prerequisites (extension and environment). Assertions validate the returned struct fields match expected values.
2224-2291: LGTM!Good test coverage for both scenarios: retrieving multiple principal kinds for an existing environment and returning an empty slice when no principal kinds exist. The empty slice case correctly expects no error (rather than
ErrNotFound), which is the appropriate behavior for a query that returns zero results.
2294-2362: LGTM!Deletion test correctly verifies both success and not-found cases. The post-deletion verification using
GetSchemaEnvironmentPrincipalKindsByEnvironmentIdconfirms the principal kind was removed.
2365-2418: LGTM!Comprehensive cascade deletion test. This test verifies that deleting a
GraphSchemaExtensionproperly cascades to delete all dependent entities: node kinds, properties, edge kinds, environments, relationship findings, remediations, and principal kinds. This aligns with the database schema's foreign key cascade constraints.cmd/api/src/model/graphschema.go (1)
139-149: LGTM!Model correctly represents the
schema_environments_principal_kindstable. The struct appropriately includes onlyCreatedAt(noUpdatedAt/DeletedAt) since principal kinds are created or deleted but not updated. Based on learnings, thePrincipalKindfield serves as the primary key, enforcing global uniqueness where each principal kind is exclusively owned by a single environment.cmd/api/src/api/v2/mocks/graphschemaextensions.go (1)
1-72: LGTM! Generated mock looks correct.This is a properly generated gomock mock for the OpenGraphSchemaService interface. The mock structure and method signatures align with the interface definition in cmd/api/src/api/v2/opengraphschema.go.
cmd/api/src/api/v2/opengraphschema.go (1)
42-42: Remove or update the TODO comment.The TODO states "barebones in order to test handler," but the PR is marked as WIP (Work In Progress). Clarify whether this implementation is intentional for the current PR scope or if additional work is planned.
If the implementation is complete for this PR's scope, remove the TODO. If not, consider adding a tracking issue reference.
cmd/api/src/database/upsert_schema_environment_integration_test.go (2)
28-305: Well-structured integration tests with comprehensive coverage.The test suite covers:
- Successful environment creation with principal kinds
- Upsert behavior (replacing existing environments)
- Auto-registration of new source kinds
- Error conditions (missing environment kind, missing principal kind)
- Rollback verification for partial failures
- Multiple environments scenario
The assertions are thorough and verify both positive and negative outcomes.
275-276: The referenced functions are correctly defined in the same package. BothsetupIntegrationTestSuiteandteardownIntegrationTestSuiteare defined incmd/api/src/database/database_integration_test.go(lines 44 and 112, respectively) and are part of the samedatabase_testpackage asupsert_schema_environment_integration_test.go. Helper functions in separate_test.gofiles within the same package are accessible to all test files in that package per Go's testing conventions. No undefined reference or compilation error exists.Likely an incorrect or invalid review comment.
cmd/api/src/services/opengraphschema/mocks/opengraphschema.go (1)
1-71: LGTM! Generated mock looks correct.This is a properly generated gomock mock for the OpenGraphSchemaRepository interface. The mock structure and method signatures are standard gomock output.
cmd/api/src/database/graphschema.go (4)
56-56: LGTM! Interface additions are well-designed.The new methods follow existing naming conventions and patterns in the OpenGraphSchema interface. Method signatures are clear and consistent with the rest of the codebase.
Also applies to: 69-71
542-556: LGTM! GetSchemaEnvironmentByKinds implementation is correct.The implementation properly:
- Uses parameterized queries to prevent SQL injection
- Filters soft-deleted records with
deleted_at IS NULL- Handles the not-found case by returning
ErrNotFound- Uses the existing
CheckErrorhelper consistently
745-757: LGTM! GetSchemaEnvironmentPrincipalKindsByEnvironmentId implementation is correct.The implementation properly retrieves all principal kinds for a given environment using a parameterized query and consistent error handling.
759-770: LGTM! DeleteSchemaEnvironmentPrincipalKind implementation is correct.The implementation properly deletes the association using both
environment_idandprincipal_kindin the WHERE clause for safety, handles the not-found case, and uses consistent error handling.cmd/api/src/database/upsert_schema_environment.go (4)
27-64: LGTM!The orchestration logic is sound: validate and translate all kinds upfront, then perform the upsert operations. The transaction management delegation to the caller is clearly documented, and error wrapping provides good context.
66-75: LGTM!The validation and translation logic correctly handles both the "not found" and unexpected error cases, returning appropriate error messages.
77-97: LGTM!The auto-registration of missing source kinds aligns with the PR requirements. The design correctly relies on the caller-managed transaction for atomicity if the subsequent lookup fails.
139-160: No issues found. Theschema_environments_principal_kindstable hasON DELETE CASCADEconfigured on theenvironment_idforeign key, so principal kinds are automatically deleted when their environment is deleted. The delete loop inupsertPrincipalKindswill not find orphaned rows because CASCADE cleanup happens at the database level.cmd/api/src/database/mocks/db.go (1)
588-601: LGTM - Generated mock implementations.These mock methods follow the standard MockGen pattern and correctly mirror the new interface methods added for schema environment and principal kind operations. As auto-generated code, they should remain consistent with the source interface.
Also applies to: 1004-1016, 1950-1963, 2205-2233, 2351-2364
kpowderly
changed the title
|
@coderabbitai please re-review this PR using the following AGENTS.md instructions and tell me if you find anything new: AGENTS.mdGolang code standards
PR instructions
Test code instructions for Golang
Code review instructions
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 56 🏁 Script executed: Length of output: 577 🏁 Script executed: Length of output: 1007 🏁 Script executed: Length of output: 17238 🏁 Script executed: Length of output: 4985 🏁 Script executed: Length of output: 7242 🏁 Script executed: Length of output: 2763 🏁 Script executed: Length of output: 784 🏁 Script executed: Length of output: 38 🏁 Script executed: Length of output: 2426 🏁 Script executed: Length of output: 672 🏁 Script executed: Length of output: 448 🏁 Script executed: Length of output: 152 🏁 Script executed: Length of output: 1388 @sircodemane: I've completed the review against the AGENTS.md standards. Here are my findings: Issues Found1. ❌ PR Title Format ViolationCurrent title: The PR title is missing a dash before the Jira tag. According to AGENTS.md, the format should be: 2. ❌ Receiver Variable Name ViolationFile: The receiver uses Should be: func (s *OpenGraphSchemaService) UpsertGraphSchemaExtension(ctx context.Context, req v2.GraphSchemaExtension) error {3.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/api/src/database/sourcekinds_integration_test.go (1)
209-257: LGTM! Consider adding edge case coverage.The test follows existing patterns and correctly validates the
GetSourceKindByNamemethod. The implementation is solid.For enhanced test coverage, consider adding test cases for edge scenarios such as non-existent source kinds or empty name parameters.
cmd/api/src/api/v2/opengraphschema.go (1)
42-60: Stale TODO comment and inconsistent context usage.
- The TODO on line 42 mentions "skeleton endpoint" but the implementation appears complete.
- Lines 50 and 55 call
request.Context()again instead of using the already-extractedctxvariable from line 45.Suggested fix
-// TODO: Implement this - skeleton endpoint to simply test the handler. func (s Resources) OpenGraphSchemaIngest(response http.ResponseWriter, request *http.Request) { var ( ctx = request.Context() ) 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 } response.WriteHeader(http.StatusCreated) }cmd/api/src/database/upsert_schema_environment.go (1)
119-137: Consider simplifying the double-negative condition for readability.Line 124's
!errors.Is(err, ErrNotFound)after checkingerr != nil && !errors.Is(err, ErrNotFound)on line 122 is correct but slightly obscure. The same pattern appears inupsertPrincipalKinds.Alternative pattern for clarity
func (s *BloodhoundDB) upsertSchemaEnvironment(ctx context.Context, graphSchema model.SchemaEnvironment) (int32, error) { - if existing, err := s.GetSchemaEnvironmentByKinds(ctx, graphSchema.EnvironmentKindId, graphSchema.SourceKindId); err != nil && !errors.Is(err, ErrNotFound) { + existing, err := s.GetSchemaEnvironmentByKinds(ctx, graphSchema.EnvironmentKindId, graphSchema.SourceKindId) + if err != nil && !errors.Is(err, ErrNotFound) { return 0, fmt.Errorf("error retrieving schema environment: %w", err) - } else if !errors.Is(err, ErrNotFound) { - // Environment exists - delete it first + } + + if err == nil { + // Environment exists — delete it first if err := s.DeleteSchemaEnvironment(ctx, existing.ID); err != nil { return 0, fmt.Errorf("error deleting schema environment %d: %w", existing.ID, err) } } // Create Environment - if created, err := s.CreateSchemaEnvironment(ctx, graphSchema.SchemaExtensionId, graphSchema.EnvironmentKindId, graphSchema.SourceKindId); err != nil { + created, err := s.CreateSchemaEnvironment(ctx, graphSchema.SchemaExtensionId, graphSchema.EnvironmentKindId, graphSchema.SourceKindId) + if err != nil { return 0, fmt.Errorf("error creating schema environment: %w", err) - } else { - return created.ID, nil } + return created.ID, nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/api/src/api/v2/opengraphschema.gocmd/api/src/database/kind_integration_test.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/services/opengraphschema/extension.gocmd/api/src/services/opengraphschema/opengraphschema_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/database/kind_integration_test.go
- cmd/api/src/services/opengraphschema/opengraphschema_test.go
🧰 Additional context used
🧠 Learnings (6)
📚 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/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/sourcekinds_integration_test.gocmd/api/src/database/upsert_schema_environment_integration_test.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/sourcekinds_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/services/opengraphschema/extension.gocmd/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/upsert_schema_environment.gocmd/api/src/database/upsert_schema_environment_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/database/upsert_schema_environment.go
🧬 Code graph analysis (5)
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/services/opengraphschema/extension.go (3)
cmd/api/src/api/v2/opengraphschema.go (2)
OpenGraphSchemaService(28-30)GraphSchemaExtension(32-34)cmd/api/src/services/opengraphschema/opengraphschema.go (1)
OpenGraphSchemaService(29-31)cmd/api/src/database/sourcekinds.go (1)
SourceKind(59-63)
cmd/api/src/database/upsert_schema_environment.go (2)
cmd/api/src/database/db.go (2)
BloodhoundDB(197-200)ErrNotFound(42-42)cmd/api/src/database/kind.go (1)
Kind(24-26)
cmd/api/src/database/upsert_schema_environment_integration_test.go (1)
cmd/api/src/database/db.go (1)
BloodhoundDB(197-200)
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(105-119)cmd/api/src/api/marshalling.go (1)
WriteErrorResponse(77-85)cmd/api/src/api/error.go (2)
BuildErrorResponse(134-145)ErrorResponsePayloadUnmarshalError(60-60)
⏰ 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: run-analysis
- GitHub Check: build-ui
🔇 Additional comments (5)
cmd/api/src/database/sourcekinds_integration_test.go (1)
145-145: Correct error assertion argument order.The corrected assertions now properly pass the actual error as the first argument and the expected error message string as the second argument, matching the
testifyAPI signature.Also applies to: 200-200, 426-426
cmd/api/src/services/opengraphschema/extension.go (1)
25-34: LGTM!The method correctly implements the
UpsertGraphSchemaExtensioninterface. Error handling with wrapping is appropriate, and the TODO comment clearly documents the temporary hardcoded extension ID.cmd/api/src/database/upsert_schema_environment_integration_test.go (1)
31-307: Comprehensive integration test suite.The table-driven tests provide excellent coverage of:
- Happy paths (create, upsert replacement, auto-registration)
- Error scenarios (missing environment kind, missing principal kind)
- Rollback behavior on partial failures
- Multiple environment combinations
The test structure with
setupDataandassertcallbacks is clean and maintainable.cmd/api/src/database/upsert_schema_environment.go (2)
27-64: Well-structured upsert orchestration.The method correctly coordinates validation, translation, and upsert operations with proper error propagation. The documentation clearly states the transaction responsibility lies with the caller.
139-160: Verify intended behavior whenprincipalKindsslice is empty.If an empty slice is passed, this method will delete all existing principal kinds without creating new ones. If this is intentional (allowing environments with no principal kinds), the behavior is correct. If principal kinds are required, consider adding validation.
|
@coderabbitai i have a new version of AGENTS.md for you, please use this to re-review this PR and let me know if you find anything: AGENTS.mdGolang code standards
PR instructions
Test code instructions for Golang
Code review instructions
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1082 🏁 Script executed: Length of output: 1524 🏁 Script executed: Length of output: 1897 🏁 Script executed: Length of output: 185 🏁 Script executed: Length of output: 267 🏁 Script executed: Length of output: 548 🏁 Script executed: Length of output: 785 🏁 Script executed: Length of output: 2676 🏁 Script executed: Length of output: 112 🏁 Script executed: Length of output: 255 🏁 Script executed: Length of output: 80 🏁 Script executed: Length of output: 284 🏁 Script executed: Length of output: 1864 🏁 Script executed: Length of output: 3692 🏁 Script executed: Length of output: 2255 🏁 Script executed: Length of output: 575 🏁 Script executed: Length of output: 3148 @sircodemane: Thank you for the updated AGENTS.md! I've completed the re-review. Here are my findings with the updated standards: ✅ Issues Now Resolved1. ✅ PR Title Format - NOW COMPLIANTCurrent title: The updated AGENTS.md format is: 2. ✅ Receiver Variable - NOW A PREFERENCEFile: The updated AGENTS.md states: "Receiver functions on structs should prefer 3. ✅ Integration Test Build Tags - NOW EXPLICITLY ACCEPTABLEAll integration test files use 4. ✅ MockDatabase Implementation CheckI previously confirmed that ❌ NEW ISSUE FOUNDMissing OpenAPI DocumentationImpact: This suggests Details:
According to AGENTS.md:
Missing Documentation:
Reference Pattern:
Recommendation: ✅ What Looks Good
🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @cmd/api/src/database/upsert_schema_extension.go:
- Around line 29-38: In UpsertGraphSchemaExtension, update the fmt.Errorf
message inside the loop that wraps tx.UpsertSchemaEnvironmentWithPrincipalKinds
to use "upsert" instead of "upload" (i.e., change "failed to upload environment
with principal kinds: %w" to "failed to upsert environment with principal kinds:
%w") so the error text matches the Upsert operation and function names.
🧹 Nitpick comments (5)
cmd/api/src/services/opengraphschema/extension_test.go (1)
31-163: Well-structured table-driven tests with proper parallelization.The tests correctly:
- Use external test package (
opengraphschema_test)- Create isolated
gomock.Controllerper subtest (safe fort.Parallel())- Verify field mapping from
v2.Environmenttodatabase.EnvironmentInput- Test both error propagation and success paths
Consider adding a test case for an empty
environmentsslice to verify the behavior when no environments are provided:💡 Optional: Add edge case test
{ name: "Success: empty environments", args: args{ environments: []v2.Environment{}, }, setupMocks: func(t *testing.T, m *mocks) { t.Helper() m.mockOpenGraphSchema.EXPECT().UpsertGraphSchemaExtension( gomock.Any(), int32(1), []database.EnvironmentInput{}, ).Return(nil) }, expected: nil, },cmd/api/src/services/opengraphschema/extension.go (2)
39-40: Hardcoded extension ID noted.The TODO comment documents this as intentional tech debt. Ensure this is tracked for follow-up.
Would you like me to open an issue to track removing the hardcoded extension ID?
31-37: Consider nil-safety forPrincipalKindsslice.If
environment.PrincipalKindsisnil, it will be assigned as-is. Verify that the repository layer handlesnilvs empty slice consistently, or normalize here:💡 Optional: Normalize nil to empty slice
for i, environment := range req.Environments { + principalKinds := environment.PrincipalKinds + if principalKinds == nil { + principalKinds = []string{} + } environments[i] = database.EnvironmentInput{ EnvironmentKindName: environment.EnvironmentKind, SourceKindName: environment.SourceKind, - PrincipalKinds: environment.PrincipalKinds, + PrincipalKinds: principalKinds, } }cmd/api/src/database/upsert_schema_extension_integration_test.go (1)
111-127: Consider verifying principal kind linkage per environment.This test creates two environments with different principal kinds but only verifies the count of principal kinds per environment. It doesn't verify that the correct principal kind is linked to the correct environment (e.g.,
Tag_Tier_Zeroprincipal kind should be with theTag_Tier_Zeroenvironment).Suggested enhancement
// Verify first environment env1PrincipalKinds, err := db.GetSchemaEnvironmentPrincipalKindsByEnvironmentId(context.Background(), environments[0].ID) assert.NoError(t, err) assert.Equal(t, 1, len(env1PrincipalKinds)) + +// Verify the correct principal kind is linked +env1ExpectedKind, err := db.GetKindByName(context.Background(), "Tag_Tier_Zero") +assert.NoError(t, err) +assert.Equal(t, int32(env1ExpectedKind.ID), env1PrincipalKinds[0].PrincipalKind) // Verify second environment env2PrincipalKinds, err := db.GetSchemaEnvironmentPrincipalKindsByEnvironmentId(context.Background(), environments[1].ID) assert.NoError(t, err) assert.Equal(t, 1, len(env2PrincipalKinds)) + +env2ExpectedKind, err := db.GetKindByName(context.Background(), "Tag_Owned") +assert.NoError(t, err) +assert.Equal(t, int32(env2ExpectedKind.ID), env2PrincipalKinds[0].PrincipalKind)cmd/api/src/database/upsert_schema_environment.go (1)
75-95: Consider returning the source kind from RegisterSourceKind to avoid double lookup.After calling
RegisterSourceKind, the code performs a second lookup to retrieve the newly registered source kind. IfRegisterSourceKindcould return the created/updated entity, this would be more efficient. However, this would require interface changes that may be out of scope for this PR.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
cmd/api/src/api/registration/registration.gocmd/api/src/api/registration/v2.gocmd/api/src/api/v2/model.gocmd/api/src/database/migration/migrations/v8.5.0.sqlcmd/api/src/database/upsert_schema_environment.gocmd/api/src/database/upsert_schema_extension.gocmd/api/src/database/upsert_schema_extension_integration_test.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
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/api/src/api/registration/v2.go
- cmd/api/src/services/opengraphschema/opengraphschema.go
- cmd/api/src/api/registration/registration.go
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/services/entrypoint.gocmd/api/src/database/upsert_schema_extension.gocmd/api/src/database/upsert_schema_extension_integration_test.gocmd/api/src/database/upsert_schema_environment.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.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/services/opengraphschema/extension.gocmd/api/src/database/upsert_schema_extension.gocmd/api/src/database/upsert_schema_extension_integration_test.gocmd/api/src/database/upsert_schema_environment.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/services/opengraphschema/extension.gocmd/api/src/database/upsert_schema_environment.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/services/opengraphschema/extension_test.gocmd/api/src/database/upsert_schema_extension_integration_test.go
🧬 Code graph analysis (7)
cmd/api/src/services/entrypoint.go (1)
cmd/api/src/services/opengraphschema/opengraphschema.go (1)
NewOpenGraphSchemaService(35-39)
cmd/api/src/services/opengraphschema/extension.go (4)
cmd/api/src/services/opengraphschema/opengraphschema.go (1)
OpenGraphSchemaService(31-33)cmd/api/src/api/v2/opengraphschema.go (2)
OpenGraphSchemaService(28-30)GraphSchemaExtension(32-34)cmd/api/src/database/upsert_schema_extension.go (1)
EnvironmentInput(23-27)cmd/api/src/database/sourcekinds.go (1)
SourceKind(59-63)
cmd/api/src/database/upsert_schema_extension.go (1)
cmd/api/src/database/db.go (1)
BloodhoundDB(197-200)
cmd/api/src/services/opengraphschema/extension_test.go (3)
cmd/api/src/services/opengraphschema/mocks/opengraphschema.go (2)
MockOpenGraphSchemaRepository(37-41)NewMockOpenGraphSchemaRepository(49-53)cmd/api/src/database/upsert_schema_extension.go (1)
EnvironmentInput(23-27)cmd/api/src/services/opengraphschema/opengraphschema.go (1)
NewOpenGraphSchemaService(35-39)
cmd/api/src/services/opengraphschema/mocks/opengraphschema.go (1)
cmd/api/src/database/upsert_schema_extension.go (1)
EnvironmentInput(23-27)
cmd/api/src/database/upsert_schema_environment.go (2)
cmd/api/src/database/db.go (2)
BloodhoundDB(197-200)ErrNotFound(42-42)cmd/api/src/database/kind.go (1)
Kind(24-26)
cmd/api/src/api/v2/model.go (2)
cmd/api/src/services/opengraphschema/opengraphschema.go (1)
OpenGraphSchemaService(31-33)cmd/api/src/api/v2/opengraphschema.go (1)
OpenGraphSchemaService(28-30)
⏰ 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 (11)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
92-102: LGTM! FK reference correction aligns the schema with the column semantics.The change from
REFERENCES kind(id)toREFERENCES source_kinds(id)forsource_kind_idis correct—this column should reference thesource_kindstable, not the generickindtable. Theenvironment_kind_idcorrectly continues to referencekind(id).cmd/api/src/api/v2/model.go (2)
119-119: LGTM! New service field follows the existing pattern.The unexported
openGraphSchemaServicefield is consistent with the design pattern used for other services inResources.
134-134: Constructor wiring is correct.The new parameter and field initialization follow the established pattern for dependency injection in
NewResources.Also applies to: 150-150
cmd/api/src/services/entrypoint.go (2)
133-134: Route registration correctly propagates the new service.The middleware and route registration calls are properly updated to include the new
openGraphSchemaServiceparameter.
130-130: Service instantiation follows the established pattern.The
openGraphSchemaServiceis created usingconnections.RDMSas the repository, consistent with how other services are wired.BloodhoundDBimplements theOpenGraphSchemaRepositoryinterface with the requiredUpsertGraphSchemaExtensionmethod.cmd/api/src/services/opengraphschema/extension.go (1)
26-46: Clean implementation with proper error wrapping.The method correctly:
- Pre-allocates the slice with the right capacity
- Maps fields from the API model to the database model
- Wraps errors with context for debugging
cmd/api/src/database/upsert_schema_extension.go (1)
23-27: LGTM!The
EnvironmentInputstruct is well-defined with clear field names that align with the domain model.cmd/api/src/services/opengraphschema/mocks/opengraphschema.go (1)
17-72: Generated mock file — no review required.This file is auto-generated by mockgen and correctly implements the
OpenGraphSchemaRepositoryinterface. The mock signature aligns with theUpsertGraphSchemaExtensionmethod and usesdatabase.EnvironmentInputas expected.cmd/api/src/database/upsert_schema_extension_integration_test.go (1)
30-404: Excellent test coverage for transactional upsert behavior.The test suite comprehensively covers:
- Success cases: single/multiple environments, upsert replacement, source kind auto-registration
- Error cases: invalid environment kind, invalid principal kind
- Rollback verification: ensures complete transaction rollback when any environment fails
The table-driven pattern with
setupDataandassertcallbacks is clean and maintainable.cmd/api/src/database/upsert_schema_environment.go (2)
27-62: Well-structured upsert flow with proper separation of concerns.The
UpsertSchemaEnvironmentWithPrincipalKindsmethod follows a clear pattern:
- Build initial model
- Validate/translate all inputs before mutations
- Upsert environment
- Upsert principal kinds
Error wrapping at each phase provides good context for debugging.
137-158: LGTM — clean delete-then-create pattern for principal kinds.The logic correctly handles both scenarios:
- When existing principal kinds are found (
err == nil), they are deleted before creating new ones- When no existing kinds exist (
ErrNotFound), it proceeds directly to creationThis aligns with the upsert semantics described in the PR objectives.
| ) | ||
|
|
||
| var req GraphSchemaExtension | ||
| if err := json.NewDecoder(request.Body).Decode(&req); err != nil { |
There was a problem hiding this comment.
do you know if we plan to use JSONSchema in the API handler, similar to what is done for OpenGraph ingest? curious if lawson is using it for his work. we may want to reject bad requests with 400 at the API layer but that may be coming in a follow up?
just leaving this as a note here so i dont forget but lemme know
There was a problem hiding this comment.
It looks like Lawson is doing something similar..
I was kind of basing my work off of this example in the RFC.
Side note (and probably a question for my other PR) but do you know if findings and remediations get uploaded in the same way? I guess I'm not sure what the use case is for this handler.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/api/src/database/migration/migrations/v8.6.0.sql`:
- Around line 16-22: Don't drop and re-add source_kind_id (which loses data and
fails on populated tables); instead modify the FK in-place on table
schema_environments: if the column source_kind_id exists, drop only the existing
foreign key constraint and create a new REFERENCES source_kinds(id) constraint;
if the column is missing add it as INTEGER NULLABLE, backfill values as needed,
then ALTER COLUMN to NOT NULL and add the FK constraint; reference the table
schema_environments, column source_kind_id and target table source_kinds(id)
when locating the change.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/migration/migrations/v8.6.0.sqlcmd/api/src/database/mocks/db.gocmd/api/src/database/upsert_schema_environment.gocmd/api/src/database/upsert_schema_environment_integration_test.gocmd/api/src/database/upsert_schema_extension_integration_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/database/upsert_schema_environment_integration_test.go
🧰 Additional context used
🧠 Learnings (6)
📓 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-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.6.0.sqlcmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/upsert_schema_environment.gocmd/api/src/database/graphschema.gocmd/api/src/database/mocks/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/graphschema_integration_test.gocmd/api/src/database/upsert_schema_environment.gocmd/api/src/database/upsert_schema_extension_integration_test.gocmd/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/upsert_schema_environment.gocmd/api/src/database/upsert_schema_extension_integration_test.gocmd/api/src/database/graphschema.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/upsert_schema_environment.gocmd/api/src/database/graphschema.gocmd/api/src/database/mocks/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/upsert_schema_extension_integration_test.go
🧬 Code graph analysis (3)
cmd/api/src/database/upsert_schema_environment.go (2)
cmd/api/src/database/db.go (2)
BloodhoundDB(197-200)ErrNotFound(42-42)cmd/api/src/database/kind.go (1)
Kind(24-26)
cmd/api/src/database/graphschema.go (3)
cmd/api/src/model/graphschema.go (5)
SchemaEnvironment(101-106)SchemaEnvironment(108-110)SchemaEnvironmentPrincipalKind(141-145)SchemaEnvironmentPrincipalKind(147-149)SchemaEnvironmentPrincipalKinds(139-139)cmd/api/src/database/db.go (1)
ErrNotFound(42-42)cmd/api/src/database/helper.go (1)
CheckError(39-45)
cmd/api/src/database/mocks/db.go (4)
cmd/api/src/model/graphschema.go (5)
SchemaEnvironmentPrincipalKind(141-145)SchemaEnvironmentPrincipalKind(147-149)SchemaEnvironmentPrincipalKinds(139-139)SchemaEnvironment(101-106)SchemaEnvironment(108-110)cmd/api/src/database/kind.go (1)
Kind(24-26)cmd/api/src/model/kind.go (1)
Kind(18-21)cmd/api/src/database/sourcekinds.go (1)
SourceKind(59-63)
🔇 Additional comments (13)
cmd/api/src/database/upsert_schema_extension_integration_test.go (1)
30-432: Solid end-to-end coverage for environment upsert behavior.The scenarios exercise success paths, source-kind auto-registration, and rollback behavior in a clear table-driven style.
cmd/api/src/database/graphschema_integration_test.go (4)
2227-2234: Rename alignment looks good.Updated call site matches the new CreatePrincipalKind API.
2265-2304: Rename alignment looks good.The test now uses CreatePrincipalKind/GetPrincipalKindsByEnvironmentId consistently.
2335-2373: Rename alignment looks good.DeletePrincipalKind usage is consistent with the updated API.
2406-2432: Rename alignment looks good.Cascade delete assertions now exercise the renamed principal-kind accessors.
cmd/api/src/database/upsert_schema_environment.go (3)
27-59: Ensure this upsert is always transactional.If this method is invoked outside a transaction, a failure during principal-kind creation can leave the environment replaced but incomplete. Please confirm callers always wrap this in
Transaction, or document/enforce that expectation.💡 Optional doc update to make the contract explicit
-// UpsertSchemaEnvironmentWithPrincipalKinds creates or updates an environment with its principal kinds. +// UpsertSchemaEnvironmentWithPrincipalKinds creates or updates an environment with its principal kinds. +// NOTE: Callers should run this within a Transaction to ensure atomicity.
64-115: Validation helpers look good.Clear error context and consistent NotFound handling for kinds/source kinds.
117-159: Replacement flow is straightforward.Delete-and-recreate logic is clear and easy to follow.
cmd/api/src/database/graphschema.go (4)
55-72: Interface additions and renames are consistent.New environment-by-kinds lookup and principal-kind APIs align with the updated surface.
244-248: Formatting-only SQL alignment.No behavioral concerns in these adjustments.
Also applies to: 293-295, 441-441, 462-466, 547-547
599-613: Environment lookup by kind pair looks good.Deleted-at filtering and ErrNotFound handling are clear.
788-827: Principal-kind CRUD additions are clear.The new create/get/delete helpers are straightforward and match the interface.
cmd/api/src/database/mocks/db.go (1)
17-22: Auto-generated mock file looks correct.This file is generated by MockGen and includes the new mock methods for the environment functionality (
CreatePrincipalKind,DeletePrincipalKind,GetKindByName,GetPrincipalKindsByEnvironmentId,GetSchemaEnvironmentByKinds,GetSourceKindByName). The generation command and output are consistent with the Database interface changes introduced in this PR.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
This ticket is to add the Environment Functionality for the Schema Service. It's not necessary to completely hook it up yet since the other pieces of the Schema Service are still in flight.
Description
Environment w/ Principal Kinds Specific
Flow:
Validations
Motivation and Context
Resolves BED-6852
Why is this change required? What problem does it solve?
How Has This Been Tested?
Unit tests/Integration tests
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.