fix(OpenGraph): fix ID column for schema_findings table BED-7734#2538
fix(OpenGraph): fix ID column for schema_findings table BED-7734#2538sirisjo wants to merge 1 commit intostage/v8.9.1from
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a SQL migration ( Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.9.1.sql`:
- Around line 17-18: Modify the SETVAL call to handle an empty schema_findings
table by replacing MAX(id) with COALESCE(MAX(id), 0) so the sequence is set to 0
when no rows exist; update the statement using the existing sequence name
schema_findings_id_seq and table schema_findings (i.e., call
setval('schema_findings_id_seq', COALESCE(MAX(id), 0), true) FROM
schema_findings) to avoid NULL causing setval() to fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: ca7f9ab6-44f5-452e-8b66-264ea2141336
📒 Files selected for processing (1)
cmd/api/src/database/migration/migrations/v8.9.1.sql
| -- Fix schema_findings_pkey duplicate key value violation | ||
| SELECT setval('schema_findings_id_seq', MAX(id), true) FROM schema_findings; No newline at end of file |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Test the migration on an empty schema_findings table scenario
# Check if there are any test files that cover this migration
rg -n "v8\.9\.1|schema_findings_id_seq" --type=go --glob '*test*'
# Check for similar sequence reset patterns in other migrations
rg -n "setval.*COALESCE|setval.*MAX\(id\)" --type=sqlRepository: SpecterOps/BloodHound
Length of output: 874
🏁 Script executed:
# Check the migration execution order and schema_findings initialization
# Look at v8.9.0 to understand what triggered the need for this fix
fd -e sql -name "v8.9.0" | head -20Repository: SpecterOps/BloodHound
Length of output: 295
🏁 Script executed:
# Search for schema_findings table creation and when it's populated
rg -n "schema_findings" --type=sql -B 2 -A 2 | head -50Repository: SpecterOps/BloodHound
Length of output: 3822
🏁 Script executed:
# Check if there's any migration framework logic or safeguards
fd -e go -name "*migrat*" | head -20Repository: SpecterOps/BloodHound
Length of output: 295
🌐 Web query:
PostgreSQL setval function behavior with NULL value empty table
💡 Result:
The PostgreSQL setval function requires a non-NULL bigint value as its second argument and cannot accept NULL directly. Attempting SELECT setval('seq', NULL) will raise an error like "function setval(unknown, unknown) does not exist" or a type mismatch error, as NULL lacks a specific type and the function signature expects bigint. For an empty table where MAX(id) returns NULL (common when resetting sequences after TRUNCATE or for new tables), directly passing NULL to setval fails. Instead, use COALESCE to handle NULL: SELECT setval('seq', COALESCE((SELECT MAX(id) FROM table), 0), false); This sets last_value to 0 (allowed only if minvalue <=0, else use 1 and adjust), with is_called=false so nextval returns 1. Official docs confirm setval(regclass, bigint [, boolean]) → bigint, with the second parameter being the value to set last_value to, which must be within the sequence's minvalue/maxvalue bounds (default minvalue=1 for ascending sequences created by SERIAL). Setting below minvalue errors: "setval: value X is out of bounds". To reset for empty table to start at 1: - If minvalue=1: SELECT setval('seq', 1, false); -- next nextval returns 1 - Or use ALTER SEQUENCE seq RESTART; -- equivalent to setval with is_called=false using recorded start value (usually 1) Sources confirm this behavior across versions up to 18; no direct NULL support exists.
Citations:
- 1: https://www.postgresql.org/docs/current/functions-sequence.html
- 2: https://www.postgresql.org/docs/current/sql-altersequence.html
- 3: https://stackoverflow.com/questions/244243/how-to-reset-postgres-primary-key-sequence-when-it-falls-out-of-sync
- 4: https://pgpedia.info/s/setval.html
- 5: https://www.postgresql.org/message-id/CAFj8pRCpK3%3D6om%2BFUw%3DqQXr45Jfyisj8AO_4B-5RACrxCp6BqQ%40mail.gmail.com
- 6: https://www.tutorialpedia.org/blog/postgres-manually-alter-sequence/
- 7: https://www.postgresql.org/message-id/sdf0544f.093%40mail.tpchd.org
- 8: https://database.guide/how-setval-works-in-postgresql/
- 9: https://www.postgresql.org/docs/current/sql-createsequence.html
- 10: https://stackoverflow.com/questions/16485708/reset-sequence-value-as-1
- 11: https://w3resource.com/PostgreSQL/snippets/resetting-a-primary-key-sequence-in-postgresql.php
- 12: https://stackoverflow.com/posts/3698777/revisions
🏁 Script executed:
# Check the context of schema.sql - when/how is it executed?
head -50 cmd/api/src/database/migration/migrations/schema.sqlRepository: SpecterOps/BloodHound
Length of output: 1640
🏁 Script executed:
# Look at the migration framework - how are versions ordered and applied?
fd -type f -name "*.go" | xargs grep -l "migration\|Migration" | head -5Repository: SpecterOps/BloodHound
Length of output: 235
🏁 Script executed:
# Check if there's any logic about when migrations apply or reset sequences
rg -n "schema.sql" --type=go -B 2 -A 2 | head -50Repository: SpecterOps/BloodHound
Length of output: 1571
🏁 Script executed:
# Verify: does v8.9.0 migration actually insert data, or is schema_findings empty after v8.9.0?
cat cmd/api/src/database/migration/migrations/v8.9.0.sqlRepository: SpecterOps/BloodHound
Length of output: 8878
🏁 Script executed:
# Check migration ordering and execution - is v8.9.0/v8.9.1 only for upgrades?
rg -n "version|Version" cmd/api/src/database/migration/migrations/v8.9.0.sql | head -5Repository: SpecterOps/BloodHound
Length of output: 116
🏁 Script executed:
# Look at how migrations are applied - check manifest or migration runner
fd -type f -name "*.go" -path "*/migration/*" | xargs grep -l "manifest\|apply\|version" | head -3Repository: SpecterOps/BloodHound
Length of output: 235
🏁 Script executed:
# Check: does the base schema.sql have the same setval NULL issue?
# Find where setval calls on empty tables would occur
rg "SELECT setval.*MAX\(id\)" cmd/api/src/database/migration/migrations/schema.sql -B 5 -A 1 | head -40Repository: SpecterOps/BloodHound
Length of output: 47
Handle NULL case when schema_findings table is empty.
The MAX(id) function returns NULL when the table is empty, causing setval() to fail and block the migration. This occurs when schema_relationship_findings doesn't exist (fresh installations or previously migrated systems).
Proposed fix
-- Fix schema_findings_pkey duplicate key value violation
-SELECT setval('schema_findings_id_seq', MAX(id), true) FROM schema_findings;
+SELECT setval('schema_findings_id_seq', COALESCE(MAX(id), 0), true) FROM schema_findings;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| -- Fix schema_findings_pkey duplicate key value violation | |
| SELECT setval('schema_findings_id_seq', MAX(id), true) FROM schema_findings; | |
| -- Fix schema_findings_pkey duplicate key value violation | |
| SELECT setval('schema_findings_id_seq', COALESCE(MAX(id), 0), true) FROM schema_findings; |
🤖 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.9.1.sql` around lines 17 - 18,
Modify the SETVAL call to handle an empty schema_findings table by replacing
MAX(id) with COALESCE(MAX(id), 0) so the sequence is set to 0 when no rows
exist; update the statement using the existing sequence name
schema_findings_id_seq and table schema_findings (i.e., call
setval('schema_findings_id_seq', COALESCE(MAX(id), 0), true) FROM
schema_findings) to avoid NULL causing setval() to fail.
1 participant
Description
Fixes a
duplicate key errorthat occurs during the on-start findings migration if the database had existing findings in theschema_relationship_findingstable during the v8.9.0 migration.Motivation and Context
Resolves BED-7734
Why is this change required? What problem does it solve?
How Has This Been Tested?
Reproduced the issue locally, and then ran this fix and the issue was solved.
To repro locally, run the v.8.7.0 version of the code and upload an OpenGraph schema with findings. When migrating to v8.9.0, you should encounter the error. Run the v8.9.1 migration to fix the error. The database
schema_findingstable should be populated with your OpenGraph findings first, and then the built-in findings.Screenshots (optional):
Types of changes
Checklist:
Summary by CodeRabbit