Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/d1-binding-config-validation.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"@cloudflare/workers-utils": patch
---

Validate optional configuration fields for D1 database bindings

Enforce type checks for the optional D1 database properties `database_name`, `migrations_dir`, `migrations_table`, and `database_internal_env` to ensure consistency with other binding types.
5 changes: 5 additions & 0 deletions .changeset/types-ignore-parent-directory-dts.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"wrangler": patch
---

Fix false-positive validation error in `wrangler types` when a custom `worker-configuration.d.ts` file exists in a parent directory.
36 changes: 36 additions & 0 deletions packages/workers-utils/src/config/validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4153,6 +4153,42 @@ const validateD1Binding: ValidatorFn = (diagnostics, field, value) => {
isValid = false;
}

if (!isOptionalProperty(value, "database_name", "string")) {
diagnostics.errors.push(
`"${field}" bindings should, optionally, have a string "database_name" field but got ${JSON.stringify(
value
)}.`
);
isValid = false;
}

if (!isOptionalProperty(value, "migrations_dir", "string")) {
diagnostics.errors.push(
`"${field}" bindings should, optionally, have a string "migrations_dir" field but got ${JSON.stringify(
value
)}.`
);
isValid = false;
}

if (!isOptionalProperty(value, "migrations_table", "string")) {
diagnostics.errors.push(
`"${field}" bindings should, optionally, have a string "migrations_table" field but got ${JSON.stringify(
value
)}.`
);
isValid = false;
}

if (!isOptionalProperty(value, "database_internal_env", "string")) {
diagnostics.errors.push(
`"${field}" bindings should, optionally, have a string "database_internal_env" field but got ${JSON.stringify(
value
)}.`
);
isValid = false;
}

validateAdditionalProperties(diagnostics, field, Object.keys(value), [
"binding",
"database_id",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3700,6 +3700,106 @@ describe("normalizeAndValidateConfig()", () => {
expect(diagnostics.hasWarnings()).toBe(false);
expect(diagnostics.hasErrors()).toBe(false);
});

it("should error if D1 database database_name has incorrect type", ({
expect,
}) => {
const { diagnostics } = normalizeAndValidateConfig(
{
d1_databases: [
{
binding: "DB",
database_name: true,
},
],
} as unknown as RawConfig,
undefined,
undefined,
{ env: undefined }
);

expect(diagnostics.hasWarnings()).toBe(false);
expect(diagnostics.hasErrors()).toBe(true);
expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
"Processing wrangler configuration:
- "d1_databases[0]" bindings should, optionally, have a string "database_name" field but got {"binding":"DB","database_name":true}."
`);
});

it("should error if D1 database migrations_dir has incorrect type", ({
expect,
}) => {
const { diagnostics } = normalizeAndValidateConfig(
{
d1_databases: [
{
binding: "DB",
migrations_dir: 123,
},
],
} as unknown as RawConfig,
undefined,
undefined,
{ env: undefined }
);

expect(diagnostics.hasWarnings()).toBe(false);
expect(diagnostics.hasErrors()).toBe(true);
expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
"Processing wrangler configuration:
- "d1_databases[0]" bindings should, optionally, have a string "migrations_dir" field but got {"binding":"DB","migrations_dir":123}."
`);
});

it("should error if D1 database migrations_table has incorrect type", ({
expect,
}) => {
const { diagnostics } = normalizeAndValidateConfig(
{
d1_databases: [
{
binding: "DB",
migrations_table: {},
},
],
} as unknown as RawConfig,
undefined,
undefined,
{ env: undefined }
);

expect(diagnostics.hasWarnings()).toBe(false);
expect(diagnostics.hasErrors()).toBe(true);
expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
"Processing wrangler configuration:
- "d1_databases[0]" bindings should, optionally, have a string "migrations_table" field but got {"binding":"DB","migrations_table":{}}."
`);
});

it("should error if D1 database database_internal_env has incorrect type", ({
expect,
}) => {
const { diagnostics } = normalizeAndValidateConfig(
{
d1_databases: [
{
binding: "DB",
database_internal_env: false,
},
],
} as unknown as RawConfig,
undefined,
undefined,
{ env: undefined }
);

expect(diagnostics.hasWarnings()).toBe(false);
expect(diagnostics.hasErrors()).toBe(true);
expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
"Processing wrangler configuration:
- "d1_databases[0]" bindings should, optionally, have a string "database_internal_env" field but got {"binding":"DB","database_internal_env":false}."
`);
});
});

