Skipped set/drop nullable migration queries when not necessary #24533

kevinansfield · 2025-07-29T12:00:51Z

no issue

added pre-condition checks to createDropNullableMigration and createSetNullableMigration migration utils that skips expensive ALTER TABLE queries when the database is already in the correct state

coderabbitai · 2025-07-29T12:00:58Z

Walkthrough

The changes introduce and enhance utility functions for handling nullable column migrations in the database schema migration utilities. Two new asynchronous helper functions, isColumnNullable and isColumnNotNullable, are added to check a column's nullability status for both SQLite and other SQL clients. The migration functions createSetNullableMigration and createDropNullableMigration now use these helpers to skip redundant operations if the column is already in the desired state, logging warnings when such skips occur or when status checks fail. Integration and unit tests are added to verify these behaviors, covering transactional integrity, logging, foreign key handling, and edge cases for different databases.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~40 minutes

Complexity: The changes involve multiple new utility functions and enhanced migration logic with conditional flows, wrapped in try-catch for backward compatibility. The addition of comprehensive integration and unit tests spanning multiple database engines and edge cases increases the scope and complexity. Review requires detailed examination of database schema queries, migration control flow, logging behavior, and test correctness across different environments.

Note

⚡️ Unit Test Generation is now available in beta!

Learn more here, or try it out under "Finishing Touches" below.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 07b16da and 2522184.

📒 Files selected for processing (1)

