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: Init resource relationship routes #219

Merged
merged 4 commits into from
Nov 18, 2024
Merged

fix: Init resource relationship routes #219

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
57de45f
fix: Init resource relationship routes
adityachoudhari26 Nov 18, 2024
e7c526c
Merge branch 'main' of github.com:sizzldev/ctrlplane into init-relati…
adityachoudhari26 Nov 18, 2024
2c3b4da
add migration
adityachoudhari26 Nov 18, 2024
17a9569
cleanup
adityachoudhari26 Nov 18, 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
58 changes: 58 additions & 0 deletions 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
@@ -0,0 +1,58 @@
import { z } from "zod";

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

import { authn } from "../../auth";
import { parseBody } from "../../body-parser";
import { request } from "../../middleware";

const bodySchema = z.object({
workspaceId: z.string().uuid(),
deploymentId: z.string().uuid(),
resourceIdentifier: z.string(),
});

export const POST = request()
.use(authn)
.use(parseBody(bodySchema))
.handle<{ body: z.infer<typeof bodySchema> }>(async (ctx) => {
const { body, db } = ctx;

const resource = await db
.select()
.from(SCHEMA.resource)
.where(
and(
eq(SCHEMA.resource.identifier, body.resourceIdentifier),
eq(SCHEMA.resource.workspaceId, body.workspaceId),
),
)
.then(takeFirstOrNull);
if (!resource)
return Response.json({ error: "Resource not found" }, { status: 404 });
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

Enhance error handling and add transaction support

Two suggestions for improvement:

  1. The error message could be more descriptive by including the resourceIdentifier.
  2. The entire operation should be wrapped in a transaction to ensure data consistency.
+    const result = await db.transaction(async (tx) => {
     const resource = await db
       .select()
       .from(SCHEMA.resource)
       .where(
         and(
           eq(SCHEMA.resource.identifier, body.resourceIdentifier),
           eq(SCHEMA.resource.workspaceId, body.workspaceId),
         ),
       )
       .then(takeFirstOrNull);
     if (!resource)
-      return Response.json({ error: "Resource not found" }, { status: 404 });
+      return Response.json(
+        { error: `Resource not found: ${body.resourceIdentifier}` },
+        { status: 404 }
+      );

Committable suggestion skipped: line range outside the PR's diff.


const deployment = await db
.select()
.from(SCHEMA.deployment)
.innerJoin(
SCHEMA.system,
eq(SCHEMA.deployment.systemId, SCHEMA.system.id),
)
.where(
and(
eq(SCHEMA.deployment.id, body.deploymentId),
eq(SCHEMA.system.workspaceId, body.workspaceId),
),
)
.then(takeFirstOrNull);
if (!deployment)
return Response.json({ error: "Deployment not found" }, { status: 404 });

await db
.insert(SCHEMA.deploymentResourceRelationship)
.values(body)
.returning();

return Response.json(body);
});
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

Add uniqueness check and improve response data

Several improvements needed for the relationship creation:

  1. Check for existing relationships to prevent duplicates
  2. Return the created record instead of the request body
  3. Add error handling for potential constraint violations
     await db
+      .select()
+      .from(SCHEMA.deploymentResourceRelationship)
+      .where(
+        and(
+          eq(SCHEMA.deploymentResourceRelationship.deploymentId, body.deploymentId),
+          eq(SCHEMA.deploymentResourceRelationship.resourceIdentifier, body.resourceIdentifier)
+        )
+      )
+      .then(takeFirstOrNull)
+      .then((existing) => {
+        if (existing) {
+          return Response.json(
+            { error: "Relationship already exists" },
+            { status: 409 }
+          );
+        }
+      });
+
+    try {
+      const [created] = await db
       .insert(SCHEMA.deploymentResourceRelationship)
       .values(body)
-      .returning();
+      .returning({
+        id: SCHEMA.deploymentResourceRelationship.id,
+        workspaceId: SCHEMA.deploymentResourceRelationship.workspaceId,
+        deploymentId: SCHEMA.deploymentResourceRelationship.deploymentId,
+        resourceIdentifier: SCHEMA.deploymentResourceRelationship.resourceIdentifier,
+        createdAt: SCHEMA.deploymentResourceRelationship.createdAt
+      });
 
-    return Response.json(body);
+      return Response.json(created);
+    } catch (error) {
+      return Response.json(
+        { error: "Failed to create relationship" },
+        { status: 500 }
+      );
+    }
📝 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
await db
.insert(SCHEMA.deploymentResourceRelationship)
.values(body)
.returning();
return Response.json(body);
});
await db
.select()
.from(SCHEMA.deploymentResourceRelationship)
.where(
and(
eq(SCHEMA.deploymentResourceRelationship.deploymentId, body.deploymentId),
eq(SCHEMA.deploymentResourceRelationship.resourceIdentifier, body.resourceIdentifier)
)
)
.then(takeFirstOrNull)
.then((existing) => {
if (existing) {
return Response.json(
{ error: "Relationship already exists" },
{ status: 409 }
);
}
});
try {
const [created] = await db
.insert(SCHEMA.deploymentResourceRelationship)
.values(body)
.returning({
id: SCHEMA.deploymentResourceRelationship.id,
workspaceId: SCHEMA.deploymentResourceRelationship.workspaceId,
deploymentId: SCHEMA.deploymentResourceRelationship.deploymentId,
resourceIdentifier: SCHEMA.deploymentResourceRelationship.resourceIdentifier,
createdAt: SCHEMA.deploymentResourceRelationship.createdAt
});
return Response.json(created);
} catch (error) {
return Response.json(
{ error: "Failed to create relationship" },
{ status: 500 }
);
}

