From c6c61b59431443b2bcda25f3af7624dd2ce19b9b Mon Sep 17 00:00:00 2001 From: David Laban Date: Wed, 3 Jun 2026 10:12:46 +0100 Subject: [PATCH 1/5] CFSQL-1589 migrations_pattern for configuring recursive migration discovery (#14089) --- .changeset/d1-execute-logger-level.md | 9 + .changeset/d1-migrations-pattern.md | 28 + .../workers-utils/src/config/environment.ts | 15 + .../workers-utils/src/config/validation.ts | 10 + packages/workers-utils/src/worker.ts | 1 + .../wrangler/src/__tests__/d1/migrate.test.ts | 484 ++++++++- .../__tests__/d1/migrations/helpers.test.ts | 997 +++++++++++++++++- .../wrangler/src/__tests__/d1/utils.test.ts | 12 +- packages/wrangler/src/d1/execute.ts | 85 +- packages/wrangler/src/d1/migrations/apply.ts | 53 +- packages/wrangler/src/d1/migrations/create.ts | 82 +- .../wrangler/src/d1/migrations/helpers.ts | 359 ++++++- packages/wrangler/src/d1/migrations/list.ts | 27 +- packages/wrangler/src/d1/types.ts | 12 +- packages/wrangler/src/d1/utils.ts | 6 +- 15 files changed, 2008 insertions(+), 172 deletions(-) create mode 100644 .changeset/d1-execute-logger-level.md create mode 100644 .changeset/d1-migrations-pattern.md diff --git a/.changeset/d1-execute-logger-level.md b/.changeset/d1-execute-logger-level.md new file mode 100644 index 0000000000..2b9ed5a53c --- /dev/null +++ b/.changeset/d1-execute-logger-level.md @@ -0,0 +1,9 @@ +--- +"wrangler": patch +--- + +Restore the D1 `executeSql` logger level via try/finally + +`wrangler d1 execute --json` and the internal `executeSql` helper temporarily lower the global logger to `"error"` to keep human-readable output out of the JSON payload. Previously the level was restored only on the happy path, so any early return or thrown error left the singleton logger muted, silencing later `logger.warn`/`logger.log` output (notably from migration helpers that wrap `executeSql` and are commonly mocked in tests). + +The level swap is now wrapped in `try`/`finally` so it is always restored. diff --git a/.changeset/d1-migrations-pattern.md b/.changeset/d1-migrations-pattern.md new file mode 100644 index 0000000000..9095a2187b --- /dev/null +++ b/.changeset/d1-migrations-pattern.md @@ -0,0 +1,28 @@ +--- +"wrangler": minor +"@cloudflare/workers-utils": minor +--- + +Add `migrations_pattern` to D1 database bindings + +The D1 binding now accepts an optional `migrations_pattern` field, allowing you to point `wrangler d1 migrations apply` and `wrangler d1 migrations list` at migration files in nested layouts (e.g. ORM-generated folders like `migrations/0000_init/migration.sql`). + +`migrations_pattern` is a glob (relative to the wrangler config file) and defaults to `${migrations_dir}/*.sql`, which preserves today's behaviour. Files that do not match the pattern are not executed. + +```jsonc +{ + "d1_databases": [ + { + "binding": "DB", + "database_name": "my-db", + "database_id": "...", + "migrations_dir": "migrations", + "migrations_pattern": "migrations/*/migration.sql", + }, + ], +} +``` + +When no migrations match the configured pattern but files matching the common `migrations/*/migration.sql` (drizzle-style) layout do exist, Wrangler logs a hint suggesting `migrations_pattern` as an opt-in. + +`wrangler d1 migrations create` now returns an actionable error if the generated migration filename would not match the configured pattern. diff --git a/packages/workers-utils/src/config/environment.ts b/packages/workers-utils/src/config/environment.ts index a69f1ed315..9fad6ef9a7 100644 --- a/packages/workers-utils/src/config/environment.ts +++ b/packages/workers-utils/src/config/environment.ts @@ -990,6 +990,21 @@ export interface EnvironmentNonInheritable { migrations_table?: string; /** The path to the directory of migrations for this D1 database (defaults to './migrations'). */ migrations_dir?: string; + /** + * A glob pattern (relative to the Wrangler config file) used to discover + * migration files for this D1 database. Defaults to `${migrations_dir}/*.sql` + * if not specified. + * + * Use this to opt in to nested layouts such as `migrations/*\/migration.sql` + * (as produced by some ORMs). + * + * When `migrations_pattern` is set, `migrations_dir` must also be set, and + * `migrations_pattern` must start with `${migrations_dir}/`. This keeps the + * relationship between the two settings explicit and lets Wrangler record + * each migration's name in the migrations table as a path relative to + * `migrations_dir`. + */ + migrations_pattern?: string; /** Internal use only. */ database_internal_env?: string; /** Whether the D1 database should be remote or not in local development */ diff --git a/packages/workers-utils/src/config/validation.ts b/packages/workers-utils/src/config/validation.ts index 61612f1e76..56e6639ee0 100644 --- a/packages/workers-utils/src/config/validation.ts +++ b/packages/workers-utils/src/config/validation.ts @@ -4117,12 +4117,22 @@ const validateD1Binding: ValidatorFn = (diagnostics, field, value) => { isValid = false; } + if (!isOptionalProperty(value, "migrations_pattern", "string")) { + diagnostics.errors.push( + `"${field}" bindings should, optionally, have a string "migrations_pattern" field but got ${JSON.stringify( + value + )}.` + ); + isValid = false; + } + validateAdditionalProperties(diagnostics, field, Object.keys(value), [ "binding", "database_id", "database_internal_env", "database_name", "migrations_dir", + "migrations_pattern", "migrations_table", "preview_database_id", "remote", diff --git a/packages/workers-utils/src/worker.ts b/packages/workers-utils/src/worker.ts index c6ea6edc03..d5ab168da3 100644 --- a/packages/workers-utils/src/worker.ts +++ b/packages/workers-utils/src/worker.ts @@ -220,6 +220,7 @@ export interface CfD1Database { database_internal_env?: string; migrations_table?: string; migrations_dir?: string; + migrations_pattern?: string; remote?: boolean; raw?: boolean; } diff --git a/packages/wrangler/src/__tests__/d1/migrate.test.ts b/packages/wrangler/src/__tests__/d1/migrate.test.ts index 168002c52b..f86f97bd8c 100644 --- a/packages/wrangler/src/__tests__/d1/migrate.test.ts +++ b/packages/wrangler/src/__tests__/d1/migrate.test.ts @@ -1,3 +1,5 @@ +import * as fs from "node:fs"; +import * as path from "node:path"; import { runInTempDir, writeWranglerConfig, @@ -52,6 +54,180 @@ describe("migrate", () => { await runWrangler("d1 migrations create D1 test-migration"); expect(mockStd.out).toContain("Successfully created Migration"); }); + + it("`create` succeeds when the new file matches a permissive pattern", async ({ + expect, + }) => { + setIsTTY(false); + writeWranglerConfig({ + d1_databases: [ + { + binding: "DATABASE", + database_name: "db", + database_id: "xxxx", + migrations_dir: "migrations", + migrations_pattern: "migrations/**/*.sql", + }, + ], + }); + + await runWrangler("d1 migrations create db test"); + + const files = fs.readdirSync("./migrations"); + expect(files).toEqual(["0001_test.sql"]); + expect( + fs.readFileSync(path.join("./migrations", "0001_test.sql"), "utf8") + ).toContain("Migration number: 0001"); + }); + + it("rejects `wrangler d1 migrations create` with an actionable error when the new file would not match the configured pattern", async ({ + expect, + }) => { + setIsTTY(false); + // Use a JSONC config (what we recommend in our docs) so the + // snapshot below reflects the typical user-facing message. + writeWranglerConfig( + { + d1_databases: [ + { + binding: "DATABASE", + database_name: "db", + database_id: "xxxx", + migrations_dir: "migrations", + // Pattern only matches nested files, so a top-level + // `0001_test.sql` should be rejected. + migrations_pattern: "migrations/*/migration.sql", + }, + ], + }, + "./wrangler.jsonc" + ); + fs.mkdirSync("./migrations", { recursive: true }); + + await expect(runWrangler("d1 migrations create db test")).rejects + .toThrowErrorMatchingInlineSnapshot(` + [Error: Wrangler would like to make a new migration called \`migrations/0001_test.sql\` but it does not match the configured \`migrations_pattern: "migrations/*/migration.sql"\` in your wrangler.jsonc file, so \`wrangler d1 migrations apply\` would not pick it up. \`wrangler d1 migrations create\` only writes top-level files inside \`migrations_dir\`. + + If you are using an ORM like drizzle to manage migrations, use the ORM's command (e.g. \`drizzle-kit generate\`) instead of \`wrangler d1 migrations create\` — it will create files in the nested layout your \`migrations_pattern\` expects. + + Otherwise, change \`migrations_pattern\` in your wrangler.jsonc file to match top-level \`.sql\` files (for example, \`migrations/*.sql\`).] + `); + }); + + it("does not create migrations_dir when `wrangler d1 migrations create` fails the pattern check", async ({ + expect, + }) => { + setIsTTY(false); + writeWranglerConfig( + { + d1_databases: [ + { + binding: "DATABASE", + database_name: "db", + database_id: "xxxx", + migrations_dir: "migrations", + migrations_pattern: "migrations/*/migration.sql", + }, + ], + }, + "./wrangler.jsonc" + ); + // Deliberately do NOT pre-create `./migrations` and do NOT + // `mockConfirm` the "Ok to create" prompt: if the pattern check + // runs first we never reach the prompt, and the dir is still + // absent after the throw. + + await expect( + runWrangler("d1 migrations create db test") + ).rejects.toThrowError(/does not match the configured/); + + expect(fs.existsSync("./migrations")).toBe(false); + }); + + it('`create` succeeds with `migrations_dir: "."` (project root as the migrations dir)', async ({ + expect, + }) => { + setIsTTY(false); + writeWranglerConfig( + { + d1_databases: [ + { + binding: "DATABASE", + database_name: "db", + database_id: "xxxx", + migrations_dir: ".", + }, + ], + }, + "./wrangler.jsonc" + ); + + // `list`/`apply` discover top-level `.sql` files when migrations_dir + // is ".", so `create` must be able to write one. The default pattern + // normalizes to "*.sql"; the new file "0001_test.sql" at the project + // root matches it, so create should succeed (not reject as "does not + // match the configured migrations_pattern"). + await runWrangler("d1 migrations create db test"); + + expect(fs.existsSync("0001_test.sql")).toBe(true); + expect(fs.readFileSync("0001_test.sql", "utf8")).toContain( + "Migration number: 0001" + ); + }); + + it("rejects a migration name containing a path separator with a clear error", async ({ + expect, + }) => { + setIsTTY(false); + writeWranglerConfig( + { + d1_databases: [ + { binding: "DATABASE", database_name: "db", database_id: "xxxx" }, + ], + }, + "./wrangler.jsonc" + ); + + // A `/` in the name would otherwise produce an extra path segment + // (`migrations/0001_foo/bar.sql`) that the default pattern can't + // match — surfacing as a confusing "does not match migrations_pattern" + // error. We reject it up front instead. + await expect( + runWrangler("d1 migrations create db foo/bar") + ).rejects.toThrowErrorMatchingInlineSnapshot( + // snapshot process seems to replace \ with / for consistency across platforms + `[Error: The migration name "foo/bar" contains a path separator ("/" or "/"). Please remove this and try again.]` + ); + + // And the dir must not have been created as a side effect. + expect(fs.existsSync("migrations")).toBe(false); + }); + + it("rejects a migration name containing a backslash with a clear error", async ({ + expect, + }) => { + setIsTTY(false); + writeWranglerConfig( + { + d1_databases: [ + { binding: "DATABASE", database_name: "db", database_id: "xxxx" }, + ], + }, + "./wrangler.jsonc" + ); + + await expect( + runWrangler( + // windows shells need less escaping of backslashes than unix ones? + process.platform === "win32" + ? "d1 migrations create db foo\\bar" + : "d1 migrations create db foo\\\\bar" + ) + ).rejects.toThrowErrorMatchingInlineSnapshot( + // snapshot process seems to replace \ with / for consistency across platforms + `[Error: The migration name "foo//bar" contains a path separator ("/" or "/"). Please remove this and try again.]` + ); + }); }); describe("apply", () => { @@ -123,13 +299,14 @@ describe("migrate", () => { }) => { setIsTTY(false); + const migrationsDir = path.join(process.cwd(), "my-migrations-go-here"); writeWranglerConfig({ d1_databases: [ { binding: "DATABASE", database_name: "db", database_id: "xxxx", - migrations_dir: "/tmp/my-migrations-go-here", + migrations_dir: migrationsDir, }, ], }); @@ -141,7 +318,7 @@ describe("migrate", () => { ); mockConfirm({ text: `No migrations folder found. -Ok to create /tmp/my-migrations-go-here?`, +Ok to create ${migrationsDir}?`, result: true, }); await runWrangler("d1 migrations create db test"); @@ -204,20 +381,21 @@ Your database may not be available to serve requests during the migration, conti { id: "R2-D2", name: "enterprise-nx" }, ]) ); + const migrationsDir = path.join(process.cwd(), "my-migrations-go-here"); writeWranglerConfig({ d1_databases: [ { binding: "DATABASE", database_name: "db", database_id: "xxxx", - migrations_dir: "/tmp/my-migrations-go-here", + migrations_dir: migrationsDir, }, ], account_id: "nx01", }); mockConfirm({ text: `No migrations folder found. -Ok to create /tmp/my-migrations-go-here?`, +Ok to create ${migrationsDir}?`, result: true, }); await runWrangler("d1 migrations create db test"); @@ -227,7 +405,8 @@ Your database may not be available to serve requests during the migration, conti result: true, }); await runWrangler("d1 migrations apply db --remote"); - expect(std.out).toBe(""); + expect(std.out).toContain("Migrations to be applied:"); + expect(std.out).toContain("0001_test.sql"); }); it("should throw a clear error when executeSql returns null (execution cancelled)", async ({ @@ -264,6 +443,82 @@ Your database may not be available to serve requests during the migration, conti `Migration "0001_test.sql" was not applied — execution was cancelled.` ); }); + + it("`apply` records each migration's name in `d1_migrations` as a path relative to `migrations_dir`", async ({ + expect, + }) => { + setIsTTY(false); + writeWranglerConfig({ + d1_databases: [ + { + binding: "DATABASE", + database_name: "db", + database_id: "xxxx", + migrations_dir: "migrations", + migrations_pattern: "migrations/**/*.sql", + }, + ], + }); + + // 3-levels-deep layout: + // migrations/0001_top.sql (level 1) + // migrations/0002_users/0001_init.sql (level 2) + // migrations/0003_features/auth/0001_oauth.sql (level 3) + fs.mkdirSync("./migrations/0002_users", { recursive: true }); + fs.mkdirSync("./migrations/0003_features/auth", { recursive: true }); + fs.writeFileSync("./migrations/0001_top.sql", "-- top"); + fs.writeFileSync("./migrations/0002_users/0001_init.sql", "-- mid"); + fs.writeFileSync( + "./migrations/0003_features/auth/0001_oauth.sql", + "-- deep" + ); + + // Spy on `executeSql` and pluck the `INSERT INTO d1_migrations + // (name) values ('...');` lines back out of the SQL `apply` emits. + // Fragile — couples to the exact INSERT statement in apply.ts — + // but unavoidable: applying against real Miniflare/D1 then + // `SELECT name FROM d1_migrations` boots a fresh Miniflare per + // executeSql call (~6 boots × ~2s for a 3-migration test) and + // blows the per-test timeout. If executeSql ever batches, or + // Miniflare startup gets materially faster, rewrite this against + // real D1. + const insertedNames: string[] = []; + const spy = vi + .spyOn(d1Execute, "executeSql") + .mockImplementation(async ({ command }) => { + if (typeof command === "string") { + const match = command.match( + /INSERT INTO d1_migrations \(name\)\s*values\s*\('([^']+)'\)/ + ); + if (match) { + insertedNames.push(match[1]); + } + } + return [{ results: [], success: true, meta: {} as never }]; + }); + + mockConfirm({ + text: `About to apply 3 migration(s)\nYour database may not be available to serve requests during the migration, continue?`, + result: true, + }); + + try { + await runWrangler("d1 migrations apply db --local"); + } finally { + spy.mockRestore(); + } + + // `name` values must be relative to `migrations_dir`, not absolute, + // not project-relative. The snapshot locks in the exact shape so + // it's obvious at a glance. + expect(insertedNames).toMatchInlineSnapshot(` + [ + "0001_top.sql", + "0002_users/0001_init.sql", + "0003_features/auth/0001_oauth.sql", + ] + `); + }); }); describe("list", () => { @@ -313,6 +568,54 @@ Your database may not be available to serve requests during the migration, conti ); }); + it("hints at `migrations_dir` when the folder is missing and the user has not set one", async ({ + expect, + }) => { + setIsTTY(false); + writeWranglerConfig( + { + d1_databases: [ + { binding: "DATABASE", database_name: "db", database_id: "xxxx" }, + ], + }, + "./wrangler.jsonc" + ); + await expect( + runWrangler("d1 migrations list --local DATABASE") + ).rejects.toThrowError(`No migrations present at /migrations.`); + expect(mockStd.warn).toContain( + "Set `migrations_dir` in your wrangler.jsonc file to choose a different path." + ); + }); + + it("does not hint at `migrations_dir` when the user set it, even to the default `./migrations`", async ({ + expect, + }) => { + setIsTTY(false); + // `./migrations` normalizes to `migrations`, so a check against the + // default path would wrongly conclude the user is on the default and + // show the (misleading) "Set `migrations_dir`..." hint. The user did + // set it explicitly, so no hint should appear. + writeWranglerConfig( + { + d1_databases: [ + { + binding: "DATABASE", + database_name: "db", + database_id: "xxxx", + migrations_dir: "./migrations", + }, + ], + }, + "./wrangler.jsonc" + ); + await expect( + runWrangler("d1 migrations list --local DATABASE") + ).rejects.toThrowError(`No migrations present at /migrations.`); + expect(mockStd.warn).toContain("No migrations folder found."); + expect(mockStd.warn).not.toContain("Set `migrations_dir`"); + }); + it("should try to read D1 config from wrangler.toml when logged in", async ({ expect, }) => { @@ -348,5 +651,176 @@ Your database may not be available to serve requests during the migration, conti runWrangler("d1 migrations list DATABASE") ).rejects.toThrowError(`No migrations present at /migrations.`); }); + + it("`list` only shows migrations matching migrations_pattern (nested layout)", async ({ + expect, + }) => { + setIsTTY(false); + writeWranglerConfig({ + d1_databases: [ + { + binding: "DATABASE", + database_name: "db", + database_id: "xxxx", + migrations_dir: "migrations", + migrations_pattern: "migrations/*/migration.sql", + }, + ], + }); + + fs.mkdirSync("./migrations/0000_init", { recursive: true }); + fs.writeFileSync( + "./migrations/0000_init/migration.sql", + "-- nested migration" + ); + // Top-level .sql file should NOT be listed because it doesn't match + // the pattern. + fs.writeFileSync("./migrations/should_be_ignored.sql", "-- noop"); + + const spy = vi + .spyOn(d1Execute, "executeSql") + .mockResolvedValue([{ results: [], success: true, meta: {} as never }]); + + try { + await runWrangler("d1 migrations list --local db"); + } finally { + spy.mockRestore(); + } + + expect(mockStd.out).toContain("0000_init/migration.sql"); + expect(mockStd.out).not.toContain("should_be_ignored.sql"); + }); + + it("`list` prints a drizzle hint when migrations_pattern is the default but a nested layout exists", async ({ + expect, + }) => { + setIsTTY(false); + // Use a JSONC config (the format we recommend in our docs) so the + // snapshot below reflects the typical user-facing message. + writeWranglerConfig( + { + d1_databases: [ + { + binding: "DATABASE", + database_name: "db", + database_id: "xxxx", + }, + ], + }, + "./wrangler.jsonc" + ); + + fs.mkdirSync("./migrations/0000_init", { recursive: true }); + fs.writeFileSync("./migrations/0000_init/migration.sql", "-- nested"); + + const spy = vi + .spyOn(d1Execute, "executeSql") + .mockResolvedValue([{ results: [], success: true, meta: {} as never }]); + + try { + await runWrangler("d1 migrations list --local db"); + } finally { + spy.mockRestore(); + } + + expect(mockStd.warn).toMatchInlineSnapshot(` + "▲ [WARNING] Could not find any migration files matching \`migrations/*.sql\`. It looks like there are migration files matching \`migrations/*/migration.sql\` though. If you are using drizzle to manage your migrations, please set \`migrations_pattern\` to \`migrations/*/migration.sql\` in wrangler.jsonc. + + " + `); + }); + + it('`list` with `migrations_dir: "."` treats the project root as the migrations dir', async ({ + expect, + }) => { + setIsTTY(false); + writeWranglerConfig({ + d1_databases: [ + { + binding: "DATABASE", + database_name: "db", + database_id: "xxxx", + migrations_dir: ".", + }, + ], + }); + + // Seed the project root with a mix of files. Only the top-level + // `.sql` files should be picked up. + fs.writeFileSync("0001_top.sql", "-- top"); + fs.writeFileSync("0002_users.sql", "-- users"); + fs.writeFileSync("README.md", "# not a migration"); + fs.mkdirSync("nested", { recursive: true }); + fs.writeFileSync("nested/0003_deep.sql", "-- deep"); + + const spy = vi + .spyOn(d1Execute, "executeSql") + .mockResolvedValue([{ results: [], success: true, meta: {} as never }]); + + try { + await runWrangler("d1 migrations list --local db"); + } finally { + spy.mockRestore(); + } + + expect(mockStd.out).toContain("0001_top.sql"); + expect(mockStd.out).toContain("0002_users.sql"); + expect(mockStd.out).not.toContain("README.md"); + expect(mockStd.out).not.toContain("0003_deep.sql"); + }); + + it("rejects a `migrations_pattern` that does not start with `migrations_dir`", async ({ + expect, + }) => { + setIsTTY(false); + writeWranglerConfig( + { + d1_databases: [ + { + binding: "DATABASE", + database_name: "db", + database_id: "xxxx", + migrations_dir: "migrations", + migrations_pattern: "schema/*.sql", + }, + ], + }, + "./wrangler.jsonc" + ); + + await expect(runWrangler("d1 migrations list --local db")).rejects + .toThrowErrorMatchingInlineSnapshot(` + [Error: The configured \`migrations_pattern: "schema/*.sql"\` in your wrangler.jsonc file must start with \`migrations/\` to match \`"migrations_dir": "migrations"\`. + + Either change \`migrations_pattern\` so it starts with \`migrations/\` (for example, \`"migrations/*.sql"\`), or change \`migrations_dir\` to match the start of your pattern.] + `); + }); + + it("rejects a `migrations_pattern` set without `migrations_dir` with an actionable error", async ({ + expect, + }) => { + setIsTTY(false); + writeWranglerConfig( + { + d1_databases: [ + { + binding: "DATABASE", + database_name: "db", + database_id: "xxxx", + // migrations_dir intentionally not set. + migrations_pattern: "migrations/*.sql", + }, + ], + }, + "./wrangler.jsonc" + ); + + await expect(runWrangler("d1 migrations list --local db")).rejects + .toThrowErrorMatchingInlineSnapshot(` + [Error: You have set \`migrations_pattern: "migrations/*.sql"\` in your wrangler.jsonc file but have not set \`migrations_dir\` for this D1 binding. + + When \`migrations_pattern\` is set, \`migrations_dir\` must also be set, and \`migrations_pattern\` must start with \`\${migrations_dir}/\`. Add a \`migrations_dir\` entry to your wrangler.jsonc file (for example, \`"migrations_dir": "migrations"\`).] + `); + }); }); }); diff --git a/packages/wrangler/src/__tests__/d1/migrations/helpers.test.ts b/packages/wrangler/src/__tests__/d1/migrations/helpers.test.ts index 5bffc4f304..840c5a5ea6 100644 --- a/packages/wrangler/src/__tests__/d1/migrations/helpers.test.ts +++ b/packages/wrangler/src/__tests__/d1/migrations/helpers.test.ts @@ -2,35 +2,80 @@ import * as fs from "node:fs"; import * as path from "node:path"; import { runInTempDir } from "@cloudflare/workers-utils/test-helpers"; import { describe, it } from "vitest"; -import { getMigrationNames } from "../../../d1/migrations/helpers"; +import { + compareMigrationPaths, + getMigrationNames, + getNextMigrationNumber, + maybeLogHint, + resolveMigrationsConfig, +} from "../../../d1/migrations/helpers"; +import { mockConsoleMethods } from "../../helpers/mock-console"; +import type { MigrationsConfig } from "../../../d1/migrations/helpers"; +import type { Database } from "../../../d1/types"; + +/** + * Write a set of project files to the current working directory. + * + * Each path is interpreted as relative to where `wrangler.toml` would live + * (i.e. the project root — which `runInTempDir` makes the current working + * directory). This mirrors the way `migrations_pattern` globs are resolved at + * runtime, so the tests below read very close to what a user would write. + * + * For example: `seedProjectFiles(["migrations/0001_init.sql"])` puts an empty + * SQL file at `/migrations/0001_init.sql`. + */ +function seedProjectFiles(filesRelativeToProject: string[]) { + for (const file of filesRelativeToProject) { + const abs = path.resolve(file); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, "-- test migration"); + } +} + +/** + * Build a `MigrationsConfig` for tests. The defaults match what + * `resolveMigrationsConfig` would produce for a wrangler.jsonc at the + * project root with `migrations_dir: "migrations"` and no + * `migrations_pattern`. Tests pass the result straight to + * `getMigrationNames` / `getNextMigrationNumber`. + */ +function migrationsConfig( + props: Partial = {} +): MigrationsConfig { + return { + projectPath: ".", + configFile: "wrangler.jsonc", + migrationsDir: "migrations", + migrationsPattern: "migrations/*.sql", + migrationsTableName: "d1_migrations", + ...props, + }; +} + describe("getMigrationNames", () => { runInTempDir(); + // `getMigrationNames` never logs — the drizzle hint comes from + // `maybeLogHint` (tested separately). Swallow console output anyway. + mockConsoleMethods(); - it("should return empty array for empty directory", ({ expect }) => { - const migrationsDir = "./migrations"; - fs.mkdirSync(migrationsDir, { recursive: true }); - - const result = getMigrationNames(migrationsDir); + it("returns an empty array for an empty directory", ({ expect }) => { + fs.mkdirSync("migrations", { recursive: true }); + const result = getMigrationNames(migrationsConfig()); expect(result).toEqual([]); }); - it("should return sorted migration files", ({ expect }) => { - const migrationsDir = "./migrations"; - fs.mkdirSync(migrationsDir, { recursive: true }); - - const files = [ - "0003_add_indexes.sql", - "0001_create_tables.sql", - "0002_add_columns.sql", - "0005_update_views.sql", - "0004_drop_unused.sql", - ]; - - files.forEach((file) => { - fs.writeFileSync(path.join(migrationsDir, file), "-- test migration"); - }); + it("returns top-level .sql files sorted lexicographically with the default pattern", ({ + expect, + }) => { + seedProjectFiles([ + "migrations/0003_add_indexes.sql", + "migrations/0001_create_tables.sql", + "migrations/0002_add_columns.sql", + "migrations/0005_update_views.sql", + "migrations/0004_drop_unused.sql", + ]); - const result = getMigrationNames(migrationsDir); + const result = getMigrationNames(migrationsConfig()); expect(result).toEqual([ "0001_create_tables.sql", @@ -41,35 +86,903 @@ describe("getMigrationNames", () => { ]); }); - it("should ignore non-SQL files", ({ expect }) => { - const migrationsDir = "./migrations"; - fs.mkdirSync(migrationsDir, { recursive: true }); + it("ignores non-SQL files under the default pattern", ({ expect }) => { + // `migrations_pattern` does the filtering — the walk picks up + // everything under `migrations_dir`, then minimatch rejects files + // that don't match. So `README.md`, `config.json`, etc. living + // alongside the migrations are invisible under the default + // `migrations/*.sql` pattern without anyone needing to know about + // them ahead of time. + seedProjectFiles([ + "migrations/0001_create_tables.sql", + "migrations/0002_add_columns.sql", + "migrations/README.md", + "migrations/config.json", + "migrations/migration_lock.toml", + ]); + + const result = getMigrationNames(migrationsConfig()); + + expect(result).toEqual(["0001_create_tables.sql", "0002_add_columns.sql"]); + }); + + it("does not pick up nested .sql files with the default pattern", ({ + expect, + }) => { + seedProjectFiles([ + "migrations/0001_create_tables.sql", + "migrations/test_data/destroy.sql", + ]); - fs.writeFileSync( - path.join(migrationsDir, "0001_create_tables.sql"), - "-- test" + const result = getMigrationNames(migrationsConfig()); + + // Top-level file is included; nested is not. + expect(result).toEqual(["0001_create_tables.sql"]); + }); + + it("picks up nested .sql files when migrations_pattern is configured", ({ + expect, + }) => { + seedProjectFiles([ + "migrations/0000_init/migration.sql", + "migrations/0001_users/migration.sql", + ]); + + const result = getMigrationNames( + migrationsConfig({ migrationsPattern: "migrations/*/migration.sql" }) ); - fs.writeFileSync( - path.join(migrationsDir, "0002_add_columns.sql"), - "-- test" + + expect(result).toEqual([ + "0000_init/migration.sql", + "0001_users/migration.sql", + ]); + }); + + it("matches files by whatever extension `migrations_pattern` specifies, not just `.sql`", ({ + expect, + }) => { + // The walk doesn't filter by extension — `migrations_pattern` decides + // what counts as a migration. A user whose ORM emits `.up.sql` files + // (or anything else) can write the matching pattern and have those + // picked up. Top-level `.sql` files that don't match the pattern are + // invisible. + seedProjectFiles([ + "migrations/0001_init.up.sql", + "migrations/0002_users.up.sql", + "migrations/0099_legacy.sql", + ]); + + const result = getMigrationNames( + migrationsConfig({ migrationsPattern: "migrations/*.up.sql" }) ); - fs.writeFileSync(path.join(migrationsDir, "README.md"), "# readme"); - fs.writeFileSync(path.join(migrationsDir, "config.json"), "{}"); - fs.writeFileSync(path.join(migrationsDir, "migration_lock.toml"), ""); - const result = getMigrationNames(migrationsDir); + expect(result).toEqual(["0001_init.up.sql", "0002_users.up.sql"]); + }); - expect(result).toEqual(["0001_create_tables.sql", "0002_add_columns.sql"]); + it("returns names relative to migrations_dir even when the pattern has a literal sub-segment between migrations_dir and the first glob", ({ + expect, + }) => { + // A pattern like `migrations/sub/*.sql` adds a literal `sub/` + // segment between `migrations_dir` and the first glob. The recorded + // names must still be relative to `migrations_dir` (not relative to + // `migrations/sub`), so the `d1_migrations` table records + // `sub/0001_x.sql` — which is what the user can reason about. + seedProjectFiles([ + "migrations/sub/0001_x.sql", + "migrations/sub/0002_y.sql", + // Top-level .sql files must NOT match this pattern. + "migrations/should_be_ignored.sql", + ]); + + const result = getMigrationNames( + migrationsConfig({ migrationsPattern: "migrations/sub/*.sql" }) + ); + + expect(result).toEqual(["sub/0001_x.sql", "sub/0002_y.sql"]); }); - it("should handle directory with only non-SQL files", ({ expect }) => { - const migrationsDir = "./migrations"; - fs.mkdirSync(migrationsDir, { recursive: true }); + it("recursively finds .sql files 3 levels deep when migrations_pattern uses **", ({ + expect, + }) => { + seedProjectFiles([ + // One file at each of levels 1, 2, and 3 under migrations_dir. + "migrations/0001_top.sql", + "migrations/feature_a/0002_mid.sql", + "migrations/feature_b/sub/0003_deep.sql", + ]); - fs.writeFileSync(path.join(migrationsDir, "README.md"), "# readme"); - fs.writeFileSync(path.join(migrationsDir, "migration_lock.toml"), ""); + const result = getMigrationNames( + migrationsConfig({ migrationsPattern: "migrations/**/*.sql" }) + ); + + expect(result).toEqual([ + "0001_top.sql", + "feature_a/0002_mid.sql", + "feature_b/sub/0003_deep.sql", + ]); + }); + + it("ignores nested .sql files that the configured pattern can't reach", ({ + expect, + }) => { + // A nested file under a flat `migrations/*.sql` pattern is not a + // migration. (Whether this should print a drizzle hint is + // `maybeLogHint`'s job, tested separately — `getMigrationNames` + // itself never logs.) + seedProjectFiles(["migrations/test_data/destroy.sql"]); + + const result = getMigrationNames( + migrationsConfig({ migrationsPattern: "migrations/*.sql" }) + ); - const result = getMigrationNames(migrationsDir); expect(result).toEqual([]); }); + + it("does not descend into subdirectories the configured pattern cannot reach", ({ + expect, + }) => { + // Imagine the user has somehow ended up with a `node_modules` inside + // their migrations dir. Walking it would be expensive and pointless, + // because the default pattern `migrations/*.sql` only matches + // top-level files. The walker should prune `node_modules/` before + // descending into it. We observe the effect: a file deep under + // `node_modules/` is not reported. + seedProjectFiles([ + "migrations/0001_real.sql", + "migrations/node_modules/some_pkg/0001/migration.sql", + ]); + + const result = getMigrationNames(migrationsConfig()); + + // The configured pattern matches only the top-level file. + expect(result).toEqual(["0001_real.sql"]); + }); + + it("descends arbitrarily deep when migrations_pattern uses `**`", ({ + expect, + }) => { + // The flip side of the prune test above: when the user has opted + // into a globstar pattern, the walker must NOT prune. The user has + // explicitly asked for "search everything under migrations_dir", so + // even a `node_modules/`-style nested tree should be visited. + seedProjectFiles(["migrations/very/deeply/nested/0001_init.sql"]); + + const result = getMigrationNames( + migrationsConfig({ migrationsPattern: "migrations/**/*.sql" }) + ); + + expect(result).toEqual(["very/deeply/nested/0001_init.sql"]); + }); + + it("works end-to-end when migrations_dir / migrations_pattern are absolute Windows-style backslash paths", ({ + expect, + }) => { + // `runInTempDir()` makes cwd a fresh temp dir; `path.resolve` gives its + // absolute path. We convert the separators to backslashes to mirror a + // user who wrote a Windows-style absolute path + // (`C:\Users\…\migrations`) in their config. + // On Mac/Linux it's \Users\…\migrations and on Windows it's C:\…\migrations. + // This happens not to break anything on mac. + const absMigrationsDir = path.resolve("migrations").replace(/\//g, "\\"); + seedProjectFiles([ + "migrations/0001_init/migration.sql", + "migrations/0002_users/migration.sql", + ]); + + // Build the config the way production does — through + // resolveMigrationsConfig — so the backslashes are normalized before + // getMigrationNames walks the tree. + const config = resolveMigrationsConfig({ + databaseInfo: { + uuid: "x", + binding: "DB", + migrationsTableName: "d1_migrations", + migrationsDirRaw: absMigrationsDir, + migrationsPattern: `${absMigrationsDir}\\*\\migration.sql`, + }, + // An unrelated project dir: because migrations_dir is absolute, it + // is ignored when resolving the walk root. + configPath: path.join("some", "other", "place", "wrangler.jsonc"), + }); + + const result = getMigrationNames(config); + + // Names are recorded relative to migrations_dir, so the d1_migrations + // table records the same name regardless of how the user wrote the path. + expect(result).toEqual([ + "0001_init/migration.sql", + "0002_users/migration.sql", + ]); + }); +}); + +describe("maybeLogHint", () => { + runInTempDir(); + const std = mockConsoleMethods(); + + it("logs an actionable hint when nested files match `*/migration.sql` (drizzle layout) but the configured pattern finds nothing", ({ + expect, + }) => { + seedProjectFiles([ + "migrations/0000_init/migration.sql", + "migrations/0001_users/migration.sql", + ]); + + maybeLogHint( + migrationsConfig({ + migrationsPattern: "migrations/*.sql", + }) + ); + + // Lock in the full warning so we can eyeball that it's actionable — + // it identifies the layout, says exactly what to set, and names the + // config file. + expect(std.warn).toMatchInlineSnapshot(` + "▲ [WARNING] Could not find any migration files matching \`migrations/*.sql\`. It looks like there are migration files matching \`migrations/*/migration.sql\` though. If you are using drizzle to manage your migrations, please set \`migrations_pattern\` to \`migrations/*/migration.sql\` in wrangler.jsonc. + + " + `); + }); + + it("does not log a hint for nested .sql files that aren't named `migration.sql`", ({ + expect, + }) => { + // Drizzle's layout is specifically `/migration.sql`. + // A nested file with any other name (here `destroy.sql`) is not the + // drizzle layout, so no hint — the user may have intentionally placed + // a test-data dump or similar under their migrations_dir. + seedProjectFiles(["migrations/test_data/destroy.sql"]); + + maybeLogHint(migrationsConfig({ migrationsPattern: "migrations/*.sql" })); + + expect(std.warn).toBe(""); + }); + + it("does not log a hint when there are no nested files at all", ({ + expect, + }) => { + // Only top-level files (which the default pattern would have matched + // anyway). Nothing looks like the drizzle layout, so no hint. + seedProjectFiles(["migrations/0001_init.sql"]); + + maybeLogHint(migrationsConfig({ migrationsPattern: "migrations/*.sql" })); + + expect(std.warn).toBe(""); + }); +}); + +// These tests pin down the ordering contract of `getMigrationNames`. The +// rule is: sort by the integer at the start of the first path segment, then +// by full relative path lexicographically as a tiebreaker. This preserves the +// ordering the old explicit numeric-prefix comparator in apply.ts produced, +// while also being deterministic in the cases the old comparator left +// unsorted (same prefix, no prefix at all) and supporting nested layouts. +describe("getMigrationNames ordering", () => { + runInTempDir(); + mockConsoleMethods(); + + it("sorts zero-padded migrations in numeric order", ({ expect }) => { + // `wrangler d1 migrations create` produces 4-digit-padded prefixes. + // This is the common case and must work. + seedProjectFiles([ + "migrations/0010_j.sql", + "migrations/0002_b.sql", + "migrations/0001_a.sql", + "migrations/0100_h.sql", + ]); + + const result = getMigrationNames(migrationsConfig()); + + expect(result).toEqual([ + "0001_a.sql", + "0002_b.sql", + "0010_j.sql", + "0100_h.sql", + ]); + }); + + it("sorts inconsistently-padded numeric prefixes in numeric order, NOT lexicographic", ({ + expect, + }) => { + // A user with hand-written migrations `1_a.sql`, `9_b.sql`, + // `10_c.sql` expects them to run in numeric order [1, 9, 10] — not + // lexicographic order [10, 1, 9], which would interleave them. + // + // This is the case where the old explicit numeric-prefix comparator + // in apply.ts mattered: without it, a default `Array.sort()` would + // re-order existing users' migrations. We can't break that. + seedProjectFiles([ + "migrations/1_a.sql", + "migrations/9_b.sql", + "migrations/10_c.sql", + ]); + + const result = getMigrationNames(migrationsConfig()); + + expect(result).toEqual(["1_a.sql", "9_b.sql", "10_c.sql"]); + }); + + it("gives a deterministic order for files with the same numeric prefix (the old comparator returned 0)", ({ + expect, + }) => { + // The old comparator returned 0 for any two files with the same + // numeric prefix, leaving their relative order up to whatever the + // caller passed in. Lex tiebreak makes this deterministic. + seedProjectFiles([ + "migrations/0001_beta.sql", + "migrations/0001_alpha.sql", + "migrations/0001_gamma.sql", + ]); + + const result = getMigrationNames(migrationsConfig()); + + expect(result).toEqual([ + "0001_alpha.sql", + "0001_beta.sql", + "0001_gamma.sql", + ]); + }); + + it("gives a deterministic order for files without a numeric prefix (the old comparator returned 0)", ({ + expect, + }) => { + // `parseInt("init")` is `NaN`. The old comparator returned 0 for any + // pair with NaN parses; the new comparator falls back to lex order on + // the full relative path so ordering is deterministic. + seedProjectFiles([ + "migrations/migrate.sql", + "migrations/init.sql", + "migrations/seed.sql", + ]); + + const result = getMigrationNames(migrationsConfig()); + + expect(result).toEqual(["init.sql", "migrate.sql", "seed.sql"]); + }); + + it("puts numbered files before unnumbered files", ({ expect }) => { + // If a project mixes both styles, run the numbered migrations first + // (the user clearly knew about numeric ordering) and the unnumbered + // ones after, in alphabetical order. + seedProjectFiles([ + "migrations/setup.sql", + "migrations/0002_users.sql", + "migrations/cleanup.sql", + "migrations/0001_init.sql", + ]); + + const result = getMigrationNames(migrationsConfig()); + + expect(result).toEqual([ + "0001_init.sql", + "0002_users.sql", + "cleanup.sql", + "setup.sql", + ]); + }); + + it("uses the directory's numeric prefix for nested drizzle-style layouts", ({ + expect, + }) => { + // For `0001_a/migration.sql`, the number comes from the directory + // (`0001_a`), not the filename (`migration.sql`). That way users with + // inconsistently-padded directory numbers get the same numeric + // ordering they would for a flat layout. + seedProjectFiles([ + "migrations/10_c/migration.sql", + "migrations/2_b/migration.sql", + "migrations/1_a/migration.sql", + ]); + + const result = getMigrationNames( + migrationsConfig({ migrationsPattern: "migrations/*/migration.sql" }) + ); + + expect(result).toEqual([ + "1_a/migration.sql", + "2_b/migration.sql", + "10_c/migration.sql", + ]); + }); +}); + +describe("compareMigrationPaths", () => { + it("orders numbered files inside a shared numbered directory numerically", ({ + expect, + }) => { + const unsorted = [ + "0001_posts/10_c.sql", + "0001_posts/1_a.sql", + "0001_posts/9_b.sql", + ]; + + expect([...unsorted].sort(compareMigrationPaths)).toEqual([ + "0001_posts/1_a.sql", + "0001_posts/9_b.sql", + "0001_posts/10_c.sql", + ]); + }); +}); + +describe("getNextMigrationNumber", () => { + runInTempDir(); + // We don't assert on the hint output here, but `getMigrationNames` will + // log one when the configured pattern finds nothing but a drizzle-style + // layout exists — capture it so it doesn't pollute the test runner. + mockConsoleMethods(); + + it("returns 1 for an empty directory", ({ expect }) => { + fs.mkdirSync("migrations", { recursive: true }); + expect(getNextMigrationNumber(migrationsConfig())).toEqual(1); + }); + + it("returns highest top-level number + 1 for flat layouts (default pattern)", ({ + expect, + }) => { + seedProjectFiles([ + "migrations/0001_create.sql", + "migrations/0002_users.sql", + ]); + expect(getNextMigrationNumber(migrationsConfig())).toEqual(3); + }); + + it("counts numbered directories the same as numbered files when the pattern matches both", ({ + expect, + }) => { + // Mixed flat and nested layout, pattern matches both. `0099_nested/` + // contributes 99 to the set (the directory carries the number), so + // the next migration should be 100. + seedProjectFiles([ + "migrations/0001_create.sql", + "migrations/0002_users.sql", + "migrations/0099_nested/migration.sql", + ]); + expect( + getNextMigrationNumber( + migrationsConfig({ migrationsPattern: "migrations/**/*.sql" }) + ) + ).toEqual(100); + }); + + it("ignores files in unnumbered subdirectories (default pattern)", ({ + expect, + }) => { + // Default pattern is `migrations/*.sql`, so the nested file under + // `test_data/` doesn't participate at all. + seedProjectFiles([ + "migrations/0001_create.sql", + "migrations/README.md", + "migrations/test_data/destroy.sql", + ]); + expect(getNextMigrationNumber(migrationsConfig())).toEqual(2); + }); + + it("collapses multiple files inside a single numbered directory to one number", ({ + expect, + }) => { + // `0001_init/` is a single migration (the directory itself); even + // though there are three `.sql` files inside, they all live under + // the same numbered directory, so the highest number is 1 and the + // next is 2 — not 100. + seedProjectFiles([ + "migrations/0001_init/01.sql", + "migrations/0001_init/02.sql", + "migrations/0001_init/99.sql", + ]); + expect( + getNextMigrationNumber( + migrationsConfig({ migrationsPattern: "migrations/**/*.sql" }) + ) + ).toEqual(2); + }); + + // --- Pattern-aware semantics --- + + it("uses only files that match `migrations_pattern` — top-level files are invisible under a nested-only pattern", ({ + expect, + }) => { + // User has switched to a drizzle-style nested pattern but a stale + // top-level file is sitting in the dir. That file would not be + // applied (pattern doesn't match it), so it shouldn't influence + // numbering either. + seedProjectFiles([ + "migrations/0099_stale_topfile.sql", + "migrations/0001_init/migration.sql", + "migrations/0002_users/migration.sql", + ]); + expect( + getNextMigrationNumber( + migrationsConfig({ migrationsPattern: "migrations/*/migration.sql" }) + ) + ).toEqual(3); + }); + + it("uses only files that match `migrations_pattern` — nested files are invisible under a flat-only pattern", ({ + expect, + }) => { + // Mirror of the above. Drizzle-style files exist on disk but the + // configured pattern only matches top-level, so the nested ones + // don't contribute to numbering. + seedProjectFiles([ + "migrations/0001_create.sql", + "migrations/0099_drizzle/migration.sql", + ]); + expect( + getNextMigrationNumber( + migrationsConfig({ migrationsPattern: "migrations/*.sql" }) + ) + ).toEqual(2); + }); + + it("returns 1 when `migrations_pattern` matches nothing at all", ({ + expect, + }) => { + // User has migrations in nested directories but their pattern is + // the default `migrations/*.sql`. `getNextMigrationNumber` sees no + // matched files, so the next number is 1. (`getMigrationNames` + // will also log a drizzle hint here, captured by mockConsoleMethods.) + seedProjectFiles([ + "migrations/0001_init/migration.sql", + "migrations/0002_users/migration.sql", + ]); + expect(getNextMigrationNumber(migrationsConfig())).toEqual(1); + }); +}); + +describe("resolveMigrationsConfig", () => { + // Pure syntactic check — no filesystem involved. Each test reads as + // "given this `migrations_pattern` and `migrations_dir` from the user's + // config, does the helper accept or reject it?" + + /** + * Build a minimal `Database` for tests. Defaults give a binding with no + * `migrations_dir` / `migrations_pattern` set; overrides go in `props`. + */ + function databaseInfo(props: Partial = {}): Database { + return { + uuid: "x", + binding: "DB", + migrationsTableName: "d1_migrations", + ...props, + }; + } + + // --- No-op cases --- + + it("is a no-op when migrations_pattern is not set", ({ expect }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo(), + configPath: "wrangler.jsonc", + }) + ).not.toThrow(); + }); + + it("is a no-op when migrations_pattern is not set even if migrations_dir is", ({ + expect, + }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "migrations", + }), + configPath: "wrangler.jsonc", + }) + ).not.toThrow(); + }); + + // --- Accepted configurations --- + + it("accepts pattern that literally starts with migrations_dir", ({ + expect, + }) => { + const result = resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "migrations", + migrationsPattern: "migrations/*.sql", + }), + configPath: "wrangler.jsonc", + }); + expect(result.migrationsPattern).toBe("migrations/*.sql"); + }); + + it("accepts nested drizzle-style pattern", ({ expect }) => { + const result = resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "migrations", + migrationsPattern: "migrations/*/migration.sql", + }), + configPath: "wrangler.jsonc", + }); + expect(result.migrationsPattern).toBe("migrations/*/migration.sql"); + }); + + it("accepts deeply nested pattern with deeply nested dir", ({ expect }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "db/migrations", + migrationsPattern: "db/migrations/**/*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).not.toThrow(); + }); + + it("normalises `./` prefix on migrations_dir before comparing", ({ + expect, + }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "./migrations", + migrationsPattern: "migrations/*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).not.toThrow(); + }); + + it("normalises trailing `/` on migrations_dir before comparing", ({ + expect, + }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "migrations/", + migrationsPattern: "migrations/*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).not.toThrow(); + }); + + it("normalises `./` prefix on migrations_pattern before comparing", ({ + expect, + }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "migrations", + migrationsPattern: "./migrations/*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).not.toThrow(); + }); + + it("normalises a Windows drive-letter path with backslashes", ({ + expect, + }) => { + // A user on Windows may write an absolute, backslashed path in their + // config. `normalizeRelativePath` flips the backslashes to forward + // slashes (on every platform, so this test isn't Windows-gated) and + // the drive letter survives, so the dir and pattern line up and the + // resolved config carries forward-slash paths. This is the layer where + // backslashes are handled — `getMigrationNames` only ever sees the + // normalized result. (The end-to-end Windows walk is covered by the + // Windows-only test in the `getMigrationNames` block.) + const result = resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "C:\\some\\windows\\path", + migrationsPattern: "C:\\some\\windows\\path\\*\\migration.sql", + }), + configPath: "wrangler.jsonc", + }); + expect(result.migrationsDir).toBe("C:/some/windows/path"); + expect(result.migrationsPattern).toBe( + "C:/some/windows/path/*/migration.sql" + ); + // The pattern stays under the dir, so getMigrationNames can strip the + // prefix to walk relative to it. + expect( + result.migrationsPattern.startsWith(`${result.migrationsDir}/`) + ).toBe(true); + }); + + it('accepts `migrations_dir: "."` with a pattern rooted at the project root', ({ + expect, + }) => { + // A user can treat the project root itself as the migrations dir by + // setting `migrations_dir: "."`. Both `.` and `./` normalize to `.`, + // and a pattern like `./*.sql` normalizes to `*.sql`. The two are + // consistent — the pattern targets files directly inside the dir — + // so this should be accepted, not rejected by the "starts with" + // check. + const result = resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: ".", + migrationsPattern: "./*.sql", + }), + configPath: "wrangler.jsonc", + }); + expect(result.migrationsDir).toBe("."); + expect(result.migrationsPattern).toBe("*.sql"); + }); + + // --- Rejected: migrations_pattern set without migrations_dir --- + + it("rejects migrations_pattern set without an explicit migrations_dir, with an actionable hint", ({ + expect, + }) => { + const call = () => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: undefined, + migrationsPattern: "migrations/*.sql", + }), + configPath: "wrangler.jsonc", + }); + expect(call).toThrow(/have not set `migrations_dir`/); + // The error should also tell the user how to fix it (add a + // migrations_dir entry, with a worked example). + expect(call).toThrow( + /Add a `migrations_dir` entry.*`"migrations_dir": "migrations"`/s + ); + }); + + // --- Rejected: pattern doesn't start with dir --- + + it("rejects pattern with wrong literal prefix, with an actionable hint", ({ + expect, + }) => { + const call = () => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "migrations", + migrationsPattern: "schema/*.sql", + }), + configPath: "wrangler.jsonc", + }); + expect(call).toThrow(/must start with `migrations\/`/); + // The error should also tell the user the actionable fix: change + // migrations_pattern to start with the dir (with a worked example). + expect(call).toThrow( + /change `migrations_pattern`.*`"migrations\/\*\.sql"`/s + ); + }); + + it("rejects a top-level `*.sql` pattern when dir is a subdirectory", ({ + expect, + }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "migrations", + migrationsPattern: "*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).toThrow(/must start with `migrations\/`/); + }); + + it("rejects a globstar-prefix pattern (must start with the literal dir)", ({ + expect, + }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "migrations", + migrationsPattern: "**/*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).toThrow(/must start with `migrations\/`/); + }); + + it("rejects a pattern that only partially overlaps the dir name", ({ + expect, + }) => { + // "migrations" is a prefix of "migrationsfoo" as a *string*, but not as + // a *path segment*. The check requires the trailing `/` to avoid + // matching `migrations_foo/x.sql` as "starts with `migrations`". + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "migrations", + migrationsPattern: "migrationsfoo/*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).toThrow(/must start with `migrations\/`/); + }); + + // --- Absolute paths --- + + it("accepts matching absolute paths", ({ expect }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "/abs/migrations", + migrationsPattern: "/abs/migrations/*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).not.toThrow(); + }); + + it("rejects mismatched absolute paths", ({ expect }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "/abs/migrations", + migrationsPattern: "/abs/other/*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).toThrow(/must start with `\/abs\/migrations\/`/); + }); + + it("rejects mixing absolute dir with relative pattern", ({ expect }) => { + // The pattern must literally start with the (normalized) dir, so a + // relative pattern can't satisfy an absolute dir. + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "/abs/migrations", + migrationsPattern: "migrations/*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).toThrow(/must start with `\/abs\/migrations\/`/); + }); + + it("rejects mixing relative dir with absolute pattern", ({ expect }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "migrations", + migrationsPattern: "/abs/migrations/*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).toThrow(/must start with `migrations\/`/); + }); + + // --- Windows-style absolute paths (drive letters + backslashes) --- + + it("accepts Windows-style absolute paths with backslashes", ({ expect }) => { + // normalizeRelativePath flips `\` to `/` before comparing, so a user + // writing `C:\…\migrations` as a config value (which is natural on + // Windows) is accepted when the pattern uses the same prefix. + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "C:\\Users\\Dave\\proj\\migrations", + migrationsPattern: "C:\\Users\\Dave\\proj\\migrations\\*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).not.toThrow(); + }); + + it("accepts a Windows-style dir with a forward-slash pattern (typical config)", ({ + expect, + }) => { + // A user might write the dir in their config in the OS-native form + // (`C:\…\migrations`) while writing the pattern as a glob with + // forward slashes (which minimatch wants). Both normalize to the + // same form. + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "C:\\Users\\Dave\\proj\\migrations", + migrationsPattern: "C:/Users/Dave/proj/migrations/*/migration.sql", + }), + configPath: "wrangler.jsonc", + }) + ).not.toThrow(); + }); + + it("rejects mismatched Windows-style absolute paths", ({ expect }) => { + expect(() => + resolveMigrationsConfig({ + databaseInfo: databaseInfo({ + migrationsDirRaw: "C:\\Users\\Dave\\proj\\migrations", + migrationsPattern: "C:\\Users\\Dave\\proj\\other\\*.sql", + }), + configPath: "wrangler.jsonc", + }) + ).toThrow(/must start with `C:\/Users\/Dave\/proj\/migrations\/`/); + }); }); diff --git a/packages/wrangler/src/__tests__/d1/utils.test.ts b/packages/wrangler/src/__tests__/d1/utils.test.ts index b6050d4d9e..6bb7d7b206 100644 --- a/packages/wrangler/src/__tests__/d1/utils.test.ts +++ b/packages/wrangler/src/__tests__/d1/utils.test.ts @@ -37,7 +37,8 @@ describe("getDatabaseInfoFromConfig", () => { binding: "DATABASE", migrationsTableName: "d1_migrations", name: "db", - migrationsFolderPath: "./migrations", + migrationsDirRaw: undefined, + migrationsPattern: undefined, internal_env: undefined, }); }); @@ -61,7 +62,8 @@ describe("getDatabaseInfoFromConfig", () => { binding: "DATABASE", migrationsTableName: "d1_migrations", name: "db", - migrationsFolderPath: "./custom_migrations", + migrationsDirRaw: "./custom_migrations", + migrationsPattern: undefined, internal_env: undefined, }); }); @@ -85,7 +87,8 @@ describe("getDatabaseInfoFromConfig", () => { binding: "DATABASE", migrationsTableName: "custom_migrations", name: "db", - migrationsFolderPath: "./migrations", + migrationsDirRaw: undefined, + migrationsPattern: undefined, internal_env: undefined, }); }); @@ -105,7 +108,8 @@ describe("getDatabaseInfoFromConfig", () => { binding: "DATABASE2", migrationsTableName: "d1_migrations", name: "db2", - migrationsFolderPath: "./migrations", + migrationsDirRaw: undefined, + migrationsPattern: undefined, internal_env: undefined, }); }); diff --git a/packages/wrangler/src/d1/execute.ts b/packages/wrangler/src/d1/execute.ts index 007c25f8eb..56cf6504a1 100644 --- a/packages/wrangler/src/d1/execute.ts +++ b/packages/wrangler/src/d1/execute.ts @@ -121,6 +121,9 @@ export const d1ExecuteCommand = createCommand({ } if (file && command) { + if (json) { + logger.loggerLevel = existingLogLevel; + } throw createFatalError( `Error: can't provide both --command and --file.`, json, @@ -175,7 +178,6 @@ export const d1ExecuteCommand = createCommand({ } } catch (error) { if (json && error instanceof Error) { - logger.loggerLevel = existingLogLevel; const messageToDisplay = error.name === "APIError" ? error : { text: error.message }; throw new JsonFriendlyFatalError( @@ -185,6 +187,13 @@ export const d1ExecuteCommand = createCommand({ } else { throw error; } + } finally { + // Always restore the log level, including on the throwing paths + // above, so a singleton `logger` muted for `--json` output does not + // leak `"error"` to anything else that reuses it. + if (json) { + logger.loggerLevel = existingLogLevel; + } } }, }); @@ -222,40 +231,40 @@ export async function executeSql({ logger.loggerLevel = "error"; } - const input = file - ? ({ file } as ExecuteInput) - : command - ? ({ command } as ExecuteInput) - : null; - if (!input) { - throw new UserError(`Error: must provide --command or --file.`, { - telemetryMessage: "d1 execute missing command or file", - }); - } - if (local && remote) { - throw new UserError( - `Error: can't use --local and --remote at the same time`, - { - telemetryMessage: "d1 execute conflicting local and remote flags", - } - ); - } - if (preview && !remote) { - throw new UserError(`Error: can't use --preview without --remote`, { - telemetryMessage: "d1 execute preview requires remote", - }); - } - if (persistTo && !local) { - throw new UserError(`Error: can't use --persist-to without --local`, { - telemetryMessage: "d1 execute persist-to requires local", - }); - } - if (input.file) { - await checkForSQLiteBinary(input.file); - } + try { + const input = file + ? ({ file } as ExecuteInput) + : command + ? ({ command } as ExecuteInput) + : null; + if (!input) { + throw new UserError(`Error: must provide --command or --file.`, { + telemetryMessage: "d1 execute missing command or file", + }); + } + if (local && remote) { + throw new UserError( + `Error: can't use --local and --remote at the same time`, + { + telemetryMessage: "d1 execute conflicting local and remote flags", + } + ); + } + if (preview && !remote) { + throw new UserError(`Error: can't use --preview without --remote`, { + telemetryMessage: "d1 execute preview requires remote", + }); + } + if (persistTo && !local) { + throw new UserError(`Error: can't use --persist-to without --local`, { + telemetryMessage: "d1 execute persist-to requires local", + }); + } + if (input.file) { + await checkForSQLiteBinary(input.file); + } - const result = - remote || preview + return remote || preview ? await executeRemotely({ config, name, @@ -269,11 +278,11 @@ export async function executeSql({ input, persistTo, }); - - if (json) { - logger.loggerLevel = existingLogLevel; + } finally { + if (json) { + logger.loggerLevel = existingLogLevel; + } } - return result; } async function executeLocally({ diff --git a/packages/wrangler/src/d1/migrations/apply.ts b/packages/wrangler/src/d1/migrations/apply.ts index 6d4e07c505..9a3902886a 100644 --- a/packages/wrangler/src/d1/migrations/apply.ts +++ b/packages/wrangler/src/d1/migrations/apply.ts @@ -1,5 +1,4 @@ import fs from "node:fs"; -import path from "node:path"; import { configFileName, UserError } from "@cloudflare/workers-utils"; import dedent from "ts-dedent"; import { createCommand } from "../../core/create-command"; @@ -7,13 +6,13 @@ import { confirm } from "../../dialogs"; import { isNonInteractiveOrCI } from "../../is-interactive"; import { logger } from "../../logger"; import { isLocal } from "../../utils/is-local"; -import { DEFAULT_MIGRATION_PATH, DEFAULT_MIGRATION_TABLE } from "../constants"; import { executeSql } from "../execute"; import { getDatabaseInfoFromConfig } from "../utils"; import { getMigrationsPath, getUnappliedMigrations, initMigrationsTable, + resolveMigrationsConfig, } from "./helpers"; import type { ParseError } from "@cloudflare/workers-utils"; @@ -89,18 +88,20 @@ export const d1MigrationsApplyCommand = createCommand({ ); } + const migrationsConfig = resolveMigrationsConfig({ + databaseInfo: databaseInfo ?? null, + configPath: config.configPath, + }); const migrationsPath = await getMigrationsPath({ - projectPath: path.dirname(config.configPath), - migrationsFolderPath: - databaseInfo?.migrationsFolderPath ?? DEFAULT_MIGRATION_PATH, + projectPath: migrationsConfig.projectPath, + migrationsDir: migrationsConfig.migrationsDir, + migrationsDirRaw: migrationsConfig.migrationsDirRaw, createIfMissing: false, configPath: config.configPath, }); - const migrationsTableName = - databaseInfo?.migrationsTableName ?? DEFAULT_MIGRATION_TABLE; await initMigrationsTable({ - migrationsTableName, + migrationsTableName: migrationsConfig.migrationsTableName, local, remote, config, @@ -109,10 +110,14 @@ export const d1MigrationsApplyCommand = createCommand({ preview, }); + // `getUnappliedMigrations` returns paths already sorted by + // `compareMigrationPaths` in helpers.ts: numeric order on the first + // path segment's leading integer (matching the comparator this code + // used to do inline), with a lex tiebreaker for files that share a + // numeric prefix or have none. const unappliedMigrations = ( await getUnappliedMigrations({ - migrationsTableName, - migrationsPath, + migrationsConfig, local, remote, config, @@ -120,26 +125,12 @@ export const d1MigrationsApplyCommand = createCommand({ persistTo, preview, }) - ) - .map((migration) => { - return { - name: migration, - status: "🕒️", - }; - }) - .sort((a, b) => { - const migrationNumberA = parseInt(a.name.split("_")[0]); - const migrationNumberB = parseInt(b.name.split("_")[0]); - if (migrationNumberA < migrationNumberB) { - return -1; - } - if (migrationNumberA > migrationNumberB) { - return 1; - } - - // numbers must be equal - return 0; - }); + ).map((migration) => { + return { + name: migration, + status: "🕒️", + }; + }); if (unappliedMigrations.length === 0) { logger.log("✅ No migrations to apply!"); @@ -162,7 +153,7 @@ Your database may not be available to serve requests during the migration, conti "utf8" ); query += ` - INSERT INTO ${migrationsTableName} (name) + INSERT INTO ${migrationsConfig.migrationsTableName} (name) values ('${migration.name}'); `; diff --git a/packages/wrangler/src/d1/migrations/create.ts b/packages/wrangler/src/d1/migrations/create.ts index cd267640be..462e9e92db 100644 --- a/packages/wrangler/src/d1/migrations/create.ts +++ b/packages/wrangler/src/d1/migrations/create.ts @@ -1,12 +1,16 @@ import fs from "node:fs"; -import path from "node:path"; import { configFileName, UserError } from "@cloudflare/workers-utils"; +import { Minimatch } from "minimatch"; import dedent from "ts-dedent"; import { createCommand } from "../../core/create-command"; import { logger } from "../../logger"; -import { DEFAULT_MIGRATION_PATH } from "../constants"; import { getDatabaseInfoFromConfig } from "../utils"; -import { getMigrationsPath, getNextMigrationNumber } from "./helpers"; +import { + getMigrationsPath, + getNextMigrationNumber, + normalizeRelativePath, + resolveMigrationsConfig, +} from "./helpers"; export const d1MigrationsCreateCommand = createCommand({ metadata: { @@ -59,18 +63,78 @@ export const d1MigrationsCreateCommand = createCommand({ ); } - const migrationsPath = await getMigrationsPath({ - projectPath: path.dirname(config.configPath), - migrationsFolderPath: - databaseInfo.migrationsFolderPath ?? DEFAULT_MIGRATION_PATH, - createIfMissing: true, + const migrationsConfig = resolveMigrationsConfig({ + databaseInfo, configPath: config.configPath, }); - const nextMigrationNumber = pad(getNextMigrationNumber(migrationsPath), 4); + + const nextMigrationNumber = pad( + getNextMigrationNumber(migrationsConfig), + 4 + ); const migrationName = message.replaceAll(" ", "_"); + // `wrangler d1 migrations create` writes a single file directly inside + // `migrations_dir`, so a name containing a path separator can't work — + // it would imply nested directories. Reject it up front with a clear + // message, rather than letting it fall through to the confusing "does + // not match migrations_pattern" error below (a name like `foo/bar` + // produces an extra path segment that the pattern can't match). + if (/[\\/]/.test(migrationName)) { + throw new UserError( + `The migration name ${JSON.stringify(message)} contains a path separator ("/" or "\\"). Please remove this and try again.`, + { + telemetryMessage: "d1 migrations create name contains path separator", + } + ); + } + const newMigrationName = `${nextMigrationNumber}_${migrationName}.sql`; + // Make sure the migration we are about to create will actually be picked + // up by `wrangler d1 migrations apply` — i.e. it matches the configured + // `migrations_pattern`. The default `${migrations_dir}/*.sql` always + // matches top-level `.sql` files, so this only fires for a user-set + // pattern. + // + // This runs BEFORE `getMigrationsPath` so we don't prompt the user to + // create a `migrations_dir` we'll then refuse to write into. + const matcher = new Minimatch(migrationsConfig.migrationsPattern, { + dot: false, + }); + // Normalize so the path is shaped like the entries + // `getMigrationNames` matches against `migrationsPattern` — both + // `migrationsDir` and `migrationsPattern` are normalized, so the + // proposed path must be too. In particular, for `migrations_dir: "."` + // this strips the leading `./` (which would otherwise split into a + // separate segment and never match the normalized pattern). + const proposedPath = normalizeRelativePath( + `${migrationsConfig.migrationsDir}/${newMigrationName}` + ); + if (!matcher.match(proposedPath)) { + throw new UserError( + dedent` + Wrangler would like to make a new migration called \`${proposedPath}\` but it does not match the configured \`migrations_pattern: "${migrationsConfig.migrationsPattern}"\` in your ${migrationsConfig.configFile} file, so \`wrangler d1 migrations apply\` would not pick it up. \`wrangler d1 migrations create\` only writes top-level files inside \`migrations_dir\`. + + If you are using an ORM like drizzle to manage migrations, use the ORM's command (e.g. \`drizzle-kit generate\`) instead of \`wrangler d1 migrations create\` — it will create files in the nested layout your \`migrations_pattern\` expects. + + Otherwise, change \`migrations_pattern\` in your ${migrationsConfig.configFile} file to match top-level \`.sql\` files (for example, \`${migrationsConfig.migrationsDir}/*.sql\`). + `, + { + telemetryMessage: + "d1 migrations create new file does not match migrations_pattern", + } + ); + } + + const migrationsPath = await getMigrationsPath({ + projectPath: migrationsConfig.projectPath, + migrationsDir: migrationsConfig.migrationsDir, + migrationsDirRaw: migrationsConfig.migrationsDirRaw, + createIfMissing: true, + configPath: config.configPath, + }); + fs.writeFileSync( `${migrationsPath}/${newMigrationName}`, `-- Migration number: ${nextMigrationNumber} \t ${new Date().toISOString()}\n` diff --git a/packages/wrangler/src/d1/migrations/helpers.ts b/packages/wrangler/src/d1/migrations/helpers.ts index 4792b7185b..6bb6661c2a 100644 --- a/packages/wrangler/src/d1/migrations/helpers.ts +++ b/packages/wrangler/src/d1/migrations/helpers.ts @@ -1,33 +1,174 @@ import fs from "node:fs"; import path from "node:path"; import { configFileName, UserError } from "@cloudflare/workers-utils"; +import { Minimatch } from "minimatch"; import { confirm } from "../../dialogs"; import { isNonInteractiveOrCI } from "../../is-interactive"; import { logger } from "../../logger"; -import { DEFAULT_MIGRATION_PATH } from "../constants"; +import { DEFAULT_MIGRATION_PATH, DEFAULT_MIGRATION_TABLE } from "../constants"; import { executeSql } from "../execute"; import type { QueryResult } from "../execute"; -import type { Migration } from "../types"; +import type { Database, Migration } from "../types"; import type { Config } from "@cloudflare/workers-utils"; +function getDefaultMigrationsPattern(migrationsDir: string) { + return normalizeRelativePath(`${migrationsDir}/*.sql`); +} + +/** + * Fully-resolved view of the D1 migrations configuration for one binding. + * Build with {@link resolveMigrationsConfig}. + * + * Field invariants: + * - `migrationsDir` is normalized (forward slashes, no leading `./`, + * no trailing `/`) and not empty. `"."` is the project root — the user + * can set it to treat the project directory itself as the migrations dir. + * - `migrationsPattern` is normalized in the same way, and is under + * `migrationsDir` — i.e. {@link stripDirPrefix} can rewrite it relative + * to `migrationsDir` without throwing. (When `migrationsDir` is `"."` the + * pattern carries no prefix, since normalization strips any leading `./`.) + * - `projectPath` is the directory containing the user's Wrangler config — + * the base that `migrationsDir` and `migrationsPattern` resolve against. + * - `configFile` is the short display name (e.g. `"wrangler.jsonc"`) used + * in error messages. + */ +export type MigrationsConfig = { + projectPath: string; + configFile: string; + migrationsDir: string; + migrationsPattern: string; + migrationsTableName: string; + migrationsDirRaw?: string; +}; + +/** + * Resolve the migrations-related config for one D1 binding into a + * `MigrationsConfig`, throwing a `UserError` if `migrations_pattern` is set + * without `migrations_dir`, or doesn't start with `${migrations_dir}/`. + * + * `databaseInfo` may be `null` — `apply --local` and `list --local` pass + * `null` when the binding can't be found, and fall back to the defaults + * so the commands surface "no migrations found" instead of crashing. + */ +export function resolveMigrationsConfig({ + databaseInfo, + configPath, +}: { + databaseInfo: Database | null; + configPath: string; +}): MigrationsConfig { + const configFile = configFileName(configPath); + const projectPath = path.dirname(configPath); + + const rawDir = databaseInfo?.migrationsDirRaw; + const rawPattern = databaseInfo?.migrationsPattern; + + if (rawPattern !== undefined && rawDir === undefined) { + throw new UserError( + `You have set \`migrations_pattern: "${rawPattern}"\` in your ${configFile} file but have not set \`migrations_dir\` for this D1 binding.\n\n` + + `When \`migrations_pattern\` is set, \`migrations_dir\` must also be set, and \`migrations_pattern\` must start with \`\${migrations_dir}/\`. Add a \`migrations_dir\` entry to your ${configFile} file (for example, \`"migrations_dir": "migrations"\`).`, + { + telemetryMessage: + "d1 migrations migrations_pattern set without migrations_dir", + } + ); + } + + const migrationsDir = normalizeRelativePath( + databaseInfo?.migrationsDirRaw ?? DEFAULT_MIGRATION_PATH + ); + + let migrationsPattern: string; + if (rawPattern === undefined) { + migrationsPattern = getDefaultMigrationsPattern(migrationsDir); + } else { + migrationsPattern = normalizeRelativePath(rawPattern); + try { + // Called for its throw-on-non-prefix side effect + stripDirPrefix(migrationsPattern, migrationsDir); + } catch { + const suggestedPattern = getDefaultMigrationsPattern(migrationsDir); + throw new UserError( + `The configured \`migrations_pattern: "${rawPattern}"\` in your ${configFile} file must start with \`${migrationsDir}/\` to match \`"migrations_dir": "${migrationsDir}"\`.\n\n` + + `Either change \`migrations_pattern\` so it starts with \`${migrationsDir}/\` (for example, \`"${suggestedPattern}"\`), or change \`migrations_dir\` to match the start of your pattern.`, + { + telemetryMessage: + "d1 migrations migrations_pattern does not start with migrations_dir", + } + ); + } + } + + const migrationsTableName = + databaseInfo?.migrationsTableName ?? DEFAULT_MIGRATION_TABLE; + + return { + projectPath, + configFile, + migrationsDir, + migrationsPattern, + migrationsTableName, + migrationsDirRaw: rawDir, + }; +} + +/** + * Normalize a relative path or glob into a canonical form for string-prefix + * comparisons: + * + * - Backslashes flipped to forward slashes. + * - Leading `./` and `//` runs collapsed (via `path.posix.normalize`). + * - Trailing `/` stripped (`normalize("foo/")` keeps it; we don't want it). + */ +/** + * Rewrite `pattern` relative to `dir` by stripping the `${dir}/` prefix. Both + * `pattern` and `dir` must already be normalized (see + * {@link normalizeRelativePath}). + * + * Throws if `pattern` is not under `dir`. + */ +function stripDirPrefix(pattern: string, dir: string): string { + if (dir === ".") { + return pattern; + } + const prefix = `${dir}/`; + if (!pattern.startsWith(prefix)) { + throw new Error( + `Expected migrations pattern ${JSON.stringify(pattern)} to start with ${JSON.stringify(prefix)}` + ); + } + return pattern.slice(prefix.length); +} + +export function normalizeRelativePath(p: string): string { + const forwardSlashed = p.replace(/\\/g, "/"); + const normalized = path.posix.normalize(forwardSlashed); + if (normalized.endsWith("/")) { + return normalized.slice(0, -1); + } + return normalized; +} + export async function getMigrationsPath({ projectPath, - migrationsFolderPath, + migrationsDir, + migrationsDirRaw, createIfMissing, configPath, }: { projectPath: string; - migrationsFolderPath: string; + migrationsDir: string; + migrationsDirRaw: string | undefined; createIfMissing: boolean; configPath: string | undefined; }): Promise { - const dir = path.resolve(projectPath, migrationsFolderPath); + const dir = path.resolve(projectPath, migrationsDir); if (fs.existsSync(dir)) { return dir; } const warning = `No migrations folder found.${ - migrationsFolderPath === DEFAULT_MIGRATION_PATH + migrationsDirRaw === undefined ? ` Set \`migrations_dir\` in your ${configFileName(configPath)} file to choose a different path.` : "" }`; @@ -45,8 +186,7 @@ export async function getMigrationsPath({ } export async function getUnappliedMigrations({ - migrationsTableName, - migrationsPath, + migrationsConfig, local, remote, config, @@ -54,8 +194,7 @@ export async function getUnappliedMigrations({ persistTo, preview, }: { - migrationsTableName: string; - migrationsPath: string; + migrationsConfig: MigrationsConfig; local: boolean | undefined; remote: boolean | undefined; config: Config; @@ -65,7 +204,7 @@ export async function getUnappliedMigrations({ }): Promise> { const appliedMigrations = ( await listAppliedMigrations({ - migrationsTableName, + migrationsTableName: migrationsConfig.migrationsTableName, local, remote, config, @@ -76,7 +215,10 @@ export async function getUnappliedMigrations({ ).map((migration) => { return migration.name; }); - const projectMigrations = getMigrationNames(migrationsPath); + const projectMigrations = getMigrationNames(migrationsConfig); + if (projectMigrations.length === 0) { + maybeLogHint(migrationsConfig); + } const unappliedMigrations: Array = []; @@ -130,33 +272,196 @@ const listAppliedMigrations = async ({ return response[0].results as Migration[]; }; -export function getMigrationNames(migrationsPath: string): Array { - const migrations = []; +/** + * Recursively list regular files under `dir` whose `dir`-relative path + * matches `matcher` (a `Minimatch` whose pattern is also `dir`-relative). + * + * Paths use forward-slash separators (so they match globs the same on POSIX + * and Windows), sorted by {@link compareMigrationPaths}. + * + * Prunes the walk with minimatch's `partial: true` mode: before descending + * into a subdirectory we ask whether its relative path could be a prefix of + * something matching `matcher.pattern`. If not, we skip the descent. So a + * `*.sql` pattern never recurses, `*\/migration.sql` only descends one + * level, `**\/*.sql` recurses unconditionally. + */ +function listFilesRelative(dir: string, matcher: Minimatch): string[] { + const out: string[] = []; + const stack: Array<{ abs: string; rel: string }> = [{ abs: dir, rel: "" }]; + + while (stack.length > 0) { + const { abs, rel } = stack.pop() as { abs: string; rel: string }; + let entries: fs.Dirent[]; + try { + entries = fs.readdirSync(abs, { withFileTypes: true }); + } catch { + continue; + } + for (const entry of entries) { + const childRel = rel === "" ? entry.name : `${rel}/${entry.name}`; + if (entry.isDirectory()) { + if (matcher.match(childRel, true /* partial */)) { + stack.push({ abs: path.join(abs, entry.name), rel: childRel }); + } + } else if (entry.isFile() && matcher.match(childRel)) { + out.push(childRel); + } + } + } + + return out.sort(compareMigrationPaths); +} - const dir = fs.opendirSync(migrationsPath); +/** + * Compare two migration paths by the leading integer of in each path + * segment, falling back to lex order on ties. Numbered files sort before + * unnumbered ones. + * + * Numeric ordering matters for users with inconsistently-padded numeric + * prefixes (`1_a.sql`, `9_b.sql`, `10_c.sql`); a pure lex sort would put + * `10_c.sql` between `1_a.sql` and `9_b.sql`. + */ +export function compareMigrationPaths(a: string, b: string): number { + const aSegments = a.split("/"); + const bSegments = b.split("/"); + const shared = Math.min(aSegments.length, bSegments.length); + for (let i = 0; i < shared; i++) { + const cmp = compareSegments(aSegments[i], bSegments[i]); + if (cmp !== 0) { + return cmp; + } + } + // Every shared segment is equal: the shorter path sorts first (e.g. + // `0001_a` before `0001_a/migration.sql`). This is impossible because + // listFilesRelative() will never output a directory. + return aSegments.length - bSegments.length; +} - let dirent; - while ((dirent = dir.readSync()) !== null) { - if (dirent.name.endsWith(".sql")) { - migrations.push(dirent.name); +function compareSegments(a: string, b: string): number { + const aNum = leadingMigrationNumber(a); + const bNum = leadingMigrationNumber(b); + if (aNum !== bNum) { + // `NaN !== NaN` is true, so unprefixed paths hit this branch. Guard + // with isFinite to fall through to the lex tiebreaker below. + if (Number.isFinite(aNum) && Number.isFinite(bNum)) { + return aNum - bNum; + } + // Numbered files sort before unnumbered ones. + if (Number.isFinite(aNum)) { + return -1; } + if (Number.isFinite(bNum)) { + return 1; + } + } + // Same number, or both unnumbered: lex order for determinism. + if (a < b) { + return -1; } + if (a > b) { + return 1; + } + return 0; +} + +/** + * Parse the leading integer from a migration's first path segment. + * - `0001_init.sql` → `1` + * - `0001_init/migration.sql` → `1` (directory carries the number, as in + * drizzle-style layouts) + * - `init.sql` → `NaN` + */ +function leadingMigrationNumber(relativePath: string): number { + const firstSegment = relativePath.split("/")[0]; + return parseInt(firstSegment.split("_")[0], 10); +} + +/** + * Returns migration names matching `migrationsPattern`, as paths relative to + * `migrationsDir` with forward-slash separators (e.g. `0000_init/migration.sql`). + * + * Walk root is `projectPath/migrationsDir`. Each file is matched against + * `migrationsPattern` interpreted as a glob relative to `projectPath` (i.e. + * against `${migrationsDir}/${relativePath}`). + * + * If no files match but `*\/migration.sql` (drizzle's layout) matches files + * on disk, logs a hint to stderr suggesting that pattern. + */ +export function getMigrationNames({ + projectPath, + migrationsDir, + migrationsPattern, +}: MigrationsConfig): Array { + const walkRoot = path.resolve(projectPath, migrationsDir); - dir.closeSync(); + // `listFilesRelative` returns paths relative to `walkRoot`, so the + // matcher must also be `migrationsDir`-relative. The MigrationsConfig + // invariant guarantees the pattern is under migrationsDir, so this never + // throws. + const dirRelativePattern = stripDirPrefix(migrationsPattern, migrationsDir); + const matches = listFilesRelative( + walkRoot, + new Minimatch(dirRelativePattern, { dot: false }) + ); - return migrations.toSorted(); + return matches; } /** - * Returns the highest current migration number plus one, ignoring any missing numbers. + * If the configured `migrations_pattern` found nothing but `*\/migration.sql` + * (drizzle's layout) matches files under `migrations_dir`, point the user + * at the right pattern. Runs its own narrow walk — only called in the + * no-matches branch. */ -export function getNextMigrationNumber(migrationsPath: string): number { - const migrationNumbers = getMigrationNames(migrationsPath).map((migration) => - parseInt(migration.split("_")[0]) +export function maybeLogHint({ + projectPath, + migrationsDir, + migrationsPattern, + configFile, +}: Pick< + MigrationsConfig, + "projectPath" | "migrationsDir" | "migrationsPattern" | "configFile" +>) { + const walkRoot = path.resolve(projectPath, migrationsDir); + const drizzleFiles = listFilesRelative( + walkRoot, + new Minimatch("*/migration.sql", { dot: false }) + ); + if (drizzleFiles.length === 0) { + return; + } + const drizzlePattern = normalizeRelativePath( + `${migrationsDir}/*/migration.sql` ); - const highestMigrationNumber = Math.max(...migrationNumbers, 0); + logger.warn( + `Could not find any migration files matching \`${migrationsPattern}\`. It looks like there are migration files matching \`${drizzlePattern}\` though. If you are using drizzle to manage your migrations, please set \`migrations_pattern\` to \`${drizzlePattern}\` in ${configFile}.` + ); +} - return highestMigrationNumber + 1; +/** + * Returns the highest current migration number plus one. + * + * Numbers come from the leading integer of each matched migration's first + * path segment: + * - `0001_init.sql` → 1 (flat layout) + * - `0003_init/migration.sql` → 3 (drizzle-style; directory carries the + * number, and multiple files inside it + * collapse to that one number) + * + * Only files that match `migrationsPattern` participate — a stray top-level + * `0099_x.sql` is invisible when the pattern only matches + * `migrations/*\/migration.sql`, because `apply` wouldn't run it either. + */ +export function getNextMigrationNumber( + migrationsConfig: MigrationsConfig +): number { + const matchedNames = getMigrationNames(migrationsConfig); + const migrationNumbers = matchedNames + .map((name) => leadingMigrationNumber(name)) + // Drop unnumbered migrations (parseInt → NaN) so they don't poison + // Math.max. + .filter((n) => Number.isFinite(n)); + return Math.max(...migrationNumbers, 0) + 1; } export const initMigrationsTable = async ({ diff --git a/packages/wrangler/src/d1/migrations/list.ts b/packages/wrangler/src/d1/migrations/list.ts index fddb8cf08e..e1350ed8bf 100644 --- a/packages/wrangler/src/d1/migrations/list.ts +++ b/packages/wrangler/src/d1/migrations/list.ts @@ -1,15 +1,14 @@ -import path from "node:path"; import { configFileName, UserError } from "@cloudflare/workers-utils"; import { createCommand } from "../../core/create-command"; import { logger } from "../../logger"; import { requireAuth } from "../../user"; import { isLocal } from "../../utils/is-local"; -import { DEFAULT_MIGRATION_PATH, DEFAULT_MIGRATION_TABLE } from "../constants"; import { getDatabaseInfoFromConfig } from "../utils"; import { getMigrationsPath, getUnappliedMigrations, initMigrationsTable, + resolveMigrationsConfig, } from "./helpers"; export const d1MigrationsListCommand = createCommand({ @@ -74,19 +73,24 @@ export const d1MigrationsListCommand = createCommand({ ); } - const migrationsPath = await getMigrationsPath({ - projectPath: path.dirname(config.configPath), - migrationsFolderPath: - databaseInfo?.migrationsFolderPath ?? DEFAULT_MIGRATION_PATH, + const migrationsConfig = resolveMigrationsConfig({ + databaseInfo: databaseInfo ?? null, + configPath: config.configPath, + }); + // Side-effect only: confirm the migrations dir exists (or surface an + // actionable error). The returned absolute path is not used in `list` + // because `getUnappliedMigrations` resolves files itself from + // `projectPath` + `migrationsPattern`. + await getMigrationsPath({ + projectPath: migrationsConfig.projectPath, + migrationsDir: migrationsConfig.migrationsDir, + migrationsDirRaw: migrationsConfig.migrationsDirRaw, createIfMissing: false, configPath: config.configPath, }); - const migrationsTableName = - databaseInfo?.migrationsTableName ?? DEFAULT_MIGRATION_TABLE; - await initMigrationsTable({ - migrationsTableName, + migrationsTableName: migrationsConfig.migrationsTableName, local, remote, config, @@ -97,8 +101,7 @@ export const d1MigrationsListCommand = createCommand({ const unappliedMigrations = ( await getUnappliedMigrations({ - migrationsTableName, - migrationsPath, + migrationsConfig, local, remote, config, diff --git a/packages/wrangler/src/d1/types.ts b/packages/wrangler/src/d1/types.ts index 523409965b..06d8390589 100644 --- a/packages/wrangler/src/d1/types.ts +++ b/packages/wrangler/src/d1/types.ts @@ -5,7 +5,17 @@ export type Database = { binding: string; internal_env?: string; migrationsTableName: string; - migrationsFolderPath: string; + /** + * The raw `migrations_dir` value the user set in their Wrangler config, + * or undefined if they did not set one. + */ + migrationsDirRaw?: string; + /** + * Optional glob (relative to the Wrangler config file) for discovering + * migration files. When not set, callers should default to + * `${migrationsDirRaw ?? DEFAULT_MIGRATION_PATH}/*.sql`. + */ + migrationsPattern?: string; }; export type DatabaseCreationResult = { diff --git a/packages/wrangler/src/d1/utils.ts b/packages/wrangler/src/d1/utils.ts index b424751141..298a7c20e8 100644 --- a/packages/wrangler/src/d1/utils.ts +++ b/packages/wrangler/src/d1/utils.ts @@ -1,6 +1,6 @@ import { UserError } from "@cloudflare/workers-utils"; import { fetchResult } from "../cfetch"; -import { DEFAULT_MIGRATION_PATH, DEFAULT_MIGRATION_TABLE } from "./constants"; +import { DEFAULT_MIGRATION_TABLE } from "./constants"; import { listDatabases } from "./list"; import type { Database, DatabaseInfo } from "./types"; import type { ComplianceConfig, Config } from "@cloudflare/workers-utils"; @@ -40,8 +40,8 @@ export function getDatabaseInfoFromConfig( name: d1Database.database_name, migrationsTableName: d1Database.migrations_table || DEFAULT_MIGRATION_TABLE, - migrationsFolderPath: - d1Database.migrations_dir || DEFAULT_MIGRATION_PATH, + migrationsDirRaw: d1Database.migrations_dir, + migrationsPattern: d1Database.migrations_pattern, internal_env: d1Database.database_internal_env, }; } From c4f45e8b8694c60fb1808f7fbb130e4b4893d20c Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Wed, 3 Jun 2026 10:17:42 +0100 Subject: [PATCH 2/5] Simplify `constructWranglerConfig` to accept a single worker instead of an array (#14146) --- .../simplify-construct-wrangler-config.md | 7 + .../src/construct-wrangler-config.ts | 58 +------ .../tests/construct-wrangler-config.test.ts | 143 +----------------- 3 files changed, 16 insertions(+), 192 deletions(-) create mode 100644 .changeset/simplify-construct-wrangler-config.md diff --git a/.changeset/simplify-construct-wrangler-config.md b/.changeset/simplify-construct-wrangler-config.md new file mode 100644 index 0000000000..69a85f2709 --- /dev/null +++ b/.changeset/simplify-construct-wrangler-config.md @@ -0,0 +1,7 @@ +--- +"@cloudflare/workers-utils": minor +--- + +Simplify `constructWranglerConfig` to accept a single worker instead of an array + +The `constructWranglerConfig` function now accepts a single `APIWorkerConfig` object instead of `APIWorkerConfig | APIWorkerConfig[]`. The multi-environment array support has been removed since the array use-case was removed and now the only call site already passes a single worker object. This is a breaking change to the function's public signature. diff --git a/packages/workers-utils/src/construct-wrangler-config.ts b/packages/workers-utils/src/construct-wrangler-config.ts index 2078bbb257..608483a354 100644 --- a/packages/workers-utils/src/construct-wrangler-config.ts +++ b/packages/workers-utils/src/construct-wrangler-config.ts @@ -1,5 +1,4 @@ import { getTodaysCompatDate } from "./compatibility-date"; -import { ENVIRONMENT_TAG_PREFIX, SERVICE_TAG_PREFIX } from "./constants"; import { mapWorkerMetadataBindings } from "./map-worker-metadata-bindings"; import type { RawConfig } from "./config"; import type { @@ -48,7 +47,14 @@ interface APIWorkerConfig { }; } -function convertWorkerToWranglerConfig(config: APIWorkerConfig): RawConfig { +/** + * Given information about a Worker, + * construct a Wrangler config file for the application. + * + * @param config - The Worker configuration sourced from the Cloudflare API + * @returns A Wrangler-compatible raw config object + */ +export function constructWranglerConfig(config: APIWorkerConfig): RawConfig { const mappedBindings = mapWorkerMetadataBindings(config.bindings); const durableObjectClassNames = config.bindings @@ -109,51 +115,3 @@ function convertWorkerToWranglerConfig(config: APIWorkerConfig): RawConfig { ...mappedBindings, }; } - -/** - * Given the information of multiple Workers (representing different environments), - * construct a Wrangler config file for the application. - */ -export function constructWranglerConfig( - workerOrWorkers: APIWorkerConfig | APIWorkerConfig[] -): RawConfig { - let workers: APIWorkerConfig[]; - if (Array.isArray(workerOrWorkers)) { - workers = workerOrWorkers; - } else { - workers = [workerOrWorkers]; - } - - const topLevelEnv = workers.find( - (w) => !w.tags?.some((t) => t.startsWith(ENVIRONMENT_TAG_PREFIX)) - ); - const workerName = topLevelEnv?.name ?? workers[0].name; - const entrypoint = topLevelEnv?.entrypoint ?? workers[0].entrypoint; - let combinedConfig: RawConfig; - if (topLevelEnv) { - combinedConfig = convertWorkerToWranglerConfig(topLevelEnv); - } else { - // Make a synthetic top level environment - combinedConfig = { - name: workerName, - main: entrypoint, - }; - } - - for (const env of workers) { - const serviceTag = env.tags?.find( - (t) => t === `${SERVICE_TAG_PREFIX}${workerName}` - ); - const envTag = env.tags?.find((t) => t.startsWith(ENVIRONMENT_TAG_PREFIX)); - if ( - serviceTag !== `${SERVICE_TAG_PREFIX}${workerName}` || - envTag === undefined - ) { - continue; - } - const [_, envName] = envTag.split("="); - combinedConfig.env ??= {}; - combinedConfig.env[envName] = convertWorkerToWranglerConfig(env); - } - return combinedConfig; -} diff --git a/packages/workers-utils/tests/construct-wrangler-config.test.ts b/packages/workers-utils/tests/construct-wrangler-config.test.ts index 0c696da6bc..071cc057d7 100644 --- a/packages/workers-utils/tests/construct-wrangler-config.test.ts +++ b/packages/workers-utils/tests/construct-wrangler-config.test.ts @@ -1,9 +1,7 @@ import { describe, it } from "vitest"; -import { ENVIRONMENT_TAG_PREFIX, SERVICE_TAG_PREFIX } from "../src/constants"; import { constructWranglerConfig } from "../src/construct-wrangler-config"; -type APIWorkerConfigOrArray = Parameters[0]; -type APIWorkerConfig = Exclude; +type APIWorkerConfig = Parameters[0]; /** * Factory for a minimal valid APIWorkerConfig. @@ -36,22 +34,6 @@ function makeWorkerConfig( } describe("constructWranglerConfig", () => { - describe("input normalization", () => { - it("accepts a single worker (non-array)", ({ expect }) => { - const config = makeWorkerConfig(); - const result = constructWranglerConfig(config); - expect(result.name).toBe("my-worker"); - expect(result.main).toBe("index.js"); - }); - - it("accepts an array with one worker", ({ expect }) => { - const config = makeWorkerConfig(); - const result = constructWranglerConfig([config]); - expect(result.name).toBe("my-worker"); - expect(result.main).toBe("index.js"); - }); - }); - describe("basic field mapping", () => { it("maps core fields correctly", ({ expect }) => { const config = makeWorkerConfig({ @@ -391,127 +373,4 @@ describe("constructWranglerConfig", () => { expect(JSON.stringify(result)).not.toContain("s3cret"); }); }); - - describe("multi-environment support", () => { - it("uses top-level worker as base config and tagged workers as environments", ({ - expect, - }) => { - const topLevel = makeWorkerConfig({ - name: "my-worker", - entrypoint: "index.js", - tags: [], - compatibility_date: "2025-01-01", - }); - const staging = makeWorkerConfig({ - name: "my-worker-staging", - entrypoint: "index.js", - tags: [ - `${SERVICE_TAG_PREFIX}my-worker`, - `${ENVIRONMENT_TAG_PREFIX}staging`, - ], - compatibility_date: "2025-02-01", - }); - const production = makeWorkerConfig({ - name: "my-worker-production", - entrypoint: "index.js", - tags: [ - `${SERVICE_TAG_PREFIX}my-worker`, - `${ENVIRONMENT_TAG_PREFIX}production`, - ], - compatibility_date: "2025-03-01", - }); - - const result = constructWranglerConfig([topLevel, staging, production]); - - expect(result.name).toBe("my-worker"); - expect(result.compatibility_date).toBe("2025-01-01"); - expect(result.env).toBeDefined(); - expect(result.env?.staging).toBeDefined(); - expect(result.env?.staging?.compatibility_date).toBe("2025-02-01"); - expect(result.env?.production).toBeDefined(); - expect(result.env?.production?.compatibility_date).toBe("2025-03-01"); - }); - - it("creates synthetic top-level when no untagged worker exists", ({ - expect, - }) => { - const staging = makeWorkerConfig({ - name: "my-worker-staging", - entrypoint: "src/index.ts", - tags: [ - `${ENVIRONMENT_TAG_PREFIX}staging`, - `${SERVICE_TAG_PREFIX}my-worker-staging`, - ], - }); - const production = makeWorkerConfig({ - name: "my-worker-prod", - entrypoint: "src/index.ts", - tags: [ - `${ENVIRONMENT_TAG_PREFIX}production`, - `${SERVICE_TAG_PREFIX}my-worker-staging`, - ], - }); - - const result = constructWranglerConfig([staging, production]); - - // Synthetic top-level should use workers[0] for name and entrypoint - expect(result.name).toBe("my-worker-staging"); - expect(result.main).toBe("src/index.ts"); - // Should not have compatibility_date etc. since it's synthetic - expect(result.compatibility_date).toBeUndefined(); - }); - - it("skips workers with mismatched service tags", ({ expect }) => { - const topLevel = makeWorkerConfig({ - name: "my-worker", - tags: [], - }); - const matchingEnv = makeWorkerConfig({ - name: "my-worker-staging", - tags: [ - `${SERVICE_TAG_PREFIX}my-worker`, - `${ENVIRONMENT_TAG_PREFIX}staging`, - ], - }); - const mismatchedEnv = makeWorkerConfig({ - name: "other-worker-prod", - tags: [ - `${SERVICE_TAG_PREFIX}other-worker`, - `${ENVIRONMENT_TAG_PREFIX}production`, - ], - }); - - const result = constructWranglerConfig([ - topLevel, - matchingEnv, - mismatchedEnv, - ]); - - expect(result.env?.staging).toBeDefined(); - expect(result.env?.production).toBeUndefined(); - }); - - it("skips workers without an environment tag", ({ expect }) => { - const topLevel = makeWorkerConfig({ - name: "my-worker", - tags: [], - }); - const noEnvTag = makeWorkerConfig({ - name: "my-worker-noenv", - tags: [`${SERVICE_TAG_PREFIX}my-worker`], - }); - - const result = constructWranglerConfig([topLevel, noEnvTag]); - expect(result.env).toBeUndefined(); - }); - - it("handles null tags by treating worker as top-level", ({ expect }) => { - const config = makeWorkerConfig({ tags: null }); - const result = constructWranglerConfig(config); - // Worker with null tags is treated as having no tags, - // so it becomes the top-level environment - expect(result.name).toBe("my-worker"); - expect(result.main).toBe("index.js"); - }); - }); }); From 0bb2d55116ce90a147582a7b4d96e3090cddf7ee Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Wed, 3 Jun 2026 11:43:24 +0100 Subject: [PATCH 3/5] In non-interactive mode remove the skills installation message (#14162) --- .changeset/good-ducks-clean.md | 11 +++++++++++ .../src/__tests__/agents-skills-install.test.ts | 8 ++++---- packages/wrangler/src/agents-skills-install.ts | 5 +---- 3 files changed, 16 insertions(+), 8 deletions(-) create mode 100644 .changeset/good-ducks-clean.md diff --git a/.changeset/good-ducks-clean.md b/.changeset/good-ducks-clean.md new file mode 100644 index 0000000000..3a27ee4520 --- /dev/null +++ b/.changeset/good-ducks-clean.md @@ -0,0 +1,11 @@ +--- +"wrangler": patch +--- + +In non-interactive mode remove the skills installation message + +When Wrangler run in non interactive mode and it detected agents that it could install skills for, it would print a message such as: + +`Cloudflare agent skills are available for: . Run wrangler in an interactive terminal to install them, or use '--install-skills' to install without prompting.` + +This message seems to be confusing and unhelpful so it has now been removed. diff --git a/packages/wrangler/src/__tests__/agents-skills-install.test.ts b/packages/wrangler/src/__tests__/agents-skills-install.test.ts index 742c840575..f233cb09de 100644 --- a/packages/wrangler/src/__tests__/agents-skills-install.test.ts +++ b/packages/wrangler/src/__tests__/agents-skills-install.test.ts @@ -322,7 +322,7 @@ describe("maybeInstallCloudflareSkillsGlobally", () => { }); }); - test("logs info and sends skills_install_skipped when TTY is false", async ({ + test("sends skills_install_skipped without logging anything in the terminal when TTY is false", async ({ expect, }) => { setIsTTY(false); @@ -330,9 +330,9 @@ describe("maybeInstallCloudflareSkillsGlobally", () => { await maybeInstallCloudflareSkillsGlobally(false); - expect(std.out).toContain( - "Cloudflare agent skills are available for: Claude Code" - ); + // Nothing has been logged + expect(std.out).toEqual(""); + // Verify no install call was made expect(mockRosieInstall).not.toHaveBeenCalled(); expect(sendMetricsEvent).toHaveBeenCalledWith( diff --git a/packages/wrangler/src/agents-skills-install.ts b/packages/wrangler/src/agents-skills-install.ts index 560bc7fb30..8ba80be4d7 100644 --- a/packages/wrangler/src/agents-skills-install.ts +++ b/packages/wrangler/src/agents-skills-install.ts @@ -87,11 +87,8 @@ export async function maybeInstallCloudflareSkillsGlobally( return; } - // In non-interactive terminals (but not CI), log a message + // In non-interactive terminals do nothing if (!force && !isInteractive()) { - logger.log( - `Cloudflare agent skills are available for: ${detectedAgents.map(({ name }) => name).join(", ")}. Run wrangler in an interactive terminal to install them, or use \`--install-skills\` to install without prompting.` - ); sendResultMetricsEvent({ skippedBecause: "Non-interactive terminal", }); From c2280cdb589c9289bb4082d0a068846f3dd22b37 Mon Sep 17 00:00:00 2001 From: MatinGathani <70268627+matingathani@users.noreply.github.com> Date: Wed, 3 Jun 2026 16:49:05 +0530 Subject: [PATCH 4/5] [wrangler] fix: warn when named env silently inherits custom_domain routes (#13979) --- .../warn-custom-domain-route-inheritance.md | 7 +++ .../workers-utils/src/config/validation.ts | 32 ++++++++++- .../__tests__/config/configuration.test.ts | 57 +++++++++++++++++++ 3 files changed, 93 insertions(+), 3 deletions(-) create mode 100644 .changeset/warn-custom-domain-route-inheritance.md diff --git a/.changeset/warn-custom-domain-route-inheritance.md b/.changeset/warn-custom-domain-route-inheritance.md new file mode 100644 index 0000000000..154e6ae06f --- /dev/null +++ b/.changeset/warn-custom-domain-route-inheritance.md @@ -0,0 +1,7 @@ +--- +"wrangler": patch +--- + +Warn when a named environment silently inherits custom_domain routes from the top-level config + +When an `env.` block does not override `routes`, it inherits the top-level `routes` array. If that array contains entries with `custom_domain: true`, every deploy to the named environment will silently reassign the custom domain away from the top-level Worker and towards the env Worker, causing routing drift. Wrangler now emits a warning in this situation and suggests adding `"routes": []` to the env block to prevent inheritance. diff --git a/packages/workers-utils/src/config/validation.ts b/packages/workers-utils/src/config/validation.ts index 56e6639ee0..83969a0e56 100644 --- a/packages/workers-utils/src/config/validation.ts +++ b/packages/workers-utils/src/config/validation.ts @@ -42,6 +42,7 @@ import type { Assets, CacheOptions, ContainerApp, + CustomDomainRoute, DispatchNamespaceOutbound, Environment, Observability, @@ -1134,9 +1135,10 @@ function normalizeAndValidateRoute( function validateRoutes( diagnostics: Diagnostics, topLevelEnv: Environment | undefined, - rawEnv: RawEnvironment + rawEnv: RawEnvironment, + envName?: string ): Config["routes"] { - return inheritable( + const result = inheritable( diagnostics, topLevelEnv, rawEnv, @@ -1144,6 +1146,25 @@ function validateRoutes( all(isRouteArray, isMutuallyExclusiveWith(rawEnv, "route")), undefined ); + + if ( + topLevelEnv !== undefined && + envName !== undefined && + rawEnv.routes === undefined + ) { + const customDomainRoutes = topLevelEnv.routes?.filter( + (r): r is CustomDomainRoute => + typeof r === "object" && r !== null && r.custom_domain === true + ); + if (customDomainRoutes && customDomainRoutes.length > 0) { + const customDomains = customDomainRoutes.map((r) => r.pattern).join(", "); + diagnostics.warnings.push( + `The "env.${envName}" environment inherits the top-level \`routes\` configuration, which includes the custom domain(s): ${customDomains}. Deploying this environment will reassign these custom domains away from the top-level Worker. Add \`"routes": []\` to "env.${envName}" to prevent inheritance, or copy the route configuration from the top level to hide this warning.` + ); + } + } + + return result; } function normalizeAndValidatePlacement( @@ -1456,7 +1477,12 @@ function normalizeAndValidateEnvironment( undefined ); - const routes = validateRoutes(diagnostics, topLevelEnv, rawEnv); + const routes = validateRoutes( + diagnostics, + topLevelEnv, + rawEnv, + topLevelEnv === undefined ? undefined : envName + ); const workers_dev = inheritable( diagnostics, diff --git a/packages/wrangler/src/__tests__/config/configuration.test.ts b/packages/wrangler/src/__tests__/config/configuration.test.ts index 894c711597..33221dafde 100644 --- a/packages/wrangler/src/__tests__/config/configuration.test.ts +++ b/packages/wrangler/src/__tests__/config/configuration.test.ts @@ -248,3 +248,60 @@ compatibility_date = "2022-01-12"`; expect(config.name).toBe("no-bom-test"); }); }); + +describe("readConfig() custom_domain inheritance warning", () => { + runInTempDir(); + const std = mockConsoleMethods(); + + it("should warn when a named env inherits custom_domain routes from the top-level config", async ({ + expect, + }) => { + writeWranglerConfig({ + routes: [{ pattern: "api.example.com", custom_domain: true }], + env: { + staging: { + name: "my-worker-staging", + workers_dev: true, + // no `routes` override — will inherit top-level custom domain + }, + }, + }); + readConfig({ config: "wrangler.toml", env: "staging" }); + await endEventLoop(); + expect(std.warn).toContain( + `"env.staging" environment inherits the top-level` + ); + expect(std.warn).toContain(`api.example.com`); + expect(std.warn).toContain(`Add \`"routes": []\``); + expect(std.warn).toContain(`prevent`); + expect(std.warn).toContain( + `copy the route configuration from the top level` + ); + }); + + it("should not warn when the env explicitly overrides routes", async ({ + expect, + }) => { + writeWranglerConfig({ + routes: [{ pattern: "api.example.com", custom_domain: true }], + env: { + staging: { + name: "my-worker-staging", + routes: [], + }, + }, + }); + readConfig({ config: "wrangler.toml", env: "staging" }); + await endEventLoop(); + expect(std.warn).not.toContain("inherits the top-level `routes`"); + }); + + it("should not warn for the top-level env itself", async ({ expect }) => { + writeWranglerConfig({ + routes: [{ pattern: "api.example.com", custom_domain: true }], + }); + readConfig({ config: "wrangler.toml" }); + await endEventLoop(); + expect(std.warn).not.toContain("inherits the top-level `routes`"); + }); +}); From 7a6b1a4f4e9d8d5bd88732c8e11368c3ad7f867b Mon Sep 17 00:00:00 2001 From: Dario Piotrowicz Date: Wed, 3 Jun 2026 15:06:22 +0100 Subject: [PATCH 5/5] Generalize `wrangler deploy` positional argument from `[script]` to `[path]` (#14153) Co-authored-by: emily-shen <69125074+emily-shen@users.noreply.github.com> --- .changeset/deploy-path-positional.md | 12 ++ .../src/__tests__/deploy/entry-points.test.ts | 167 ++++++++++++++---- packages/wrangler/src/__tests__/index.test.ts | 4 +- .../__tests__/versions/versions.help.test.ts | 4 +- .../versions/versions.upload.test.ts | 84 +++++++++ packages/wrangler/src/deploy/autoconfig.ts | 89 ++-------- packages/wrangler/src/deploy/index.ts | 8 +- .../src/deployment-bundle/deploy-args.ts | 79 ++++++++- packages/wrangler/src/versions/upload.ts | 4 +- 9 files changed, 328 insertions(+), 123 deletions(-) create mode 100644 .changeset/deploy-path-positional.md diff --git a/.changeset/deploy-path-positional.md b/.changeset/deploy-path-positional.md new file mode 100644 index 0000000000..021bd13ead --- /dev/null +++ b/.changeset/deploy-path-positional.md @@ -0,0 +1,12 @@ +--- +"wrangler": minor +--- + +Generalize `wrangler deploy` and `wrangler versions upload` positional argument from `[script]` to `[path]` + +Both `wrangler deploy` and `wrangler versions upload` now accept a generic `[path]` positional argument that can point to either a Worker entry-point file or a directory of static assets. The type is auto-detected. For example: + +- **File**: `wrangler deploy ./src/index.ts` deploys a Worker (same as before) +- **Directory**: `wrangler deploy ./public` deploys a static assets site (no interactive confirmation prompt) + +The `--script` named option is now hidden and deprecated for both commands. It continues to work for backwards compatibility but only accepts file paths. Passing a directory to `--script` now produces a clear error message suggesting the positional `path` argument or `--assets` flag instead. diff --git a/packages/wrangler/src/__tests__/deploy/entry-points.test.ts b/packages/wrangler/src/__tests__/deploy/entry-points.test.ts index 1277f9c4b9..3405328c14 100644 --- a/packages/wrangler/src/__tests__/deploy/entry-points.test.ts +++ b/packages/wrangler/src/__tests__/deploy/entry-points.test.ts @@ -887,10 +887,6 @@ addEventListener('fetch', event => {});` // TODO: remove this test once autoconfig goes GA and its experimental opt-in flag is removed it("should handle `wrangler deploy `", async ({ expect }) => { - mockConfirm({ - text: "It looks like you are trying to deploy a directory of static assets only. Is this correct?", - result: true, - }); mockPrompt({ text: "What do you want to name your project?", options: { defaultValue: "my-site" }, @@ -934,8 +930,6 @@ addEventListener('fetch', event => {});` ────────────────── - - Wrote { "name": "test-name", @@ -972,10 +966,6 @@ addEventListener('fetch', event => {});` const runAutoConfigSpy = (await import("../../autoconfig/run")) .runAutoConfig; - mockConfirm({ - text: "It looks like you are trying to deploy a directory of static assets only. Is this correct?", - result: true, - }); mockPrompt({ text: "What do you want to name your project?", options: { defaultValue: "my-site" }, @@ -1019,8 +1009,6 @@ addEventListener('fetch', event => {});` ────────────────── - - Wrote { "name": "test-name", @@ -1199,7 +1187,7 @@ addEventListener('fetch', event => {});` expect(runAutoConfigSpy).not.toHaveBeenCalled(); }); - it("should not trigger autoconfig on `wrangler deploy