feat: SQL Server FK actions are not explicit in the baseline migration - #105
Merged
Conversation
…nstraints in migration
… onUpdate actions
Contributor
WalkthroughAdds explicit Changes
Sequence Diagram(s)sequenceDiagram
participant Entity as Entity Schema
participant Migration as Migration SQL
participant Test as Drift-Guard Test
participant Script as Admin Script
Entity->>Migration: Declare FK names + onDelete/onUpdate intents
Script->>Migration: Include migration in migration list
Migration->>Migration: up(): drop/recreate FKs with explicit ON DELETE/ON UPDATE
Test->>Migration: Read UP_STATEMENTS from migration files
Test->>Entity: Load sqlServerEntities and extract declared FKs/actions
Test->>Test: Parse SQL ALTER TABLE ... ADD CONSTRAINT ... FOREIGN KEY ... (capture ON DELETE/ON UPDATE)
Test->>Test: Compare parsed actions vs entity-declared actions and fail on mismatch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #105 +/- ##
=======================================
Coverage 57.31% 57.31%
=======================================
Files 260 260
Lines 16967 16967
Branches 6492 6519 +27
=======================================
Hits 9725 9725
Misses 7129 7129
Partials 113 113
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
docs/arkitekturbeskrivning-kravhantering.md (1)
1103-1109: Document required AI reasoning capabilities instead of pinned model versions.Line 1107 hard-codes specific model names: "Claude Opus 4" is already deprecated as of April 2026. Even though "GPT-5.4" and "Gemini 2.5 Pro" are currently available, model versions advance frequently. Define the required capabilities (e.g., extended reasoning, multi-step task handling, large context windows) and note vendor models as examples that require periodic revalidation rather than listing specific version names.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/arkitekturbeskrivning-kravhantering.md` around lines 1103 - 1109, Replace the hard-coded model names in the paragraph that lists "Claude Opus 4, GPT-5.4 or Gemini 2.5 Pro (med thinking)" with a capability-driven requirement: specify required capabilities such as extended multi-step reasoning, large context-window support, robust code-editing and refactoring abilities, deterministic reproducibility for multi-step workflows, and safety/guardrails for code generation; then append a short examples sentence stating that vendor models (e.g., "GPT-5.4" or "Gemini 2.5 Pro") have met these criteria historically but should be periodically revalidated rather than pinned to fixed versions. Ensure the change updates the recommendation language around minimum levels to describe capabilities and validation cadence instead of fixed model versions.tests/unit/entities-migration-fk-actions.test.ts (3)
37-53: Optional: throw whenUP_STATEMENTSis missing instead of returning''.If a future migration is hand-authored without a
const UP_STATEMENTS = [...]block (or names the array differently),readUpStatementssilently returns an empty string, and any FK in that migration becomes invisible to the drift guard. Failing loudly here keeps the assertion meaningful as the migration set grows.♻️ Suggested change
function readUpStatements(file: string): string { const source = readFileSync(join(migrationsRoot, file), 'utf8') const start = source.indexOf('const UP_STATEMENTS') - if (start === -1) return '' + if (start === -1) { + throw new Error(`${file}: no 'const UP_STATEMENTS' declaration found`) + } const open = source.indexOf('[', start) - if (open === -1) return '' + if (open === -1) { + throw new Error(`${file}: 'UP_STATEMENTS' array opener not found`) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/entities-migration-fk-actions.test.ts` around lines 37 - 53, The helper readUpStatements currently returns an empty string when it can't find the 'const UP_STATEMENTS' marker or the opening '[' which hides missing/renamed migration arrays; update readUpStatements to throw a descriptive Error (including the filename argument) when start === -1 or open === -1 so the test fails loudly if a migration lacks the expected const UP_STATEMENTS = [...] block, and keep the existing parsing logic for the bracket depth handling and final fallback only for legitimate bracket parsing errors.
23-29: Optional: discover migrations from disk so future files aren't accidentally excluded.Hardcoding
MIGRATION_FILESmeans a developer who adds0004_*.mjsand forgets to update this list will silently bypass the drift guard for any FK that migration touches. Reading the directory keeps the test future-proof.♻️ Suggested change
-const MIGRATION_FILES = [ - '0001_initial_sqlserver.mjs', - '0002_requirement_version_revision_token.mjs', - '0003_explicit_fk_actions.mjs', -] as const - const migrationsRoot = join(process.cwd(), 'typeorm', 'migrations') + +const MIGRATION_FILES = readdirSync(migrationsRoot) + .filter(f => /^\d{4}_.+\.mjs$/.test(f)) + .sort()(Add
readdirSyncto thenode:fsimport.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/entities-migration-fk-actions.test.ts` around lines 23 - 29, Replace the hardcoded MIGRATION_FILES array by reading the migrations directory at runtime: import readdirSync from 'node:fs', list files in the directory referenced by migrationsRoot (the existing join(process.cwd(), 'typeorm', 'migrations')), filter for the migration file pattern (e.g., /^\d{4}_.*\.mjs$/) and sort them to produce a stable MIGRATION_FILES list used by the tests (keep migrationsRoot and the rest of the test logic unchanged).
43-52: Heads-up: bracket-depth scan works only because SQL Server delimited identifiers are always balanced.The depth counter walks raw JS source and counts every
[/], including those inside the SQL string literals ([table_name],[fk_…]). It happens to work because every such bracket comes in matched pairs within the same string. If a future migration ever embeds a literal]inside a string (e.g. via an escaped identifier or comment), this loop will terminate early and silently truncateUP_STATEMENTS. Worth a short comment so a future maintainer doesn't introduce that hazard.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/entities-migration-fk-actions.test.ts` around lines 43 - 52, Add a short in-code comment above the bracket-depth scan loop (the block using variables depth, open, source and returning source.slice(...)) documenting the assumption that SQL Server delimited identifiers are always balanced and warning that the loop counts every '[' and ']' (including those inside SQL string literals), which could prematurely terminate the scan and truncate UP_STATEMENTS if a literal ']' were embedded; keep the implementation as-is but record this hazard for future maintainers so they don’t accidentally introduce such a case.typeorm/migrations/0003_explicit_fk_actions.mjs (1)
188-207: Optional: factor the identical try/catch loop into a single helper.
up()anddown()are byte-identical apart from the array name. A singlerunStatements(queryRunner, statements)helper would remove the duplication and make future migrations easier to follow.♻️ Suggested refactor
+async function runStatements(queryRunner, statements) { + for (const sql of statements) { + try { + await queryRunner.query(sql) + } catch (err) { + err.message = `${err.message}\n--- failing statement:\n${sql}` + throw err + } + } +} + export class ExplicitFkActions1714000000000 { name = 'ExplicitFkActions1714000000000' - async up(queryRunner) { - for (const sql of UP_STATEMENTS) { - try { - await queryRunner.query(sql) - } catch (err) { - err.message = `${err.message}\n--- failing statement:\n${sql}` - throw err - } - } - } - async down(queryRunner) { - for (const sql of DOWN_STATEMENTS) { - try { - await queryRunner.query(sql) - } catch (err) { - err.message = `${err.message}\n--- failing statement:\n${sql}` - throw err - } - } - } + async up(queryRunner) { + await runStatements(queryRunner, UP_STATEMENTS) + } + async down(queryRunner) { + await runStatements(queryRunner, DOWN_STATEMENTS) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@typeorm/migrations/0003_explicit_fk_actions.mjs` around lines 188 - 207, The up() and down() methods duplicate the same try/catch loop over different arrays (UP_STATEMENTS and DOWN_STATEMENTS); extract that logic into a helper function runStatements(queryRunner, statements) that iterates the statements, runs await queryRunner.query(sql) and on error augments err.message with the failing SQL then rethrows; replace the body of both up() and down() to simply call await runStatements(queryRunner, UP_STATEMENTS) and await runStatements(queryRunner, DOWN_STATEMENTS) respectively so the identical code is centralized and reused.
🤖 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/arkitekturbeskrivning-kravhantering.md`:
- Around line 1103-1109: Replace the hard-coded model names in the paragraph
that lists "Claude Opus 4, GPT-5.4 or Gemini 2.5 Pro (med thinking)" with a
capability-driven requirement: specify required capabilities such as extended
multi-step reasoning, large context-window support, robust code-editing and
refactoring abilities, deterministic reproducibility for multi-step workflows,
and safety/guardrails for code generation; then append a short examples sentence
stating that vendor models (e.g., "GPT-5.4" or "Gemini 2.5 Pro") have met these
criteria historically but should be periodically revalidated rather than pinned
to fixed versions. Ensure the change updates the recommendation language around
minimum levels to describe capabilities and validation cadence instead of fixed
model versions.
In `@tests/unit/entities-migration-fk-actions.test.ts`:
- Around line 37-53: The helper readUpStatements currently returns an empty
string when it can't find the 'const UP_STATEMENTS' marker or the opening '['
which hides missing/renamed migration arrays; update readUpStatements to throw a
descriptive Error (including the filename argument) when start === -1 or open
=== -1 so the test fails loudly if a migration lacks the expected const
UP_STATEMENTS = [...] block, and keep the existing parsing logic for the bracket
depth handling and final fallback only for legitimate bracket parsing errors.
- Around line 23-29: Replace the hardcoded MIGRATION_FILES array by reading the
migrations directory at runtime: import readdirSync from 'node:fs', list files
in the directory referenced by migrationsRoot (the existing join(process.cwd(),
'typeorm', 'migrations')), filter for the migration file pattern (e.g.,
/^\d{4}_.*\.mjs$/) and sort them to produce a stable MIGRATION_FILES list used
by the tests (keep migrationsRoot and the rest of the test logic unchanged).
- Around line 43-52: Add a short in-code comment above the bracket-depth scan
loop (the block using variables depth, open, source and returning
source.slice(...)) documenting the assumption that SQL Server delimited
identifiers are always balanced and warning that the loop counts every '[' and
']' (including those inside SQL string literals), which could prematurely
terminate the scan and truncate UP_STATEMENTS if a literal ']' were embedded;
keep the implementation as-is but record this hazard for future maintainers so
they don’t accidentally introduce such a case.
In `@typeorm/migrations/0003_explicit_fk_actions.mjs`:
- Around line 188-207: The up() and down() methods duplicate the same try/catch
loop over different arrays (UP_STATEMENTS and DOWN_STATEMENTS); extract that
logic into a helper function runStatements(queryRunner, statements) that
iterates the statements, runs await queryRunner.query(sql) and on error augments
err.message with the failing SQL then rethrows; replace the body of both up()
and down() to simply call await runStatements(queryRunner, UP_STATEMENTS) and
await runStatements(queryRunner, DOWN_STATEMENTS) respectively so the identical
code is centralized and reused.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6d155db5-9a5e-4f15-bf48-40439ec46c55
📒 Files selected for processing (25)
.github/instructions/markdown.instructions.mdcspell.jsoncdocs/arkitekturbeskrivning-kravhantering.mddocs/database-schema.mdlib/typeorm/entities/deviation.tslib/typeorm/entities/improvement-suggestion.tslib/typeorm/entities/package-local-requirement-deviation.tslib/typeorm/entities/package-local-requirement-norm-reference.tslib/typeorm/entities/package-local-requirement-usage-scenario.tslib/typeorm/entities/package-local-requirement.tslib/typeorm/entities/package-needs-reference.tslib/typeorm/entities/quality-characteristic.tslib/typeorm/entities/requirement-area.tslib/typeorm/entities/requirement-package-item.tslib/typeorm/entities/requirement-package.tslib/typeorm/entities/requirement-status-transition.tslib/typeorm/entities/requirement-version-norm-reference.tslib/typeorm/entities/requirement-version-usage-scenario.tslib/typeorm/entities/requirement-version.tslib/typeorm/entities/requirement.tslib/typeorm/entities/usage-scenario.tsscripts/__tests__/db-sqlserver-admin.test.mjsscripts/db-sqlserver-admin.mjstests/unit/entities-migration-fk-actions.test.tstypeorm/migrations/0003_explicit_fk_actions.mjs
…ties in migration workflows
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/entities-migration-fk-actions.test.ts (1)
23-27: Hardcoded migration list will silently miss future migrations.
MIGRATION_FILESis a manually maintained allow‑list. When0004_*.mjs(or later) is added, this drift guard will keep passing while no longer validating the latest FK state — exactly the failure mode this test exists to prevent. Consider discovering files dynamically and sorting them, so the guard always reflects the on-disk migration history.♻️ Proposed refactor
-import { readFileSync } from 'node:fs' +import { readdirSync, readFileSync } from 'node:fs' import { join } from 'node:path' ... -const MIGRATION_FILES = [ - '0001_initial_sqlserver.mjs', - '0002_requirement_version_revision_token.mjs', - '0003_explicit_fk_actions.mjs', -] as const - const migrationsRoot = join(process.cwd(), 'typeorm', 'migrations') + +const MIGRATION_FILES = readdirSync(migrationsRoot) + .filter(f => /^\d{4}_.*\.mjs$/.test(f)) + .sort()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/entities-migration-fk-actions.test.ts` around lines 23 - 27, The hardcoded MIGRATION_FILES array will miss future migrations; replace it with a dynamic discovery that reads the migrations directory (use fs.readdirSync) and filters for migration filenames (e.g. matching /^\d{4}_.*\.mjs$/), then sort them lexicographically to preserve chronological order and use that sorted list in place of MIGRATION_FILES so the test (entities-migration-fk-actions.test / MIGRATION_FILES) always validates the on-disk migration history.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/entities-migration-fk-actions.test.ts`:
- Around line 51-66: The bracket-depth scan currently falls through to "return
source.slice(open + 1)" if the closing ']' is never found, which silently
returns the rest of the file; change that fallback to throw a clear error
instead so malformed input fails loudly: in the function containing the loop
(the code that uses variables source, open, depth and the '['/']' scan) replace
the final "return source.slice(open + 1)" with a thrown Error (include context
such as the open index and maybe a short message like "Unterminated '[' starting
at index {open}") so tests will surface malformed migrations rather than
continuing with a truncated UP_STATEMENTS window.
---
Nitpick comments:
In `@tests/unit/entities-migration-fk-actions.test.ts`:
- Around line 23-27: The hardcoded MIGRATION_FILES array will miss future
migrations; replace it with a dynamic discovery that reads the migrations
directory (use fs.readdirSync) and filters for migration filenames (e.g.
matching /^\d{4}_.*\.mjs$/), then sort them lexicographically to preserve
chronological order and use that sorted list in place of MIGRATION_FILES so the
test (entities-migration-fk-actions.test / MIGRATION_FILES) always validates the
on-disk migration history.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa8e999f-4343-45a6-8c36-1cb9733f2c4f
📒 Files selected for processing (3)
cspell.jsoncdocs/arkitekturbeskrivning-kravhantering.mdtests/unit/entities-migration-fk-actions.test.ts
✅ Files skipped from review due to trivial changes (1)
- cspell.jsonc
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/arkitekturbeskrivning-kravhantering.md
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unit/entities-migration-fk-actions.test.ts (1)
150-151: MakeADD_CONSTRAINT_REwhitespace-tolerant.The regex hard-codes single spaces between SQL keywords (
ALTER TABLE,ADD CONSTRAINT,FOREIGN KEY,REFERENCES). Any future migration that splits an FK across lines or uses tabs/multi-space indentation would be silently skipped by the parser; the only thing rescuing the test from a false pass is the presence-check on Line 186. Loosening the inter-keyword whitespace to\s+makes the guard robust to formatting variation while still anchoring on the same SQL shape.♻️ Proposed fix
-const ADD_CONSTRAINT_RE = - /ALTER TABLE \[[a-z_][a-z0-9_]*\] ADD CONSTRAINT \[([a-z_][a-z0-9_]*)\] FOREIGN KEY \([^)]*\) REFERENCES \[[a-z_][a-z0-9_]*\] \([^)]*\)([^;]*);/gi +const ADD_CONSTRAINT_RE = + /ALTER\s+TABLE\s+\[[a-z_][a-z0-9_]*\]\s+ADD\s+CONSTRAINT\s+\[([a-z_][a-z0-9_]*)\]\s+FOREIGN\s+KEY\s*\([^)]*\)\s+REFERENCES\s+\[[a-z_][a-z0-9_]*\]\s*\([^)]*\)([^;]*);/gi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/entities-migration-fk-actions.test.ts` around lines 150 - 151, The ADD_CONSTRAINT_RE pattern is brittle because it requires single spaces between SQL keywords; update the regex for ADD_CONSTRAINT_RE to allow flexible whitespace by replacing hard-coded spaces between keywords (e.g., "ALTER TABLE", "ADD CONSTRAINT", "FOREIGN KEY", "REFERENCES") with \s+ (and where appropriate allow optional whitespace around punctuation), so the matcher tolerates tabs/newlines/multiple spaces while preserving the same capture groups and flags used in the current constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unit/entities-migration-fk-actions.test.ts`:
- Around line 150-151: The ADD_CONSTRAINT_RE pattern is brittle because it
requires single spaces between SQL keywords; update the regex for
ADD_CONSTRAINT_RE to allow flexible whitespace by replacing hard-coded spaces
between keywords (e.g., "ALTER TABLE", "ADD CONSTRAINT", "FOREIGN KEY",
"REFERENCES") with \s+ (and where appropriate allow optional whitespace around
punctuation), so the matcher tolerates tabs/newlines/multiple spaces while
preserving the same capture groups and flags used in the current constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 59c6d4c0-b4a4-4059-8302-9b3a3e109507
📒 Files selected for processing (1)
tests/unit/entities-migration-fk-actions.test.ts
17 tasks
17 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
1 participant
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
Screenshots (if applicable)
Related Issues
Type of Change
Testing
npm run checkpasses locallyChecklist
Checklist
This change is