feat(OpenGraph): Generate AD and AZ extension schemas and apply on startup - BED-6721 #2201
Conversation
WalkthroughThis PR introduces extension data population functionality, adding database methods to populate extension metadata and SQL schema definitions for Active Directory and Azure components. The changes include new migration infrastructure to execute extension SQL files, integration into the application startup flow, and schema generation tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant Bootstrap
participant Entrypoint
participant Database
participant Migrator
participant FileSystem as File System
participant SQL as SQL Engine
Bootstrap->>Entrypoint: Start application
Entrypoint->>Database: MigrateDB()
Database->>Migrator: Migrate()
Migrator->>SQL: Execute migrations
SQL-->>Migrator: Success
Migrator-->>Database: Migration complete
Database-->>Entrypoint: DB ready
rect rgb(200, 220, 255)
Note over Entrypoint,SQL: Extension Data Population
Entrypoint->>Database: PopulateExtensionData(ctx)
Database->>Migrator: ExecuteExtensionDataPopulation()
Migrator->>FileSystem: ReadDir("extensions")
FileSystem-->>Migrator: Extension SQL files
loop For each .sql file
Migrator->>FileSystem: ReadFile(sqlFile)
FileSystem-->>Migrator: File content
Migrator->>SQL: Execute in transaction
SQL-->>Migrator: Success
end
Migrator-->>Database: Population complete
Database-->>Entrypoint: Success
end
Entrypoint->>Entrypoint: Continue startup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)cmd/api/src/database/mocks/db.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
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.
| }, | ||
| } | ||
|
|
||
| func GenerateExtensionSQLActiveDirectory(dir string, adSchema model.ActiveDirectory) error { |
There was a problem hiding this comment.
some simple unit testing may be helpful with maintaining this new stuff too. you can use the TempDir from testing.T to do something like:
// Setup test data
adSchema := model.ActiveDirectory{
NodeKinds: []model.NodeKind{
{Symbol: "User", Name: "User"},
{Symbol: "Computer", Name: "Computer"},
{Symbol: "Group", Name: "Group"},
},
RelationshipKinds: []model.RelationshipKind{
{Symbol: "MemberOf"},
{Symbol: "AdminTo"},
},
PathfindingRelationships: []model.PathfindingRelationship{
{Symbol: "MemberOf"},
},
}
// Create temp directory
tmpDir := t.TempDir()
// Execute
err := GenerateExtensionSQLActiveDirectory(tmpDir, adSchema)
// Assert
require.NoError(t, err)
// Verify file was created
sqlPath := filepath.Join(tmpDir, "ad.sql")
require.FileExists(t, sqlPath)
// Read and verify content
content, err := os.ReadFile(sqlPath)
require.NoError(t, err)
// Assertions on content...
}```
There was a problem hiding this comment.
and content assertions could be for anything-- node icon/color mapping, traversability flag, or just high level structure. you can read the sql file into a variable for assertions like:
content, _ := os.ReadFile(filepath.Join(tmpDir, "ad.sql"))
sql := string(content)
// Verify SQL structure
assert.Contains(t, sql, "DELETE FROM schema_extensions WHERE name = 'AD'")
assert.Contains(t, sql, "INSERT INTO schema_extensions")
assert.Contains(t, sql, "INSERT INTO schema_node_kinds")
assert.Contains(t, sql, "INSERT INTO schema_edge_kinds")
assert.Contains(t, sql, "blah blah ")
There was a problem hiding this comment.
The only reason I avoided this was because it's a temporary function and none of the other generator stuff is unit tested. Perhaps it's a good idea tho
|
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.
| sb.WriteString("\tINSERT INTO schema_node_kinds (schema_extension_id, name, display_name, description, is_display_kind, icon, icon_color) VALUES\n") | ||
|
|
||
| for i, kind := range nodeKinds { | ||
| 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)) | ||
| } else { | ||
| sb.WriteString(fmt.Sprintf("\t\t(new_extension_id, '%s', '%s', '', %t, '', '')", kind.GetRepresentation(), kind.GetName(), found)) | ||
| } | ||
|
|
||
| if i != len(nodeKinds)-1 { | ||
| sb.WriteString(",\n") | ||
| } | ||
| } | ||
|
|
||
| sb.WriteString(";\n\n") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's confirm the current code at the specified lines
sed -n '200,214p' packages/go/schemagen/generator/sql.goRepository: SpecterOps/BloodHound
Length of output: 724
🏁 Script executed:
# Check git history for the commit mentioned in the review
git log --oneline | grep -i "91c5b4a" || git log --oneline | head -20Repository: SpecterOps/BloodHound
Length of output: 107
🏁 Script executed:
# Search for any comments or context around is_display_kind in the file
rg "is_display_kind" packages/go/schemagen/generator/sql.go -B 3 -A 3Repository: SpecterOps/BloodHound
Length of output: 526
🏁 Script executed:
# Look at the broader context of the INSERT statement and nodeKinds/nodeIcons initialization
sed -n '180,220p' packages/go/schemagen/generator/sql.goRepository: SpecterOps/BloodHound
Length of output: 1657
🏁 Script executed:
# Check if there are any other references to nodeIcons to understand how it's populated
rg "nodeIcons\[" packages/go/schemagen/generator/sql.go -B 5 -A 2Repository: SpecterOps/BloodHound
Length of output: 705
🏁 Script executed:
# Find the function signature and where nodeKinds/nodeIcons come from
rg "func.*nodeKinds|func.*nodeIcons" packages/go/schemagen/generator/sql.go -B 2 -A 10Repository: SpecterOps/BloodHound
Length of output: 612
🏁 Script executed:
# Look for the function that contains this code to see parameters
sed -n '1,220p' packages/go/schemagen/generator/sql.go | grep -n "func\|nodeKinds\|nodeIcons" | head -20Repository: SpecterOps/BloodHound
Length of output: 693
🏁 Script executed:
# Get more context - find the function definition
sed -n '100,210p' packages/go/schemagen/generator/sql.goRepository: SpecterOps/BloodHound
Length of output: 3393
🏁 Script executed:
# Search for where nodeIcons is populated/created
rg "nodeIcons\s*:=|nodeIcons\s*=" packages/go/schemagen/generator/sql.go -B 5 -A 5Repository: SpecterOps/BloodHound
Length of output: 242
🏁 Script executed:
# Check if there are any comments about is_display_kind behavior
rg "is_display_kind|isDisplayKind" packages/go/schemagen/ -rRepository: SpecterOps/BloodHound
Length of output: 478
🏁 Script executed:
# Find where nodeKinds are defined (ActiveDirectory, Azure schemas)
fd "model\|schema" packages/go/ --type f | head -20Repository: SpecterOps/BloodHound
Length of output: 47
🏁 Script executed:
# Search for ActiveDirectory and Azure model definitions
rg "type ActiveDirectory|type Azure" packages/go/ -A 5Repository: SpecterOps/BloodHound
Length of output: 1023
🏁 Script executed:
# Get all keys from nodeIcons map to see coverage
rg 'nodeIcons = map' packages/go/schemagen/generator/sql.go -A 200 | grep '":' | cut -d'"' -f2 | sortRepository: SpecterOps/BloodHound
Length of output: 408
🏁 Script executed:
# Check all node types defined in Active Directory schema
rg "NodeKinds\|RelationshipKinds" packages/go/ -B 2 -A 2Repository: SpecterOps/BloodHound
Length of output: 47
🏁 Script executed:
# Look for database schema definitions to understand is_display_kind semantics
fd "\.sql|schema" packages/ --type fRepository: SpecterOps/BloodHound
Length of output: 450
🏁 Script executed:
# Find where the actual nodeKinds data is populated (likely from cue files or generated code)
fd "\.cue" packages/ | head -20Repository: SpecterOps/BloodHound
Length of output: 280
🏁 Script executed:
# Check the graphschema to understand what nodeKinds are expected
head -100 packages/go/graphschema/schema.goRepository: SpecterOps/BloodHound
Length of output: 2781
🏁 Script executed:
# Look for any documentation or comments about is_display_kind in database schema
cat packages/go/openapi/src/schemas/model.graph-schema-edge-kinds.yamlRepository: SpecterOps/BloodHound
Length of output: 861
🏁 Script executed:
# Search for is_display_kind in the codebase to understand its purpose
rg "is_display_kind" packages/ -B 3 -A 3Repository: SpecterOps/BloodHound
Length of output: 799
🏁 Script executed:
# Check if there's a migration or database schema file that documents is_display_kind
fd "migration\|schema.*sql" . --type f | head -20Repository: SpecterOps/BloodHound
Length of output: 47
🏁 Script executed:
# Look at the AD and Azure cue files to see all defined nodeKinds
cat packages/cue/bh/ad/ad.cue | grep -A 50 "nodeKinds\|NodeKinds"Repository: SpecterOps/BloodHound
Length of output: 2131
🏁 Script executed:
# Also check Azure cue file
cat packages/cue/bh/azure/azure.cue | grep -A 50 "nodeKinds\|NodeKinds"Repository: SpecterOps/BloodHound
Length of output: 2426
🏁 Script executed:
# Check if there are any comments or documentation in the cue files about display policy
cat packages/cue/bh/types/graph.cue | head -100Repository: SpecterOps/BloodHound
Length of output: 1160
🏁 Script executed:
# Look for any migration files or schema definitions that document is_display_kind
find . -name "*.sql" -type f 2>/dev/null | xargs grep -l "is_display_kind" 2>/dev/null | head -5Repository: SpecterOps/BloodHound
Length of output: 247
🏁 Script executed:
# Check git history specifically for changes to the is_display_kind logic
git log --oneline --all -- packages/go/schemagen/generator/sql.go | head -20Repository: SpecterOps/BloodHound
Length of output: 107
🏁 Script executed:
# Check the migration file to see how is_display_kind is documented
grep -A 10 -B 10 "is_display_kind" ./cmd/api/src/database/migration/migrations/v8.5.0.sql | head -40Repository: SpecterOps/BloodHound
Length of output: 1253
🏁 Script executed:
# Check one of the generated schema files to see actual values inserted
head -150 ./cmd/api/src/database/migration/extensions/ad_graph_schema.sql | tail -80Repository: SpecterOps/BloodHound
Length of output: 2936
🏁 Script executed:
# Get all nodeKinds from AD cue file and cross-check with nodeIcons
python3 << 'EOF'
# AD NodeKinds from cue output
ad_node_kinds = [
"Entity", "User", "Computer", "Group", "GPO", "OU", "Container", "Domain",
"LocalGroup", "LocalUser", "AIACA", "RootCA", "EnterpriseCA", "NTAuthStore",
"CertTemplate", "IssuancePolicy"
]
# Azure NodeKinds from cue output
az_node_kinds = [
"Entity", "VMScaleSet", "App", "Role", "Device", "FunctionApp", "Group", "KeyVault",
"ManagementGroup", "ResourceGroup", "ServicePrincipal", "Subscription", "Tenant",
"User", "VM", "ManagedCluster", "ContainerRegistry", "WebApp", "LogicApp", "AutomationAccount"
]
# nodeIcons keys from previous output
node_icons_keys = [
"AIACA", "AZApp", "AZAutomationAccount", "AZContainerRegistry", "AZDevice",
"AZFunctionApp", "AZGroup", "AZKeyVault", "AZLogicApp", "AZManagedCluster",
"AZManagementGroup", "AZResourceGroup", "AZRole", "AZServicePrincipal", "AZSubscription",
"AZTenant", "AZUser", "AZVM", "AZVMScaleSet", "AZWebApp", "CertTemplate", "Computer",
"Container", "Domain", "EnterpriseCA", "GPO", "Group", "IssuancePolicy", "NTAuthStore",
"OU", "RootCA", "User"
]
# Check AD coverage
print("=== AD NodeKinds Coverage ===")
for kind in ad_node_kinds:
if kind in node_icons_keys:
print(f"✓ {kind} has icon")
else:
print(f"✗ {kind} MISSING icon")
print("\n=== Azure NodeKinds Coverage ===")
for kind in az_node_kinds:
# Azure kinds are prefixed with AZ in nodeIcons
az_key = "AZ" + kind if kind != "Entity" else kind
if az_key in node_icons_keys or kind in node_icons_keys:
print(f"✓ {kind} has icon (as {az_key})")
else:
print(f"✗ {kind} MISSING icon (looking for {az_key})")
EOFRepository: SpecterOps/BloodHound
Length of output: 151
🏁 Script executed:
# Check if the generated SQL files are actually used or if they're examples
head -5 ./cmd/api/src/database/migration/extensions/ad_graph_schema.sqlRepository: SpecterOps/BloodHound
Length of output: 264
🏁 Script executed:
# Look at git history of the generated SQL files to see if they're manually maintained or auto-generated
git log --oneline -20 -- ./cmd/api/src/database/migration/extensions/Repository: SpecterOps/BloodHound
Length of output: 107
🏁 Script executed:
# Check if there's a build process or script that generates these files
find . -name "*.sh" -o -name "Makefile" -o -name "*.mk" | xargs grep -l "schemagen\|schema.*sql" 2>/dev/nullRepository: SpecterOps/BloodHound
Length of output: 49
🏁 Script executed:
# Compare the current sql.go logic with what's actually in the generated files
echo "=== Checking LocalGroup and LocalUser in ad_graph_schema.sql ==="
grep -E "LocalGroup|LocalUser" ./cmd/api/src/database/migration/extensions/ad_graph_schema.sqlRepository: SpecterOps/BloodHound
Length of output: 386
🏁 Script executed:
# Check Azure schema file as well
grep -E "AZGroup|AZUser" ./cmd/api/src/database/migration/extensions/az_graph_schema.sql | head -10Repository: SpecterOps/BloodHound
Length of output: 331
🏁 Script executed:
# Verify the pattern - check all AD nodeKinds and their display status in generated SQL
echo "=== AD Node Kinds Display Status ===" && \
grep "new_extension_id, '[A-Z]" ./cmd/api/src/database/migration/extensions/ad_graph_schema.sql | \
grep "schema_node_kinds" | \
sed "s/.*new_extension_id, '//" | sed "s/',.*//" | sort | uniqRepository: SpecterOps/BloodHound
Length of output: 101
🏁 Script executed:
# Extract is_display_kind values for each
echo "=== AD Node Kinds with Display Status ===" && \
grep "new_extension_id, '[A-Z]" ./cmd/api/src/database/migration/extensions/ad_graph_schema.sql | \
grep "schema_node_kinds" | \
sed "s/.*('\([^']*\)'.*'', \(true\|false\).*/\1: \2/" | sortRepository: SpecterOps/BloodHound
Length of output: 106
🏁 Script executed:
# Check Azure schema
echo "=== Azure Node Kinds with Display Status ===" && \
grep "new_extension_id, '[A-Z]" ./cmd/api/src/database/migration/extensions/az_graph_schema.sql | \
grep "schema_node_kinds" | \
sed "s/.*('\([^']*\)'.*'', \(true\|false\).*/\1: \2/" | sortRepository: SpecterOps/BloodHound
Length of output: 109
🏁 Script executed:
# Verify what the review comment claims - that this was supposedly fixed in 91c5b4a
git log --all --oneline --grep="is_display_kind\|display_kind" | head -10Repository: SpecterOps/BloodHound
Length of output: 47
🏁 Script executed:
# Let's read the files directly and look at the actual schema_node_kinds inserts
echo "=== AD schema_node_kinds inserts ===" && \
sed -n '/INSERT INTO schema_node_kinds/,/;/p' ./cmd/api/src/database/migration/extensions/ad_graph_schema.sql | head -30Repository: SpecterOps/BloodHound
Length of output: 1479
🏁 Script executed:
# Azure schema_node_kinds inserts
echo "=== Azure schema_node_kinds inserts ===" && \
sed -n '/INSERT INTO schema_node_kinds/,/;/p' ./cmd/api/src/database/migration/extensions/az_graph_schema.sql | head -30Repository: SpecterOps/BloodHound
Length of output: 1911
🏁 Script executed:
# Compare: check if LocalGroup/LocalUser (which have no icons) are marked false
grep -A 1 "LocalGroup\|LocalUser" ./cmd/api/src/database/migration/extensions/ad_graph_schema.sql | grep "false\|true"Repository: SpecterOps/BloodHound
Length of output: 416
🏁 Script executed:
# And check which ones have icons (like User, Computer, Group)
grep -E "User|Computer|Group" ./cmd/api/src/database/migration/extensions/ad_graph_schema.sql | grep "new_extension_id" | head -10Repository: SpecterOps/BloodHound
Length of output: 692
🏁 Script executed:
# Check the first lines of both migration files to see if they indicate auto-generation
head -20 ./cmd/api/src/database/migration/extensions/ad_graph_schema.sqlRepository: SpecterOps/BloodHound
Length of output: 874
🏁 Script executed:
# Check if the files have been manually edited by looking at recent commits
git log --oneline -1 -- ./cmd/api/src/database/migration/extensions/ad_graph_schema.sql ./cmd/api/src/database/migration/extensions/az_graph_schema.sqlRepository: SpecterOps/BloodHound
Length of output: 107
🏁 Script executed:
# See if there's any documentation about how these files are generated/maintained
find . -name "README*" -o -name "*.md" | xargs grep -l "schema.*sql\|schemagen" 2>/dev/null | head -5Repository: SpecterOps/BloodHound
Length of output: 98
🏁 Script executed:
# Verify: if the current sql.go code were run, would it produce the same output as the migration files?
# Let's check if the nodeIcons entries match the is_display_kind=true entries in AD schema
echo "=== Checking if is_display_kind=true entries have icons ===" && \
grep "true, 'fa-" ./cmd/api/src/database/migration/extensions/ad_graph_schema.sql | wc -l && \
echo "icon entries in nodeIcons for AD:" && \
rg "\"(User|Computer|Group|GPO|OU|Container|Domain|AIACA|RootCA|EnterpriseCA|NTAuthStore|CertTemplate|IssuancePolicy)\":" packages/go/schemagen/generator/sql.go | wc -lRepository: SpecterOps/BloodHound
Length of output: 164
🏁 Script executed:
# Read the schemagen README to understand how this is supposed to work
cat ./packages/go/schemagen/README.mdRepository: SpecterOps/BloodHound
Length of output: 1045
🏁 Script executed:
# Check if there's any documentation about is_display_kind semantics
grep -r "is_display_kind\|display.*kind" ./packages/go/schemagen/ -A 2 -B 2Repository: SpecterOps/BloodHound
Length of output: 661
🏁 Script executed:
# Verify the current behavior is consistent: check if all nodeIcons entries appear in the AD schema with is_display_kind=true
echo "=== Checking all nodeIcons entries in AD schema ===" && \
python3 << 'EOF'
import re
# Get nodeIcons keys from the sql.go file
with open('packages/go/schemagen/generator/sql.go', 'r') as f:
content = f.read()
# Extract all icon keys from nodeIcons map
icons = re.findall(r'"([^"]+)":\s*{[\s\S]*?Icon:\s*"fa-', content)
# Get unique AD-related icons (non-AZ prefix)
ad_icons = [i for i in icons if not i.startswith('AZ')]
# Check if they appear in ad_graph_schema.sql with is_display_kind=true
with open('./cmd/api/src/database/migration/extensions/ad_graph_schema.sql', 'r') as f:
ad_sql = f.read()
print("AD Icons from nodeIcons map:")
for icon in sorted(set(ad_icons)):
# Check if it appears with true value
pattern = f"'{icon}'.*true"
if re.search(pattern, ad_sql):
print(f" ✓ {icon} -> is_display_kind=true")
else:
print(f" ✗ {icon} NOT FOUND with true in SQL")
print("\nAD Nodes with is_display_kind=false (no icons):")
false_entries = re.findall(r"'([^']+)'.*false", ad_sql)
for entry in sorted(set(false_entries))[:10]:
print(f" - {entry}")
EOFRepository: SpecterOps/BloodHound
Length of output: 1069
🏁 Script executed:
# Let me verify the pattern more clearly - check what nodeKinds without icons look like
echo "=== NodeKinds without icons in AD (is_display_kind=false) ===" && \
grep "false, '', ''" ./cmd/api/src/database/migration/extensions/ad_graph_schema.sql | grep "new_extension_id"Repository: SpecterOps/BloodHound
Length of output: 325
🏁 Script executed:
# And verify if this is truly the design - check if any nodes in the schema are intentionally not meant to be displayed
echo "=== Checking schema comments or documentation about LocalGroup/LocalUser ===" && \
rg "LocalGroup|LocalUser" packages/cue/bh/ -A 2 -B 2Repository: SpecterOps/BloodHound
Length of output: 1487
🏁 Script executed:
# Final check: confirm whether the generated files match what the current sql.go code would produce
echo "=== Verify logic: all nodes with icons = is_display_kind true ===" && \
echo "Nodes with icons in nodeIcons AND is_display_kind=true in SQL:" && \
rg "true, 'fa-" ./cmd/api/src/database/migration/extensions/ad_graph_schema.sql | wc -l && \
echo "Nodes without icons AND is_display_kind=false in SQL:" && \
rg "false, '', ''" ./cmd/api/src/database/migration/extensions/ad_graph_schema.sql | wc -lRepository: SpecterOps/BloodHound
Length of output: 252
Review the semantic coupling between is_display_kind and icon availability - the design is intentional but implicit.
Lines 204 and 206 use the found boolean (whether an icon exists in the nodeIcons map) directly as the value for is_display_kind. This coupling is currently intentional: nodes without icons (Base, ADLocalGroup, ADLocalUser) are explicitly excluded from display. However, this design is implicit rather than explicit.
Current behavior (confirmed in generated SQL):
- Nodes WITH icons →
is_display_kind=true(13 AD nodes) - Nodes WITHOUT icons →
is_display_kind=false(3 AD nodes: Base, ADLocalGroup, ADLocalUser)
The concern remains valid: This implicit coupling creates a maintenance risk. If a new node kind is added to the schema but no icon is added to nodeIcons, it will silently become non-displayable. The semantic intent ("this node should not be displayed in the UI") is not explicit in the schema.
Consider:
- Document why Base, ADLocalGroup, and ADLocalUser should not be displayed (e.g., internal/structural types)
- For future node types, make the display policy explicit in the schema definition rather than relying on icon map membership
Note: This pattern was not previously marked as addressed in an earlier commit; the current behavior reflects the ongoing design approach.
🤖 Prompt for AI Agents
In @packages/go/schemagen/generator/sql.go around lines 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.
There was a problem hiding this comment.
Yes ideally this would be defined in the cue files but this is all temporary and not intended to be around long term...I think it's OK as-is
There was a problem hiding this comment.
@AD7ZJ, understood! Thanks for the clarification about the temporary nature of this code. If it's not intended to be long-term, the current implementation makes sense. I appreciate you noting the ideal approach for future reference.
/>\_/\
( o.o )
> ^ <
/| |\
(_| |_)
Thoughtful rabbit
🧠 Learnings used
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.
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.
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
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.