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

feat: Target routes #186

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ export const PATCH = request()
.then(takeFirstOrNull);

const provider = query?.target_provider;
if (!provider)
if (provider == null)
return NextResponse.json(
{ error: "Provider not found" },
{ status: 404 },
Expand Down
49 changes: 49 additions & 0 deletions apps/webservice/src/app/api/v1/targets/[targetId]/route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { NextResponse } from "next/server";

import { eq } from "@ctrlplane/db";
import * as schema from "@ctrlplane/db/schema";
import { Permission } from "@ctrlplane/validators/auth";

import { authn, authz } from "~/app/api/v1/auth";
import { request } from "~/app/api/v1/middleware";

function transformMetadata(
metadata: Array<{ key: string; value: string }>,
): Record<string, string> {
return metadata.reduce<Record<string, string>>((acc, m) => {
acc[m.key] = m.value;
return acc;
}, {});
}
Comment on lines +10 to +17
Copy link
Member

Choose a reason for hiding this comment

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

Object.fromEntries(metadata.map(v => [v.key, v.value])


export const GET = request()
.use(authn)
.use(
authz(({ can, extra }) => {
return can
.perform(Permission.TargetGet)
.on({ type: "target", id: extra.params.targetId });
}),
)
.handle(async ({ db }, { params }: { params: { targetId: string } }) => {
const { targetId } = params;

Comment on lines +28 to +30
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add input validation for targetId.

The targetId parameter should be validated to ensure it's a valid UUID format before querying the database.

 .handle(async ({ db }, { params }: { params: { targetId: string } }) => {
   const { targetId } = params;
+  if (!/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(targetId)) {
+    return NextResponse.json({ error: "Invalid target ID format" }, { status: 400 });
+  }
📝 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
.handle(async ({ db }, { params }: { params: { targetId: string } }) => {
const { targetId } = params;
.handle(async ({ db }, { params }: { params: { targetId: string } }) => {
const { targetId } = params;
if (!/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(targetId)) {
return NextResponse.json({ error: "Invalid target ID format" }, { status: 400 });
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this seems like a good check, not sure if it is worth pulling out to be used everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

@zacharyblasczyk, understood. If the input validation for targetId is only needed in a few places, it makes sense to keep it inline. Please let me know if there's anything else I can assist with.

(_/)?
(•ᴗ•)?


✏️ Learnings added
Learnt from: zacharyblasczyk
PR: ctrlplanedev/ctrlplane#186
File: apps/webservice/src/app/api/v1/targets/[targetId]/route.ts:28-30
Timestamp: 2024-10-30T20:37:42.513Z
Learning: When adding input validation for `targetId`, consider whether it's worth extracting into a reusable component based on its usage across the codebase.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

Copy link
Member

Choose a reason for hiding this comment

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

use zod

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

const target = await db.query.target.findFirst({
where: eq(schema.target.id, targetId),
with: {
metadata: true,
variables: true,
provider: true,
},
});

Comment on lines +31 to +39
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 error handling for database operations.

The database query should be wrapped in a try-catch block to handle potential errors gracefully.

-    const target = await db.query.target.findFirst({
+    let target;
+    try {
+      target = await db.query.target.findFirst({
         where: eq(schema.target.id, targetId),
         with: {
           metadata: true,
           variables: true,
           provider: true,
         },
       });
+    } catch (error) {
+      console.error('Database query failed:', error);
+      return NextResponse.json(
+        { error: "Internal server error" },
+        { 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
const target = await db.query.target.findFirst({
where: eq(schema.target.id, targetId),
with: {
metadata: true,
variables: true,
provider: true,
},
});
let target;
try {
target = await db.query.target.findFirst({
where: eq(schema.target.id, targetId),
with: {
metadata: true,
variables: true,
provider: true,
},
});
} catch (error) {
console.error('Database query failed:', error);
return NextResponse.json(
{ error: "Internal server error" },
{ status: 500 }
);
}

if (!target)
return NextResponse.json({ error: "Target not found" }, { status: 404 });

const metadata = transformMetadata(target.metadata);

return NextResponse.json({
...target,
metadata,
});
});
48 changes: 48 additions & 0 deletions apps/webservice/src/app/api/v1/targets/[targetId]/set/route.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import type { z } from "zod";
import { NextResponse } from "next/server";

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

import { authn, authz } from "~/app/api/v1/auth";
import { parseBody } from "~/app/api/v1/body-parser";
import { request } from "~/app/api/v1/middleware";
import { bodySchema } from "~/app/api/v1/targets/workspaces/[workspaceId]/route";

export const PATCH = request()
.use(authn)
.use(parseBody(bodySchema))
.use(
authz(({ can, extra }) => {
return can
.perform(Permission.TargetUpdate)
.on({ type: "target", id: extra.params.targetId });
}),
)
.handle<
{ body: z.infer<typeof bodySchema> },
{ params: { targetId: string } }
>(async (ctx, { params }) => {
const { body } = ctx;

console.log(body);
console.log(params.targetId);

const existingTarget = await db.query.target.findFirst({
where: eq(schema.target.id, params.targetId),
});

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

const targetData = {
...existingTarget,
...body.target,
};

const target = await upsertTargets(db, [targetData]);
return NextResponse.json(target);
Comment on lines +46 to +47
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

Return the updated target object instead of an array

The upsertTargets function returns an array of targets. Since this endpoint updates a single target, returning the entire array may not be appropriate. To align with API expectations and provide clarity to the client, consider returning the first element of the array.

Apply this diff to return the updated target object:

 const target = await upsertTargets(db, [targetData]);
-return NextResponse.json(target);
+return NextResponse.json(target[0]);
📝 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 target = await upsertTargets(db, [targetData]);
return NextResponse.json(target);
const target = await upsertTargets(db, [targetData]);
return NextResponse.json(target[0]);

});
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
import { NextResponse } from "next/server";
import { z } from "zod";

import { db } from "@ctrlplane/db/client";
import * as schema from "@ctrlplane/db/schema";
import { upsertTargets } from "@ctrlplane/job-dispatch";
import { Permission } from "@ctrlplane/validators/auth";

import { authn, authz } from "~/app/api/v1/auth";
import { parseBody } from "~/app/api/v1/body-parser";
import { request } from "~/app/api/v1/middleware";

export const bodySchema = z.object({
target: schema.createTarget.extend({
metadata: z.record(z.string()).optional(),
variables: z
.array(
z.object({
key: z.string(),
value: z.union([z.string(), z.number(), z.boolean(), z.null()]),
sensitive: z.boolean(),
}),
)
.optional()
.refine(
(vars) =>
vars == null || new Set(vars.map((v) => v.key)).size === vars.length,
"Duplicate variable keys are not allowed",
),
}),
});

export const POST = request()
.use(authn)
.use(parseBody(bodySchema))
.use(
authz(({ can, extra }) => {
return can
.perform(Permission.TargetCreate)
.on({ type: "workspace", id: extra.params.workspaceId });
}),
)
.handle<
{ body: z.infer<typeof bodySchema> },
{ params: { workspaceId: string } }
>(async (ctx, { params }) => {
const { body } = ctx;

const targetData = {
...body.target,
workspaceId: params.workspaceId,
variables: body.target.variables?.map((v) => ({
...v,
value: v.value ?? null,
})),
};

const target = await upsertTargets(db, [targetData]);

return NextResponse.json(target);
Comment on lines +58 to +60
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

Ensure the correct response object is returned

The upsertTargets function is called with an array of targets and likely returns an array. Currently, you're returning target directly in the response, which may be an array. If the API endpoint is expected to return a single target object, consider returning the first element of the array.

Apply this diff to return the first target in the array:

const target = await upsertTargets(db, [targetData]);

-return NextResponse.json(target);
+return NextResponse.json(target[0]);
📝 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 target = await upsertTargets(db, [targetData]);
return NextResponse.json(target);
const target = await upsertTargets(db, [targetData]);
return NextResponse.json(target[0]);

});
Loading
Loading