Skip to content

fix: reference variable working test #562

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
May 10, 2025
Merged

fix: reference variable working test #562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b0c0a14
fix: reference variable test working
adityachoudhari26 May 10, 2025
f50ceca
more fix
adityachoudhari26 May 10, 2025
465f399
fix
adityachoudhari26 May 10, 2025
01b641f
rabbit comment
adityachoudhari26 May 10, 2025
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
32 changes: 21 additions & 11 deletions apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { db } from "@ctrlplane/db/client";
import * as schema from "@ctrlplane/db/schema";
import { Channel, getQueue } from "@ctrlplane/events";
import { logger } from "@ctrlplane/logger";
import { getReferenceVariableValue } from "@ctrlplane/rule-engine";
import { variablesAES256 } from "@ctrlplane/secrets";
import { Permission } from "@ctrlplane/validators/auth";

Expand Down Expand Up @@ -57,18 +58,27 @@ export const GET = request()
);

const { metadata, ...resource } = data;
const variablesPromises = data.variables.map(async (v) => {
if (v.valueType === "direct") {
const strval = String(v.value);
const value = v.sensitive
? variablesAES256().decrypt(strval)
: v.value;
return [v.key, value] as const;
}

if (v.valueType === "reference") {
const resolvedValue = await getReferenceVariableValue(
v as schema.ReferenceResourceVariable,
);
return [v.key, resolvedValue] as const;
}

return [v.key, v.defaultValue] as const;
});

const variables = Object.fromEntries(
data.variables.map((v) => {
if (v.valueType === "direct") {
const strval = String(v.value);
const value = v.sensitive
? variablesAES256().decrypt(strval)
: v.value;
return [v.key, value];
}

return [v.key, v.defaultValue];
}),
await Promise.all(variablesPromises),
);

return NextResponse.json({
Expand Down
50 changes: 21 additions & 29 deletions ...ervice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import { NextResponse } from "next/server";
import { get } from "lodash";

import { and, eq, isNull } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import { getResourceParents } from "@ctrlplane/db/queries";
import * as schema from "@ctrlplane/db/schema";
import { Channel, getQueue } from "@ctrlplane/events";
import { getReferenceVariableValue } from "@ctrlplane/rule-engine";
import { variablesAES256 } from "@ctrlplane/secrets";
import { Permission } from "@ctrlplane/validators/auth";

Expand Down Expand Up @@ -74,34 +74,26 @@ export const GET = request()
);
}

const { relationships, getTargetsWithMetadata } = await getResourceParents(
db,
resource.id,
);
const relatipnshipTargets = await getTargetsWithMetadata();

const variables = Object.fromEntries(
resource.variables.map((v) => {
if (v.valueType === "direct") {
const strval = String(v.value);
const value = v.sensitive
? variablesAES256().decrypt(strval)
: v.value;
return [v.key, value];
}

if (v.valueType === "reference") {
if (v.path == null) return [v.key, v.defaultValue];
if (v.reference == null) return [v.key, v.defaultValue];
const target = relationships[v.reference]?.target.id;
const targetResource = relatipnshipTargets[target ?? ""];
if (targetResource == null) return [v.key, v.defaultValue];
return [v.key, get(targetResource, v.path, v.defaultValue)];
}

throw new Error(`Unknown variable value type: ${v.valueType}`);
}),
);
const { relationships } = await getResourceParents(db, resource.id);

const resourceVariablesPromises = resource.variables.map(async (v) => {
if (v.valueType === "direct") {
const strval = String(v.value);
const value = v.sensitive ? variablesAES256().decrypt(strval) : v.value;
return [v.key, value] as const;
}

if (v.valueType === "reference") {
const resolvedValue = await getReferenceVariableValue(
v as schema.ReferenceResourceVariable,
);
return [v.key, resolvedValue] as const;
}

return [v.key, v.defaultValue] as const;
});
Comment on lines +79 to +94
Copy link
Contributor

@coderabbitai coderabbitai bot May 10, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add defensive handling when resolving reference variables

getReferenceVariableValue likely performs DB look-ups.
If it throws (e.g., broken relationship, missing target, DB timeout) the whole handler will bubble a 500 response and the caller receives no partial data, even though we have a sensible defaultValue on the variable object.

Consider wrapping the call in a try / catch and falling back to v.defaultValue while logging the incident for observability:

-      if (v.valueType === "reference") {
-        const resolvedValue = await getReferenceVariableValue(
-          v as schema.ReferenceResourceVariable,
-        );
-        return [v.key, resolvedValue] as const;
-      }
+      if (v.valueType === "reference") {
+        try {
+          const resolvedValue = await getReferenceVariableValue(
+            v as schema.ReferenceResourceVariable,
+          );
+          return [v.key, resolvedValue ?? v.defaultValue] as const;
+        } catch (err) {
+          // TODO: use your structured/logger instead of console
+          console.error(
+            `Failed to resolve reference variable '${v.key}' for resource ${resource.id}:`,
+            err,
+          );
+          return [v.key, v.defaultValue] as const;
+        }
+      }

