Skip to content

Conversation

adityachoudhari26
Copy link
Contributor

@adityachoudhari26 adityachoudhari26 commented Apr 28, 2025

Summary by CodeRabbit

  • New Features

    • Introduced API endpoints to create and retrieve deployment variables for a specific deployment, supporting multiple values and default value designation.
    • Added support for sensitive variable values and resource-based selectors.
    • Enhanced validation to ensure only one default value can be set per variable.
  • Bug Fixes

    • Improved validation on resource selectors and default value constraints to prevent invalid configurations.
  • Tests

    • Added comprehensive end-to-end tests covering creation, retrieval, and validation scenarios for deployment variables.
  • Documentation

    • Updated API documentation and OpenAPI specification to reflect the new deployment variables endpoints and schemas.

Copy link
Contributor

coderabbitai bot commented Apr 28, 2025

Walkthrough

This change introduces a new API for managing deployment variables, including both backend implementation and OpenAPI documentation. It adds GET and POST endpoints at /v1/deployments/{deploymentId}/variables, allowing retrieval and creation of deployment variables for a given deployment. The change includes schema definitions for deployment variables and their values, validation logic to ensure only one default value per variable, and encryption/decryption for sensitive values. End-to-end tests and supporting YAML configuration are added to verify the new functionality. The database schema and router are updated to support the new validation and input structure.

Changes

File(s) Change Summary
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts,
openapi.v1.json,
e2e/api/schema.ts
Added OpenAPI v3 specification and schema definitions for deployment variables API, including endpoints, request/response structures, and error handling. Updated OpenAPI JSON and test schema to reflect new endpoints and data models.
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts Implemented GET and POST handlers for deployment variables API. GET retrieves all variables for a deployment, decrypting sensitive values; POST creates a new variable, validates input, ensures only one default value, encrypts sensitive data, and queues update events. Includes error handling and uses authentication/authorization middleware.
e2e/tests/api/deployment-variable.spec.ts Added end-to-end test suite for deployment variables API, covering creation, retrieval, multiple values, default value logic, and validation for multiple defaults. Includes setup and teardown of test data.
e2e/tests/api/deployment-variable.spec.yaml Added YAML configuration defining a system and deployment for use in end-to-end API tests.
packages/db/src/schema/deployment-variables.ts Refactored and enhanced schema definitions for deployment variables and values. Added validation to ensure at most one default value, refined resource selector validation, and updated omitted/extended fields for insert schemas.
packages/api/src/router/deployment-variable.ts Updated the router's create mutation input structure to accept a variableId and nested data object, aligning with the new schema and validation logic. Adjusted insertion and default value logic accordingly.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/AddVariableValueDialog.tsx Modified mutation call argument shape for creating a variable value to pass variableId separately and nest form values inside a data property, aligning with updated router input structure. Added error handling to show a toast notification on failure.
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/CreateVariableDialog.tsx Changed mutation call argument structure for creating deployment variables to nest form values inside a data property while passing deploymentId separately, matching updated router input expectations.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant API_Route
    participant Auth
    participant DB
    participant Crypto
    participant EventQueue

    Client->>API_Route: POST /v1/deployments/{deploymentId}/variables (variable data)
    API_Route->>Auth: Authenticate & authorize user
    Auth-->>API_Route: Success/Failure
    API_Route->>DB: Verify deployment exists
    DB-->>API_Route: Deployment found/not found
    API_Route->>DB: Begin transaction
    API_Route->>Crypto: Encrypt sensitive variable values
    Crypto-->>API_Route: Encrypted values
    API_Route->>DB: Insert variable and values
    DB-->>API_Route: Inserted records
    API_Route->>DB: Set default value if specified
    API_Route->>DB: Commit transaction
    API_Route->>EventQueue: Enqueue variable update event
    EventQueue-->>API_Route: Ack
    API_Route-->>Client: 201 Created (variable with default value)
