add reference variable value type#561
Conversation
|
""" WalkthroughThe changes update the schema for deployment variable values by introducing new columns ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Router
participant RuleEngine
participant DB
Client->>API_Router: Create or update deployment variable value (with valueType, reference, path, defaultValue)
API_Router->>DB: Insert/update deploymentVariableValue with new schema fields
DB-->>API_Router: Confirmation with SQL constraints enforced
API_Router-->>Client: Response
Client->>API_Router: Query resource or deployment variables
API_Router->>RuleEngine: getReferenceVariableValue(resourceId, variable)
RuleEngine->>DB: Fetch resource parents and resolve reference variable value
DB-->>RuleEngine: Resolved value
RuleEngine-->>API_Router: Variable value
API_Router-->>Client: Variable data with resolved values
Possibly related PRs
Poem
✨ 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
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/api/src/router/deployment-variable.ts (2)
321-335: 🛠️ Refactor suggestionHandle
valueTypein the default value creationWhen creating a default value as part of the variable creation, the code should now set the
valueTypefield to align with the SQL constraint.Update the default value creation code to include the
valueType:.insert(deploymentVariableValue) .values({ variableId: variable.id, value: input.data.config.default, + valueType: 'direct', })
59-81: 💡 Verification agent🧩 Analysis chain
Verify compatibility with new schema constraints
Since the schema now enforces specific combinations of fields based on
valueType, ensure that the API properly sets thevalueTypeand appropriate fields when creating new values.This code doesn't explicitly set
valueType, which could lead to inconsistencies with the new schema constraints. The SQL constraint now requires that whenvalueTypeis 'direct', onlyvalueshould be populated, and when it's 'reference', onlyreferenceandpathshould be populated.
🏁 Script executed:
#!/bin/bash # Check for places in the codebase where new deploymentVariableValue records are created rg -A 5 "insert.*deploymentVariableValue|valueType.*direct|valueType.*reference" --type tsLength of output: 16957
🏁 Script executed:
#!/bin/bash # Display the relevant parts of the deployment-variable router for context sed -n '1,200p' packages/api/src/router/deployment-variable.tsLength of output: 6099
🏁 Script executed:
#!/bin/bash # Locate the definition of createDeploymentVariableValue in the schema rg -n "export const createDeploymentVariableValue" -A 10 packages/db/src/schema/deployment-variables.tsLength of output: 497
🏁 Script executed:
#!/bin/bash # Display the first 200 lines of deployment-variables.ts for column definitions sed -n '1,200p' packages/db/src/schema/deployment-variables.tsLength of output: 6034
🏁 Script executed:
#!/bin/bash # Display createInsertSchema usage in resource schema rg -n "createInsertSchema" -A 10 packages/db/src/schema/resource.tsLength of output: 2430
Fix SQL constraint compatibility for deployment variable values
The
createDeploymentVariableValueschema and insert call don’t guarantee the requiredvalueType/field combinations, so inserting a reference‐type value withoutvalueType: "reference"(or with mismatched fields) will violate the DB check constraint.• In
packages/db/src/schema/deployment-variables.ts, override and enforcevalueTypeincreateDeploymentVariableValue:export const createDeploymentVariableValue = createInsertSchema( deploymentVariableValue, { resourceSelector: resourceCondition.refine(isValidResourceCondition), path: z.array(z.string()).optional(), valueType: z.enum(["direct", "reference"]), }, ) .omit({ id: true, variableId: true }) .extend({ default: z.boolean().optional() }) .refine( (data) => (data.valueType === "direct" && data.value != null && data.reference == null && data.path == null) || (data.valueType === "reference" && data.value == null && data.reference != null && data.path != null), { message: "For 'direct', only `value`; for 'reference', only `reference`+`path`" }, );• In the router (
packages/api/src/router/deployment-variable.ts), includevalueTypein the insert:tx .insert(deploymentVariableValue) .values({ ...input.data, valueType: input.data.valueType, // now required variableId: input.variableId, }) // …These changes align the TRPC input and SQL insert with the DB’s
valid_value_typeconstraint.
🧹 Nitpick comments (1)
packages/api/src/router/deployment-variable.ts (1)
262-270: Consider updating the SQL projection to include new fieldsThe SQL projection in the
byDeploymentIdquery doesn't include the new fields (valueType,reference,path,defaultValue). If these fields are needed by consumers of this API, they should be added to the projection.Consider updating the SQL projection to include the new fields:
.select({ id: deploymentVariableValue.id, value: deploymentVariableValue.value, variableId: deploymentVariableValue.variableId, resourceSelector: deploymentVariableValue.resourceSelector, + valueType: deploymentVariableValue.valueType, + reference: deploymentVariableValue.reference, + path: deploymentVariableValue.path, + defaultValue: deploymentVariableValue.defaultValue, })Similar updates would be needed in the JSON construction at lines 279-284.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/api/src/router/deployment-variable.ts(1 hunks)packages/db/src/schema/deployment-variables.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/api/src/router/deployment-variable.tspackages/db/src/schema/deployment-variables.ts
🧬 Code Graph Analysis (1)
packages/db/src/schema/deployment-variables.ts (1)
packages/validators/src/resources/conditions/resource-condition.ts (3)
ResourceCondition(29-39)resourceCondition(41-52)isValidResourceCondition(145-164)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
packages/db/src/schema/deployment-variables.ts (4)
62-72: Well implemented schema expansion for value referencesThe addition of
valueType,reference,path, anddefaultValuefields creates a flexible system for handling both direct values and references to external values. This pattern allows for dynamic value resolution that can adapt to different data sources.
86-89: Good use of SQL constraint for data integrityThe SQL constraint effectively enforces data integrity by ensuring that the appropriate fields are populated based on the
valueType:
- For 'direct' type: only
valueshould be populated- For 'reference' type: only
referenceandpathshould be populatedThis prevents inconsistent data states and makes the validation rules explicit at the database level.
97-100: Schema validation properly updatedThe schema validation for
createDeploymentVariableValuehas been appropriately updated to include the newpathfield and maintain the refinement forresourceSelector.
78-90:Details
❓ Verification inconclusive
Note the removal of the unique index on (variableId, value)
I notice that the previous unique index on
(variableId, value)was removed in this change. This seems appropriate given the new schema design, but ensure this doesn't impact any assumptions in the application logic about uniqueness of variable values.
🏁 Script executed:
#!/bin/bash # Check if there are any references to the unique constraint in the codebase rg -i "unique.*variableId.*value|value.*variableId.*unique" --type tsLength of output: 71
Removed unique index on
(variableId, value)— please verify no assumptions rely on itI see the unique constraint on
(variableId, value)was dropped in this schema change. While this matches the new design, please confirm that nothing in the codebase still relies on that uniqueness, for example:
- Any business logic or validation that expects duplicates to be prevented
- Tests or migrations that assert errors on inserting duplicate
(variableId, value)pairs- Seed scripts or data loaders assuming a one-to-one relationship
Running
rg -i "unique.*variableId.*value|value.*variableId.*unique" --type tsreturned no hits in
.tsfiles, but absence of evidence there doesn’t guarantee there are none in SQL files, tests, or other parts of the repo.Please manually audit these areas and update any affected code or documentation.
packages/api/src/router/deployment-variable.ts (1)
60-63: Formatting improvement enhances readabilityThe change to a multi-line format for the
.values()method call improves code readability without changing functionality. This is a good practice for object literals with multiple properties.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
e2e/tests/api/resource-variables.spec.ts (2)
237-238: Literal"metadataKeysMatches"repeated in two places – extract to a helper/constThe same hard-coded array
["e2e-test", systemPrefix]appears twice. A typo here would break both tests, and any schema rename would require multiple edits.Consider:
const METADATA_KEYS_MATCHES = ["e2e-test", systemPrefix]; ... metadataKeysMatches: METADATA_KEYS_MATCHES,Also applies to: 327-328
370-370:waitForTimeout(5 s)is brittle – poll the API insteadA fixed sleep can make the test flaky (too short on busy CI, unnecessarily long locally).
Prefer polling until the release appears, e.g.:await expect .poll(async () => { const resp = await api.GET( "/v1/resources/{resourceId}/release-targets", { params: { path: { resourceId: sourceResourceId ?? "" } } }, ); return resp.data?.length ?? 0; }) .toBeGreaterThan(0, { timeout: 15_000 });packages/rule-engine/src/manager/variables/db-variable-providers.ts (2)
98-110: Unhandled/unknownvalueTypewon’t be loggedIf a future enum value is added,
resolveVariableValuewill silently return
{resolved:false}. Logging aids debugging:return { resolved: false, value: null }; + // logger.warn(`Unknown valueType "${value.valueType}"`, { value });
117-124: Serialawaitinside loop → linear latency
await this.isSelectingResource(value)and thenawait this.resolveVariableValue(value)are executed one-by-one.
For many values you can parallelise withfor … of+Promise.allor an early-exit race.Not critical now, but worth revisiting if you notice slow resolution times.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/EditVariableDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableValueDropdown.tsx(3 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/DeploymentContent.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsx(1 hunks)apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts(2 hunks)apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(1 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts(1 hunks)e2e/api/schema.ts(2 hunks)e2e/tests/api/resource-variables.spec.ts(2 hunks)openapi.v1.json(3 hunks)packages/api/src/router/resources.ts(1 hunks)packages/db/drizzle/0100_robust_eternals.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/deployment-variables.ts(2 hunks)packages/job-dispatch/src/__test__/job-variables-deployment.test.ts(0 hunks)packages/job-dispatch/src/job-dispatch.ts(2 hunks)packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts(0 hunks)packages/job-dispatch/src/job-variables-deployment/utils.ts(0 hunks)packages/rule-engine/src/manager/variables/db-variable-providers.ts(6 hunks)packages/rule-engine/src/manager/variables/resolve-reference-variable.ts(1 hunks)
💤 Files with no reviewable changes (3)
- packages/job-dispatch/src/test/job-variables-deployment.test.ts
- packages/job-dispatch/src/job-variables-deployment/utils.ts
- packages/job-dispatch/src/job-variables-deployment/job-variables-deployment.ts
✅ Files skipped from review due to trivial changes (2)
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/EditVariableDialog.tsx
- packages/db/drizzle/meta/_journal.json
🧰 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.
apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsxapps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.tsapps/webservice/src/app/api/v1/resources/[resourceId]/route.tsapps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/DeploymentContent.tsxpackages/api/src/router/resources.tspackages/job-dispatch/src/job-dispatch.tspackages/rule-engine/src/manager/variables/resolve-reference-variable.tsapps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableValueDropdown.tsxe2e/tests/api/resource-variables.spec.tspackages/rule-engine/src/manager/variables/db-variable-providers.tsapps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.tse2e/api/schema.tspackages/db/src/schema/deployment-variables.ts
🧬 Code Graph Analysis (6)
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts (1)
packages/db/src/schema/resource.ts (1)
resource(59-87)
packages/api/src/router/resources.ts (2)
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (1)
getReferenceVariableValue(8-30)packages/db/src/schema/resource.ts (1)
resource(59-87)
packages/job-dispatch/src/job-dispatch.ts (1)
packages/events/src/index.ts (1)
getQueue(28-34)
packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (4)
packages/db/src/schema/resource.ts (1)
ReferenceResourceVariable(514-514)packages/db/src/schema/deployment-variables.ts (1)
DeploymentVariableValueReference(123-127)packages/db/src/queries/get-resource-parents.ts (1)
getResourceParents(13-153)packages/db/src/client.ts (1)
db(15-15)
packages/rule-engine/src/manager/variables/db-variable-providers.ts (4)
packages/db/src/schema/deployment-variables.ts (1)
DeploymentVariableValue(95-97)packages/db/src/selectors/index.ts (1)
selector(13-13)packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (1)
getReferenceVariableValue(8-30)packages/rule-engine/src/manager/variables/types.ts (1)
MaybeVariable(9-9)
packages/db/src/schema/deployment-variables.ts (1)
packages/validators/src/resources/conditions/resource-condition.ts (3)
ResourceCondition(29-39)resourceCondition(41-52)isValidResourceCondition(145-164)
⏰ 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 (30)
packages/job-dispatch/src/job-dispatch.ts (1)
74-92: Simplified job dispatch process by removing variable resolutionThe code now directly dispatches all jobs with job agents without first resolving release variables. This is a significant improvement that aligns with the architectural change to shift variable resolution responsibilities away from the job dispatch layer.
The return value has been updated to return all jobs with agents rather than only those that had successfully resolved variables, which makes the behavior more consistent and predictable.
apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsx (1)
251-251: Ensuring consistent string rendering of variable valuesWrapping the variable value with
String()ensures consistent string rendering in the UI, regardless of the underlying type of the value.apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts (1)
87-90: Updated function call to include resource IDThe function call to
getReferenceVariableValuenow correctly includes the resource ID as the first parameter, aligning with its updated signature. This change supports the new discriminated union types for variable values.apps/webservice/src/app/[workspaceSlug]/(app)/_components/resources/drawer/DeploymentContent.tsx (1)
67-71: Improved rendering of object-type variable valuesThe code now properly handles object-type values by converting them to JSON strings before display. This ensures that complex object values are rendered correctly in the UI rather than showing "[object Object]" or causing rendering issues.
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (1)
71-73: Correctly added resourceId parameter to getReferenceVariableValue callThe update properly adds the
resourceIdparameter to the function call, aligning with the updated function signature inpackages/rule-engine/src/manager/variables/resolve-reference-variable.ts. This change ensures correct resolution of reference variables.packages/api/src/router/resources.ts (1)
414-416: Added resource.id parameter to getReferenceVariableValue callThe update correctly adds the resource ID as the first parameter to the
getReferenceVariableValuefunction call, aligning with the updated function signature. This ensures proper resolution of reference variables in the resource router.apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/VariableValueDropdown.tsx (3)
5-5: Updated schema import from type-only to full importChanged from type-only import to full import to use schema validation functions directly.
43-43: Removed unused importsSimplified imports to only include
defaultConditionand removed unusedisValidResourceConditionandresourceCondition.
64-70: Updated form schema and defaultValues to support valueTypeThe form now uses the schema definition directly from the DB schema module and correctly initializes the
valueTypefield as either "direct" or "reference". This change aligns with the new discriminated union types for deployment variable values.packages/rule-engine/src/manager/variables/resolve-reference-variable.ts (2)
8-12: Enhanced function signature to support both variable reference typesThe function now accepts a separate
resourceIdparameter and expands thevariableparameter type to handle bothschema.ReferenceResourceVariableandschema.DeploymentVariableValueReference. This change supports the new deployment variable value types while maintaining backward compatibility.
15-18: Updated getResourceParents call to use explicit resourceId parameterThe function now passes the explicit
resourceIdparameter togetResourceParentsinstead of extracting it from the variable. This change aligns with the updated function signature and ensures correct behavior when resolving reference variables.e2e/tests/api/resource-variables.spec.ts (1)
334-335: Potential crash when no deployments are imported
const deployment = importedEntities.deployments[0]!;The non-null assertion will throw if
deploymentsis empty. Either assert length in abeforeAll, or fail the test early with a clear message:-const deployment = importedEntities.deployments[0]!; +expect(importedEntities.deployments.length).toBeGreaterThan(0); +const deployment = importedEntities.deployments[0];openapi.v1.json (5)
990-1006: Restrictvalueto explicit primitive and object types
Good addition—changing the previously free-formvaluefield to aoneOfunion tightens the request schema to only allow string, number, boolean, or object for variable values.
1056-1057: EnforcevalueTypeas required
RequiringvalueTypeensures every entry invaluesexplicitly declares its kind—this aligns with the new discriminated union approach.
4872-4888: Restrict componentvalueto explicit types
Mirroring the request change, updating theDeploymentVariableValue.valueproperty tooneOfunion primitives and objects tightens the component schema and keeps it consistent.
4896-4932: Introduce reference metadata in component schema
The newresourceSelector,valueType,reference,path, anddefaultValuefields inDeploymentVariableValueaccurately reflect the discriminated union in the database and API layers.
4935-4937: RequireresourceSelectorandvalueTypein component schema
IncludingresourceSelectorandvalueTypein the required list guarantees that every stored variable value has its source selector and type explicitly set.e2e/api/schema.ts (3)
916-923: Good refinement of thevaluetype definition.The type definition has been properly updated to be more specific (string, number, boolean, or object) instead of using the more generic
unknowntype. This improves type safety and provides better documentation of expected values.
927-940: Well-structured discriminated union type for variable values.The addition of
valueType,reference,path, anddefaultValueproperties creates a clean discriminated union pattern that distinguishes between direct values and references to other variables. This is a good design pattern that will make the code more maintainable and type-safe.
1982-2008: Correctly updated the request body schema to match the new type structure.The request body schema for creating deployment variables has been updated to match the changes in the
DeploymentVariableValuetype. This ensures consistency between the API schema and the internal data model.apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts (5)
15-22: Good refinement of value types in the OpenAPI schema.The
valueproperty has been properly defined as a union of specific types (string, number, boolean, object) rather than an unconstrained schema, which improves API documentation and validation.
29-41: Well-defined new properties for reference variable support.The addition of
valueType,reference,path, anddefaultValueproperties with appropriate types and nullability creates a clear schema for the discriminated union pattern. The schema properly represents both direct values and references to other variables.
42-42: Correctly updated required fields.The required fields have been updated to match the new structure, requiring
valueTypeinstead ofvalue, which aligns with the discriminated union pattern where value is optional depending on the valueType.
145-179: Consistent schema updates in the request body.The schema for the request body is updated to match the schema definition, ensuring consistent API documentation and validation across the codebase.
180-181: Correct required field update for request body items.Changing the required field from
["value"]to["valueType"]in the request body items schema aligns with the discriminated union pattern and ensures proper validation of API requests.packages/db/src/schema/deployment-variables.ts (5)
57-57: Good use of PostgreSQL enum for valueType.Using a PostgreSQL enum for
valueTypeensures type safety at the database level and prevents invalid values from being stored.
65-75: Well-structured database schema for discriminated union.The schema now properly supports both direct and reference variable values with appropriate columns and types. The nullable
valuefield with a specific type signature improves type safety and data integrity.
89-92: Excellent use of SQL constraint to enforce data integrity.The SQL constraint ensures that the data maintains consistency according to the valueType:
- For
directtype: value is required, while reference and path must be null- For
referencetype: value must be null, while reference and path are requiredThis prevents invalid combinations and enforces the discriminated union pattern at the database level.
100-109: Good schema validation improvements.The schema validation has been updated to:
- Validate resource selector using the
isValidResourceConditionfunction- Define proper types for optional fields (
value,path,defaultValue)- Use the appropriate union types for fields that can accept multiple types
This ensures data validation at the application level before it reaches the database.
117-139: Well-implemented TypeScript discriminated union types and type guards.The addition of
DeploymentVariableValueDirectandDeploymentVariableValueReferencetypes along with their respective type guards (isDeploymentVariableValueDirectandisDeploymentVariableValueReference) provides strong type safety for consumers of this data. This is a best practice for discriminated unions in TypeScript and will make the code more maintainable and less error-prone.
| @@ -0,0 +1,7 @@ | |||
| CREATE TYPE "public"."value_type" AS ENUM('direct', 'reference');--> statement-breakpoint | |||
| DROP INDEX "deployment_variable_value_variable_id_value_index";--> statement-breakpoint | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unique-index removed without a replacement → duplicates & slow queries possible
The original deployment_variable_value_variable_id_value_index ensured that each (variable_id,value) pair was unique and was used by lookup queries.
After dropping it, neither a new unique constraint nor a supporting index is added, so:
- Duplicate rows can now be inserted accidentally.
- Queries that used this index will fall back to seq-scans.
Consider restoring data-integrity and performance with a composite index that reflects the new schema:
-- Prevent duplicates and speed up look-ups
+CREATE UNIQUE INDEX CONCURRENTLY deployment_variable_value_unique
+ ON deployment_variable_value (
+ variable_id,
+ value_type,
+ COALESCE(value::text, reference), -- only one of the two is populated
+ path
+);There was a problem hiding this comment.
we cannot have this because now we can have variable values with null, so this would cause us to only have one null which is a problem
There was a problem hiding this comment.
@adityachoudhari26 You're absolutely right. Using COALESCE in a unique index would treat all nulls as equal, limiting you to just one record with null values for a given variable_id. This would be problematic since your schema redesign specifically needs to allow multiple records with different patterns of nulls (direct vs reference type values).
If you're still concerned about query performance without the original index, you could consider a non-unique index instead:
CREATE INDEX deployment_variable_value_lookup
ON deployment_variable_value (variable_id, value_type);This would help with performance for lookups without imposing the uniqueness constraint that would interfere with your null value requirements.
✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#561
File: packages/db/drizzle/0100_robust_eternals.sql:2-2
Timestamp: 2025-05-11T17:51:40.351Z
Learning: The deployment_variable_value table needs to support multiple records with null values for the same variable_id, which is why a unique index with COALESCE would be problematic as it would treat all nulls as equal.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| const resourceMatch = await this.db.query.resource.findFirst({ | ||
| where: and( | ||
| eq(schema.resource.id, this.options.resourceId), | ||
| selector().query().resources().where(value.resourceSelector).sql(), | ||
| ), | ||
| }); | ||
|
|
||
| return resourceMatch != null; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
N+1 query pattern – cache the current resource once
isSelectingResource issues a DB query for every candidate value. For a deployment
with many variable values this can explode to dozens of round-trips.
Consider loading the resource row once in the provider constructor and evaluating
selectors in memory or passing it into isSelectingResource.
| async isSelectingResource(value: schema.DeploymentVariableValue) { | ||
| if (value.resourceSelector == null) return false; | ||
|
|
There was a problem hiding this comment.
isSelectingResource incorrectly rejects values with resourceSelector === null
A null selector is supposed to match all resources. Returning false means such
values are silently skipped, so variables may never resolve unless a default value exists.
- if (value.resourceSelector == null) return false;
+ // Null selector ➔ applies to every resource
+ if (value.resourceSelector == null) return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async isSelectingResource(value: schema.DeploymentVariableValue) { | |
| if (value.resourceSelector == null) return false; | |
| async isSelectingResource(value: schema.DeploymentVariableValue) { | |
| // Null selector ➔ applies to every resource | |
| if (value.resourceSelector == null) return true; |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
openapi.v1.json (1)
4918-4927: Include an OpenAPI discriminator forVariableValue
To improve client generation and ensure clear schema resolution, add a discriminator onvalueTypein theVariableValueunion:VariableValue: oneOf: - $ref: '#/components/schemas/DeploymentVariableDirectValue' - $ref: '#/components/schemas/DeploymentVariableReferenceValue' discriminator: propertyName: valueType mapping: direct: '#/components/schemas/DeploymentVariableDirectValue' reference: '#/components/schemas/DeploymentVariableReferenceValue'This enforces the valid combinations of fields based on
valueType, as previously recommended.
🧹 Nitpick comments (2)
openapi.v1.json (2)
988-1003: DeduplicateresourceSelectorextension and includedefaultflag
The inlineallOfblock under thevaluesarray for thecreateDeploymentVariableendpoint reintroducesresourceSelector, which is already defined inBaseVariableValue. Additionally, thedefaultboolean fromBaseVariableValueisn’t exposed here. Refactor to only referenceVariableValueand, if needed, explicitly includedefaultat this level:"values": { "type": "array", "items": { - "allOf": [ - { "$ref": "#/components/schemas/VariableValue" }, - { - "type": "object", - "properties": { - "resourceSelector": { - "type": "object", - "additionalProperties": true, - "nullable": true - } - } - } - ] + "allOf": [ + { "$ref": "#/components/schemas/VariableValue" } + ] } }If instance-level overrides are required, consider adding back only the
defaultboolean rather than duplicatingresourceSelector.
4810-4822: Avoid naming conflicts with JSON Schema keywords
Defining a"default"property inBaseVariableValuecollides with the JSON Schemadefaultkeyword. To prevent ambiguities in codegen and validation, rename it to something more explicit, e.g.,isDefault:"BaseVariableValue": { "type": "object", - "properties": { - "resourceSelector": { /* ... */ }, - "default": { - "type": "boolean" - } - } + "properties": { + "resourceSelector": { /* ... */ }, + "isDefault": { + "type": "boolean" + } + } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts(2 hunks)apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts(3 hunks)e2e/api/schema.ts(2 hunks)e2e/tests/api/deployment-variable.spec.ts(5 hunks)e2e/tests/api/resource-variables.spec.ts(2 hunks)openapi.v1.json(2 hunks)packages/db/src/schema/deployment-variables.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/tests/api/resource-variables.spec.ts
- packages/db/src/schema/deployment-variables.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.
e2e/tests/api/deployment-variable.spec.tsapps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.tse2e/api/schema.tsapps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts
🧬 Code Graph Analysis (1)
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts (1)
packages/db/src/schema/deployment-variables.ts (1)
DeploymentVariableValueDirect(124-131)
🔇 Additional comments (10)
e2e/tests/api/deployment-variable.spec.ts (4)
98-101: Good implementation of valueType fieldThe test data has been properly updated to include the new
valueType: "direct"property for each variable value, which aligns with the schema changes for deployment variables.
132-139: Well-adapted assertion logic for handling different value typesThe conditional extraction logic correctly handles both direct and reference variable value types by checking
valueType === "direct"before deciding whether to usevalueordefaultValue. This properly tests the discriminated union type pattern introduced in the schema.
201-217: Good implementation of default value handling with new value typesThe test now appropriately handles the default value property while working with the new value type discriminator. The conditional logic for accessing values is consistent with the schema changes and properly tests both the value resolution and default value functionality.
244-245: Correctly updated validation test caseThe test for validating the uniqueness constraint on default values has been properly updated to include the new
valueType: "direct"property while maintaining the original test logic.apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts (2)
22-28: Good extraction of direct value resolution logicThe new
resolveDirectValuehelper function cleanly encapsulates the logic for processing direct variable values, including proper type handling, string conversion for object values, and conditional decryption for sensitive values.
58-71: Well-implemented discriminated union handlingThe updated variable value resolution logic effectively uses type guards to discriminate between different variable value types. The code:
- Properly checks for direct values using
isDeploymentVariableValueDirect- Applies appropriate processing based on the value type
- Handles reference values separately
- Logs errors for invalid types
- Filters out null values with
isPresentThis is a good implementation of the discriminated union pattern.
e2e/api/schema.ts (2)
913-943: Well-structured schema implementation for variable value typesThe schema has been effectively refactored to implement a discriminated union pattern for variable values:
BaseVariableValueprovides common propertiesDeploymentVariableDirectValueandDeploymentVariableReferenceValueextend the base and include type-specific propertiesVariableValuecreates a union of these typesDeploymentVariableValueextends the union with required ID fieldThis structure enables proper type discrimination while maintaining schema consistency.
1983-1987: Updated request body schema to use new value typesThe
createDeploymentVariableoperation's request body schema has been updated to use the newVariableValueunion type with optionalresourceSelector. This change aligns with the database schema and API handler changes.apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts (2)
11-84: Well-designed OpenAPI schema for variable value typesThe OpenAPI schema has been comprehensively refactored to support the new variable value types:
BaseVariableValuedefines common propertiesDeploymentVariableDirectValueextends the base for direct values with appropriate propertiesDeploymentVariableReferenceValueextends the base for reference values with appropriate propertiesVariableValuecombines these as a oneOf unionDeploymentVariableValueextends the union with required IDThe schema correctly uses OpenAPI composition patterns (allOf, oneOf) to create a properly typed discriminated union. Required fields are also correctly specified for each type.
184-197: Updated request body schema to use the new value type structureThe request body schema for the POST endpoint correctly uses the new
VariableValueschema with an additional optionalresourceSelectorproperty. The composition withallOfmaintains the base structure while adding endpoint-specific properties.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/db/drizzle/0100_robust_eternals.sql (2)
1-1: Use idempotent type creation
To avoid failures when re-running migrations, consider:CREATE TYPE IF NOT EXISTS "public"."value_type" AS ENUM('direct', 'reference');This prevents errors if the enum already exists.
8-11: Verify default_value enforcement and constraint naming
TheCHECKconstraint correctly enforces that fordirecttypes,default_valueisNULL. However, it does not impose any rule ondefault_valueforreferencetypes—confirm ifdefault_valueshould be mandatory (or always non-null) whenvalue_type = 'reference'.
Additionally, you may wish to rename the constraint to include the table name (e.g.,deployment_variable_value_valid_value_type) for clarity.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/EditVariableDialog.tsx(1 hunks)apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsx(2 hunks)packages/db/drizzle/0100_robust_eternals.sql(1 hunks)packages/db/src/schema/deployment-variables.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/webservice/src/app/[workspaceSlug]/(app)/resources/(raw)/[resourceId]/variables/page.tsx
- apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/EditVariableDialog.tsx
- packages/db/src/schema/deployment-variables.ts
🧰 Additional context used
🧠 Learnings (1)
packages/db/drizzle/0100_robust_eternals.sql (1)
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#561
File: packages/db/drizzle/0100_robust_eternals.sql:2-2
Timestamp: 2025-05-11T17:51:40.351Z
Learning: The deployment_variable_value table needs to support multiple records with null values for the same variable_id, which is why a unique index with COALESCE would be problematic as it would treat all nulls as equal.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (1)
packages/db/drizzle/0100_robust_eternals.sql (1)
3-7: Core column additions and alterations are correct
The migration dropsNOT NULLonvalue, adds a non-nullvalue_typewith a safe default, and introducesreference,path, anddefault_valuecolumns. This aligns perfectly with the new discriminated union model.
| @@ -0,0 +1,11 @@ | |||
| CREATE TYPE "public"."value_type" AS ENUM('direct', 'reference');--> statement-breakpoint | |||
| DROP INDEX "deployment_variable_value_variable_id_value_index";--> statement-breakpoint | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Perform safe index removal and restore lookup performance
Dropping the index without IF EXISTS or CONCURRENTLY can fail in certain environments and block writes. Also, removing the unique index may degrade read performance. I recommend:
-DROP INDEX "deployment_variable_value_variable_id_value_index";
+DROP INDEX IF EXISTS CONCURRENTLY deployment_variable_value_variable_id_value_index;
+
+-- Non-unique index to support efficient lookups on variable_id and value_type
+CREATE INDEX IF NOT EXISTS deployment_variable_value_lookup
+ ON deployment_variable_value (variable_id, value_type);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| DROP INDEX "deployment_variable_value_variable_id_value_index";--> statement-breakpoint | |
| -- Safe removal of the old index | |
| DROP INDEX IF EXISTS CONCURRENTLY deployment_variable_value_variable_id_value_index; | |
| -- Non-unique index to support efficient lookups on variable_id and value_type | |
| CREATE INDEX IF NOT EXISTS deployment_variable_value_lookup | |
| ON deployment_variable_value (variable_id, value_type); |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor