fix: reference variable working test #562
Conversation
WalkthroughThe changes introduce asynchronous resolution of resource variables, especially for reference-type variables, across multiple API endpoints and internal handlers. A new utility function, Changes
Sequence Diagram(s)sequenceDiagram
participant API_Handler as API Handler (GET Resource)
participant DB as Database
participant RuleEngine as Rule Engine
participant ReferenceResolver as getReferenceVariableValue
API_Handler->>DB: Fetch resource by ID
API_Handler->>DB: Fetch resource relationships
loop For each variable
alt valueType == "direct"
API_Handler->>API_Handler: Decrypt or use raw value
else valueType == "reference"
API_Handler->>ReferenceResolver: getReferenceVariableValue(variable)
ReferenceResolver->>DB: Fetch parent relationships & metadata
ReferenceResolver-->>API_Handler: Resolved value or default
else
API_Handler->>API_Handler: Use default value
end
end
API_Handler->>API_Handler: Construct variables object
API_Handler-->>Client: Return resource with resolved variables
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
adityachoudhari26
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (1)
22-23: Review the use of_.getwith an empty path arrayUsing
_.getwith an empty path array ([]) is unusual and may not behave as expected. It's unclear what value this is intended to retrieve fromtargetResourcewhen the path is empty.Consider simplifying this to directly use the target resource or clarify the intent:
- const resolvedValue = _.get(targetResource, [], variable.defaultValue); + const resolvedValue = targetResource || variable.defaultValue;apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (1)
61-78: Consider adding error handling for async variable resolutionThe implementation correctly handles different variable types and asynchronously resolves reference variables. However, there's no specific error handling for the async resolution process.
Consider adding try/catch blocks to handle potential errors in reference variable resolution:
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") { + try { const resolvedValue = await getReferenceVariableValue( v as schema.ReferenceResourceVariable, ); return [v.key, resolvedValue] as const; + } catch (error) { + log.error(`Error resolving reference variable ${v.key}: ${error}`); + return [v.key, v.defaultValue] as const; + } } return [v.key, v.defaultValue] as const; });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(2 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts(2 hunks)e2e/tests/api/resource-variables.spec.ts(5 hunks)packages/api/src/router/resources.ts(3 hunks)packages/rule-engine/src/index.ts(1 hunks)packages/rule-engine/src/manager/variables/resolve-reference-variable.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/rule-engine/src/index.tse2e/tests/api/resource-variables.spec.tsapps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.tspackages/api/src/router/resources.tsapps/webservice/src/app/api/v1/resources/[resourceId]/route.tspackages/rule-engine/src/manager/variables/resolve-reference-variable.ts
🧬 Code Graph Analysis (1)
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (2)
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (1)
getReferenceVariableValue(7-29)packages/db/src/schema/resource.ts (1)
ReferenceResourceVariable(514-514)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (7)
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (1)
7-29: Well-structured implementation of reference variable resolutionThe function correctly handles the resolution of reference variables by:
- Retrieving the resource relationships and metadata
- Finding the target resource based on the reference
- Gracefully handling missing resources or values with defaults
- Supporting both direct resource lookup and metadata path lookup
packages/rule-engine/src/index.ts (1)
8-8: Proper export of the new utility functionThe new export correctly makes the
getReferenceVariableValuefunction available to other modules, maintaining consistency with existing export patterns.e2e/tests/api/resource-variables.spec.ts (3)
179-181: Case normalization improves consistencyConverting
systemPrefixto lowercase ensures consistent matching of keys and references throughout the test, which helps prevent case-sensitive comparison issues.
190-190: Dynamic versioning improves test robustnessReplacing hardcoded version strings with dynamic versions based on the system prefix makes the tests more maintainable and adaptable to different test environments.
Also applies to: 208-208, 234-234, 236-236
218-218: Path simplification aligns with new implementationChanging the path from
["metadata", "e2e-test"]to["e2e-test"]correctly aligns with how the newgetReferenceVariableValuefunction processes paths for metadata lookup.apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (2)
70-75: Proper integration of async reference variable resolutionThe integration of the
getReferenceVariableValuefunction correctly handles reference-type variables, properly casting the variable to the expected type and resolving it asynchronously.
80-82: Efficient parallel resolution with Promise.allUsing
Promise.allto resolve all variable promises in parallel is an efficient approach that maintains good performance while handling async operations.
| 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; | ||
| }); |
There was a problem hiding this comment.
🛠️ 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 resolvedReferenceVariables = await Promise.all( | ||
| referenceVariables.map(async (v) => { | ||
| const resolvedValue = await getReferenceVariableValue(v); | ||
| return { ...v, resolvedValue }; | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
🛠️ 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 directVariables = parsedVariables.filter( | ||
| (v): v is schema.DirectResourceVariable => v.valueType === "direct", | ||
| ); | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (2)
1-6: Consider adding function documentationAdding JSDoc comments to describe the function's purpose, parameters, and return value would improve maintainability and make the API clearer to other developers.
import { getResourceParents } from "@ctrlplane/db/queries"; +/** + * Resolves the value of a reference resource variable by looking up the relationship target + * and retrieving the value at the specified path. + * + * @param variable - The reference variable containing resourceId, reference, path, and defaultValue + * @returns The resolved value from the target resource or the default value if not found + */ export const getReferenceVariableValue = async (
4-5: Consider dependency injection for better testabilityThe function currently depends on the global
dbclient, which makes unit testing more difficult. Consider accepting the database client as a parameter for better testability and modularity.import { db } from "@ctrlplane/db/client"; import { getResourceParents } from "@ctrlplane/db/queries"; -export const getReferenceVariableValue = async ( +export const getReferenceVariableValue = async ( + dbClient = db, variable: schema.ReferenceResourceVariable, ) => { const { relationships, getTargetsWithMetadata } = await getResourceParents( - db, + dbClient, variable.resourceId, );
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
e2e/tests/api/resource-variables.spec.ts(4 hunks)packages/rule-engine/src/manager/variables/resolve-reference-variable.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/tests/api/resource-variables.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts
🧬 Code Graph Analysis (1)
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (3)
packages/db/src/schema/resource.ts (1)
ReferenceResourceVariable(514-514)packages/db/src/queries/get-resource-parents.ts (1)
getResourceParents(13-153)packages/db/src/client.ts (1)
db(15-15)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (2)
7-21: Well-structured implementation of reference variable resolutionThe function correctly implements asynchronous resolution of reference variables by:
- Querying resource parents and their metadata
- Identifying the target resource based on the reference relationship
- Retrieving the value at the specified path, with appropriate fallback to default value
16-18: Good use of nullish coalescing and early return patternThe code correctly handles potential null/undefined values with the nullish coalescing operator and early return, making the logic clear and defensive.
Summary by CodeRabbit