Merged
Conversation
📝 WalkthroughWalkthroughAdds a new AddForeignKeyCheck that detects ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY statements added without NOT VALID, plus accompanying docs, pg helper functions, fixtures, tests, and registry/export integration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can enforce grammar and style rules using `languagetool`.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/src/checks/add_foreign_key.md (1)
19-19: Minor clarification: "no lock" is slightly imprecise.
ADD CONSTRAINT ... NOT VALIDstill acquires aShareRowExclusiveLock, but only briefly (to update catalog metadata), rather than holding it for the duration of a full table scan. Consider clarifying to "no table scan, brief lock" or "no validation lock" to be more precise.📝 Suggested clarification
-- Safe --- Step 1 (no table scan, no lock) +-- Step 1 (no table scan, brief metadata lock only) ALTER TABLE orders ADD CONSTRAINT fk_user_id🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/checks/add_foreign_key.md` at line 19, Update the wording in the "Step 1 (no table scan, no lock)" heading/line in add_foreign_key.md to clarify that ADD CONSTRAINT ... NOT VALID still takes a brief catalog lock: replace "no lock" with a precise phrase such as "brief lock" or "no table scan, brief lock" (update the heading "Step 1 (no table scan, no lock)" to e.g. "Step 1 (no table scan, brief lock)") so the doc accurately reflects the ShareRowExclusiveLock taken briefly to update metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/checks/add_foreign_key.rs`:
- Line 46: The remediation text in add_foreign_key.rs currently says "with any
validation" which is misleading; update the remediation string to read "without
immediate validation (NOT VALID)" so it instructs creating the foreign key
without immediate validation. Locate the remediation message in the
add_foreign_key.rs code (the string containing "with any validation") and
replace it with the corrected wording, ensuring the phrase "NOT VALID" is
included exactly as shown.
- Around line 33-51: The generated remediation SQL misformats the FK clause and
breaks for unnamed constraints: ensure the FK column list is wrapped as "FOREIGN
KEY ({columns})" (replace FOREIGN KEY {columns}) and handle constraint.conname
== "" by not emitting "CONSTRAINT <unnamed>" (or by generating a safe
deterministic name) — i.e., when constraint.conname is empty produce "ALTER
TABLE {table} ADD FOREIGN KEY ({columns}) REFERENCES {ref_table} ({ref_cols})
NOT VALID;" and only emit "ALTER TABLE {table} VALIDATE CONSTRAINT
{constraint_name};" when a real name exists; update the code that builds the
Violation message (variables constraint.conname / constraint_name, table_name,
fk_cols / columns, ref_table, ref_cols) inside add_foreign_key.rs so the SQL is
syntactically valid in both named and unnamed cases.
---
Nitpick comments:
In `@docs/src/checks/add_foreign_key.md`:
- Line 19: Update the wording in the "Step 1 (no table scan, no lock)"
heading/line in add_foreign_key.md to clarify that ADD CONSTRAINT ... NOT VALID
still takes a brief catalog lock: replace "no lock" with a precise phrase such
as "brief lock" or "no table scan, brief lock" (update the heading "Step 1 (no
table scan, no lock)" to e.g. "Step 1 (no table scan, brief lock)") so the doc
accurately reflects the ShareRowExclusiveLock taken briefly to update metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ed21ee86-5c2b-489f-87dd-09dd82c6d4d2
📒 Files selected for processing (10)
docs/src/SUMMARY.mddocs/src/checks/add_foreign_key.mdsrc/checks/add_foreign_key.rssrc/checks/mod.rssrc/checks/pg_helpers.rssrc/checks/unnamed_constraint.rstests/fixtures/add_foreign_key_safe/up.sqltests/fixtures/add_foreign_key_unsafe/up.sqltests/fixtures/unnamed_constraint_safe/up.sqltests/fixtures_test.rs
src/checks/add_foreign_key.rs
Outdated
| format!( | ||
| r#"For a safer foreign key addition on large tables: | ||
|
|
||
| 1. Create a foreign key with any validation: |
There was a problem hiding this comment.
Wording is misleading in the remediation step.
Line 46 says “with any validation”; it should state “without immediate validation” (NOT VALID) to match the intended safe flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/checks/add_foreign_key.rs` at line 46, The remediation text in
add_foreign_key.rs currently says "with any validation" which is misleading;
update the remediation string to read "without immediate validation (NOT VALID)"
so it instructs creating the foreign key without immediate validation. Locate
the remediation message in the add_foreign_key.rs code (the string containing
"with any validation") and replace it with the corrected wording, ensuring the
phrase "NOT VALID" is included exactly as shown.
valkrypton
force-pushed
the
add-foreign-key-check
branch
from
06d737a to
9bee12a
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/checks/add_foreign_key.rs (1)
47-51: Inconsistent placeholder naming in remediation template.Line 47 uses
{table}while line 51 uses{table_name}. Both resolve correctly, but using the same placeholder consistently would improve readability.Suggested fix
1. Create a foreign key without any immediate validation: ALTER TABLE {table} ADD CONSTRAINT {constraint_name} FOREIGN KEY ({columns}) REFERENCES {ref_table} ({ref_cols}) NOT VALID; 2. Step 2 (separate migration, acquires ShareUpdateExclusiveLock only) - ALTER TABLE {table_name} VALIDATE CONSTRAINT {constraint_name}; + ALTER TABLE {table} VALIDATE CONSTRAINT {constraint_name};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/checks/add_foreign_key.rs` around lines 47 - 51, Remediation template in src/checks/add_foreign_key.rs uses inconsistent placeholders ({table} vs {table_name}); locate the template string (look for the remediation/SQL template in the add_foreign_key check, e.g., a constant or function named like remediation_template, REMEDIATION, or in AddForeignKey check code) and make the placeholder names identical (choose either {table_name} or {table} and replace the other occurrence to match), then ensure any code that interpolates values uses the chosen placeholder name consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/checks/add_foreign_key.rs`:
- Around line 47-51: Remediation template in src/checks/add_foreign_key.rs uses
inconsistent placeholders ({table} vs {table_name}); locate the template string
(look for the remediation/SQL template in the add_foreign_key check, e.g., a
constant or function named like remediation_template, REMEDIATION, or in
AddForeignKey check code) and make the placeholder names identical (choose
either {table_name} or {table} and replace the other occurrence to match), then
ensure any code that interpolates values uses the chosen placeholder name
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b14e61fc-cbc2-481f-9dbd-9d64409cdfc4
📒 Files selected for processing (10)
docs/src/SUMMARY.mddocs/src/checks/add_foreign_key.mdsrc/checks/add_foreign_key.rssrc/checks/mod.rssrc/checks/pg_helpers.rssrc/checks/unnamed_constraint.rstests/fixtures/add_foreign_key_safe/up.sqltests/fixtures/add_foreign_key_unsafe/up.sqltests/fixtures/unnamed_constraint_safe/up.sqltests/fixtures_test.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/checks/pg_helpers.rs
- src/checks/mod.rs
- docs/src/SUMMARY.md
- src/checks/unnamed_constraint.rs
- tests/fixtures/unnamed_constraint_safe/up.sql
- tests/fixtures/add_foreign_key_unsafe/up.sql
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
==========================================
+ Coverage 94.18% 94.38% +0.19%
==========================================
Files 39 40 +1
Lines 1221 1264 +43
==========================================
+ Hits 1150 1193 +43
Misses 71 71
🚀 New features to boost your workflow:
|
ayarotsky
reviewed
Mar 17, 2026
Owner
ayarotsky
left a comment
ayarotsky
left a comment
There was a problem hiding this comment.
Great job! A few minor comments and we're good to merge.
docs/src/checks/add_foreign_key.md
Outdated
|
|
||
| ## Description | ||
|
|
||
| The `add_foreign_key` check flags foreign keys added without the `NOT VALID` option. Adding a foreign key with validation requires a `ShareRowExclusiveLock`, which blocks writes on the table. On large tables, this can cause outages. |
Owner
There was a problem hiding this comment.
Please follow the format of other checks' docs. Example: https://ayarotsky.github.io/diesel-guard/checks/adding-unique-constraint.html.
| - [Framework Adapters](frameworks.md) | ||
| - [Checks](checks/overview.md) | ||
| - [ADD COLUMN with DEFAULT](checks/add-column-default.md) | ||
| - [Add Foreign Key](checks/add_foreign_key.md) |
Owner
There was a problem hiding this comment.
You also need to mention this check in the table in README.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/src/checks/add_foreign_key.md`:
- Line 19: Fix the typo in the user-facing docs by replacing the misspelled word
"foregin" with "foreign" in the sentence "Add the foregin key first without
validation..." so it reads "Add the foreign key first without validation using
the `NOT VALID` clause. Validate the foreign key later in a separate". Locate
the occurrence of "foregin" and correct it to "foreign".
- Around line 23-24: Update the Step 1 description for the ALTER TABLE orders
ADD CONSTRAINT fk_user_id guidance to remove the inaccurate “no lock” phrasing
and replace it with precise wording such as “no validation scan; short metadata
lock” (or equivalent), so the guidance reflects the actual lock behavior (short
metadata lock / ShareRowExclusiveLock implication) rather than claiming no lock.
In `@README.md`:
- Line 96: The README line describing the lock for "ADD FOREIGN KEY without NOT
VALID" should use the precise PostgreSQL lock name; replace the current phrase
"SHARE lock" with "ShareRowExclusiveLock" (or state "ShareRowExclusiveLock
(blocks writes)") to match terminology used elsewhere and avoid confusion;
update the table cell text where "ADD FOREIGN KEY without NOT VALID | Blocks
writes (SHARE lock)" appears so it consistently references
ShareRowExclusiveLock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: de08f930-684c-46ba-8ace-173cd32c5808
📒 Files selected for processing (2)
README.mddocs/src/checks/add_foreign_key.md
docs/src/checks/add_foreign_key.md
Outdated
|
|
||
| ### Good | ||
|
|
||
| Add the foregin key first without validation using the `NOT VALID` clause. Validate the foreign key later in a separate |
There was a problem hiding this comment.
Correct typo in user-facing docs.
Line 19 has foregin → foreign.
Proposed typo fix
-Add the foregin key first without validation using the `NOT VALID` clause. Validate the foreign key later in a separate
+Add the foreign key first without validation using the `NOT VALID` clause. Validate the foreign key later in a separate📝 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.
Suggested change
| Add the foregin key first without validation using the `NOT VALID` clause. Validate the foreign key later in a separate | |
| Add the foreign key first without validation using the `NOT VALID` clause. Validate the foreign key later in a separate |
🧰 Tools
🪛 LanguageTool
[grammar] ~19-~19: Ensure spelling is correct
Context: ...ENCES users(id); ``` ### Good Add the foregin key first without validation using the ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/src/checks/add_foreign_key.md` at line 19, Fix the typo in the
user-facing docs by replacing the misspelled word "foregin" with "foreign" in
the sentence "Add the foregin key first without validation..." so it reads "Add
the foreign key first without validation using the `NOT VALID` clause. Validate
the foreign key later in a separate". Locate the occurrence of "foregin" and
correct it to "foreign".
README.md
Outdated
| | Check | Risk | | ||
| |-----------------------------------|---------------------------------------------------| | ||
| | ADD COLUMN with DEFAULT | Table rewrite on Postgres < 11 (ACCESS EXCLUSIVE) | | ||
| | ADD FOREIGN KEY without NOT VALID | Blocks writes (SHARE lock) | |
There was a problem hiding this comment.
Use the precise PostgreSQL lock name for this row.
Line 96 says SHARE lock, but this check/documentation elsewhere refers to ShareRowExclusiveLock. Please align wording to avoid misleading lock semantics.
Proposed wording tweak
-| ADD FOREIGN KEY without NOT VALID | Blocks writes (SHARE lock) |
+| ADD FOREIGN KEY without NOT VALID | Blocks writes (ShareRowExclusiveLock) |📝 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.
Suggested change
| | ADD FOREIGN KEY without NOT VALID | Blocks writes (SHARE lock) | | |
| | ADD FOREIGN KEY without NOT VALID | Blocks writes (ShareRowExclusiveLock) | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 96, The README line describing the lock for "ADD FOREIGN
KEY without NOT VALID" should use the precise PostgreSQL lock name; replace the
current phrase "SHARE lock" with "ShareRowExclusiveLock" (or state
"ShareRowExclusiveLock (blocks writes)") to match terminology used elsewhere and
avoid confusion; update the table cell text where "ADD FOREIGN KEY without NOT
VALID | Blocks writes (SHARE lock)" appears so it consistently references
ShareRowExclusiveLock.
c27b443 to
9eb4ed5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/src/checks/add_foreign_key.md (1)
17-17: Use consistent heading levels for sibling sections.
BadandGoodare peer sections, so using the same heading level improves readability and doc structure consistency.Proposed doc tweak
-### Good +## Good🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/src/checks/add_foreign_key.md` at line 17, The "Good" heading is using a different level ("### Good") than its sibling "Bad"; update the "Good" heading to match the sibling (use the same heading level as "Bad", e.g., change "### Good" to "## Good") so both sections are consistent in docs/src/checks/add_foreign_key.md.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/src/checks/add_foreign_key.md`:
- Line 17: The "Good" heading is using a different level ("### Good") than its
sibling "Bad"; update the "Good" heading to match the sibling (use the same
heading level as "Bad", e.g., change "### Good" to "## Good") so both sections
are consistent in docs/src/checks/add_foreign_key.md.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be3b0364-4a92-4e31-9619-01ee404c39a0
📒 Files selected for processing (2)
README.mddocs/src/checks/add_foreign_key.md
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
4 tasks
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
2 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.
What does this PR do?
Fixes #87
Summary:
AddForeignKeycheck that checks, foreign keys being added using a ALTER TABLE statement must use a NOT VALID clause.docs/src/checks/add_foreign_key.mdand linked them in the summary.unnamed_constraint_check.rstopg_helpers.rsChecklist
Developmentsectiondocs/src/checks/<check>.md+docs/src/SUMMARY.md)Summary by CodeRabbit
New Features
Documentation
Tests