fix: ensure that non-inherited fields are not removed when using an i…

…nferred named environment (#7478) It is an error for the the user to provide an environment name that doesn't match any of the named environments in the Wrangler configuration. But if there are no named environments defined at all in the Wrangler configuration, we special case the top-level environment as though it was a named environment. Previously, when this happens, we would remove all the nonInheritable fields from the configuration (essentially all the bindings) leaving an incorrect configuration. Now we correctly generate a flattened named environment that has the nonInheritable fields, plus correctly applies any transformFn on inheritable fields.
cloudflare · Dec 12, 2024 · 2e90efc · 2e90efc
1 parent ee98fd4
commit 2e90efc
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 63 deletions.
diff --git a/.changeset/khaki-steaks-sniff.md b/.changeset/khaki-steaks-sniff.md
@@ -0,0 +1,10 @@
+---
+"wrangler": patch
+---
+
+fix: ensure that non-inherited fields are not removed when using an inferred named environment
+
+It is an error for the the user to provide an environment name that doesn't match any of the named environments in the Wrangler configuration.
+But if there are no named environments defined at all in the Wrangler configuration, we special case the top-level environment as though it was a named environment.
+Previously, when this happens, we would remove all the nonInheritable fields from the configuration (essentially all the bindings) leaving an incorrect configuration.
+Now we correctly generate a flattened named environment that has the nonInheritable fields, plus correctly applies any transformFn on inheritable fields.
diff --git a/packages/wrangler/src/__tests__/configuration.test.ts b/packages/wrangler/src/__tests__/configuration.test.ts
@@ -3854,22 +3854,35 @@ describe("normalizeAndValidateConfig()", () => {
 
 	describe("named environments", () => {
 		it("should warn if we specify an environment but there are no named environments", () => {
-			const rawConfig: RawConfig = {};
-			const { diagnostics } = normalizeAndValidateConfig(rawConfig, undefined, {
-				env: "DEV",
-			});
+			const rawConfig: RawConfig = {
+				name: "my-worker",
+				kv_namespaces: [{ binding: "KV", id: "xxxx-xxxx-xxxx-xxxx" }],
+			};
+			const { diagnostics, config } = normalizeAndValidateConfig(
+				rawConfig,
+				undefined,
+				{
+					env: "dev",
+				}
+			);
+			expect(config).toEqual(
+				expect.objectContaining({
+					name: "my-worker-dev",
+					kv_namespaces: [{ binding: "KV", id: "xxxx-xxxx-xxxx-xxxx" }],
+				})
+			);
 			expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
 			        "Processing wrangler configuration:
 			        "
 		      `);
 			expect(diagnostics.renderWarnings()).toMatchInlineSnapshot(`
 				"Processing wrangler configuration:
-				  - No environment found in configuration with name \\"DEV\\".
-				    Before using \`--env=DEV\` there should be an equivalent environment section in the configuration.
+				  - No environment found in configuration with name \\"dev\\".
+				    Before using \`--env=dev\` there should be an equivalent environment section in the configuration.
 
 				    Consider adding an environment configuration section to the Wrangler configuration file:
 				    \`\`\`
-				    [env.DEV]
+				    [env.dev]
 				    \`\`\`
 				"
 			`);
@@ -4111,70 +4124,70 @@ describe("normalizeAndValidateConfig()", () => {
 			                Please remove the field from this environment."
 		        `);
 			});
-		});
 
-		it("should error if named environment contains a `account_id` field, even if there is no top-level name", () => {
-			const rawConfig: RawConfig = {
-				legacy_env: false,
-				env: {
-					DEV: {
-						account_id: "some_account_id",
+			it("should error if named environment contains a `account_id` field, even if there is no top-level name", () => {
+				const rawConfig: RawConfig = {
+					legacy_env: false,
+					env: {
+						DEV: {
+							account_id: "some_account_id",
+						},
 					},
-				},
-			};
+				};
 
-			const { config, diagnostics } = normalizeAndValidateConfig(
-				rawConfig,
-				undefined,
-				{ env: "DEV" }
-			);
+				const { config, diagnostics } = normalizeAndValidateConfig(
+					rawConfig,
+					undefined,
+					{ env: "DEV" }
+				);
 
-			expect(diagnostics.hasWarnings()).toBe(true);
-			expect(config.account_id).toBeUndefined();
-			expect(diagnostics.renderWarnings()).toMatchInlineSnapshot(`
-			        "Processing wrangler configuration:
-			          - Experimental: Service environments are in beta, and their behaviour is guaranteed to change in the future. DO NOT USE IN PRODUCTION."
-		      `);
-			expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
-			        "Processing wrangler configuration:
+				expect(diagnostics.hasWarnings()).toBe(true);
+				expect(config.account_id).toBeUndefined();
+				expect(diagnostics.renderWarnings()).toMatchInlineSnapshot(`
+					"Processing wrangler configuration:
+					  - Experimental: Service environments are in beta, and their behaviour is guaranteed to change in the future. DO NOT USE IN PRODUCTION."
+				`);
+				expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
+					"Processing wrangler configuration:
 
-			          - \\"env.DEV\\" environment configuration
-			            - The \\"account_id\\" field is not allowed in named service environments.
-			              Please remove the field from this environment."
-		      `);
-		});
+					  - \\"env.DEV\\" environment configuration
+					    - The \\"account_id\\" field is not allowed in named service environments.
+					      Please remove the field from this environment."
+				`);
+			});
 
-		it("should error if top-level config and a named environment both contain a `account_id` field", () => {
-			const rawConfig: RawConfig = {
-				account_id: "ACCOUNT_ID",
-				legacy_env: false,
-				env: {
-					DEV: {
-						account_id: "ENV_ACCOUNT_ID",
+			it("should error if top-level config and a named environment both contain a `account_id` field", () => {
+				const rawConfig: RawConfig = {
+					account_id: "ACCOUNT_ID",
+					legacy_env: false,
+					env: {
+						DEV: {
+							account_id: "ENV_ACCOUNT_ID",
+						},
 					},
-				},
-			};
+				};
 
-			const { config, diagnostics } = normalizeAndValidateConfig(
-				rawConfig,
-				undefined,
-				{ env: "DEV" }
-			);
+				const { config, diagnostics } = normalizeAndValidateConfig(
+					rawConfig,
+					undefined,
+					{ env: "DEV" }
+				);
 
-			expect(config.account_id).toEqual("ACCOUNT_ID");
-			expect(diagnostics.hasErrors()).toBe(true);
-			expect(diagnostics.hasWarnings()).toBe(true);
-			expect(diagnostics.renderWarnings()).toMatchInlineSnapshot(`
-			        "Processing wrangler configuration:
-			          - Experimental: Service environments are in beta, and their behaviour is guaranteed to change in the future. DO NOT USE IN PRODUCTION."
-		      `);
-			expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
-			        "Processing wrangler configuration:
+				expect(config.account_id).toEqual("ACCOUNT_ID");
+				expect(diagnostics.hasErrors()).toBe(true);
+				expect(diagnostics.hasWarnings()).toBe(true);
+				expect(diagnostics.renderWarnings()).toMatchInlineSnapshot(`
+					"Processing wrangler configuration:
+					  - Experimental: Service environments are in beta, and their behaviour is guaranteed to change in the future. DO NOT USE IN PRODUCTION."
+				`);
+				expect(diagnostics.renderErrors()).toMatchInlineSnapshot(`
+					"Processing wrangler configuration:
 
-			          - \\"env.DEV\\" environment configuration
-			            - The \\"account_id\\" field is not allowed in named service environments.
-			              Please remove the field from this environment."
-		      `);
+					  - \\"env.DEV\\" environment configuration
+					    - The \\"account_id\\" field is not allowed in named service environments.
+					      Please remove the field from this environment."
+				`);
+			});
 		});
 
 		it("should warn for non-inherited fields that are missing in environments", () => {

diff --git a/packages/wrangler/src/config/validation-helpers.ts b/packages/wrangler/src/config/validation-helpers.ts
@@ -69,7 +69,11 @@ export function inheritable<K extends keyof Environment>(
 ): Environment[K] {
 	validate(diagnostics, field, rawEnv[field], topLevelEnv);
 	return (
-		(rawEnv[field] as Environment[K]) ??
+		// `rawEnv === topLevelEnv` is a special case where the user has provided an environment name
+		// but that named environment is not actually defined in the configuration.
+		// In that case we have reused the topLevelEnv as the rawEnv,
+		// and so we need to process the `transformFn()` anyway rather than just using the field in the `rawEnv`.
+		(rawEnv !== topLevelEnv ? (rawEnv[field] as Environment[K]) : undefined) ??
 		transformFn(topLevelEnv?.[field]) ??
 		defaultValue
 	);

diff --git a/packages/wrangler/src/config/validation.ts b/packages/wrangler/src/config/validation.ts
@@ -224,7 +224,7 @@ export function normalizeAndValidateConfig(
 			activeEnv = normalizeAndValidateEnvironment(
 				envDiagnostics,
 				configPath,
-				{},
+				topLevelEnv, // in this case reuse the topLevelEnv to ensure that nonInherited fields are not removed
 				isDispatchNamespace,
 				envName,
 				topLevelEnv,