Conversation
WalkthroughAdds a new Database.PopulateExtensionData API and implementation, embeds and executes extension SQL migrations, generates AD/Azure extension SQL, and invokes extension data population during startup and in multiple integration/test setups. Changes
*Files with shared prefixes are grouped for brevity. Sequence Diagram(s)sequenceDiagram
participant Service as Application Service
participant Bootstrap as Bootstrap Layer
participant DB as Database
participant Migrator as Migrator
participant FS as Extension Files
Service->>Bootstrap: PopulateExtensionData(ctx, db)
Bootstrap->>DB: db.PopulateExtensionData(ctx)
DB->>Migrator: migrator.ExecuteExtensionDataPopulation()
Migrator->>FS: fs.ReadDir("extensions")
FS-->>Migrator: list of .sql files
loop per file
Migrator->>FS: fs.ReadFile(file)
FS-->>Migrator: file SQL
Migrator->>DB: BEGIN (per-file transaction)
Migrator->>DB: EXECUTE SQL content
Migrator->>DB: COMMIT / ROLLBACK
end
Migrator-->>DB: return error or nil
DB-->>Bootstrap: return error or nil
Bootstrap-->>Service: return error or nil
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
cmd/api/src/bootstrap/server.go (1)
75-81: Function implementation is correct, but can be simplified.The function correctly wraps the database's
PopulateExtensionDatamethod. However, the explicitreturn nilon line 80 is redundant since the error is already nil when reaching that point.🔎 Optional simplification
func PopulateExtensionData(ctx context.Context, db database.Database) error { - if err := db.PopulateExtensionData(ctx); err != nil { - return err - } - - return nil + return db.PopulateExtensionData(ctx) }cmd/api/src/test/lab/fixtures/postgres.go (1)
44-45: Extension data population correctly added, but consider reusing DB instance.The extension data population step is properly integrated with correct error handling. However, this fixture creates multiple
BloodhoundDBinstances (lines 40, 42, 44, and 47). While this works correctly, consider creating a single instance at the beginning and reusing it throughout the setup chain for better efficiency.🔎 Optional refactor to reduce instance creation
var PostgresFixture = lab.NewFixture(func(harness *lab.Harness) (*database.BloodhoundDB, error) { testCtx := context.Background() if labConfig, ok := lab.Unpack(harness, ConfigFixture); !ok { return nil, fmt.Errorf("unable to unpack ConfigFixture") } else if pgdb, err := database.OpenDatabase(labConfig.Database.PostgreSQLConnectionString()); err != nil { return nil, err - } else if err := integration.Prepare(testCtx, database.NewBloodhoundDB(pgdb, auth.NewIdentityResolver())); err != nil { + } else { + bhdb := database.NewBloodhoundDB(pgdb, auth.NewIdentityResolver()) + if err := integration.Prepare(testCtx, bhdb); err != nil { - return nil, fmt.Errorf("failed ensuring database: %v", err) + return nil, fmt.Errorf("failed ensuring database: %v", err) - } else if err := bootstrap.MigrateDB(testCtx, labConfig, database.NewBloodhoundDB(pgdb, auth.NewIdentityResolver()), config.NewDefaultAdminConfiguration); err != nil { + } else if err := bootstrap.MigrateDB(testCtx, labConfig, bhdb, config.NewDefaultAdminConfiguration); err != nil { - return nil, fmt.Errorf("failed migrating database: %v", err) + return nil, fmt.Errorf("failed migrating database: %v", err) - } else if err := bootstrap.PopulateExtensionData(testCtx, database.NewBloodhoundDB(pgdb, auth.NewIdentityResolver())); err != nil { + } else if err := bootstrap.PopulateExtensionData(testCtx, bhdb); err != nil { - return nil, fmt.Errorf("failed populating extension data: %v", err) + return nil, fmt.Errorf("failed populating extension data: %v", err) - } else { + } - return database.NewBloodhoundDB(pgdb, auth.NewIdentityResolver()), nil + return bhdb, nil - } + } }, nil)packages/go/schemagen/generator/sql.go (1)
212-229: Consider adding error context for debugging.The filesystem operations (stat, mkdir, open, write) return errors without additional context. While the current error handling is functionally correct, wrapping errors with context would aid debugging when generation fails.
🔎 Example using error wrapping
if _, err := os.Stat(dir); err != nil { if !os.IsNotExist(err) { - return err + return fmt.Errorf("failed to stat directory %s: %w", dir, err) } if err := os.MkdirAll(dir, defaultPackageDirPermission); err != nil { - return err + return fmt.Errorf("failed to create directory %s: %w", dir, err) } } if fout, err := os.OpenFile(path.Join(dir, "ad.sql"), fileOpenMode, defaultSourceFilePermission); err != nil { - return err + return fmt.Errorf("failed to open ad.sql: %w", err) } else { defer fout.Close() _, err := fout.WriteString(sb.String()) - return err + if err != nil { + return fmt.Errorf("failed to write ad.sql: %w", err) + } + return nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
cmd/api/src/bootstrap/server.gocmd/api/src/daemons/changelog/ingestion_integration_test.gocmd/api/src/daemons/datapipe/datapipe_integration_test.gocmd/api/src/database/database_integration_test.gocmd/api/src/database/db.gocmd/api/src/database/migration/extensions/ad.sqlcmd/api/src/database/migration/extensions/az.sqlcmd/api/src/database/migration/migration.gocmd/api/src/database/migration/stepwise.gocmd/api/src/database/mocks/db.gocmd/api/src/services/entrypoint.gocmd/api/src/services/graphify/graphify_integration_test.gocmd/api/src/test/integration/database.gocmd/api/src/test/lab/fixtures/postgres.gopackages/go/graphify/graph/graph.gopackages/go/schemagen/generator/cue.gopackages/go/schemagen/generator/sql.gopackages/go/schemagen/main.go
🧰 Additional context used
🧠 Learnings (3)
📚 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:
packages/go/schemagen/main.gopackages/go/schemagen/generator/cue.gopackages/go/schemagen/generator/sql.go
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
packages/go/schemagen/main.gopackages/go/schemagen/generator/cue.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:
packages/go/schemagen/generator/sql.go
🧬 Code graph analysis (12)
cmd/api/src/bootstrap/server.go (1)
cmd/api/src/database/db.go (1)
Database(72-192)
packages/go/graphify/graph/graph.go (1)
cmd/api/src/bootstrap/server.go (1)
PopulateExtensionData(75-81)
cmd/api/src/daemons/datapipe/datapipe_integration_test.go (1)
cmd/api/src/bootstrap/server.go (1)
PopulateExtensionData(75-81)
cmd/api/src/database/migration/stepwise.go (1)
cmd/api/src/database/migration/migration.go (1)
Migrator(47-51)
cmd/api/src/services/entrypoint.go (1)
cmd/api/src/bootstrap/server.go (1)
PopulateExtensionData(75-81)
cmd/api/src/test/lab/fixtures/postgres.go (3)
cmd/api/src/bootstrap/server.go (1)
PopulateExtensionData(75-81)cmd/api/src/database/db.go (1)
NewBloodhoundDB(225-227)cmd/api/src/auth/model.go (1)
NewIdentityResolver(74-76)
cmd/api/src/database/db.go (2)
cmd/api/src/bootstrap/server.go (1)
PopulateExtensionData(75-81)cmd/api/src/database/migration/migration.go (1)
NewMigrator(54-64)
cmd/api/src/services/graphify/graphify_integration_test.go (1)
cmd/api/src/bootstrap/server.go (1)
PopulateExtensionData(75-81)
cmd/api/src/test/integration/database.go (1)
cmd/api/src/bootstrap/server.go (1)
PopulateExtensionData(75-81)
cmd/api/src/database/mocks/db.go (1)
cmd/api/src/bootstrap/server.go (1)
PopulateExtensionData(75-81)
cmd/api/src/database/database_integration_test.go (1)
cmd/api/src/bootstrap/server.go (1)
PopulateExtensionData(75-81)
packages/go/schemagen/generator/sql.go (3)
packages/go/schemagen/model/schema.go (2)
ActiveDirectory(62-72)Azure(50-60)packages/go/schemagen/generator/golang.go (1)
SchemaSourceName(32-32)packages/go/schemagen/csgen/models.go (1)
Symbol(23-23)
⏰ 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 (21)
packages/go/schemagen/generator/cue.go (1)
98-101: LGTM! Structured logging adopted.The change from unstructured logging to structured
slog.Debugwith explicit type functions improves observability and aligns with the project's logging standards.Based on learnings, BloodHound prefers explicit slog type functions like
slog.String()for better type safety and visual distinction.cmd/api/src/services/graphify/graphify_integration_test.go (1)
88-89: LGTM: Extension data population properly integrated into test setup.The extension data population step is correctly positioned after database migration and before graph schema assertion, with appropriate error handling.
cmd/api/src/daemons/datapipe/datapipe_integration_test.go (1)
93-94: LGTM: Consistent test setup pattern.The extension data population is correctly integrated with proper error handling and sequencing.
cmd/api/src/database/database_integration_test.go (1)
61-62: LGTM: Extension data population added to database integration tests.The new initialization step is properly integrated with appropriate error handling.
cmd/api/src/test/integration/database.go (1)
139-140: LGTM: Extension data population integrated into Prepare flow.The new step is properly sequenced and includes appropriate error wrapping. The deprecation notice for this file doesn't affect the correctness of this change.
cmd/api/src/daemons/changelog/ingestion_integration_test.go (1)
119-119: LGTM: Extension data population added to changelog integration test.The initialization step is correctly positioned with proper error handling.
packages/go/graphify/graph/graph.go (1)
178-179: LGTM: Extension data population integrated into service initialization.The extension data population is correctly sequenced between database migration and graph migration, with appropriate error handling and propagation.
cmd/api/src/database/mocks/db.go (1)
2413-2425: LGTM! Generated mock aligns with interface changes.The gomock-generated
PopulateExtensionDatamethod correctly implements the new Database interface method. The mock follows the established pattern and will support testing scenarios where extension data population is invoked.cmd/api/src/services/entrypoint.go (1)
83-84: LGTM! Extension data population correctly sequenced.The new
PopulateExtensionDatastep is properly placed after RDMS migrations and before graph migrations, with appropriate error handling and descriptive error messages.packages/go/schemagen/main.go (2)
76-86: LGTM! SQL generation function follows established patterns.The
GenerateSQLfunction mirrors the structure ofGenerateGolang,GenerateSharedTypeScript, andGenerateCSharp, providing a consistent interface for generating extension SQL files.
92-93: Good use of structured logging.The migration to
slogwithattr.Errorandslog.Stringimproves observability and follows Go structured logging best practices.Based on learnings, this aligns with the preferred pattern in BloodHound for structured logging.
Also applies to: 98-99, 105-105
cmd/api/src/database/migration/stepwise.go (1)
199-239: LGTM! Extension data population is well-structured.The
ExecuteExtensionDataPopulationmethod properly:
- Iterates through extension data sources
- Filters for SQL files
- Executes each file in a transaction
- Provides clear error messages with file context
The SQL files are idempotent (DELETE before INSERT), so re-execution is safe if this method is called multiple times.
cmd/api/src/database/migration/migration.go (1)
30-31: LGTM! Clean separation of extension data from migrations.The new
ExtensionMigrationsembed andExtensionsDatafield provide a clear separation between schema migrations and extension data population, improving maintainability.Also applies to: 49-49, 59-61
cmd/api/src/database/db.go (2)
101-101: LGTM! Interface extension is focused and well-defined.The
PopulateExtensionDatamethod addition to the Database interface provides a clear contract for extension data initialization.
278-285: LGTM! Implementation properly delegates and logs errors.The
PopulateExtensionDataimplementation:
- Delegates to the migrator's
ExecuteExtensionDataPopulation- Uses structured logging with
attr.Errorfor clear diagnostics- Provides descriptive error messages for the extensions data population phase
cmd/api/src/database/migration/extensions/ad.sql (2)
24-130: LGTM! Generated SQL follows correct pattern.The DO block properly:
- Captures the new extension id with
RETURNING ... INTO- Uses the captured id for all subsequent inserts
- Provides idempotency through initial DELETE
18-18: Foreign key constraints are properly configured with CASCADE deletes.The
schema_node_kinds,schema_edge_kinds,schema_properties,schema_environments, andschema_relationship_findingstables all referenceschema_extensions(id)withON DELETE CASCADE, so the DELETE operation will safely cascade without constraint violations.cmd/api/src/database/migration/extensions/az.sql (2)
18-18: Verify foreign key constraints allow deletion.Same as ad.sql: ensure
ON DELETE CASCADEis configured for foreign keys referencingschema_extensionsto prevent deletion failures when re-running this script.
24-97: LGTM! Generated SQL follows correct pattern.The Azure extension SQL properly captures the new extension id and uses it for all node and edge kind inserts, maintaining referential integrity.
packages/go/schemagen/generator/sql.go (2)
27-163: LGTM!The NodeIcon struct and NodeIcons map provide a clean way to associate UI metadata with schema node types. The hardcoded icon and color mappings are appropriate for built-in AD and Azure node types.
180-180: This is static SQL file generation, not a security vulnerability.The code generates SQL INSERT statements and writes them to a
.sqlfile. Values come from CUE schemas and a hardcodedNodeIconsmap—both developer-controlled sources. Since the SQL is written to a static file rather than executed with user input, there is no SQL injection vector here.If you want to add defensive escaping for robustness (in case schema definitions ever contain special characters), that's reasonable as a code quality improvement, but this should not be treated as a security issue.
Likely an incorrect or invalid review comment.
|
Just a heads up, with BED-7067 incoming, we'll need to insert the node and edge kinds into the DAWGS kinds table before inserting them into their respective schema tables |
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @cmd/api/src/database/migration/extensions/az_graph_schema.sql:
- Line 33: The SQL seed values for AZGroup and AZUser in az_graph_schema.sql are
out of sync with the nodeIcons map in packages/go/schemagen/generator/sql.go;
regenerate the SQL so the icon colors match by running the schema generator (run
"just generate") which will update az_graph_schema.sql from the current
nodeIcons definitions in generator/sql.go (check the nodeIcons entries around
the map key names "AZGroup" and "AZUser" to confirm the expected colors).
♻️ Duplicate comments (2)
packages/go/schemagen/generator/sql.go (2)
208-216: Minor: SQL injection risk from unescaped string interpolation.Line 211 interpolates
kind.GetRepresentation()directly into SQL without escaping. While this is code generation from controlled CUE schema (not runtime user input), if schema data ever contains single quotes, it would break the generated SQL.This issue was flagged in previous reviews and remains unaddressed. Consider adding quote escaping or documenting that schema values must not contain quotes.
🔎 Proposed fix to escape single quotes
for i, kind := range relationshipKinds { _, traversable := traversableMap[kind.Symbol] - - sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, '%s', '', %t)", kind.GetRepresentation(), traversable)) + + // Escape single quotes in representation + escapedRep := strings.ReplaceAll(kind.GetRepresentation(), "'", "''") + sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, '%s', '', %t)", escapedRep, traversable))Apply the same escaping to node kinds at lines 188 and 190.
186-191: Critical:found(icon presence) incorrectly controlsis_display_kind.Lines 188 and 190 use the
foundboolean (indicating whether anodeIconentry exists) directly as theis_display_kindvalue in SQL. This creates semantic coupling where node types without icons are marked as non-displayable in the UI.If
is_display_kindshould reflect UI display policy rather than icon availability, this is incorrect. For example, adding a new node type to the schema without a corresponding icon entry would incorrectly mark itis_display_kind=false, potentially hiding it from the UI.This issue was flagged in previous reviews and remains unaddressed.
🔎 Proposed fix to decouple icon presence from display policy
If
is_display_kindshould be an explicit property from the schema rather than derived from icon presence, update the logic:for i, kind := range nodeKinds { + // TODO: Get is_display_kind from schema instead of icon presence + isDisplayKind := true // or kind.IsDisplayKind if available in model if iconInfo, found := nodeIcons[kind.Symbol]; found { - sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, '%s', '%s', '', %t, '%s', '%s')", kind.GetRepresentation(), kind.GetName(), found, iconInfo.Icon, iconInfo.Color)) + sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, '%s', '%s', '', %t, '%s', '%s')", kind.GetRepresentation(), kind.GetName(), isDisplayKind, iconInfo.Icon, iconInfo.Color)) } else { - sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, '%s', '%s', '', %t, '', '')", kind.GetRepresentation(), kind.GetName(), found)) + sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, '%s', '%s', '', %t, '', '')", kind.GetRepresentation(), kind.GetName(), isDisplayKind)) }Alternatively, if icon presence should determine display policy, document this explicitly.
🧹 Nitpick comments (1)
packages/go/schemagen/generator/sql.go (1)
165-238: Consider adding unit tests for SQL generation.As suggested in previous reviews, unit tests would help maintain this SQL generation code. Tests could verify:
- SQL structure (DELETE, INSERT statements present)
- Node icon/color mapping correctness
- Traversability flag assignment
- File creation and content
Example approach using
t.TempDir():func TestGenerateExtensionSQL(t *testing.T) { tmpDir := t.TempDir() // Create test schema with known node/edge kinds // Call GenerateExtensionSQL // Verify file exists and contains expected SQL structure }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
LICENSE.headercmd/api/src/database/migration/extensions/ad_graph_schema.sqlcmd/api/src/database/migration/extensions/az_graph_schema.sqlpackages/csharp/graphschema/PropertyNames.cspackages/go/graphschema/ad/ad.gopackages/go/graphschema/azure/azure.gopackages/go/graphschema/common/common.gopackages/go/graphschema/graph.gopackages/go/schemagen/generator/sql.gopackages/go/schemagen/main.gopackages/javascript/bh-shared-ui/src/graphSchema.ts
✅ Files skipped from review due to trivial changes (6)
- LICENSE.header
- packages/go/graphschema/common/common.go
- packages/go/graphschema/graph.go
- packages/csharp/graphschema/PropertyNames.cs
- packages/javascript/bh-shared-ui/src/graphSchema.ts
- packages/go/graphschema/azure/azure.go
🧰 Additional context used
🧠 Learnings (10)
📚 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:
packages/go/graphschema/ad/ad.gopackages/go/schemagen/main.gopackages/go/schemagen/generator/sql.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/migration/extensions/ad_graph_schema.sql
📚 Learning: 2025-06-25T17:52:33.291Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T17:52:33.291Z
Learning: In BloodHound Go code, prefer using explicit slog type functions like slog.Any(), slog.String(), slog.Int(), etc. over simple key-value pairs for structured logging. This provides better type safety and makes key-value pairs more visually distinct. For error types, use slog.Any("key", err) or slog.String("key", err.Error()).
Applied to files:
packages/go/schemagen/main.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:
packages/go/schemagen/main.go
📚 Learning: 2025-06-25T18:24:25.014Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1606
File: cmd/api/src/analysis/azure/post.go:33-35
Timestamp: 2025-06-25T18:24:25.014Z
Learning: In BloodHound Go code, for error types in slog structured logging, prefer using slog.String("key", err.Error()) over slog.Any("key", err). The explicit string conversion with err.Error() is preferred over using slog.Any() for error types.
Applied to files:
packages/go/schemagen/main.go
📚 Learning: 2025-05-29T18:24:23.227Z
Learnt from: superlinkx
Repo: SpecterOps/BloodHound PR: 1509
File: packages/go/stbernard/command/license/internal/cmd.go:49-51
Timestamp: 2025-05-29T18:24:23.227Z
Learning: In the stbernard package, use slog for all logging prints instead of fmt.Printf/fmt.Fprintf to maintain consistency with the codebase's logging standards.
Applied to files:
packages/go/schemagen/main.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:
packages/go/schemagen/generator/sql.go
📚 Learning: 2025-05-30T16:39:53.440Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1515
File: packages/javascript/bh-shared-ui/src/views/TierManagement/Save/SelectorForm/SelectorForm.tsx:202-211
Timestamp: 2025-05-30T16:39:53.440Z
Learning: In BloodHound, Cypher injection vulnerabilities are only a concern when `enable_cypher_mutations` is enabled, in which case users already have direct access to execute Cypher via the editor or API endpoints, making string interpolation in queries a non-security issue.
Applied to files:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.go
🧬 Code graph analysis (1)
packages/go/schemagen/generator/sql.go (3)
packages/go/schemagen/model/schema.go (2)
ActiveDirectory(62-72)StringEnum(19-24)packages/go/graphschema/ad/ad.go (2)
NodeKinds(1175-1177)PathfindingRelationships(1155-1157)packages/go/schemagen/csgen/models.go (1)
Symbol(23-23)
⏰ 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 (9)
packages/go/graphschema/ad/ad.go (1)
1-1: LGTM – Copyright year updated in generated code.The copyright year has been updated to 2026, which is appropriate for the current date. Since this is generated code, the change is a routine output of the code generation process.
packages/go/schemagen/main.go (2)
75-85: LGTM! Clean integration of SQL generation.The new
GenerateSQLfunction follows the established pattern of other generation functions and properly delegates to the generator package. Error handling is consistent with the rest of the codebase.
91-132: LGTM! Structured logging improvements align with best practices.The logging updates use
attr.Error()for error types andslog.String()for context fields, providing better type safety and visual distinction. This is consistent with the codebase's structured logging standards.Based on learnings, this approach is preferred for BloodHound Go code.
cmd/api/src/database/migration/extensions/az_graph_schema.sql (1)
18-98: Verify coordination with BED-7067 regarding DAWGS kinds table.Per LawsonWillard's comment, BED-7067 requires node and edge kinds to be inserted into the DAWGS kinds table before inserting into schema tables. This SQL script only populates
schema_extensions,schema_node_kinds, andschema_edge_kindstables.Verify whether BED-7067 has been merged and if this migration needs to be updated to insert into the DAWGS kinds table first, or if the current approach is acceptable for now with future migration planned.
cmd/api/src/database/migration/extensions/ad_graph_schema.sql (2)
26-42: LGTM! Icon colors correctly match generator code.All node kind icon colors in this SQL file match the corresponding entries in
packages/go/schemagen/generator/sql.go, confirming the file was properly generated.
18-131: Verify coordination with BED-7067 regarding DAWGS kinds table.Per LawsonWillard's comment, BED-7067 requires node and edge kinds to be inserted into the DAWGS kinds table before inserting into schema tables. This SQL script only populates
schema_extensions,schema_node_kinds, andschema_edge_kindstables.Verify whether BED-7067 has been merged and if this migration needs to be updated to insert into the DAWGS kinds table first, or if the current approach is acceptable for now with future migration planned.
packages/go/schemagen/generator/sql.go (3)
27-163: LGTM! Well-organized icon metadata.The
nodeIconstruct andnodeIconsmap provide a clean, centralized way to manage UI display metadata for node types. The data is appropriately scoped as unexported and covers both AD and Azure node types.
165-171: LGTM! Duplication eliminated through common function.The refactoring to delegate both
GenerateExtensionSQLActiveDirectoryandGenerateExtensionSQLAzureto a commonGenerateExtensionSQLfunction successfully addresses the duplication concern raised in previous reviews.
220-237: LGTM! Proper file handling with error checking.The directory creation and file writing logic correctly handles errors, creates directories as needed, and properly defers file close. The error handling is thorough and appropriate.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/go/schemagen/generator/sql.go (1)
178-211: Consider adding quote escaping for robustness.While the inputs are trusted (from CUE schemas), adding single-quote escaping would prevent SQL syntax errors if schema definitions ever contain quotes. This is a defensive coding practice rather than a security concern.
💡 Example helper for quote escaping
// escapeSQLString escapes single quotes for SQL string literals func escapeSQLString(s string) string { return strings.ReplaceAll(s, "'", "''") }Then use it in the fmt.Sprintf calls:
sb.WriteString(fmt.Sprintf("DELETE FROM schema_extensions WHERE name = '%s';\n\n", escapeSQLString(name)))
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
cmd/api/src/api/mocks/authenticator.gocmd/api/src/daemons/datapipe/mocks/cleanup.gocmd/api/src/database/migration/extensions/az_graph_schema.sqlcmd/api/src/database/mocks/auth.gocmd/api/src/database/mocks/db.gocmd/api/src/queries/mocks/graph.gocmd/api/src/services/agi/mocks/mock.gocmd/api/src/services/dataquality/mocks/mock.gocmd/api/src/services/fs/mocks/fs.gocmd/api/src/services/graphify/mocks/ingest.gocmd/api/src/services/oidc/mocks/oidc.gocmd/api/src/services/saml/mocks/saml.gocmd/api/src/services/upload/mocks/mock.gocmd/api/src/utils/validation/mocks/validator.gocmd/api/src/vendormocks/dawgs/graph/mock.gocmd/api/src/vendormocks/io/fs/mock.gocmd/api/src/vendormocks/neo4j/neo4j-go-driver/v5/neo4j/mock.gopackages/go/crypto/mocks/digest.gopackages/go/schemagen/generator/sql.gopackages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsx
✅ Files skipped from review due to trivial changes (16)
- cmd/api/src/vendormocks/dawgs/graph/mock.go
- cmd/api/src/utils/validation/mocks/validator.go
- packages/javascript/bh-shared-ui/src/views/PrivilegeZones/Details/SelectedDetailsTabs/SelectedDetailsTabs.test.tsx
- cmd/api/src/services/agi/mocks/mock.go
- cmd/api/src/queries/mocks/graph.go
- cmd/api/src/services/upload/mocks/mock.go
- cmd/api/src/services/saml/mocks/saml.go
- cmd/api/src/services/oidc/mocks/oidc.go
- cmd/api/src/daemons/datapipe/mocks/cleanup.go
- cmd/api/src/services/graphify/mocks/ingest.go
- cmd/api/src/vendormocks/io/fs/mock.go
- cmd/api/src/api/mocks/authenticator.go
- cmd/api/src/services/dataquality/mocks/mock.go
- packages/go/crypto/mocks/digest.go
- cmd/api/src/vendormocks/neo4j/neo4j-go-driver/v5/neo4j/mock.go
- cmd/api/src/database/mocks/auth.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/database/mocks/db.go
🧰 Additional context used
🧠 Learnings (5)
📚 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/migration/extensions/az_graph_schema.sqlpackages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.go
📚 Learning: 2025-05-30T16:39:53.440Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1515
File: packages/javascript/bh-shared-ui/src/views/TierManagement/Save/SelectorForm/SelectorForm.tsx:202-211
Timestamp: 2025-05-30T16:39:53.440Z
Learning: In BloodHound, Cypher injection vulnerabilities are only a concern when `enable_cypher_mutations` is enabled, in which case users already have direct access to execute Cypher via the editor or API endpoints, making string interpolation in queries a non-security issue.
Applied to files:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.go
⏰ 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 (5)
packages/go/schemagen/generator/sql.go (3)
27-30: Good encapsulation.The
nodeIconstruct is correctly not exported, limiting its scope to internal use within the generator package.
165-171: Refactoring successfully eliminates duplication.The extraction of common SQL generation logic into
GenerateExtensionSQL(line 173) addresses the previous duplication concerns. The wrapper functions provide clean, schema-specific entry points.
173-238: Clarify DAWGS kinds table requirement scope.The review references a requirement that node and edge kinds must be inserted into a DAWGS kinds table (the
kindtable) before inserting them into their respective schema tables. However, existing migration files (ad_graph_schema.sql,az_graph_schema.sql) insert directly intoschema_node_kindsandschema_edge_kindswithout first inserting into thekindtable. The generated SQL in this file follows the same established pattern.Before requesting changes, confirm:
- Whether this ordering requirement is applicable to the current PR scope or deferred to future work
- The source and timing of this requirement (BED-7067 and the referenced comment could not be found in the codebase)
cmd/api/src/services/fs/mocks/fs.go (1)
1-1: LGTM - Copyright year updated.The copyright year update from 2025 to 2026 is a routine maintenance change with no functional impact.
cmd/api/src/database/migration/extensions/az_graph_schema.sql (1)
16-98: Generated SQL correctly matches source definitions.The SQL file is properly generated from CUE schemas (as indicated in the header) and correctly reflects the
nodeIconsmap frompackages/go/schemagen/generator/sql.go. Icon colors and display kinds are consistent with the generator logic.However, the same verification request from the generator file applies here: please confirm whether LawsonWillard's comment about inserting into the DAWGS kinds table before schema tables applies to this generated SQL.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/api/src/database/graphschema.go (2)
266-286: Dead code: Duplicate name error check.Since
UpdateGraphSchemaNodeKindno longer updates thenamecolumn (lines 274, 277), the duplicate name error check at lines 278-280 is now unreachable. The only way to triggerErrDuplicateSchemaNodeKindNamewas by updating to a conflicting name.Consider removing this dead code for clarity, or keep it as a defensive measure if the behavior might change in the future.
🔎 Optional: Remove dead code
func (s *BloodhoundDB) UpdateGraphSchemaNodeKind(ctx context.Context, schemaNodeKind model.GraphSchemaNodeKind) (model.GraphSchemaNodeKind, error) { if result := s.db.WithContext(ctx).Raw(fmt.Sprintf(` UPDATE %s SET schema_extension_id = ?, display_name = ?, description = ?, is_display_kind = ?, icon = ?, icon_color = ?, updated_at = NOW() WHERE id = ? RETURNING id, name, schema_extension_id, display_name, description, is_display_kind, icon, icon_color, created_at, updated_at, deleted_at`, schemaNodeKind.TableName()), schemaNodeKind.SchemaExtensionId, schemaNodeKind.DisplayName, schemaNodeKind.Description, schemaNodeKind.IsDisplayKind, schemaNodeKind.Icon, schemaNodeKind.IconColor, schemaNodeKind.ID).Scan(&schemaNodeKind); result.Error != nil { - if strings.Contains(result.Error.Error(), DuplicateKeyValueErrorString) { - return model.GraphSchemaNodeKind{}, fmt.Errorf("%w: %v", ErrDuplicateSchemaNodeKindName, result.Error) - } return model.GraphSchemaNodeKind{}, CheckError(result) } else if result.RowsAffected == 0 { return model.GraphSchemaNodeKind{}, ErrNotFound } return schemaNodeKind, nil }
511-532: Dead code: Duplicate name error check (same as node kinds).Similar to
UpdateGraphSchemaNodeKind, lines 524-526 are now unreachable since the name column is no longer updated.🔎 Optional: Remove dead code
func (s *BloodhoundDB) UpdateGraphSchemaEdgeKind(ctx context.Context, schemaEdgeKind model.GraphSchemaEdgeKind) (model.GraphSchemaEdgeKind, error) { if result := s.db.WithContext(ctx).Raw(fmt.Sprintf(` UPDATE %s SET schema_extension_id = ?, description = ?, is_traversable = ?, updated_at = NOW() WHERE id = ? RETURNING id, name, schema_extension_id, description, is_traversable, created_at, updated_at, deleted_at`, schemaEdgeKind.TableName()), schemaEdgeKind.SchemaExtensionId, schemaEdgeKind.Description, schemaEdgeKind.IsTraversable, schemaEdgeKind.ID).Scan(&schemaEdgeKind); result.Error != nil { - if strings.Contains(result.Error.Error(), DuplicateKeyValueErrorString) { - return schemaEdgeKind, fmt.Errorf("%w: %v", ErrDuplicateSchemaEdgeKindName, result.Error) - } return model.GraphSchemaEdgeKind{}, CheckError(result) } else if result.RowsAffected == 0 { return model.GraphSchemaEdgeKind{}, ErrNotFound } return schemaEdgeKind, nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/migration/migrations/v8.5.0.sql
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-25T22:11:53.518Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 2107
File: cmd/api/src/database/graphschema.go:86-100
Timestamp: 2025-11-25T22:11:53.518Z
Learning: In cmd/api/src/database/graphschema.go, the CreateSchemaEdgeKind method intentionally does not use AuditableTransaction or audit logging because it would create too much noise in the audit log, unlike CreateGraphSchemaExtension which does use auditing.
Applied to files:
cmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/migration/migrations/v8.5.0.sqlcmd/api/src/database/graphschema.go
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/database/graphschema_integration_test.gocmd/api/src/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/graphschema_integration_test.gocmd/api/src/database/graphschema.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.sql
🧬 Code graph analysis (2)
cmd/api/src/database/graphschema_integration_test.go (2)
cmd/api/src/model/graphschema.go (6)
GraphSchemaNodeKind(50-60)GraphSchemaNodeKind(63-65)GraphSchemaEdgeKind(89-95)GraphSchemaEdgeKind(97-99)GraphSchemaEdgeKind(127-132)GraphSchemaEdgeKind(134-136)cmd/api/src/model/model.go (1)
Basic(27-31)
cmd/api/src/database/graphschema.go (2)
cmd/api/src/database/db.go (1)
BloodhoundDB(196-199)cmd/api/src/model/graphschema.go (6)
GraphSchemaNodeKind(50-60)GraphSchemaNodeKind(63-65)GraphSchemaEdgeKind(89-95)GraphSchemaEdgeKind(97-99)GraphSchemaEdgeKind(127-132)GraphSchemaEdgeKind(134-136)
⏰ 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 (8)
cmd/api/src/database/graphschema_integration_test.go (2)
465-484: LGTM - Test correctly verifies name immutability.The test appropriately validates that the
Namefield is preserved asnodeKind1.Nameafter update, reflecting the new behavior where the name column is not updated due to FK constraints with the DAWGS kind table.Minor observation: The
CreatedAtandUpdatedAtfields fromupdateWantwill be zero values since they weren't initialized, but this is fine becausecompareGraphSchemaNodeKindonly verifies that the returned timestamps are non-zero rather than comparing against expected values.
903-919: LGTM - Test correctly verifies name immutability for edge kinds.Consistent with the node kind update test, this correctly validates that
Nameis preserved asedgeKind1.Nameafter update, aligning with the append-only nature of the DAWGS kind table.cmd/api/src/database/migration/migrations/v8.5.0.sql (2)
75-79: LGTM - Consistent FK design with schema_node_kinds.The schema_edge_kinds table changes mirror the schema_node_kinds changes, maintaining consistency in how both reference the DAWGS kind table.
41-45: The dual FK design is intentional and justified by the DAWGS schema requirements. Thekindtable has bothid(SMALLSERIAL PRIMARY KEY) andname(VARCHAR UNIQUE) as distinct unique identifiers, making both valid foreign key targets. Referencing both ensures that inserted rows match an actual kind record on both dimensions, providing tighter referential integrity. TheSMALLINTchange fromSERIALis correct since IDs are managed by the kind table, not auto-generated locally. No changes needed.cmd/api/src/database/graphschema.go (4)
64-66: LGTM - Const block consolidation.Moving
DuplicateKeyValueErrorStringto a const block is a clean refactor.
402-429: LGTM - Consistent CTE pattern for edge kinds.The implementation mirrors
CreateGraphSchemaNodeKindwith the same CTE approach, maintaining consistency.The documentation correctly notes that callers must invoke DAWGS
RefreshKindsafter calling this function.
431-464: LGTM - NewGetGraphSchemaEdgeKindsmethod.The implementation follows the established pattern used by
GetGraphSchemaNodeKindsand other paginated query methods in this file.
194-221:kindTableconstant is properly defined and accessible.The
kindTableconstant is defined as a package-level constant incmd/api/src/database/assetgrouptags.go:36with the value"kind". Since both files are in the samecmd/api/src/database/package, the constant is correctly accessible to this method. The CTE approach correctly ensures the kind is created in the DAWGS kind table before inserting into schema_node_kinds, and theON CONFLICT DO UPDATE SET name = ?pattern to force a RETURNING row is valid.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @packages/go/schemagen/generator/sql.go:
- Around line 200-214: The code currently sets is_display_kind using the boolean
found from nodeIcons lookup in the SQL generation loop for nodeKinds (using
kind.GetRepresentation(), kind.GetName()), which implicitly ties displayability
to icon presence; change this to an explicit source of truth: add or consult an
explicit display flag (e.g., a method on the kind like ShouldDisplay(), a
displayableKinds set, or a field on the schema kind) and use that value when
writing the is_display_kind column instead of the nodeIcons lookup; keep using
nodeIcons only to populate icon and icon_color (iconInfo.Icon, iconInfo.Color)
but do not derive displayability from its existence so future kinds without
icons don’t become silently non-displayable.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/api/src/database/migration/extensions/ad_graph_schema.sqlcmd/api/src/database/migration/extensions/az_graph_schema.sqlpackages/go/schemagen/generator/sql.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/api/src/database/migration/extensions/ad_graph_schema.sql
- cmd/api/src/database/migration/extensions/az_graph_schema.sql
🧰 Additional context used
🧠 Learnings (5)
📚 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:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.go
📚 Learning: 2025-05-30T16:39:53.440Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1515
File: packages/javascript/bh-shared-ui/src/views/TierManagement/Save/SelectorForm/SelectorForm.tsx:202-211
Timestamp: 2025-05-30T16:39:53.440Z
Learning: In BloodHound, Cypher injection vulnerabilities are only a concern when `enable_cypher_mutations` is enabled, in which case users already have direct access to execute Cypher via the editor or API endpoints, making string interpolation in queries a non-security issue.
Applied to files:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.go
🧬 Code graph analysis (1)
packages/go/schemagen/generator/sql.go (5)
packages/go/schemagen/model/schema.go (3)
ActiveDirectory(62-73)Azure(50-60)StringEnum(19-24)packages/go/graphschema/ad/ad.go (2)
NodeKinds(1178-1180)PathfindingRelationships(1158-1160)packages/go/graphschema/common/common.go (1)
NodeKinds(39-41)packages/go/graphschema/azure/azure.go (2)
NodeKinds(427-429)PathfindingRelationships(421-423)packages/go/schemagen/generator/golang.go (1)
SchemaSourceName(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-tests
🔇 Additional comments (4)
packages/go/schemagen/generator/sql.go (4)
27-163: LGTM! Well-structured icon metadata.The
nodeIconstruct andnodeIconsmap provide clean separation of UI metadata from the SQL generation logic. The icon/color mappings are comprehensive for both AD and Azure node types.
165-171: LGTM! Clean delegation pattern.The wrapper functions provide a clean API while delegating to the shared
GenerateExtensionSQLfunction, eliminating duplication between AD and Azure SQL generation.
216-254: LGTM! Traversability logic and file I/O are correct.The edge kinds insertion correctly:
- Builds a set of traversable relationship kinds for O(1) lookup
- Sets the
is_traversableflag based on presence inpathfindingRelationshipKinds- Handles directory creation and file I/O with proper error handling
178-192: Schema representations are constrained to safe alphanumeric identifiers and cannot contain SQL special characters.The CUE type system (primitives.cue) enforces that all
representationvalues match the pattern#Symbol: =~"[A-Za-z][A-Za-z0-9]+", which permits only letters and digits. This eliminates the SQL injection risk from unescaped string interpolation since no single quotes or other SQL special characters can be present in schema-derived values.While the current implementation is safe due to this upstream constraint, consider documenting this assumption in the code or adding defensive escaping (e.g., replacing single quotes with
'') for future-proofing against schema system changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI Agents
In @packages/go/schemagen/generator/sql.go:
- Around line 173-254: GenerateExtensionSQL builds SQL by interpolating values
directly into strings (e.g., uses kind.GetRepresentation(), kind.GetName(),
displayName, name, version) which breaks on embedded single quotes; add a small
escaping helper (e.g., escapeSingleQuote(s string) string) and call it for every
place you currently interpolate external strings: the INSERT INTO kind block
(nodeKinds and relationshipKinds loops), the DELETE and INSERT INTO
schema_extensions values (name, displayName, version), the schema_node_kinds
values (kind.GetRepresentation(), kind.GetName(), icon strings), and the
schema_edge_kinds values (relationshipKinds and pathfindingRelationshipKinds
lookups), replacing each direct use of
kind.GetRepresentation()/GetName()/name/displayName/version/icon with the
escaped result before formatting into the SQL builder.
🧹 Nitpick comments (2)
packages/go/schemagen/generator/sql.go (2)
200-214: Consider making display policy explicit rather than icon-derived.Lines 203-206 derive
is_display_kinddirectly from icon availability (foundboolean). This creates an implicit coupling where node types without icons automatically become non-displayable. While this appears intentional for types likeEntity,LocalGroup, andLocalUser, the design is not self-documenting.Risk: If a new node kind is added to the CUE schema without a corresponding entry in
nodeIcons, it will silently becomeis_display_kind=falsewith no explicit indication that this is intentional.Recommendation: Consider adding explicit documentation or a separate data structure (e.g., a set of non-displayable kinds or a field in the schema) to make the display policy clear and intentional rather than derived from icon presence.
💡 Example: Explicit non-displayable kinds set
+// nonDisplayableKinds defines node kinds that should not be displayed in the UI +var nonDisplayableKinds = map[string]struct{}{ + "Base": {}, + "Entity": {}, + "ADLocalGroup": {}, + "ADLocalUser": {}, +} + func GenerateExtensionSQL(...) error { ... for i, kind := range nodeKinds { + _, isNonDisplayable := nonDisplayableKinds[kind.GetRepresentation()] + isDisplayKind := !isNonDisplayable + if iconInfo, found := nodeIcons[kind.GetRepresentation()]; found { - sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, '%s', '%s', '', %t, '%s', '%s')", kind.GetRepresentation(), kind.GetName(), found, iconInfo.Icon, iconInfo.Color)) + sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, '%s', '%s', '', %t, '%s', '%s')", kind.GetRepresentation(), kind.GetName(), isDisplayKind, iconInfo.Icon, iconInfo.Color)) } else { - sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, '%s', '%s', '', %t, '', '')", kind.GetRepresentation(), kind.GetName(), found)) + sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, '%s', '%s', '', %t, '', '')", kind.GetRepresentation(), kind.GetName(), isDisplayKind)) }
173-254: Consider adding unit tests for SQL generation.As suggested by brandonshearin in earlier reviews, unit tests would help maintain this SQL generation logic. Tests could verify:
- Correct SQL structure (DELETE, INSERT statements)
- Icon and color mapping for known node types
- Traversability flag derivation from pathfinding relationships
- Handling of node types with/without icons
💡 Example test structure using t.TempDir()
func TestGenerateExtensionSQL(t *testing.T) { // Setup test data nodeKinds := []model.StringEnum{ {Symbol: "User", Name: "User", Representation: "User"}, {Symbol: "Group", Name: "Group", Representation: "Group"}, } relationshipKinds := []model.StringEnum{ {Symbol: "MemberOf", Representation: "MemberOf"}, } pathfindingKinds := []model.StringEnum{ {Symbol: "MemberOf", Representation: "MemberOf"}, } // Create temp directory tmpDir := t.TempDir() // Execute err := GenerateExtensionSQL("TEST", "Test Extension", "v0.0.1", tmpDir, "test.sql", nodeKinds, relationshipKinds, pathfindingKinds) // Assert require.NoError(t, err) // Verify file exists sqlPath := filepath.Join(tmpDir, "test.sql") require.FileExists(t, sqlPath) // Verify content structure content, err := os.ReadFile(sqlPath) require.NoError(t, err) sql := string(content) assert.Contains(t, sql, "DELETE FROM schema_extensions WHERE name = 'TEST'") assert.Contains(t, sql, "INSERT INTO kind (name) VALUES") assert.Contains(t, sql, "INSERT INTO schema_node_kinds") assert.Contains(t, sql, "INSERT INTO schema_edge_kinds") assert.Contains(t, sql, "ON CONFLICT (name) DO NOTHING") }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/api/src/daemons/datapipe/datapipe_integration_test.gocmd/api/src/database/migration/extensions/ad_graph_schema.sqlcmd/api/src/database/migration/extensions/az_graph_schema.sqlcmd/api/src/database/migration/migrations/v8.5.0.sqlcmd/api/src/services/entrypoint.gocmd/api/src/services/graphify/graphify_integration_test.gopackages/go/graphify/graph/graph.gopackages/go/schemagen/generator/sql.go
🚧 Files skipped from review as they are similar to previous changes (4)
- cmd/api/src/services/entrypoint.go
- cmd/api/src/database/migration/extensions/ad_graph_schema.sql
- cmd/api/src/database/migration/extensions/az_graph_schema.sql
- cmd/api/src/daemons/datapipe/datapipe_integration_test.go
🧰 Additional context used
🧠 Learnings (7)
📚 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:
packages/go/schemagen/generator/sql.gocmd/api/src/services/graphify/graphify_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:
packages/go/schemagen/generator/sql.go
📚 Learning: 2025-05-30T16:39:53.440Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1515
File: packages/javascript/bh-shared-ui/src/views/TierManagement/Save/SelectorForm/SelectorForm.tsx:202-211
Timestamp: 2025-05-30T16:39:53.440Z
Learning: In BloodHound, Cypher injection vulnerabilities are only a concern when `enable_cypher_mutations` is enabled, in which case users already have direct access to execute Cypher via the editor or API endpoints, making string interpolation in queries a non-security issue.
Applied to files:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.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.sql
📚 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/migration/migrations/v8.5.0.sql
🧬 Code graph analysis (2)
packages/go/graphify/graph/graph.go (1)
cmd/api/src/bootstrap/server.go (1)
PopulateExtensionData(75-81)
packages/go/schemagen/generator/sql.go (4)
packages/go/schemagen/model/schema.go (3)
ActiveDirectory(62-73)Azure(50-60)StringEnum(19-24)packages/go/graphschema/azure/azure.go (2)
NodeKinds(427-429)PathfindingRelationships(421-423)packages/go/graphschema/common/common.go (1)
NodeKinds(39-41)packages/go/schemagen/generator/golang.go (1)
SchemaSourceName(32-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: run-analysis
- GitHub Check: build-ui
- GitHub Check: run-tests
🔇 Additional comments (7)
cmd/api/src/services/graphify/graphify_integration_test.go (1)
91-92: LGTM! Extension data population correctly integrated into test setup.The addition of
PopulateExtensionData(ctx)properly mirrors the production startup flow and is positioned logically after schema assertion but before ingest schema loading. Error handling is appropriate for test initialization.Note: Based on the PR comments, be aware that BED-7067 may introduce ordering requirements around DAWGS kinds table population, but that appears to be a future consideration rather than a current blocker.
packages/go/schemagen/generator/sql.go (2)
165-171: LGTM! Clean wrapper design eliminates duplication.The refactoring to use a shared
GenerateExtensionSQLfunction with thin wrapper functions is a good approach. This eliminates code duplication while maintaining clear, domain-specific entry points for AD and Azure schema generation.
178-198: Verify kind insertion order aligns with BED-7067 requirements.Per LawsonWillard's comment, "node and edge kinds need to be inserted into the DAWGS kinds table before inserting them into their respective schema tables." The current code inserts into
kindtable (lines 178-192) before populatingschema_node_kindsandschema_edge_kinds(lines 200+). Please confirm:
- Is the
kindtable the DAWGS kinds table referenced in the comment?- Does this order satisfy the BED-7067 requirement, or will additional changes be needed?
The
ON CONFLICT (name) DO NOTHINGclause (line 192) suggests this handles pre-existing kinds, which may be the intended behavior.packages/go/graphify/graph/graph.go (1)
180-181: The initialization sequence ordering is correct.migrations.NewGraphMigrator(graphDB).Migrate(ctx)establishes the graph schema (viaAssertSchemaandExecuteStepwiseMigrations), which ensures thekindtable exists in DAWGS. Subsequently,s.db.PopulateExtensionData(ctx)executes the extension SQL files that insert AD and AZ kinds into this table, satisfying the requirement that kinds be present before schema table operations. No changes needed.cmd/api/src/database/migration/migrations/v8.5.0.sql (3)
45-45: Thekindtable schema (defined in v7.3.0.sql) usesvarchar(256)with a UNIQUE constraint on thenamecolumn, which is fully compatible with the foreign key constraints in lines 45 and 79. No type mismatch or performance issues exist.
29-39: This review comment is based on an incorrect premise. The tablesschema_extensions,schema_node_kinds, andschema_edge_kindsare being introduced for the first time in v8.5.0—they do not exist in any earlier migration files (v6.1.0 through v8.4.0) or in schema.sql. Therefore, the concern about pre-existing tables with different schemas causing drift is not applicable here.The use of
CREATE TABLE IF NOT EXISTSis appropriate for these new tables because:
- The migration framework tracks completed versions in the
migrationstable- For new installations, all migrations run sequentially
- For upgrades, only migrations after the last successful version are executed
- If v8.5.0 is re-run, the
IF NOT EXISTSclause prevents errors while the framework's version tracking ensures the migration only executes onceNo ALTER statements are needed, and the framework's version-based approach inherently prevents partial migrations.
Likely an incorrect or invalid review comment.
43-45: Critical migration bug: Missingidvalues in INSERT statements will cause constraint violations.The
schema_node_kindsandschema_edge_kindstable definitions requireidas a PRIMARY KEY with a foreign key reference tokind(id), but the extension SQL files don't provideidvalues in their INSERT statements. PostgreSQL will reject these inserts with a constraint violation because:
idis defined asPRIMARY KEYon both tables with noDEFAULTvalue and no auto-increment mechanism- Extension INSERT statements only provide
(schema_extension_id, name, display_name, ...)but omit the requiredidcolumn- The tables cannot auto-generate these IDs since they're foreign keys constrained to existing
kind.idvaluesThe inserts need to either:
- Include
idvalues that correspond to the matching rows in thekindtable, or- Implement a trigger to auto-populate
idby looking up thekindtable based on thenamecolumnAs currently written, the migration will fail when loading extension schemas.
⛔ Skipped due to 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/go/schemagen/generator/sql.go (1)
246-253: Consider usingfilepath.Joininstead ofpath.Joinfor file operations.
path.Joinuses forward slashes regardless of OS, whilefilepath.Joinuses the OS-specific separator. For file system operations,filepath.Joinis the idiomatic choice in Go.🔎 Proposed fix
import ( "fmt" "os" - "path" + "path/filepath" "strings" "github.com/specterops/bloodhound/packages/go/schemagen/model" )- if fout, err := os.OpenFile(path.Join(dir, fileName), fileOpenMode, defaultSourceFilePermission); err != nil { + if fout, err := os.OpenFile(filepath.Join(dir, fileName), fileOpenMode, defaultSourceFilePermission); err != nil {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/api/src/daemons/datapipe/datapipe_integration_test.gocmd/api/src/database/migration/extensions/ad_graph_schema.sqlcmd/api/src/database/migration/extensions/az_graph_schema.sqlcmd/api/src/services/entrypoint.gocmd/api/src/services/graphify/graphify_integration_test.gopackages/go/graphify/graph/graph.gopackages/go/schemagen/generator/sql.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/api/src/services/entrypoint.go
- cmd/api/src/database/migration/extensions/az_graph_schema.sql
- cmd/api/src/services/graphify/graphify_integration_test.go
- packages/go/graphify/graph/graph.go
- cmd/api/src/daemons/datapipe/datapipe_integration_test.go
🧰 Additional context used
🧠 Learnings (6)
📚 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/migration/extensions/ad_graph_schema.sql
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.go
📚 Learning: 2025-05-30T16:39:53.440Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1515
File: packages/javascript/bh-shared-ui/src/views/TierManagement/Save/SelectorForm/SelectorForm.tsx:202-211
Timestamp: 2025-05-30T16:39:53.440Z
Learning: In BloodHound, Cypher injection vulnerabilities are only a concern when `enable_cypher_mutations` is enabled, in which case users already have direct access to execute Cypher via the editor or API endpoints, making string interpolation in queries a non-security issue.
Applied to files:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build BloodHound Container Image / Build and Package Container
- GitHub Check: build-ui
- GitHub Check: run-analysis
- GitHub Check: run-tests
🔇 Additional comments (3)
cmd/api/src/database/migration/extensions/ad_graph_schema.sql (1)
18-129: Structure and ordering look correct for extension data population.The SQL correctly inserts kinds into the
kindtable first (withON CONFLICT DO NOTHINGfor idempotency), then populates the extension and its related schema tables atomically within the DO block.Note on BED-7067: Per the PR comment from LawsonWillard, ensure the kinds are also inserted into the DAWGS kinds table before these schema table inserts. Verify this ordering requirement is satisfied at the migration execution layer.
packages/go/schemagen/generator/sql.go (2)
27-163: LGTM: Well-organized icon metadata map.The
nodeIconsmap provides clear, centralized icon and color configuration for both AD and Azure node types. The organization with comments separating AD and Azure types improves maintainability.
165-171: LGTM: Clean wrapper functions address DRY concerns.The refactoring to delegate to
GenerateExtensionSQLeliminates the duplication flagged in earlier reviews.
…lized kind_id column as FK to DAWGS
# Conflicts: # cmd/api/src/database/migration/migrations/v8.5.0.sql
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/go/schemagen/generator/sql.go:
- Around line 194-198: The DELETE statement emitted with
sb.WriteString(fmt.Sprintf("DELETE FROM schema_extensions WHERE name =
'%s';\n\n", name)) is placed before the DO $$ BEGIN block so a failed INSERT
leaves the extension deleted; move that DELETE into the DO $$ BEGIN block (e.g.,
emit the DELETE immediately after "BEGIN\n" and before the INSERT line produced
by sb.WriteString(fmt.Sprintf("\tINSERT INTO schema_extensions ... RETURNING id
INTO new_extension_id;\n\n", name, displayName, version))) so the
delete-and-insert occur atomically inside the same DO $$; ensure SQL
syntax/semicolons remain correct.
🧹 Nitpick comments (5)
cmd/api/src/database/migration/extensions/az_graph_schema.sql (1)
90-96: Consider moving DELETE inside the DO $$ block for atomicity.The
DELETE FROM schema_extensions WHERE name = 'AZ'on line 90 executes outside theDO $$block. If the subsequent INSERT fails, the extension will be deleted without replacement, leaving the system in an inconsistent state. Consider moving the DELETE inside the BEGIN block to ensure atomic delete-and-recreate.Suggested change
-DELETE FROM schema_extensions WHERE name = 'AZ'; - DO $$ DECLARE new_extension_id INT; BEGIN + DELETE FROM schema_extensions WHERE name = 'AZ'; + INSERT INTO schema_extensions (name, display_name, version, is_builtin) VALUES ('AZ', 'Azure', 'v0.0.1', true) RETURNING id INTO new_extension_id;cmd/api/src/database/migration/extensions/ad_graph_schema.sql (1)
123-129: Same atomicity concern as Azure extension script.The
DELETE FROM schema_extensions WHERE name = 'AD'on line 123 executes outside theDO $$block, creating the same atomicity risk as noted in the Azure script. Since this is generated code, the fix should be applied inpackages/go/schemagen/generator/sql.go.packages/go/schemagen/generator/sql.go (1)
202-212: Consider renamingfoundto clarify its use asis_display_kind.The
foundvariable from the icon lookup is directly used as theis_display_kindvalue. While this logic is correct (nodes with icons should be displayed), usingfoundfor this purpose reduces readability. Consider renaming to make the intent clearer.Suggested improvement
for i, kind := range nodeKinds { - if iconInfo, found := nodeIcons[kind.GetRepresentation()]; found { - sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, (SELECT id FROM kind WHERE name = '%s'), '%s', '', %t, '%s', '%s')", kind.GetRepresentation(), kind.GetName(), found, iconInfo.Icon, iconInfo.Color)) + iconInfo, isDisplayKind := nodeIcons[kind.GetRepresentation()] + if isDisplayKind { + sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, (SELECT id FROM kind WHERE name = '%s'), '%s', '', %t, '%s', '%s')", kind.GetRepresentation(), kind.GetName(), isDisplayKind, iconInfo.Icon, iconInfo.Color)) } else { - sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, (SELECT id FROM kind WHERE name = '%s'), '%s', '', %t, '', '')", kind.GetRepresentation(), kind.GetName(), found)) + sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, (SELECT id FROM kind WHERE name = '%s'), '%s', '', %t, '', '')", kind.GetRepresentation(), kind.GetName(), isDisplayKind)) }cmd/api/src/database/graphschema.go (2)
239-262: Consider extracting the table-qualification logic into a helper function.The same pattern of iterating over filters and sort columns to add table prefixes is repeated in
GetGraphSchemaEdgeKinds(lines 482-505). This duplication could be reduced with a shared helper function.
534-570: Inconsistent filter handling compared to other Get functions.Unlike
GetGraphSchemaNodeKindsandGetGraphSchemaEdgeKinds, this function does not apply table prefixes to filter columns. With three tables joined (schema_edge_kinds,schema_extensions,kind), filtering on common column names likenamewill cause ambiguous column errors unless the caller explicitly qualifies them (e.g.,schema.nameas shown in tests).Consider either:
- Adding the same table-qualification logic as the other Get functions
- Documenting this behavior in the function's doc comment to clarify that callers must qualify ambiguous column names
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
cmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/migration/extensions/ad_graph_schema.sqlcmd/api/src/database/migration/extensions/az_graph_schema.sqlcmd/api/src/database/migration/migrations/v8.5.0.sqlcmd/api/src/database/mocks/db.gocmd/api/src/database/parameters_test.gocmd/api/src/services/entrypoint.gopackages/go/schemagen/generator/sql.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/database/mocks/db.go
🧰 Additional context used
🧠 Learnings (8)
📚 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/migration/extensions/ad_graph_schema.sqlcmd/api/src/database/graphschema_integration_test.gopackages/go/schemagen/generator/sql.gocmd/api/src/database/graphschema.gocmd/api/src/database/migration/extensions/az_graph_schema.sqlcmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-06-06T23:12:14.181Z
Learnt from: elikmiller
Repo: SpecterOps/BloodHound PR: 1563
File: packages/go/graphschema/azure/azure.go:24-24
Timestamp: 2025-06-06T23:12:14.181Z
Learning: In BloodHound, files in packages/go/graphschema/*/`*.go` are generated from CUE schemas. When `just prepare-for-codereview` is run, it triggers code generation that may automatically add import aliases or other formatting changes. These changes are legitimate outputs of the generation process, not manual edits that would be overwritten.
Applied to files:
cmd/api/src/database/graphschema_integration_test.gopackages/go/schemagen/generator/sql.gocmd/api/src/database/graphschema.gocmd/api/src/database/migration/extensions/az_graph_schema.sql
📚 Learning: 2025-07-22T20:30:34.839Z
Learnt from: LawsonWillard
Repo: SpecterOps/BloodHound PR: 1700
File: cmd/api/src/api/v2/saved_queries_test.go:3182-3182
Timestamp: 2025-07-22T20:30:34.839Z
Learning: In Go table-driven tests in cmd/api/src/api/v2/saved_queries_test.go, subtest parallelization with t.Parallel() is acceptable when tests are self-contained, each creating their own mock controller (gomock.NewController(t)) and having isolated mock expectations without shared state between subtests.
Applied to files:
cmd/api/src/database/graphschema_integration_test.gopackages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.gocmd/api/src/database/graphschema.go
📚 Learning: 2025-05-30T16:39:53.440Z
Learnt from: benwaples
Repo: SpecterOps/BloodHound PR: 1515
File: packages/javascript/bh-shared-ui/src/views/TierManagement/Save/SelectorForm/SelectorForm.tsx:202-211
Timestamp: 2025-05-30T16:39:53.440Z
Learning: In BloodHound, Cypher injection vulnerabilities are only a concern when `enable_cypher_mutations` is enabled, in which case users already have direct access to execute Cypher via the editor or API endpoints, making string interpolation in queries a non-security issue.
Applied to files:
packages/go/schemagen/generator/sql.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:
packages/go/schemagen/generator/sql.go
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
packages/go/schemagen/generator/sql.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.sql
🧬 Code graph analysis (4)
cmd/api/src/services/entrypoint.go (1)
cmd/api/src/bootstrap/server.go (1)
PopulateExtensionData(75-81)
cmd/api/src/database/graphschema_integration_test.go (3)
cmd/api/src/model/graphschema.go (6)
GraphSchemaNodeKind(50-60)GraphSchemaNodeKind(63-65)GraphSchemaEdgeKind(89-95)GraphSchemaEdgeKind(97-99)GraphSchemaEdgeKind(139-144)GraphSchemaEdgeKind(146-148)cmd/api/src/model/model.go (1)
Basic(27-31)packages/go/graphschema/common/common.go (2)
DisplayName(54-54)Description(55-55)
packages/go/schemagen/generator/sql.go (5)
packages/go/schemagen/model/schema.go (3)
ActiveDirectory(62-73)Azure(50-60)StringEnum(19-24)packages/go/graphschema/common/common.go (1)
NodeKinds(39-41)packages/go/graphschema/azure/azure.go (2)
NodeKinds(427-429)PathfindingRelationships(421-423)packages/go/graphschema/ad/ad.go (2)
NodeKinds(1178-1180)PathfindingRelationships(1158-1160)packages/go/schemagen/generator/golang.go (1)
SchemaSourceName(32-32)
cmd/api/src/database/graphschema.go (2)
cmd/api/src/model/graphschema.go (6)
GraphSchemaNodeKind(50-60)GraphSchemaNodeKind(63-65)GraphSchemaEdgeKind(89-95)GraphSchemaEdgeKind(97-99)GraphSchemaEdgeKind(139-144)GraphSchemaEdgeKind(146-148)cmd/api/src/database/helper.go (1)
CheckError(39-45)
⏰ 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 (15)
cmd/api/src/database/migration/migrations/v8.5.0.sql (2)
76-86: LGTM on the schema_edge_kinds changes.The structure mirrors
schema_node_kindsappropriately, with inline PRIMARY KEY andkind_idforeign key referencingkind(id)with ON DELETE CASCADE. The UNIQUE constraint onkind_idenforces a one-to-one mapping.Per the PR comment regarding BED-7067: ensure node and edge kinds are inserted into the DAWGS
kindtable before populating these schema tables, as the foreign key constraints will reject inserts for non-existent kind IDs.
44-54: LGTM on the schema_node_kinds changes.The inline PRIMARY KEY declaration and the new
kind_idforeign key tokind(id)with ON DELETE CASCADE are appropriate. The UNIQUE constraint ensures each kind maps to exactly one schema_node_kinds entry.One minor observation:
kind_idusesSMALLINThere, which correctly matches thekindtable'sSMALLSERIALid column (defined in v7.3.0.sql). However, other tables in this migration (schema_environmentsat lines 94-95 andschema_relationship_findingsat line 109) useINTEGERfor their kind references instead ofSMALLINT. This type inconsistency within the same migration file may warrant a review to ensure it's intentional.cmd/api/src/database/parameters_test.go (1)
176-180: Verify the expected default value change for ExpansionWorkerLimit.The expected
ExpansionWorkerLimitchanged from 7 to 3 when an invalid value (-1) is provided. This suggests the fallback/default behavior was updated. Please confirm this aligns with the intended default configuration forExpansionWorkerLimit.cmd/api/src/database/migration/extensions/az_graph_schema.sql (1)
18-88: Verify ordering with DAWGS kinds table per BED-7067.Per PR comment from LawsonWillard, with BED-7067 incoming, node and edge kinds must be inserted into the DAWGS kinds table before inserting them into their respective schema tables. Ensure the extension population flow handles this ordering requirement, or coordinate with BED-7067 to address it.
cmd/api/src/services/entrypoint.go (1)
94-96: LGTM!The extension data population step is correctly placed after database and graph migrations, with proper error handling that follows the established pattern. The error message clearly identifies the failure source.
packages/go/schemagen/generator/sql.go (2)
165-171: LGTM!The wrapper functions cleanly delegate to
GenerateExtensionSQLwith appropriate parameters for each extension type.
236-253: LGTM!File operations are handled correctly with proper directory creation, error handling, and resource cleanup via deferred close.
cmd/api/src/database/graphschema.go (5)
70-72: LGTM on the constant definition.Centralizing the duplicate key error string promotes consistency and maintainability for error detection across all schema operations.
299-326: LGTM on the update semantics.The implementation correctly preserves the FK relationship to the DAWGS kind table by not allowing name updates. The doc comment clearly explains this constraint and the alternative approach.
443-470: LGTM on the edge kind creation pattern.Consistent with the node kind creation approach - ensures kind exists in DAWGS table before inserting into
schema_edge_kinds. The doc comment appropriately notes the need to callRefreshKindsafter this operation.
581-608: LGTM on the edge kind update semantics.Consistent with node kind updates - preserves the FK relationship to the DAWGS kind table.
200-227: Scope confusion: ThekindTableconstant is properly defined but is not used in the reviewed function.The
kindTableconstant is correctly defined incmd/api/src/database/assetgrouptags.go:36askindTable = "kind"and is used throughout the database package. However, theCreateGraphSchemaNodeKindfunction (lines 200-227) does not referencekindTable—it constructs table names directly via model methods (model.GraphSchemaNodeKind{}.TableName()). The references tokindTableat lines 272 and beyond are in other functions, not in the function being reviewed.cmd/api/src/database/graphschema_integration_test.go (3)
466-485: LGTM on updated test semantics.The test correctly verifies that the
Namefield is preserved from the originalnodeKind1rather than being updated toupdateWant.Name, which aligns with the new implementation where names cannot be changed (FK to DAWGS kind table).
904-920: LGTM on edge kind update test.Correctly verifies that
edgeKind1.Nameis preserved after update, consistent with the node kind test pattern.
461-461: LGTM on error message assertions.The updated error messages with table prefixes (
nk.,ek.) correctly reflect the table-qualified filter handling in the implementation.Also applies to: 899-899
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@cmd/api/src/database/migration/extensions/ad_graph_schema.sql`:
- Around line 42-74: The SQL generator is emitting two identical definitions of
genscript_upsert_schema_edge_kind (duplicate function block); fix the generator
so it only emits one upsert function for edge kinds by removing the duplicate
emission path in the generator code that produces the
genscript_upsert_schema_edge_kind block (the duplicate-emitting logic in
sql.go), ensure the generator deduplicates or guards against emitting the same
function twice, then regenerate the SQL file so the duplicate definition is
gone.
In `@packages/go/schemagen/generator/sql.go`:
- Around line 207-243: The generator emits the same CREATE OR REPLACE FUNCTION
for genscript_upsert_schema_edge_kind twice via two sb.WriteString(...) calls;
remove the duplicate sb.WriteString block so the function is written only once
(keep a single sb.WriteString that contains the
genscript_upsert_schema_edge_kind function definition and delete the repeated
block).
♻️ Duplicate comments (1)
packages/go/schemagen/generator/sql.go (1)
292-327: SQL injection risk from unescaped string interpolation persists.Lines 292-295, 300, 305, 313, and 327 interpolate values directly into SQL strings without escaping single quotes. While these values originate from controlled CUE schemas, embedded single quotes would break the generated SQL.
🔧 Proposed fix using an escaping helper
// escapeSQLString escapes single quotes for SQL string literals func escapeSQLString(s string) string { return strings.ReplaceAll(s, "'", "''") }Then apply to all interpolation sites, e.g.:
-sb.WriteString(fmt.Sprintf("\tPERFORM genscript_upsert_kind('%s');\n", kind.GetRepresentation())) +sb.WriteString(fmt.Sprintf("\tPERFORM genscript_upsert_kind('%s');\n", escapeSQLString(kind.GetRepresentation())))
🧹 Nitpick comments (2)
packages/go/schemagen/generator/sql.go (1)
367-380: Minor: Redundantgenscript_upsert_kindcalls for 'Domain' and 'Tenant'.Line 368 calls
genscript_upsert_kind('Domain')but 'Domain' is already inserted as part ofnodeKindsin the AD schema (line 300 loop). Similarly, line 375 inserts 'Tenant' which is already in Azure'snodeKinds. The upsert semantics make this harmless, but it's redundant.cmd/api/src/database/migration/extensions/ad_graph_schema.sql (1)
27-27: Typo:retreivedshould beretrieved.The variable name
retreived_kind_id(and similar variants) appears throughout the generated functions. This typo originates from the generator and should be fixed there.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cmd/api/src/database/graphschema.go`:
- Around line 488-497: The loop that maps edgeKindFilters into
tableIdentifiedFilters is mutating the caller's map by calling
delete(edgeKindFilters, column); remove those delete(...) calls so you only
populate tableIdentifiedFilters (using fmt.Sprintf with "k" or "ek" as you
already do) and do not alter edgeKindFilters; locate the loop that references
edgeKindFilters and tableIdentifiedFilters in graphschema.go and simply stop
deleting keys from edgeKindFilters.
- Around line 243-252: The loop over filters in the graph schema mutates the
caller's map via delete(filters, column); update the implementation to stop
modifying the input map by removing the delete(filters, column) calls and only
write to tableIdentifiedFilters (using the existing key logic with "k" and
"nk"); ensure the code still iterates over filters and assigns to
tableIdentifiedFilters[fmt.Sprintf("%s.%s", "k"/"nk", column)] = filter without
deleting from filters so the caller's map remains unchanged.
- Around line 225-229: The error handling in the CreateGraphSchemaNodeKind path
returns result.Error directly instead of normalizing with CheckError(result);
update the failure return to use CheckError(result) while preserving the
existing DuplicateKeyValueErrorString special-case that maps to
ErrDuplicateSchemaNodeKindName (i.e., keep the if
strings.Contains(result.Error.Error(), DuplicateKeyValueErrorString) branch
as-is, and change the final return from "result.Error" to "CheckError(result)"
so behavior matches CreateGraphSchemaEdgeKind and other create functions).
♻️ Duplicate comments (1)
cmd/api/src/database/graphschema.go (1)
243-266: Consider extracting table identifier logic to a helper function.This logic for adding table prefixes to filters and sort columns is duplicated in
GetGraphSchemaEdgeKinds(lines 488-511). A helper function could reduce duplication and ensure consistent behavior.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@cmd/api/src/database/migration/extensions/ad_graph_schema.sql`:
- Around line 337-341: The cleanup DROP list omits the helper function
genscript_upsert_source_kind so it remains after migration; update the generator
output that produces the cleanup section (the block that emits DROP FUNCTION IF
EXISTS for helper functions) to include genscript_upsert_source_kind alongside
genscript_upsert_kind, genscript_upsert_schema_node_kind,
genscript_upsert_schema_edge_kind, genscript_upsert_schema_environments, and
genscript_upsert_schema_environments_principal_kinds so the migration drops
genscript_upsert_source_kind as well; locate the code that generates that DROP
FUNCTION list and add "genscript_upsert_source_kind" to it.
In `@cmd/api/src/database/migration/extensions/az_graph_schema.sql`:
- Around line 272-276: The cleanup sections for both generated schema files are
missing a DROP for the helper function genscript_upsert_source_kind (created
earlier in the script); update the SQL generator
(packages/go/schemagen/generator/sql.go or the equivalent generator used to emit
az_graph_schema.sql and ad_graph_schema.sql) to include "DROP FUNCTION IF EXISTS
genscript_upsert_source_kind;" in the cleanup block for generated schema outputs
so both az_graph_schema.sql and ad_graph_schema.sql emit that DROP alongside the
other genscript_upsert_* DROP statements.
In `@packages/go/schemagen/generator/sql.go`:
- Around line 330-335: Add a DROP for genscript_upsert_source_kind to the
cleanup block that builds the DROP statements (the sb.WriteString call that
currently lists genscript_upsert_kind, genscript_upsert_schema_node_kind,
genscript_upsert_schema_edge_kind, genscript_upsert_schema_environments, and
genscript_upsert_schema_environments_principal_kinds). Modify that string to
include "DROP FUNCTION IF EXISTS genscript_upsert_source_kind;" so the function
created earlier in the file (genscript_upsert_source_kind at lines ~188-195) is
removed during cleanup.
♻️ Duplicate comments (1)
packages/go/schemagen/generator/sql.go (1)
282-286: SQL injection risk from unescaped string interpolation.String values (
name,displayName,version) are interpolated directly into SQL without escaping single quotes. While these originate from controlled code rather than user input, embedded single quotes would break the generated SQL.This was flagged in a previous review. Consider adding an escaping helper as suggested.
🧹 Nitpick comments (6)
cmd/api/src/database/migration/extensions/ad_graph_schema.sql (4)
32-47: Minor typo in variable name:retreived→retrieved.The variable
retreived_kind_id(line 34) is misspelled. Since this is generated code, the fix should be applied in the Cuelang generator.
77-80: Misleading error message for source_kind lookup.The exception at line 79 says "couldn't find matching kind_id" but it's actually looking for a
source_kind_idin thesource_kindstable. This could confuse debugging efforts. Since this is generated code, the generator should be updated to produce a more specific error message.♻️ Suggested fix (for the generator)
SELECT id INTO retreived_source_kind_id FROM source_kinds WHERE name = v_source_kind_name; IF retreived_source_kind_id IS NULL THEN - RAISE EXCEPTION 'couldn''t find matching kind_id'; + RAISE EXCEPTION 'couldn''t find matching source_kind_id for name: %', v_source_kind_name; END IF;
330-331: Redundant kind insertion for 'Domain'.Line 331 calls
genscript_upsert_kind('Domain')but 'Domain' was already inserted at line 128. This is harmless due to the idempotent function, but unnecessary.
112-112: Consider locking all modified tables for consistency.The LOCK statement only locks
schema_extensions, schema_node_kinds, schema_edge_kinds, kind, but the script also modifiessource_kinds,schema_environments, andschema_environments_principal_kinds. While this may not be an issue if the script runs exclusively at startup, adding these tables to the lock would ensure consistency if concurrent access is ever possible.♻️ Suggested fix (for the generator)
- LOCK schema_extensions, schema_node_kinds, schema_edge_kinds, kind; + LOCK schema_extensions, schema_node_kinds, schema_edge_kinds, schema_environments, schema_environments_principal_kinds, kind, source_kinds;cmd/api/src/database/migration/extensions/az_graph_schema.sql (1)
107-112: Consider addingsource_kindsto the LOCK statement.Line 264 modifies the
source_kindstable viagenscript_upsert_source_kind('AZBase'), but this table is not included in the LOCK statement. If this script could run concurrently, this might cause race conditions.Since this is generated code, the fix would need to be applied in the SQL generator.
packages/go/schemagen/generator/sql.go (1)
197-214: Minor typo:retreived→retrieved.The variable name
retreived_kind_idappears in multiple generated PL/pgSQL functions (lines 200, 219, 238, 239, 266). While functionally harmless, consider fixing for consistency.✏️ Proposed fix
CREATE OR REPLACE FUNCTION genscript_upsert_schema_node_kind(v_extension_id INT, v_kind_name VARCHAR(256), v_display_name TEXT, v_description TEXT, v_is_display_kind BOOLEAN, v_icon TEXT, v_icon_color TEXT) RETURNS void AS $$ DECLARE - retreived_kind_id SMALLINT; + retrieved_kind_id SMALLINT; BEGIN - SELECT id INTO retreived_kind_id FROM kind WHERE name = v_kind_name; - IF retreived_kind_id IS NULL THEN + SELECT id INTO retrieved_kind_id FROM kind WHERE name = v_kind_name; + IF retrieved_kind_id IS NULL THEN RAISE EXCEPTION 'couldn''t find matching kind_id'; END IF; - IF NOT EXISTS (SELECT id FROM schema_node_kinds nk WHERE nk.kind_id = retreived_kind_id) THEN - INSERT INTO schema_node_kinds (schema_extension_id, kind_id, display_name, description, is_display_kind, icon, icon_color) VALUES (v_extension_id, retreived_kind_id, v_display_name, v_description, v_is_display_kind, v_icon, v_icon_color); + IF NOT EXISTS (SELECT id FROM schema_node_kinds nk WHERE nk.kind_id = retrieved_kind_id) THEN + INSERT INTO schema_node_kinds (schema_extension_id, kind_id, display_name, description, is_display_kind, icon, icon_color) VALUES (v_extension_id, retrieved_kind_id, v_display_name, v_description, v_is_display_kind, v_icon, v_icon_color); ELSE - UPDATE schema_node_kinds SET display_name = v_display_name, description = v_description, is_display_kind = v_is_display_kind, icon = v_icon, icon_color = v_icon_color WHERE kind_id = retreived_kind_id; + UPDATE schema_node_kinds SET display_name = v_display_name, description = v_description, is_display_kind = v_is_display_kind, icon = v_icon, icon_color = v_icon_color WHERE kind_id = retrieved_kind_id; END IF; END; $$ LANGUAGE plpgsql;Apply similar changes to the other generated functions.
6 participants
Description
Creates SQL files to populate AD and AZ extension schemas
Motivation and Context
Resolves BED-6721
Why is this change required? What problem does it solve?
All schema definitions are being moved over to postgres. This incremental change moves the AD and AZ schemas over to postgres without removing existing functionality.
How Has This Been Tested?
Code is generated running
just generate.Data population is run on app startup.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.