Conversation
WalkthroughThe pull request expands documentation for the database migration skill across three files. The skill description is updated to include database views alongside tables and columns. New sections document how views are defined in a shared views module and created during initialization, along with clarification on test database setup behavior. An examples section is added showing migration syntax for creating and modifying views using non-transactional migrations. A rules section introduces requirements for using Knex parameter bindings instead of string interpolation and maintaining database-agnostic migrations compatible with both MySQL and SQLite environments. 🚥 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 |
…guidance Added documentation for database views (views.js, init path, migration patterns), test DB initialization behavior, Knex parameter bindings, and MySQL/SQLite cross-database compatibility requirements
rob-ghost
force-pushed
the
chore/improve-create-database-migration-skill
branch
from
36f693e to
7aee8b2
Compare
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 @.agents/skills/create-database-migration/examples.md:
- Around line 26-35: The example risks a runtime error when views.find(...)
returns undefined and also needs clearer wording about bindings vs SQL
fragments: add a guard after computing viewDef (the result of views.find) to
throw or log a clear error if viewDef is falsy before using viewDef.body, and
update the documentation text in rules.md to state that parameter bindings (??
and ?) only apply to identifiers/values and cannot be used for SQL fragments so
trusted static SQL (like viewDef.body) must be concatenated into the query;
locate the example in createNonTransactionalMigration's up function and the
views/viewDef usage to apply the guard and update the rules wording accordingly.
In @.agents/skills/create-database-migration/SKILL.md:
- Around line 22-33: The doc incorrectly claims that 1-create-tables.js creates
views from views.js; update SKILL.md to either remove or correct that assertion
and instead state the actual behavior: confirm whether views are created during
init and, if not, explain that version migrations (or a different initializer)
are required to create views; reference the specific symbols views.js,
1-create-tables.js, schema.js, testUtils.setup('roles') and
knexMigrator.init({skip: 2}) in the updated text and/or add a precise link to
the initializer that actually performs view creation so readers aren’t misled.
- Line 3: Step 4 in SKILL.md currently directs unconditional updates to
schema.js even though the skill now explicitly includes views; update the text
so it makes the update conditional: instruct contributors to modify schema.js
when the migration changes tables/columns/indexes/settings, and to modify
views.js when the migration adds/changes database views (mention both filenames
schema.js and views.js and the step number "Step 4" so reviewers can locate it),
ensuring the guidance clearly distinguishes table/schema changes vs. view
changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7def24ca-017f-4a00-8697-ce1e24777b3a
📒 Files selected for processing (3)
.agents/skills/create-database-migration/SKILL.md.agents/skills/create-database-migration/examples.md.agents/skills/create-database-migration/rules.md
| const VIEW_NAME = 'my_view_name'; | ||
| const views = require('../../../schema/views'); | ||
| const viewDef = views.find(v => v.name === VIEW_NAME); | ||
|
|
||
| module.exports = createNonTransactionalMigration( | ||
| async function up(knex) { | ||
| logging.info(`Creating view: ${VIEW_NAME}`); | ||
| await knex.raw('DROP VIEW IF EXISTS ??', [VIEW_NAME]); | ||
| await knex.raw('CREATE VIEW ?? AS ' + viewDef.body, [VIEW_NAME]); | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Knex, what is the recommended safe pattern for CREATE VIEW statements when the view name is dynamic but the SELECT body is a trusted static SQL string? Can the full CREATE VIEW body be parameter-bound, or is concatenating a trusted SQL fragment the expected approach?
💡 Result:
In Knex, bindings are meant for values (? / :name) and identifiers (?? / :name:), not for arbitrary SQL fragments like an entire SELECT ... statement body. Knex will treat ? as a value placeholder (i.e., it becomes a driver parameter like $1), which cannot stand in for a chunk of executable SQL. (knexjs.org)
Recommended safe pattern (dynamic view name, trusted static SELECT string):
Option A (preferred when available): schema builder
Use createView / createViewOrReplace, and pass the SELECT as a raw expression if it’s truly trusted/static:
await knex.schema.createViewOrReplace(viewName, (view) => {
view.as(knex.raw(TRUSTED_SELECT_SQL));
});Knex will generate create (or replace) view "viewName" as select .... (knexjs.org)
Option B: raw DDL with identifier binding + trusted concatenation
Bind only the view name as an identifier, and concatenate the trusted SELECT text:
await knex.raw(`CREATE OR REPLACE VIEW ?? AS ${TRUSTED_SELECT_SQL}`, [viewName]);This uses ?? for safe identifier escaping (view name), while accepting that the SELECT body is injected as a trusted SQL fragment. (knexjs.org)
What not to do
// NOT what you want: the SELECT becomes a *value parameter*, not executable SQL
await knex.raw('CREATE VIEW ?? AS ?', [viewName, TRUSTED_SELECT_SQL]);Bottom line: you generally cannot (and shouldn’t expect to) parameter-bind the full CREATE VIEW ... AS <select> body as a single parameter; the expected approach is bind identifiers/values, and concatenate only trusted SQL fragments when you truly need to inject SQL text. (knexjs.org)
Citations:
- 1: https://knexjs.org/guide/raw.html
- 2: https://knexjs.org/guide/schema-builder.html
- 3: https://knexjs.org/guide/raw.html
- 4: https://knexjs.org/guide/raw.html
🏁 Script executed:
# Find and examine rules.md
fd -t f 'rules.md' .agents/
# Also check the examples.md file to see full context and any existing guards
head -50 .agents/skills/create-database-migration/examples.md
# Look for views.js schema to understand the source of viewDef
fd -t f 'views.js' schema/Repository: TryGhost/Ghost
Length of output: 2010
🏁 Script executed:
# Check both examples.md and rules.md to see if they're separate files
fd -t f -i 'examples.md\|rules.md' .agents/
# Look for the "always use bindings" rule mentioned in the review
rg -i "always use binding" .agents/
# Search for binding-related rules in the .agents directory
rg -i "binding" .agents/skills/create-database-migration/ -A 2 -B 2
# Get the full content of rules.md to understand all rules
cat -n .agents/skills/create-database-migration/rules.mdRepository: TryGhost/Ghost
Length of output: 4578
🏁 Script executed:
# Check if there are any guards or comments in the full examples.md file
cat -n .agents/skills/create-database-migration/examples.md
# Search for viewDef usage patterns in actual migrations to see if guards are used
rg -i "viewDef" --type js -A 3 -B 3
# Check the actual schema/views file to understand the data structure
find . -path "*/schema/views.js" -type f | head -5Repository: TryGhost/Ghost
Length of output: 2149
🏁 Script executed:
# Check if there's a real migration example to see if guards are used
cat -n ghost/core/core/server/data/migrations/versions/6.27/2026-04-08-10-56-56-add-members-effective-subscriptions-view.js 2>/dev/null | head -50
# If that doesn't work, find the file
find . -name "*add-members-effective-subscriptions-view*" -type fRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Search for actual view migrations in the codebase
find . -path "*/migrations/versions/*" -name "*view*" -type f | head -10
# Also search for viewDef usage in migrations
rg "viewDef" --type js -l
# Search for how views.find is used in migrations
rg "views\.find" --type js -A 5 -B 2Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Find the schema/views.js file to understand the data structure
find . -name "views.js" -path "*/schema/*" -type f
# Look at migration directory structure
ls -la ghost/core/core/server/data/migrations/versions/ | head -20
# Search more broadly for any viewDef or view-related migrations
rg "CREATE VIEW" --type js -l | head -5Repository: TryGhost/Ghost
Length of output: 1133
🏁 Script executed:
# Search for CREATE VIEW usage in all JS files
rg "CREATE VIEW" -A 2 -B 2 --type js | head -40
# Search for view migrations more broadly
find ghost/core/core/server/data/migrations/versions -name "*.js" -exec grep -l "CREATE VIEW" {} \; | head -5
# Check if schema/views.js exists
find . -type f -name "views.js" | grep -v node_modulesRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
# Check what files exist in the ghost/core/core/server/data/schema directory
ls -la ghost/core/core/server/data/schema/ 2>/dev/null || echo "Directory not found"
# Search for any file containing "views" in the data directory
find ghost/core/core/server/data -name "*view*" -type f 2>/dev/null
# Look for the schema directory mentioned in the example
find . -type d -name "schema" | grep -v node_modules | head -5
# Check if the example file exists and check around line 34
sed -n '20,45p' .agents/skills/create-database-migration/examples.mdRepository: TryGhost/Ghost
Length of output: 1650
🏁 Script executed:
# Verify if views.js exists in the schema directory
ls -la ghost/core/core/server/data/schema/views.js 2>&1
# Search for any views.js file in the entire repo
find . -name "views.js" -type f 2>/dev/null | head -10
# Check if the migration directory 6.27 exists
ls -la ghost/core/core/server/data/migrations/versions/6.27/ 2>&1 | head -20
# Search for any CREATE VIEW migrations that actually exist
grep -r "CREATE VIEW" ghost/core/core/server/data/migrations/versions --include="*.js" | head -5Repository: TryGhost/Ghost
Length of output: 707
Clarify rule wording and add guard for missing viewDef.
The example pattern (binding view name with ??, concatenating the trusted viewDef.body as SQL text) is correct and follows Knex's recommended approach—SQL fragments cannot be parameter-bound. However, the rule wording in rules.md (lines 35-39) is unclear and may mislead contributors. Clarify that bindings apply to values and identifiers only, not SQL fragments; concatenating trusted static SQL is the expected pattern.
Additionally, line 28 calls views.find() without guarding against a missing result. Add a check before accessing viewDef.body on line 34 to prevent runtime errors if the view definition is not found.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/create-database-migration/examples.md around lines 26 - 35,
The example risks a runtime error when views.find(...) returns undefined and
also needs clearer wording about bindings vs SQL fragments: add a guard after
computing viewDef (the result of views.find) to throw or log a clear error if
viewDef is falsy before using viewDef.body, and update the documentation text in
rules.md to state that parameter bindings (?? and ?) only apply to
identifiers/values and cannot be used for SQL fragments so trusted static SQL
(like viewDef.body) must be concatenated into the query; locate the example in
createNonTransactionalMigration's up function and the views/viewDef usage to
apply the guard and update the rules wording accordingly.
| --- | ||
| name: Create database migration | ||
| description: Create a database migration to add a table, add columns to an existing table, add a setting, or otherwise change the schema of Ghost's MySQL database. Use this skill whenever the task involves modifying Ghost's database schema — including adding, removing, or renaming columns or tables, adding new settings, creating indexes, updating data, or any change that requires a migration file in ghost/core. Also use when the user references schema.js, knex-migrator, the migrations directory, or asks to "add a field" or "add a column" to any Ghost model/table. Even if the user frames it as a feature or Linear issue, if the implementation requires a schema change, this skill applies. | ||
| description: Create a database migration to add a table, add columns to an existing table, add a setting, create or modify a database view, or otherwise change the schema of Ghost's database. Use this skill whenever the task involves modifying Ghost's database schema — including adding, removing, or renaming columns, tables, or views, adding new settings, creating indexes, updating data, or any change that requires a migration file in ghost/core. Also use when the user references schema.js, views.js, knex-migrator, the migrations directory, or asks to "add a field", "add a column", or "create a view" on any Ghost model/table. Even if the user frames it as a feature or Linear issue, if the implementation requires a schema change, this skill applies. |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Scope expansion to views needs matching instruction updates.
Now that this skill explicitly covers views, step 4 should not read as unconditional schema.js updates. Make it conditional (schema.js for tables, views.js for views) to avoid contradictory guidance within the same document.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/create-database-migration/SKILL.md at line 3, Step 4 in
SKILL.md currently directs unconditional updates to schema.js even though the
skill now explicitly includes views; update the text so it makes the update
conditional: instruct contributors to modify schema.js when the migration
changes tables/columns/indexes/settings, and to modify views.js when the
migration adds/changes database views (mention both filenames schema.js and
views.js and the step number "Step 4" so reviewers can locate it), ensuring the
guidance clearly distinguishes table/schema changes vs. view changes.
| Database views are defined in `ghost/core/core/server/data/schema/views.js` and created during database initialization in `ghost/core/core/server/data/migrations/init/1-create-tables.js`. When adding or modifying a view: | ||
|
|
||
| 1. Define or update the view in `ghost/core/core/server/data/schema/views.js` — this is the single source of truth for all view definitions. | ||
| 2. Create a version migration that references the view definition from `views.js` (see examples). | ||
| 3. Views do NOT need entries in `schema.js`, `table-lists.js`, or the exporter test — they are not tables. | ||
| 4. The init step (`1-create-tables.js`) automatically creates all views defined in `views.js` after creating tables, so tests and fresh installations will have the view without running version migrations. | ||
|
|
||
| ## Test database initialization | ||
|
|
||
| Understanding the test DB init path is important: | ||
| - `testUtils.setup('roles')` calls `knexMigrator.init({skip: 2})` — this only runs `1-create-tables.js` (which creates tables and views). **Version migrations are NOT executed.** | ||
| - This means any schema object that only exists in a version migration will be missing in tests. Tables are covered because they're defined in `schema.js`. Views are covered because they're defined in `views.js` and created by `1-create-tables.js`. |
There was a problem hiding this comment.
The init-path claim for views appears incorrect and is likely misleading.
This section states that 1-create-tables.js creates views from views.js, but the provided implementation in ghost/core/core/server/data/migrations/init/1-create-tables.js (Lines 1-17 in the shared snippet) only creates missing tables from schema.js. If this doc stays as-is, contributors may assume views exist in tests/fresh init when they do not.
Please either update the docs to match current behavior or link the exact initializer that actually creates views.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.agents/skills/create-database-migration/SKILL.md around lines 22 - 33, The
doc incorrectly claims that 1-create-tables.js creates views from views.js;
update SKILL.md to either remove or correct that assertion and instead state the
actual behavior: confirm whether views are created during init and, if not,
explain that version migrations (or a different initializer) are required to
create views; reference the specific symbols views.js, 1-create-tables.js,
schema.js, testUtils.setup('roles') and knexMigrator.init({skip: 2}) in the
updated text and/or add a precise link to the initializer that actually performs
view creation so readers aren’t misled.
1 participant
The create-database-migration skill was missing guidance for several patterns that come up regularly when working with Ghost's database layer. This PR fills those gaps.
The most significant addition is documentation for database views. Views are defined in
views.jsand created during init in1-create-tables.js, but this workflow wasn't documented in the skill. The new "Database views" section in SKILL.md explains the full lifecycle — where to define views, how to write version migrations that reference the shared definition, and the important distinction that views don't need entries inschema.jsortable-lists.js. A corresponding example migration template was added to examples.md showing the correct pattern of importing fromviews.jsand usingknex.rawwith identifier bindings.A section on test database initialization was also added. Understanding that
testUtils.setup('roles')only runs1-create-tables.js(skipping version migrations) is critical for knowing why schema objects must be registered inschema.jsorviews.js— otherwise they won't exist in the test database.Finally, two new rules were added to rules.md: always use Knex parameter bindings (
?for values,??for identifiers) instead of string interpolation in raw queries, and ensure migrations are compatible with both MySQL and SQLite since Ghost uses MySQL in production but SQLite in tests.