feat: init computed resource selector tables #484
Conversation
WalkthroughThis pull request integrates new worker functions and enhanced database schema elements to manage resource selectors more dynamically across deployments and environments. It introduces new SQL tables for computed resource selectors and updates various TypeScript modules with builder classes that facilitate query construction. New enum values and corresponding channel mappings are added for deployment updates. Additionally, the control flow in worker functions and API endpoints is modified to recompute resource selectors through asynchronous calls, centralizing resource matching logic and eliminating redundant helper functions. Changes
Sequence Diagram(s)sequenceDiagram
actor Event as Event Source
participant UDW as UpdateDeploymentWorker
participant Sel as Selector Function
participant DB as Database
Event->>UDW: Trigger UpdateDeployment event (includes oldSelector & resourceSelector)
UDW->>UDW: Compare selectors using _.isEqual
alt Selectors are equal
UDW-->>Event: Exit early (no action)
else Selectors differ
UDW->>Sel: Compute deployment resource selectors
Sel->>DB: Query and update computed resources
DB-->>Sel: Return updated resource data
Sel-->>UDW: Return computed results
UDW-->>Event: Proceed with deployment update
end
sequenceDiagram
actor Event as Event Source
participant UEW as UpdateEnvironmentWorker
participant Sel as Selector Function
participant DB as Database
Event->>UEW: Trigger UpdateEnvironment event (with workspaceId)
UEW->>Sel: Compute environment resource selectors
Sel->>DB: Query and update computed selectors for environments
DB-->>Sel: Return updated data
Sel-->>UEW: Return computed results
UEW-->>Event: Continue with resource matching operations
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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
|
![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/db/src/schema/deployment.ts (1)
116-129: Consider using stricter constraints or a composite primary key.
Currently,deploymentIdandresourceIdare not explicitly markednotNull(). If the relationship is mandatory, marking them as non-null can prevent accidental null inserts. Additionally, you could enforce uniqueness by making(deploymentId, resourceId)a composite primary key (and remove theidcolumn), if you want to simplify the schema.packages/db/src/schema/environment.ts (1)
135-148: Consider non-null ID references or a composite primary key.
Similar to yourdeploymentSelectorComputedResourcetable, you may want to markenvironmentIdandresourceIdas non-null if they're always required. You could also consider using(environmentId, resourceId)as a composite primary key instead of a separateidfield, which might simplify the schema.packages/db/src/resources/selector-diff.ts (1)
1-63: Well-structured resource selector diff logic.
The function is clear and maintains readability. As a nice-to-have, consider adding unit tests or integration tests with edge cases (e.g., both selectors empty, large sets) to ensure future reliability.apps/event-worker/src/workers/update-deployment.ts (1)
6-35: Optimize insert handling for empty arrays.
TheupdateDeploymentSelectorComputedResourcesfunction is well-structured. For a slight optimization, you can skip the insert statement whenmatchedResourceIdsis empty, reducing an unnecessary DB call.apps/event-worker/src/workers/update-environment.ts (1)
129-158: Consider wrapping these operations in a transaction for atomic updates.
Deleting unmatched resources followed by inserting newly/unchanged matched ones in separate statements can introduce race conditions if multiple updates occur concurrently. Executing them in a single transaction can prevent partial updates and maintain stronger data consistency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/event-worker/src/workers/index.ts(2 hunks)apps/event-worker/src/workers/update-deployment.ts(1 hunks)apps/event-worker/src/workers/update-environment.ts(3 hunks)packages/db/src/index.ts(1 hunks)packages/db/src/resources/index.ts(1 hunks)packages/db/src/resources/selector-diff.ts(1 hunks)packages/db/src/schema/deployment.ts(2 hunks)packages/db/src/schema/environment.ts(2 hunks)packages/events/src/types.ts(2 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/resources/index.tsapps/event-worker/src/workers/index.tspackages/db/src/schema/deployment.tspackages/events/src/types.tspackages/db/src/schema/environment.tsapps/event-worker/src/workers/update-environment.tspackages/db/src/resources/selector-diff.tsapps/event-worker/src/workers/update-deployment.ts
🧬 Code Graph Analysis (6)
apps/event-worker/src/workers/index.ts (1)
apps/event-worker/src/workers/update-deployment.ts (1)
updateDeploymentWorker(37-65)
packages/db/src/schema/deployment.ts (1)
packages/db/src/schema/resource.ts (1)
resource(59-87)
packages/events/src/types.ts (2)
packages/db/src/schema/deployment.ts (1)
Deployment(103-103)packages/validators/src/resources/conditions/resource-condition.ts (1)
ResourceCondition(29-39)
packages/db/src/schema/environment.ts (1)
packages/db/src/schema/resource.ts (1)
resource(59-87)
apps/event-worker/src/workers/update-environment.ts (3)
packages/db/src/client.ts (1)
db(14-14)packages/db/src/resources/selector-diff.ts (1)
getResourceSelectorDiff(20-63)packages/db/src/schema/environment.ts (1)
environment(58-83)
apps/event-worker/src/workers/update-deployment.ts (5)
packages/db/src/client.ts (1)
db(14-14)packages/events/src/index.ts (1)
createWorker(10-25)packages/db/src/schema/job.ts (1)
job(74-106)packages/db/src/schema/deployment.ts (1)
deployment(67-91)packages/db/src/resources/selector-diff.ts (1)
getResourceSelectorDiff(20-63)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (11)
packages/db/src/resources/index.ts (1)
1-1: Clean export of selector-diff moduleThis export makes the functionality from selector-diff.js accessible through the resources index, which is a good practice for modular code organization.
packages/db/src/index.ts (1)
12-12: Good re-export pattern for resourcesThe addition of this export line correctly exposes the resources module (including the new selector-diff functionality) to consumers of the DB package.
packages/events/src/types.ts (2)
23-23: Well-named event channel for deployment updatesThe new
UpdateDeploymentchannel follows the existing naming convention pattern in the Channel enum.
47-49: Properly structured channel data typeThe data structure for the UpdateDeployment channel correctly includes the deployment data and an oldSelector property, which is necessary for computing differences between old and new selectors.
This matches the pattern used for UpdateEnvironment (lines 44-46), maintaining consistency across similar operations.
apps/event-worker/src/workers/index.ts (2)
13-13: Clean import of the new workerThe import follows the established pattern for worker imports in this file.
27-27: Proper registration of the deployment update workerThe worker is correctly registered to handle the UpdateDeployment channel events, maintaining the pattern established for other workers.
This registration enables the system to process deployment update events through the defined worker, completing the event processing pipeline.
packages/db/src/schema/deployment.ts (1)
24-24: Import looks good.
No concerns with this additional import; it correctly brings in theresourceschema for reference.packages/db/src/schema/environment.ts (1)
35-35: Import looks good.
The new import aligns the file with the references to theresourceschema.apps/event-worker/src/workers/update-deployment.ts (1)
37-65: Deployment update worker logic is solid.
Your approach cleanly handles old vs. new selectors and updates the computed resource table accordingly. The use ofonConflictDoNothing()reduces potential concurrency conflicts.apps/event-worker/src/workers/update-environment.ts (2)
6-12: All set on the new imports.
They appear consistent with the rest of the file and align with existing usage of@ctrlplane/dbhelpers.
200-212: Good use ofgetResourceSelectorDiffresults to update computed resources.
Merging newly matched and unchanged resources into one list for insertion ensures that the table reflects the final, correct state of environment-resource associations.
| resourceId: uuid("resource_id").references(() => resource.id, { | ||
| onDelete: "cascade", | ||
| }), | ||
| }, |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/db/src/resources/deployment-computed-resources.ts (1)
6-44: Consider adding logging for better observability.While the implementation is solid, consider adding logging at key points (function entry, after deletion, after insertion) to improve observability, especially during debugging or troubleshooting scenarios.
import { and, eq } from "drizzle-orm"; import { Tx } from "../common"; import * as SCHEMA from "../schema/index.js"; +// Import logging utility if available in your project +// import { logger } from "../utils/logger"; export const computeDeploymentComputedResources = async ( db: Tx, deploymentId: string, ) => { + // logger.debug(`Computing resources for deployment: ${deploymentId}`); const deployment = await db.query.deployment.findFirst({ where: eq(SCHEMA.deployment.id, deploymentId), with: { system: true }, }); if (deployment == null) throw new Error(`Deployment not found: ${deploymentId}`); const { system } = deployment; const { workspaceId } = system; + // logger.debug(`Deleting existing computed resources for deployment: ${deploymentId}`); await db .delete(SCHEMA.deploymentSelectorComputedResource) .where( eq(SCHEMA.deploymentSelectorComputedResource.deploymentId, deploymentId), ); + // logger.debug(`Computing and inserting new resources for deployment: ${deploymentId}`); await db .insert(SCHEMA.deploymentSelectorComputedResource) .select( db .select({ deploymentId: SCHEMA.deployment.id, resourceId: SCHEMA.resource.id, }) .from(SCHEMA.resource) .innerJoin(SCHEMA.deployment, eq(SCHEMA.deployment.id, deploymentId)) .where( and( eq(SCHEMA.resource.workspaceId, workspaceId), SCHEMA.resourceMatchesMetadata(db, deployment.resourceSelector), ), ), ) .onConflictDoNothing(); + // logger.debug(`Completed computing resources for deployment: ${deploymentId}`); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/event-worker/src/workers/update-deployment.ts(1 hunks)packages/db/package.json(1 hunks)packages/db/src/resources/deployment-computed-resources.ts(1 hunks)packages/db/src/resources/env-computed-resources.ts(1 hunks)packages/db/src/resources/index.ts(1 hunks)packages/events/src/types.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/resources/index.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.
apps/event-worker/src/workers/update-deployment.tspackages/events/src/types.tspackages/db/src/resources/deployment-computed-resources.tspackages/db/src/resources/env-computed-resources.ts
🧬 Code Graph Analysis (3)
apps/event-worker/src/workers/update-deployment.ts (3)
packages/events/src/index.ts (1)
createWorker(10-25)packages/db/src/client.ts (1)
db(14-14)packages/db/src/resources/deployment-computed-resources.ts (1)
computeDeploymentComputedResources(6-44)
packages/db/src/resources/deployment-computed-resources.ts (2)
packages/db/src/client.ts (1)
db(14-14)packages/db/src/common.ts (1)
Tx(22-22)
packages/db/src/resources/env-computed-resources.ts (3)
packages/db/src/client.ts (1)
db(14-14)packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/schema/environment.ts (1)
environment(58-83)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (11)
apps/event-worker/src/workers/update-deployment.ts (1)
5-11: Well-structured worker implementation.The worker correctly uses database transactions when computing deployment resources, ensuring atomicity and data consistency. The implementation is concise and appropriately handles the deployment update event.
packages/events/src/types.ts (2)
28-29: LGTM! Well-structured enum additions.The new channel enum values follow the existing naming convention and structure.
49-50: LGTM! Type definitions are correctly structured.The ChannelMap entries properly define the expected payload structure for the new channels, maintaining type safety.
packages/db/src/resources/env-computed-resources.ts (1)
6-47: Well-implemented database transaction function.The function correctly:
- Queries for the environment and handles the not-found case
- Deletes existing computed resources
- Inserts new resources based on the selector criteria
- Uses onConflictDoNothing for idempotency
The implementation maintains transactional integrity by using the provided transaction object throughout.
Verify that
SCHEMA.resourceMatchesMetadatacorrectly handles the case whenenvironment.resourceSelectoris null, as the schema indicates this field can be null.packages/db/src/resources/deployment-computed-resources.ts (7)
1-5: Imports look good.The imports are clean and appropriate for the file's functionality. Using named imports from drizzle-orm for specific SQL operations is a good practice.
6-9: Function signature appropriately uses transaction object.The function is properly exported and uses the
Txtype for transaction handling, which is a good practice for ensuring database operations can be part of larger transactions.
10-15: Error handling for missing deployment is well implemented.The code correctly checks if the deployment exists and throws a descriptive error if not found. The query also efficiently includes the related system information in a single database call.
17-19: Clean data extraction with destructuring.Good use of destructuring to extract the required information from the deployment object. This makes the code more readable.
20-24: Appropriate cleanup of existing resources before recalculation.The code correctly deletes existing computed resources for the deployment before inserting new ones. This ensures a clean slate for the computation results.
26-43: Well-structured query for computing resources.The insertion query is well-structured with appropriate joins and filtering. Using
innerJoinensures only valid deployments are processed, and the combination of workspace filtering with metadata matching provides proper scoping of resources.
43-44: Good conflict handling strategy.Using
onConflictDoNothing()is appropriate for this use case, ensuring idempotency if the function is called multiple times with the same data.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/db/src/resources/env-computed-resources.ts (2)
10-19: Consider using domain-specific errors for better clarity.
Throwing a plainErrormakes debugging more challenging. If possible, introduce a custom error or include error codes for improved error handling.
31-48: Be mindful of potential concurrency issues.
If multiple transactions invoke this function concurrently for the sameenvironmentId, there could be unintended race conditions during the delete-and-insert operations. Ensure higher-level transaction or locking mechanisms handle concurrent calls to guarantee consistent state.packages/db/src/resources/deployment-computed-resources.ts (1)
6-16: Consider adding a short docstring for clarity.
Adding a concise description abovecomputeDeploymentSelectorResourcescan improve readability by clarifying the function’s intent and expected side effects, especially for contributors discovering this code.packages/db/src/schema/deployment.ts (1)
117-128: New table definition is appropriate.
Defining a composite primary key for(deploymentId, resourceId)suits the context. Should queries often filter bydeploymentIdalone, consider adding a dedicated index ondeploymentIdfor improved performance.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/db/src/resources/deployment-computed-resources.ts(1 hunks)packages/db/src/resources/env-computed-resources.ts(1 hunks)packages/db/src/schema/deployment.ts(3 hunks)packages/db/src/schema/environment.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/schema/environment.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/schema/deployment.tspackages/db/src/resources/env-computed-resources.tspackages/db/src/resources/deployment-computed-resources.ts
🧬 Code Graph Analysis (2)
packages/db/src/schema/deployment.ts (1)
packages/db/src/schema/resource.ts (1)
resource(59-87)
packages/db/src/resources/deployment-computed-resources.ts (3)
packages/db/src/client.ts (1)
db(14-14)packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/schema/deployment.ts (1)
deployment(68-92)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
🔇 Additional comments (5)
packages/db/src/resources/env-computed-resources.ts (2)
6-9: Function definition looks good.
This async function signature is consistent with the overall naming and type usage.
20-29: Logic for deletion and early return is solid.
Clearing old records before insertion and skipping whenresourceSelectorisnullseems correct.packages/db/src/resources/deployment-computed-resources.ts (2)
20-25: Double-check the delete approach.
Deleting all existing computed resources for the given deployment before re-inserting is suitable if you always want to rebuild them from scratch. Ensure this aligns with your business rules (e.g., partial updates vs. full regeneration).
28-44: Insertion logic looks solid.
UsingINSERT ... SELECTwithonConflictDoNothing()is a good approach to prevent duplicates. Make sure an appropriate primary or unique key exists on the target table (which you defined indeploymentSelectorComputedResource) so the conflict resolution works as expected.packages/db/src/schema/deployment.ts (1)
25-25: Import statement aligns with usage.
Theresourceimport is necessary for referencing in the new table definition. No issues here.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/db/src/resources/deployment-computed-resources.ts (5)
1-2: Remove unused importsql
sqlis imported but not used anywhere in this file. Removing it will help keep the code clean.Apply this diff if
sqlis truly unused:- import { and, eq, sql } from "drizzle-orm"; + import { and, eq } from "drizzle-orm";
6-9: Consider adding JSDoc comments
Documenting parameters and the purpose ofcomputeDeploymentSelectorResourcescan help future collaborators quickly understand its functionality and expected inputs.
10-15: Handle the case when no workspace is found
If the query returns no rows for the provided deployment ID,takeFirstmight returnundefined, leading to aTypeErrorwhen destructuring. Consider verifying the result and throwing an error or handling it gracefully if no matching record is found.
23-23: Early return ensures no resources are stored if selector is null
Make sure to test the scenario whereresourceSelectorisnullorundefined, confirming this short-circuit logic works as intended.Would you like help generating a targeted test for this scenario?
40-44: EvaluateonConflictDoNothing()usage
This prevents duplicate entries when inserting. If you eventually need to update existing records in case of changes, consideronConflictDoUpdate()or a different approach. Otherwise, this usage looks correct for skipping duplicates.packages/db/src/resources/env-computed-resources.ts (2)
38-41: Consider batch processing for large resource sets.The current implementation processes all matching resources at once. For systems with a large number of resources, consider implementing batch processing to avoid memory issues or database constraints on bulk operations.
- const inserts = resourceIds.map((r) => ({ - environmentId: environment.id, - resourceId: r.resourceId, - })); - - await db - .insert(SCHEMA.environmentSelectorComputedResource) - .values(inserts) - .onConflictDoNothing(); + // Process in batches of 1000 resources + const batchSize = 1000; + for (let i = 0; i < resourceIds.length; i += batchSize) { + const batch = resourceIds.slice(i, i + batchSize).map((r) => ({ + environmentId: environment.id, + resourceId: r.resourceId, + })); + + if (batch.length > 0) { + await db + .insert(SCHEMA.environmentSelectorComputedResource) + .values(batch) + .onConflictDoNothing(); + } + }
1-47: Consider adding error handling and logging.The function doesn't include explicit error handling. Consider adding try/catch blocks with appropriate error logging, especially since this appears to be part of a worker system that processes events.
export const computeEnvironmentSelectorResources = async ( db: Tx, environment: Pick<SCHEMA.Environment, "id" | "resourceSelector">, ) => { + try { const { workspaceId } = await db .select({ workspaceId: SCHEMA.system.workspaceId }) .from(SCHEMA.environment) .innerJoin(SCHEMA.system, eq(SCHEMA.environment.systemId, SCHEMA.system.id)) .where(eq(SCHEMA.environment.id, environment.id)) .then(takeFirst); await db .delete(SCHEMA.environmentSelectorComputedResource) .where( eq( SCHEMA.environmentSelectorComputedResource.environmentId, environment.id, ), ); if (environment.resourceSelector == null) return; const resourceIds = await db .select({ resourceId: SCHEMA.resource.id }) .from(SCHEMA.resource) .where( and( eq(SCHEMA.resource.workspaceId, workspaceId), SCHEMA.resourceMatchesMetadata(db, environment.resourceSelector), ), ); const inserts = resourceIds.map((r) => ({ environmentId: environment.id, resourceId: r.resourceId, })); await db .insert(SCHEMA.environmentSelectorComputedResource) .values(inserts) .onConflictDoNothing(); + } catch (error) { + console.error(`Error computing environment selector resources for environment ${environment.id}:`, error); + throw error; + } };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/event-worker/src/workers/update-deployment.ts(1 hunks)apps/event-worker/src/workers/update-environment.ts(2 hunks)packages/db/src/resources/deployment-computed-resources.ts(1 hunks)packages/db/src/resources/env-computed-resources.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/event-worker/src/workers/update-environment.ts
- apps/event-worker/src/workers/update-deployment.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/resources/deployment-computed-resources.tspackages/db/src/resources/env-computed-resources.ts
🧬 Code Graph Analysis (1)
packages/db/src/resources/env-computed-resources.ts (2)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/schema/environment.ts (2)
environment(59-84)Environment(86-86)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Typecheck
- GitHub Check: Format
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (10)
packages/db/src/resources/deployment-computed-resources.ts (4)
3-5: Imports from local modules look good
The import statements fortakeFirst,Tx, and theSCHEMAindex are precise and align with the usage in this file.
17-21: Potential concurrency considerations
If multiple transactions call this function simultaneously for the same deployment, the process of deleting and then re-inserting could momentarily leave the table empty or cause race conditions. Verify synchronization needs or wrap this process in a transaction at the caller level to ensure consistency.
25-33: Verify the logic ofSCHEMA.resourceMatchesMetadata
The function usesSCHEMA.resourceMatchesMetadatato filter resources by metadata. Confirm that the criteria in this helper method correctly represent the desired matching logic and that all necessary keys are checked. If additional verification or custom logic is needed, update accordingly.
35-38: Straightforward record mapping
Creating insertion objects by mapping the found resource IDs to(deploymentId, resourceId)pairs is a clean and readable approach.packages/db/src/resources/env-computed-resources.ts (6)
6-9: Well-designed function signature with proper type definitions.The function signature is clear and uses appropriate types, making good use of TypeScript's
Pickutility to specify exactly which properties of theEnvironmenttype are needed.
10-15: Code correctly retrieves the workspace ID for the environment.The database query joins the necessary tables and uses
takeFirstappropriately to get a single result. This is a clean approach to get the required context for the resource computation.
26-26: Good early return pattern for null selector.The early return pattern when the resourceSelector is null is efficient and clear, avoiding unnecessary processing when there's nothing to compute.
43-47: Good use of onConflictDoNothing.Using
onConflictDoNothing()is a good approach to handle potential duplicate inserts without causing errors.
17-24:✅ Verification successful
Consider using a transaction for delete and insert operations.
The code deletes existing resources before computing new ones. This is a good pattern to ensure clean data, but make sure the calling code wraps this in a transaction to avoid potential data integrity issues if a failure occurs between delete and insert operations.
🏁 Script executed:
#!/bin/bash # Check if the function is called within transactions in other files rg -A 3 -B 3 "computeEnvironmentSelectorResources" --glob "*.ts"Length of output: 1636
Transaction already in use for delete/insert operations
The function
computeEnvironmentSelectorResourcesis consistently invoked within a transaction (e.g., as seen inapps/event-worker/src/workers/update-environment.ts), ensuring atomicity for the delete and insert operations. No further changes are needed at the moment.
28-36:❓ Verification inconclusive
Verify the behavior of resourceMatchesMetadata.
The code uses
SCHEMA.resourceMatchesMetadata(db, environment.resourceSelector)to filter resources, but the implementation of this function isn't visible in the current context. Make sure it handles all edge cases correctly, especially with complex selectors.
🏁 Script executed:
#!/bin/bash # Check the implementation of resourceMatchesMetadata rg -A 10 "resourceMatchesMetadata.*=" --glob "*.ts"Length of output: 53
Action Required: Manual Verification of
resourceMatchesMetadataImplementationIt appears that our initial search did not reveal the implementation details of
resourceMatchesMetadata. As a result, we cannot currently confirm whether it handles all edge cases (especially with complex selectors) as expected. Please perform a manual verification of the function to ensure its behavior is correct.
- Verify that
resourceMatchesMetadataproperly handles complex selectors and edge cases.- Confirm that its implementation (whether defined in another module or dynamically) is correct and appropriately tested.
To further assist, you may run the following broader search command:
#!/bin/bash # Search for any occurrences of resourceMatchesMetadata across the repository rg "resourceMatchesMetadata" -nThis should help locate its definition or any related comments for deeper context.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/event-worker/src/utils/upsert-release-targets.ts (2)
6-6: Unused import detected.The
takeFirstOrNullfunction is imported but never used in this file. Consider removing unused imports to keep the codebase clean.-import { and, eq, takeFirstOrNull } from "@ctrlplane/db"; +import { and, eq } from "@ctrlplane/db";
21-65: Consider adding explanatory comments.While the code is well-structured, adding comments explaining the purpose of the filtering logic would improve readability, especially for other developers who might not be familiar with the selector matching criteria.
const targets = rows .filter( (r) => + // Include targets where either: + // 1. The deployment has no resource selector (applies to all resources), or + // 2. The deployment has a matching computed resource for this specific resource r.deployment.resourceSelector == null || r.deployment_selector_computed_resource != null, )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/event-worker/src/utils/upsert-release-targets.ts(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- 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/event-worker/src/utils/upsert-release-targets.ts
🧬 Code Graph Analysis (1)
apps/event-worker/src/utils/upsert-release-targets.ts (4)
apps/event-worker/src/workers/updated-resources/span.ts (1)
withSpan(6-6)apps/event-worker/src/utils/spans.ts (1)
makeWithSpan(28-49)packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/schema/resource.ts (1)
resource(59-87)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Lint
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
🔇 Additional comments (3)
apps/event-worker/src/utils/upsert-release-targets.ts (3)
21-47: Good database query structure with appropriate joins.The query efficiently retrieves data by:
- Selecting from
environmentSelectorComputedResource- Inner joining with related tables
- Left joining with
deploymentSelectorComputedResource- Filtering by the resource ID
The structure is well-organized and handles the relationship between environments, deployments, and computed resources correctly.
49-59: Clear filtering logic with explicit mapping.The filtering logic properly handles the case where either:
- The deployment doesn't specify a resource selector, OR
- The deployment has a matching computed resource
The mapping to create targets with the necessary IDs is concise and appropriate.
61-65: Good optimization with early return.The early return when no targets are found avoids an unnecessary database operation. The values insertion with the mapped targets is clean and efficient.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/db/src/resources/env-computed-resources.ts (2)
31-47: Consider adding a comment explaining resource matching logicThe resource matching logic using
resourceMatchesMetadatais a key part of this function, but it might not be immediately clear how it works to future developers. Consider adding a brief comment explaining this matching process.
71-71: Consider consistent error message formatThe error message uses string interpolation for the workspace ID. Consider using a structured format that might be easier to parse in error handling systems.
- if (workspace == null) throw new Error(`Workspace not found: ${workspaceId}`); + if (workspace == null) throw new Error(`Workspace not found: ${workspaceId}`);This is a minor suggestion, but consistent error formatting can help with logging and monitoring.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/event-worker/src/utils/upsert-release-targets.ts(1 hunks)packages/db/src/resources/deployment-computed-resources.ts(1 hunks)packages/db/src/resources/env-computed-resources.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/src/resources/deployment-computed-resources.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.
apps/event-worker/src/utils/upsert-release-targets.tspackages/db/src/resources/env-computed-resources.ts
🧬 Code Graph Analysis (2)
apps/event-worker/src/utils/upsert-release-targets.ts (2)
packages/db/src/client.ts (1)
db(14-14)packages/db/src/schema/resource.ts (1)
resource(59-87)
packages/db/src/resources/env-computed-resources.ts (2)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/schema/environment.ts (2)
environment(59-84)Environment(86-86)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (8)
packages/db/src/resources/env-computed-resources.ts (4)
7-53: Well-structured function with clear purposeThe
computeEnvironmentSelectorResourcesfunction handles the computation of environment selector resources with a clean transaction-based approach. It properly retrieves workspace information, cleans up existing computed resources, and efficiently handles early returns for edge cases.
47-48: Optimize empty check before DB operationGood check for empty inserts to avoid unnecessary database operations.
55-77: Parallel processing with proper error handlingThe
recomputeAllEnvSelectorsInWorkspacefunction efficiently usesPromise.allfor parallel processing of environments and has appropriate error handling for missing workspaces.
59-70: Efficient query with nested relationshipsThe query efficiently retrieves all relevant environments with non-null resource selectors in a single database call, which is good for performance.
apps/event-worker/src/utils/upsert-release-targets.ts (4)
19-45: Complex join query implemented effectivelyThe join logic correctly connects environment computed resources with deployments, ensuring proper resource targeting. The query structure follows a logical progression from environments to deployments with appropriate conditions.
47-57: Clear filtering logic for target generationThe filtering logic correctly handles cases where either a deployment has no resource selector or has a matching computed resource. The mapping to target objects is concise and captures all necessary information.
59-64: Good optimization for empty targetsThe early return for empty targets avoids an unnecessary database operation, which is a good performance optimization.
33-42:✅ Verification successful
Verify deployment selector join conditions
The left join with
deploymentSelectorComputedResourceuses two conditions: matching the deployment ID and the resource ID. Ensure this is the intended behavior as it might filter out some deployments if the second condition is too restrictive.
🏁 Script executed:
#!/bin/bash # Verify if the join conditions for deploymentSelectorComputedResource are correctly implemented # by checking if there are any potential edge cases being missed # 1. Check the schema definition cat packages/db/src/schema/deployment.ts | grep -A 20 "deploymentSelectorComputedResource" # 2. Check if there are any other usages of this table with different join conditions rg "deploymentSelectorComputedResource" --type tsLength of output: 1795
Verified: Deployment Selector Join Conditions Are Correct
The left join in
apps/event-worker/src/utils/upsert-release-targets.tshas been inspected. The join conditions—matching the deployment ID and the resource ID—are consistent with the schema defined inpackages/db/src/schema/deployment.tsand align with other usages in the codebase (e.g., indeployment-computed-resources.ts). The composite primary key on these columns confirms that filtering by both fields is intentional. No adjustments are needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/db/src/selectors/compute/compute.ts (2)
1-4: Ensure imports are complete and properly arranged.The import statements look correct but seem incomplete. Consider adding imports for any operations you plan to use in the commented-out code blocks, such as
eqfrom Drizzle ORM, which appears to be needed for the commented query conditions.
1-63: Add type annotations and documentation to improve code clarity.Consider enhancing the code with:
- Return type annotations for methods
- JSDoc comments explaining the purpose of each class and method
- A brief file-level comment explaining the overall purpose of this module
This will improve maintainability and help future developers understand the code more easily.
Example for improved type annotations:
- async insert() { + async insert(): Promise<void | InsertResult> { const vals = await this.values(); if (vals.length === 0) return; return this.tx.insert(this.table).values(vals); } - resourceSelector() { + resourceSelector(): InsertBuilder<typeof resource> {Example for documentation:
/** * Handles building and executing computed resource operations for environment selectors. * This module provides classes to manage the relationship between resources and * environment/deployment selectors in the database. */ /** * Generic builder for database insert operations. * Only performs insert if values are present. */ class InsertBuilder<T extends PgTableWithColumns<any>> { // ... }packages/db/src/selectors/query/builder.ts (1)
10-10: Implement or remove the emptydeployments()method.
Leaving this method empty might lead to confusion. If it is not ready to be implemented, consider adding a TODO comment or removing it until you are ready to implement deployments-related query building.packages/db/src/selectors/query/environments-selector.ts (1)
17-88: Consider reducing repetitive logic inbuildMetadataCondition.
Eachifblock repeats the environment ID matching and key check, which could be unified or handled via a switch statement to reduce verbosity.Example of a possible switch-based approach for clarity:
- if (cond.operator === MetadataOperator.StartsWith) - return exists( ... ilike(environmentMetadata.value, `${cond.value}%`) ... ); - if (cond.operator === MetadataOperator.EndsWith) - return exists( ... ilike(environmentMetadata.value, `%${cond.value}`) ... ); + switch (cond.operator) { + case MetadataOperator.StartsWith: + return exists( + tx.select({ value: sql<number>`1` }) + .from(environmentMetadata) + .where( + and( + eq(environmentMetadata.environmentId, environment.id), + eq(environmentMetadata.key, cond.key), + ilike(environmentMetadata.value, `${cond.value}%`), + ), + ), + ); + // ... other cases ... + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/db/src/selectors/compute/compute.ts(1 hunks)packages/db/src/selectors/index.ts(1 hunks)packages/db/src/selectors/query/builder-types.ts(1 hunks)packages/db/src/selectors/query/builder.ts(1 hunks)packages/db/src/selectors/query/environments-selector.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/db/src/selectors/query/builder.tspackages/db/src/selectors/index.tspackages/db/src/selectors/query/builder-types.tspackages/db/src/selectors/query/environments-selector.tspackages/db/src/selectors/compute/compute.ts
🧬 Code Graph Analysis (4)
packages/db/src/selectors/query/builder.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/query/builder-types.ts (1)
WhereBuilder(20-29)packages/validators/src/environments/environment-condition.ts (1)
EnvironmentCondition(19-25)packages/db/src/selectors/query/environments-selector.ts (1)
EnvironmentOutputBuilder(120-131)
packages/db/src/selectors/index.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/compute/compute.ts (1)
ComputeBuilder(52-62)packages/db/src/selectors/query/builder.ts (1)
QueryBuilder(8-18)packages/db/src/client.ts (1)
db(14-14)
packages/db/src/selectors/query/environments-selector.ts (4)
packages/db/src/common.ts (2)
Tx(22-22)ColumnOperatorFn(54-65)packages/validators/src/conditions/metadata-condition.ts (1)
MetadataCondition(56-56)packages/db/src/schema/environment.ts (2)
environmentMetadata(123-134)environment(59-84)packages/validators/src/environments/environment-condition.ts (1)
EnvironmentCondition(19-25)
packages/db/src/selectors/compute/compute.ts (2)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/schema/resource.ts (1)
resource(59-87)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (8)
packages/db/src/selectors/compute/compute.ts (2)
6-18: LGTM! Generic insert builder is well-designed.The
InsertBuilderclass is well-structured with a generic type parameter that allows it to work with any table. The implementation avoids unnecessary database operations when there are no values to insert, which is efficient.
52-62: LGTM! Factory pattern implementation looks good.The
ComputeBuilderclass effectively serves as a factory for creating instances ofWorkspaceEnvironmentBuilderandEnvironmentBuilderwith the appropriate transaction context.packages/db/src/selectors/query/builder.ts (1)
12-17: Return type consistency check.
Theenvironments()method returns aWhereBuilderinstance, which is consistent with the approach of building queries. Everything else looks good for the new environments query builder.packages/db/src/selectors/index.ts (1)
6-16:❓ Verification inconclusive
Ensure clarity on the default transaction usage.
When notxis provided, the code falls back to the globaldb. Confirm that this fallback aligns with the desired transactional boundaries, especially when multiple operations share the same default context.Would you like to verify whether the fallback database usage introduces any unintentional shared state?
🏁 Script executed:
#!/bin/bash # This script searches for all calls to the `selector` function, # checking if they're passed with or without a transaction argument. rg -A 3 "selector\(.*\)"Length of output: 26
Verify Fallback Mechanism for Global DB Usage
The code in
packages/db/src/selectors/index.ts(lines 6–16) creates builder instances by passing atxinstance. The current design expects that when no explicittxis provided, a fallback to the globaldbis used. Although our initial search for calls to theselectorfunction did not yield any output, this result is inconclusive. Please manually verify that the fallback to the globaldbdoes not inadvertently introduce shared state between operations, especially in scenarios involving concurrent operations or operations spanning multiple transactional boundaries.
- Action Needed: Manually confirm that when
txis undefined, the fallback to the globaldbbehaves as intended without causing shared state issues.- Location for Attention:
packages/db/src/selectors/index.ts(lines 6–16).packages/db/src/selectors/query/builder-types.ts (2)
9-13: Solid union type forSelectorCondition.
The union type properly covers resource, version, deployment, and environment conditions. This is a good pattern for building flexible queries.
21-29: Clear separation of concerns withWhereBuilder.
Instantiating a new output builder via the provided class is straightforward. No issues found.packages/db/src/selectors/query/environments-selector.ts (2)
90-119: Comprehensive handling of environment conditions inbuildCondition.
The logic covers name, directory, system, metadata, and composite conditions, which should meet your varied querying needs. Nice job.
120-131:EnvironmentOutputBuilderintegrates well withWhereBuilder.
This class neatly wraps condition building while deferring tobuildCondition. It’s good that unspecified conditions returnundefined.
| class WorkspaceEnvironmentBuilder { | ||
| constructor( | ||
| private readonly tx: Tx, | ||
| private readonly workspaceId: string, | ||
| ) {} | ||
|
|
||
| resourceSelectors() { | ||
| return new InsertBuilder(this.tx, resource, async () => { | ||
| // return this.tx | ||
| // .select() | ||
| // .from(resource) | ||
| // .where(eq(resource.workspaceId, this.workspaceId)); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement or remove commented-out code in resourceSelectors().
Similar to the EnvironmentBuilder, the resourceSelectors() method contains commented-out code that doesn't return any values, causing the InsertBuilder to always receive an empty array. This means no inserts will ever be performed.
resourceSelectors() {
return new InsertBuilder(this.tx, resource, async () => {
- // return this.tx
- // .select()
- // .from(resource)
- // .where(eq(resource.workspaceId, this.workspaceId));
+ // TODO: Implement resource selection logic using this.workspaceId
+ return [];
});
}📝 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.
| class WorkspaceEnvironmentBuilder { | |
| constructor( | |
| private readonly tx: Tx, | |
| private readonly workspaceId: string, | |
| ) {} | |
| resourceSelectors() { | |
| return new InsertBuilder(this.tx, resource, async () => { | |
| // return this.tx | |
| // .select() | |
| // .from(resource) | |
| // .where(eq(resource.workspaceId, this.workspaceId)); | |
| }); | |
| } | |
| } | |
| class WorkspaceEnvironmentBuilder { | |
| constructor( | |
| private readonly tx: Tx, | |
| private readonly workspaceId: string, | |
| ) {} | |
| resourceSelectors() { | |
| return new InsertBuilder(this.tx, resource, async () => { | |
| // TODO: Implement resource selection logic using this.workspaceId | |
| return []; | |
| }); | |
| } | |
| } |
| class EnvironmentBuilder { | ||
| constructor( | ||
| private readonly tx: Tx, | ||
| private readonly ids: string[], | ||
| ) {} | ||
|
|
||
| resourceSelector() { | ||
| return new InsertBuilder(this.tx, resource, async () => { | ||
| // return this.tx | ||
| // .select() | ||
| // .from(resource) | ||
| // .where(eq(resource.workspaceId, this.workspaceId)); | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Implement or remove commented-out code in resourceSelector().
The resourceSelector() method contains commented-out code that doesn't return any values, causing the InsertBuilder to always receive an empty array. This means no inserts will ever be performed.
Additionally, the ids parameter in the constructor isn't used in any active code.
resourceSelector() {
return new InsertBuilder(this.tx, resource, async () => {
- // return this.tx
- // .select()
- // .from(resource)
- // .where(eq(resource.workspaceId, this.workspaceId));
+ // TODO: Implement resource selection logic using this.ids
+ return [];
});
}📝 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.
| class EnvironmentBuilder { | |
| constructor( | |
| private readonly tx: Tx, | |
| private readonly ids: string[], | |
| ) {} | |
| resourceSelector() { | |
| return new InsertBuilder(this.tx, resource, async () => { | |
| // return this.tx | |
| // .select() | |
| // .from(resource) | |
| // .where(eq(resource.workspaceId, this.workspaceId)); | |
| }); | |
| } | |
| } | |
| class EnvironmentBuilder { | |
| constructor( | |
| private readonly tx: Tx, | |
| private readonly ids: string[], | |
| ) {} | |
| resourceSelector() { | |
| return new InsertBuilder(this.tx, resource, async () => { | |
| // TODO: Implement resource selection logic using this.ids | |
| return []; | |
| }); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/db/src/selectors/query/builder.ts (1)
11-14: Unimplementeddeploymentsmethod.Currently, the
deployments()method is an empty placeholder. If a future implementation is planned, consider adding a TODO comment to clarify its purpose, or remove it if it’s not needed.packages/db/src/selectors/query/resource-selector.ts (2)
39-115: Consider refactoring repetitive subquery patterns inbuildMetadataCondition().Although the logic is correct, each metadata operator (StartsWith, EndsWith, Contains, etc.) uses a similar
exists(...)pattern. If more operators are introduced, you may want to unify or generalize these checks to reduce code repetition.
117-135: Unify date-based condition builders if desired.
buildCreatedAtCondition()andbuildLastSyncCondition()perform nearly identical comparisons on different columns (createdAtvs.updatedAt). Merging or sharing logic could help maintainability if more date-based fields are introduced.apps/event-worker/src/workers/update-deployment.ts (1)
1-17: Consider adding logging similar to the environment workerThe implementation is clean and follows the same pattern as the environment worker. However, unlike the environment worker, this one doesn't include any logging.
Consider adding some basic logging for better observability:
import _ from "lodash"; -import { selector } from "@ctrlplane/db"; +import { selector } from "@ctrlplane/db"; +import { logger } from "@ctrlplane/logger"; import { Channel, createWorker } from "@ctrlplane/events"; +const log = logger.child({ + module: "deployment-selector-update", + function: "updateDeploymentWorker", +}); export const updateDeploymentWorker = createWorker( Channel.UpdateDeployment, async ({ data }) => { const { oldSelector, resourceSelector } = data; - if (_.isEqual(oldSelector, resourceSelector)) return; + if (_.isEqual(oldSelector, resourceSelector)) { + log.info("No change in deployment selector", { deploymentId: data.id }); + return; + } + log.info("Computing resource selectors for deployment", { deploymentId: data.id }); await selector() .compute() .deployments([data.id]) .resourceSelectors() .replace(); + log.info("Completed computing resource selectors for deployment", { deploymentId: data.id }); }, );packages/db/src/selectors/compute/deployment-builder.ts (1)
17-58: Consider performance implications of large batch operations.
If thedeploymentsorresourcesarrays grow large, themapoperation (line 37) followed byPromise.all()(line 54) may cause high memory usage. You might consider chunking or paging through resources to avoid potential performance bottlenecks.packages/db/src/selectors/compute/environment-builder.ts (1)
17-58: Consider performance implications of large batch operations.
Similar toDeploymentBuilder, the combined size ofenvs(line 32) andresources(line 41) could become quite large. If so, you might encounter high memory usage in the mapped promises. Consider chunking or lazy-loading results for efficiency.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/event-worker/src/workers/update-deployment.ts(1 hunks)apps/event-worker/src/workers/update-environment.ts(2 hunks)apps/pty-proxy/src/controller/agent-socket.ts(2 hunks)apps/webservice/src/app/api/v1/resources/route.ts(2 hunks)packages/db/src/index.ts(1 hunks)packages/db/src/selectors/compute/compute.ts(1 hunks)packages/db/src/selectors/compute/deployment-builder.ts(1 hunks)packages/db/src/selectors/compute/environment-builder.ts(1 hunks)packages/db/src/selectors/compute/replace-builder.ts(1 hunks)packages/db/src/selectors/query/builder.ts(1 hunks)packages/db/src/selectors/query/resource-selector.ts(1 hunks)packages/job-dispatch/src/resource/handle-provider-scan.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/webservice/src/app/api/v1/resources/route.ts
- packages/db/src/index.ts
- apps/pty-proxy/src/controller/agent-socket.ts
- packages/job-dispatch/src/resource/handle-provider-scan.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.
apps/event-worker/src/workers/update-environment.tsapps/event-worker/src/workers/update-deployment.tspackages/db/src/selectors/compute/replace-builder.tspackages/db/src/selectors/query/builder.tspackages/db/src/selectors/compute/compute.tspackages/db/src/selectors/compute/deployment-builder.tspackages/db/src/selectors/compute/environment-builder.tspackages/db/src/selectors/query/resource-selector.ts
🧬 Code Graph Analysis (7)
apps/event-worker/src/workers/update-environment.ts (3)
packages/db/src/selectors/index.ts (1)
selector(18-18)packages/db/src/client.ts (1)
db(14-14)packages/db/src/schema/environment.ts (1)
environment(59-84)
apps/event-worker/src/workers/update-deployment.ts (2)
packages/events/src/index.ts (1)
createWorker(10-25)packages/db/src/selectors/index.ts (1)
selector(18-18)
packages/db/src/selectors/compute/replace-builder.ts (1)
packages/db/src/common.ts (1)
Tx(22-22)
packages/db/src/selectors/query/builder.ts (6)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/query/builder-types.ts (1)
WhereBuilder(20-29)packages/validators/src/environments/environment-condition.ts (1)
EnvironmentCondition(19-25)packages/db/src/selectors/query/environments-selector.ts (1)
EnvironmentOutputBuilder(120-131)packages/validators/src/resources/conditions/resource-condition.ts (1)
ResourceCondition(29-39)packages/db/src/selectors/query/resource-selector.ts (1)
ResourceOutputBuilder(165-176)
packages/db/src/selectors/compute/compute.ts (3)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/compute/environment-builder.ts (2)
WorkspaceEnvironmentBuilder(78-121)EnvironmentBuilder(8-59)packages/db/src/selectors/compute/deployment-builder.ts (2)
WorkspaceDeploymentBuilder(78-127)DeploymentBuilder(8-59)
packages/db/src/selectors/compute/environment-builder.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/query/builder.ts (2)
QueryBuilder(11-28)resources(22-27)packages/db/src/selectors/compute/replace-builder.ts (1)
ReplaceBuilder(6-26)packages/db/src/schema/workspace.ts (1)
workspace(18-27)
packages/db/src/selectors/query/resource-selector.ts (6)
packages/db/src/common.ts (2)
Tx(22-22)ColumnOperatorFn(54-65)packages/validators/src/conditions/metadata-condition.ts (1)
MetadataCondition(56-56)packages/db/src/schema/resource.ts (2)
resourceMetadata(155-166)resource(59-87)packages/validators/src/conditions/date-condition.ts (1)
CreatedAtCondition(37-37)packages/validators/src/resources/conditions/last-sync-condition.ts (1)
LastSyncCondition(11-11)packages/validators/src/resources/conditions/resource-condition.ts (1)
ResourceCondition(29-39)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: Lint
🔇 Additional comments (10)
packages/db/src/selectors/query/builder.ts (3)
1-9: Imports look good.The imported types and utilities (
EnvironmentCondition,ResourceCondition,Tx, etc.) are consistent with their usage in this file. No issues detected.
15-20:environmentsbuilder is well-structured.The
environments()method correctly returns aWhereBuilderspecialized forEnvironmentConditionandEnvironment. This approach keeps the query-building logic clean and consistent.
22-27:resourcesbuilder is coherent.Similar to
environments(), theresources()method cleanly returns aWhereBuilderforResourceConditionandResource. Implementation is straightforward and adheres to the established pattern.packages/db/src/selectors/query/resource-selector.ts (2)
137-163: Comprehensive condition handling.The
buildCondition()function covers a wide array of resource conditions (kind, name, provider, etc.). This thorough approach to combining multiple conditions via AND, OR, and optional negation looks well-structured, with no apparent logical errors.
165-176:ResourceOutputBuilderimplementation is sound.Using
buildCondition()inside thesql()method effectively delegates the condition-building logic. Returningundefinedfor null or empty conditions is a clean approach that can simplify upstream usage.apps/event-worker/src/workers/update-environment.ts (1)
203-207: LGTM: Good addition of resource selector computationThe addition of the selector computation step here ensures that environment resource selectors are up-to-date before proceeding with the resource matching logic. This is a logical enhancement that maintains the flow of the worker function.
packages/db/src/selectors/compute/replace-builder.ts (1)
1-26: Well-structured generic builder class for database operationsThis
ReplaceBuilderclass follows a good pattern for transaction management and provides a clean API for replacing records. The implementation correctly handles:
- Transactional operations
- Early return when no values are present
- Conflict handling with
onConflictDoNothingThe generic type parameter ensures type safety across different table operations.
packages/db/src/selectors/compute/compute.ts (1)
1-29: LGTM: Well-designed builder pattern implementationThe
ComputeBuilderclass provides a clean API for creating different types of builders. The implementation is:
- Consistent in its patterns
- Well-organized with clear separation of concerns
- Type-safe with proper imports
This builder pattern will make the code more maintainable and easier to use.
packages/db/src/selectors/compute/deployment-builder.ts (1)
61-76: Validate existence of systems ingetDeploymentsInWorkspace.
This function throws an error if theworkspaceis not found, but if the workspace is present with zero or mismatched systems, the result will simply be an empty array. If this is expected behavior, it’s fine; otherwise, consider validating thatworkspace.systemsis not empty to catch unexpected data inconsistencies.packages/db/src/selectors/compute/environment-builder.ts (1)
61-76: Validate zero-systems scenario ingetEnvsInWorkspace.
While an error is thrown if no workspace is found (line 74), having an existing workspace with no systems or no valid environments will produce an empty array (line 75). If this is intentional, no change is needed; otherwise, consider additional validation.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/db/src/selectors/compute/environment-builder.ts (1)
41-46:⚠️ Potential issueUse the local transaction parameter for query consistency
The resource query is using
this.txinstead of the localtxparameter passed to the callback. This could lead to transaction isolation issues.- const resources = await this.tx.query.resource.findMany({ + const resources = await tx.query.resource.findMany({ where: and( eq(SCHEMA.resource.workspaceId, workspaceId), this._queryBuilder.resources().where(env.resourceSelector).sql(), ), });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/db/src/selectors/compute/deployment-builder.ts(1 hunks)packages/db/src/selectors/compute/environment-builder.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/db/src/selectors/compute/deployment-builder.tspackages/db/src/selectors/compute/environment-builder.ts
🧬 Code Graph Analysis (2)
packages/db/src/selectors/compute/deployment-builder.ts (3)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/query/builder.ts (3)
QueryBuilder(11-28)deployments(13-13)resources(22-27)packages/db/src/selectors/compute/replace-builder.ts (1)
ReplaceBuilder(6-26)
packages/db/src/selectors/compute/environment-builder.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/query/builder.ts (2)
QueryBuilder(11-28)resources(22-27)packages/db/src/selectors/compute/replace-builder.ts (1)
ReplaceBuilder(6-26)packages/db/src/schema/workspace.ts (1)
workspace(18-27)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/amd64)
🔇 Additional comments (5)
packages/db/src/selectors/compute/deployment-builder.ts (3)
8-59: LGTM: Well-structured DeploymentBuilder implementationThis class provides a clean implementation for managing deployment resource selectors. The transaction context is properly maintained throughout the operations.
61-76: LGTM: Well-implemented helper functionThe
getDeploymentsInWorkspacefunction properly fetches deployments with non-null resource selectors from a specified workspace, with appropriate error handling when the workspace isn't found.
78-127: LGTM: WorkspaceDeploymentBuilder is properly implementedThe WorkspaceDeploymentBuilder class provides workspace-scoped deployment resource selector computation. Nice job consistently using the local transaction parameter (
tx) in the callbacks to ensure proper transaction scoping.packages/db/src/selectors/compute/environment-builder.ts (2)
61-76: LGTM: Well-implemented helper functionThe
getEnvsInWorkspacefunction correctly fetches environments with non-null resource selectors from a specified workspace and includes appropriate error handling.
78-121: LGTM: WorkspaceEnvironmentBuilder correctly uses the transaction parameterThe WorkspaceEnvironmentBuilder implementation correctly uses the local transaction parameter in all database operations, ensuring proper transaction isolation.
| export class EnvironmentBuilder { | ||
| private readonly _queryBuilder; | ||
| constructor( | ||
| private readonly tx: Tx, | ||
| private readonly ids: string[], | ||
| ) { | ||
| this._queryBuilder = new QueryBuilder(tx); | ||
| } | ||
|
|
||
| resourceSelectors() { | ||
| return new ReplaceBuilder( | ||
| this.tx, | ||
| SCHEMA.environmentSelectorComputedResource, | ||
| async (tx) => { | ||
| await tx | ||
| .delete(SCHEMA.environmentSelectorComputedResource) | ||
| .where( | ||
| inArray( | ||
| SCHEMA.environmentSelectorComputedResource.environmentId, | ||
| this.ids, | ||
| ), | ||
| ); | ||
| }, | ||
| async (tx) => { | ||
| const envs = await tx.query.environment.findMany({ | ||
| where: inArray(SCHEMA.environment.id, this.ids), | ||
| with: { system: true }, | ||
| }); | ||
|
|
||
| const promises = envs.map(async (env) => { | ||
| const { system } = env; | ||
| const { workspaceId } = system; | ||
|
|
||
| const resources = await this.tx.query.resource.findMany({ | ||
| where: and( | ||
| eq(SCHEMA.resource.workspaceId, workspaceId), | ||
| this._queryBuilder.resources().where(env.resourceSelector).sql(), | ||
| ), | ||
| }); | ||
|
|
||
| return resources.map((r) => ({ | ||
| environmentId: env.id, | ||
| resourceId: r.id, | ||
| })); | ||
| }); | ||
|
|
||
| const fulfilled = await Promise.all(promises); | ||
| return fulfilled.flat(); | ||
| }, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Overall implementation of EnvironmentBuilder is sound
Apart from the transaction inconsistency noted above, the class is well-structured and follows a clean pattern for computing resource selectors for environments.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/db/src/selectors/compute/environment-builder.ts (2)
17-55:⚠️ Potential issueThe EnvironmentBuilder's resourceSelectors method looks well-structured but has a transaction inconsistency.
The
resourceSelectorsmethod properly implements a pattern for computing and replacing resource selectors for environments. However, there's an issue with transaction scope.On line 38, you're using
this.txrather than the local transaction parametertxthat was passed to the callback:- const resources = await this.tx.query.resource.findMany({ + const resources = await tx.query.resource.findMany({ where: and( eq(SCHEMA.resource.workspaceId, workspaceId), this._queryBuilder.resources().where(env.resourceSelector).sql(), ), });This inconsistency can cause transaction isolation issues since other operations in this method use the local transaction parameter.
84-117:⚠️ Potential issueWorkspaceEnvironmentBuilder's resourceSelectors has the same transaction inconsistency issue.
Similar to the first builder class, the resourceSelectors method has a transaction inconsistency issue.
On line 103, you're using
this._queryBuilder(initialized withthis.tx) rather than the local transaction parameter:- this._queryBuilder.resources().where(env.resourceSelector).sql(), + new QueryBuilder(tx).resources().where(env.resourceSelector).sql(),Both builder classes have this same issue, which breaks proper transaction isolation. All database operations within the callback should use the same transaction context.
🧹 Nitpick comments (1)
packages/db/drizzle/0090_organic_sinister_six.sql (1)
1-14: Table schema looks good but consider creating an index for query performance.The creation of the computed resource selector tables with composite primary keys is well-structured. The dropping of the old tables indicates this is a replacement of an existing feature.
Consider adding an index on the foreign key columns to improve query performance, especially if you'll be frequently querying by just the resource_id:
CREATE TABLE IF NOT EXISTS "computed_deployment_resource" ( "deployment_id" uuid NOT NULL, "resource_id" uuid NOT NULL, CONSTRAINT "computed_deployment_resource_deployment_id_resource_id_pk" PRIMARY KEY("deployment_id","resource_id") ); +CREATE INDEX IF NOT EXISTS "computed_deployment_resource_resource_id_idx" ON "computed_deployment_resource" ("resource_id"); CREATE TABLE IF NOT EXISTS "computed_environment_resource" ( "environment_id" uuid NOT NULL, "resource_id" uuid NOT NULL, CONSTRAINT "computed_environment_resource_environment_id_resource_id_pk" PRIMARY KEY("environment_id","resource_id") ); +CREATE INDEX IF NOT EXISTS "computed_environment_resource_resource_id_idx" ON "computed_environment_resource" ("resource_id");
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/event-worker/src/utils/upsert-release-targets.ts(1 hunks)packages/db/drizzle/0090_organic_sinister_six.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/deployment.ts(3 hunks)packages/db/src/schema/environment.ts(3 hunks)packages/db/src/selectors/compute/deployment-builder.ts(1 hunks)packages/db/src/selectors/compute/environment-builder.ts(1 hunks)packages/db/src/selectors/query/builder.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/db/drizzle/meta/_journal.json
- packages/db/src/selectors/query/builder.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{ts,tsx}`: **Note on Error Handling:** Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error...
**/*.{ts,tsx}: Note on Error Handling:
Avoid strict enforcement of try/catch blocks. Code may use early returns, Promise chains (.then().catch()), or other patterns for error handling. These are acceptable as long as they maintain clarity and predictability.
packages/db/src/schema/deployment.tsapps/event-worker/src/utils/upsert-release-targets.tspackages/db/src/schema/environment.tspackages/db/src/selectors/compute/deployment-builder.tspackages/db/src/selectors/compute/environment-builder.ts
🧬 Code Graph Analysis (5)
packages/db/src/schema/deployment.ts (1)
packages/db/src/schema/resource.ts (1)
resource(59-87)
apps/event-worker/src/utils/upsert-release-targets.ts (2)
packages/db/src/client.ts (1)
db(14-14)packages/db/src/schema/resource.ts (1)
resource(59-87)
packages/db/src/schema/environment.ts (1)
packages/db/src/schema/resource.ts (1)
resource(59-87)
packages/db/src/selectors/compute/deployment-builder.ts (5)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/query/builder.ts (3)
QueryBuilder(10-27)deployments(12-12)resources(21-26)packages/db/src/selectors/compute/replace-builder.ts (1)
ReplaceBuilder(6-26)packages/db/src/selectors/compute/compute.ts (1)
deployments(26-28)packages/db/src/schema/workspace.ts (1)
workspace(18-27)
packages/db/src/selectors/compute/environment-builder.ts (4)
packages/db/src/common.ts (1)
Tx(22-22)packages/db/src/selectors/query/builder.ts (2)
QueryBuilder(10-27)resources(21-26)packages/db/src/selectors/compute/replace-builder.ts (1)
ReplaceBuilder(6-26)packages/db/src/schema/workspace.ts (1)
workspace(18-27)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (linux/amd64)
- GitHub Check: Typecheck
- GitHub Check: build (linux/amd64)
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
🔇 Additional comments (10)
packages/db/src/schema/environment.ts (1)
136-147: The composite primary key implementation looks good.The
computedEnvironmentResourcetable structure is well-defined with appropriate foreign key references and cascading delete behavior. Using a composite primary key on both columns is an excellent approach for this many-to-many relationship.packages/db/src/schema/deployment.ts (1)
117-128: Proper implementation of the computed relationship table.The
computedDeploymentResourcetable follows the same pattern as the environment counterpart, with appropriate foreign key references and a composite primary key. This consistent approach will ensure referential integrity and efficient query patterns.apps/event-worker/src/utils/upsert-release-targets.ts (1)
19-57: Query optimization looks great!The refactored implementation leverages the new computed relationship tables to simplify the resource matching process. This approach is more efficient as it:
- Eliminates the need for complex client-side filtering
- Pushes the filtering logic to the database layer
- Reduces the amount of data transferred between DB and application
The query structure with appropriate inner and left joins shows good understanding of the data relationships.
packages/db/src/selectors/compute/deployment-builder.ts (3)
8-56: Well-structured builder pattern for deployment resources.The
DeploymentBuilderclass effectively encapsulates the logic for managing computed resources for specific deployments. The transaction handling and resource selection approach are appropriate. The pattern of using aReplaceBuilderfor the atomic delete-then-insert operation ensures data consistency.
58-73: Good separation of concerns with the workspace deployment helper.The
getDeploymentsInWorkspacefunction provides a clean abstraction for fetching deployments within a workspace context. The error handling for non-existent workspaces is appropriate, ensuring that callers receive clear feedback.
75-124: Consistent transaction handling in workspace deployment builder.The
WorkspaceDeploymentBuilderimplementation follows the same pattern as theDeploymentBuilder, maintaining consistency across the codebase. Good job on using the passed transaction object consistently in both callbacks, which ensures proper transactional integrity.packages/db/src/selectors/compute/environment-builder.ts (2)
58-73: Helper function is well-implemented and includes proper error handling.The
getEnvsInWorkspacehelper function properly:
- Queries for the workspace with its related systems and environments
- Filters for environments with non-null resource selectors
- Includes appropriate error handling when a workspace isn't found
75-83: WorkspaceEnvironmentBuilder's constructor is well-designed.The constructor properly initializes the query builder and stores the necessary parameters.
packages/db/drizzle/0090_organic_sinister_six.sql (2)
15-37: Well-structured foreign key constraints with proper cascade behavior.The foreign key constraints are properly defined with:
- CASCADE delete behavior - ensures referential integrity when parent records are deleted
- Exception handling for duplicate objects - makes the migration script idempotent
The exception handling is particularly important in migration scripts as it allows the script to be run multiple times without errors, which is useful in development and deployment scenarios.
1-37:❓ Verification inconclusive
Verify data migration strategy between old and new tables.
You're dropping existing tables (
deployment_selector_computed_resourceandenvironment_selector_computed_resource) and creating new ones with similar names. Make sure you have a strategy to migrate any existing data or recompute it.
🏁 Script executed:
#!/bin/bash # Check if the old tables had records that need migration echo "Checking how many records might need migration..." cat << 'EOF' > check_tables.sql SELECT 'deployment_selector_computed_resource' as table_name, count(*) FROM information_schema.tables WHERE table_name = 'deployment_selector_computed_resource'; SELECT 'environment_selector_computed_resource' as table_name, count(*) FROM information_schema.tables WHERE table_name = 'environment_selector_computed_resource'; EOF # This would need to be run in a PostgreSQL environment echo "Note: This script needs to be run in an environment with PostgreSQL access" echo "In an actual production environment, you'd need to run this script against your database."Length of output: 465
Data Migration Strategy Verification
The script output confirms that the checks were only echoed rather than executed against an actual PostgreSQL instance, so there's no evidence that a data migration is occurring. Since the code is dropping the old tables (
deployment_selector_computed_resourceandenvironment_selector_computed_resource) and creating new ones without an embedded migration step, please ensure you have a robust plan in place to migrate or recompute any data from the old tables.
- Verify that a data migration procedure is implemented (or scheduled) during the deployment process.
- Confirm that records from the old tables are either preserved or recomputed in the new table structures in a production environment.
- Update the migration documentation if necessary.
Summary by CodeRabbit
New Features
Refactor
Chores