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
📝 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
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
| type INTEGER NOT NULL, | ||
| schema_extension_id INTEGER NOT NULL REFERENCES schema_extensions(id) ON DELETE CASCADE, | ||
| environment_id INTEGER NOT NULL REFERENCES schema_environments(id) ON DELETE CASCADE, | ||
| kind_id INTEGER NOT NULL REFERENCES kind(id), |
Contributor
There was a problem hiding this comment.
question: why not ON DELETE CASCADE for kinds as well?
Contributor
Author
There was a problem hiding this comment.
The original schema_relationship_findings table didn't have this constraint so I elected to follow the design choice here as well.
I believe one of the higher assumptions for this architecture is that no kind is ever deleted, and so it'd be a waste to have that here now
Contributor
There was a problem hiding this comment.
Can we confirm that OG Kinds will never be deleted? That feels like an assumption made for AD and AZ, but might not hold up for OG.
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
Comment on lines
+3574
to
+3577
| finding := model.SchemaFinding{ | ||
| SchemaExtensionId: extension.ID, | ||
| KindId: 1, | ||
| EnvironmentId: environment.ID, |
Contributor
There was a problem hiding this comment.
Avoid hard-coded KindId: 1 in finding setup
Lines 3576/3600/3621/3652/3682 and Line 3808 hard-code KindId to 1. This couples tests to seed ordering and can become flaky across migrations.
Use a created relationship kind’s DAWGS kind ID in setup, then pass that ID into finding fixtures.
Also applies to: 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 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.
Comment on lines
66
to
81
| // replaceFinding creates or updates a schema relationship finding. | ||
| // If a finding with the given name exists, it deletes it first before creating the new one. | ||
| func (s *BloodhoundDB) replaceFinding(ctx context.Context, extensionId, relationshipKindId, environmentId int32, name, displayName string) (model.SchemaRelationshipFinding, error) { | ||
| if existing, err := s.GetSchemaRelationshipFindingByName(ctx, name); err != nil && !errors.Is(err, ErrNotFound) { | ||
| return model.SchemaRelationshipFinding{}, fmt.Errorf("error retrieving schema relationship finding: %w", err) | ||
| func (s *BloodhoundDB) replaceFinding(ctx context.Context, findingType model.SchemaFindingType, extensionId, kindId, environmentId int32, name, displayName string) (model.SchemaFinding, error) { | ||
| if existing, err := s.GetSchemaFindingByName(ctx, name); err != nil && !errors.Is(err, ErrNotFound) { | ||
| return model.SchemaFinding{}, fmt.Errorf("error retrieving schema relationship finding: %w", err) | ||
| } else if err == nil { | ||
| // Finding exists - delete it first | ||
| if err := s.DeleteSchemaRelationshipFinding(ctx, existing.ID); err != nil { | ||
| return model.SchemaRelationshipFinding{}, fmt.Errorf("error deleting schema relationship finding %d: %w", existing.ID, err) | ||
| if err := s.DeleteSchemaFinding(ctx, existing.ID); err != nil { | ||
| return model.SchemaFinding{}, fmt.Errorf("error deleting schema relationship finding %d: %w", existing.ID, err) | ||
| } | ||
| } | ||
|
|
||
| finding, err := s.CreateSchemaRelationshipFinding(ctx, extensionId, relationshipKindId, environmentId, name, displayName) | ||
| finding, err := s.CreateSchemaFinding(ctx, findingType, extensionId, kindId, environmentId, name, displayName) | ||
| if err != nil { | ||
| return model.SchemaRelationshipFinding{}, fmt.Errorf("error creating schema relationship finding: %w", err) | ||
| return model.SchemaFinding{}, fmt.Errorf("error creating schema relationship finding: %w", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Error/context text still uses deprecated “schema relationship finding” wording
Lines 70, 74, and 80 (and the comment at Line 66) still reference the old entity name, which makes diagnostics inconsistent with the new schema_findings terminology.
🛠️ Suggested terminology cleanup
-// replaceFinding creates or updates a schema relationship finding.
+// replaceFinding creates or updates a schema finding.
// If a finding with the given name exists, it deletes it first before creating the new one.
func (s *BloodhoundDB) replaceFinding(ctx context.Context, findingType model.SchemaFindingType, extensionId, kindId, environmentId int32, name, displayName string) (model.SchemaFinding, error) {
if existing, err := s.GetSchemaFindingByName(ctx, name); err != nil && !errors.Is(err, ErrNotFound) {
- return model.SchemaFinding{}, fmt.Errorf("error retrieving schema relationship finding: %w", err)
+ return model.SchemaFinding{}, fmt.Errorf("error retrieving schema finding: %w", err)
} else if err == nil {
// Finding exists - delete it first
if err := s.DeleteSchemaFinding(ctx, existing.ID); err != nil {
- return model.SchemaFinding{}, fmt.Errorf("error deleting schema relationship finding %d: %w", existing.ID, err)
+ return model.SchemaFinding{}, fmt.Errorf("error deleting schema finding %d: %w", existing.ID, err)
}
}
finding, err := s.CreateSchemaFinding(ctx, findingType, extensionId, kindId, environmentId, name, displayName)
if err != nil {
- return model.SchemaFinding{}, fmt.Errorf("error creating schema relationship finding: %w", err)
+ return model.SchemaFinding{}, fmt.Errorf("error creating schema finding: %w", err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/api/src/database/upsert_schema_finding.go` around lines 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.
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