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: Release channel unique constraint #205

Merged
merged 4 commits into from
Nov 7, 2024
Merged

fix: Release channel unique constraint #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8f29a2c
fix: Release channel unique constraint
adityachoudhari26 Nov 7, 2024
56d005b
another route
adityachoudhari26 Nov 7, 2024
18ff76f
more
adityachoudhari26 Nov 7, 2024
145ab4d
error handling
adityachoudhari26 Nov 7, 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
12 changes: 10 additions & 2 deletions ...workspaceSlug]/systems/[systemSlug]/deployments/[deploymentSlug]/NavigationMenuAction.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { usePathname } from "next/navigation";
import { Button } from "@ctrlplane/ui/button";

import { CreateReleaseDialog } from "~/app/[workspaceSlug]/_components/CreateRelease";
import { api } from "~/trpc/react";
import { CreateReleaseChannelDialog } from "./release-channels/CreateReleaseChannelDialog";
import { CreateVariableDialog } from "./releases/CreateVariableDialog";

Expand All @@ -17,6 +18,10 @@ export const NavigationMenuAction: React.FC<{
const isVariablesActive = pathname.includes("variables");
const isReleaseChannelsActive = pathname.includes("release-channels");

const releaseChannelsQ =
api.deployment.releaseChannel.list.byDeploymentId.useQuery(deploymentId);
const releaseChannels = releaseChannelsQ.data ?? [];

return (
<div>
{isVariablesActive && (
Expand All @@ -27,8 +32,11 @@ export const NavigationMenuAction: React.FC<{
</CreateVariableDialog>
)}

{isReleaseChannelsActive && (
<CreateReleaseChannelDialog deploymentId={deploymentId}>
{isReleaseChannelsActive && !releaseChannelsQ.isLoading && (
<CreateReleaseChannelDialog
deploymentId={deploymentId}
releaseChannels={releaseChannels}
>
<Button size="sm" variant="secondary">
New Release Channel
</Button>
Expand Down
29 changes: 20 additions & 9 deletions ...[systemSlug]/deployments/[deploymentSlug]/release-channels/CreateReleaseChannelDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"use client";

import type * as SCHEMA from "@ctrlplane/db/schema";
import type { ReleaseCondition } from "@ctrlplane/validators/releases";
import { useState } from "react";
import Link from "next/link";
Expand Down Expand Up @@ -41,23 +42,33 @@ import { api } from "~/trpc/react";

type CreateReleaseChannelDialogProps = {
deploymentId: string;
releaseChannels: SCHEMA.ReleaseChannel[];
children: React.ReactNode;
};

const schema = z.object({
name: z.string().min(1).max(50),
description: z.string().max(1000).optional(),
releaseFilter: releaseCondition
.optional()
.refine((cond) => cond == null || isValidReleaseCondition(cond)),
});

const getFinalFilter = (filter?: ReleaseCondition) =>
filter && !isEmptyCondition(filter) ? filter : undefined;

export const CreateReleaseChannelDialog: React.FC<
CreateReleaseChannelDialogProps
> = ({ deploymentId, children }) => {
> = ({ deploymentId, children, releaseChannels }) => {
// schema needs to be in the component scope to use the releaseChannels
// to validate the name uniqueness
const schema = z.object({
name: z
.string()
.min(1)
.max(50)
.refine(
(name) => !releaseChannels.some((rc) => rc.name === name),
"Release channel name must be unique",
),
description: z.string().max(1000).optional(),
releaseFilter: releaseCondition
.optional()
.refine((cond) => cond == null || isValidReleaseCondition(cond)),
});

const [open, setOpen] = useState(false);
const { workspaceSlug, systemSlug, deploymentSlug } = useParams<{
workspaceSlug: string;
Expand Down
24 changes: 22 additions & 2 deletions apps/webservice/src/app/api/v1/deployments/[deploymentId]/release-channels/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { User } from "@ctrlplane/db/schema";
import type { z } from "zod";
import { NextResponse } from "next/server";

import { and, eq, takeFirst, takeFirstOrNull } from "@ctrlplane/db";
import { createReleaseChannel } from "@ctrlplane/db/schema";
import * as schema from "@ctrlplane/db/schema";
import { Permission } from "@ctrlplane/validators/auth";
Expand All @@ -27,8 +28,27 @@ export const POST = request()
{ params: { deploymentId: string } }
>(async (ctx, extra) => {
const releaseChannel = await ctx.db
.select()
.from(schema.releaseChannel)
.where(
and(
eq(schema.releaseChannel.deploymentId, extra.params.deploymentId),
eq(schema.releaseChannel.name, ctx.body.name),
),
)
.then(takeFirstOrNull);

if (releaseChannel)
return NextResponse.json(
{ error: "Release channel already exists" },
{ status: 409 },
);

return ctx.db
.insert(schema.releaseChannel)
.values({ ...ctx.body, deploymentId: extra.params.deploymentId })
.returning();
return NextResponse.json(releaseChannel);
.returning()
.then(takeFirst)
.then((releaseChannel) => NextResponse.json(releaseChannel))
.catch((error) => NextResponse.json({ error }, { status: 500 }));
});
26 changes: 22 additions & 4 deletions apps/webservice/src/app/api/v1/release-channels/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { z } from "zod";
import { NextResponse } from "next/server";

import { takeFirst } from "@ctrlplane/db";
import { and, eq, takeFirst, takeFirstOrNull } from "@ctrlplane/db";
import { createReleaseChannel } from "@ctrlplane/db/schema";
import * as SCHEMA from "@ctrlplane/db/schema";
import { Permission } from "@ctrlplane/validators/auth";
Expand All @@ -21,12 +21,30 @@ export const POST = request()
),
)
.handle<{ body: z.infer<typeof createReleaseChannel> }>(
async ({ db, body }) =>
db
async ({ db, body }) => {
const releaseChannel = await db
.select()
.from(SCHEMA.releaseChannel)
.where(
and(
eq(SCHEMA.releaseChannel.deploymentId, body.deploymentId),
eq(SCHEMA.releaseChannel.name, body.name),
),
)
.then(takeFirstOrNull);

if (releaseChannel)
return NextResponse.json(
{ error: "Release channel already exists" },
{ status: 409 },
);

return db
.insert(SCHEMA.releaseChannel)
.values(body)
.returning()
.then(takeFirst)
.then((releaseChannel) => NextResponse.json(releaseChannel))
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

Improve error handling with specific error codes.

The current implementation returns a 500 status code for all errors, which doesn't distinguish between different types of errors (e.g., validation errors, database constraints, server errors).

Consider implementing more specific error handling:

-        .catch((error) => NextResponse.json({ error }, { status: 500 })),
+        .catch((error) => {
+          if (error.code === '23505') { // PostgreSQL unique violation
+            return NextResponse.json(
+              { error: 'Release channel with this name already exists' },
+              { status: 409 }
+            );
+          }
+          console.error('Failed to create release channel:', error);
+          return NextResponse.json(
+            { error: 'Internal server error' },
+            { status: 500 }
+          );
+        }),

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

.catch((error) => NextResponse.json({ error }, { status: 500 })),
.catch((error) => NextResponse.json({ error }, { status: 500 }));
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 with logging and specific error codes.

The current error handling could be more robust:

  1. Add error logging for debugging
  2. Handle specific database errors (e.g., constraint violations)
-        .catch((error) => NextResponse.json({ error }, { status: 500 }));
+        .catch((error) => {
+          console.error('Failed to create release channel:', error);
+          if (error.code === '23505') { // PostgreSQL unique violation
+            return NextResponse.json(
+              { error: 'Release channel with this name already exists' },
+              { status: 409 }
+            );
+          }
+          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
.catch((error) => NextResponse.json({ error }, { status: 500 }));
.catch((error) => {
console.error('Failed to create release channel:', error);
if (error.code === '23505') { // PostgreSQL unique violation
return NextResponse.json(
{ error: 'Release channel with this name already exists' },
{ status: 409 }
);
}
return NextResponse.json(
{ error: 'Internal server error' },
{ status: 500 }
);
});

},
);
1 change: 1 addition & 0 deletions packages/db/drizzle/0033_abnormal_microchip.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
CREATE UNIQUE INDEX IF NOT EXISTS "release_channel_deployment_id_name_index" ON "release_channel" USING btree ("deployment_id","name");
Loading
Loading