10 changes: 0 additions & 10 deletions apps/webservice/src/app/api/v1/relationship/openapi.ts
View file
Open in desktop

This file was deleted.

62 changes: 62 additions & 0 deletions 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
@@ -0,0 +1,62 @@
import { z } from "zod";

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

import { authn } from "../../auth";
import { parseBody } from "../../body-parser";
import { request } from "../../middleware";

const bodySchema = z.object({
workspaceId: z.string().uuid(),
fromIdentifier: z.string(),
toIdentifier: z.string(),
type: z.enum(["associated_with", "depends_on"]),
});

export const POST = request()
.use(authn)
.use(parseBody(bodySchema))
.handle<{ body: z.infer<typeof bodySchema> }>(async (ctx) => {
const { body, db } = ctx;

const fromResource = await db
.select()
.from(SCHEMA.resource)
.where(
and(
eq(SCHEMA.resource.identifier, body.fromIdentifier),
eq(SCHEMA.resource.workspaceId, body.workspaceId),
),
)
.then(takeFirstOrNull);
if (!fromResource)
return Response.json(
{ error: `${body.fromIdentifier} not found` },
{ status: 404 },
);

const toResource = await db
.select()
.from(SCHEMA.resource)
.where(
and(
eq(SCHEMA.resource.identifier, body.toIdentifier),
eq(SCHEMA.resource.workspaceId, body.workspaceId),
),
)
.then(takeFirstOrNull);
if (!toResource)
return Response.json(
{ error: `${body.toIdentifier} not found` },
{ status: 404 },
);

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

Add transaction support and uniqueness validation.

The resource validation logic should be wrapped in a transaction, and you should check for existing relationships to prevent duplicates.

+    const existing = await db
+      .select()
+      .from(SCHEMA.resourceRelationship)
+      .where(
+        and(
+          eq(SCHEMA.resourceRelationship.sourceId, fromResource.id),
+          eq(SCHEMA.resourceRelationship.targetId, toResource.id),
+          eq(SCHEMA.resourceRelationship.type, body.type),
+        ),
+      )
+      .then(takeFirstOrNull);
+
+    if (existing) {
+      return Response.json(
+        { error: "Relationship already exists" },
+        { status: 409 },
+      );
+    }

Committable suggestion skipped: line range outside the PR's diff.

await db.insert(SCHEMA.resourceRelationship).values({
sourceId: fromResource.id,
targetId: toResource.id,
type: body.type,
});

return Response.json({ message: "Relationship created" }, { status: 200 });
});
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

Enhance relationship creation with better error handling and response.

The relationship creation needs improvements in several areas:

  1. Wrap database operations in a transaction
  2. Validate circular dependencies for "depends_on" relationships
  3. Return created relationship details in response
