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: Job distribution graph #152

Merged
merged 2 commits into from
Oct 20, 2024
Merged
Changes from 1 commit
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
Next Next commit
fix: Job distribution graph
adityachoudhari26 committed Oct 20, 2024
commit 9fbe8b4a9904bee421abee0bc2ec84e54eb24b12
4 changes: 3 additions & 1 deletion apps/webservice/src/app/[workspaceSlug]/(job)/jobs/page.tsx
Original file line number Diff line number Diff line change
@@ -21,7 +21,9 @@ export default async function JobsPage({
const workspace = await api.workspace.bySlug(params.workspaceSlug);
if (workspace == null) return notFound();

const releaseJobTriggers = await api.job.config.byWorkspaceId(workspace.id);
const releaseJobTriggers = await api.job.config.byWorkspaceId.list(
workspace.id,
);

if (releaseJobTriggers.length === 0) return <JobsGettingStarted />;

Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"use client";

import type { Workspace } from "@ctrlplane/db/schema";
import { startOfDay, sub } from "date-fns";
import { isSameDay, startOfDay, sub } from "date-fns";
import _ from "lodash";
import { Bar, BarChart, CartesianGrid, XAxis } from "recharts";

@@ -25,21 +25,21 @@ export const JobHistoryChart: React.FC<{
workspace: Workspace;
className?: string;
}> = ({ className, workspace }) => {
const releaseJobTriggers = api.job.config.byWorkspaceId.useQuery(
const releaseJobTriggers = api.job.config.byWorkspaceId.list.useQuery(
workspace.id,
{ refetchInterval: 60_000 },
);

const dailyCounts = api.job.config.byWorkspaceId.dailyCount.useQuery({
workspaceId: workspace.id,
timezone: Intl.DateTimeFormat().resolvedOptions().timeZone,
});

const now = startOfDay(new Date());
const chartData = dateRange(sub(now, { weeks: 6 }), now, 1, "days").map(
(d) => ({
date: d.toString(),
jobs: (releaseJobTriggers.data ?? []).filter(
(j) =>
j.job.createdAt != null &&
j.job.status !== JobStatus.Pending &&
startOfDay(j.job.createdAt).toString() === d.toString(),
).length,
date: d,
jobs: dailyCounts.data?.find((c) => isSameDay(c.date, d))?.count ?? 0,
}),
);

82 changes: 54 additions & 28 deletions packages/api/src/router/job.ts
Original file line number Diff line number Diff line change
@@ -3,7 +3,7 @@ import _ from "lodash";
import { isPresent } from "ts-is-present";
import { z } from "zod";

import { and, asc, desc, eq, isNull, takeFirst } from "@ctrlplane/db";
import { and, asc, desc, eq, isNull, sql, takeFirst } from "@ctrlplane/db";
import {
createJobAgent,
deployment,
@@ -42,33 +42,59 @@ const releaseJobTriggerQuery = (tx: Tx) =>
.innerJoin(jobAgent, eq(jobAgent.id, deployment.jobAgentId));

const releaseJobTriggerRouter = createTRPCRouter({
byWorkspaceId: protectedProcedure
.input(z.string().uuid())
.meta({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.SystemList)
.on({ type: "workspace", id: input }),
})
.query(({ ctx, input }) =>
releaseJobTriggerQuery(ctx.db)
.leftJoin(system, eq(system.id, deployment.systemId))
.where(
and(eq(system.workspaceId, input), isNull(environment.deletedAt)),
)
.orderBy(asc(releaseJobTrigger.createdAt))
.limit(1_000)
.then((data) =>
data.map((t) => ({
...t.release_job_trigger,
job: t.job,
agent: t.job_agent,
target: t.target,
release: { ...t.release, deployment: t.deployment },
environment: t.environment,
})),
),
),
byWorkspaceId: createTRPCRouter({
list: protectedProcedure
.input(z.string().uuid())
.meta({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.SystemList)
.on({ type: "workspace", id: input }),
})
.query(({ ctx, input }) =>
releaseJobTriggerQuery(ctx.db)
.leftJoin(system, eq(system.id, deployment.systemId))
.where(
and(eq(system.workspaceId, input), isNull(environment.deletedAt)),
)
.orderBy(asc(releaseJobTrigger.createdAt))
.limit(1_000)
.then((data) =>
data.map((t) => ({
...t.release_job_trigger,
job: t.job,
agent: t.job_agent,
target: t.target,
release: { ...t.release, deployment: t.deployment },
environment: t.environment,
})),
),
),
dailyCount: protectedProcedure
.input(
z.object({
workspaceId: z.string().uuid(),
timezone: z.string(),
}),
)
.query(async ({ ctx, input }) => {
const dateTruncExpr = sql<Date>`date_trunc('day', ${releaseJobTrigger.createdAt} AT TIME ZONE 'UTC' AT TIME ZONE '${sql.raw(input.timezone)}')`;
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

Validate 'timezone' input to prevent SQL injection

Using sql.raw(input.timezone) directly interpolates user input into the SQL query, which could lead to SQL injection vulnerabilities if the input is not properly sanitized. To mitigate this risk, validate input.timezone against a list of allowed timezones.

Apply this diff to validate the timezone input:

 .input(
   z.object({
     workspaceId: z.string().uuid(),
-    timezone: z.string(),
+    timezone: z.enum([
+      'UTC',
+      'America/New_York',
+      'Europe/London',
+      'Asia/Tokyo',
+      // Add all supported time zones
+    ]),
 ),

Alternatively, consider using a library like moment-timezone to validate that the provided timezone is a valid IANA timezone identifier before using it in the SQL query.

Committable suggestion was skipped due to low confidence.

return ctx.db
.select({
date: dateTruncExpr.as("date"),
count: sql<number>`COUNT(*)`.as("count"),
})
.from(releaseJobTrigger)
.innerJoin(
environment,
eq(releaseJobTrigger.environmentId, environment.id),
)
.innerJoin(system, eq(environment.systemId, system.id))
.where(eq(system.workspaceId, input.workspaceId))
.groupBy(dateTruncExpr)
.orderBy(dateTruncExpr);
}),
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 missing authorization check to 'dailyCount' procedure

The dailyCount procedure currently lacks an authorization check, which could allow unauthorized access to sensitive data. To ensure that only authorized users can access this endpoint, please add an authorizationCheck similar to the one used in the list procedure.

Apply this diff to add the authorization check:

  dailyCount: protectedProcedure
+   .meta({
+     authorizationCheck: ({ canUser, input }) =>
+       canUser
+         .perform(Permission.SystemList)
+         .on({ type: "workspace", id: input.workspaceId }),
+   })
    .input(
      z.object({
        workspaceId: z.string().uuid(),
        timezone: z.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
dailyCount: protectedProcedure
.input(
z.object({
workspaceId: z.string().uuid(),
timezone: z.string(),
}),
)
.query(async ({ ctx, input }) => {
const dateTruncExpr = sql<Date>`date_trunc('day', ${releaseJobTrigger.createdAt} AT TIME ZONE 'UTC' AT TIME ZONE '${sql.raw(input.timezone)}')`;
return ctx.db
.select({
date: dateTruncExpr.as("date"),
count: sql<number>`COUNT(*)`.as("count"),
})
.from(releaseJobTrigger)
.innerJoin(
environment,
eq(releaseJobTrigger.environmentId, environment.id),
)
.innerJoin(system, eq(environment.systemId, system.id))
.where(eq(system.workspaceId, input.workspaceId))
.groupBy(dateTruncExpr)
.orderBy(dateTruncExpr);
}),
dailyCount: protectedProcedure
.meta({
authorizationCheck: ({ canUser, input }) =>
canUser
.perform(Permission.SystemList)
.on({ type: "workspace", id: input.workspaceId }),
})
.input(
z.object({
workspaceId: z.string().uuid(),
timezone: z.string(),
}),
)
.query(async ({ ctx, input }) => {
const dateTruncExpr = sql<Date>`date_trunc('day', ${releaseJobTrigger.createdAt} AT TIME ZONE 'UTC' AT TIME ZONE '${sql.raw(input.timezone)}')`;
return ctx.db
.select({
date: dateTruncExpr.as("date"),
count: sql<number>`COUNT(*)`.as("count"),
})
.from(releaseJobTrigger)
.innerJoin(
environment,
eq(releaseJobTrigger.environmentId, environment.id),
)
.innerJoin(system, eq(environment.systemId, system.id))
.where(eq(system.workspaceId, input.workspaceId))
.groupBy(dateTruncExpr)
.orderBy(dateTruncExpr);
}),

}),

byDeploymentAndEnvironment: protectedProcedure
.meta({

Unchanged files with check annotations Beta

EXPOSE 3000
ENV PORT=3000
ENV AUTH_TRUST_HOST=true

Check warning on line 59 in apps/webservice/Dockerfile

GitHub Actions / build (linux/amd64)

Sensitive data should not be used in the ARG or ENV commands

SecretsUsedInArgOrEnv: Do not use ARG or ENV instructions for sensitive data (ENV "AUTH_TRUST_HOST") More info: https://docs.docker.com/go/dockerfile/rule/secrets-used-in-arg-or-env/
ENV NODE_ENV=production
ENV HOSTNAME=0.0.0.0
import { eq } from "@ctrlplane/db";

Check failure on line 1 in apps/jobs/src/policy-checker/index.ts

GitHub Actions / Lint

'and' is defined but never used. Allowed unused vars must match /^_/u

Check failure on line 1 in apps/jobs/src/policy-checker/index.ts

GitHub Actions / Lint

'isNull' is defined but never used. Allowed unused vars must match /^_/u

Check failure on line 1 in apps/jobs/src/policy-checker/index.ts

GitHub Actions / Lint

'or' is defined but never used. Allowed unused vars must match /^_/u
import { db } from "@ctrlplane/db/client";
import * as schema from "@ctrlplane/db/schema";
import {