feat: init computed resource selector tables#485
feat: init computed resource selector tables#485adityachoudhari26 merged 2 commits intoselector-computed-resourcesfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes simplify the release target upsert process by removing a complex helper function in favor of a direct query. New worker functions—most notably for deployment updates—have been introduced, along with modifications to environment worker logic. Resource update handlers in both the proxy and web service now trigger concurrent recomputation of deployment and environment selectors. The database schema and migration scripts have been extended with new tables for computed resources, and additional selector-builder modules have been added. Type definitions and job dispatch logic were also updated to support these enhancements. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Channel.UpdateDeployment
participant UD as updateDeploymentWorker
participant DB as Database
C->>UD: Trigger update (oldSelector, resourceSelector)
alt Selectors are Equal
UD-->>C: Exit without action
else Selectors differ
UD->>DB: computeDeploymentSelectorResources(...)
DB-->>UD: Resources updated
end
sequenceDiagram
participant RU as Resource Updater
participant DB as Database
RU->>RU: Upsert resource
RU->>DB: recomputeAllDeploymentSelectorsInWorkspace(tx, workspaceId)
RU->>DB: recomputeAllEnvSelectorsInWorkspace(tx, workspaceId)
DB-->>RU: Both recomputations complete
Poem
🪧 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
|
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🧹 Nitpick comments (14)
apps/event-worker/src/workers/update-deployment.ts (1)
10-11: Consider adding logging for debugging purposes.While the current implementation works well, adding a simple log statement (either on early return or when processing changes) would improve observability and debugging experience.
async ({ data }) => { const { oldSelector, resourceSelector } = data; - if (_.isEqual(oldSelector, resourceSelector)) return; + if (_.isEqual(oldSelector, resourceSelector)) { + logger.debug('Skipping deployment update - selectors unchanged'); + return; + } + logger.info('Updating deployment selector resources', { deploymentId: data.id }); await computeDeploymentSelectorResources(db, data); },apps/webservice/src/app/api/v1/resources/route.ts (1)
81-84: Consider adding error handling for the recomputation operations.If one of the recomputation functions fails, it will cause the entire request to fail. Consider adding specific error handling to make the endpoint more robust.
await Promise.all([ - recomputeAllEnvSelectorsInWorkspace(db, workspaceId), - recomputeAllDeploymentSelectorsInWorkspace(db, workspaceId), + recomputeAllEnvSelectorsInWorkspace(db, workspaceId).catch(err => { + // Log error but continue processing + console.error('Failed to recompute environment selectors:', err); + }), + recomputeAllDeploymentSelectorsInWorkspace(db, workspaceId).catch(err => { + console.error('Failed to recompute deployment selectors:', err); + }), ]);packages/job-dispatch/src/resource/handle-provider-scan.ts (1)
51-54: Consider adding specific error handling for the recomputation steps.Since this code is inside a try-catch block, any error in recomputation will be caught, but it will also fail the entire resource processing. Consider adding specific error handling to make this operation more resilient.
await Promise.all([ - recomputeAllEnvSelectorsInWorkspace(tx, workspaceId), - recomputeAllDeploymentSelectorsInWorkspace(tx, workspaceId), + recomputeAllEnvSelectorsInWorkspace(tx, workspaceId).catch(err => { + log.error('Failed to recompute environment selectors', { error: err }); + // Continue processing despite this error + }), + recomputeAllDeploymentSelectorsInWorkspace(tx, workspaceId).catch(err => { + log.error('Failed to recompute deployment selectors', { error: err }); + // Continue processing despite this error + }), ]);packages/db/src/selectors/index.ts (1)
1-18: Consider adding documentation for the selector builder.While the code is well-structured, adding JSDoc comments would help explain the purpose and usage of this selector builder to other developers.
+/** + * Provides a builder pattern for constructing database selector operations. + * This allows for fluent interfaces when working with computed resources. + */ class InitialBuilder { constructor(private readonly tx: Tx) {} + /** + * Returns a builder for computing selector resources. + */ compute() { return new ComputeBuilder(this.tx); } + /** + * Returns a builder for querying selector resources. + */ query() { return new QueryBuilder(this.tx); } } +/** + * Factory function that creates a selector builder. + * @param tx Optional transaction object. If not provided, uses the global db instance. + * @returns An InitialBuilder instance configured with the provided transaction or global db. + */ export const selector = (tx?: Tx) => new InitialBuilder(tx ?? db);packages/db/src/selectors/query/builder-types.ts (1)
15-18: Consider removing the unused type variable ESLint directive.There appears to be an ESLint directive to disable the unused type variable warning. If the generic type
Tis actually used within the interface, this directive may be unnecessary.-// eslint-disable-next-line @typescript-eslint/no-unused-vars export interface OutputBuilder<T extends object> { sql(): SQL<unknown> | undefined; }packages/db/src/resources/deployment-computed-resources.ts (1)
59-77: Consider limiting concurrency when processing large batches.Invoking
computeDeploymentSelectorResourcesfor all deployments in parallel might overwhelm the database if the workspace contains a large number of deployments. Consider bounded concurrency (e.g., usingPromise.allSettledor a pool-based approach).packages/db/src/resources/env-computed-resources.ts (1)
59-77: Consider limiting concurrency for mass environment recomputation.When calling
computeEnvironmentSelectorResourcesin parallel for all environments withPromise.all, it may cause significant load if the workspace has many environments. An approach with batched or pooled concurrency could help.packages/db/src/selectors/query/environments-selector.ts (2)
90-119: Robust handling of different environment condition types.This function cleanly combines conditions with logical operators
AND/OR. Just ensure that all unioned condition types are covered if more condition variants might be added in the future.
120-131: Optionally add documentation for the builder class.Providing a short docstring for
EnvironmentOutputBuilderwould help new contributors understand how the class is intended to be used and how the condition is transformed into SQL.apps/event-worker/src/utils/upsert-release-targets.ts (2)
19-45: Add documentation for complex DB query logicThis is a complex query with multiple joins that's critical to the release target upsert process. Consider adding comments to explain the logic behind the joins and what each part of the query is achieving to improve maintainability.
47-57: Consider adding error handlingThe filtering and mapping logic handles the query results but lacks error handling. If the query returns unexpected data structures, it might lead to runtime errors. Consider adding try/catch blocks or data validation to handle potential failures gracefully.
packages/db/src/selectors/compute/compute.ts (3)
18-32: Unused parameter and inconsistent namingThe
idsparameter inEnvironmentBuilderis not being used in the implementation. Additionally, consider maintaining consistency in method naming betweenresourceSelector(singular) in this class andresourceSelectors(plural) inWorkspaceEnvironmentBuilder.
6-16: Improve type safety and add documentationThe
InsertBuilderusesany[]for values, which reduces type safety. Consider using a generic type parameter for the values array. Also, adding JSDoc comments would clarify the purpose and usage of this class./** * Generic builder for constructing and executing insert operations * @template T The table type * @template V The values type */ -class InsertBuilder<T extends PgTableWithColumns<any>> { +class InsertBuilder<T extends PgTableWithColumns<any>, V = any> { constructor( private readonly tx: Tx, private readonly table: T, - private readonly values: () => Promise<any[]>, + private readonly values: () => Promise<V[]>, ) {} async insert() { return this.tx.insert(this.table).values(await this.values()); } }
50-60: Add documentation for ComputeBuilder classThe
ComputeBuilderclass lacks documentation explaining its purpose and how it's intended to be used. Consider adding JSDoc comments to make the API more self-explanatory.+/** + * Factory class for creating environment builders + * Facilitates computing various resource selectors + */ export class ComputeBuilder { constructor(private readonly tx: Tx) {} + /** + * Creates a builder for all environments in a workspace + * @param workspaceId The ID of the workspace + */ allEnvironments(workspaceId: string) { return new WorkspaceEnvironmentBuilder(this.tx, workspaceId); } + /** + * Creates a builder for specific environments by ID + * @param ids Array of environment IDs + */ environments(ids: string[]) { return new EnvironmentBuilder(this.tx, ids); } }
🛑 Comments failed to post (5)
packages/db/src/selectors/query/builder.ts (1)
8-18:
⚠️ Potential issueEmpty
deployments()method needs implementation.The
deployments()method is currently empty while theenvironments()method is implemented. This suggests that deployment query building functionality is planned but not yet implemented.Considering that the PR includes deployment computed resources, the empty method should either be implemented or have a TODO comment explaining when it will be implemented:
deployments() {} + // TODO: Implement deployment query builder similar to environmentsAlternatively, if this is intentional and will be implemented in a follow-up PR, consider adding a comment explaining this decision.
📝 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.export class QueryBuilder { constructor(private readonly tx: Tx) {} deployments() {} // TODO: Implement deployment query builder similar to environments environments() { return new WhereBuilder<EnvironmentCondition, Environment>( this.tx, EnvironmentOutputBuilder, ); } }packages/db/src/resources/deployment-computed-resources.ts (1)
11-20: 🛠️ Refactor suggestion
Handle the scenario of no matching deployment record.
If
takeFirstreturnsundefined, destructuringworkspaceIdwill throw a runtime error. Consider explicitly checking for a returned row.Here's a minimal example:
const row = await db .select({ workspaceId: SCHEMA.system.workspaceId }) .from(SCHEMA.deployment) .innerJoin(SCHEMA.system, eq(SCHEMA.deployment.systemId, SCHEMA.system.id)) .where(eq(SCHEMA.deployment.id, deployment.id)) .then(takeFirst); + if (!row) { + throw new Error(`Deployment not found: ${deployment.id}`); + } const { workspaceId } = row;📝 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.db.transaction(async (db) => { - const { workspaceId } = await db - .select({ workspaceId: SCHEMA.system.workspaceId }) - .from(SCHEMA.deployment) - .innerJoin( - SCHEMA.system, - eq(SCHEMA.deployment.systemId, SCHEMA.system.id), - ) - .where(eq(SCHEMA.deployment.id, deployment.id)) - .then(takeFirst); + const row = await db + .select({ workspaceId: SCHEMA.system.workspaceId }) + .from(SCHEMA.deployment) + .innerJoin(SCHEMA.system, eq(SCHEMA.deployment.systemId, SCHEMA.system.id)) + .where(eq(SCHEMA.deployment.id, deployment.id)) + .then(takeFirst); + + if (!row) { + throw new Error(`Deployment not found: ${deployment.id}`); + } + + const { workspaceId } = row;packages/db/src/resources/env-computed-resources.ts (1)
12-20: 🛠️ Refactor suggestion
Handle the scenario of no matching environment record.
If
takeFirstreturnsundefined, destructuringworkspaceIdwill throw a runtime error. Consider explicitly checking for a returned row.A possible example:
const row = 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); + if (!row) { + throw new Error(`Environment not found: ${environment.id}`); + } const { workspaceId } = row;📝 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.const row = 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); if (!row) { throw new Error(`Environment not found: ${environment.id}`); } const { workspaceId } = row;packages/db/src/selectors/compute/compute.ts (2)
24-31:
⚠️ Potential issueImplement the commented-out resource selector logic
The
resourceSelectormethod contains commented-out code and doesn't return any values, which will cause theinsert()method to fail when callingawait this.values(). Either implement the resource selection logic or throw a "Not implemented" error until it's ready.resourceSelector() { return new InsertBuilder(this.tx, resource, async () => { - // return this.tx - // .select() - // .from(resource) - // .where(eq(resource.workspaceId, this.workspaceId)); + throw new Error("Resource selector implementation not yet available"); + // TODO: Implement resource selection logic }); }📝 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.resourceSelector() { return new InsertBuilder(this.tx, resource, async () => { throw new Error("Resource selector implementation not yet available"); // TODO: Implement resource selection logic }); }
40-47:
⚠️ Potential issueImplement the commented-out resource selectors logic
Similar to the
resourceSelectormethod inEnvironmentBuilder, this method returns anInsertBuilderwith a values function that doesn't return anything, which will cause runtime errors wheninsert()is called.resourceSelectors() { return new InsertBuilder(this.tx, resource, async () => { - // return this.tx - // .select() - // .from(resource) - // .where(eq(resource.workspaceId, this.workspaceId)); + throw new Error("Resource selectors implementation not yet available"); + // TODO: Implement resource selection logic for workspace }); }📝 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.resourceSelectors() { return new InsertBuilder(this.tx, resource, async () => { throw new Error("Resource selectors implementation not yet available"); // TODO: Implement resource selection logic for workspace }); }
1ced035
into
selector-computed-resources
Summary by CodeRabbit
New Features
Refactor
Chores