fix: Soft delete resources #223
Conversation
WalkthroughThis pull request introduces significant changes across multiple files in the codebase, primarily focused on enhancing the logic for filtering resources based on their deletion status. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 7
🧹 Outside diff range and nitpick comments (21)
packages/job-dispatch/src/events/triggers/resource-deleted.ts (1)
Line range hint
15-19: Consider improving function name or documentationWhile the implementation is correct, the function name
getEventsForResourceDeletedmight be misleading since it actually needs to operate on non-deleted resources. Consider either:
- Renaming to
getEventsForResourceBeforeDeletion, or- Enhancing the documentation to more prominently explain the timing requirement
/** * Get events for a resource that has been deleted. - * NOTE: Because we may need to do inner joins on resource metadata for the filter, - * this actually needs to be called before the resource is actually deleted. + * IMPORTANT: This function MUST be called BEFORE the resource is deleted because + * it needs to perform inner joins on resource metadata for the filter. * @param resource */apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts (1)
Line range hint
58-77: Consider adding transaction support for relationship creationWhile the current implementation correctly handles soft-deleted resources, there's a potential race condition where resources could be soft-deleted between the existence check and relationship creation. Consider wrapping the checks and insert in a transaction.
Example implementation:
await db.transaction(async (tx) => { const fromResource = await tx .select() .from(SCHEMA.resource) .where(/* ... */); if (!fromResource) throw new Error(`${body.fromIdentifier} not found`); const toResource = await tx .select() .from(SCHEMA.resource) .where(/* ... */); if (!toResource) throw new Error(`${body.toIdentifier} not found`); await tx.insert(SCHEMA.resourceRelationship).values({ sourceId: fromResource.id, targetId: toResource.id, type: body.type, }); });packages/job-dispatch/src/events/triggers/environment-deleted.ts (1)
55-55: LGTM: Consider adding tests for soft delete scenariosThe soft delete filtering is correctly implemented in the
removedFromSystemResourcesquery. To ensure reliability, consider adding tests that verify:
- Resources marked as deleted are properly filtered out
- The combination of filters (metadata matching + soft delete) works as expected
Would you like me to help create test cases for these scenarios?
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts (1)
Line range hint
46-52: Maintain consistent terminology in error messages and variable names.The variable has been renamed from
targettoresourcein some places, but the error message and response still use "target".if (data == null) return NextResponse.json( - { error: "Target not found" }, + { error: "Resource not found" }, { status: 404 }, ); - const { metadata, ...target } = data; + const { metadata, ...resource } = data; return NextResponse.json({ - ...target, + ...resource, metadata: Object.fromEntries(metadata.map((t) => [t.key, t.value])), });apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (1)
Line range hint
143-167: Add soft delete check to PATCH handler for consistencyThe PATCH handler lacks the soft delete filtering that was added to the GET handler. This inconsistency could allow updates to jobs associated with deleted resources, while preventing their retrieval.
Consider adding the same check to the PATCH handler:
const je = await db .update(job) .set(body) - .where(and(eq(job.id, params.jobId))) + .where( + and( + eq(job.id, params.jobId), + isNull(resource.deletedAt) + ) + ) .returning() .then(takeFirstOrNull);Note: This requires joining with the resource table first:
const je = await db .update(job) + .leftJoin(releaseJobTrigger, eq(releaseJobTrigger.jobId, job.id)) + .leftJoin(resource, eq(resource.id, releaseJobTrigger.resourceId)) .set(body)packages/api/src/router/release-deploy.ts (1)
136-137: Consider improving the error message for clarityThe current error message "Resource not found" doesn't distinguish between non-existent and soft-deleted resources. This could be confusing for API consumers.
Consider this improvement:
- if (!t) throw new Error("Resource not found"); + if (!t) throw new Error("Resource not found or has been deleted");packages/api/src/router/workspace.ts (1)
233-233: LGTM! Consider adding an index for query performance.The soft delete implementation looks good. The
isNull(resource.deletedAt)condition is correctly combined with the workspace filter.Consider adding an index on
(workspaceId, deletedAt)to optimize the query performance, as these columns are frequently used together in the WHERE clause.packages/api/src/router/target-provider.ts (2)
227-228: Soft delete implementation looks good, but consider cascade implications.The change from hard delete to soft delete for resources is implemented correctly. However, there are some architectural considerations to note.
Consider standardizing the deletion approach across the system:
- The resources are soft deleted but the provider is hard deleted
- This mixed approach might lead to data consistency issues if the provider needs to be restored
- Consider implementing soft delete for the provider as well to maintain consistency
Line range hint
236-238: Address the race condition with in-progress scans.The comment raises a valid concern about in-progress scans during provider deletion.
Consider implementing one of these approaches:
- Wait for in-progress scans to complete before deletion
- Mark the provider as "pending deletion" and handle cleanup in a background job
- Forcefully terminate in-progress scans
Here's a suggested implementation for approach #2:
.mutation(async ({ ctx, input }) => ctx.db.transaction(async (tx) => { + // Mark provider as pending deletion + await tx + .update(resourceProvider) + .set({ status: 'PENDING_DELETION' }) + .where(eq(resourceProvider.id, input.providerId)); + if (input.deleteResources) await tx .update(resource) .set({ deletedAt: new Date() }) .where(eq(resource.providerId, input.providerId)); - const deletedProvider = await tx - .delete(resourceProvider) - .where(eq(resourceProvider.id, input.providerId)) - .returning() - .then(takeFirst); + // Queue cleanup job + await cleanupQueue.add( + `cleanup-provider-${input.providerId}`, + { providerId: input.providerId }, + { delay: 5000 } // 5s delay to allow in-progress scans to complete + ); await targetScanQueue.remove(input.providerId); - return deletedProvider; + return { id: input.providerId, status: 'PENDING_DELETION' }; }),packages/db/src/schema/resource.ts (2)
Line range hint
242-242: Add soft delete filter to resource query builder.The
resourceMatchesMetadatafunction needs to account for soft deleted resources to ensure consistency with the new soft delete functionality.Consider adding this filter:
export function resourceMatchesMetadata( tx: Tx, metadata?: ResourceCondition | null, ): SQL<unknown> | undefined { + const baseCondition = eq(resource.deletedAt, null); return metadata == null || Object.keys(metadata).length === 0 - ? undefined - : buildCondition(tx, metadata); + ? baseCondition + : and(baseCondition, buildCondition(tx, metadata)); }This ensures that soft-deleted resources are consistently excluded from all queries using this helper function.
Also applies to: 243-243, 244-244, 245-245, 246-246, 247-247, 248-248, 249-249
Line range hint
1-249: Consider adding helper functions for soft delete operations.To ensure consistent handling of soft deletes across the codebase, consider adding helper functions for soft delete operations.
Consider adding these utility functions:
export function softDelete(tx: Tx, resourceId: string) { return tx .update(resource) .set({ deletedAt: new Date() }) .where(eq(resource.id, resourceId)); } export function restore(tx: Tx, resourceId: string) { return tx .update(resource) .set({ deletedAt: null }) .where(eq(resource.id, resourceId)); } export function isDeleted(tx: Tx, resourceId: string) { return tx .select({ deletedAt: resource.deletedAt }) .from(resource) .where(eq(resource.id, resourceId)) .then((result) => result[0]?.deletedAt != null); }packages/auth/src/utils/rbac.ts (2)
195-203: Consider adding error handling for database operationsThe function should handle potential database errors to prevent uncaught exceptions from propagating up the call stack.
const getResourceScopes = async (id: string) => { + try { const result = await db .select() .from(workspace) .innerJoin(resource, eq(resource.workspaceId, workspace.id)) .where(and(eq(resource.id, id), isNull(resource.deletedAt))) .then(takeFirstOrNull); if (result == null) return []; return [ { type: "resource" as const, id: result.resource.id }, { type: "workspace" as const, id: result.workspace.id }, ]; + } catch (error) { + console.error('Failed to fetch resource scopes:', error); + throw new Error('Failed to fetch resource scopes'); + } };
195-203: Consider centralizing soft delete logicThe soft delete checks are currently scattered across different queries. Consider creating a reusable utility function or query builder to centralize this logic and ensure consistent handling across all resource-related queries.
Example approach:
// Create a utility function const withoutDeleted = <T extends { deletedAt: Date | null }>(qb: QueryBuilder<T>) => { return qb.where(isNull('deletedAt')); }; // Usage in queries const result = await db .select() .from(resource) .pipe(withoutDeleted) .where(eq(resource.id, id));Also applies to: 359-359
packages/api/src/router/environment.ts (1)
276-276: Consider consolidating the soft delete checkWhile the soft delete filtering is correctly implemented, there's a minor opportunity to reduce code duplication. The
isNotDeletedcondition at line 308 could be combined with the existing checks at lines 276 and 285.Consider this refactoring:
- isNull(resource.deletedAt), + const isNotDeleted = isNull(resource.deletedAt);Then reuse
isNotDeletedin all three locations:where: and( eq(resource.workspaceId, oldEnv.system.workspaceId), - isNull(resource.deletedAt), + isNotDeleted, newQuery, oldQuery && not(oldQuery), ),Also applies to: 285-285, 308-308, 312-312
packages/api/src/router/deployment-variable.ts (1)
341-343: Consider optimizing condition order for better performanceWhile the soft delete filtering is correctly implemented, consider reordering the conditions to optimize query performance. Place simpler conditions before potentially expensive ones:
and( - eq(resource.id, input), - isNull(resource.deletedAt), - resourceMatchesMetadata(ctx.db, targetFilter), + eq(resource.id, input), + isNull(resource.deletedAt), + resourceMatchesMetadata(ctx.db, targetFilter), ),This ensures that cheaper conditions (
eqandisNull) are evaluated before the potentially more expensiveresourceMatchesMetadatafunction.packages/job-dispatch/src/__test__/job-variables-deployment.test.ts (1)
66-66: LGTM! Consider adding soft delete test cases.The addition of
deletedAtproperty aligns with the PR's soft delete implementation. However, since this is a significant change in resource handling, consider adding test cases that verify the behavior when:
- The target resource is soft-deleted (
deletedAtis not null)- The variable resolution process encounters soft-deleted resources
Example test case to consider adding:
it("should handle soft-deleted target resources appropriately", async () => { vi.mocked(utils.getJob).mockResolvedValue(job); const deletedTarget = { ...target, deletedAt: new Date(), }; vi.mocked(utils.getTarget).mockResolvedValue(deletedTarget); // ... rest of the test setup const result = await jobVariablesDeployment.determineVariablesForReleaseJob( db, { ...job.job, releaseJobTrigger: job.release_job_trigger, }, ); // Assert the expected behavior for soft-deleted resources expect(result).toHaveLength(0); // or whatever the expected behavior is });packages/api/src/router/deployment.ts (1)
625-625: Consider optimizing the duplicate soft delete checks.The soft delete filtering is correctly implemented at both the resource lookup and join levels. However, consider using a CTE or subquery to avoid the duplicate
isNull(resource.deletedAt)check, which could improve maintainability.Here's a suggested optimization using a CTE:
.query(async ({ ctx, input }) => { - const tg = await ctx.db - .select() - .from(resource) - .where(and(eq(resource.id, input), isNull(resource.deletedAt))) - .then(takeFirst); + const activeResource = ctx.db + .with('active_resource', (db) => + db + .select() + .from(resource) + .where(and(eq(resource.id, input), isNull(resource.deletedAt))) + ); + + const tg = await activeResource + .select() + .from('active_resource') + .then(takeFirst); // ... rest of the query using activeResource CTE ... .where( and( eq(resource.id, input), - isNull(resource.deletedAt), inArray(job.status, [ JobStatus.Completed, JobStatus.Pending, JobStatus.InProgress, ]), ), )Also applies to: 664-664
packages/job-dispatch/src/resource.ts (2)
385-394: SimplifyPromise.allusage indeleteResourcesfunctionThe variable
deleteAssociatedObjectsis assigned the result ofPromise.all(), which is then included in anotherPromise.all()call. This is redundant becausedeleteAssociatedObjectsis already a promise. Consider simplifying the code by directly including the promises in a singlePromise.all()call.Here is a suggested refactor:
- const deleteAssociatedObjects = Promise.all( - resources.map((r) => deleteObjectsAssociatedWithResource(tx, r)), - ); - await Promise.all([ - deleteAssociatedObjects, - tx - .update(resource) - .set({ deletedAt: new Date() }) - .where(inArray(resource.id, resourceIds)), - ]); + await Promise.all([ + ...resources.map((r) => deleteObjectsAssociatedWithResource(tx, r)), + tx + .update(resource) + .set({ deletedAt: new Date() }) + .where(inArray(resource.id, resourceIds)), + ]);
385-394: Update JSDoc comments fordeleteResourcesfunctionThe JSDoc comments for the
deleteResourcesfunction mentionresourceIdsas a parameter, but the function signature acceptsresources: Resource[]. Please update the comments to reflect the current parameters for clarity and maintain documentation accuracy.packages/api/src/router/resources.ts (2)
539-542: Handling updates to deleted resourcesBy adding
isNotDeletedto thewhereclause, the update operation prevents modifications to deleted resources. Consider returning an explicit error message to inform the user that the resource has been deleted and cannot be updated.
663-668: Validating resource existence and active status before redeployIncluding
isNotDeletedin thewhereclause ensures only active resources are redeployed. Consider returning a clear error message if the resource is deleted or not found to improve user feedback.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (32)
apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts(2 hunks)apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts(2 hunks)apps/webservice/src/app/api/v1/relationship/deployment-to-resource/route.ts(2 hunks)apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts(3 hunks)apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(4 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts(5 hunks)packages/api/src/router/deployment-variable.ts(4 hunks)packages/api/src/router/deployment.ts(4 hunks)packages/api/src/router/environment.ts(5 hunks)packages/api/src/router/job.ts(9 hunks)packages/api/src/router/release-deploy.ts(2 hunks)packages/api/src/router/release.ts(2 hunks)packages/api/src/router/resources.ts(15 hunks)packages/api/src/router/target-provider.ts(1 hunks)packages/api/src/router/workspace.ts(1 hunks)packages/auth/src/utils/rbac.ts(4 hunks)packages/db/drizzle/0041_smooth_tarantula.sql(1 hunks)packages/db/drizzle/meta/_journal.json(1 hunks)packages/db/src/schema/job.ts(2 hunks)packages/db/src/schema/resource-agent.ts(0 hunks)packages/db/src/schema/resource-group.ts(1 hunks)packages/db/src/schema/resource-session.ts(0 hunks)packages/db/src/schema/resource.ts(1 hunks)packages/job-dispatch/src/__test__/job-variables-deployment.test.ts(1 hunks)packages/job-dispatch/src/environment-creation.ts(2 hunks)packages/job-dispatch/src/events/triggers/deployment-deleted.ts(2 hunks)packages/job-dispatch/src/events/triggers/environment-deleted.ts(3 hunks)packages/job-dispatch/src/events/triggers/resource-deleted.ts(2 hunks)packages/job-dispatch/src/job-variables-deployment/utils.ts(3 hunks)packages/job-dispatch/src/lock-checker.ts(1 hunks)packages/job-dispatch/src/release-job-trigger.ts(1 hunks)packages/job-dispatch/src/resource.ts(7 hunks)
💤 Files with no reviewable changes (2)
- packages/db/src/schema/resource-agent.ts
- packages/db/src/schema/resource-session.ts
✅ Files skipped from review due to trivial changes (1)
- packages/db/src/schema/resource-group.ts
🔇 Additional comments (74)
packages/db/drizzle/0041_smooth_tarantula.sql (2)
2-2: LGTM! Well-implemented soft delete column
The deleted_at timestamp column implementation follows best practices:
- Uses timestamp with timezone for accurate deletion tracking
- Column is nullable by default which is correct for soft delete pattern
- Existing rows will have NULL deleted_at indicating they are active
Let's verify the implementation of soft delete filtering:
✅ Verification successful
Soft delete implementation is well-integrated across the codebase
The verification shows comprehensive implementation of soft delete filtering:
- All resource queries consistently include
isNull(resource.deletedAt)checks - Proper soft delete handling in critical paths (RBAC, jobs, deployments, etc.)
- Deletion is implemented via
SET deletedAt = new Date()rather than row deletion - No direct access to deleted resources due to consistent null checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify soft delete implementation across codebase
# Check for proper null checks on deleted_at
rg -A 2 'isNull.*deletedAt'
# Look for direct deleted_at column access to ensure we're not missing any checks
rg -A 2 'deleted_at|deletedAt'
Length of output: 35314
1-1: Verify data migration for resource_session table
Dropping tables is a destructive operation. Please ensure that:
- Any important data has been migrated if needed
- No foreign key constraints are broken by this removal
Let's check for potential foreign key relationships and table usage:
✅ Verification successful
Let me gather more context about the table's usage in the codebase.
Let me check for any schema definitions or models that might be using this table.
Safe to drop resource_session table
Based on the verification results:
- The table was recently created in migration 0037 and is being dropped in 0041
- No code references found in TypeScript/JavaScript files
- Only references are in SQL migrations and their metadata snapshots
- The table has foreign key relationships to
resourceandusertables, but since there are no code references, it appears to be an unused/experimental table that was added recently
The table can be safely dropped as it appears to be unused in the codebase. The foreign key constraints will be automatically removed when dropping the table.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for foreign keys and usage of resource_session table
# Search for foreign key references
rg -i "resource_session" --type sql
# Search for table usage in code
ast-grep --pattern 'resourceSession'
Length of output: 747
Script:
#!/bin/bash
# Check for any code references to resource_session
rg -i "resource.?session" -g "!*.sql"
# Check git history for migration 37 which created the table
git show $(git rev-list --all | grep -f <(git log --format=%H -- packages/db/drizzle/0037_giant_stardust.sql))
Length of output: 3019
Script:
#!/bin/bash
# Check for schema definitions and models
rg -i "resource.?session" -g "*.{ts,js,tsx,jsx}"
# Check for any recent commits mentioning resource_session
git log --grep="resource.?session" -i --pretty=format:"%h %s" -n 5
Length of output: 50
packages/job-dispatch/src/lock-checker.ts (1)
21-21: LGTM! Well-implemented soft delete filter.
The addition of isNull(resource.deletedAt) condition is correct and consistent with the existing pattern. This ensures that locked resources which have been soft-deleted are properly excluded from the policy check.
Let's verify the impact on existing queries and tests:
✅ Verification successful
The isNull(resource.deletedAt) condition is correctly implemented and consistent across the codebase
The verification shows that:
- The
deletedAtcolumn is properly defined in the database schema (packages/db/src/schema/resource.ts) - The
isNull(resource.deletedAt)condition is consistently used across the codebase to filter out soft-deleted resources - The lock checker's implementation aligns with other resource queries in the codebase that also check for soft-deleted resources
- The condition is properly placed in the AND clause alongside other filtering conditions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other similar queries that might need the same condition
# and verify test coverage for the new condition
# Look for similar resource queries that might need the deletedAt check
rg -A 5 "from\(resource\)" --type typescript
# Check for existing test cases
rg -A 10 "isPassingLockingPolicy" --type typescript --glob "*test*"
Length of output: 184
Script:
#!/bin/bash
# Let's try without typescript file type specification since it's not recognized
# Look for similar resource queries
rg -A 5 "from\(resource\)"
# Check for test cases
fd "test" | rg "lock" | xargs rg -A 10 "isPassingLockingPolicy"
# Also check for similar deletedAt conditions to ensure consistency
rg -A 3 "deletedAt"
Length of output: 32029
packages/job-dispatch/src/events/triggers/deployment-deleted.ts (2)
5-5: LGTM! Import changes are well-organized
The new import statement correctly adds the necessary database operators for the soft delete implementation.
34-37: Implementation looks good, verify event handler compatibility
The soft delete implementation is correct, using and to combine the existing metadata filter with the new deletion status check. This ensures only active resources are processed.
Let's verify that all deployment-related event handlers are prepared for this change:
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information to verify the event handler compatibility:
Implementation is safe, all handlers consistently check for soft deletion
The codebase shows consistent handling of soft-deleted resources across all deployment-related event handlers and queries:
- All job queries include
isNull(resource.deletedAt)checks - Resource removal events (
deployment.resource.removed) properly handle soft deletion - Event handlers in
resource-removed.tsanddeployment-deleted.tsmaintain consistent filtering
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check all deployment event handlers for consistent resource filtering
# Search for deployment-related event handlers and their resource queries
rg -l "deployment\." --type ts | xargs rg -B3 -A3 "resource\."
# Check for any direct deletedAt field access patterns
ast-grep --pattern 'resource.deletedAt'
Length of output: 98556
apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts (2)
4-4: LGTM! Import changes are well-organized.
The addition of isNull operator alongside other query operators is clean and consistent.
43-43: LGTM! Verify index coverage for optimal performance.
The soft delete filter is correctly implemented. However, since this query joins multiple tables and includes several conditions, let's verify that we have proper index coverage.
packages/job-dispatch/src/events/triggers/resource-deleted.ts (2)
5-5: LGTM: Clean import statement
The import statement correctly includes the necessary database utilities for implementing soft delete functionality.
40-43: Verify impact on existing event triggers
The implementation correctly filters out soft-deleted resources. However, let's verify that this change doesn't affect existing event triggers.
✅ Verification successful
Let me analyze the results and gather more information to verify the impact comprehensively.
Let me try a different approach to gather the necessary information.
Based on the comprehensive code review, I can now provide the final response:
Implementation is correct and consistent with existing behavior
The getEventsForResourceDeleted function is correctly implemented and its usage is consistent throughout the codebase. The isNull(SCHEMA.resource.deletedAt) check is necessary because:
- The function is explicitly documented to run BEFORE the actual deletion occurs
- It's used in
deleteResourcesfunction to gather events before performing the soft delete - The implementation correctly identifies resources that will be affected by the deletion
The change doesn't affect existing event triggers as the function is only used in one place (resource.ts) and maintains its documented behavior of running before the actual deletion.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other files that might be affected by this change
rg -l "getEventsForResourceDeleted" | while read -r file; do
echo "=== $file ==="
rg -A 5 "getEventsForResourceDeleted" "$file"
done
Length of output: 1257
Script:
#!/bin/bash
# Let's check the implementation details and usage context
ast-grep --pattern 'getEventsForResourceDeleted = async ($_, $_) => { $$$ }'
# Also check for any test files related to this functionality
rg -l "getEventsForResourceDeleted.*test"
Length of output: 122
Script:
#!/bin/bash
# Check the full implementation context of the file
cat packages/job-dispatch/src/events/triggers/resource-deleted.ts
# Check how this function is used in resource.ts
cat packages/job-dispatch/src/resource.ts
Length of output: 12848
apps/webservice/src/app/api/v1/relationship/deployment-to-resource/route.ts (1)
3-3: LGTM: Clean import addition
The isNull import is properly grouped with related database utilities.
apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts (2)
3-3: LGTM: Import addition aligns with soft delete implementation
The addition of isNull import is consistent with the PR's objective to implement soft delete functionality.
31-31: LGTM: Proper filtering of soft-deleted resources
The addition of isNull(SCHEMA.resource.deletedAt) checks ensures that relationships cannot be created with soft-deleted resources, maintaining data integrity.
Let's verify this is consistently applied across other relationship endpoints:
Also applies to: 48-48
✅ Verification successful
Soft delete checks are consistently implemented across relationship endpoints
The verification confirms that both relationship endpoints (deployment-to-resource and resource-to-resource) properly implement soft delete checks using isNull(SCHEMA.resource.deletedAt) in their WHERE clauses. The resource-to-resource endpoint even has this check implemented twice - once for each resource in the relationship (fromResource and toResource).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all resource relationship endpoints properly handle soft deletes
# Expected: All resource relationship endpoints should have isNull checks for deletedAt
ast-grep --pattern 'select()
.from(SCHEMA.resource)
.where(
and(
$$$
isNull(SCHEMA.resource.deletedAt)
)
)'
Length of output: 135
Script:
#!/bin/bash
# Let's try a different approach to find resource relationship endpoints and their soft delete handling
# First, find all API route files related to relationships
fd "route.ts" apps/webservice/src/app/api/v1/relationship
# Then, let's examine the content of these files focusing on deletedAt checks
fd "route.ts" apps/webservice/src/app/api/v1/relationship -x rg -A 5 -B 5 "deletedAt|isNull|softDelete" {}
Length of output: 1948
packages/job-dispatch/src/events/triggers/environment-deleted.ts (2)
5-5: LGTM: Import changes are appropriate
The addition of isNull import aligns with the soft delete implementation requirements.
18-23: Verify index existence for optimal query performance
The soft delete filtering logic is correctly implemented. However, since we're adding a new condition to filter by deletedAt, we should ensure proper indexing for optimal query performance.
✅ Verification successful
Let me search for the resource table definition and any indexes on deletedAt.
Let me search specifically for the resource table definition to see if deletedAt is part of its schema.
Let me try a different approach to find the schema definition.
Let me check the complete resource schema definition.
No index needed for the current query pattern
The deletedAt column is part of the resource table schema and is used in a simple isNull check. For this type of query pattern, PostgreSQL can efficiently evaluate the condition without requiring a dedicated index. The database can perform a sequential scan or utilize existing indexes (like the unique index on identifier and workspace_id) effectively. Adding an index specifically for deletedAt would not provide significant performance benefits in this case, as NULL checks on sparsely populated columns (where most values are NULL) don't benefit much from indexing.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if an index exists on the deletedAt column
# Look for index definitions in migration files or schema files
# Search for index definitions
rg -i "index.*deletedat|deletedat.*index" --type sql
# Search for create table definitions to check inline index definitions
rg -i "create.*table.*resource" -A 20 --type sql
Length of output: 4153
Script:
#!/bin/bash
# Search for resource table definition and indexes
rg -i "create.*table.*resource\W" -B 2 -A 30 --type sql
# Search specifically for resource table indexes
rg -i "create.*index.*resource" --type sql
Length of output: 1809
Script:
#!/bin/bash
# Search for resource table definition in migration files
rg -i "create table.*resource\W" -B 2 -A 40 --type sql
# Also search for any alter table statements that might have added deletedAt
rg -i "alter table.*resource.*add.*deletedat" --type sql
Length of output: 115
Script:
#!/bin/bash
# Look for schema definitions in TypeScript files or schema files
rg -i "resource.*=.*{" -A 20 --type ts packages/db/
# Also search for any column definitions related to deletedAt
rg -i "deletedAt|deleted_at" --type ts packages/db/
Length of output: 402
Script:
#!/bin/bash
# Get the complete resource schema definition
cat packages/db/src/schema/resource.ts
Length of output: 10005
packages/job-dispatch/src/job-variables-deployment/utils.ts (3)
5-5: LGTM: Import statement is correctly updated
The addition of isNull import aligns with the soft delete implementation requirements.
77-77: LGTM: Consistent soft delete implementation
The soft delete filter is correctly implemented and consistent with the getTarget function. The null handling in getFirstMatchedTarget already accommodates this change.
33-35: LGTM: Verify impact on consumers
The soft delete filter is correctly implemented. However, let's verify that all consumers of getTarget handle the case where previously available targets might now be filtered out due to deletion status.
✅ Verification successful
LGTM: Consumer already handles null case properly
The consumer in job-variables-deployment.ts already has proper error handling for when getTarget returns null, throwing an explicit error: if (!jobTarget) throw new Error(\Target not found for job ${job.id}`);`. This means it's already prepared for cases where targets might be filtered out due to deletion status.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of getTarget to verify error handling
ast-grep --pattern 'getTarget($$$)' -f .ts,.tsx | rg -A 5 'getTarget'
Length of output: 247
Script:
#!/bin/bash
# Search for all usages of getTarget function
rg "getTarget\(" -A 5
Length of output: 703
packages/job-dispatch/src/environment-creation.ts (2)
4-4: LGTM! Import statement is correctly placed.
The addition of isNull import aligns with the soft delete implementation and follows the existing import pattern.
47-47: LGTM! Soft delete filter is correctly implemented.
The addition of isNull(SCHEMA.resource.deletedAt) ensures that only non-deleted resources are processed, which aligns with the soft delete implementation.
Let's verify if there are any other resource queries in this file that might need similar filtering:
✅ Verification successful
All resource queries in the codebase consistently implement soft delete filtering
The verification shows that all resource queries across the codebase already include isNull(SCHEMA.resource.deletedAt) in their conditions, and the ordering is consistent - typically placed after workspace/identifier checks and metadata matching. No additional changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other resource queries in this file that might need soft delete filtering
ast-grep --pattern 'from(SCHEMA.resource)' packages/job-dispatch/src/environment-creation.ts
# Check the order of conditions in similar queries across the codebase for consistency
ast-grep --pattern 'and(
$$$,
isNull(SCHEMA.resource.deletedAt),
$$$
)'
Length of output: 4873
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts (2)
3-3: LGTM: Import changes align with soft delete implementation.
83-87: Verify the impact of soft delete on related entities.
The soft delete implementation might need to handle related entities (metadata, variables) to maintain data consistency.
✅ Verification successful
Based on the database schema and code analysis, I can now provide a final response:
Soft delete implementation correctly handles related entities
The soft delete implementation for resources is properly designed with the following evidence:
- All queries consistently check for
isNull(resource.deletedAt)when accessing resources - Related entities (metadata, variables) have
ON DELETE CASCADEconstraints in the schema, ensuring they are automatically handled when the parent resource is deleted - Foreign key relationships from other tables (like
release_job_trigger,resource_relationship, etc.) properly respect the soft delete status by includingisNull(resource.deletedAt)in their queries
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for related tables that might need soft delete handling
ast-grep --pattern 'CREATE TABLE $_resource_related {
$$$
resourceId $_,
$$$
}'
# Check for foreign key constraints that might be affected
ast-grep --pattern 'FOREIGN KEY ($$$) REFERENCES resource($$$)'
Length of output: 153
Script:
#!/bin/bash
# Let's try a different approach to find related tables and their relationships
# Search for table definitions in SQL schema files
fd -e sql . | xargs cat
# Search for references to resource table in TypeScript files
rg -A 5 "resource\." --type ts
# Look for foreign key relationships in schema definitions
rg -A 5 "references" -i --type sql
Length of output: 100858
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts (4)
5-5: LGTM! Import changes align with soft delete implementation.
The new imports support the soft delete functionality by providing necessary database operations and resource management utilities.
Also applies to: 7-7
25-29: LGTM! Proper implementation of soft delete filtering.
The changes correctly implement soft delete filtering by:
- Creating a clear isNotDeleted condition
- Properly combining it with the resource ID check
86-89: LGTM! Consistent implementation of soft delete filtering.
The changes maintain consistency with the GET handler implementation and properly prevent modifications to soft-deleted resources.
112-115: Verify the soft delete implementation in deleteResources.
The filtering logic is consistent with other handlers, but let's verify the implementation of the deleteResources function to ensure it properly handles soft deletion.
Also applies to: 123-123
✅ Verification successful
Let me gather more specific information about the deletion implementation.
Based on the search results, I can now generate the final response as I have enough information about the soft delete implementation.
Soft delete implementation in deleteResources is verified and working correctly
The implementation properly handles soft deletion through:
- Setting
deletedAttimestamp instead of performing hard deletes - Consistent filtering of soft-deleted resources using
isNull(resource.deletedAt)across the codebase - Proper handling of associated objects and events before marking resources as deleted
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of deleteResources function
# Check if deleteResources performs soft delete
ast-grep --pattern 'export async function deleteResources($_, $_) {
$$$
}'
# Check for any direct delete operations that might bypass soft delete
rg -l 'db\.delete'
Length of output: 329
Script:
#!/bin/bash
# Search for deleteResources implementation and its usage
rg -A 10 "deleteResources"
# Search for soft delete related operations
rg -A 5 "deletedAt|softDelete"
# Check the schema definition for the resource table
fd "schema.ts|schema.prisma" --exec cat {}
Length of output: 63298
apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts (2)
4-4: LGTM!
The isNull import is correctly added and aligns with the soft delete implementation.
86-86: Verify the impact of filtering deleted resources
The addition of isNull(resource.deletedAt) to the where clause might have unintended consequences:
- Since
resourceis left-joined, this condition could filter out jobs that don't have an associated resource. - There might be valid use cases where we need to view jobs even if their associated resource is deleted.
Consider if the soft delete filter should be conditional based on the query context.
Let's verify the impact:
✅ Verification successful
Based on the search results, I can now provide a conclusive response about the impact of filtering deleted resources in the job query:
The isNull(resource.deletedAt) check is consistent with the codebase pattern
The codebase consistently applies the soft delete filter (isNull(resource.deletedAt)) across various queries involving resources, including:
- Job queries in multiple endpoints
- Resource retrieval endpoints
- Deployment and environment related queries
- Release and job trigger queries
This pattern ensures that:
- Deleted resources are consistently hidden across the entire application
- The left join with resources table still works correctly as the
isNullcheck is part of thewhereclause - Jobs without associated resources are still visible as the condition is part of a compound
andclause with the job ID check
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential job visibility issues
# Look for other job queries in the codebase to understand the consistency of this pattern
rg -A 5 "from\(job\).*where"
# Check if there are places where deleted resources are explicitly queried
ast-grep --pattern 'where($$$isNotNull(resource.deletedAt)$$$)'
Length of output: 96
Script:
#!/bin/bash
# Let's try a different approach to understand the job-resource relationship and query patterns
# Check the schema definition for job and resource tables
fd "schema\.ts" --exec cat {}
# Look for job queries with different patterns
rg "from.*job.*leftJoin.*resource" -A 5
# Look for resource deletion patterns
rg "resource.*deletedAt" -A 3
Length of output: 45018
packages/api/src/router/release-deploy.ts (2)
6-13: LGTM! Import changes are well-structured
The new imports are properly organized and include the necessary isNull operator for implementing soft delete functionality.
133-135: LGTM! Soft delete check properly implemented
The query correctly filters out soft-deleted resources using isNull(resource.deletedAt).
Let's verify consistent implementation across other resource queries:
✅ Verification successful
Soft delete check is consistently implemented across resource queries
The search results show that resource queries throughout the codebase consistently include the soft delete check (isNull(resource.deletedAt)) when filtering resources. This is evident in multiple files:
packages/api/src/router/deployment.tspackages/api/src/router/job.tspackages/api/src/router/workspace.ts- And other relevant files
The implementation in the reviewed code follows the established pattern across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for resource queries that might need similar soft delete checks
ast-grep --pattern 'select()
.from(resource)
.where($$$)'
Length of output: 63
Script:
#!/bin/bash
# Search for resource queries using ripgrep for better pattern matching
rg "from\(resource\).+where" -A 2 --type ts
Length of output: 45
Script:
#!/bin/bash
# Let's try a broader search for resource queries
rg "from\(resource\)" --type ts -B 2 -A 2
Length of output: 6121
packages/job-dispatch/src/release-job-trigger.ts (1)
148-148: LGTM! The soft delete filter is correctly implemented.
The addition of isNull(resource.deletedAt) aligns with the PR objective and ensures that job triggers are only created for non-deleted resources. The implementation follows the established pattern and maintains the existing query structure.
Let's verify the consistent implementation of this pattern across similar resource queries:
✅ Verification successful
LGTM! The soft delete filter is consistently implemented across the codebase.
The search results confirm that the isNull(resource.deletedAt) pattern is consistently implemented across all resource queries in the codebase. This includes:
- Job triggers and job-related queries
- Environment-related queries
- Deployment-related queries
- Resource relationship queries
- API endpoints
- Event triggers
The implementation in release-job-trigger.ts follows the established pattern and maintains consistency with the rest of the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that similar resource queries also filter deleted resources
# Search for resource queries that might need the deletedAt filter
ast-grep --pattern 'from(resource)
.where($$$)'
# Search for existing implementations of the soft delete pattern
rg 'isNull.*deletedAt.*resource'
Length of output: 86
Script:
#!/bin/bash
# Let's try a broader search pattern to find resource queries and soft delete patterns
# Search for resource queries using different patterns
rg -A 5 'from\(.*resource'
# Search for any deletedAt conditions
rg -A 2 'deletedAt'
# Search for isNull conditions
rg -A 2 'isNull'
Length of output: 58066
packages/db/drizzle/meta/_journal.json (1)
291-297: LGTM! Verify the corresponding migration script.
The new migration entry follows the established pattern and is properly sequenced. However, since this implements soft deletion functionality, we should verify the corresponding migration script.
✅ Verification successful
LGTM! Migration successfully implements soft deletion.
The migration script correctly implements the soft delete functionality by:
- Adding
deleted_atcolumn to theresourcetable - Removing the
resource_sessiontable as planned
The changes align with the PR objectives and follow proper database schema modification practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the migration script for soft delete implementation
# Check the migration file content
echo "Checking migration script content:"
cat packages/db/drizzle/*/0041_smooth_tarantula.sql
# Look for deletedAt column additions
echo -e "\nVerifying deletedAt column additions:"
rg -l "ADD COLUMN.*deletedAt" packages/db/drizzle/*/*.sql
# Check for resource_session table removal
echo -e "\nVerifying resource_session table removal:"
rg -l "DROP TABLE.*resource_session" packages/db/drizzle/*/*.sql
Length of output: 673
Script:
#!/bin/bash
# Let's first check the directory structure to locate migration files
echo "Checking directory structure:"
fd -t d drizzle
echo -e "\nListing files in drizzle directory:"
fd . -t f packages/db/drizzle
echo -e "\nChecking meta journal content:"
cat packages/db/drizzle/meta/_journal.json
echo -e "\nLooking for migration files with pattern 0041:"
fd "0041" packages/db/drizzle
Length of output: 10583
Script:
#!/bin/bash
# Now let's check the content of the migration file and its snapshot
echo "Checking migration file content:"
cat packages/db/drizzle/0041_smooth_tarantula.sql
echo -e "\nChecking snapshot content:"
cat packages/db/drizzle/meta/0041_snapshot.json
Length of output: 121561
packages/api/src/router/workspace.ts (1)
233-233: Verify consistent implementation of soft delete filtering.
Let's ensure that all resource-related endpoints consistently implement the soft delete filtering.
✅ Verification successful
Soft delete filtering is consistently implemented across the codebase
The verification shows that isNull(resource.deletedAt) is consistently used across all resource-related queries in the codebase, including:
- Workspace routes
- Job routes
- Deployment routes
- Environment routes
- Release routes
- Resource routes
- Authentication/RBAC checks
- Job dispatch system
The implementation is uniform and follows the same pattern throughout the application, ensuring consistent handling of soft-deleted resources.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent implementation of soft delete filtering across resource-related endpoints
# Search for resource queries that might need soft delete filtering
rg -t typescript "from\(resource\)" -B 2 -A 5
# Search for existing soft delete implementations
rg -t typescript "isNull.*deletedAt"
Length of output: 156
Script:
#!/bin/bash
# Let's try without the typescript file type flag since it's not recognized
# Search for resource queries and deletedAt filtering
rg "from\(resource\)" -B 2 -A 5
rg "isNull.*deletedAt"
# Also search for table definitions to understand the schema
rg "createTable\(.*resource"
Length of output: 15153
packages/db/src/schema/job.ts (2)
14-14: LGTM: Import addition is appropriate
The isNull import aligns with the PR's soft delete implementation objective and is properly utilized in the code.
222-223: Implementation looks good, but verify impact on existing queries
The soft delete implementation is correct and aligns with the PR objectives. However, this change will affect all job target queries by filtering out soft-deleted resources.
Let's verify the impact on existing queries:
✅ Verification successful
Change is safe, limited usage with consistent implementation
The JobFilterType.JobTarget is used in a controlled manner:
- Schema implementation in
packages/db/src/schema/job.ts - Type definition in
packages/validators/src/jobs/conditions/job-condition.ts - UI component for condition creation in
apps/webservice/src/app/[workspaceSlug]/(app)/_components/job-condition/JobComparisonConditionRender.tsx
The change to filter out soft-deleted resources is consistent with the system's design and doesn't introduce breaking changes as:
- It's only used in condition building for new queries
- The UI component creates new conditions with this filter
- No existing tests specifically rely on this filter type's behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of JobFilterType.JobTarget to identify affected queries
rg -l "JobFilterType.JobTarget" --type typescript
# Search for potential test cases that might need updating
rg -l "test.*JobTarget" --type typescript
Length of output: 163
Script:
#!/bin/bash
# Search for direct usage of JobFilterType.JobTarget without file type restriction
rg -l "JobFilterType.JobTarget"
# Search for potential test cases that might need updating
rg "test.*JobTarget" -C 2
# Search for enum definition to understand its usage
rg "enum.*JobFilterType" -A 10
Length of output: 1222
Script:
#!/bin/bash
# Check the usage context in the identified files
rg -C 5 "JobFilterType.JobTarget" packages/validators/src/jobs/conditions/job-condition.ts
rg -C 5 "JobFilterType.JobTarget" apps/webservice/src/app/[workspaceSlug]/\(app\)/_components/job-condition/JobComparisonConditionRender.tsx
# Look for any existing tests related to job filtering
rg "describe.*job.*filter|test.*job.*filter" -C 2
Length of output: 1038
packages/api/src/router/target-provider.ts (1)
Line range hint 227-238: Verify the impact on existing queries.
The switch to soft delete requires verification of existing queries to ensure they properly filter out deleted resources.
✅ Verification successful
Let me analyze the search results and generate additional verification scripts to check the impact of soft delete on existing queries.
This will help us verify if the existing queries properly handle soft deletion by checking for the deletedAt filter in WHERE clauses and JOIN conditions.
Existing queries properly handle soft deletion with isNull(resource.deletedAt) checks
The verification shows that existing queries consistently handle soft deletion by including isNull(resource.deletedAt) checks in their WHERE clauses and JOIN conditions. This pattern is uniformly applied across:
- Job queries
- Release and deployment queries
- Environment-related queries
- RBAC checks
- Resource listing queries
The implementation of soft delete in the reviewed code is safe as all relevant queries already include the necessary filter.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for resource queries that might need updating with deletedAt filter
ast-grep --pattern 'select().from(resource)'
# Search for potential joins with the resource table
ast-grep --pattern 'join($_, resource, $_)'
# Look for direct resource table references
rg '\bresource\b' --type ts
Length of output: 62214
Script:
#!/bin/bash
# Search for direct resource table queries to verify deletedAt filter usage
ast-grep --pattern 'from(resource).where($_)'
# Search for join conditions with resource table to verify deletedAt filter usage
ast-grep --pattern 'join(resource, $_)'
# Search specifically for isNull(resource.deletedAt) usage
ast-grep --pattern 'isNull(resource.deletedAt)'
Length of output: 2952
packages/api/src/router/release.ts (1)
11-11: LGTM: Import addition is appropriate
The addition of isNull import is necessary for implementing the soft delete functionality and follows the existing import organization pattern.
packages/auth/src/utils/rbac.ts (4)
3-10: LGTM: Import changes align with soft delete requirements
The addition of isNull and takeFirstOrNull imports supports the new soft delete functionality.
195-203: LGTM: Improved resource scope handling
The changes enhance the function by:
- Using a more descriptive name
- Properly handling soft-deleted resources
- Returning an empty array instead of null for better type safety
381-381: LGTM: Scope handler mapping updated correctly
The handler mapping is properly updated to use the renamed getResourceScopes function.
359-359: Verify soft delete handling in runbook job path
While the resource-based job path correctly checks for soft-deleted resources, the runbook-based job path (lines 331-347) doesn't have similar checks. This could lead to inconsistent behavior.
packages/api/src/router/environment.ts (3)
13-13: LGTM: Import addition for soft delete support
The isNull import is appropriately placed and necessary for implementing the soft delete filtering functionality.
174-179: LGTM: Proper implementation of soft delete filtering
The changes correctly implement soft delete filtering by combining isNull(resource.deletedAt) with the existing resource metadata filter using the and operator.
Line range hint 174-312: Verify comprehensive coverage of soft delete filtering
While the implemented soft delete filtering looks correct, let's verify that all resource-related queries in the codebase are properly handling soft-deleted resources.
packages/api/src/router/deployment-variable.ts (3)
12-12: LGTM: Import addition aligns with soft delete implementation
The addition of isNull import is appropriate for implementing the soft delete filtering logic.
218-221: LGTM: Consistent implementation of soft delete filtering
The addition of isNull(resource.deletedAt) to both queries ensures that only non-deleted resources are considered when updating deployment variables. The implementation is consistent and properly combined with existing conditions.
Also applies to: 225-228
Line range hint 1-479: Verify consistent soft delete implementation across all resource queries
Let's verify that all resource queries in this file properly handle soft-deleted resources.
✅ Verification successful
All resource queries properly include soft delete checks
After analyzing the codebase, all resource queries in this file consistently implement soft delete checks using isNull(resource.deletedAt). This is verified in:
- Resource queries for old targets
- Resource queries for new targets
- Resource queries with target filters
- Resource queries with metadata matching
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for resource queries that might be missing soft delete checks
# Search for resource queries that might need soft delete checks
rg -U "from\(resource\)[^]*?where" packages/api/src/router/deployment-variable.ts
# Search for potential resource joins that might need soft delete checks
rg -U "join\([^)]*resource[^)]*\)[^]*?where" packages/api/src/router/deployment-variable.ts
Length of output: 409
Script:
#!/bin/bash
# Search for resource queries with improved regex patterns
rg -U "from\(resource\)" -A 5 packages/api/src/router/deployment-variable.ts
# Search for resource joins
rg -U "join.*resource" -A 5 packages/api/src/router/deployment-variable.ts
# Search for isNull(resource.deletedAt) checks
rg "isNull\(resource\.deletedAt\)" packages/api/src/router/deployment-variable.ts
Length of output: 924
Script:
#!/bin/bash
# Search for all resource queries to ensure we haven't missed any
rg -U "resource.*where" -B 5 -A 5 packages/api/src/router/deployment-variable.ts
# Search for all resource metadata matches to ensure complete context
rg "resourceMatchesMetadata" -B 2 -A 2 packages/api/src/router/deployment-variable.ts
Length of output: 833
packages/job-dispatch/src/__test__/job-variables-deployment.test.ts (1)
Line range hint 1-589: LGTM! Test coverage is comprehensive.
The test suite demonstrates thorough coverage of the variable resolution logic, including:
- Direct matches
- Default values
- Variable set priorities
- Sensitive values
- Multiple variable sets
- Target variable precedence
The test structure is clean and consistent.
packages/api/src/router/job.ts (8)
28-28: LGTM: Import addition aligns with soft delete implementation.
The addition of isNull import is appropriate for implementing the soft delete filtering.
166-166: LGTM: Consistent soft delete filtering in byWorkspaceId queries.
The soft delete checks are correctly implemented in both the main query and count query, ensuring consistency in the results.
Also applies to: 201-201
290-290: LGTM: Soft delete filtering added to byDeploymentAndEnvironment query.
The soft delete check is properly implemented in the deployment and environment-based query.
334-334: LGTM: Soft delete filtering added to byReleaseId query.
The soft delete check is properly implemented in the release-based query.
379-379: LGTM: Soft delete filtering added to byId query.
The soft delete check is properly implemented in the ID-based query.
475-478: LGTM: Resource filtering in release dependencies.
The filtering ensures that only non-deleted resources are considered in the dependency check, maintaining consistency with the soft delete implementation.
633-633: LGTM: Soft delete filtering added to byResourceId query.
The soft delete check is properly implemented in the resource-based query.
Line range hint 166-633: Verify the impact of soft delete filtering on existing queries.
While the implementation is thorough, let's verify that the added soft delete filtering doesn't affect any existing query results unexpectedly.
✅ Verification successful
Soft delete filtering is consistently implemented across all resource queries
The verification shows that:
- All direct queries on the resource table include the
isNull(resource.deletedAt)check - All JOIN operations with the resource table are properly filtered with soft delete checks in their corresponding WHERE clauses
- The implementation maintains consistency across different query types (list, by ID, filtered queries)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential regressions in job-related queries
# Check if there are any direct resource accesses without soft delete checks
echo "Checking for potential unchecked resource accesses..."
ast-grep --pattern 'from(resource)
.where(
$$$
)'
# Check for any JOIN operations with resources that might need soft delete checks
echo "Checking for potential unchecked resource JOINs..."
ast-grep --pattern 'innerJoin(resource, $_)'
Length of output: 334
Script:
#!/bin/bash
# Let's try a different approach to check resource-related queries
# Check for all resource table references in the file
echo "Checking all resource table references..."
rg "resource\." packages/api/src/router/job.ts -B 2 -A 2
# Check for isNull(resource.deletedAt) occurrences
echo "Checking soft delete filter usage..."
rg "isNull\(resource\.deletedAt\)" packages/api/src/router/job.ts
# Check for any JOIN operations with the resource table
echo "Checking resource JOIN operations..."
rg "join.*resource" packages/api/src/router/job.ts -i
Length of output: 3779
packages/api/src/router/deployment.ts (2)
12-12: LGTM: Import addition aligns with soft delete implementation.
The addition of isNull import is necessary for implementing the soft delete filtering logic.
421-421: Verify impact on distribution calculations.
The soft delete filter is correctly implemented. However, please verify that excluding deleted resources doesn't impact any distribution calculations or reporting that might expect historical data.
Let's check for any code that might depend on historical resource data:
✅ Verification successful
Soft delete filter is safe to implement
The distribution data is only used for displaying current resource distribution in a bar chart visualization. The component ReleaseDistributionBarChart only shows active distribution counts across different release versions, refreshing every 2 seconds. Excluding deleted resources is the correct behavior as we want to show the current state of resource distribution.
- The bar chart in
ReleaseDistributionBarChart.tsxonly displays current resource counts per release version - No historical calculations or reporting features depend on deleted resources
- The 2-second refresh interval indicates this is for real-time monitoring of active resources
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for code that might rely on resource distribution calculations
rg -l 'distribution.*resource|resource.*distribution' | while read -r file; do
echo "=== $file ==="
rg -A 5 'distribution.*resource|resource.*distribution' "$file"
done
Length of output: 70
Script:
#!/bin/bash
# Let's try a broader search for distribution-related queries and calculations
rg -A 5 "distributionById|getDistribution" --type ts
# Also search for resource calculations
rg -A 5 "resource.*calculation|calculate.*resource" --type ts
# Check for any tests related to distributions
fd "distribution.*test" -e ts -e js
# Look for distribution types or interfaces
ast-grep --pattern 'interface $_Distribution {
$$$
}'
Length of output: 1666
Script:
#!/bin/bash
# Let's examine the ReleaseDistributionBarChart component to understand how the data is used
cat "apps/webservice/src/app/[workspaceSlug]/(app)/systems/[systemSlug]/deployments/[deploymentSlug]/releases/ReleaseDistributionBarChart.tsx"
# Let's also check the full context of the distributionById query implementation
ast-grep --pattern 'distributionById: protectedProcedure
$$$
'
Length of output: 1714
packages/job-dispatch/src/resource.ts (2)
278-278: Verify behavior when upserting previously deleted resources
Setting deletedAt: null during an upsert operation will revive resources that were previously soft-deleted if they have the same identifier and workspaceId. Please confirm if this is the intended behavior.
66-66: Ensure isNotDeleted is consistently applied in resource queries
While the isNotDeleted condition has been added to several resource queries to exclude soft-deleted resources, please verify that all queries fetching resources include this condition. This will prevent operations on resources that have been marked as deleted.
Also applies to: 252-252
packages/api/src/router/resources.ts (15)
14-14: Importing isNull
The isNull function is correctly imported from @ctrlplane/db for checking null values.
39-39: Defining isNotDeleted condition
The isNotDeleted constant is appropriately defined to check for resources that are not deleted by verifying if deletedAt is NULL.
45-46: Including isNotDeleted in query conditions
The query now correctly filters out deleted resources by including isNotDeleted in the where condition, ensuring only active resources are retrieved.
107-107: Filtering out deleted resources in resource retrieval
The addition of isNotDeleted ensures that only active resources are considered when fetching related resources.
197-202: Adding isNotDeleted to resource view filters
Including isNotDeleted in the where clause correctly filters out deleted resources when calculating the total count in resource views.
206-206: Returning total count with resource view
The total number of active resources is correctly returned along with the view details.
335-335: Updating byId procedure to filter out deleted resources
Adding isNotDeleted ensures that the byId procedure retrieves only active resources, preventing operations on deleted ones.
351-351: Including isNotDeleted in resource relationship queries
By adding isNotDeleted, the relationship queries now exclude deleted resources, ensuring data integrity.
357-357: Filtering environments with a resource filter
The addition of where: hasFilter correctly filters environments to include only those with a defined resourceFilter, optimizing query results.
373-373: Ensuring active resources in environment matching
Including isNotDeleted in the environment matching ensures that only active resources are considered, which is essential for accurate environment-resource associations.
439-443: Incorporating isNotDeleted into workspace resource listing
Updating the checks array to include isNotDeleted ensures that the resource list for a workspace excludes deleted resources.
595-597: Preventing deletion of already deleted resources
Including isNotDeleted ensures that the deletion operation only targets active resources, avoiding redundant actions on already deleted items.
617-617: Excluding deleted resources when fetching metadata keys
Adding isNotDeleted refines the metadata keys query to gather keys only from active resources, enhancing data relevance.
633-633: Ensuring resource is active before locking
Including isNotDeleted verifies that only active resources can be locked, preventing invalid operations on deleted resources.
650-650: Ensuring resource is active before unlocking
Adding isNotDeleted ensures that unlock operations are performed only on active resources, maintaining consistency.
| @@ -27,6 +27,7 @@ export const POST = request() | |||
| and( | |||
| eq(SCHEMA.resource.identifier, body.resourceIdentifier), | |||
| eq(SCHEMA.resource.workspaceId, body.workspaceId), | |||
| isNull(SCHEMA.resource.deletedAt), | |||
There was a problem hiding this comment.
💡 Codebase verification
Soft delete check should be added for deployments
Based on the code analysis, deployments do support soft delete functionality, as evidenced by:
- The deployment-deleted trigger file which handles soft delete events
- The deployment router's usage of
isNull(resource.deletedAt)in multiple queries
Add the suggested check to prevent relationships with soft-deleted deployments:
where(
and(
eq(SCHEMA.deployment.id, body.deploymentId),
eq(SCHEMA.system.workspaceId, body.workspaceId),
+ isNull(SCHEMA.deployment.deletedAt),
),
)🔗 Analysis chain
Verify soft delete implementation for deployments
While the resource query correctly filters out soft-deleted resources, the deployment query doesn't have a similar check. This could potentially allow creating relationships with soft-deleted deployments.
Let's verify if deployments also support soft delete:
If deployments support soft delete, consider adding the check:
where(
and(
eq(SCHEMA.deployment.id, body.deploymentId),
eq(SCHEMA.system.workspaceId, body.workspaceId),
+ isNull(SCHEMA.deployment.deletedAt),
),
)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if deployment table has deletedAt column
ast-grep --pattern 'deployment = {
$$$
deletedAt: timestamp("deletedAt")
$$$
}'
# Check if other routes filter deleted deployments
rg -A 2 "from\(SCHEMA\.deployment\)" | rg "isNull.*deletedAt"
Length of output: 151
Script:
#!/bin/bash
# Let's try a different approach to check for deletedAt in deployment schema
rg -A 10 "deployment\s*=\s*{"
# Check for soft delete usage in deployment-related files
fd deployment | xargs rg "deletedAt|soft.*delete" -l
# Look for isNull checks in deployment queries
rg "deployment.*isNull" -A 2
Length of output: 570
Script:
#!/bin/bash
# Fix the regex syntax and check deployment schema
rg -A 10 "deployment = {"
# Check content of deployment-deleted trigger file
cat packages/job-dispatch/src/events/triggers/deployment-deleted.ts
# Check deployment router for soft delete implementation
cat packages/api/src/router/deployment.ts
Length of output: 22994
| if (resource == null) { | ||
| return NextResponse.json( | ||
| { error: `Target not found for identifier: ${params.identifier}` }, | ||
| { status: 404 }, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Maintain consistent terminology in error messages.
The error message still refers to "Target" instead of "Resource".
if (resource == null) {
return NextResponse.json(
- { error: `Target not found for identifier: ${params.identifier}` },
+ { error: `Resource not found for identifier: ${params.identifier}` },
{ status: 404 },
);
}📝 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.
| if (resource == null) { | |
| return NextResponse.json( | |
| { error: `Target not found for identifier: ${params.identifier}` }, | |
| { status: 404 }, | |
| ); | |
| } | |
| if (resource == null) { | |
| return NextResponse.json( | |
| { error: `Resource not found for identifier: ${params.identifier}` }, | |
| { status: 404 }, | |
| ); | |
| } |
| await db | ||
| .delete(schema.resource) | ||
| .where(eq(schema.resource.id, resource.id)); |
There was a problem hiding this comment.
Implement soft delete instead of hard delete.
The PR objective is to implement soft delete, but the code is performing a hard delete operation.
await db
- .delete(schema.resource)
- .where(eq(schema.resource.id, resource.id));
+ .update(schema.resource)
+ .set({ deletedAt: new Date() })
+ .where(eq(schema.resource.id, resource.id));📝 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.
| await db | |
| .delete(schema.resource) | |
| .where(eq(schema.resource.id, resource.id)); | |
| await db | |
| .update(schema.resource) | |
| .set({ deletedAt: new Date() }) | |
| .where(eq(schema.resource.id, resource.id)); |
| const resource = await db.query.resource.findFirst({ | ||
| where: and( | ||
| eq(schema.resource.workspaceId, workspaceId), | ||
| eq(schema.resource.identifier, identifier), | ||
| isNull(schema.resource.deletedAt), | ||
| ), | ||
| }); | ||
|
|
||
| if (target == null) return false; | ||
| if (resource == null) return false; | ||
| return can | ||
| .perform(Permission.ResourceGet) | ||
| .on({ type: "resource", id: target.id }); | ||
| .on({ type: "resource", id: resource.id }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate database queries for better performance.
The authorization check and the main handler are performing the same database query. Consider extracting this to a single query and reusing the result.
export const GET = request()
.use(authn)
.use(
authz(async ({ can, extra }) => {
const { workspaceId, identifier } = extra;
- const resource = await db.query.resource.findFirst({
- where: and(
- eq(schema.resource.workspaceId, workspaceId),
- eq(schema.resource.identifier, identifier),
- isNull(schema.resource.deletedAt),
- ),
- });
+ // Use the resource from the request context
+ const resource = extra.resource;
if (resource == null) return false;
return can
.perform(Permission.ResourceGet)
.on({ type: "resource", id: resource.id });
}),
)
.handle<unknown, { params: { workspaceId: string; identifier: string } }>(
async (_, { params }) => {
const data = await db.query.resource.findFirst({
where: and(
eq(schema.resource.workspaceId, params.workspaceId),
eq(schema.resource.identifier, params.identifier),
isNull(schema.resource.deletedAt),
),
with: {
metadata: true,
variables: true,
provider: true,
},
});
+ // Store in request context for authorization
+ extra.resource = data;Committable suggestion skipped: line range outside the PR's diff.
| @@ -61,6 +61,7 @@ export const resource = pgTable( | |||
| updatedAt: timestamp("updated_at", { withTimezone: true }).$onUpdate( | |||
| () => new Date(), | |||
| ), | |||
| deletedAt: timestamp("deleted_at", { withTimezone: true }), | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the analysis of the codebase, I can now provide a final response regarding the soft delete implementation:
Soft delete implementation requires query updates
The deletedAt column is correctly defined, but several query patterns need to be updated to handle soft deletes:
isNull(resource.deletedAt)condition is already being used in some queries (e.g., injob.ts,workspace.ts), showing awareness of soft delete pattern- However, many direct queries from
resourcetable don't include the soft delete check:packages/api/src/router/target-metadata-group.tspackages/api/src/router/environment.tspackages/api/src/router/resources.ts- Multiple other locations using raw
.from(resource)queries
The cascade delete constraints are safe as they operate at the database level while soft delete is handled at the application level.
🔗 Analysis chain
LGTM! Verify cascade behavior with soft deletes.
The deletedAt timestamp column is correctly defined with timezone support and is nullable, which is appropriate for soft delete functionality.
Let's verify the cascade behavior with soft deletes:
Let me analyze the cascade relationships further to ensure proper handling with soft deletes.
Let me check one final aspect to understand the query patterns used with resources.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any ON DELETE CASCADE constraints that might need review with soft delete implementation
# Search for cascade delete constraints in schema
rg -i 'ondelete.*cascade' --type ts
# Search for foreign key relationships to resource table
ast-grep --pattern 'references(() => resource.$$$)'
Length of output: 6271
Script:
#!/bin/bash
# Let's check for tables referencing the resource table and their relationships
ast-grep --pattern 'references(() => resource.$$$)'
# Also check for any existing soft delete handling in queries
rg -A 3 'deletedAt.*null' --type ts
Length of output: 641
Script:
#!/bin/bash
# Check for resource query patterns to ensure we catch all places that need soft delete handling
rg -B 2 -A 2 'from.*resource' --type ts
# Also check for any existing where conditions on resources
ast-grep --pattern 'where(eq(resource.$$$))'
Length of output: 53523
| .innerJoin( | ||
| resource, | ||
| and( | ||
| eq(releaseJobTrigger.resourceId, resource.id), | ||
| isNull(resource.deletedAt), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response as we have found several other queries using the releaseJobTrigger and resource join that don't include the deletedAt check.
Breaking change: Inconsistent handling of deleted resources across queries
The addition of isNull(resource.deletedAt) in this query creates an inconsistency with other similar queries in the codebase. Found multiple locations where releaseJobTrigger is joined with resource table without the deletedAt check:
packages/job-dispatch/src/lock-checker.tspackages/auth/src/utils/rbac.tspackages/api/src/router/deployment.tspackages/api/src/router/job.ts
This change should either be:
- Reverted to maintain consistency with existing queries, or
- Applied consistently across all similar joins in the codebase to maintain the same behavior for deleted resources
🔗 Analysis chain
Verify the impact of filtering out deleted resources
The addition of isNull(resource.deletedAt) to the join condition correctly implements the soft delete functionality. However, this is a potentially breaking change that requires verification.
Let's verify the impact of this change:
Consider adding the following test cases:
- Verify that job triggers for deleted resources are properly filtered out
- Ensure that existing functionality continues to work for non-deleted resources
- Test query performance with a significant number of deleted resources
Let me gather more specific information about how releaseJobTrigger is being used in queries.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for direct usage of releaseJobTrigger data that might be affected by the new filter
# Check for direct queries or references to releaseJobTrigger
rg -l "releaseJobTrigger" --type ts
# Check for potential performance impact by looking for similar joins
ast-grep --pattern 'innerJoin($_, and(eq($_, $_), isNull($_)))'
Length of output: 3080
Script:
#!/bin/bash
# Check for similar join patterns with releaseJobTrigger and resource tables
rg "innerJoin.*resource.*releaseJobTrigger|innerJoin.*releaseJobTrigger.*resource" -A 3 --type ts
# Check for direct queries that might be affected by the filter
rg "from\(.*releaseJobTrigger.*\)" -A 3 --type ts
# Look for any explicit handling of deleted resources
rg "deletedAt.*resource" --type ts
Length of output: 8015
| const deleteObjectsAssociatedWithResource = (tx: Tx, resource: Resource) => | ||
| Promise.all([ | ||
| tx | ||
| .delete(resourceRelationship) | ||
| .where( | ||
| or( | ||
| eq(resourceRelationship.sourceId, resource.id), | ||
| eq(resourceRelationship.targetId, resource.id), | ||
| ), | ||
| ), | ||
| tx | ||
| .delete(deploymentResourceRelationship) | ||
| .where( | ||
| eq( | ||
| deploymentResourceRelationship.resourceIdentifier, | ||
| resource.identifier, | ||
| ), | ||
| ), | ||
| ]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling in deleteObjectsAssociatedWithResource function
The deleteObjectsAssociatedWithResource function does not currently handle errors that might occur during the deletion of associated objects. Failure in any of these deletions could lead to an inconsistent state. Consider adding error handling to ensure the system remains consistent even if a deletion fails.
You can modify the function to include error handling, such as using try-catch blocks or handling promise rejections.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts (2)
34-35: Consider implementing access controls for soft-deleted resources.The handler allows querying soft-deleted resources without any additional access controls. Consider:
- Adding a specific permission check for accessing soft-deleted resources
- Adding query parameters to explicitly request soft-deleted resources
- Logging access to soft-deleted resources for audit purposes
Line range hint
18-99: Standardize the handling of soft-deleted resources across operations.There's an inconsistency in how soft-deleted resources are handled:
- GET operations allow access to soft-deleted resources
- DELETE operations prevent access to soft-deleted resources
This inconsistency could lead to security issues and confusion. Consider implementing a consistent policy across all operations, with clear access controls and audit logging for any access to soft-deleted resources.
packages/api/src/router/target-metadata-group.ts (2)
156-161: Simplify the where clause indentation.While the soft delete filtering is correctly implemented, the indentation could be simplified for better readability.
- .where( - and( - eq(resource.workspaceId, group.workspaceId), - isNull(resource.deletedAt), - ), - ) + .where(and( + eq(resource.workspaceId, group.workspaceId), + isNull(resource.deletedAt), + ))
Line range hint
248-254: Update delete procedure to implement soft delete.The delete procedure currently performs a hard delete, which is inconsistent with the PR's objective of implementing soft deletes. This should be updated to set the
deletedAttimestamp instead..mutation(({ ctx, input }) => ctx.db - .delete(resourceMetadataGroup) + .update(resourceMetadataGroup) + .set({ deletedAt: new Date() }) .where(eq(resourceMetadataGroup.id, input)), ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts(4 hunks)apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts(4 hunks)packages/api/src/router/target-metadata-group.ts(4 hunks)packages/api/src/router/target-provider.ts(4 hunks)packages/auth/src/utils/rbac.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts
- packages/auth/src/utils/rbac.ts
🔇 Additional comments (9)
packages/api/src/router/target-provider.ts (4)
4-12: Importing isNull for filtering soft-deleted resources
The addition of isNull to the imports allows the code to filter out soft-deleted resources effectively.
54-59: Adding isNull(resource.deletedAt) to exclude deleted resources
The query now correctly excludes resources that have been soft-deleted by adding isNull(resource.deletedAt) to the where clause. This ensures that only active resources are counted.
73-78: Filtering out soft-deleted resources in providerKinds
Including isNull(resource.deletedAt) in the query ensures that the resource kinds and versions counted are only from active resources, enhancing data accuracy.
241-242:
Potential issue with soft-deleted resources referencing a deleted provider
While resources are soft-deleted by setting deletedAt, the associated resourceProvider is hard-deleted. This may lead to orphaned resources that reference a provider that no longer exists, potentially causing integrity issues.
Please ensure that:
- There are no foreign key constraints that would be violated by this operation.
- The application handles resources with deleted providers gracefully.
- There are no unintended side effects when accessing or querying these soft-deleted resources.
apps/webservice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts (3)
71-73: LGTM!
Good addition of the isNull check to prevent deleting already soft-deleted resources.
18-19: Verify the security implications of accessing soft-deleted resources.
The comment indicates that soft-deleted resources are intentionally queryable. This could potentially expose sensitive data that was meant to be hidden through soft deletion.
Run the following script to check if there are any other endpoints that allow access to soft-deleted resources:
✅ Verification successful
Accessing soft-deleted resources is an intentional design pattern
Based on the comprehensive analysis of the codebase, the ability to query soft-deleted resources is a deliberate architectural decision that's consistently implemented across the system:
-
The pattern is used in multiple endpoints with proper authorization checks:
- All resource access is gated behind authorization middleware (
authnandauthz) - The DELETE endpoints explicitly check for non-deleted resources before performing operations
- Job-related queries include soft-deleted resources for maintaining execution history
- All resource access is gated behind authorization middleware (
-
The system maintains data integrity by:
- Using
isNull(resource.deletedAt)checks when listing active resources - Only allowing soft-deleted resource queries in specific read operations
- Enforcing proper authorization before any access is granted
- Using
The comment "we don't check deletedAt as we may be querying for soft-deleted resources" is part of a well-implemented soft deletion pattern, not a security concern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar patterns where deletedAt check is intentionally omitted
rg -A 5 "don't check deletedAt" --type ts
Length of output: 2458
Script:
#!/bin/bash
# Let's check if there are any authorization checks before accessing soft-deleted resources
rg -B 10 "don't check deletedAt" --type ts
# Also check if there are any other endpoints that handle soft-deleted resources
rg -A 5 "deletedAt" --type ts
Length of output: 31455
84-99: Verify the soft delete implementation in deleteResources helper.
The code now uses the deleteResources helper instead of direct DB operations. Let's verify its implementation.
✅ Verification successful
Soft delete is correctly implemented in deleteResources helper
The deleteResources helper in packages/job-dispatch/src/resource.ts properly implements soft delete by:
- Setting
deletedAttimestamp instead of performing a hard delete:.set({ deletedAt: new Date() }) - Consistently checking for soft-deleted resources across the codebase using
isNull(resource.deletedAt)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the implementation of deleteResources
ast-grep --pattern 'export function deleteResources($_, $_) {
$$$
}'
# Check for any direct delete operations that might bypass soft delete
rg -A 5 "delete\(.*resource\)" --type ts
Length of output: 115
Script:
#!/bin/bash
# Let's try a broader search for the deleteResources function
rg -A 10 "deleteResources" --type ts
# Also search for any file that might contain this function
fd "deleteResources" --type f --extension ts
Length of output: 10997
Script:
#!/bin/bash
# Let's check the implementation in job-dispatch package where deleteResources is defined
rg -A 20 "export const deleteResources = async" packages/job-dispatch/src/resource.ts
# Also check for any soft delete related operations
rg "deletedAt|set\(\{.*deletedAt" --type ts
Length of output: 6519
packages/api/src/router/target-metadata-group.ts (2)
10-10: LGTM: Import addition is correct.
The isNull operator is properly imported and grouped with other query operators from '@ctrlplane/db'.
53-53: Consider handling potential race conditions in resource counting.
While the soft delete filtering is correctly implemented, there's a potential race condition between the two separate queries that could lead to inconsistent resource counts if resources are deleted during query execution.
Consider either:
- Wrapping both queries in a transaction
- Using a single query to get both counts
Example of a single query approach:
const results = await ctx.db
.select({
totalCount: count(),
nonNullGroups: /* your existing nonNullGroups subquery */
})
.from(resource)
.where(and(eq(resource.workspaceId, input), isNull(resource.deletedAt)))
/* rest of the query */Also applies to: 86-86
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Chores