Loading
sequenceDiagram
    participant Client
    participant API_Route
    participant Auth
    participant DB
    participant Crypto

    Client->>API_Route: GET /v1/deployments/{deploymentId}/variables
    API_Route->>Auth: Authenticate & authorize user
    Auth-->>API_Route: Success/Failure
    API_Route->>DB: Fetch deployment and variables
    DB-->>API_Route: Variables with values
    API_Route->>Crypto: Decrypt sensitive values
    Crypto-->>API_Route: Decrypted values
    API_Route-->>Client: 200 OK (variables and values)
Loading

Suggested reviewers

  • jsbroks

Poem

🥕
A variable hops in, encrypted with care,
One default per patch, lest bugs should ensnare.
With schemas refined and tests in a row,
Deployments now flourish, their secrets in tow.
The API garden grows neat and robust—
A rabbit’s delight: in code we trust!
🐇


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ff28409 and e48bfe2.

📒 Files selected for processing (8)
  • apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/CreateVariableDialog.tsx (1 hunks)
  • apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts (1 hunks)
  • e2e/api/schema.ts (3 hunks)
  • e2e/tests/api/deployment-variable.spec.ts (1 hunks)
  • e2e/tests/api/deployment-variable.spec.yaml (1 hunks)
  • openapi.v1.json (2 hunks)
  • packages/api/src/router/deployment-variable.ts (2 hunks)
  • packages/db/src/schema/deployment-variables.ts (3 hunks)
✅ Files skipped from review due to trivial changes (1)
  • e2e/tests/api/deployment-variable.spec.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
  • apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts
  • packages/api/src/router/deployment-variable.ts
  • e2e/tests/api/deployment-variable.spec.ts
  • packages/db/src/schema/deployment-variables.ts
  • e2e/api/schema.ts
  • openapi.v1.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)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/CreateVariableDialog.tsx
⏰ 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 (1)
apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/CreateVariableDialog.tsx (1)

61-65: API input structure update correctly implemented.

The change properly restructures the argument passed to create.mutateAsync by wrapping form values inside a data property while keeping deploymentId at the top level. This aligns with the API schema updates that now expect this nested structure.

✨ Finishing Touches
  • 📝 Generate Docstrings

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (6)
apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts (2)

11-24: Consider specifying the type for the value property.

While the current definition works, since the value property can contain any type, it would be more explicit to define it as:

-          value: {},
+          value: { 
+            type: "object",
+            additionalProperties: true,
+            description: "Any valid JSON value" 
+          },

This would improve documentation and make it clearer that any JSON value is permitted.


126-135: Consider explicit type for request body value property.

Similar to the schema definition, the value property in the request body would benefit from more explicit typing:

-                        value: {},
+                        value: { 
+                          type: "object",
+                          additionalProperties: true,
+                          description: "Any valid JSON value" 
+                        },

This would improve API documentation for consumers.

e2e/tests/api/deployment-variable.spec.ts (2)

129-134: Order-based value assertions are non-deterministic

receivedValues[0] vs receivedValues[1] assumes the DB/orderBy will always return rows in insertion order – that is not guaranteed unless you specify an ORDER BY.
Safer:

-    const receivedValueA = receivedValues[0]!.value;
-    const receivedValueB = receivedValues[1]!.value;
-    expect(receivedValueA).toBe(valueA);
-    expect(receivedValueB).toBe(valueB);
+    const values = receivedValues.map((v) => v.value);
+    expect(values).toEqual(expect.arrayContaining([valueA, valueB]));

Also applies to: 190-196


201-204: Typo in test title

"shoudl fail…" → "should fail…" – improves readability in test reports.

