From 98cca7039cc02beb1e773df7d211d77b27d7395e Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Mon, 7 Oct 2024 14:00:20 +0800 Subject: [PATCH] misc: addressed comments --- backend/package.json | 12 ++--- ...241007052025_make-audit-log-independent.ts | 48 +++++++++++++++++++ ...=> 20241007052449_partition-audit-logs.ts} | 40 ++-------------- .../ee/services/audit-log/audit-log-dal.ts | 4 +- backend/src/lib/errors/index.ts | 12 +++++ backend/src/server/plugins/error-handler.ts | 8 +++- 6 files changed, 78 insertions(+), 46 deletions(-) create mode 100644 backend/src/db/migrations/20241007052025_make-audit-log-independent.ts rename backend/src/db/migrations/{20241003075413_partition-audit-logs.ts => 20241007052449_partition-audit-logs.ts} (77%) diff --git a/backend/package.json b/backend/package.json index 886b80893..332c5a961 100644 --- a/backend/package.json +++ b/backend/package.json @@ -52,12 +52,12 @@ "auditlog-migration:status": "knex --knexfile ./src/db/auditlog-knexfile.ts --client pg migrate:status", "auditlog-migration:rollback": "knex --knexfile ./src/db/auditlog-knexfile.ts migrate:rollback", "migration:new": "tsx ./scripts/create-migration.ts", - "migration:up": "knex --knexfile ./src/db/knexfile.ts --client pg migrate:up && npm run auditlog-migration:up", - "migration:down": "knex --knexfile ./src/db/knexfile.ts --client pg migrate:down && npm run auditlog-migration:down", - "migration:list": "knex --knexfile ./src/db/knexfile.ts --client pg migrate:list && npm run auditlog-migration:list", - "migration:latest": "knex --knexfile ./src/db/knexfile.ts --client pg migrate:latest && npm run auditlog-migration:latest", - "migration:status": "knex --knexfile ./src/db/knexfile.ts --client pg migrate:status && npm run auditlog-migration:status", - "migration:rollback": "knex --knexfile ./src/db/knexfile.ts migrate:rollback && npm run auditlog-migration:rollback", + "migration:up": "npm run auditlog-migration:up && knex --knexfile ./src/db/knexfile.ts --client pg migrate:up", + "migration:down": "npm run auditlog-migration:down && knex --knexfile ./src/db/knexfile.ts --client pg migrate:down", + "migration:list": "npm run auditlog-migration:list && knex --knexfile ./src/db/knexfile.ts --client pg migrate:list", + "migration:latest": "npm run auditlog-migration:latest && knex --knexfile ./src/db/knexfile.ts --client pg migrate:latest", + "migration:status": "npm run auditlog-migration:status && knex --knexfile ./src/db/knexfile.ts --client pg migrate:status", + "migration:rollback": "npm run auditlog-migration:rollback && knex --knexfile ./src/db/knexfile.ts migrate:rollback", "seed:new": "tsx ./scripts/create-seed-file.ts", "seed": "knex --knexfile ./src/db/knexfile.ts --client pg seed:run", "db:reset": "npm run migration:rollback -- --all && npm run migration:latest" diff --git a/backend/src/db/migrations/20241007052025_make-audit-log-independent.ts b/backend/src/db/migrations/20241007052025_make-audit-log-independent.ts new file mode 100644 index 000000000..b6b98b9bc --- /dev/null +++ b/backend/src/db/migrations/20241007052025_make-audit-log-independent.ts @@ -0,0 +1,48 @@ +import { Knex } from "knex"; + +import { TableName } from "../schemas"; + +export async function up(knex: Knex): Promise { + if (await knex.schema.hasTable(TableName.AuditLog)) { + const doesProjectIdExist = await knex.schema.hasColumn(TableName.AuditLog, "projectId"); + const doesOrgIdExist = await knex.schema.hasColumn(TableName.AuditLog, "orgId"); + const doesProjectNameExist = await knex.schema.hasColumn(TableName.AuditLog, "projectName"); + + await knex.schema.alterTable(TableName.AuditLog, (t) => { + if (doesOrgIdExist) { + t.dropForeign("orgId"); + } + + if (doesProjectIdExist) { + t.dropForeign("projectId"); + } + + // add normalized field + if (!doesProjectNameExist) { + t.string("projectName"); + } + }); + } +} + +export async function down(knex: Knex): Promise { + const doesProjectIdExist = await knex.schema.hasColumn(TableName.AuditLog, "projectId"); + const doesOrgIdExist = await knex.schema.hasColumn(TableName.AuditLog, "orgId"); + const doesProjectNameExist = await knex.schema.hasColumn(TableName.AuditLog, "projectName"); + + if (await knex.schema.hasTable(TableName.AuditLog)) { + await knex.schema.alterTable(TableName.AuditLog, (t) => { + if (doesOrgIdExist) { + t.foreign("orgId").references("id").inTable(TableName.Organization).onDelete("CASCADE"); + } + if (doesProjectIdExist) { + t.foreign("projectId").references("id").inTable(TableName.Project).onDelete("CASCADE"); + } + + // remove normalized field + if (doesProjectNameExist) { + t.dropColumn("projectName"); + } + }); + } +} diff --git a/backend/src/db/migrations/20241003075413_partition-audit-logs.ts b/backend/src/db/migrations/20241007052449_partition-audit-logs.ts similarity index 77% rename from backend/src/db/migrations/20241003075413_partition-audit-logs.ts rename to backend/src/db/migrations/20241007052449_partition-audit-logs.ts index d0c923103..a8b68ef83 100644 --- a/backend/src/db/migrations/20241003075413_partition-audit-logs.ts +++ b/backend/src/db/migrations/20241007052449_partition-audit-logs.ts @@ -28,27 +28,9 @@ const isUsingDedicatedAuditLogDb = Boolean(process.env.AUDIT_LOGS_DB_CONNECTION_ export async function up(knex: Knex): Promise { if (!isUsingDedicatedAuditLogDb && (await knex.schema.hasTable(TableName.AuditLog))) { - // prepare the existing audit log table for it to become a partition - const doesProjectIdExist = await knex.schema.hasColumn(TableName.AuditLog, "projectId"); - const doesOrgIdExist = await knex.schema.hasColumn(TableName.AuditLog, "orgId"); - const doesProjectNameExist = await knex.schema.hasColumn(TableName.AuditLog, "projectName"); - await knex.schema.alterTable(TableName.AuditLog, (t) => { // remove existing keys t.dropPrimary(); - - if (doesOrgIdExist) { - t.dropForeign("orgId"); - } - - if (doesProjectIdExist) { - t.dropForeign("projectId"); - } - - // add normalized fields present in the partition table - if (!doesProjectNameExist) { - t.string("projectName"); - } }); } @@ -119,7 +101,7 @@ export async function up(knex: Knex): Promise { // create partitions 4 years ahead const partitionMonths = 4 * 12; const partitionPromises: Promise[] = []; - for (let x = 1; x < partitionMonths; x += 1) { + for (let x = 1; x <= partitionMonths; x += 1) { partitionPromises.push( createAuditLogPartition( knex, @@ -134,14 +116,14 @@ export async function up(knex: Knex): Promise { } export async function down(knex: Knex): Promise { - const result = await knex.raw(` + const partitionSearchResult = await knex.raw(` SELECT inhrelid::regclass::text FROM pg_inherits WHERE inhparent::regclass::text = '${TableName.PartitionedAuditLog}' AND inhrelid::regclass::text = '${TableName.AuditLog}' `); - const isAuditLogAPartition = result.rows.length > 0; + const isAuditLogAPartition = partitionSearchResult.rows.length > 0; if (isAuditLogAPartition) { // detach audit log from partition await knex.schema.raw(` @@ -151,10 +133,6 @@ export async function down(knex: Knex): Promise { `); // revert audit log modifications - const doesProjectIdExist = await knex.schema.hasColumn(TableName.AuditLog, "projectId"); - const doesOrgIdExist = await knex.schema.hasColumn(TableName.AuditLog, "orgId"); - const doesProjectNameExist = await knex.schema.hasColumn(TableName.AuditLog, "projectName"); - if (await knex.schema.hasTable(TableName.AuditLog)) { await knex.schema.alterTable(TableName.AuditLog, (t) => { // we drop this first because adding to the partition results in a new primary key @@ -164,18 +142,6 @@ export async function down(knex: Knex): Promise { t.primary(["id"], { constraintName: "audit_logs_pkey" }); - - if (doesOrgIdExist) { - t.foreign("orgId").references("id").inTable(TableName.Organization).onDelete("CASCADE"); - } - if (doesProjectIdExist) { - t.foreign("projectId").references("id").inTable(TableName.Project).onDelete("CASCADE"); - } - - // remove normalized fields - if (doesProjectNameExist) { - t.dropColumn("projectName"); - } }); } } diff --git a/backend/src/ee/services/audit-log/audit-log-dal.ts b/backend/src/ee/services/audit-log/audit-log-dal.ts index 37247ee30..214fc7b4f 100644 --- a/backend/src/ee/services/audit-log/audit-log-dal.ts +++ b/backend/src/ee/services/audit-log/audit-log-dal.ts @@ -3,7 +3,7 @@ import knex from "knex"; import { TDbClient } from "@app/db"; import { TableName } from "@app/db/schemas"; -import { BadRequestError, DatabaseError } from "@app/lib/errors"; +import { DatabaseError, GatewayTimeoutError } from "@app/lib/errors"; import { ormify, selectAllTableCols } from "@app/lib/knex"; import { logger } from "@app/lib/logger"; import { QueueName } from "@app/queue"; @@ -112,7 +112,7 @@ export const auditLogDALFactory = (db: TDbClient) => { return docs; } catch (error) { if (error instanceof knex.KnexTimeoutError) { - throw new BadRequestError({ + throw new GatewayTimeoutError({ error, message: "Failed to fetch audit logs due to timeout. Add more search filters." }); diff --git a/backend/src/lib/errors/index.ts b/backend/src/lib/errors/index.ts index fd5631788..01d1c8b8e 100644 --- a/backend/src/lib/errors/index.ts +++ b/backend/src/lib/errors/index.ts @@ -23,6 +23,18 @@ export class InternalServerError extends Error { } } +export class GatewayTimeoutError extends Error { + name: string; + + error: unknown; + + constructor({ name, error, message }: { message?: string; name?: string; error?: unknown }) { + super(message || "Timeout error"); + this.name = name || "GatewayTimeoutError"; + this.error = error; + } +} + export class UnauthorizedError extends Error { name: string; diff --git a/backend/src/server/plugins/error-handler.ts b/backend/src/server/plugins/error-handler.ts index 8ea8b8223..03681b1c8 100644 --- a/backend/src/server/plugins/error-handler.ts +++ b/backend/src/server/plugins/error-handler.ts @@ -7,6 +7,7 @@ import { BadRequestError, DatabaseError, ForbiddenRequestError, + GatewayTimeoutError, InternalServerError, NotFoundError, ScimRequestError, @@ -25,7 +26,8 @@ enum HttpStatusCodes { Unauthorized = 401, Forbidden = 403, // eslint-disable-next-line @typescript-eslint/no-shadow - InternalServerError = 500 + InternalServerError = 500, + GatewayTimeout = 504 } export const fastifyErrHandler = fastifyPlugin(async (server: FastifyZodProvider) => { @@ -47,6 +49,10 @@ export const fastifyErrHandler = fastifyPlugin(async (server: FastifyZodProvider void res .status(HttpStatusCodes.InternalServerError) .send({ statusCode: HttpStatusCodes.InternalServerError, message: "Something went wrong", error: error.name }); + } else if (error instanceof GatewayTimeoutError) { + void res + .status(HttpStatusCodes.GatewayTimeout) + .send({ statusCode: HttpStatusCodes.GatewayTimeout, message: error.message, error: error.name }); } else if (error instanceof ZodError) { void res .status(HttpStatusCodes.Unauthorized)