-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Hooks reference resource + trigger hooks on filter change for env #220
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,4 +1,5 @@ | ||||||||||||||||||||||||||||||||||
import type { Tx } from "@ctrlplane/db"; | ||||||||||||||||||||||||||||||||||
import type { ResourceCondition } from "@ctrlplane/validators/resources"; | ||||||||||||||||||||||||||||||||||
import _ from "lodash"; | ||||||||||||||||||||||||||||||||||
import { isPresent } from "ts-is-present"; | ||||||||||||||||||||||||||||||||||
import { z } from "zod"; | ||||||||||||||||||||||||||||||||||
|
@@ -8,6 +9,8 @@ import { | |||||||||||||||||||||||||||||||||
buildConflictUpdateColumns, | ||||||||||||||||||||||||||||||||||
eq, | ||||||||||||||||||||||||||||||||||
inArray, | ||||||||||||||||||||||||||||||||||
isNotNull, | ||||||||||||||||||||||||||||||||||
ne, | ||||||||||||||||||||||||||||||||||
not, | ||||||||||||||||||||||||||||||||||
takeFirst, | ||||||||||||||||||||||||||||||||||
} from "@ctrlplane/db"; | ||||||||||||||||||||||||||||||||||
|
@@ -26,6 +29,10 @@ import { | |||||||||||||||||||||||||||||||||
import { getEventsForEnvironmentDeleted, handleEvent } from "@ctrlplane/events"; | ||||||||||||||||||||||||||||||||||
import { dispatchJobsForNewResources } from "@ctrlplane/job-dispatch"; | ||||||||||||||||||||||||||||||||||
import { Permission } from "@ctrlplane/validators/auth"; | ||||||||||||||||||||||||||||||||||
import { | ||||||||||||||||||||||||||||||||||
ComparisonOperator, | ||||||||||||||||||||||||||||||||||
FilterType, | ||||||||||||||||||||||||||||||||||
} from "@ctrlplane/validators/conditions"; | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
import { createTRPCRouter, protectedProcedure } from "../trpc"; | ||||||||||||||||||||||||||||||||||
import { policyRouter } from "./environment-policy"; | ||||||||||||||||||||||||||||||||||
|
@@ -234,21 +241,74 @@ export const environmentRouter = createTRPCRouter({ | |||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
if (hasResourceFiltersChanged) { | ||||||||||||||||||||||||||||||||||
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 }, | ||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||
Comment on lines
+244
to
+251
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
const otherEnvFilters = | ||||||||||||||||||||||||||||||||||
sys?.environments.map((e) => e.resourceFilter).filter(isPresent) ?? | ||||||||||||||||||||||||||||||||||
[]; | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
const oldQuery = resourceMatchesMetadata( | ||||||||||||||||||||||||||||||||||
ctx.db, | ||||||||||||||||||||||||||||||||||
oldEnv.environment.resourceFilter, | ||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||
const newQuery = resourceMatchesMetadata(ctx.db, resourceFilter); | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
const newResources = await ctx.db | ||||||||||||||||||||||||||||||||||
.select({ id: resource.id }) | ||||||||||||||||||||||||||||||||||
.from(resource) | ||||||||||||||||||||||||||||||||||
.where( | ||||||||||||||||||||||||||||||||||
and( | ||||||||||||||||||||||||||||||||||
eq(resource.workspaceId, oldEnv.system.workspaceId), | ||||||||||||||||||||||||||||||||||
resourceMatchesMetadata(ctx.db, resourceFilter), | ||||||||||||||||||||||||||||||||||
newQuery, | ||||||||||||||||||||||||||||||||||
oldQuery && not(oldQuery), | ||||||||||||||||||||||||||||||||||
Comment on lines
+269
to
270
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logical Error in Query Conditions The condition To find resources that match newQuery,
- oldQuery && not(oldQuery),
+ not(oldQuery), So the corrected query becomes: where: and(
eq(resource.workspaceId, oldEnv.system.workspaceId),
newQuery,
not(oldQuery),
), |
||||||||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
const removedResources = await ctx.db.query.resource.findMany({ | ||||||||||||||||||||||||||||||||||
where: and( | ||||||||||||||||||||||||||||||||||
eq(resource.workspaceId, oldEnv.system.workspaceId), | ||||||||||||||||||||||||||||||||||
oldQuery, | ||||||||||||||||||||||||||||||||||
newQuery && not(newQuery), | ||||||||||||||||||||||||||||||||||
Comment on lines
+277
to
+278
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Logical Error in Query Conditions Similarly, the condition To find resources that match oldQuery,
- newQuery && not(newQuery),
+ not(newQuery), So the corrected query becomes: where: and(
eq(resource.workspaceId, oldEnv.system.workspaceId),
oldQuery,
not(newQuery),
), |
||||||||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
if (removedResources.length > 0) { | ||||||||||||||||||||||||||||||||||
const sysFilter: ResourceCondition = { | ||||||||||||||||||||||||||||||||||
type: FilterType.Comparison, | ||||||||||||||||||||||||||||||||||
operator: ComparisonOperator.Or, | ||||||||||||||||||||||||||||||||||
not: true, | ||||||||||||||||||||||||||||||||||
conditions: otherEnvFilters, | ||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
const removedFromSystemResources = | ||||||||||||||||||||||||||||||||||
await ctx.db.query.resource.findMany({ | ||||||||||||||||||||||||||||||||||
where: and( | ||||||||||||||||||||||||||||||||||
inArray( | ||||||||||||||||||||||||||||||||||
resource.id, | ||||||||||||||||||||||||||||||||||
removedResources.map((r) => r.id), | ||||||||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||||||||
resourceMatchesMetadata(ctx.db, sysFilter), | ||||||||||||||||||||||||||||||||||
), | ||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
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); | ||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||
if (newResources.length > 0) { | ||||||||||||||||||||||||||||||||||
await dispatchJobsForNewResources( | ||||||||||||||||||||||||||||||||||
ctx.db, | ||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,15 +2,15 @@ import type { HookEvent } from "@ctrlplane/validators/events"; | |
|
||
import { db } from "@ctrlplane/db/client"; | ||
import * as SCHEMA from "@ctrlplane/db/schema"; | ||
import { isTargetRemoved } from "@ctrlplane/validators/events"; | ||
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`); | ||
Comment on lines
+5
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Inconsistent terminology transition detected in events package The codebase shows an incomplete transition from "target" to "resource" terminology:
This inconsistency should be addressed by renaming the following files to maintain consistency:
🔗 Analysis chainVerify 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 executedThe 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 |
||
}; |
Original file line number | Diff line number | Diff line change | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,21 +1,17 @@ | ||||||||||||||||
import { z } from "zod"; | ||||||||||||||||
|
||||||||||||||||
import type { TargetDeleted, TargetRemoved } from "./target.js"; | ||||||||||||||||
import { targetDeleted, targetRemoved } from "./target.js"; | ||||||||||||||||
import type { ResourceRemoved } from "./target.js"; | ||||||||||||||||
import { resourceRemoved } from "./target.js"; | ||||||||||||||||
|
||||||||||||||||
export * from "./target.js"; | ||||||||||||||||
|
||||||||||||||||
export const hookEvent = z.union([targetRemoved, targetDeleted]); | ||||||||||||||||
export const hookEvent = resourceRemoved; | ||||||||||||||||
export type HookEvent = z.infer<typeof hookEvent>; | ||||||||||||||||
|
||||||||||||||||
// typeguards | ||||||||||||||||
export const isTargetRemoved = (event: HookEvent): event is TargetRemoved => | ||||||||||||||||
event.action === "deployment.target.removed"; | ||||||||||||||||
export const isTargetDeleted = (event: HookEvent): event is TargetDeleted => | ||||||||||||||||
event.action === "deployment.target.deleted"; | ||||||||||||||||
export const isResourceRemoved = (event: HookEvent): event is ResourceRemoved => | ||||||||||||||||
true; | ||||||||||||||||
Comment on lines
+12
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix the type guard implementation - Critical Security Issue The current implementation of 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
Suggested change
|
||||||||||||||||
|
||||||||||||||||
// action | ||||||||||||||||
export const hookActionsList = hookEvent.options.map( | ||||||||||||||||
(schema) => schema.shape.action.value, | ||||||||||||||||
); | ||||||||||||||||
export const hookActionsList = ["deployment.resource.removed"]; | ||||||||||||||||
export const hookActions = z.enum(hookActionsList as [string, ...string[]]); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,14 @@ | ||
import { z } from "zod"; | ||
|
||
const deployment = z.object({ id: z.string().uuid(), name: z.string() }); | ||
const target = z.object({ | ||
const resource = z.object({ | ||
id: z.string().uuid(), | ||
name: z.string(), | ||
config: z.record(z.any()), | ||
}); | ||
|
||
export const targetRemoved = z.object({ | ||
action: z.literal("deployment.target.removed"), | ||
payload: z.object({ deployment, target }), | ||
export const resourceRemoved = z.object({ | ||
action: z.literal("deployment.resource.removed"), | ||
payload: z.object({ deployment, resource }), | ||
}); | ||
export type TargetRemoved = z.infer<typeof targetRemoved>; | ||
|
||
export const targetDeleted = z.object({ | ||
action: z.literal("deployment.target.deleted"), | ||
payload: z.object({ target }), | ||
}); | ||
export type TargetDeleted = z.infer<typeof targetDeleted>; | ||
export type ResourceRemoved = z.infer<typeof resourceRemoved>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error Handling for
sys
VariableEnsure that the
sys
variable is properly handled in casefindFirst
returnsnull
. This will prevent potential runtime errors when accessing properties ofsys
later in the code.Consider adding a check or default value:
📝 Committable suggestion