Skip to content
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

Merged
merged 3 commits into from
Nov 18, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 61 additions & 1 deletion packages/api/src/router/environment.ts
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";
Expand All @@ -8,6 +9,8 @@ import {
buildConflictUpdateColumns,
eq,
inArray,
isNotNull,
ne,
not,
takeFirst,
} from "@ctrlplane/db";
Expand All @@ -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";
Expand Down Expand Up @@ -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 +248 to +251
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.`);
}

Comment on lines +244 to +251
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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 },
});


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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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),
),

),
);

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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),
),

),
});

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,
Expand Down
4 changes: 2 additions & 2 deletions packages/api/src/router/resources.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import {
takeFirstOrNull,
} from "@ctrlplane/db";
import * as schema from "@ctrlplane/db/schema";
import { getEventsForTargetDeleted, handleEvent } from "@ctrlplane/events";
import { getEventsForResourceDeleted, handleEvent } from "@ctrlplane/events";
import {
cancelOldReleaseJobTriggersOnJobDispatch,
createJobApprovals,
Expand Down Expand Up @@ -582,7 +582,7 @@ export const resourceRouter = createTRPCRouter({
where: inArray(schema.resource.id, input),
});
const events = (
await Promise.allSettled(resources.map(getEventsForTargetDeleted))
await Promise.allSettled(resources.map(getEventsForResourceDeleted))
).flatMap((r) => (r.status === "fulfilled" ? r.value : []));
await Promise.allSettled(events.map(handleEvent));

Expand Down
16 changes: 8 additions & 8 deletions packages/events/src/handlers/target-removed.ts
Original file line number Diff line number Diff line change
@@ -1,28 +1,28 @@
import type { TargetRemoved } from "@ctrlplane/validators/events";
import type { ResourceRemoved } from "@ctrlplane/validators/events";

import { and, eq } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import * as SCHEMA from "@ctrlplane/db/schema";
import { dispatchRunbook } from "@ctrlplane/job-dispatch";

export const handleTargetRemoved = async (event: TargetRemoved) => {
const { target, deployment } = event.payload;
export const handleResourceRemoved = async (event: ResourceRemoved) => {
const { resource, deployment } = event.payload;

const isSubscribedToTargetRemoved = and(
const isSubscribedToResourceRemoved = and(
eq(SCHEMA.hook.scopeId, deployment.id),
eq(SCHEMA.hook.scopeType, "deployment"),
eq(SCHEMA.hook.action, "deployment.target.removed"),
eq(SCHEMA.hook.action, "deployment.resource.removed"),
);
const runhooks = await db
.select()
.from(SCHEMA.runhook)
.innerJoin(SCHEMA.hook, eq(SCHEMA.runhook.hookId, SCHEMA.hook.id))
.where(isSubscribedToTargetRemoved);
.where(isSubscribedToResourceRemoved);

const targetId = target.id;
const resourceId = resource.id;
const deploymentId = deployment.id;
const handleRunhooksPromises = runhooks.map(({ runhook }) =>
dispatchRunbook(db, runhook.runbookId, { targetId, deploymentId }),
dispatchRunbook(db, runhook.runbookId, { resourceId, deploymentId }),
);

await Promise.all(handleRunhooksPromises);
Expand Down
8 changes: 4 additions & 4 deletions packages/events/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The 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:

  • packages/events/src/handlers/target-removed.ts imports ResourceRemoved type but the file itself uses old "target" terminology
  • packages/events/src/triggers/target-deleted.ts still uses "target" in filename while importing resource-related types
  • The event handling logic in index.ts uses "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.tsresource-removed.ts
  • packages/events/src/triggers/target-deleted.tsresource-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

};
8 changes: 4 additions & 4 deletions packages/events/src/triggers/deployment-deleted.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,12 @@ export const getEventsForDeploymentDeleted = async (
conditions: envFilters,
};

const targets = await db.query.resource.findMany({
const resources = await db.query.resource.findMany({
where: SCHEMA.resourceMatchesMetadata(db, systemFilter),
});

return targets.map((target) => ({
action: "deployment.target.removed",
payload: { deployment, target },
return resources.map((resource) => ({
action: "deployment.resource.removed",
payload: { deployment, resource },
}));
};
18 changes: 9 additions & 9 deletions packages/events/src/triggers/environment-deleted.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ export const getEventsForEnvironmentDeleted = async (
environment: SCHEMA.Environment,
): Promise<HookEvent[]> => {
if (environment.resourceFilter == null) return [];
const targets = await db
const resources = await db
.select()
.from(SCHEMA.resource)
.where(SCHEMA.resourceMatchesMetadata(db, environment.resourceFilter));
if (targets.length === 0) return [];
if (resources.length === 0) return [];

const checks = and(
isNotNull(SCHEMA.environment.resourceFilter),
Expand All @@ -39,7 +39,7 @@ export const getEventsForEnvironmentDeleted = async (
conditions: envFilters,
};

const removedFromSystemTargets =
const removedFromSystemResources =
envFilters.length > 0
? await db
.select()
Expand All @@ -49,17 +49,17 @@ export const getEventsForEnvironmentDeleted = async (
SCHEMA.resourceMatchesMetadata(db, removedFromSystemFilter),
inArray(
SCHEMA.resource.id,
targets.map((t) => t.id),
resources.map((r) => r.id),
),
),
)
: targets;
if (removedFromSystemTargets.length === 0) return [];
: resources;
if (removedFromSystemResources.length === 0) return [];

return system.deployments.flatMap((deployment) =>
removedFromSystemTargets.map((target) => ({
action: "deployment.target.removed",
payload: { deployment, target },
removedFromSystemResources.map((resource) => ({
action: "deployment.resource.removed",
payload: { deployment, resource },
})),
);
};
22 changes: 11 additions & 11 deletions packages/events/src/triggers/target-deleted.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,16 @@ import { ComparisonOperator } from "@ctrlplane/validators/conditions";
import { ResourceFilterType } from "@ctrlplane/validators/resources";

/**
* Get events for a target that has been deleted.
* NOTE: Because we may need to do inner joins on target metadata for the filter,
* this actually needs to be called before the target is actually deleted.
* @param target
* 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.
* @param resource
*/
export const getEventsForTargetDeleted = async (
target: SCHEMA.Resource,
export const getEventsForResourceDeleted = async (
resource: SCHEMA.Resource,
): Promise<HookEvent[]> => {
const systems = await db.query.system.findMany({
where: eq(SCHEMA.system.workspaceId, target.workspaceId),
where: eq(SCHEMA.system.workspaceId, resource.workspaceId),
with: {
environments: { where: isNotNull(SCHEMA.environment.resourceFilter) },
deployments: true,
Expand All @@ -36,17 +36,17 @@ export const getEventsForTargetDeleted = async (
conditions: filters,
};

const matchedTarget = await db.query.resource.findFirst({
const matchedResource = await db.query.resource.findFirst({
where: SCHEMA.resourceMatchesMetadata(db, systemFilter),
});
if (matchedTarget == null) return [];
if (matchedResource == null) return [];

return s.deployments;
});
const deployments = (await Promise.all(deploymentPromises)).flat();

return deployments.map((deployment) => ({
action: "deployment.target.removed",
payload: { target, deployment },
action: "deployment.resource.removed",
payload: { resource, deployment },
}));
};
16 changes: 6 additions & 10 deletions packages/validators/src/events/hooks/index.ts
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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";
};


// 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[]]);
16 changes: 5 additions & 11 deletions packages/validators/src/events/hooks/target.ts
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>;
Loading