fix: Environment create route supports upsert #424
Conversation
WalkthroughThis pull request revises the environment creation process across multiple modules. The traditional Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RouteHandler
participant EnvironmentService
participant Database
Client->>RouteHandler: POST /environments with body data
RouteHandler->>EnvironmentService: upsertEnv(body)
EnvironmentService->>Database: Upsert environment using conflict resolution
Database-->>EnvironmentService: Return result or error
EnvironmentService-->>RouteHandler: Send outcome
RouteHandler-->>Client: HTTP response
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (5)
packages/api/src/router/system.ts (1)
160-163: Confirm return handling forupsertEnv.The parallel
Promise.allcalls successfully upsert each environment (Production, QA, Staging), but the returned environment objects aren’t consumed or logged. Verify if you need to store or log these returned objects for downstream operations.apps/webservice/src/app/api/v1/environments/route.ts (2)
32-33: Streamlined transaction usage.Refactoring your handler into an async transaction call improves clarity. Ensure consistent handling of transaction rollbacks in case of partial failures within the transaction scope.
39-39: Check array length constraints.When using
inArray, consider potential performance issues for large arrays. Ifbody.releaseChannelscan grow large, you might want an alternative approach or indexing strategy.packages/db/src/create-env.ts (2)
9-33:upsertVersionChannelsfunction.
- The on-conflict usage on
policyId, deploymentIdwith partial updates tochannelIdis correct for avoiding duplicates.- Consider ensuring the
channelIdis indeed the only field that can differ on conflict; if additional fields are introduced later, you may need to expand the conflict resolution strategy.
71-72: Upsert version channels conditionally.This call is contingent upon
versionChannels.length > 0. If you need to handle an empty array or re-upserting channels to clear them, consider an explicit strategy for removal when the input is no longer present.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webservice/src/app/api/v1/environments/route.ts(3 hunks)packages/api/src/router/environment.ts(2 hunks)packages/api/src/router/system.ts(2 hunks)packages/db/src/create-env.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.
packages/api/src/router/system.tspackages/api/src/router/environment.tspackages/db/src/create-env.tsapps/webservice/src/app/api/v1/environments/route.ts
🧬 Code Definitions (4)
packages/api/src/router/system.ts (1)
packages/db/src/create-env.ts (1)
upsertEnv(34-80)
packages/api/src/router/environment.ts (1)
packages/db/src/create-env.ts (1)
upsertEnv(34-80)
packages/db/src/create-env.ts (1)
packages/db/src/schema/environment.ts (1)
environment(49-74)
apps/webservice/src/app/api/v1/environments/route.ts (2)
packages/db/src/create-env.ts (1)
upsertEnv(34-80)packages/job-dispatch/src/environment-creation.ts (1)
createJobsForNewEnvironment(13-93)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (8)
packages/api/src/router/system.ts (1)
17-17:✅ Verification successful
Ensure all references to
upsertEnvare updated.This import statement reflects a switch from
createEnvtoupsertEnv. Verify that any related call sites and tests elsewhere in the codebase are properly updated to avoid reference errors or inconsistent usage.Run this script to identify any residual references to
createEnv:
🏁 Script executed:
#!/bin/bash rg 'createEnv' -A 3 -B 3Length of output: 13827
All
upsertEnvreferences verified and correctly updatedIt appears that the business logic updating is complete. In files like packages/api/src/router/system.ts and packages/api/src/router/environment.ts, the use of
upsertEnvis correctly applied. The remaining occurrences ofcreateEnv(e.g., in various environment configuration files such as packages/api/src/config.ts, packages/db/src/config.ts, packages/secrets/src/index.ts, etc.) are part of the intended environment setup from@t3-oss/env-coreand do not require updating.packages/api/src/router/environment.ts (2)
15-15: Import alignment looks good.The replacement of
createEnvwithupsertEnvaligns with the new conflict-resolution strategy. No issues found.
216-216: Validate create vs. upsert scenarios.Switching this mutation to
upsertEnvmeans existing environments will be updated. Ensure this behavior is intended and doesn’t break existing assumptions about creating strictly new environments.Please verify in your tests that calling this endpoint with an already-existing environment name behaves as expected.
apps/webservice/src/app/api/v1/environments/route.ts (3)
7-7:upsertEnvimport.Bringing in
upsertEnvis consistent with the updated database logic. Looks fine.
48-51: Migration toupsertEnvrecognized.This mirrors the PR’s overall shift to upsert logic. It simplifies environment creation by removing explicit conflict checks. Verify that you don’t require a
409 Conflictstatus if the environment already exists.
57-64: Improved error handling.Centralizing the error logging here is good for maintainability. The error response is clear, and the logging is helpful. No immediate concerns.
packages/db/src/create-env.ts (2)
5-5: Utility functions usage confirmed.Ensuring
buildConflictUpdateColumnsandtakeFirstare imported is crucial for the upsert flow. Looks good.
50-58: Conflict resolution for environment records.Your
onConflictDoUpdateapproach on[environment.name, environment.systemId]ensures name collisions under the same system are handled gracefully. Great for multi-tenant or multi-environment setups.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/db/src/upsert-env.ts (1)
34-80: Revisit error handling for environmentPolicy creation.While transitioning to an upsert approach is beneficial, the code does not appear to handle conflicts for
environmentPolicy. If multiple calls insert the samename+systemIdcombination, this might cause an error. Consider either:
- Using
onConflictDoUpdateonenvironmentPolicyif you aim for a true upsert everywhere.- Or verifying that no duplicates exist prior to insertion.
This consistency can prevent runtime errors due to race conditions.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/db/src/index.ts(1 hunks)packages/db/src/upsert-env.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.
packages/db/src/index.tspackages/db/src/upsert-env.ts
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (2)
packages/db/src/index.ts (1)
9-9:✅ Verification successful
Ensure all references to the new upsert-based function are updated.
Exporting from
"./upsert-env.js"aligns with the PR's goal of transitioning fromcreateEnvtoupsertEnv. Double-check that any code still referencingcreate-env.jshas also been migrated to avoid import errors.To verify, you may search your entire codebase for references to
create-env.js:
🏁 Script executed:
#!/bin/bash rg 'create-env'Length of output: 15
Code Migration Validation: Upsert Functionality Confirmed
- The export in
packages/db/src/index.tscorrectly references"./upsert-env.js".- Our search for any references to
create-env.js(and similar patterns) returned no results, indicating that all occurrences have been migrated.packages/db/src/upsert-env.ts (1)
5-5: Good use ofbuildConflictUpdateColumnsfor partial updates.Bringing this helper function into scope is essential for dynamically handling conflict columns. Confirm that the columns specified in your upsert operations accurately reflect all the fields you want to allow updates for.
| const upsertVersionChannels = ( | ||
| db: Tx, | ||
| policyId: string, | ||
| deploymentVersionChannels: { channelId: string; deploymentId: string }[], | ||
| ) => | ||
| db.insert(SCHEMA.environmentPolicyDeploymentVersionChannel).values( | ||
| deploymentVersionChannels.map(({ channelId, deploymentId }) => ({ | ||
| policyId, | ||
| channelId, | ||
| deploymentId, | ||
| })), | ||
| ); | ||
|
|
||
| export const createEnv = async ( | ||
| db | ||
| .insert(SCHEMA.environmentPolicyDeploymentVersionChannel) | ||
| .values( | ||
| deploymentVersionChannels.map(({ channelId, deploymentId }) => ({ | ||
| policyId, | ||
| channelId, | ||
| deploymentId, | ||
| })), | ||
| ) | ||
| .onConflictDoUpdate({ | ||
| target: [ | ||
| SCHEMA.environmentPolicyDeploymentVersionChannel.policyId, | ||
| SCHEMA.environmentPolicyDeploymentVersionChannel.deploymentId, | ||
| ], | ||
| set: buildConflictUpdateColumns( | ||
| SCHEMA.environmentPolicyDeploymentVersionChannel, | ||
| ["channelId"], | ||
| ), | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Validate that policyId exists and is correct before processing.
While this upsert logic for version channels works well, consider pre-validating that policyId is non-empty, ensuring your insertion or update does not fail silently. If policyId can be null or empty, you may want to guard against that earlier or provide an explicit error path.
🏁 Script executed:
#!/bin/bash
# Searching for upsertVersionChannels calls outside of this file to confirm they safeguard valid policyId
rg 'upsertVersionChannels'Length of output: 182
Action Required: Validate that policyId is non-empty before processing
After reviewing the call sites, we found that upsertVersionChannels is invoked with a policyId value without explicit checks to ensure it is non-empty. In the file packages/db/src/upsert-env.ts, the function is called as follows:
await upsertVersionChannels(db, policyId, versionChannels);
There’s no evidence in this file or call chain of an earlier validation for policyId. To prevent silent failures during insert or update operations, please add a guard to validate that policyId is defined and non-empty—either before calling upsertVersionChannels or within the function itself.
Summary by CodeRabbit
New Features
Bug Fixes
Chores