-    await db.insert(SCHEMA.resourceRelationship).values({
-      sourceId: fromResource.id,
-      targetId: toResource.id,
-      type: body.type,
-    });
-
-    return Response.json({ message: "Relationship created" }, { status: 200 });
+    return await db.transaction(async (tx) => {
+      if (body.type === "depends_on") {
+        const hasCycle = await checkForDependencyCycle(
+          tx,
+          fromResource.id,
+          toResource.id
+        );
+        if (hasCycle) {
+          return Response.json(
+            { error: "Circular dependency detected" },
+            { status: 400 }
+          );
+        }
+      }
+
+      const [relationship] = await tx
+        .insert(SCHEMA.resourceRelationship)
+        .values({
+          sourceId: fromResource.id,
+          targetId: toResource.id,
+          type: body.type,
+        })
+        .returning();
+
+      return Response.json({
+        message: "Relationship created",
+        relationship: {
+          fromIdentifier: body.fromIdentifier,
+          toIdentifier: body.toIdentifier,
+          type: body.type,
+        }
+      }, { status: 201 });
+    });

Committable suggestion skipped: line range outside the PR's diff.

46 changes: 0 additions & 46 deletions apps/webservice/src/app/api/v1/relationship/route.ts
View file
Open in desktop

This file was deleted.

17 changes: 17 additions & 0 deletions packages/db/src/schema/resource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import {
} from "@ctrlplane/validators/resources";

import type { Tx } from "../common.js";
import { deployment } from "./deployment.js";
import { resourceProvider } from "./resource-provider.js";
import { workspace } from "./workspace.js";

Expand Down Expand Up @@ -270,6 +271,22 @@ export type ResourceRelationship = InferSelectModel<
typeof resourceRelationship
>;

export const deploymentResourceRelationship = pgTable(
"deployment_resource_relationship",
{
id: uuid("id").primaryKey().defaultRandom(),
workspaceId: uuid("workspace_id")
.references(() => workspace.id, { onDelete: "cascade" })
.notNull(),
deploymentId: uuid("deployment_id")
.references(() => deployment.id, { onDelete: "cascade" })
.notNull(),
resourceIdentifier: text("resource_identifier")
.references(() => resource.identifier, { onDelete: "cascade" })
.notNull(),
},
);
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

Several improvements needed for the deploymentResourceRelationship table.

  1. Add a unique index to prevent duplicate relationships:
 export const deploymentResourceRelationship = pgTable(
   "deployment_resource_relationship",
   {
     id: uuid("id").primaryKey().defaultRandom(),
     workspaceId: uuid("workspace_id")
       .references(() => workspace.id, { onDelete: "cascade" })
       .notNull(),
     deploymentId: uuid("deployment_id")
       .references(() => deployment.id, { onDelete: "cascade" })
       .notNull(),
     resourceIdentifier: text("resource_identifier")
       .references(() => resource.identifier, { onDelete: "cascade" })
       .notNull(),
   },
+  (t) => ({
+    uniq: uniqueIndex().on(t.workspaceId, t.deploymentId, t.resourceIdentifier),
+  }),
 );
  1. Consider using resource.id instead of resource.identifier for the foreign key:
-    resourceIdentifier: text("resource_identifier")
-      .references(() => resource.identifier, { onDelete: "cascade" })
+    resourceId: uuid("resource_id")
+      .references(() => resource.id, { onDelete: "cascade" })
  1. Add relations definition for better type safety and querying:
export const deploymentResourceRelationshipRelations = relations(
  deploymentResourceRelationship,
  ({ one }) => ({
    workspace: one(workspace, {
      fields: [deploymentResourceRelationship.workspaceId],
      references: [workspace.id],
    }),
    deployment: one(deployment, {
      fields: [deploymentResourceRelationship.deploymentId],
      references: [deployment.id],
    }),
    resource: one(resource, {
      fields: [deploymentResourceRelationship.resourceId],
      references: [resource.id],
    }),
  }),
);
  1. Add schema validation like other tables:
export const createDeploymentResourceRelationship = createInsertSchema(
  deploymentResourceRelationship,
).omit({ id: true });

export const updateDeploymentResourceRelationship = 
  createDeploymentResourceRelationship.partial();

export type DeploymentResourceRelationship = InferSelectModel<
  typeof deploymentResourceRelationship
>;


export const resourceVariable = pgTable(
"resource_variable",
{
Expand Down
Loading