describe("[hyperdrive]", () => {
Expand Down
37 changes: 37 additions & 0 deletions packages/wrangler/src/__tests__/type-generation.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as fs from "node:fs";
import path from "node:path";
import { runInTempDir } from "@cloudflare/workers-utils/test-helpers";
import { http, HttpResponse } from "msw";
import { afterAll, beforeAll, beforeEach, describe, it, vi } from "vitest";
Expand Down Expand Up @@ -3405,7 +3406,7 @@
"env",
];

for (const path of invalidPaths) {

Check failure on line 3409 in packages/wrangler/src/__tests__/type-generation.test.ts

View workflow job for this annotation

GitHub Actions / Checks

eslint(no-shadow)

'path' is already declared in the upper scope.
await expect(runWrangler(`types ${path}`)).rejects.toThrow(
/The provided output path '.*?' does not point to a declaration file/
);
Expand All @@ -3429,6 +3430,42 @@
"At least one of --include-env or --include-runtime must be enabled."
);
});

it("should ignore a non-Wrangler types file in a parent directory", async ({
expect,
}) => {
const parentDir = path.resolve("..");
const parentTypesPath = path.join(
parentDir,
"worker-configuration.d.ts"
);

fs.writeFileSync(parentTypesPath, "export interface CustomEnv {}");

try {
fs.writeFileSync(
"./index.ts",
"export default { async fetch() {} };"
);
fs.writeFileSync(
"./wrangler.jsonc",
JSON.stringify({
compatibility_date: "2022-01-12",
name: "test-name",
main: "./index.ts",
}),
"utf-8"
);

await runWrangler("types");

expect(fs.existsSync("./worker-configuration.d.ts")).toBe(true);
} finally {
try {
fs.unlinkSync(parentTypesPath);
} catch {}
}
});
Comment on lines +3434 to +3468

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔍 Test writes to parent of temp directory, potential parallel test interference

The new test at line 3434 writes worker-configuration.d.ts into path.resolve("..") — the parent of the runInTempDir() sandbox. If multiple test shards or parallel runs use sibling temp directories under the same parent (e.g., /tmp/), this file could collide with another test's parent directory check. The finally block cleans it up, but a crash before cleanup or a parallel read could cause flakiness. Consider using a dedicated isolated directory instead of .., or at minimum using a unique filename.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

});

it("should allow multiple customizations to be applied together", async ({
Expand Down
9 changes: 6 additions & 3 deletions packages/wrangler/src/type-generation/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
UserError,
} from "@cloudflare/workers-utils";
import chalk from "chalk";
import * as find from "empathic/find";

Check failure on line 13 in packages/wrangler/src/type-generation/index.ts

View workflow job for this annotation

GitHub Actions / Checks

eslint(no-unused-vars)

Identifier 'find' is imported but never used.
import { getNodeCompat } from "miniflare";
import yargs from "yargs";
import { readConfig } from "../config";
Expand Down Expand Up @@ -1781,13 +1781,16 @@
* @throws {UserError} If a non-Wrangler .d.ts file already exists at the given path.
*/
const validateTypesFile = (path: string): void => {
const wranglerOverrideDTSPath = find.file(path);
if (wranglerOverrideDTSPath === undefined) {
if (!fs.existsSync(path)) {
return;
}
const stats = fs.statSync(path);
if (!stats.isFile()) {
return;
}

try {
const fileContent = fs.readFileSync(wranglerOverrideDTSPath, "utf8");
const fileContent = fs.readFileSync(path, "utf8");
if (
!fileContent.includes("Generated by Wrangler") &&
!fileContent.includes("Runtime types generated with workerd")
Expand Down
Loading