feat: OpenGraph - Add extension schema migrations BED-6751 #2105
Conversation
WalkthroughRemoves the public Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
cweidenkeller
force-pushed
the
BED-6751
branch
from
baf5c9d to
7e9f168
Compare
cweidenkeller
changed the title
cweidenkeller
force-pushed
the
BED-6751
branch
2 times, most recently
from
d49eb16 to
c9deab2
Compare
cweidenkeller
force-pushed
the
BED-6751
branch
from
c9deab2 to
9c8cf7d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
134-137: Consider adding CASCADE DELETE to schema_extension_environments.The
schema_extension_environmentstable references bothschema_extensions(id)andschema_environments(id), with cascade delete only onschema_extensions(line 135). If a schema environment is deleted, orphaned records will remain. Consider addingON DELETE CASCADEto theschema_environment_idforeign key (line 136) for consistency with the referential integrity pattern used elsewhere in this migration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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
🔇 Additional comments (5)
cmd/api/src/database/migration/migrations/v8.5.0.sql (5)
92-98: Verify foreign key dependency: kind table.The
schema_environmentstable referenceskind(id)for bothenvironment_kind_idandsource_kind_id(lines 94–95). Confirm that thekindtable exists in the database schema and has a primary key onid.
101-110: Verify foreign key dependency: kind table and consider cascade delete semantics.The
schema_relationship_findingstable referenceskind(id)at line 104 andschema_extensions(id)at line 103. Confirm that thekindtable exists and is properly indexed. The cascade delete onschema_extensionsis appropriate, ensuring findings are cleaned up when an extension is removed.
125-130: Verify foreign key dependency: kind table.The
schema_environments_principal_kindstable referenceskind(id)at line 128. Confirm that thekindtable exists in the database schema.
76-89: No action required — schema_edge_kinds is a new table, not a modification of an existing one.The
schema_edge_kindstable is created for the first time in v8.5.0.sql. Theis_display_kindcolumn referenced in the review exists in a separatekindtable, not inschema_edge_kinds. Theis_traversablecolumn is the appropriate and intentional column for this new table and does not replace or remove any prior column.Likely an incorrect or invalid review comment.
30-40: No inconsistency found: schema_extensions table is correctly present and being created.The v8.5.0.sql migration file creates the schema_extensions table at lines 30-40 as part of the OpenGraph extension feature. The table is referenced throughout the migration via foreign keys from schema_node_kinds, schema_properties, schema_edge_kinds, schema_relationship_findings, and schema_extension_environments—all indicating a new feature addition, not a removal. No DROP TABLE or removal statement for schema_extensions exists in the migration. The PR objective explicitly states this adds "extension schema migrations for custom findings in OpenGraph."
Likely an incorrect or invalid review comment.
cweidenkeller
changed the title
cweidenkeller
changed the title
cweidenkeller
changed the title
cweidenkeller
changed the title
cweidenkeller
force-pushed
the
BED-6751
branch
from
9c8cf7d to
d143d43
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
cmd/api/src/database/migration/migrations/v8.5.0.sql (2)
125-130: Standardize foreign key column naming conventions.Junction table column names are inconsistent:
schema_environments_principal_kindsusesenvironment(line 127) whileschema_extension_environmentsusesschema_environment_id(line 136). Additionally,environmentshould follow the_idsuffix convention used inenvironment_kind_idandsource_kind_idelsewhere in the schema.Rename for consistency:
CREATE TABLE IF NOT EXISTS schema_environments_principal_kinds ( id SERIAL PRIMARY KEY, - environment INTEGER NOT NULL REFERENCES schema_environments(id) ON DELETE CASCADE, + environment_id INTEGER NOT NULL REFERENCES schema_environments(id) ON DELETE CASCADE, principal_kind INTEGER NOT NULL REFERENCES kind(id), - UNIQUE(environment,principal_kind) + UNIQUE(environment_id,principal_kind) );Consider renaming
schema_environment_idtoenvironment_idinschema_extension_environmentsfor brevity and consistency.Also applies to: 134-137
101-110: Add indexes on foreign key columns for query performance.New tables with foreign keys lack indexes on those columns, which could degrade query performance on common join patterns (e.g., finding results by extension, environment, or remediation lookups).
Add indexes after table creation:
CREATE TABLE IF NOT EXISTS schema_relationship_findings ( id SERIAL PRIMARY KEY, extension_id INTEGER NOT NULL REFERENCES schema_extensions (id) ON DELETE CASCADE, relationship_kind_id INTEGER NOT NULL REFERENCES kind(id), environment_id INTEGER NOT NULL REFERENCES schema_environments (id) ON DELETE CASCADE, name TEXT NOT NULL, display_name TEXT NOT NULL, created_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP, UNIQUE(name) ); +CREATE INDEX idx_schema_relationship_findings_extension_id ON schema_relationship_findings(extension_id); +CREATE INDEX idx_schema_relationship_findings_environment_id ON schema_relationship_findings(environment_id); + -- Remediation content table with FK to findings CREATE TABLE IF NOT EXISTS schema_remediations ( id SERIAL PRIMARY KEY, finding_id INTEGER NOT NULL REFERENCES schema_relationship_findings(id) ON DELETE CASCADE, short_description TEXT, long_description TEXT, short_remediation TEXT, long_remediation TEXT, CONSTRAINT unique_finding_remediation UNIQUE (finding_id) ); +CREATE INDEX idx_schema_remediations_finding_id ON schema_remediations(finding_id);Also applies to: 113-123
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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
⏰ 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
cweidenkeller
force-pushed
the
BED-6751
branch
from
d143d43 to
1463e56
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
30-40: Remove the duplicateschema_extensionstable definition.Lines 30–40 and lines 59–70 both define the
schema_extensionstable identically. The duplication will cause the migration to fail or create unexpected behavior. Remove one of the definitions.-- OpenGraph graph schema - extensions (collectors) CREATE TABLE IF NOT EXISTS schema_extensions ( id SERIAL NOT NULL, name TEXT UNIQUE NOT NULL, display_name TEXT NOT NULL, version TEXT NOT NULL, is_builtin BOOLEAN DEFAULT FALSE, created_at TIMESTAMP WITH TIME ZONE DEFAULT current_timestamp, updated_at TIMESTAMP WITH TIME ZONE DEFAULT current_timestamp, deleted_at TIMESTAMP WITH TIME ZONE DEFAULT NULL, PRIMARY KEY (id) ); - --- OpenGraph graph schema - extensions (collectors) -CREATE TABLE IF NOT EXISTS schema_extensions ( - id SERIAL NOT NULL, - name TEXT UNIQUE NOT NULL, - display_name TEXT NOT NULL, - version TEXT NOT NULL, - is_builtin BOOLEAN DEFAULT FALSE, - created_at TIMESTAMP WITH TIME ZONE DEFAULT current_timestamp, - updated_at TIMESTAMP WITH TIME ZONE DEFAULT current_timestamp, - deleted_at TIMESTAMP WITH TIME ZONE DEFAULT NULL, - PRIMARY KEY (id) -);Also applies to: 59-70
♻️ Duplicate comments (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
145-148: Add a composite PRIMARY KEY toschema_extension_environmentsto prevent duplicate pairs.This junction table lacks a PRIMARY KEY or composite uniqueness constraint, allowing duplicate
(extension_id, schema_environment_id)pairs. A prior review flagged this; it remains unfixed.CREATE TABLE IF NOT EXISTS schema_extension_environments ( extension_id INTEGER NOT NULL REFERENCES schema_extensions(id) ON DELETE CASCADE, - schema_environment_id INTEGER NOT NULL REFERENCES schema_environments(id) + schema_environment_id INTEGER NOT NULL REFERENCES schema_environments(id), + PRIMARY KEY (extension_id, schema_environment_id) );
🧹 Nitpick comments (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
119-119: Use lowercasecurrent_timestampfor consistency with the codebase.Line 119 uses
CURRENT_TIMESTAMP(uppercase), while all other timestamp defaults in this file usecurrent_timestamp(lowercase). Based on learnings, the lowercase form is the established pattern across BloodHound migrations.- created_at TIMESTAMP WITH TIME ZONE DEFAULT CURRENT_TIMESTAMP, + created_at TIMESTAMP WITH TIME ZONE DEFAULT current_timestamp,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-30T15:01:15.324Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:01:15.324Z
Learning: BloodHound migration files consistently use `current_timestamp` for both `created_at` and `updated_at` fields in INSERT statements across all migration versions. This is the established pattern that should be maintained for consistency across the codebase.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-07-30T15:02:05.134Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:02:05.134Z
Learning: BloodHound migrations consistently use `current_timestamp` for created_at and updated_at fields across all tables (feature_flags, permissions, roles, parameters, etc.). This is the established pattern from schema.sql through all version migrations (v6.1.0+).
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
⏰ 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 (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
115-115: Thekindtable reference is valid and no issue exists.The
kindtable is created in v7.3.0.sql (line 21) before this migration file (v8.5.0.sql) runs, so the foreign key references tokind(id)on lines 115 and 139 are correct and will maintain referential integrity. This table is distinct fromschema_node_kindsand is used specifically for relationship and principal kind classifications.Likely an incorrect or invalid review comment.
cweidenkeller
force-pushed
the
BED-6751
branch
from
1463e56 to
f5d70e9
Compare
cweidenkeller
force-pushed
the
BED-6751
branch
2 times, most recently
from
1b85435 to
606f48f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
30-40: Remove duplicateschema_extensionstable definition.The
schema_extensionstable is defined twice in this migration file (lines 30–40 and lines 60–70). AlthoughIF NOT EXISTSprevents a runtime error, the duplicate creates confusion and should be removed.Delete the second definition (lines 60–70) and keep the first.
Also applies to: 60-70
🧹 Nitpick comments (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
103-142: Consider adding indexes on foreign key columns for query performance.The new tables define foreign keys to
schema_extensions,schema_environments, andkindtables, but lack indexes on these columns. Query performance on joins and lookups may benefit from indexes onextension_id,environment_id, and relationship foreign keys, especially as data grows.Add indexes after each table creation:
CREATE INDEX idx_schema_environments_extension_id ON schema_environments(extension_id); CREATE INDEX idx_schema_relationship_findings_extension_id ON schema_relationship_findings(extension_id); CREATE INDEX idx_schema_relationship_findings_environment_id ON schema_relationship_findings(environment_id); CREATE INDEX idx_schema_remediations_finding_id ON schema_remediations(finding_id); CREATE INDEX idx_schema_environments_principal_kinds_environment_id ON schema_environments_principal_kinds(environment_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-30T15:01:15.324Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:01:15.324Z
Learning: BloodHound migration files consistently use `current_timestamp` for both `created_at` and `updated_at` fields in INSERT statements across all migration versions. This is the established pattern that should be maintained for consistency across the codebase.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-07-30T15:02:05.134Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:02:05.134Z
Learning: BloodHound migrations consistently use `current_timestamp` for created_at and updated_at fields across all tables (feature_flags, permissions, roles, parameters, etc.). This is the established pattern from schema.sql through all version migrations (v6.1.0+).
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
103-142: Code structure aligns with past feedback.The table definitions correctly incorporate previous review feedback:
extension_idFK added toschema_environments,UNIQUE(principal_kind)constraint in place, proper references to thekindtable (not schema-specific kind tables), and removal of the junction table. Past comments are properly resolved.
cweidenkeller
force-pushed
the
BED-6751
branch
2 times, most recently
from
3744f57 to
8f27b6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-07-30T15:01:15.324Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:01:15.324Z
Learning: BloodHound migration files consistently use `current_timestamp` for both `created_at` and `updated_at` fields in INSERT statements across all migration versions. This is the established pattern that should be maintained for consistency across the codebase.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-07-30T15:02:05.134Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:02:05.134Z
Learning: BloodHound migrations consistently use `current_timestamp` for created_at and updated_at fields across all tables (feature_flags, permissions, roles, parameters, etc.). This is the established pattern from schema.sql through all version migrations (v6.1.0+).
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
⏰ 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 (3)
cmd/api/src/database/migration/migrations/v8.5.0.sql (3)
113-123: Schema looks good forschema_relationship_findings.Foreign keys, constraints, and timestamp defaults are correctly defined. The use of lowercase
current_timestampaligns with established BloodHound migration patterns.
126-136: Schema looks good forschema_remediations.The one-to-one relationship via
UNIQUE(finding_id)is appropriately enforced, and theON DELETE CASCADErule maintains referential integrity correctly.
138-144: Schema looks good forschema_environments_principal_kinds.The composite structure,
ON DELETE CASCADE, and uniqueness constraint are correct. The table properly links environments to principal kinds.
cweidenkeller
force-pushed
the
BED-6751
branch
3 times, most recently
from
b70910d to
6aef693
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-30T15:01:15.324Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:01:15.324Z
Learning: BloodHound migration files consistently use `current_timestamp` for both `created_at` and `updated_at` fields in INSERT statements across all migration versions. This is the established pattern that should be maintained for consistency across the codebase.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-07-30T15:02:05.134Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:02:05.134Z
Learning: BloodHound migrations consistently use `current_timestamp` for created_at and updated_at fields across all tables (feature_flags, permissions, roles, parameters, etc.). This is the established pattern from schema.sql through all version migrations (v6.1.0+).
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
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
cweidenkeller
force-pushed
the
BED-6751
branch
2 times, most recently
from
c2c60df to
7391ec1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql (1)
91-98: Critical: AddON DELETE CASCADEtoextension_idforeign key inschema_environments.Line 93 is missing the
ON DELETE CASCADEconstraint on theextension_idforeign key, which is inconsistent with other extension-referencing tables throughout this migration (lines 103, 116, 128). Without cascading deletes, orphaned environment records will remain if their extension is deleted, breaking referential integrity.This was flagged in a previous review cycle but appears unresolved in the current code.
Apply this diff to add cascading delete semantics:
CREATE TABLE IF NOT EXISTS schema_environments ( id SERIAL, - extension_id INTEGER NOT NULL REFERENCES schema_extensions(id), + extension_id INTEGER NOT NULL REFERENCES schema_extensions(id) ON DELETE CASCADE, environment_kind_id INTEGER NOT NULL REFERENCES kind(id), source_kind_id INTEGER NOT NULL REFERENCES kind(id), PRIMARY KEY (id), UNIQUE(environment_kind_id,source_kind_id) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-30T15:01:15.324Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:01:15.324Z
Learning: BloodHound migration files consistently use `current_timestamp` for both `created_at` and `updated_at` fields in INSERT statements across all migration versions. This is the established pattern that should be maintained for consistency across the codebase.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-07-30T15:02:05.134Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:02:05.134Z
Learning: BloodHound migrations consistently use `current_timestamp` for created_at and updated_at fields across all tables (feature_flags, permissions, roles, parameters, etc.). This is the established pattern from schema.sql through all version migrations (v6.1.0+).
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
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
⏰ 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
cweidenkeller
force-pushed
the
BED-6751
branch
from
ec14e20 to
bda1c85
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cmd/api/src/database/migration/migrations/v8.5.0.sql (4)
88-98: Add index onschema_extension_idfor consistency with established pattern.The new
schema_environmentstable stores extension-related data but lacks an index onschema_extension_id, while existing similar tables (schema_node_kindsline 57,schema_propertiesline 74,schema_edge_kindsline 88) all define indexes on this column. This inconsistency may impact query performance for extension lookups.Apply this diff to add the index:
CREATE TABLE IF NOT EXISTS schema_environments ( id SERIAL, schema_extension_id INTEGER NOT NULL REFERENCES schema_extensions(id) ON DELETE CASCADE, environment_kind_id INTEGER NOT NULL REFERENCES kind(id), source_kind_id INTEGER NOT NULL REFERENCES kind(id), PRIMARY KEY (id), UNIQUE(environment_kind_id,source_kind_id) ); + +CREATE INDEX idx_schema_environments_extensions_id ON schema_environments (schema_extension_id);
101-111: Add indexes on frequently queried foreign keys inschema_relationship_findings.This table references
schema_extensions,kind, andschema_environments, but has no indexes. Given that queries will likely filter by extension or environment to retrieve findings, indexes on these columns would improve query performance, consistent with the pattern established elsewhere in this file.Apply this diff to add indexes:
CREATE TABLE IF NOT EXISTS schema_relationship_findings ( id SERIAL, schema_extension_id INTEGER NOT NULL REFERENCES schema_extensions(id) ON DELETE CASCADE, relationship_kind_id INTEGER NOT NULL REFERENCES kind(id), environment_id INTEGER NOT NULL REFERENCES schema_environments(id) ON DELETE CASCADE, name TEXT NOT NULL, display_name TEXT NOT NULL, created_at TIMESTAMP WITH TIME ZONE DEFAULT current_timestamp, PRIMARY KEY(id), UNIQUE(name) ); + +CREATE INDEX idx_schema_relationship_findings_extensions_id ON schema_relationship_findings (schema_extension_id); +CREATE INDEX idx_schema_relationship_findings_environments_id ON schema_relationship_findings (environment_id);
114-123: Consider adding index onfinding_idinschema_remediations.This table's primary relationship is to
schema_relationship_findingsviafinding_id. An index on this column would improve query performance when retrieving remediations for a given finding, following standard relational database design practices.Apply this diff to add the index:
CREATE TABLE IF NOT EXISTS schema_remediations ( id SERIAL, finding_id INTEGER NOT NULL REFERENCES schema_relationship_findings(id) ON DELETE CASCADE, short_description TEXT, long_description TEXT, short_remediation TEXT, long_remediation TEXT, PRIMARY KEY(id), UNIQUE(finding_id) ); + +CREATE INDEX idx_schema_remediations_finding_id ON schema_remediations (finding_id);
126-132: Consider adding index onenvironment_idinschema_environments_principal_kinds.This junction table maps environments to principal kinds. An index on
environment_idwould improve query performance when retrieving all principal kinds for a given environment.Apply this diff to add the index:
CREATE TABLE IF NOT EXISTS schema_environments_principal_kinds ( id SERIAL, environment_id INTEGER NOT NULL REFERENCES schema_environments(id) ON DELETE CASCADE, principal_kind INTEGER NOT NULL REFERENCES kind(id), PRIMARY KEY(id), UNIQUE(principal_kind) ); + +CREATE INDEX idx_schema_environments_principal_kinds_environment_id ON schema_environments_principal_kinds (environment_id);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-30T15:01:15.324Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:01:15.324Z
Learning: BloodHound migration files consistently use `current_timestamp` for both `created_at` and `updated_at` fields in INSERT statements across all migration versions. This is the established pattern that should be maintained for consistency across the codebase.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-07-30T15:02:05.134Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:02:05.134Z
Learning: BloodHound migrations consistently use `current_timestamp` for created_at and updated_at fields across all tables (feature_flags, permissions, roles, parameters, etc.). This is the established pattern from schema.sql through all version migrations (v6.1.0+).
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
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
⏰ 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
cweidenkeller
force-pushed
the
BED-6751
branch
2 times, most recently
from
95172d7 to
f4148f9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-30T15:01:15.324Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:01:15.324Z
Learning: BloodHound migration files consistently use `current_timestamp` for both `created_at` and `updated_at` fields in INSERT statements across all migration versions. This is the established pattern that should be maintained for consistency across the codebase.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-07-30T15:02:05.134Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:02:05.134Z
Learning: BloodHound migrations consistently use `current_timestamp` for created_at and updated_at fields across all tables (feature_flags, permissions, roles, parameters, etc.). This is the established pattern from schema.sql through all version migrations (v6.1.0+).
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
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
⏰ 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 (6)
cmd/api/src/database/migration/migrations/v8.5.0.sql (6)
17-26: LGTM!Correct use of lowercase
current_timestampand idempotent insertion pattern.
30-87: LGTM!Existing table definitions follow established BloodHound patterns: lowercase
current_timestamp, proper foreign key constraints withON DELETE CASCADE, and appropriate indexes.
91-102: LGTM!The composite
UNIQUE(environment_kind_id, source_kind_id)constraint properly enforces the data model, and theON DELETE CASCADEensures referential integrity when extensions are removed.
104-118: LGTM!The foreign key references are properly defined with
ON DELETE CASCADE, theUNIQUE(name)constraint ensures finding uniqueness, and indexes support query performance on extension and environment lookups.
128-133: LGTM!The normalized
schema_remediationstable with composite primary key(finding_id, content_type)cleanly separates remediation content by type, enabling flexible content management. TheON DELETE CASCADEonfinding_idmaintains referential integrity.
145-145: Add index onenvironment_idfor query performance.The index on line 145 supports lookups by
environment_id, which is helpful for queries retrieving all principal kinds for a given environment.
cweidenkeller
force-pushed
the
BED-6751
branch
from
f4148f9 to
45ebd25
Compare
cweidenkeller
force-pushed
the
BED-6751
branch
from
45ebd25 to
42404ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.5.0.sql(2 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-12-10T20:16:47.083Z
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:47.083Z
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-07-30T15:01:15.324Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:01:15.324Z
Learning: BloodHound migration files consistently use `current_timestamp` for both `created_at` and `updated_at` fields in INSERT statements across all migration versions. This is the established pattern that should be maintained for consistency across the codebase.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-07-30T15:02:05.134Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1735
File: cmd/api/src/database/migration/migrations/v8.1.0.sql:2-10
Timestamp: 2025-07-30T15:02:05.134Z
Learning: BloodHound migrations consistently use `current_timestamp` for created_at and updated_at fields across all tables (feature_flags, permissions, roles, parameters, etc.). This is the established pattern from schema.sql through all version migrations (v6.1.0+).
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-08-27T21:15:32.207Z
Learnt from: jvacca-specterops
Repo: SpecterOps/BloodHound PR: 1823
File: packages/javascript/bh-shared-ui/src/commonSearchesAGT.ts:184-187
Timestamp: 2025-08-27T21:15:32.207Z
Learning: In the BloodHound codebase, syntax fixes for invalid Cypher patterns (like `*..` → `*1..`) may be considered out of scope for specific PRs, even when flagged during review.
Applied to files:
cmd/api/src/database/migration/migrations/v8.5.0.sql
📚 Learning: 2025-09-09T19:19:06.998Z
Learnt from: mvlipka
Repo: SpecterOps/BloodHound PR: 1784
File: cmd/api/src/api/v2/auth/auth.go:377-383
Timestamp: 2025-09-09T19:19:06.998Z
Learning: In the BloodHound codebase, during user creation with ETAC (Environment Access Control), GORM automatically handles the UserID foreign key association when persisting the User model with its EnvironmentAccessControl association, eliminating the need to manually set UserID in the EnvironmentAccess records before persistence.
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
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-analysis
- GitHub Check: run-tests
- GitHub Check: build-ui
🔇 Additional comments (2)
cmd/api/src/database/migration/migrations/v8.5.0.sql (2)
104-136: Major inconsistency: AI-generated summary does not match actual code implementation.The AI summary describes
schema_remediationswith separate columns (short_description,long_description,short_remediation,long_remediation) andPRIMARY KEY(id), UNIQUE(finding_id), but the actual code uses a normalized structure with:
- Composite
PRIMARY KEY(finding_id, content_type)- Single
contentcolumnremediation_content_typeenum discriminator (lines 120-125)The implemented design is normalized and correct, but the summary is misleading for future reference.
131-131: STORAGE MAIN syntax is valid PostgreSQL.The
TEXT STORAGE MAINclause on line 131 is correct syntax for controlling TOAST storage strategy. This setting keeps values inline with compression enabled while avoiding out-of-row storage except as a last resort—appropriate for optimizing row scan performance for this column.
Description
We need new tables to handle an expansion to opengraph to allow for custom findings
Motivation and Context
Resolves BED-6751
We need new tables for extending opengraph
How Has This Been Tested?
Ran the migrations locally.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.