-test("shoudl fail if more than one default value is provided", async ({
+test("should fail if more than one default value is provided", async ({
openapi.v1.json (2)

888-1089: Add descriptive metadata, examples, and pagination options to /v1/deployments/{deploymentId}/variables
The new GET and POST operations look correct, but we can improve discoverability and scalability:

  1. Add a description for the deploymentId path parameter for consistency.
  2. Introduce optional limit and offset query parameters to support pagination on GET.
  3. Provide request/response examples under each response code.
  4. In the POST body schema, clarify the semantics of the "default" flag in values (e.g. only one may be true).

Example diff:

--- a/openapi.v1.json
+++ b/openapi.v1.json
@@ "/v1/deployments/{deploymentId}/variables": {
     "get": {
         "summary": "Get all variables for a deployment",
         "operationId": "getDeploymentVariables",
-        "parameters": [
-          {
-            "name": "deploymentId",
-            "in": "path",
-            "required": true,
-            "schema": {
-              "type": "string",
-              "format": "uuid"
-            }
-          }
-        ],
+        "parameters": [
+          {
+            "name": "deploymentId",
+            "in": "path",
+            "required": true,
+            "schema": {
+              "type": "string",
+              "format": "uuid"
+            },
+            "description": "UUID of the deployment to fetch variables for"
+          },
+          {
+            "name": "limit",
+            "in": "query",
+            "schema": {
+              "type": "integer",
+              "minimum": 1,
+              "default": 100
+            },
+            "description": "Maximum number of variables to return"
+          },
+          {
+            "name": "offset",
+            "in": "query",
+            "schema": {
+              "type": "integer",
+              "minimum": 0,
+              "default": 0
+            },
+            "description": "Offset for pagination"
+          }
+        ],
+        "responses": {
+          "200": {
+            "description": "Variables fetched successfully",
+            "content": {
+              "application/json": {
+                "schema": {
+                  "type": "array",
+                  "items": { "$ref": "#/components/schemas/DeploymentVariable" }
+                },
+                "examples": {
+                  "success": {
+                    "value": [
+                      {
+                        "id": "f47ac10b-58cc-4372-a567-0e02b2c3d479",
+                        "key": "API_KEY",
+                        "description": "Key for external API",
+                        "values": [
+                          {
+                            "id": "a12bc34d-56ef-78gh-90ij-123klm456nop",
+                            "value": "abcd1234",
+                            "sensitive": true,
+                            "resourceSelector": null
+                          }
+                        ],
+                        "defaultValue": {
+                          "id": "a12bc34d-56ef-78gh-90ij-123klm456nop",
+                          "value": "abcd1234",
+                          "sensitive": true,
+                          "resourceSelector": null
+                        },
+                        "config": {}
+                      }
+                    ]
+                  }
+                }
+              }
+            }
+          }

4625-4684: Enhance the DeploymentVariableValue and DeploymentVariable schema definitions
The new schemas capture the core data but lack descriptive metadata and explicit nullability on defaultValue. Consider:

  1. Adding description and example fields to each schema and property.
  2. Marking defaultValue as nullable to clarify it's optional.
  3. Optionally enforcing additionalProperties: false to prevent unknown fields.

Example diff:

--- a/openapi.v1.json
+++ b/openapi.v1.json
@@ -4625,7 +4625,13 @@
       "DeploymentVariableValue": {
         "type": "object",
+        "description": "An entry for a deployment variable's possible value, including sensitivity and resource targeting",
         "properties": {
           "id": {
             "type": "string",
             "format": "uuid",
+            "description": "Unique ID for this variable value",
+            "example": "a12bc34d-56ef-78gh-90ij-123klm456nop"
           },
           "value": {},
+          "value": {
+            "description": "Stored value (plaintext or encrypted)",
+            "example": "secret-token"
+          },
           "sensitive": {
             "type": "boolean",
             "description": "Whether this value should be encrypted at rest",
@@ -4648,7 +4654,14 @@
       "DeploymentVariable": {
         "type": "object",
+        "description": "A variable attached to a deployment, with all its values and the optional default",
         "properties": {
           "id": {
             "type": "string",
             "format": "uuid",
+            "description": "Unique ID for this variable",
+            "example": "f47ac10b-58cc-4372-a567-0e02b2c3d479"
           },
           "key": {
             "type": "string",
@@ -4668,7 +4683,14 @@
           "defaultValue": {
             "$ref": "#/components/schemas/DeploymentVariableValue"
           },
+          "defaultValue": {
+            "$ref": "#/components/schemas/DeploymentVariableValue",
+            "nullable": true,
+            "description": "The default value from the `values` list, if one is marked"
+          },
           "config": {
             "type": "object",
             "additionalProperties": true,
+            "description": "Arbitrary key/value map for variable behavior customization",
+            "example": { "env": "production" }
           }
         },
         "required": [
📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 86b951a and 8dca9fb.

📒 Files selected for processing (8)
  • apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts (1 hunks)
  • apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts (1 hunks)
  • e2e/api/schema.ts (3 hunks)
  • e2e/tests/api/deployment-variable.spec.ts (1 hunks)
  • e2e/tests/api/deployment-variable.spec.yaml (1 hunks)
  • openapi.v1.json (2 hunks)
  • packages/api/src/router/deployment-variable.ts (1 hunks)
  • packages/db/src/schema/deployment-variables.ts (3 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.

  • e2e/tests/api/deployment-variable.spec.ts
  • apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/route.ts
  • apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts
  • packages/db/src/schema/deployment-variables.ts
  • packages/api/src/router/deployment-variable.ts
  • e2e/api/schema.ts
🧬 Code Graph Analysis (1)
packages/db/src/schema/deployment-variables.ts (2)
packages/validators/src/resources/conditions/resource-condition.ts (2)
  • resourceCondition (41-52)
  • isValidResourceCondition (145-164)
packages/validators/src/variables/index.ts (1)
  • VariableConfig (55-60)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Typecheck
  • GitHub Check: Lint
  • GitHub Check: build (linux/amd64)
🔇 Additional comments (11)
packages/db/src/schema/deployment-variables.ts (3)

18-21: Added imports for resource validation functions.

The imports for isValidResourceCondition and resourceCondition are properly added to support the enhanced validation of resource selectors in the schema definitions.


82-85: Enhanced validation for deployment variable values.

Good improvements to the createDeploymentVariableValue schema:

  • Refined validation for resourceSelector using the isValidResourceCondition function
  • Omitted auto-generated fields (id and variableId) as these are handled by the database
  • Added an optional default boolean field to identify default values

These changes strengthen the validation and provide better structure for variable values.


117-135: Good implementation of the single default value constraint.

The createDeploymentVariable schema now includes proper validation with:

  • Refined field validation for key and config
  • Omitted auto-generated and internal fields
  • Support for an optional array of values
  • A refinement function that ensures at most one value can be marked as default

This implementation correctly enforces the business rule that a deployment variable can have at most one default value.

e2e/tests/api/deployment-variable.spec.yaml (1)

1-10: Well-structured test configuration.

The YAML configuration for E2E tests is properly set up with:

  • Clear naming using template variables for dynamic test environment setup
  • Appropriate descriptions for both system and deployment entities
  • Minimal but sufficient configuration for testing the deployment variables feature

This configuration correctly supports the E2E tests for the deployment variables API.

packages/api/src/router/deployment-variable.ts (2)

50-55: Good restructuring of the input schema.

The input schema has been properly updated to use a nested structure with:

  • A required variableId field to identify the target variable
  • A nested data field containing the actual variable value properties

This structure provides better separation of concerns between entity identification and entity data.


60-64: Updated implementation to match new input schema.

The mutation implementation correctly adapts to the new input schema by:

  • Spreading input.data instead of the entire input object
  • Explicitly setting variableId from input.variableId
  • Checking input.data.default instead of input.default

These changes maintain consistency with the schema updates.

apps/webservice/src/app/api/v1/deployments/[deploymentId]/variables/openapi.ts (4)

3-8: Clean OpenAPI metadata setup.

The OpenAPI specification is properly initialized with:

  • Correct OpenAPI version
  • Appropriate title and version information

25-44: Well-defined DeploymentVariable schema.

The DeploymentVariable schema is complete with all required fields and appropriate references to the DeploymentVariableValue schema.


49-97: Complete GET endpoint definition.

The GET endpoint is well-defined with:

  • Proper path parameters
  • Appropriate response schemas for success and error cases
  • Clear operation summary and ID

98-191: Comprehensive POST endpoint definition.

The POST endpoint is well-structured with:

  • Required path parameters
  • Detailed request body schema including optional and required fields
  • Appropriate response schemas for success and error cases
  • Proper validation messages for different error scenarios

The endpoint definition aligns well with the schema changes in the database layer.

e2e/api/schema.ts (1)

148-165: OpenAPI spec responses disagree with implementation & tests

  1. POST /v1/deployments/{deploymentId}/variables
    • Route returns 201 Created, Playwright tests expect 201, but the spec lists only 200.
  2. requestBody is marked optional (requestBody?) although the route requires it (validated by zod schema).
  3. Similar mismatch exists at path level (lines 148-165) where no 201 is documented.

Leaving the discrepancy will break client SDK generation and contract testing.

Action items:
• Update the OpenAPI source to include 201 (and preferably 400, 404, 500) for createDeploymentVariable.
• Mark requestBody as required.
• Regenerate e2e/api/schema.ts via openapi-typescript.

Would you like me to open a follow-up issue or submit a PR to adjust the OpenAPI spec?

Also applies to: 1894-1902

Comment on lines +70 to +76
expect(variableGetResponse.response.status).toBe(200);
const variables = variableGetResponse.data ?? [];
expect(variables.length).toBe(1);
const receivedVariable = variables[0]!;
const receivedGetKey = receivedVariable.key;
expect(receivedGetKey).toBe(key);
});
Copy link
Contributor

@coderabbitai coderabbitai bot Apr 28, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Tests rely on a fixed variable count – causes inter-test coupling & flakiness

Each test creates a new variable but still asserts variables.length is exactly 1.
After the first test runs, the deployment already has one variable persisted, so subsequent tests will inevitably get length >= 2 and fail.
Playwright executes tests in the same file serially, yet their state is shared unless you clean it up between tests.

Suggested change (apply to every place the length is asserted):

-    expect(variables.length).toBe(1);
-    const receivedVariable = variables[0]!;
+    const receivedVariable = variables.find((v) => v.key === key)!;
+    expect(receivedVariable).toBeDefined();

This isolates each test from the others and makes the suite order-independent.
Optionally add test.describe.serial if you really need serial execution.

Also applies to: 122-128, 184-188

Copy link
Contributor Author

Choose a reason for hiding this comment

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

each test is isolated in its own prefixed system

Copy link
Contributor

Choose a reason for hiding this comment

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

@adityachoudhari26 I understand that each test run is isolated with a uniquely prefixed system through the YAML template with {{ randId }}.

However, my concern was about isolation between tests within a single run. Looking at the code, all tests in this file:

  1. Use the same deployment instance (importedEntities.deployments[0])
  2. Create a new variable on this deployment
  3. Assert expect(variables.length).toBe(1)

When tests run sequentially in the same process, the second test would find 2 variables (one from the first test, one it just created), causing the assertion to fail.

If there's a mechanism that isolates each test case that isn't visible in the code, please let me know. Otherwise, changing to something like this would make tests independent:

const receivedVariable = variables.find((v) => v.key === key)!;
expect(receivedVariable).toBeDefined();

✏️ Learnings added
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#517
File: e2e/tests/api/deployment-variable.spec.ts:70-76
Timestamp: 2025-04-28T21:59:04.706Z
Learning: In the ctrlplane e2e tests, tests can be isolated between runs using prefixed systems with random IDs in YAML templates, but tests within the same file may still need to handle isolation between test cases if they operate on the same resources.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you misunderstand. each test within a file has its own isolated system

Copy link
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Let's first check the content of the YAML file
cat e2e/tests/api/deployment-variable.spec.yaml

# Then examine how the import entities function works
cat $(find e2e -type f -name "*.ts" | grep -E 'importEntitiesFromYaml|api\.ts')

# Look at the test fixtures to understand how tests are configured
cat e2e/tests/fixtures.ts

# Check if there's randomization per test in any related files
grep -r "test\(" --include="*.ts" e2e/tests/api | grep -E "workspace|deployment|system" | head -n 20

Length of output: 1349


@adityachoudhari26 Thanks for the clarification. I apologize for the misunderstanding.

Looking at the code more carefully, I see that the YAML template uses {{ prefix }} for system and deployment names/slugs, which suggests a mechanism for creating uniquely identified systems for each test.

However, I'm still a bit confused because in the test file:

  1. The entities are imported once via importEntitiesFromYaml in the beforeAll hook
  2. All tests reference the same deployment with importedEntities.deployments[0]
  3. Each test creates variables on this deployment and expects variables.length to be 1

Could you help me understand how each test gets its own isolated system when they all appear to use the same importedEntities object? Is there additional test isolation machinery that's not visible in the provided code?


🧬 Code Graph Analysis Results

Relevant snippets from e2e/tests/fixtures.ts (lines 11-26)

export const test = base.extend<{
  workspace: WorkspaceFixture;
  api: ReturnType<typeof createClient>;
}>({
  workspace: async ({}, use) => {
    const workspaceData: WorkspaceFixture = JSON.parse(
      await fs.promises.readFile(workspaceFile, "utf-8"),
    );

    await use(workspaceData);
  },
  api: async ({ workspace: { apiKey }, baseURL }, use) => {
    const api = createClient({ apiKey, baseUrl: baseURL });
    await use(api);
  },
});
  • Defines a Playwright test fixture extension that provides workspace and api fixtures.
  • workspace fixture loads workspace data from a file.
  • api fixture creates an API client using the workspace's API key and base URL.
  • These fixtures are used in the test file to provide isolated API clients and workspace context per test.

Relevant snippets from e2e/api/yaml-loader.ts (lines 109-300)

export async function importEntitiesFromYaml(
  api: ApiClient,
  workspaceId: string,
  yamlFilePath: string,
): Promise<ImportedEntities> {
  // Loads and parses a YAML file describing system, environments, resources, deployments, policies.
  // Creates these entities via API calls in the given workspace.
  // Returns an ImportedEntities object containing created entities' IDs and metadata.

  // Key steps:
  // - Reads and templates YAML file with a random prefix.
  // - Creates system via POST /v1/systems.
  // - Creates resources via POST /v1/resources.
  // - Creates environments via POST /v1/environments.
  // - Creates deployments via POST /v1/deployments and deployment versions.
  // - Creates policies via POST /v1/policies.
  // Throws errors if any creation fails.
}

Relevant snippets from e2e/api/yaml-loader.ts (lines 307-355)

export async function cleanupImportedEntities(
  api: ApiClient,
  entities: ImportedEntities,
  workspaceId: string,
): Promise<void> {
  // Deletes policies, deployments, environments, system, and resources created during import.
  // Uses DELETE API calls with appropriate entity IDs.
  // Logs errors if resource deletion fails.
}

Summary of e2e/tests/api/deployment-variable.spec.ts

  • Uses test fixture from fixtures.ts which provides isolated api and workspace fixtures.
  • Uses importEntitiesFromYaml to import system, deployments, etc. before all tests in the file.
  • Uses cleanupImportedEntities after all tests to clean up.
  • Each test uses the imported deployment entities to create and verify deployment variables via API.
  • Tests rely on the shared imported entities created in beforeAll and cleaned up in afterAll.

These snippets provide context on how the test environment and imported entities are set up and cleaned up, and how the API client and workspace fixtures are provided in an isolated manner per test. This is relevant to understanding the isolation scope of tests within the file.


🧠 Learnings used
Learnt from: adityachoudhari26
PR: ctrlplanedev/ctrlplane#517
File: e2e/tests/api/deployment-variable.spec.ts:70-76
Timestamp: 2025-04-28T21:59:04.706Z
Learning: In the ctrlplane e2e tests, tests can be isolated between runs using prefixed systems with random IDs in YAML templates, but tests within the same file may still need to handle isolation between test cases if they operate on the same resources.

Comment on lines +95 to +109
>(async ({ db, body }, { params }) => {
try {
const { deploymentId } = await params;

const deployment = await db.query.deployment.findFirst({
where: eq(schema.deployment.id, deploymentId),
});

if (deployment == null)
return NextResponse.json(
{ error: "Deployment not found" },
{ status: NOT_FOUND },
);

const { values, ...rest } = body;
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Path-/body deploymentId mismatch can create/overwrite variables in the wrong deployment

The current implementation trusts the deploymentId coming from the body (rest) instead of the path param.
A malicious caller could POST to /…/deployments/A/variables with { deploymentId: "B", … } and create data under deployment B if they have only update permission on A.

-      const { values, ...rest } = body;
+      const { values, deploymentId: bodyDeploymentId, ...rest } = body;
+
+      if (bodyDeploymentId != null && bodyDeploymentId !== deploymentId) {
+        return NextResponse.json(
+          { error: "Body deploymentId does not match path parameter" },
+          { status: 400 },
+        );
+      }

and later

-          .values(rest)
+          .values({ ...rest, deploymentId })

Also applies to: 111-116, 132-136

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8dca9fb and b7dd1cf.

📒 Files selected for processing (1)
  • apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/AddVariableValueDialog.tsx (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.

  • apps/webservice/src/app/[workspaceSlug]/(app)/(deploy)/(raw)/systems/[systemSlug]/(raw)/deployments/[deploymentSlug]/(sidebar)/variables/AddVariableValueDialog.tsx

Comment on lines 59 to 63
const onSubmit = form.handleSubmit(async (values) => {
await create.mutateAsync({ ...values, variableId: variable.id });
await create.mutateAsync({ variableId: variable.id, data: values });
router.refresh();
setOpen(false);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

API schema change implemented correctly, but error handling is missing.

The change to wrap form values in a data property correctly aligns with the updated backend schema. However, there's no error handling if the mutation fails, which could lead to a poor user experience.

Consider adding error handling to provide user feedback when the mutation fails:

 const onSubmit = form.handleSubmit(async (values) => {
-    await create.mutateAsync({ variableId: variable.id, data: values });
-    router.refresh();
-    setOpen(false);
+    try {
+      await create.mutateAsync({ variableId: variable.id, data: values });
+      router.refresh();
+      setOpen(false);
+    } catch (error) {
+      // Show error notification or set form error
+      console.error("Failed to create variable value:", error);
+      // Optional: Add UI notification here
+    }
 });
📝 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.

Suggested change
const onSubmit = form.handleSubmit(async (values) => {
await create.mutateAsync({ ...values, variableId: variable.id });
await create.mutateAsync({ variableId: variable.id, data: values });
router.refresh();
setOpen(false);
});
const onSubmit = form.handleSubmit(async (values) => {
try {
await create.mutateAsync({ variableId: variable.id, data: values });
router.refresh();
setOpen(false);
} catch (error) {
// Show error notification or set form error
console.error("Failed to create variable value:", error);
// Optional: Add UI notification here
}
});

@adityachoudhari26 adityachoudhari26 merged commit 0430014 into main Apr 28, 2025
8 of 10 checks passed
@adityachoudhari26 adityachoudhari26 deleted the deployment-variable-route branch April 28, 2025 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant