Skip to content

Commit e1ac0ac

Browse files
matt-aitkenmatt-aitken
committed
fix(webapp): tighten branch env var prune after parent-level import
The branch-copy prune after an import now deletes every qualifying value row, its secret and its reference in a single transaction using the rows it already fetched, instead of re-reading the project and the whole variable once per key. The import only offers keys for pruning that the parent write actually accepted, so a value the parent rejected (empty or disallowed) can no longer cause the branch to fall back to an older parent value. Keys the same request also sets on the branch are still excluded.
1 parent a082b62 commit e1ac0ac

7 files changed

Lines changed: 184 additions & 19 deletions

‎.server-changes/envvar-import-prune-branch-copies.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,4 @@ area: webapp
33
type: fix
44
---
55

6-
Importing environment variables to a preview branch's parent now removes that same source's earlier copies of those variables on the branch, so the branch picks up the shared value instead of a stale override. Variables set on the branch by anyone else are left untouched.
6+
When an integration syncs a variable to a preview branch's parent, its earlier copy of that variable on the branch is removed so the branch inherits the shared value. Values set on the branch by anyone else are kept.

‎apps/webapp/app/routes/api.v1.projects.$projectRef.envvars.$slug.import.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
} from "~/services/environmentVariableApiAccess.server";
1414
import { logger } from "~/services/logger.server";
1515
import { EnvironmentVariablesRepository } from "~/v3/environmentVariables/environmentVariablesRepository.server";
16+
import { parentKeysToPrune } from "~/v3/environmentVariables/parentKeysToPrune";
1617

1718
const ParamsSchema = z.object({
1819
projectRef: z.string(),
@@ -84,12 +85,15 @@ export async function action({ params, request }: ActionFunctionArgs) {
8485
});
8586

8687
if (parentResult.success && body.source) {
87-
const branchKeys = new Set(Object.keys(body.variables));
8888
await pruneBranchCopies(repository, {
8989
projectId: environment.project.id,
9090
environmentId: environment.id,
9191
parentEnvironmentId: environment.parentEnvironmentId,
92-
keys: Object.keys(body.parentVariables).filter((key) => !branchKeys.has(key)),
92+
keys: parentKeysToPrune({
93+
variables: body.variables,
94+
parentVariables: body.parentVariables,
95+
acceptedParentKeys: parentResult.acceptedKeys ?? [],
96+
}),
9397
source: body.source,
9498
});
9599
}

‎apps/webapp/app/v3/environmentVariables/environmentVariablesRepository.server.ts‎

Lines changed: 53 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,7 @@ export class EnvironmentVariablesRepository implements Repository {
149149
options.lastUpdatedBy?.type === "integration" &&
150150
!removedBlacklisted
151151
) {
152-
return { success: true as const };
152+
return { success: true as const, acceptedKeys: [] };
153153
}
154154
}
155155
if (values.length === 0) {
@@ -313,6 +313,7 @@ export class EnvironmentVariablesRepository implements Repository {
313313

314314
return {
315315
success: true as const,
316+
acceptedKeys: values.map((v) => v.key),
316317
};
317318
} catch (error) {
318319
if (error instanceof Prisma.PrismaClientKnownRequestError) {
@@ -975,9 +976,8 @@ export class EnvironmentVariablesRepository implements Repository {
975976
projectId: string,
976977
options: PruneBranchValuesShadowingParent
977978
): Promise<PruneBranchValuesResult> {
978-
const result: PruneBranchValuesResult = { prunedKeys: [], failures: [] };
979979
if (options.keys.length === 0) {
980-
return result;
980+
return { prunedKeys: [], failures: [] };
981981
}
982982

983983
const variables = await this.prismaClient.environmentVariable.findMany({
@@ -991,11 +991,17 @@ export class EnvironmentVariablesRepository implements Repository {
991991
key: true,
992992
values: {
993993
where: { environmentId: { in: [options.environmentId, options.parentEnvironmentId] } },
994-
select: { environmentId: true, lastUpdatedBy: true },
994+
select: {
995+
id: true,
996+
environmentId: true,
997+
lastUpdatedBy: true,
998+
valueReference: { select: { key: true } },
999+
},
9951000
},
9961001
},
9971002
});
9981003

1004+
const shadowing: { id: string; key: string; secretReferenceKey: string | undefined }[] = [];
9991005
for (const variable of variables) {
10001006
const branchValue = variable.values.find((v) => v.environmentId === options.environmentId);
10011007
const parentValue = variable.values.find(
@@ -1008,19 +1014,52 @@ export class EnvironmentVariablesRepository implements Repository {
10081014
) {
10091015
continue;
10101016
}
1011-
1012-
const deleted = await this.deleteValue(projectId, {
1013-
id: variable.id,
1014-
environmentId: options.environmentId,
1017+
shadowing.push({
1018+
id: branchValue.id,
1019+
key: variable.key,
1020+
secretReferenceKey: branchValue.valueReference?.key,
10151021
});
1016-
if (deleted.success) {
1017-
result.prunedKeys.push(variable.key);
1018-
} else {
1019-
result.failures.push({ key: variable.key, error: deleted.error });
1020-
}
10211022
}
10221023

1023-
return result;
1024+
if (shadowing.length === 0) {
1025+
return { prunedKeys: [], failures: [] };
1026+
}
1027+
1028+
const keys = shadowing.map((v) => v.key);
1029+
try {
1030+
await $transaction(this.prismaClient, "prune branch env var values", async (tx) => {
1031+
const secretStore = getSecretStore("DATABASE", {
1032+
prismaClient: tx,
1033+
});
1034+
1035+
for (const value of shadowing) {
1036+
await secretStore.deleteSecret(secretKey(projectId, options.environmentId, value.key));
1037+
1038+
if (value.secretReferenceKey) {
1039+
await tx.secretReference.delete({
1040+
where: {
1041+
key: value.secretReferenceKey,
1042+
},
1043+
});
1044+
}
1045+
1046+
await tx.environmentVariableValue.delete({
1047+
where: {
1048+
id: value.id,
1049+
},
1050+
});
1051+
}
1052+
});
1053+
1054+
return { prunedKeys: keys, failures: [] };
1055+
} catch (error) {
1056+
return {
1057+
prunedKeys: [],
1058+
failures: [
1059+
{ keys, error: error instanceof Error ? error.message : "Something went wrong" },
1060+
],
1061+
};
1062+
}
10241063
}
10251064
}
10261065

‎apps/webapp/app/v3/environmentVariables/parentKeysToPrune.test.ts‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import { describe, expect, test } from "vitest";
2+
import { parentKeysToPrune } from "./parentKeysToPrune";
3+
4+
describe("parentKeysToPrune", () => {
5+
test("returns accepted parent keys the request does not also set on the branch", () => {
6+
expect(
7+
parentKeysToPrune({
8+
variables: { BRANCH_ONLY: "b", BOTH: "b" },
9+
parentVariables: { SHARED: "p", BOTH: "p" },
10+
acceptedParentKeys: ["SHARED", "BOTH"],
11+
})
12+
).toEqual(["SHARED"]);
13+
});
14+
15+
test("excludes parent keys the write did not accept", () => {
16+
expect(
17+
parentKeysToPrune({
18+
variables: {},
19+
parentVariables: { KEPT: "p", DROPPED: "", TRIGGER_SECRET_KEY: "x" },
20+
acceptedParentKeys: ["KEPT"],
21+
})
22+
).toEqual(["KEPT"]);
23+
});
24+
25+
test("ignores accepted keys that were not part of parentVariables", () => {
26+
expect(
27+
parentKeysToPrune({
28+
variables: {},
29+
parentVariables: { A: "p" },
30+
acceptedParentKeys: ["A", "B"],
31+
})
32+
).toEqual(["A"]);
33+
});
34+
35+
test("handles empty inputs", () => {
36+
expect(
37+
parentKeysToPrune({ variables: {}, parentVariables: {}, acceptedParentKeys: [] })
38+
).toEqual([]);
39+
expect(
40+
parentKeysToPrune({ variables: {}, parentVariables: { A: "p" }, acceptedParentKeys: [] })
41+
).toEqual([]);
42+
expect(
43+
parentKeysToPrune({ variables: { A: "b" }, parentVariables: {}, acceptedParentKeys: ["A"] })
44+
).toEqual([]);
45+
});
46+
});

‎apps/webapp/app/v3/environmentVariables/parentKeysToPrune.ts‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
/**
2+
* Keys whose branch copy an import may retract: the parent-level keys the write actually accepted,
3+
* minus any key the same request also sets on the branch itself (an explicit branch override).
4+
*/
5+
export function parentKeysToPrune(options: {
6+
variables: Record<string, string>;
7+
parentVariables: Record<string, string>;
8+
acceptedParentKeys: string[];
9+
}): string[] {
10+
const branchKeys = new Set(Object.keys(options.variables));
11+
const accepted = new Set(options.acceptedParentKeys);
12+
return Object.keys(options.parentVariables).filter(
13+
(key) => accepted.has(key) && !branchKeys.has(key)
14+
);
15+
}

‎apps/webapp/app/v3/environmentVariables/repository.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ export type CreateEnvironmentVariables = z.infer<typeof CreateEnvironmentVariabl
3232
export type CreateResult =
3333
| {
3434
success: true;
35+
/** Keys that survived blacklist and empty-value filtering and were written or already held. */
36+
acceptedKeys?: string[];
3537
}
3638
| {
3739
success: false;
@@ -72,7 +74,7 @@ export type PruneBranchValuesShadowingParent = {
7274

7375
export type PruneBranchValuesResult = {
7476
prunedKeys: string[];
75-
failures: { key: string; error: string }[];
77+
failures: { keys: string[]; error: string }[];
7678
};
7779

7880
// Forms preserve explicit empty strings through their custom coercion.
@@ -156,7 +158,9 @@ export interface Repository {
156158
* A source that writes a key at the parent level retracts its own earlier copy of that key on
157159
* the branch. Deletes the branch's own value rows for `keys` that were last written by `source`
158160
* and that now shadow a value the parent holds. Rows written by anyone else, and rows whose key
159-
* the parent has no value for, are untouched.
161+
* the parent has no value for, are untouched. A caller that wants both a branch value and a
162+
* parent value for the same key must send both in the same import request; the same-request
163+
* exclusion is what keeps the branch value from being retracted.
160164
*/
161165
pruneBranchValuesShadowingParent(
162166
projectId: string,

‎apps/webapp/test/environmentVariablesRepository.test.ts‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ vi.mock("~/db.server", () => ({
2020
import { postgresTest } from "@internal/testcontainers";
2121
import { emptyEnvironmentVariableValuesEnabled } from "~/v3/environmentVariables/emptyValuesFlag.server";
2222
import { EnvironmentVariablesRepository } from "~/v3/environmentVariables/environmentVariablesRepository.server";
23+
import { parentKeysToPrune } from "~/v3/environmentVariables/parentKeysToPrune";
2324
import {
2425
createEnvironmentVariable,
2526
createRuntimeEnvironment,
@@ -835,6 +836,16 @@ describe("EnvironmentVariablesRepository.pruneBranchValuesShadowingParent", () =
835836
]);
836837
expect(secrets.has(`${branch.id}:SHARED`)).toBe(false);
837838
expect(secrets.get(`${parent.id}:SHARED`)).toBe("root");
839+
expect(
840+
await prisma.secretReference.findFirst({
841+
where: { key: `environmentvariable:${project.id}:${branch.id}:SHARED` },
842+
})
843+
).toBeNull();
844+
expect(
845+
await prisma.secretReference.findFirst({
846+
where: { key: `environmentvariable:${project.id}:${parent.id}:SHARED` },
847+
})
848+
).not.toBeNull();
838849
expect(
839850
Object.fromEntries(
840851
(await repository.getEnvironmentVariables(project.id, branch.id, parent.id)).map(
@@ -937,6 +948,52 @@ describe("EnvironmentVariablesRepository.pruneBranchValuesShadowingParent", () =
937948
}
938949
);
939950

951+
postgresTest(
952+
"keeps a same-source branch row when the parent write dropped the key",
953+
async ({ prisma }) => {
954+
const { project, parent, branch, repository, write, ownKeys } = await createBranchWithParent(
955+
prisma,
956+
{ allowEmptyValues: false }
957+
);
958+
await write(parent.id, { K: "old" }, vercel);
959+
await write(branch.id, { K: "branch-copy" }, vercel);
960+
961+
const parentResult = await repository.create(project.id, {
962+
override: true,
963+
environmentIds: [parent.id],
964+
variables: [{ key: "K", value: "" }],
965+
lastUpdatedBy: vercel,
966+
});
967+
expect(parentResult).toEqual({ success: true, acceptedKeys: [] });
968+
969+
const keys = parentKeysToPrune({
970+
variables: {},
971+
parentVariables: { K: "" },
972+
acceptedParentKeys: parentResult.success ? (parentResult.acceptedKeys ?? []) : [],
973+
});
974+
expect(keys).toEqual([]);
975+
const result = await repository.pruneBranchValuesShadowingParent(project.id, {
976+
environmentId: branch.id,
977+
parentEnvironmentId: parent.id,
978+
keys,
979+
source: vercel,
980+
});
981+
expect(result).toEqual({ prunedKeys: [], failures: [] });
982+
expect(await ownKeys(branch.id)).toEqual(["K"]);
983+
984+
const refreshed = await repository.create(project.id, {
985+
override: true,
986+
environmentIds: [parent.id],
987+
variables: [
988+
{ key: "K", value: "new" },
989+
{ key: "TRIGGER_SECRET_KEY", value: "not-allowed" },
990+
],
991+
lastUpdatedBy: vercel,
992+
});
993+
expect(refreshed).toEqual({ success: true, acceptedKeys: ["K"] });
994+
}
995+
);
996+
940997
postgresTest("does not reach into another project's variables", async ({ prisma }) => {
941998
const mine = await createBranchWithParent(prisma);
942999
const theirs = await createBranchWithParent(prisma);

0 commit comments

Comments
 (0)