ghost/core/test/unit/server/data/migrations/utils.test.js (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

ghost/core/test/unit/server/data/migrations/utils.test.js

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: Setup

✨ Finishing Touches

📝 Generate Docstrings

🧪 Generate unit tests

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch skip-drop-null-when-not-needed

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.

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Explain this complex logic.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai explain this code block.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and explain its main purpose.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai generate docstrings to generate docstrings for this PR.
@coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
@coderabbitai generate unit tests to generate unit tests for this PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

no issue - added pre-condition checks to `createDropNullableMigration` and `createSetNullableMigration` migration utils that skips expensive `ALTER TABLE` queries when the database is already in the correct state

coderabbitai

Actionable comments posted: 3

🧹 Nitpick comments (1)

ghost/core/test/integration/migrations/nullable-utils.test.js (1)

93-119: Consider reusing the actual implementation functions to avoid duplication.

These helper functions duplicate the logic from ghost/core/core/server/data/migrations/utils/schema.js. This creates a maintenance burden and could lead to tests passing even if the actual implementation changes.

Consider importing and using the actual isColumnNullable and isColumnNotNullable functions from the schema utils, or at least extract these helpers to a shared test utility file.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ac10801 and 07b16da.

📒 Files selected for processing (3)

ghost/core/core/server/data/migrations/utils/schema.js (4 hunks)
ghost/core/test/integration/migrations/nullable-utils.test.js (1 hunks)
ghost/core/test/unit/server/data/migrations/utils.test.js (1 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: cathysarisky
PR: TryGhost/Ghost#0
File: :0-0
Timestamp: 2025-05-20T21:08:21.026Z
Learning: In the Ghost project, translation files (ghost/i18n/locales/*/*.json) commonly have blank values for translations, and this is normal behavior that should not be flagged in reviews. These empty translations will be filled in separate PRs.

ghost/core/test/integration/migrations/nullable-utils.test.js (3)

Learnt from: mike182uk
PR: #22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file apps/admin-x-activitypub/test/unit/utils/pending-activity.ts.

Learnt from: mike182uk
PR: #22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in apps/admin-x-activitypub/test/unit/utils/pending-activity.ts, which covers generatePendingActivityId, isPendingActivity, and generatePendingActivity functions.

Learnt from: ErisDS
PR: #23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response body field is empty while the actual CSV data is provided through the text field. Tests should use expectEmptyBody() and then validate the CSV content via .expect(({text}) => ...) - this is not contradictory behavior.

ghost/core/core/server/data/migrations/utils/schema.js (1)

Learnt from: mike182uk
PR: #22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in apps/admin-x-activitypub/test/unit/utils/pending-activity.ts, which covers generatePendingActivityId, isPendingActivity, and generatePendingActivity functions.

ghost/core/test/unit/server/data/migrations/utils.test.js (3)

Learnt from: mike182uk
PR: #22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in the Ghost ActivityPub module are covered by tests in the file apps/admin-x-activitypub/test/unit/utils/pending-activity.ts.

Learnt from: mike182uk
PR: #22471
File: apps/admin-x-activitypub/src/utils/pending-activity.ts:13-71
Timestamp: 2025-03-13T09:00:20.205Z
Learning: The pending activity utilities in Ghost's ActivityPub module are thoroughly tested in apps/admin-x-activitypub/test/unit/utils/pending-activity.ts, which covers generatePendingActivityId, isPendingActivity, and generatePendingActivity functions.

Learnt from: ErisDS
PR: #23588
File: ghost/core/test/e2e-api/admin/backup.test.js:136-148
Timestamp: 2025-05-29T10:37:26.369Z
Learning: In the Ghost test framework, when testing CSV exports via the admin API, the response body field is empty while the actual CSV data is provided through the text field. Tests should use expectEmptyBody() and then validate the CSV content via .expect(({text}) => ...) - this is not contradictory behavior.

🧬 Code Graph Analysis (2)

ghost/core/core/server/data/migrations/utils/schema.js (2)

ghost/core/test/integration/migrations/nullable-utils.test.js (16)

knex (19-19)

knex (60-60)

knex (94-94)

knex (108-108)

logging (5-5)

isNotNullable (286-286)

response (97-97)

response (101-101)

response (111-111)

response (115-115)

response (291-291)

columnInfo (98-98)

columnInfo (102-102)

columnInfo (112-112)

columnInfo (116-116)

columnInfo (292-292)

ghost/core/test/unit/server/data/migrations/utils.test.js (14)

knex (174-181)

knex (248-255)

knex (292-292)

knex (324-324)

knex (384-384)

knex (401-401)

knex (417-417)

knex (434-434)

knex (453-453)

knex (580-580)

knex (611-611)

knex (652-652)

response (738-738)

columnInfo (739-739)

ghost/core/test/unit/server/data/migrations/utils.test.js (3)

ghost/core/test/integration/migrations/nullable-utils.test.js (25)

knex (19-19)

knex (60-60)

knex (94-94)

knex (108-108)

response (97-97)

response (101-101)

response (111-111)

response (115-115)

response (291-291)

columnInfo (98-98)

columnInfo (102-102)

columnInfo (112-112)

columnInfo (116-116)

columnInfo (292-292)

migration (123-123)

migration (149-149)

migration (170-172)

migration (199-199)

migration (225-225)

migration (247-249)

migration (272-272)

migration (298-298)

utils (7-7)

should (1-1)

sinon (2-2)

ghost/core/core/server/data/migrations/utils/schema.js (10)

response (181-181)

response (185-185)

response (202-202)

response (206-206)

columnInfo (182-182)

columnInfo (186-186)

columnInfo (203-203)

columnInfo (207-207)

require (2-2)

require (5-5)

ghost/core/core/server/data/migrations/utils/settings.js (1)

require (4-4)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)

GitHub Check: Ghost-CLI tests
GitHub Check: E2E tests
GitHub Check: Acceptance tests (Node 22.13.1, sqlite3)
GitHub Check: Acceptance tests (Node 22.13.1, mysql8)
GitHub Check: Legacy tests (Node 22.13.1, sqlite3)
GitHub Check: Legacy tests (Node 22.13.1, mysql8)
GitHub Check: Unit tests (Node 22.13.1)
GitHub Check: i18n
GitHub Check: Lint

🔇 Additional comments (1)

ghost/core/core/server/data/migrations/utils/schema.js (1)

110-110: Minor: Make logging consistent between up and down migrations.

The down migration includes the foreign key status in the log message, but the up migration doesn't log when starting the operation after the checks pass.

Consider adding a matching info log in the up migration after the checks:
             }
 
+            logging.info(`Setting nullable: ${table}.${column}`);
-            logging.info(`Setting nullable: ${table}.${column}`);
             await commands.setNullable(table, column, knex);
Wait, I see the log is already there at line 89. Please disregard this comment.

ghost/core/core/server/data/migrations/utils/schema.js

ghost/core/test/unit/server/data/migrations/utils.test.js

kevinansfield force-pushed the skip-drop-null-when-not-needed branch from bce0956 to df9890c Compare July 30, 2025 13:00

Skipped set/drop nullable migration queries when not necessary

07b16da

no issue - added pre-condition checks to `createDropNullableMigration` and `createSetNullableMigration` migration utils that skips expensive `ALTER TABLE` queries when the database is already in the correct state

kevinansfield force-pushed the skip-drop-null-when-not-needed branch from df9890c to 07b16da Compare July 30, 2025 16:48

kevinansfield changed the title ~~Skipped alter table in createDropNullableMigration when not necessary~~ Skipped set/drop nullable migration queries when not necessary Jul 30, 2025

kevinansfield requested a review from jloh July 30, 2025 16:49

kevinansfield marked this pull request as ready for review July 30, 2025 16:50

kevinansfield requested a review from allouis July 30, 2025 16:51

coderabbitai bot reviewed Jul 30, 2025

View reviewed changes

ghost/core/core/server/data/migrations/utils/schema.js Show resolved Hide resolved

ghost/core/test/unit/server/data/migrations/utils.test.js Outdated Show resolved Hide resolved

ghost/core/test/unit/server/data/migrations/utils.test.js Outdated Show resolved Hide resolved

allouis approved these changes Jul 31, 2025

View reviewed changes

address 🐰 feedback

2522184

jloh approved these changes Jul 31, 2025

View reviewed changes

ErisDS added the ready-to-merge Blocked by code freeze label Aug 4, 2025

kevinansfield merged commit 8b66ede into main Aug 11, 2025
50 checks passed

kevinansfield deleted the skip-drop-null-when-not-needed branch August 11, 2025 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Skipped set/drop nullable migration queries when not necessary #24533

Skipped set/drop nullable migration queries when not necessary #24533

Uh oh!

kevinansfield commented Jul 29, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Jul 29, 2025 •

edited

Loading

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Skipped set/drop nullable migration queries when not necessary #24533

Skipped set/drop nullable migration queries when not necessary #24533

Uh oh!

Conversation

kevinansfield commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Estimated code review effort

Chat

Support

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kevinansfield commented Jul 29, 2025 •

edited

Loading

coderabbitai bot commented Jul 29, 2025 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)