chore(OG): convert schema_relationship_findings to schema_findings BED-7508#2404
Open
chore(OG): convert schema_relationship_findings to schema_findings BED-7508#2404
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCreates a new schema_findings model (and schema_findings_subtypes), migrates data from schema_relationship_findings, updates remediations FK to reference the new table, drops legacy schema_list_findings and schema_relationship_findings, and updates Go code, mocks, and tests to use SchemaFinding (with a Type discriminator). Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/database/migration/migrations/v8.8.0.sql`:
- Around line 21-42: The migration inserts rows from
schema_relationship_findings into schema_findings but omits the legacy id, which
breaks FK references (schema_remediations.finding_id); modify the INSERT INTO
schema_findings to include the id column and SELECT the id from
schema_relationship_findings (i.e., INSERT INTO schema_findings (id, type,
schema_extension_id, kind_id, environment_id, name, display_name, created_at)
SELECT id, 1, schema_extension_id, relationship_kind_id, environment_id, name,
display_name, created_at FROM schema_relationship_findings ON CONFLICT (name) DO
NOTHING), and after the insert run a setval on the schema_findings id sequence
(setval('schema_findings_id_seq', (SELECT COALESCE(MAX(id),0) FROM
schema_findings))) so future inserts get correct next ids.
- Around line 36-58: The migration inserts rows into schema_findings from
schema_relationship_findings without preserving ids, which will break existing
schema_remediations.finding_id references; modify the migration so you either
(A) INSERT including the id column from schema_relationship_findings into
schema_findings and then reset the schema_findings id sequence (nextval) to
max(id)+1, or (B) after inserting (if not preserving ids) update
schema_remediations.finding_id to point at the new schema_findings ids by
joining on a stable business key (e.g., name + schema_extension_id +
relationship_kind_id) before adding the foreign key; apply the change around the
INSERT/select block involving schema_findings and the ALTER TABLE that adds
schema_remediations_schema_finding_id_fkey so existing finding_id values remain
valid.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.8.0.sql
kpowderly
reviewed
Feb 24, 2026
sirisjo
reviewed
Feb 25, 2026
sirisjo
reviewed
Feb 25, 2026
mistahj67
changed the title
mistahj67
force-pushed
the
refactor-rel-findings-to-findings
branch
from
4663c6c to
ccfeb36
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/api/src/database/migration/migrations/v8.8.0.sql (1)
56-61:⚠️ Potential issue | 🔴 CriticalRealign
schema_findingssequence after explicit-ID backfill.Line [56] now correctly preserves legacy IDs, but PostgreSQL won’t auto-advance the
SERIALsequence when IDs are inserted explicitly. Subsequent inserts can fail with duplicate key violations unless the sequence is reset after the backfill.Suggested fix
DO $$ BEGIN IF EXISTS (SELECT FROM information_schema.tables WHERE table_name = 'schema_relationship_findings') THEN INSERT INTO schema_findings (id, type, schema_extension_id, kind_id, environment_id, name, display_name, created_at) SELECT id, 1, schema_extension_id, relationship_kind_id, environment_id, name, display_name, created_at FROM schema_relationship_findings ON CONFLICT (name) DO NOTHING; + + PERFORM setval( + pg_get_serial_sequence('schema_findings', 'id'), + COALESCE((SELECT MAX(id) FROM schema_findings), 1), + true + ); END IF; END $$;#!/bin/bash # Verify explicit-ID insert exists and sequence realignment is missing. file="$(fd '^v8\.8\.0\.sql$' cmd/api/src/database/migration/migrations -a | head -n1)" nl -ba "$file" | sed -n '52,80p' rg -n "INSERT INTO schema_findings \\(id" "$file" rg -n "setval|pg_get_serial_sequence\\('schema_findings'" "$file"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/migration/migrations/v8.8.0.sql` around lines 56 - 61, After the explicit-ID backfill into schema_findings (the INSERT INTO schema_findings ... SELECT ... FROM schema_relationship_findings block) you need to realign the table's SERIAL sequence so future inserts don't hit duplicate keys: add a statement that computes the current max(id) from schema_findings and calls setval(pg_get_serial_sequence('schema_findings','id'), COALESCE(MAX(id), 1)) (or MAX(id) + 1 as appropriate) to advance the sequence. Place this immediately after the backfill block and before the END of the migration transaction so the schema_findings.id sequence is updated atomically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/api/src/database/migration/migrations/v8.8.0.sql`:
- Around line 56-61: After the explicit-ID backfill into schema_findings (the
INSERT INTO schema_findings ... SELECT ... FROM schema_relationship_findings
block) you need to realign the table's SERIAL sequence so future inserts don't
hit duplicate keys: add a statement that computes the current max(id) from
schema_findings and calls setval(pg_get_serial_sequence('schema_findings','id'),
COALESCE(MAX(id), 1)) (or MAX(id) + 1 as appropriate) to advance the sequence.
Place this immediately after the backfill block and before the END of the
migration transaction so the schema_findings.id sequence is updated atomically.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.8.0.sql
coderabbitai
bot
added
documentation
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/api/src/database/graphschema.go (1)
978-982:⚠️ Potential issue | 🔴 CriticalUpdate
GetRemediationByFindingNamequery to referenceschema_findingstable.The
schema_relationship_findingstable was dropped in the v8.8.0 migration after data was migrated toschema_findings. The JOIN on line 980 references the dropped table and will fail at runtime after the migration executes.Fix
FROM schema_remediations sr - JOIN schema_relationship_findings srf ON sr.finding_id = srf.id + JOIN schema_findings srf ON sr.finding_id = srf.id WHERE srf.name = ?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/graphschema.go` around lines 978 - 982, The SQL in GetRemediationByFindingName still JOINs schema_relationship_findings (alias srf) which was removed; update the query to JOIN schema_findings instead and keep the same alias (srf) so downstream column references (srf.id, srf.name, srf.display_name) still resolve; ensure the WHERE clause uses srf.name and the GROUP BY still groups by sr.finding_id and srf.display_name.cmd/api/src/database/graphschema_integration_test.go (1)
3525-3546:⚠️ Potential issue | 🟡 Minor
assertContainsFindingsdoes not validate theTypefield ofSchemaFinding.Since the whole purpose of this refactoring is to generalize findings beyond relationship-only (introducing a
Typefield), the assertion helper should verify that theTypeis correctly persisted. Currently, a bug that drops or misassigns the type would go undetected by these tests.Consider adding
finding.Type == want.Typeto the comparison and includingTypein theSchemaFindingstruct literals used in test cases.Proposed fix
assertContainsFindings := func(t *testing.T, got []model.SchemaFinding, expected ...model.SchemaFinding) { t.Helper() for _, want := range expected { found := false for _, finding := range got { if finding.SchemaExtensionId == want.SchemaExtensionId && finding.KindId == want.KindId && finding.EnvironmentId == want.EnvironmentId && finding.Name == want.Name && - finding.DisplayName == want.DisplayName { + finding.DisplayName == want.DisplayName && + finding.Type == want.Type {And update the struct literals, e.g.:
finding := model.SchemaFinding{ SchemaExtensionId: extension.ID, KindId: 1, EnvironmentId: environment.ID, Name: "finding", DisplayName: "display name", + Type: model.SchemaFindingTypeRelationship, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/graphschema_integration_test.go` around lines 3525 - 3546, The test helper assertContainsFindings must be extended to check the SchemaFinding.Type field: in the comparison loop inside assertContainsFindings, add a check that finding.Type == want.Type (and include Type when constructing expected model.SchemaFinding literals in the tests) so the assertion fails if Type is dropped or misassigned; keep the existing extra validations (ID, CreatedAt) as-is but include Type in the equality comparison and update all expected SchemaFinding instances used by tests to set the Type field accordingly.
🧹 Nitpick comments (4)
cmd/api/src/database/graphschema.go (1)
830-872: Nit: Consider renaming thesrftable alias.The
srfalias historically stood for "schema_relationship_findings." Now that the table isschema_findings, a more intuitive alias likesfwould reduce confusion for future readers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/graphschema.go` around lines 830 - 872, The query in getSchemaFindingsFiltered uses the outdated alias "srf" which is confusing for the schema_findings table; update the SQL in the query string to use a clearer alias (e.g., "sf") everywhere it's referenced (SELECT columns, FROM clause, ORDER BY) and ensure any references to model.SchemaFinding{}.TableName() remain unchanged so only the alias token in the query variable is replaced.cmd/api/src/database/upsert_schema_extension_integration_test.go (1)
1070-1070: Leftover naming:gotSchemaRelationshipFindingshould begotSchemaFindings.The variable is now typed
[]model.SchemaFinding, but the name still says "RelationshipFinding." This is a rename oversight.♻️ Proposed rename
- gotSchemaRelationshipFinding []model.SchemaFinding + gotSchemaFindings []model.SchemaFindingAnd update the two usages on lines 1150, 1153, 1154 accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/upsert_schema_extension_integration_test.go` at line 1070, Rename the leftover variable gotSchemaRelationshipFinding to gotSchemaFindings (type []model.SchemaFinding) throughout the test, updating every usage and assertion that references gotSchemaRelationshipFinding (e.g., the comparisons/assertions where it is populated and checked) so names match; ensure any variable declarations, assignments, and comparisons in upsert_schema_extension_integration_test.go that previously referenced gotSchemaRelationshipFinding are changed to gotSchemaFindings.cmd/api/src/model/graphschema.go (1)
169-199: LGTM — NewSchemaFindingmodel andSchemaFindingsSubtypeare well structured.One minor naming observation:
SchemaFindingsSubtypeuses plural "Findings" whileSchemaFindingis singular. ConsiderSchemaFindingSubtypefor consistency, though this is cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/model/graphschema.go` around lines 169 - 199, Rename the plural struct SchemaFindingsSubtype to the singular SchemaFindingSubtype to match SchemaFinding; update the type declaration name and the receiver on its TableName() method (keep the returned table name "schema_findings_subtypes" unchanged so DB mapping stays the same) and update all references/usages of SchemaFindingsSubtype throughout the codebase (imports, variable declarations, function signatures) to the new SchemaFindingSubtype identifier.cmd/api/src/database/graphschema_integration_test.go (1)
3521-3523: Stale comment: still references "Relationship Findings" instead of "Findings".The comment on line 3521 says "Graph Schema Relationship Findings" but the test and model have been generalized to just "Findings" (
SchemaFinding). Update for consistency.-// Graph Schema Relationship Findings may contain dynamically pre-inserted data, meaning the database +// Graph Schema Findings may contain dynamically pre-inserted data, meaning the database🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/graphschema_integration_test.go` around lines 3521 - 3523, Update the stale comment above TestDatabase_Findings_CRUD to reference "Findings" (or SchemaFinding) instead of "Graph Schema Relationship Findings"; locate the comment near the TestDatabase_Findings_CRUD function and modify the text so it consistently mentions "Findings" / "SchemaFinding" to match the test and model names.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/api/src/database/graphschema_integration_test.go`:
- Around line 3525-3546: The test helper assertContainsFindings must be extended
to check the SchemaFinding.Type field: in the comparison loop inside
assertContainsFindings, add a check that finding.Type == want.Type (and include
Type when constructing expected model.SchemaFinding literals in the tests) so
the assertion fails if Type is dropped or misassigned; keep the existing extra
validations (ID, CreatedAt) as-is but include Type in the equality comparison
and update all expected SchemaFinding instances used by tests to set the Type
field accordingly.
In `@cmd/api/src/database/graphschema.go`:
- Around line 978-982: The SQL in GetRemediationByFindingName still JOINs
schema_relationship_findings (alias srf) which was removed; update the query to
JOIN schema_findings instead and keep the same alias (srf) so downstream column
references (srf.id, srf.name, srf.display_name) still resolve; ensure the WHERE
clause uses srf.name and the GROUP BY still groups by sr.finding_id and
srf.display_name.
---
Nitpick comments:
In `@cmd/api/src/database/graphschema_integration_test.go`:
- Around line 3521-3523: Update the stale comment above
TestDatabase_Findings_CRUD to reference "Findings" (or SchemaFinding) instead of
"Graph Schema Relationship Findings"; locate the comment near the
TestDatabase_Findings_CRUD function and modify the text so it consistently
mentions "Findings" / "SchemaFinding" to match the test and model names.
In `@cmd/api/src/database/graphschema.go`:
- Around line 830-872: The query in getSchemaFindingsFiltered uses the outdated
alias "srf" which is confusing for the schema_findings table; update the SQL in
the query string to use a clearer alias (e.g., "sf") everywhere it's referenced
(SELECT columns, FROM clause, ORDER BY) and ensure any references to
model.SchemaFinding{}.TableName() remain unchanged so only the alias token in
the query variable is replaced.
In `@cmd/api/src/database/upsert_schema_extension_integration_test.go`:
- Line 1070: Rename the leftover variable gotSchemaRelationshipFinding to
gotSchemaFindings (type []model.SchemaFinding) throughout the test, updating
every usage and assertion that references gotSchemaRelationshipFinding (e.g.,
the comparisons/assertions where it is populated and checked) so names match;
ensure any variable declarations, assignments, and comparisons in
upsert_schema_extension_integration_test.go that previously referenced
gotSchemaRelationshipFinding are changed to gotSchemaFindings.
In `@cmd/api/src/model/graphschema.go`:
- Around line 169-199: Rename the plural struct SchemaFindingsSubtype to the
singular SchemaFindingSubtype to match SchemaFinding; update the type
declaration name and the receiver on its TableName() method (keep the returned
table name "schema_findings_subtypes" unchanged so DB mapping stays the same)
and update all references/usages of SchemaFindingsSubtype throughout the
codebase (imports, variable declarations, function signatures) to the new
SchemaFindingSubtype identifier.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/mocks/db.gocmd/api/src/database/upsert_schema_extension_integration_test.gocmd/api/src/database/upsert_schema_finding.gocmd/api/src/database/upsert_schema_finding_integration_test.gocmd/api/src/database/upsert_schema_remediation_integration_test.gocmd/api/src/model/graphschema.go
mistahj67
force-pushed
the
refactor-rel-findings-to-findings
branch
from
9b40fdb to
2d31431
Compare
3 tasks
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/api/src/database/graphschema_integration_test.go (1)
3525-3535:⚠️ Potential issue | 🟠 MajorAssert
SchemaFinding.Typein finding comparison helperAt Line 3530, the helper validates core fields but skips
Type. This misses regressions in the new subtype discriminator path introduced by this PR.Suggested fix
for _, finding := range got { - if finding.SchemaExtensionId == want.SchemaExtensionId && + if finding.Type == want.Type && + finding.SchemaExtensionId == want.SchemaExtensionId && finding.KindId == want.KindId && finding.EnvironmentId == want.EnvironmentId && finding.Name == want.Name && finding.DisplayName == want.DisplayName {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/graphschema_integration_test.go` around lines 3525 - 3535, The comparison helper assertContainsFindings is missing validation of the SchemaFinding.Type field; update the inner equality check in assertContainsFindings (used in graphschema_integration_test.go) to also compare finding.Type == want.Type along with SchemaExtensionId, KindId, EnvironmentId, Name, and DisplayName so tests catch regressions in the subtype discriminator path.
🧹 Nitpick comments (2)
cmd/api/src/database/upsert_schema_extension_integration_test.go (1)
1150-1173: Add an explicit assertion for finding typeSince Line 1150 now fetches generalized findings, add a
Typeassertion so relationship ingestion regressions are caught immediately.✅ Suggested assertion
for i, finding := range gotSchemaRelationshipFinding { // Finding require.Greater(t, finding.ID, int32(0)) + require.Equalf(t, model.SchemaFindingTypeRelationship, finding.Type, "RelationshipFindingInput - finding type mismatch") require.Equalf(t, gotGraphExtension.ID, finding.SchemaExtensionId, "RelationshipFindingInput - graph schema extension id should be greater than 0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/upsert_schema_extension_integration_test.go` around lines 1150 - 1173, The test fetches generalized findings via GetSchemaFindingsBySchemaExtensionId but lacks an assertion that each finding is a relationship, so add an explicit assertion comparing finding.Type to the expected relationship type for each item in the loop (use want.RelationshipFindingsInput[i].Type or the project constant that represents "relationship") to catch any relationship-ingestion regressions; update the loop that iterates gotSchemaRelationshipFinding (and/or the comparator block for RelationshipFindingsInput) to include this Type equality check alongside the existing ID, SchemaExtensionId, Kind and Environment assertions.cmd/api/src/database/graphschema.go (1)
795-796: Update stale method comment to match the new modelLine 795 still says “schema relationship finding,” but this method now creates generic
schema_findings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/graphschema.go` around lines 795 - 796, Update the stale doc comment above CreateSchemaFinding to reflect that it creates generic schema_findings (not "schema relationship finding"); change the comment text to describe that CreateSchemaFinding on BloodhoundDB inserts/creates a new schema_finding (including parameters like findingType, extensionId, kindId, environmentId, name, displayName) and returns the created model.SchemaFinding and error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/api/src/database/graphschema_integration_test.go`:
- Around line 3574-3577: Replace all hard-coded KindId: 1 in SchemaFinding setup
with the ID from the created relationship kind (e.g., use the DAWGS relationship
kind variable such as relKind.ID or dawgsKind.ID) so tests don't depend on seed
ordering; update each finding construction (model.SchemaFinding where KindId is
set alongside SchemaExtensionId: extension.ID and EnvironmentId: environment.ID)
to accept and use that relationship kind ID (pass it into any finding fixture
helpers or inline setups) and ensure any helper/factory signatures are updated
to accept the relationship kind ID parameter.
In `@cmd/api/src/database/upsert_schema_finding.go`:
- Around line 66-81: The function replaceFinding and its related error
messages/comments still use the deprecated phrase "schema relationship finding";
update the comment and all error strings to use "schema finding" instead.
Specifically change the top comment for replaceFinding and the three error
fmt.Errorf messages that mention "schema relationship finding" (the errors
returned around calls to GetSchemaFindingByName, DeleteSchemaFinding, and
CreateSchemaFinding) so they read "schema finding" and keep the same context and
included values; ensure references to ErrNotFound and model.SchemaFinding remain
unchanged.
---
Outside diff comments:
In `@cmd/api/src/database/graphschema_integration_test.go`:
- Around line 3525-3535: The comparison helper assertContainsFindings is missing
validation of the SchemaFinding.Type field; update the inner equality check in
assertContainsFindings (used in graphschema_integration_test.go) to also compare
finding.Type == want.Type along with SchemaExtensionId, KindId, EnvironmentId,
Name, and DisplayName so tests catch regressions in the subtype discriminator
path.
---
Nitpick comments:
In `@cmd/api/src/database/graphschema.go`:
- Around line 795-796: Update the stale doc comment above CreateSchemaFinding to
reflect that it creates generic schema_findings (not "schema relationship
finding"); change the comment text to describe that CreateSchemaFinding on
BloodhoundDB inserts/creates a new schema_finding (including parameters like
findingType, extensionId, kindId, environmentId, name, displayName) and returns
the created model.SchemaFinding and error.
In `@cmd/api/src/database/upsert_schema_extension_integration_test.go`:
- Around line 1150-1173: The test fetches generalized findings via
GetSchemaFindingsBySchemaExtensionId but lacks an assertion that each finding is
a relationship, so add an explicit assertion comparing finding.Type to the
expected relationship type for each item in the loop (use
want.RelationshipFindingsInput[i].Type or the project constant that represents
"relationship") to catch any relationship-ingestion regressions; update the loop
that iterates gotSchemaRelationshipFinding (and/or the comparator block for
RelationshipFindingsInput) to include this Type equality check alongside the
existing ID, SchemaExtensionId, Kind and Environment assertions.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/migration/migrations/v8.8.0.sqlcmd/api/src/database/mocks/db.gocmd/api/src/database/upsert_schema_extension.gocmd/api/src/database/upsert_schema_extension_integration_test.gocmd/api/src/database/upsert_schema_finding.gocmd/api/src/database/upsert_schema_finding_integration_test.gocmd/api/src/database/upsert_schema_remediation_integration_test.gocmd/api/src/model/graphschema.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cmd/api/src/database/migration/migrations/v8.8.0.sql
- cmd/api/src/database/upsert_schema_remediation_integration_test.go
- cmd/api/src/database/upsert_schema_finding_integration_test.go
kpowderly
reviewed
Feb 26, 2026
kpowderly
approved these changes
Feb 26, 2026
kpowderly
reviewed
Feb 26, 2026
sirisjo
approved these changes
Feb 26, 2026
Contributor
sirisjo
left a comment
sirisjo
left a comment
There was a problem hiding this comment.
Ran locally in conjunction with the findings on-start migration to test it out, and everything appears to be populated correctly! 🎉
mistahj67
force-pushed
the
refactor-rel-findings-to-findings
branch
from
2d31431 to
25bebe9
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
cmd/api/src/database/migration/migrations/v8.8.0.sql (1)
52-61:⚠️ Potential issue | 🔴 CriticalReset sequence after inserting explicit IDs to prevent primary key conflicts.
The INSERT correctly preserves legacy IDs, but the
schema_findings_id_seqsequence is not updated afterward. When the application later inserts new findings (without explicit IDs), PostgreSQL will generate IDs starting from the sequence's current value (likely 1), causing duplicate key violations with the migrated data.🐛 Proposed fix to reset sequence
INSERT INTO schema_findings (id, type, schema_extension_id, kind_id, environment_id, name, display_name, created_at) SELECT id, 1, schema_extension_id, relationship_kind_id, environment_id, name, display_name, created_at FROM schema_relationship_findings ON CONFLICT (name) DO NOTHING; + + -- Reset sequence to max(id) to prevent PK conflicts on future inserts + PERFORM setval( + pg_get_serial_sequence('schema_findings', 'id'), + COALESCE((SELECT MAX(id) FROM schema_findings), 1), + true + ); END IF; END $$;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/migration/migrations/v8.8.0.sql` around lines 52 - 61, After migrating rows from schema_relationship_findings into schema_findings with the INSERT that preserves legacy IDs, update the schema_findings_id_seq sequence so future inserts don’t collide: after the INSERT in the v8.8.0.sql DO block, run a setval on the sequence (schema_findings_id_seq) to at least the current MAX(id) from schema_findings (use COALESCE to handle empty table), ensuring the sequence next value is greater than existing IDs; place this immediately after the INSERT that references schema_relationship_findings.cmd/api/src/database/graphschema.go (1)
795-795:⚠️ Potential issue | 🟡 MinorUpdate stale method comment to match the new model name.
Line 795 still says “schema relationship finding”; this method now creates
schema_findingsrecords generically with aTypediscriminator.-// CreateSchemaFinding - creates a new schema relationship finding. +// CreateSchemaFinding - creates a new schema finding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/graphschema.go` at line 795, Update the stale doc comment on the CreateSchemaFinding method: replace "creates a new schema relationship finding" with a description that it creates a schema_findings record (generic) and that the record uses the Type discriminator to distinguish subtypes; keep the method name CreateSchemaFinding and mention the Type discriminator so the comment reflects the new model naming and behavior.cmd/api/src/database/graphschema_integration_test.go (2)
3525-3534:⚠️ Potential issue | 🟡 MinorAssert finding
Typein shared finding assertions.The helper verifies
KindId, name, and environment, but notType. Since this PR introduces discriminator-based findings, tests should assertTypeto catch regression in write/read paths.Suggested fix
if finding.SchemaExtensionId == want.SchemaExtensionId && + finding.Type == want.Type && finding.KindId == want.KindId && finding.EnvironmentId == want.EnvironmentId && finding.Name == want.Name && finding.DisplayName == want.DisplayName {finding := model.SchemaFinding{ + Type: model.SchemaFindingTypeRelationship, SchemaExtensionId: extension.ID, KindId: findingKindID, EnvironmentId: environment.ID, Name: "finding", DisplayName: "display name", }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/graphschema_integration_test.go` around lines 3525 - 3534, The test helper assertContainsFindings does not check the SchemaFinding.Type field, so add a Type equality assertion when comparing expected vs got to ensure discriminator-based findings roundtrip correctly; update the loop inside assertContainsFindings (which iterates over got and expected model.SchemaFinding) to include a check comparing finding.Type == want.Type (alongside SchemaExtensionId, KindId, EnvironmentId, Name, DisplayName) and fail the test if the Type differs or is missing.
3553-3560:⚠️ Potential issue | 🟡 MinorAvoid test fixtures pinned to
KindId: 1.Line 3576 (and the same pattern at Lines 3600, 3621, 3652, 3682, and 3808) hard-codes kind IDs, which makes these integration tests depend on DB seed ordering instead of explicit test setup.
Suggested fix
-setupFindingDeps := func(t *testing.T, testSuite IntegrationTestSuite) (model.GraphSchemaExtension, model.SchemaEnvironment) { +setupFindingDeps := func(t *testing.T, testSuite IntegrationTestSuite) (model.GraphSchemaExtension, model.SchemaEnvironment, int32) { t.Helper() extension := createTestExtension(t, testSuite, "test_extension", "test_extension", "1.0.0", "Test") nodeKind := createTestNodeKind(t, testSuite, "nodeKind1", extension.ID, "Node Kind 1", "Test description", false, "fa-test", "#000000") envKind := getKindByName(t, testSuite, nodeKind.Name) sourceKind := registerAndGetSourceKind(t, testSuite, "Source_Kind_1") environment := createTestEnvironment(t, testSuite, extension.ID, envKind.ID, int32(sourceKind.ID)) - return extension, environment + relationshipKind := createTestRelationshipKind(t, testSuite, "test_edge_kind", extension.ID, "test edge kind", true) + dawgsKind := getKindByName(t, testSuite, relationshipKind.Name) + return extension, environment, dawgsKind.ID }- extension, environment := setupFindingDeps(t, testSuite) + extension, environment, findingKindID := setupFindingDeps(t, testSuite) finding := model.SchemaFinding{ SchemaExtensionId: extension.ID, - KindId: 1, + KindId: findingKindID, EnvironmentId: environment.ID, Name: "finding", DisplayName: "display name", }Also applies to: 3574-3577, 3598-3601, 3619-3622, 3650-3653, 3680-3683, 3801-3808
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/graphschema_integration_test.go` around lines 3553 - 3560, Tests are pinning KindId to 1 which couples them to DB seed order; update setupFindingDeps and related tests to derive kind IDs from the fixtures you already create instead of hard-coding 1: use the ID returned by createTestNodeKind/getKindByName (envKind.ID) and the sourceKind.ID when calling createTestEnvironment and any other setup functions, and replace any literal KindId: 1 occurrences in the tests with those variables so the tests are self-contained and deterministic.
🧹 Nitpick comments (2)
cmd/api/src/database/migration/migrations/v8.8.0.sql (1)
36-50: Consider documenting validtypevalues.The
typecolumn uses magic number1(line 57) without documentation or a CHECK constraint. A brief comment indicating valid values (e.g.,-- type: 1 = relationship, 2 = list) would improve maintainability. This is optional since the values are likely documented elsewhere in code.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/migration/migrations/v8.8.0.sql` around lines 36 - 50, Add documentation and optional validation for the magic `type` values on the schema_findings table: in the CREATE TABLE for schema_findings, add a brief SQL comment above the `type INTEGER NOT NULL` line describing the enumerated values (e.g., "-- type: 1 = relationship, 2 = list") and, if you want enforcement, add a CHECK constraint on the `type` column (referencing the `schema_findings.type` column name) to restrict allowed integers to the documented set; update any related migration comments to match the chosen values.cmd/api/src/database/upsert_schema_extension_integration_test.go (1)
1153-1173: Add an assertion for finding discriminator type.This test now exercises
SchemaFinding; assertingfinding.Typewould lock in relationship-finding behavior and catch regressions early.Suggested test assertion
for i, finding := range gotSchemaRelationshipFinding { // Finding require.Greater(t, finding.ID, int32(0)) + require.Equalf(t, model.SchemaFindingTypeRelationship, finding.Type, "RelationshipFindingInput - finding type mismatch") require.Equalf(t, gotGraphExtension.ID, finding.SchemaExtensionId, "RelationshipFindingInput - graph schema extension id should be greater than 0")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/api/src/database/upsert_schema_extension_integration_test.go` around lines 1153 - 1173, Add an assertion inside the loop that checks the finding.Type discriminator to lock in relationship-finding behavior; e.g., in the for i, finding := range gotSchemaRelationshipFinding loop add require.Equalf(t, want.RelationshipFindingsInput[i].Type, finding.Type, "RelationshipFindingInput - type discriminator mismatch") (if the test fixture in want includes Type) or compare against the known constant for relationship findings (e.g., SchemaFindingRelationshipType or the project enum value representing a relationship schema finding) to ensure the returned finding.Type is the expected discriminator.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/api/src/database/graphschema_integration_test.go`:
- Around line 3525-3534: The test helper assertContainsFindings does not check
the SchemaFinding.Type field, so add a Type equality assertion when comparing
expected vs got to ensure discriminator-based findings roundtrip correctly;
update the loop inside assertContainsFindings (which iterates over got and
expected model.SchemaFinding) to include a check comparing finding.Type ==
want.Type (alongside SchemaExtensionId, KindId, EnvironmentId, Name,
DisplayName) and fail the test if the Type differs or is missing.
- Around line 3553-3560: Tests are pinning KindId to 1 which couples them to DB
seed order; update setupFindingDeps and related tests to derive kind IDs from
the fixtures you already create instead of hard-coding 1: use the ID returned by
createTestNodeKind/getKindByName (envKind.ID) and the sourceKind.ID when calling
createTestEnvironment and any other setup functions, and replace any literal
KindId: 1 occurrences in the tests with those variables so the tests are
self-contained and deterministic.
In `@cmd/api/src/database/graphschema.go`:
- Line 795: Update the stale doc comment on the CreateSchemaFinding method:
replace "creates a new schema relationship finding" with a description that it
creates a schema_findings record (generic) and that the record uses the Type
discriminator to distinguish subtypes; keep the method name CreateSchemaFinding
and mention the Type discriminator so the comment reflects the new model naming
and behavior.
In `@cmd/api/src/database/migration/migrations/v8.8.0.sql`:
- Around line 52-61: After migrating rows from schema_relationship_findings into
schema_findings with the INSERT that preserves legacy IDs, update the
schema_findings_id_seq sequence so future inserts don’t collide: after the
INSERT in the v8.8.0.sql DO block, run a setval on the sequence
(schema_findings_id_seq) to at least the current MAX(id) from schema_findings
(use COALESCE to handle empty table), ensuring the sequence next value is
greater than existing IDs; place this immediately after the INSERT that
references schema_relationship_findings.
---
Nitpick comments:
In `@cmd/api/src/database/migration/migrations/v8.8.0.sql`:
- Around line 36-50: Add documentation and optional validation for the magic
`type` values on the schema_findings table: in the CREATE TABLE for
schema_findings, add a brief SQL comment above the `type INTEGER NOT NULL` line
describing the enumerated values (e.g., "-- type: 1 = relationship, 2 = list")
and, if you want enforcement, add a CHECK constraint on the `type` column
(referencing the `schema_findings.type` column name) to restrict allowed
integers to the documented set; update any related migration comments to match
the chosen values.
In `@cmd/api/src/database/upsert_schema_extension_integration_test.go`:
- Around line 1153-1173: Add an assertion inside the loop that checks the
finding.Type discriminator to lock in relationship-finding behavior; e.g., in
the for i, finding := range gotSchemaRelationshipFinding loop add
require.Equalf(t, want.RelationshipFindingsInput[i].Type, finding.Type,
"RelationshipFindingInput - type discriminator mismatch") (if the test fixture
in want includes Type) or compare against the known constant for relationship
findings (e.g., SchemaFindingRelationshipType or the project enum value
representing a relationship schema finding) to ensure the returned finding.Type
is the expected discriminator.
ℹ️ Review info
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
cmd/api/src/database/graphschema.gocmd/api/src/database/graphschema_integration_test.gocmd/api/src/database/migration/migrations/v8.8.0.sqlcmd/api/src/database/mocks/db.gocmd/api/src/database/upsert_schema_extension.gocmd/api/src/database/upsert_schema_extension_integration_test.gocmd/api/src/database/upsert_schema_finding.gocmd/api/src/database/upsert_schema_finding_integration_test.gocmd/api/src/database/upsert_schema_remediation_integration_test.gocmd/api/src/model/graphschema.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/api/src/database/upsert_schema_remediation_integration_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Describe your changes in detail
Motivation and Context
Prep work for dynamic OG findings
Resolves BED-7508
Why is this change required? What problem does it solve?
How Has This Been Tested?
Please describe in detail how you tested your changes.
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Chores
Tests