fix: Hooks reference resource + trigger hooks on filter change for env #220
Conversation
WalkthroughThe pull request implements significant modifications across multiple files to enhance resource filtering and event handling in the context of environment updates. Key changes include the introduction of new types and functions related to resource management, renaming of existing functions and variables to reflect a shift from "target" to "resource," and updates to event handling logic. The updates ensure that the system accurately tracks and manages resources, generating appropriate events during resource deletions and updates. 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: 6
🧹 Outside diff range and nitpick comments (12)
packages/validators/src/events/hooks/target.ts (1)
4-8: Consider adding type constraints to config fieldThe schema structure looks good, and the renaming from 'target' to 'resource' is consistent with the PR objectives. However, using
z.record(z.any())for config provides maximum flexibility at the cost of type safety.Consider defining a more specific schema for the config field if there are known common properties or types:
const resource = z.object({ id: z.string().uuid(), name: z.string(), - config: z.record(z.any()), + config: z.record(z.union([z.string(), z.number(), z.boolean()])), });packages/validators/src/events/hooks/index.ts (1)
3-4: Consider renaming the target.js file to maintain consistencyThe imports are from a file named "target.js" but the types have been renamed to use "resource" terminology. This naming inconsistency could cause confusion.
Consider renaming "target.js" to "resource.js" to maintain consistency with the new terminology.
packages/events/src/index.ts (1)
14-15: Consider improving error handling and type safety.Two suggestions for improvement:
- The simplified error message
Unhandled eventprovides less context than before. Consider including the event type or action for easier debugging.- Add type narrowing validation after the condition to ensure type safety.
- if (isResourceRemoved(event)) return handleResourceRemoved(event); - throw new Error(`Unhandled event`); + if (isResourceRemoved(event)) { + return handleResourceRemoved(event); + } + throw new Error(`Unhandled event type: ${event.type}`);packages/events/src/handlers/target-removed.ts (2)
1-7: File name needs to be updated to match the new terminologyThe file is still named
target-removed.tswhile the codebase has moved to use "resource" terminology. Consider renaming the file toresource-removed.tsfor consistency.
22-28: Consider adding error handling and loggingThe runbook dispatch could benefit from:
- Try-catch blocks to handle potential dispatch failures
- Logging for monitoring and debugging purposes
Here's a suggested improvement:
const resourceId = resource.id; const deploymentId = deployment.id; - const handleRunhooksPromises = runhooks.map(({ runhook }) => - dispatchRunbook(db, runhook.runbookId, { resourceId, deploymentId }), - ); + const handleRunhooksPromises = runhooks.map(async ({ runhook }) => { + try { + console.log(`Dispatching runbook ${runhook.runbookId} for resource ${resourceId}`); + await dispatchRunbook(db, runhook.runbookId, { resourceId, deploymentId }); + } catch (error) { + console.error(`Failed to dispatch runbook ${runhook.runbookId}:`, error); + throw error; + } + }); await Promise.all(handleRunhooksPromises);packages/events/src/triggers/target-deleted.ts (1)
39-42: Consider enhancing error handling and logging.While the null check is present, consider adding explicit error handling and logging for better debugging capabilities.
- const matchedResource = await db.query.resource.findFirst({ - where: SCHEMA.resourceMatchesMetadata(db, systemFilter), - }); - if (matchedResource == null) return []; + try { + const matchedResource = await db.query.resource.findFirst({ + where: SCHEMA.resourceMatchesMetadata(db, systemFilter), + }); + if (matchedResource == null) { + console.debug('No matching resource found for system filter'); + return []; + } + } catch (error) { + console.error('Failed to query resource:', error); + throw error; + }packages/events/src/triggers/environment-deleted.ts (1)
Line range hint
42-56: Add documentation for complex filtering logicWhile the implementation is correct, the complex filtering logic to determine system-wide resource removal could benefit from documentation.
Consider adding comments to explain the purpose:
+ // Create a negative filter that excludes resources still referenced by other environments const removedFromSystemFilter: ResourceCondition = { type: ResourceFilterType.Comparison, operator: ComparisonOperator.Or, not: true, conditions: envFilters, }; + // Query resources that are both matched by the environment filter and not referenced elsewhere const removedFromSystemResources = envFilters.length > 0 ? await dbpackages/api/src/router/resources.ts (1)
585-587: Consider adding error logging for failed promisesWhile the error handling using
Promise.allSettledis robust, adding error logging for failed promises would help with debugging and monitoring. Consider logging the rejected promises:- const events = ( - await Promise.allSettled(resources.map(getEventsForResourceDeleted)) - ).flatMap((r) => (r.status === "fulfilled" ? r.value : [])); + const results = await Promise.allSettled(resources.map(getEventsForResourceDeleted)); + const events = results.flatMap((r) => { + if (r.status === "rejected") { + console.error("Failed to get events for resource deletion:", r.reason); + return []; + } + return r.value; + });packages/api/src/router/environment.ts (4)
244-247: Clarify Variable Scope and NamingConsider renaming the
isOtherEnvvariable tootherEnvConditionfor better clarity, as it represents a condition used in a query, not a boolean value.Apply this diff to rename the variable:
- const isOtherEnv = and( + const otherEnvCondition = and(
283-310: Ensure Proper Handling of EmptyremovedFromSystemResourcesAfter fetching
removedFromSystemResources, it's good practice to check if the array is not empty before proceeding to generate events and handle them. This avoids unnecessary processing when there are no resources to handle.Add a condition to check before proceeding:
const removedFromSystemResources = await ctx.db.query.resource.findMany({ where: and( inArray( resource.id, removedResources.map((r) => r.id), ), resourceMatchesMetadata(ctx.db, sysFilter), ), }); + if (removedFromSystemResources.length > 0) { const events = removedFromSystemResources.flatMap((resource) => (sys?.deployments ?? []).map((deployment) => ({ action: "deployment.resource.removed" as const, payload: { deployment, resource }, })), ); const handleEventPromises = events.map(handleEvent); await Promise.allSettled(handleEventPromises); + }
308-309: Consider UsingPromise.allInstead ofPromise.allSettledIf you want to fail fast on the first rejection of a promise,
Promise.allmight be more appropriate thanPromise.allSettled, which waits for all promises to settle regardless of their outcome.Apply this diff to change the promise handling:
- await Promise.allSettled(handleEventPromises); + await Promise.all(handleEventPromises);
283-310: Optimize Event Handling LogicTo improve readability and maintainability, consider extracting the event generation and handling logic into a separate function.
Example:
const generateAndHandleRemovalEvents = async (resources, deployments) => { const events = resources.flatMap((resource) => deployments.map((deployment) => ({ action: "deployment.resource.removed" as const, payload: { deployment, resource }, })), ); const handleEventPromises = events.map(handleEvent); await Promise.all(handleEventPromises); }; // Usage: if (removedFromSystemResources.length > 0) { await generateAndHandleRemovalEvents(removedFromSystemResources, sys.deployments ?? []); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (10)
packages/api/src/router/environment.ts(4 hunks)packages/api/src/router/resources.ts(2 hunks)packages/events/src/handlers/target-removed.ts(1 hunks)packages/events/src/index.ts(1 hunks)packages/events/src/triggers/deployment-deleted.ts(1 hunks)packages/events/src/triggers/environment-deleted.ts(3 hunks)packages/events/src/triggers/target-deleted.ts(2 hunks)packages/job-dispatch/src/resource.ts(2 hunks)packages/validators/src/events/hooks/index.ts(1 hunks)packages/validators/src/events/hooks/target.ts(1 hunks)
🔇 Additional comments (20)
packages/validators/src/events/hooks/target.ts (2)
14-14: LGTM: Type export is consistent with schema changes
The type inference and naming are consistent with the schema changes.
10-13: Verify event handler updates for action literal change
The schema structure is correct and the renaming is consistent. However, changing the action literal from "deployment.target.removed" to "deployment.resource.removed" requires verification of all consuming event handlers.
✅ Verification successful
Action literal change has been properly propagated
The verification shows that the new action literal "deployment.resource.removed" is consistently used across the codebase:
- Defined in validators package (
packages/validators/src/events/hooks/target.ts) - Listed in hook actions (
packages/validators/src/events/hooks/index.ts) - Used in event handlers (
packages/events/src/handlers/target-removed.ts) - Properly triggered in multiple scenarios:
- Environment deletion (
packages/events/src/triggers/environment-deleted.ts) - Deployment deletion (
packages/events/src/triggers/deployment-deleted.ts) - Target deletion (
packages/events/src/triggers/target-deleted.ts) - Environment router (
packages/api/src/router/environment.ts)
- Environment deletion (
No references to the old action literal "deployment.target.removed" were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old action literal
rg "deployment\.target\.removed" --type ts
# Search for event handler implementations
ast-grep --pattern 'function handle$_(event: $_) {
$$$
}'
Length of output: 106
Script:
#!/bin/bash
# Search for event handlers or consumers using the new action literal
rg "deployment\.resource\.removed" --type ts -A 5
# Search for any event handler or consumer patterns
ast-grep --pattern 'case "deployment.resource.removed": {
$$$
}'
# Search for event handler class methods
ast-grep --pattern 'class $_ {
$$$
handle(event: $_) {
$$$
}
$$$
}'
Length of output: 2680
packages/validators/src/events/hooks/index.ts (2)
16-16: Verify action string updates across the codebase
The hookActionsList has been updated to use the new "resource" terminology. Ensure all consumers of these action strings have been updated accordingly.
#!/bin/bash
# Description: Check for any remaining references to old action strings
# Search for old target-related action strings
rg -l "deployment\.target\.(removed|deleted)" --type ts
# Search for places where hookActions enum is used
rg -A 3 "hookActions\." --type ts8-8: Verify event handler compatibility
The hookEvent constant now only handles resourceRemoved events. This change could impact existing event handlers that were previously handling other event types.
✅ Verification successful
Let me gather more information about the event handling to verify if this change is safe.
Based on the search results, I can now provide a final response:
Change is safe and properly aligned with codebase
The change to hookEvent is consistent with the existing implementation:
- The only event type being handled is
ResourceRemovedas evidenced byhandleEventfunction inpackages/events/src/index.ts - The event handling infrastructure (
hookActions,isResourceRemoved,handleResourceRemoved) is already set up specifically for resource removal events - No other event types are referenced or handled in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to target events in the codebase
# Search for any remaining target event handlers
rg -l "targetRemoved|targetDeleted|TargetRemoved|TargetDeleted" --type ts
# Search for direct usage of hookEvent to verify all consumers are updated
rg -A 3 "hookEvent" --type ts
Length of output: 1750
Script:
#!/bin/bash
# Search for any references to hookEvent in test files and implementation
rg -A 5 "hookEvent|ResourceRemoved" --type ts
# Search for any event-related imports and type usage
ast-grep --pattern 'import { $_ } from "$_/events/hooks"'
Length of output: 5550
packages/events/src/index.ts (1)
5-5: LGTM: Import changes align with resource terminology.
The import changes consistently reflect the shift from "target" to "resource" terminology.
Also applies to: 7-7
packages/events/src/handlers/target-removed.ts (2)
8-9: LGTM! Function signature and payload destructuring are properly updated
The changes correctly reflect the shift from "target" to "resource" terminology.
11-20: Verify consistent update of action types across the codebase
The action type has been updated to "deployment.resource.removed". Let's ensure this change is consistent across all related files.
✅ Verification successful
Based on the search results, I can generate the final response as we have comprehensive information about the usage of action types across the codebase.
Action type "deployment.resource.removed" is consistently used across the codebase
The verification shows that:
- The old action type "deployment.target.removed" is not present anywhere in the codebase
- The new action type "deployment.resource.removed" is consistently used across all relevant files:
- Properly defined in validators (
packages/validators/src/events/hooks/) - Correctly used in API routes (
packages/api/src/router/environment.ts) - Consistently implemented in event triggers and handlers (
packages/events/src/)
- Properly defined in validators (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old action type
rg "deployment\.target\.removed" --type ts
# Search for the new action type to verify consistent usage
rg "deployment\.resource\.removed" --type ts
Length of output: 790
packages/events/src/triggers/deployment-deleted.ts (2)
37-39: Verify event handler compatibility with new action name.
The event structure looks correct, but we should verify that all consumers of this event are updated to handle the new deployment.resource.removed action.
✅ Verification successful
Let me search for event handlers using a different approach to verify the compatibility.
Let me try one final search pattern to find event handlers.
Event handler compatibility verified for deployment.resource.removed
The verification shows the event action is properly integrated:
- Validated in schema:
packages/validators/src/events/hooks/target.ts - Handled in target removal:
packages/events/src/handlers/target-removed.ts - Referenced consistently across multiple files without any legacy
deployment.target.removedreferences - Event schema and handler implementation are aligned with the new action name
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for handlers of the old and new event action names
# to ensure complete migration
# Search for old event action name
echo "Checking for old event action references:"
rg "deployment\.target\.removed"
# Search for new event action name
echo "Checking for new event action references:"
rg "deployment\.resource\.removed"
# Search for potential event handlers
echo "Checking for related event handlers:"
rg -A 5 "case 'deployment\.(target|resource)\.removed'"
Length of output: 1103
Script:
#!/bin/bash
# Search for files that might contain event handlers
echo "Searching for potential event handler files:"
fd "handler" --type f
# Search for switch/case statements or event handling patterns in these files
echo "Checking event handling patterns:"
ast-grep --pattern 'case $_ : { $$$_ }'
# Search for any direct references to deployment events in handler files
echo "Checking deployment event handlers:"
rg -A 5 "deployment\." $(fd "handler" --type f)
Length of output: 494
Script:
#!/bin/bash
# Search for files containing event handling logic
echo "Searching for event handler implementations:"
rg -l "handleEvent|EventHandler|processEvent|onEvent"
# Check the content of these files for deployment event handling
echo "Checking event handler contents:"
rg -B2 -A5 "deployment\.resource\.removed" packages/events/src/handlers/target-removed.ts
# Look for event validator schemas
echo "Checking event validator schemas:"
rg -B2 -A5 "deployment\.resource\.removed" packages/validators/src/events/hooks/target.ts
Length of output: 1305
33-35: Verify database query compatibility with resource filtering.
The database query looks correct, but let's verify that the resourceMatchesMetadata function properly handles the resource filtering logic.
✅ Verification successful
Based on the gathered information, I can now generate the final response:
Database query is correctly implemented and compatible with resource filtering.
The resourceMatchesMetadata function is properly implemented in packages/db/src/schema/resource.ts and handles null/empty metadata cases appropriately. The query in the deployment-deleted trigger correctly uses this function to filter resources based on system filters. The implementation maintains backward compatibility while adopting the new resource terminology.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of resourceMatchesMetadata function
# and its compatibility with resource filtering
# Check the implementation of resourceMatchesMetadata
ast-grep --pattern 'export const resourceMatchesMetadata = $$$'
# Look for related test cases
rg -l "resourceMatchesMetadata.*test"
Length of output: 1103
Script:
#!/bin/bash
# Let's try a different approach to find the schema definition
# Search for SCHEMA object definition
rg -A 10 "export const SCHEMA = {"
# Search for resource-related schema definitions
rg "resource(Matches|Filter|Query)" -A 5
# Look for the actual database query implementation
ast-grep --pattern 'query.resource.findMany({
$$$
})'
Length of output: 49175
Script:
#!/bin/bash
# Let's check the schema definition and implementation in the db package
# Look for the schema definition in db package
rg -A 5 "export const resource = " packages/db/src/
# Check for any test files related to resource queries
fd -e test.ts -e spec.ts . -x rg -l "resource.*findMany"
# Look for the actual implementation of resourceMatchesMetadata
rg -A 10 "export function resourceMatchesMetadata" packages/db/src/
Length of output: 1290
packages/events/src/triggers/target-deleted.ts (3)
12-15: LGTM! Clear and informative documentation.
The documentation clearly explains the function's purpose and includes a crucial note about timing requirements for implementers.
49-50: Verify event consumers handle the updated action string.
The event action has changed from "deployment.target.removed" to "deployment.resource.removed". This needs verification across event consumers.
#!/bin/bash
# Description: Verify that event consumers are updated to handle the new action string
# Search for any remaining references to the old action string
echo "Checking for any remaining references to 'deployment.target.removed'..."
rg "deployment\.target\.removed"
# Search for handlers of the new action string
echo "Checking for handlers of new action 'deployment.resource.removed'..."
rg "deployment\.resource\.removed"
# Search for event handler definitions that might need updates
echo "Checking event handler definitions..."
ast-grep --pattern 'action: "deployment.$_.$_"'17-18: Verify the function rename impact across the codebase.
The function rename from getEventsForTargetDeleted to getEventsForResourceDeleted needs to be verified for consistent usage.
✅ Verification successful
Let me generate additional scripts to verify the function rename impact.
Function rename has been properly implemented across the codebase
The verification shows that:
- All references to
getEventsForTargetDeletedhave been replaced withgetEventsForResourceDeleted - The new function is correctly imported and used in:
- packages/job-dispatch/src/resource.ts
- packages/api/src/router/resources.ts
- packages/events/src/triggers/target-deleted.ts (function definition)
The git history confirms this was part of a coordinated rename effort, with all necessary changes made consistently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all references to the old function name have been updated.
# Search for any remaining references to the old function name
echo "Checking for any remaining references to 'getEventsForTargetDeleted'..."
rg "getEventsForTargetDeleted"
# Search for references to the new function name to confirm proper usage
echo "Checking usage of new function name 'getEventsForResourceDeleted'..."
rg "getEventsForResourceDeleted"
Length of output: 1217
Script:
#!/bin/bash
# Search for any remaining references to the old function name
echo "Searching for 'getEventsForTargetDeleted'..."
rg "getEventsForTargetDeleted"
# Search for references to the new function name
echo "Searching for 'getEventsForResourceDeleted'..."
rg "getEventsForResourceDeleted"
# Check git history for the function rename
echo "Checking git history for the function rename..."
git log -p | rg -A 5 -B 5 "getEventsForTargetDeleted|getEventsForResourceDeleted"
Length of output: 5172
packages/events/src/triggers/environment-deleted.ts (2)
15-19: LGTM: Resource query implementation is correct
The database query correctly fetches resources using the environment's resource filter, with appropriate early return optimization.
60-62: Verify event type change impact
The event generation logic is correct, but let's verify that all consumers handle the new "deployment.resource.removed" event type correctly.
✅ Verification successful
The search results show that the new event type "deployment.resource.removed" is consistently used across the codebase in validators, API routes, triggers, and handlers. There's no usage of the old "deployment.target.removed" event type, indicating a complete migration. Let's verify the event handler implementation to ensure it properly processes this event type.
Event type change is properly implemented and handled
The verification shows that:
- The new event type "deployment.resource.removed" is consistently used across the codebase
- The event handler (target-removed.ts) correctly processes this event type
- The event schema is properly defined in the validators
- No traces of the old event type "deployment.target.removed" remain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for consumers of the old and new event types
echo "Checking for old event type usage:"
rg "deployment\.target\.removed" --type ts
echo -e "\nChecking for new event type usage:"
rg "deployment\.resource\.removed" --type ts
echo -e "\nChecking for event handlers:"
ast-grep --pattern 'action: "deployment.$_.$_"'
Length of output: 1077
Script:
#!/bin/bash
# Check the implementation of the target-removed handler
echo "Checking target-removed handler implementation:"
cat packages/events/src/handlers/target-removed.ts
# Check for any event type validation schemas
echo -e "\nChecking event validation schemas:"
rg "deployment\.resource\.removed" -A 5 -B 5 packages/validators/src/events/hooks/target.ts
Length of output: 1628
packages/api/src/router/resources.ts (1)
20-20: LGTM! Import renamed for consistency
The import statement has been correctly updated to use the new function name, maintaining consistency with the terminology change from "target" to "resource".
packages/job-dispatch/src/resource.ts (1)
22-22: Import statement is appropriate
The import of getEventsForResourceDeleted and handleEvent from "@ctrlplane/events" integrates the necessary functions for event handling related to resource deletion.
packages/api/src/router/environment.ts (4)
2-2: Import Statement Approved
The import of ResourceCondition from @ctrlplane/validators/resources is appropriate and necessary for the updated logic.
12-13: Inclusion of isNotNull Function
Adding isNotNull to the imported functions from @ctrlplane/db is correct, as it is used in subsequent query conditions.
32-35: Importing New Comparison Operators
The import of ComparisonOperator and FilterType from @ctrlplane/validators/conditions is appropriate for defining filter conditions.
301-307: Potential Issue with sys?.deployments Being Undefined
While accessing (sys?.deployments ?? []), if sys is null or deployments is undefined, it defaults to an empty array, resulting in no events being generated. Ensure that this is the intended behavior.
Run the following script to check if sys?.deployments could be undefined:
✅ Verification successful
Based on the schema files and type definitions found, I can now generate the final response:
The Null-Coalescing Behavior for sys?.deployments is Correct
The code's behavior of defaulting to an empty array when sys is null or deployments is undefined is the correct implementation because:
- The system schema shows that
deploymentsis a relation field defined insystemRelations, meaning it's an optional array that could be undefined if not included in the query. - When no events need to be generated (empty array case), this is the desired behavior as it safely handles cases where:
- The system doesn't exist (
sysis null) - The system exists but deployments weren't loaded in the query
- The system exists but has no deployments
- The system doesn't exist (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if `deployments` can be undefined when `sys` is present.
# Expected: All systems with non-null `deployments` property.
# Check systems where `deployments` might be undefined
ast-grep --pattern $'const sys = await $_.db.query.system.findFirst({
where: $_,
with: { $_ },
});'
Length of output: 104
Script:
#!/bin/bash
# Let's try a different approach to find system queries and their usage patterns
# First, let's find the file content around the code snippet
rg -B 10 -A 10 "const events = removedFromSystemResources\.flatMap"
# Also search for system type definitions to understand the structure
rg -l "type.*System.*=.*{"
# And look for deployment-related queries
rg -B 5 "system.*findFirst.*deployment"
Length of output: 1801
Script:
#!/bin/bash
# Let's fix the regex and try again to find system type definitions
rg -l "type\s+System\s*=\s*\{"
# Look for system query with deployments in the with clause
rg -B 5 "findFirst.*\{.*deployments"
# Check the schema file for system and deployment relationship
fd -g "*schema*" -x cat {}
Length of output: 32063
Script:
#!/bin/bash
# Let's search for the schema definition in drizzle schema files
fd -g "*.ts" -g "!*.test.ts" -g "!*.spec.ts" --full-path "./packages/db/src/schema" -x cat {}
# Also search for system type definitions in the codebase
rg "interface.*System.*{" -A 10
Length of output: 347
Script:
#!/bin/bash
# Fix the fd command to search for schema files
fd -e ts -E "*.test.ts" -E "*.spec.ts" . "./packages/db/src/schema" -x cat {}
# Fix the regex for system interface search
rg "interface\s+System\s*\{" -A 10
# Also look for system-related types and schemas
rg "type\s+System\s*=" -A 10
Length of output: 70847
| export const isResourceRemoved = (event: HookEvent): event is ResourceRemoved => | ||
| true; |
There was a problem hiding this comment.
Fix the type guard implementation - Critical Security Issue
The current implementation of isResourceRemoved always returns true, which defeats the purpose of type checking. This could lead to runtime errors if invalid events are processed as ResourceRemoved events.
Implement proper type checking:
-export const isResourceRemoved = (event: HookEvent): event is ResourceRemoved =>
- true;
+export const isResourceRemoved = (event: HookEvent): event is ResourceRemoved => {
+ return event.type === "deployment.resource.removed" &&
+ "resourceId" in event &&
+ typeof event.resourceId === "string";
+};📝 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 const isResourceRemoved = (event: HookEvent): event is ResourceRemoved => | |
| true; | |
| export const isResourceRemoved = (event: HookEvent): event is ResourceRemoved => { | |
| return event.type === "deployment.resource.removed" && | |
| "resourceId" in event && | |
| typeof event.resourceId === "string"; | |
| }; |
| import { isResourceRemoved } from "@ctrlplane/validators/events"; | ||
|
|
||
| import { handleTargetRemoved } from "./handlers/target-removed.js"; | ||
| import { handleResourceRemoved } from "./handlers/index.js"; | ||
|
|
||
| export * from "./triggers/index.js"; | ||
| export * from "./handlers/index.js"; | ||
|
|
||
| export const handleEvent = async (event: HookEvent) => { | ||
| await db.insert(SCHEMA.event).values(event); | ||
| if (isTargetRemoved(event)) return handleTargetRemoved(event); | ||
| throw new Error(`Unhandled event: ${event.action}`); | ||
| if (isResourceRemoved(event)) return handleResourceRemoved(event); | ||
| throw new Error(`Unhandled event`); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent terminology transition detected in events package
The codebase shows an incomplete transition from "target" to "resource" terminology:
packages/events/src/handlers/target-removed.tsimportsResourceRemovedtype but the file itself uses old "target" terminologypackages/events/src/triggers/target-deleted.tsstill uses "target" in filename while importing resource-related types- The event handling logic in
index.tsuses "resource" terminology (isResourceRemoved,handleResourceRemoved) but references files with "target" terminology
This inconsistency should be addressed by renaming the following files to maintain consistency:
packages/events/src/handlers/target-removed.ts→resource-removed.tspackages/events/src/triggers/target-deleted.ts→resource-deleted.ts
🔗 Analysis chain
Verify consistent terminology across codebase.
Let's ensure the terminology change from "target" to "resource" is consistent across the codebase.
Let me analyze the results more deeply with additional scripts to understand the context and nature of these occurrences.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining "target" terminology that should be "resource"
# Search for remaining "target" usage in relevant contexts
echo "Checking for remaining 'target' terminology in event-related files:"
rg -i "target.*(?:event|hook|remove|delete)" --type ts
# Search for potential inconsistencies where both terms are used
echo -e "\nChecking for files using both 'target' and 'resource' terminology:"
rg -l "target" --type ts | xargs rg -l "resource"
Length of output: 9318
Script:
#!/bin/bash
# Check the content of specific files that show the terminology transition
echo "Checking event handler implementations:"
rg -A 5 "isResourceRemoved|isTargetRemoved" --type ts
echo -e "\nChecking event type definitions:"
ast-grep --pattern 'type $_ = {
$$$
(target|resource)
$$$
}'
echo -e "\nChecking specific imports in events package:"
rg -A 2 "from.*validators/events" packages/events/src/
Length of output: 3219
| const resources = await tx.query.resource.findMany({ | ||
| where: inArray(resource.id, resourceIds), | ||
| }); | ||
| const eventsPromises = resources.map(getEventsForResourceDeleted); | ||
| const events = (await Promise.allSettled(eventsPromises)).flatMap((r) => | ||
| r.status === "fulfilled" ? r.value : [], | ||
| ); | ||
| await Promise.allSettled(events.map(handleEvent)); |
There was a problem hiding this comment.
Handle rejected promises when processing events
The current implementation uses Promise.allSettled to process event promises but does not handle rejected cases. This could lead to silent failures during event handling, potentially causing issues downstream. It's important to handle rejected promises to ensure reliability and maintainability.
Apply this diff to handle rejected promises:
+ const eventsResults = await Promise.allSettled(eventsPromises);
+ const events = eventsResults.flatMap((r) =>
+ r.status === "fulfilled" ? r.value : []
+ );
+ eventsResults
+ .filter((r) => r.status === "rejected")
+ .forEach((r) =>
+ log.error("Failed to get events for resource deletion", { error: r.reason })
+ );
- const events = (await Promise.allSettled(eventsPromises)).flatMap((r) =>
- r.status === "fulfilled" ? r.value : [],
- );
+ const handleEventResults = await Promise.allSettled(events.map(handleEvent));
+ handleEventResults
+ .filter((r) => r.status === "rejected")
+ .forEach((r) =>
+ log.error("Failed to handle event", { error: r.reason })
+ );
- await Promise.allSettled(events.map(handleEvent));Additionally, consider whether resource deletion should proceed if event handling fails. Depending on system requirements, you may need to halt the deletion to maintain data integrity if events are not processed successfully.
📝 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 resources = await tx.query.resource.findMany({ | |
| where: inArray(resource.id, resourceIds), | |
| }); | |
| const eventsPromises = resources.map(getEventsForResourceDeleted); | |
| const events = (await Promise.allSettled(eventsPromises)).flatMap((r) => | |
| r.status === "fulfilled" ? r.value : [], | |
| ); | |
| await Promise.allSettled(events.map(handleEvent)); | |
| const resources = await tx.query.resource.findMany({ | |
| where: inArray(resource.id, resourceIds), | |
| }); | |
| const eventsPromises = resources.map(getEventsForResourceDeleted); | |
| const eventsResults = await Promise.allSettled(eventsPromises); | |
| const events = eventsResults.flatMap((r) => | |
| r.status === "fulfilled" ? r.value : [] | |
| ); | |
| eventsResults | |
| .filter((r) => r.status === "rejected") | |
| .forEach((r) => | |
| log.error("Failed to get events for resource deletion", { error: r.reason }) | |
| ); | |
| const handleEventResults = await Promise.allSettled(events.map(handleEvent)); | |
| handleEventResults | |
| .filter((r) => r.status === "rejected") | |
| .forEach((r) => | |
| log.error("Failed to handle event", { error: r.reason }) | |
| ); |
| newQuery, | ||
| oldQuery && not(oldQuery), |
There was a problem hiding this comment.
Logical Error in Query Conditions
The condition oldQuery && not(oldQuery) will always evaluate to false, resulting in no resources being returned. This is likely a logical error in constructing the query for newResources.
To find resources that match newQuery and do not match oldQuery, the condition should be:
newQuery,
- oldQuery && not(oldQuery),
+ not(oldQuery),So the corrected query becomes:
where: and(
eq(resource.workspaceId, oldEnv.system.workspaceId),
newQuery,
not(oldQuery),
),| oldQuery, | ||
| newQuery && not(newQuery), |
There was a problem hiding this comment.
Logical Error in Query Conditions
Similarly, the condition newQuery && not(newQuery) will always evaluate to false, resulting in no resources being returned for removedResources. This should be corrected.
To find resources that match oldQuery and do not match newQuery, the condition should be:
oldQuery,
- newQuery && not(newQuery),
+ not(newQuery),So the corrected query becomes:
where: and(
eq(resource.workspaceId, oldEnv.system.workspaceId),
oldQuery,
not(newQuery),
),| const sys = await ctx.db.query.system.findFirst({ | ||
| where: eq(system.id, oldEnv.system.id), | ||
| with: { environments: { where: isOtherEnv }, deployments: true }, | ||
| }); |
There was a problem hiding this comment.
Error Handling for sys Variable
Ensure that the sys variable is properly handled in case findFirst returns null. This will prevent potential runtime errors when accessing properties of sys later in the code.
Consider adding a check or default value:
const sys = await ctx.db.query.system.findFirst({
where: eq(system.id, oldEnv.system.id),
with: { environments: { where: isOtherEnv }, deployments: true },
});
+ if (!sys) {
+ throw new Error(`System with ID ${oldEnv.system.id} not found.`);
+ }📝 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 sys = await ctx.db.query.system.findFirst({ | |
| where: eq(system.id, oldEnv.system.id), | |
| with: { environments: { where: isOtherEnv }, deployments: true }, | |
| }); | |
| const sys = await ctx.db.query.system.findFirst({ | |
| where: eq(system.id, oldEnv.system.id), | |
| with: { environments: { where: isOtherEnv }, deployments: true }, | |
| }); | |
| if (!sys) { | |
| throw new Error(`System with ID ${oldEnv.system.id} not found.`); | |
| } |
| const isOtherEnv = and( | ||
| isNotNull(environment.resourceFilter), | ||
| ne(environment.id, input.id), | ||
| ); | ||
| const sys = await ctx.db.query.system.findFirst({ | ||
| where: eq(system.id, oldEnv.system.id), | ||
| with: { environments: { where: isOtherEnv }, deployments: true }, | ||
| }); |
There was a problem hiding this comment.
| const isOtherEnv = and( | |
| isNotNull(environment.resourceFilter), | |
| ne(environment.id, input.id), | |
| ); | |
| const sys = await ctx.db.query.system.findFirst({ | |
| where: eq(system.id, oldEnv.system.id), | |
| with: { environments: { where: isOtherEnv }, deployments: true }, | |
| }); | |
| const where = and( | |
| isNotNull(environment.resourceFilter), | |
| ne(environment.id, input.id), | |
| ); | |
| const sys = await ctx.db.query.system.findFirst({ | |
| where: eq(system.id, oldEnv.system.id), | |
| with: { environments: { where }, deployments: true }, | |
| }); |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores