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.

Sign up for GitHub

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: Soft delete resources #223

Merged
merged 2 commits into from
Nov 21, 2024
Merged

fix: Soft delete resources #223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c65e731
fix: Soft delete resources
adityachoudhari26 Nov 20, 2024
7f3c290
some cleanup
adityachoudhari26 Nov 21, 2024
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
3 changes: 2 additions & 1 deletion apps/webservice/src/app/api/v1/job-agents/[agentId]/jobs/running/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { NextRequest } from "next/server";
import { NextResponse } from "next/server";

import { and, eq, notInArray } from "@ctrlplane/db";
import { and, eq, isNull, notInArray } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import {
deployment,
Expand Down Expand Up @@ -40,6 +40,7 @@ export const GET = async (
"completed",
"invalid_job_agent",
]),
isNull(resource.deletedAt),
),
)
.then((rows) =>
Expand Down
4 changes: 2 additions & 2 deletions apps/webservice/src/app/api/v1/jobs/[jobId]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { NextRequest } from "next/server";
import { NextResponse } from "next/server";

import { and, eq, takeFirstOrNull } from "@ctrlplane/db";
import { and, eq, isNull, takeFirstOrNull } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import {
deployment,
Expand Down Expand Up @@ -83,7 +83,7 @@ export const GET = request()
.leftJoin(resource, eq(resource.id, releaseJobTrigger.resourceId))
.leftJoin(release, eq(release.id, releaseJobTrigger.releaseId))
.leftJoin(deployment, eq(deployment.id, release.deploymentId))
.where(eq(job.id, params.jobId))
.where(and(eq(job.id, params.jobId), isNull(resource.deletedAt)))
.then(takeFirstOrNull);

if (row == null)
Expand Down
3 changes: 2 additions & 1 deletion apps/webservice/src/app/api/v1/relationship/deployment-to-resource/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { z } from "zod";

import { and, eq, takeFirstOrNull } from "@ctrlplane/db";
import { and, eq, isNull, takeFirstOrNull } from "@ctrlplane/db";
import * as SCHEMA from "@ctrlplane/db/schema";

import { authn } from "../../auth";
Expand All @@ -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),
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

Soft delete check should be added for deployments

Based on the code analysis, deployments do support soft delete functionality, as evidenced by:

  1. The deployment-deleted trigger file which handles soft delete events
  2. 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

),
)
.then(takeFirstOrNull);
Expand Down
4 changes: 3 additions & 1 deletion apps/webservice/src/app/api/v1/relationship/resource-to-resource/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { z } from "zod";

import { and, eq, takeFirstOrNull } from "@ctrlplane/db";
import { and, eq, isNull, takeFirstOrNull } from "@ctrlplane/db";
import * as SCHEMA from "@ctrlplane/db/schema";

import { authn } from "../../auth";
Expand Down Expand Up @@ -28,6 +28,7 @@ export const POST = request()
and(
eq(SCHEMA.resource.identifier, body.fromIdentifier),
eq(SCHEMA.resource.workspaceId, body.workspaceId),
isNull(SCHEMA.resource.deletedAt),
),
)
.then(takeFirstOrNull);
Expand All @@ -44,6 +45,7 @@ export const POST = request()
and(
eq(SCHEMA.resource.identifier, body.toIdentifier),
eq(SCHEMA.resource.workspaceId, body.workspaceId),
isNull(SCHEMA.resource.deletedAt),
),
)
.then(takeFirstOrNull);
Expand Down
30 changes: 13 additions & 17 deletions apps/webservice/src/app/api/v1/resources/[resourceId]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ import { NextResponse } from "next/server";
import _ from "lodash";
import { z } from "zod";

import { eq } from "@ctrlplane/db";
import { and, eq, isNull } from "@ctrlplane/db";
import * as schema from "@ctrlplane/db/schema";
import { upsertResources } from "@ctrlplane/job-dispatch";
import { deleteResources, upsertResources } from "@ctrlplane/job-dispatch";
import { variablesAES256 } from "@ctrlplane/secrets";
import { Permission } from "@ctrlplane/validators/auth";

Expand All @@ -22,13 +22,10 @@ export const GET = request()
}),
)
.handle(async ({ db }, { params }: { params: { resourceId: string } }) => {
// we don't check deletedAt as we may be querying for soft-deleted resources
const data = await db.query.resource.findFirst({
where: eq(schema.resource.id, params.resourceId),
with: {
metadata: true,
variables: true,
provider: true,
},
with: { metadata: true, variables: true, provider: true },
});

if (data == null)
Expand Down Expand Up @@ -85,9 +82,10 @@ export const PATCH = request()
{ body: z.infer<typeof patchSchema> },
{ params: { resourceId: string } }
>(async ({ db, body }, { params }) => {
const resource = await db.query.resource.findFirst({
where: eq(schema.resource.id, params.resourceId),
});
const isResource = eq(schema.resource.id, params.resourceId);
const isNotDeleted = isNull(schema.resource.deletedAt);
const where = and(isResource, isNotDeleted);
const resource = await db.query.resource.findFirst({ where });

if (resource == null)
return NextResponse.json(
Expand All @@ -110,19 +108,17 @@ export const DELETE = request()
),
)
.handle(async ({ db }, { params }: { params: { resourceId: string } }) => {
const resource = await db.query.resource.findFirst({
where: eq(schema.resource.id, params.resourceId),
});
const isResource = eq(schema.resource.id, params.resourceId);
const isNotDeleted = isNull(schema.resource.deletedAt);
const where = and(isResource, isNotDeleted);
const resource = await db.query.resource.findFirst({ where });

if (resource == null)
return NextResponse.json(
{ error: "Resource not found" },
{ status: 404 },
);

await db
.delete(schema.resource)
.where(eq(schema.resource.id, params.resourceId));

await deleteResources(db, [resource]);
return NextResponse.json({ success: true });
});
19 changes: 12 additions & 7 deletions ...ervice/src/app/api/v1/workspaces/[workspaceId]/resources/identifier/[identifier]/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { NextResponse } from "next/server";

import { and, eq } from "@ctrlplane/db";
import { and, eq, isNull } from "@ctrlplane/db";
import { db } from "@ctrlplane/db/client";
import * as schema from "@ctrlplane/db/schema";
import { deleteResources } from "@ctrlplane/job-dispatch";
import { Permission } from "@ctrlplane/validators/auth";

import { authn, authz } from "~/app/api/v1/auth";
Expand All @@ -14,21 +15,23 @@ export const GET = request()
authz(async ({ can, extra }) => {
const { workspaceId, identifier } = extra;

const target = await db.query.resource.findFirst({
// we don't check deletedAt as we may be querying for soft-deleted resources
const resource = await db.query.resource.findFirst({
where: and(
eq(schema.resource.workspaceId, workspaceId),
eq(schema.resource.identifier, identifier),
),
});

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 });
}),
)
.handle<unknown, { params: { workspaceId: string; identifier: string } }>(
async (_, { params }) => {
// we don't check deletedAt as we may be querying for soft-deleted resources
const data = await db.query.resource.findFirst({
where: and(
eq(schema.resource.workspaceId, params.workspaceId),
Expand Down Expand Up @@ -66,6 +69,7 @@ export const DELETE = request()
where: and(
eq(schema.resource.workspaceId, workspaceId),
eq(schema.resource.identifier, identifier),
isNull(schema.resource.deletedAt),
),
});

Expand All @@ -77,21 +81,22 @@ export const DELETE = request()
)
.handle<unknown, { params: { workspaceId: string; identifier: string } }>(
async (_, { params }) => {
const target = await db.query.resource.findFirst({
const resource = await db.query.resource.findFirst({
where: and(
eq(schema.resource.workspaceId, params.workspaceId),
eq(schema.resource.identifier, params.identifier),
isNull(schema.resource.deletedAt),
),
});

if (target == null) {
if (resource == null) {
return NextResponse.json(
{ error: `Target not found for identifier: ${params.identifier}` },
{ status: 404 },
);
}
Comment on lines +92 to 97
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
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, target.id));
await deleteResources(db, [resource]);

return NextResponse.json({ success: true });
},
Expand Down
14 changes: 11 additions & 3 deletions packages/api/src/router/deployment-variable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
asc,
eq,
isNotNull,
isNull,
ne,
sql,
takeFirst,
Expand Down Expand Up @@ -214,11 +215,17 @@ const valueRouter = createTRPCRouter({
};

const oldTargets = await ctx.db.query.resource.findMany({
where: resourceMatchesMetadata(ctx.db, oldTargetFilter),
where: and(
resourceMatchesMetadata(ctx.db, oldTargetFilter),
isNull(resource.deletedAt),
),
});

const newTargets = await ctx.db.query.resource.findMany({
where: resourceMatchesMetadata(ctx.db, newTargetFilter),
where: and(
resourceMatchesMetadata(ctx.db, newTargetFilter),
isNull(resource.deletedAt),
),
});

const oldTargetIds = new Set(oldTargets.map((t) => t.id));
Expand Down Expand Up @@ -299,7 +306,6 @@ export const deploymentVariableRouter = createTRPCRouter({
const deploymentVariables = await ctx.db
.select()
.from(resource)
.where(eq(resource.id, input))
.innerJoin(system, eq(resource.workspaceId, system.workspaceId))
.innerJoin(deployment, eq(deployment.systemId, system.id))
.innerJoin(
Expand All @@ -310,6 +316,7 @@ export const deploymentVariableRouter = createTRPCRouter({
deploymentVariableValue,
eq(deploymentVariableValue.variableId, deploymentVariable.id),
)
.where(and(eq(resource.id, input), isNull(resource.deletedAt)))
.then((rows) =>
_.chain(rows)
.groupBy((r) => r.deployment_variable.id)
Expand All @@ -331,6 +338,7 @@ export const deploymentVariableRouter = createTRPCRouter({
.where(
and(
eq(resource.id, input),
isNull(resource.deletedAt),
resourceMatchesMetadata(ctx.db, targetFilter),
),
)
Expand Down
5 changes: 4 additions & 1 deletion packages/api/src/router/deployment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
eq,
inArray,
isNotNull,
isNull,
sql,
takeFirst,
takeFirstOrNull,
Expand Down Expand Up @@ -417,6 +418,7 @@ export const deploymentRouter = createTRPCRouter({
and(
eq(release.deploymentId, input),
eq(latestJobsPerResource.rank, 1),
isNull(resource.deletedAt),
),
)
.then((r) =>
Expand Down Expand Up @@ -620,7 +622,7 @@ export const deploymentRouter = createTRPCRouter({
const tg = await ctx.db
.select()
.from(resource)
.where(eq(resource.id, input))
.where(and(eq(resource.id, input), isNull(resource.deletedAt)))
.then(takeFirst);

const envs = await ctx.db
Expand Down Expand Up @@ -659,6 +661,7 @@ export const deploymentRouter = createTRPCRouter({
.where(
and(
eq(resource.id, input),
isNull(resource.deletedAt),
inArray(job.status, [
JobStatus.Completed,
JobStatus.Pending,
Expand Down
15 changes: 11 additions & 4 deletions packages/api/src/router/environment.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
eq,
inArray,
isNotNull,
isNull,
ne,
not,
takeFirst,
Expand Down Expand Up @@ -170,9 +171,12 @@ export const environmentRouter = createTRPCRouter({
.select()
.from(resource)
.where(
resourceMatchesMetadata(
ctx.db,
e.environment.resourceFilter,
and(
isNull(resource.deletedAt),
resourceMatchesMetadata(
ctx.db,
e.environment.resourceFilter,
),
),
)
: [],
Expand Down Expand Up @@ -269,6 +273,7 @@ export const environmentRouter = createTRPCRouter({
.where(
and(
eq(resource.workspaceId, oldEnv.system.workspaceId),
isNull(resource.deletedAt),
newQuery,
oldQuery && not(oldQuery),
),
Expand All @@ -277,6 +282,7 @@ export const environmentRouter = createTRPCRouter({
const removedResources = await ctx.db.query.resource.findMany({
where: and(
eq(resource.workspaceId, oldEnv.system.workspaceId),
isNull(resource.deletedAt),
oldQuery,
newQuery && not(newQuery),
),
Expand All @@ -299,10 +305,11 @@ export const environmentRouter = createTRPCRouter({
otherEnvFilters.length > 0
? resourceMatchesMetadata(ctx.db, sysFilter)
: undefined;
const isNotDeleted = isNull(resource.deletedAt);

const removedFromSystemResources =
await ctx.db.query.resource.findMany({
where: and(isRemovedFromEnv, isRemovedFromSystem),
where: and(isRemovedFromEnv, isRemovedFromSystem, isNotDeleted),
});

const events = removedFromSystemResources.flatMap((resource) =>
Expand Down
Loading
Loading