This keeps the endpoint resilient and prevents a single bad reference from breaking the entire response.

const resourceVariables = await Promise.all(resourceVariablesPromises);
const variables = Object.fromEntries(resourceVariables);

const metadata = Object.fromEntries(
resource.metadata.map((t) => [t.key, t.value]),
Expand Down
15 changes: 8 additions & 7 deletions e2e/tests/api/resource-variables.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,9 @@ test.describe("Resource Variables API", () => {
api,
workspace,
}) => {
const systemPrefix = importedEntities.system.slug.split("-")[0]!;
const systemPrefix = importedEntities.system.slug
.split("-")[0]!
.toLowerCase();

// Create target resource
const targetResource = await api.POST("/v1/resources", {
Expand All @@ -185,13 +187,12 @@ test.describe("Resource Variables API", () => {
name: `${systemPrefix}-target`,
kind: "Target",
identifier: `${systemPrefix}-target`,
version: "test-version/v1",
version: `${systemPrefix}-version/v1`,
config: { "e2e-test": true } as any,
metadata: {
"e2e-test": "true",
[`${systemPrefix}`]: "true",
[systemPrefix]: "true",
},
variables: [{ key: "target-var", value: "target-value" }],
},
});
expect(targetResource.response.status).toBe(200);
Expand All @@ -204,7 +205,7 @@ test.describe("Resource Variables API", () => {
name: `${systemPrefix}-source`,
kind: "Source",
identifier: `${systemPrefix}-source`,
version: "test-version/v1",
version: `${systemPrefix}-version/v1`,
config: { "e2e-test": true } as any,
metadata: {
"e2e-test": "true",
Expand All @@ -230,9 +231,9 @@ test.describe("Resource Variables API", () => {
reference: systemPrefix,
dependencyType: "depends_on",
sourceKind: "Source",
sourceVersion: "test-version/v1",
sourceVersion: `${systemPrefix}-version/v1`,
targetKind: "Target",
targetVersion: "test-version/v1",
targetVersion: `${systemPrefix}-version/v1`,
metadataKeysMatch: ["e2e-test", systemPrefix],
},
});
Expand Down
124 changes: 45 additions & 79 deletions packages/api/src/router/resources.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { SQL, Tx } from "@ctrlplane/db";
import type { ResourceCondition } from "@ctrlplane/validators/resources";
import _, { get } from "lodash";
import _ from "lodash";
import { isPresent } from "ts-is-present";
import { z } from "zod";

Expand Down Expand Up @@ -32,6 +32,7 @@ import {
isPassingChannelSelectorPolicy,
isPassingNoPendingJobsPolicy,
} from "@ctrlplane/job-dispatch";
import { getReferenceVariableValue } from "@ctrlplane/rule-engine";
import { Permission } from "@ctrlplane/validators/auth";
import { resourceCondition } from "@ctrlplane/validators/resources";

Expand Down Expand Up @@ -383,87 +384,52 @@ export const resourceRouter = createTRPCRouter({
.on({ type: "resource", id: input }),
})
.input(z.string().uuid())
.query(({ ctx, input }) =>
ctx.db.query.resource
.findFirst({
where: and(eq(schema.resource.id, input), isNotDeleted),
with: { metadata: true, variables: true, provider: true },
.query(async ({ ctx, input }) => {
const resource = await ctx.db.query.resource.findFirst({
where: and(eq(schema.resource.id, input), isNotDeleted),
with: { metadata: true, variables: true, provider: true },
});
if (resource == null) return null;

const { relationships } = await getResourceParents(ctx.db, resource.id);

const parsedVariables = resource.variables
.map((v) => {
const parsed = schema.resourceVariableSchema.safeParse(v);
if (!parsed.success) return null;
return parsed.data;
})
.then(async (t) => {
if (t == null) return null;

const { relationships, getTargetsWithMetadata } =
await getResourceParents(ctx.db, t.id);
const relationshipTargets = await getTargetsWithMetadata();

const parsedVariables = t.variables
.map((v) => {
try {
return schema.resourceVariableSchema.parse(v);
} catch (error) {
console.error(
`Failed to parse variable ${v.key} for resource ${t.id}:`,
error,
);
return null;
}
})
.filter(isPresent);

const directVariables = parsedVariables.filter(
(v): v is schema.DirectResourceVariable => v.valueType === "direct",
);
.filter(isPresent);

const referenceVariables = parsedVariables
.filter(
(v): v is schema.ReferenceResourceVariable =>
v.valueType === "reference",
)
.map((v) => {
const relationshipInfo = relationships[v.reference];
if (!relationshipInfo)
return { ...v, resolvedValue: v.defaultValue };

const targetId = relationshipInfo.target.id;
const targetResource = relationshipTargets[targetId];

if (!targetResource)
return { ...v, resolvedValue: v.defaultValue };

if (v.path.length === 0)
return {
...v,
resolvedValue: get(targetResource, [], v.defaultValue),
};

const metadataKey = v.path.join("/");
const metadataValue =
metadataKey in targetResource.metadata
? targetResource.metadata[metadataKey]
: undefined;

if (metadataValue !== undefined)
return { ...v, resolvedValue: metadataValue };

return {
...v,
resolvedValue: get(targetResource, v.path, v.defaultValue),
};
});

const metadata = Object.fromEntries(
t.metadata.map((m) => [m.key, m.value]),
);
return {
...t,
relationships,
directVariables,
referenceVariables,
metadata,
rules: await getResourceRelationshipRules(ctx.db, t.id),
};
const directVariables = parsedVariables.filter(
(v): v is schema.DirectResourceVariable => v.valueType === "direct",
);

Comment on lines +404 to +407
Copy link
Contributor

@coderabbitai coderabbitai bot May 10, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Sensitive direct variables are returned encrypted

In the webservice route (apps/webservice/.../route.ts) direct variables are decrypted:

const value = v.sensitive ? variablesAES256().decrypt(strval) : v.value;

The TRPC byId query, however, forwards the cipher-text as-is.
Clients depending on byId will now receive different payloads than the REST handler, and, even worse, may inadvertently expose encrypted secrets in logs/UI.

Proposed fix:

+import { variablesAES256 } from "@ctrlplane/secrets";
...
-      const directVariables = parsedVariables.filter(
-        (v): v is schema.DirectResourceVariable => v.valueType === "direct",
-      );
+      const directVariables = parsedVariables
+        .filter(
+          (v): v is schema.DirectResourceVariable => v.valueType === "direct",
+        )
+        .map((v) => ({
+          ...v,
+          resolvedValue: v.sensitive
+            ? variablesAES256().decrypt(String(v.value))
+            : v.value,
+        }));

This aligns both endpoints, avoids leaking cipher-text, and gives callers a uniform resolvedValue shape similar to reference variables.

const referenceVariables = parsedVariables.filter(
(v): v is schema.ReferenceResourceVariable =>
v.valueType === "reference",
);

const resolvedReferenceVariables = await Promise.all(
referenceVariables.map(async (v) => {
const resolvedValue = await getReferenceVariableValue(v);
return { ...v, resolvedValue };
}),
),
);
Comment on lines +413 to +418
Copy link
Contributor

@coderabbitai coderabbitai bot May 10, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Gracefully degrade when a reference variable cannot be resolved

Same resilience comment as for the REST handler: a single failing reference resolution should not break the whole response.
Wrap getReferenceVariableValue in a try / catch, log the error, and fall back to v.defaultValue (or null) so the API remains predictable.


const metadata = Object.fromEntries(
resource.metadata.map((m) => [m.key, m.value]),
);

return {
...resource,
relationships,
directVariables,
referenceVariables: resolvedReferenceVariables,
metadata,
rules: await getResourceRelationshipRules(ctx.db, resource.id),
};
}),

latestDeployedVersions: createTRPCRouter({
byResourceAndEnvironmentId: protectedProcedure
Expand Down
1 change: 1 addition & 0 deletions packages/rule-engine/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ export * from "./utils/merge-policies.js";
export * from "./types.js";
export * from "./manager/version-manager.js";
export * from "./manager/variable-manager.js";
export * from "./manager/variables/resolve-reference-variable.js";
27 changes: 27 additions & 0 deletions packages/rule-engine/src/manager/variables/resolve-reference-variable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import type * as schema from "@ctrlplane/db/schema";
import _ from "lodash";

import { db } from "@ctrlplane/db/client";
import { getResourceParents } from "@ctrlplane/db/queries";
import { logger } from "@ctrlplane/logger";

export const getReferenceVariableValue = async (
variable: schema.ReferenceResourceVariable,
) => {
try {
const { relationships, getTargetsWithMetadata } = await getResourceParents(
db,
variable.resourceId,
);
const relationshipTargets = await getTargetsWithMetadata();

const targetId = relationships[variable.reference]?.target.id ?? "";
const targetResource = relationshipTargets[targetId];
if (targetResource == null) return variable.defaultValue;

return _.get(targetResource, variable.path, variable.defaultValue);
} catch (error) {
logger.error("Error resolving reference variable", { error, variable });
return variable.defaultValue;
}
};
Loading