Skip to content

fix(core): Report when migration patterns match no files - #5039

Open
alex-hahn wants to merge 1 commit into
vendurehq:masterfrom
alex-hahn:fix/5001-report-unmatched-migration-globs
Open

fix(core): Report when migration patterns match no files#5039
alex-hahn wants to merge 1 commit into
vendurehq:masterfrom
alex-hahn:fix/5001-report-unmatched-migration-globs

Conversation

@alex-hahn

@alex-hahn alex-hahn commented Jul 27, 2026

Copy link
Copy Markdown

Description

runMigrations() returns an empty array in two different situations:

  1. every migration has already been applied, and
  2. the configured migrations glob patterns matched no files at all.

Both CLI entry points derive their report from that array alone, so the second case is
presented as No pending migrations found. A database that is out of sync with the entity
schema is therefore indistinguishable from an up-to-date one, and vendure migrate --run
exits 0 having applied nothing.

The patterns are resolved relative to the current working directory, so this happens when the
command is run from an unexpected directory, or when the patterns point at compiled output
(e.g. dist/migrations/*.js) that has not been built.

Approach

The case is detectable from the migration classes TypeORM actually loaded (connection.migrations),
which is empty when the globs matched nothing. Where to report it needs some care:

  • Logging it directly does not work. log() in migrate.ts is a no-op while running from
    the CLI, because a spinner is active for the duration of the call and writing to stdout
    would corrupt it. That suppression is why the condition is currently invisible.
  • Throwing would break the default project template. The scaffold in
    packages/create/templates/vendure-config.hbs configures
    migrations: [path.join(__dirname, './migrations/*.+(js|ts)')] before any migration file
    exists, so a freshly created project would fail vendure migrate --run with a non-zero exit.

So the diagnostic is handed to the caller through a new optional
RunMigrationsOptions.onNoMigrationsFound callback, and both CLI entry points surface it in
place of the misleading default message. This is additive and non-breaking; the exit code is
deliberately unchanged.

If you would prefer a non-zero exit here, that is a one-line change — happy to adjust.

Tests

  • packages/core/src/migrate.spec.ts (new file) — 7 tests covering the message builder:
    loaded migrations, unconfigured/empty patterns, class-valued entries, the object form of the
    migrations option, and the reported patterns and cwd.
  • packages/cli/src/commands/migrate/migration-operations-reporting.spec.ts (new file) —
    3 tests covering what the CLI actually reports. Verified that the first fails without this
    change (expected 'No pending migrations found' to be 'No migration files matched…') while
    the other two stay green.

Full packages/cli suite passes (275 tests), and tsc --noEmit reports no new errors in
either package.

Fixes #5001


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

`runMigrations()` returns an empty array both when every migration has
already been applied and when the configured `migrations` glob patterns
matched no files at all. The CLI derives its report from that array
alone, so the second case is presented as "No pending migrations found"
— a database that is out of sync with the entity schema looks identical
to an up-to-date one.

The patterns are resolved relative to the current working directory, so
this happens when the command is run from an unexpected directory, or
when the patterns point at compiled output that has not been built.

Detect the case via the migration classes TypeORM actually loaded and
hand the diagnostic to the caller through a new optional
`RunMigrationsOptions.onNoMigrationsFound` callback. A callback is used
rather than logging directly because `log()` is a no-op while running
from the CLI, where a spinner is active for the duration of the call.
Both CLI entry points surface the message in place of the misleading
default.

The exit code is deliberately unchanged: a freshly scaffolded project
configures a `migrations` glob before any migration file exists, so
failing here would break the default project template.

Fixes vendurehq#5001
@vercel

vercel Bot commented Jul 27, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
vendure-storybook Ready Ready Preview, Comment Jul 27, 2026 6:05pm

Request Review

@vendure-ci-automation-bot

Copy link
Copy Markdown
Contributor


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@coderabbitai

coderabbitai Bot commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: f728b8ab-8719-4e9c-ae39-dffd91107c32

📥 Commits

Reviewing files that changed from the base of the PR and between 9d4dda1 and 0cf5eaa.

📒 Files selected for processing (6)
  • packages/cli/src/commands/migrate/migration-operations-reporting.spec.ts
  • packages/cli/src/commands/migrate/migration-operations.ts
  • packages/cli/src/commands/migrate/run-migration/run-migration.ts
  • packages/core/src/index.ts
  • packages/core/src/migrate.spec.ts
  • packages/core/src/migrate.ts

📝 Walkthrough

Walkthrough

The migration runner now accepts an optional callback for no-migration messages and provides a helper that reports unmatched configured patterns. The callback option is publicly exported. CLI migration commands capture the message and use it when no migrations are returned, while retaining the existing fallback for pending migrations being absent. New tests cover core message generation and CLI reporting outcomes.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly reflects the main change: reporting when migration glob patterns match no files.
Description check ✅ Passed The description covers the summary, approach, tests, and linked issue; only non-critical template sections are missing.
Linked Issues check ✅ Passed The PR adds detection and CLI reporting for unmatched migration globs, matching the linked issue's key reporting objective.
Out of Scope Changes check ✅ Passed All code changes stay within migration reporting, callback plumbing, exports, and tests; no unrelated scope stands out.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment @coderabbitai help to get the list of available commands.

@biggamesmallworld biggamesmallworld left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this — the underlying observation is a good catch, and the PR description does a genuinely useful thing by writing down why the obvious fixes (log directly, throw) don't work. The onNoMigrationsFound seam is the right shape. Two things need to change before this lands, and I think one of them makes the PR considerably more valuable rather than smaller.

The signal is one step away from a better one

I built packages/core from this branch and ran the CLI e2e suite (e2e/vitest.e2e.config.mts), which the unit suite doesn't cover. One existing test fails:

FAIL  runMigrationsOperation > should report no pending migrations when none exist
AssertionError: expected 'No migration files matched the config…' to contain 'No pending migrations found'

- No pending migrations found
+ No migration files matched the configured `migrations` patterns, so no migrations can be run.
+ Patterns are resolved relative to the current directory (…/e2e/fixtures/test-project):
+  - …/e2e/fixtures/test-project/migrations/*.ts

That isn't a stale assertion to update — it's the regression. The fixture has a valid glob over an empty directory, which is what every project looks like before its first migration is authored. connection.migrations.length === 0 overwhelmingly means "no migrations written yet", not "your globs are broken", so as written this prints an alarming, factually-true non-problem to exactly the users least able to evaluate it. You identified this scenario in the description (the create template configures the glob before any migration exists) and concluded it must not throw — but not throwing isn't the same as not being wrong.

There is a signal that discriminates exactly, and it's proof rather than heuristic: zero migration classes loaded while the migrations table has rows can only mean the globs stopped matching. That's also the dangerous real-world case — a deploy pointed at dist/migrations/*.js that was never built, against a database with history.

import { MigrationExecutor } from 'typeorm';

const executed = await new MigrationExecutor(connection).getExecutedMigrations();
if (!connection.migrations.length && executed.length) {
    // globs definitively broken
}

With that gate, the failing test above passes untouched, and the warning only fires when it's true.

I checked the fresh-database case since it's the one that would bite: against a sqlite DB with no migrations table, getExecutedMigrations() returns [] rather than throwing, so no special-casing is needed. It does create the migrations table as a side effect — which is fine at this call site, because connection.runMigrations() on the next line creates it anyway, but worth knowing before reusing the call somewhere read-only.

The bigger hole is already computed and thrown away

While you're in here: checkMigrationStatus() (migrate.ts:87) already computes builderLog.upQueries and emits precisely the warning #5001 asks for —

Your database schema does not match your current configuration. Generate a new migration for the following changes:

— via log(), which is a hard no-op whenever VENDURE_RUNNING_IN_CLI is set (migrate.ts:312). So on every single vendure migrate --run, the "your DB is out of sync" diagnostic is calculated and discarded. That is the data-safety gap in the issue, it costs nothing extra to detect, and the callback you've just added is the natural way to surface it. Routing that through the same seam would be the higher-value half of this change.

Worth noting the same suppression applies to revertLastMigration (migrate.ts:155), which the issue explicitly calls out as having identical symptoms.

Fixes #5001 isn't accurate

The reporter's glob matches — their workaround DataSource uses the identical pattern and applies migrations successfully — so connection.migrations.length is non-zero in their scenario and onNoMigrationsFound never fires. Their symptom is exit 0 in ~3s with no output at all, before any migration work happens.

I tried to reproduce that and could not. Every config-load failure I could construct against the e2e fixture exits non-zero with a message:

config = undefined            → ■ Cannot read properties of undefined (reading 'plugins')          EXIT=1
config with top-level await   → ■ require() cannot be used on an ESM graph with top-level await    EXIT=1

So that root cause is still open and needs a repro from the reporter (their tsconfig, plus NODE_DEBUG=module npx vendure migrate --run). Could you retarget this to Relates to #5001? Landing it as Fixes would close a live data-safety report that remains unaddressed.

Test that would have caught this

Nothing currently executes warnIfNoMigrationsFound — the CLI spec mocks @vendure/core wholesale, so runMigrations is a vi.fn(), and the core spec only exercises the message builder. The e2e fixture drives real TypeORM against real sqlite and is the right home. This passes on your branch and fails on master:

// #5001 — a `migrations` glob that resolves to nothing must not be reported as "up to date"
it('should report unmatched migration patterns when migrations have already been applied', async () => {
    process.chdir(TEST_PROJECT_DIR);

    // Apply a migration so the `migrations` table is non-empty
    const generateResult = await generateMigrationOperation({
        name: 'TestMigration',
        outputDir: MIGRATIONS_DIR,
    });
    expect(generateResult.success).toBe(true);
    expect((await runMigrationsOperation()).migrationsRan?.length).toBeGreaterThan(0);

    // Simulate the glob resolving to nothing (wrong cwd, or unbuilt `dist/migrations/*.js`)
    await fs.emptyDir(MIGRATIONS_DIR);

    const result = await runMigrationsOperation();

    expect(result.migrationsRan).toHaveLength(0);
    expect(result.message).toContain('No migration files matched');
});

Together with the existing 'should report no pending migrations when none exist', those two pin the discriminator from both sides.

One warning about verifying this locally: the CLI e2e suite resolves @vendure/core through packages/core/dist, so it will happily pass against a build that predates your change and tell you nothing. Run npm run build in packages/core first.

Smaller things

  • Drop the two class-configuration tests in migrate.spec.ts. If a migration class is in options.migrations, TypeORM loads it, so connection.migrations.length can't be 0 — those states are unreachable in production. They only typecheck because getNoMigrationsFoundMessage takes the count and the config as independent parameters; passing the connection (or just the two derived booleans) would make them impossible to express.
  • The report-building block is now byte-identical in migration-operations.ts:81-88 and run-migration/run-migration.ts:26-32, and 'No pending migrations found' appears in three places plus an assertion in migration-operations.spec.ts:194. Worth extracting into one formatMigrationReport().
  • Wrong channel. run-migration.ts:33 passes a multi-line message to runSpinner.stop() and migrate.ts:87 renders it with log.success() — clack prefixes only the first line, so the pattern list hangs outside the box, and a warning gets a green success glyph with exit 0. If it's worth reporting, it isn't a success.
  • Public API. RunMigrationsOptions is exported from packages/core/src/index.ts with @docsCategory and @since, and its whole payload is a pre-formatted English string a programmatic consumer can only print. If it stays public, hand over data ({ patterns, cwd }, or the pending-change list) and let each caller format. Also prefer export type { RunMigrationsOptions } — the current value-export breaks downstream consumers building with isolatedModules.
  • migration-operations-reporting.spec.ts:50 (expect(...).not.toBe('No pending migrations found')) can't fail if line 49 passes. And that file duplicates the existing describe('runMigrationsOperation()') in migration-operations.spec.ts under a different mocking regime — worth merging.
  • The comment at migrate.ts:141 says log() is suppressed because a spinner is active. It's actually suppressed whenever VENDURE_RUNNING_IN_CLI is set, spinner or not — the migrate --run path (migrate.ts:62) has no spinner at all. Same for the ten-line block at migrate.ts:104; most of it is commit-message material, and the one durable sentence is "TypeORM resolves migration globs relative to process.cwd() and silently yields zero classes when nothing matches."

Happy to review again once the gate changes — the diagnostic seam itself is good work and I'd like to see it land carrying the schema-drift warning too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

T3: Systemic Involves a systemic decision. Decide before implementing. type: bug 🐛 Something isn't working @vendure/cli @vendure/core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

npx vendure migrate --run silently no-ops (exits 0, ~3s) instead of running pending migrations

3 participants