From 86383cf9efc80c1bc5e5b05d0344d4cde10a205b Mon Sep 17 00:00:00 2001 From: Seif Soliman Date: Thu, 24 Apr 2025 16:25:43 +0200 Subject: [PATCH] refactor(backend): improve redis and json.parse type safety with zod (@byseif21, @miodec) (#6481) ### Description refactored backend files to enhance type safety and reliability using Zod validation and Redis instead of JSON.parse , I tried to avoid the files that isn't necessary tho so I hope I don't miss any or included unnecessary ones!! didn't fully test only verified code compilation and partial tests without Redis!!. Should Close #5881 Related to #6207 --------- Co-authored-by: Miodec --- backend/src/api/controllers/result.ts | 2 +- backend/src/dal/new-quotes.ts | 45 +++++--- backend/src/dal/user.ts | 6 +- backend/src/init/redis.ts | 44 +++++++- backend/src/server.ts | 6 +- backend/src/services/weekly-xp-leaderboard.ts | 102 +++++++++++------- backend/src/utils/daily-leaderboards.ts | 70 +++++++----- .../contracts/src/schemas/leaderboards.ts | 31 +++++- packages/contracts/src/schemas/quotes.ts | 2 +- 9 files changed, 209 insertions(+), 99 deletions(-) diff --git a/backend/src/api/controllers/result.ts b/backend/src/api/controllers/result.ts index ce0c47bce..1ff05f1ca 100644 --- a/backend/src/api/controllers/result.ts +++ b/backend/src/api/controllers/result.ts @@ -606,9 +606,9 @@ export async function addResult( badgeId: selectedBadgeId, lastActivityTimestamp: Date.now(), isPremium, + timeTypedSeconds: totalDurationTypedSeconds, }, xpGained: xpGained.xp, - timeTypedSeconds: totalDurationTypedSeconds, } ); } diff --git a/backend/src/dal/new-quotes.ts b/backend/src/dal/new-quotes.ts index 9e4752ba2..128fa8f71 100644 --- a/backend/src/dal/new-quotes.ts +++ b/backend/src/dal/new-quotes.ts @@ -8,20 +8,23 @@ import MonkeyError from "../utils/error"; import { compareTwoStrings } from "string-similarity"; import { ApproveQuote, Quote } from "@monkeytype/contracts/schemas/quotes"; import { WithObjectId } from "../utils/misc"; +import { parseWithSchema as parseJsonWithSchema } from "@monkeytype/util/json"; +import { z } from "zod"; -type JsonQuote = { - text: string; - britishText?: string; - source: string; - length: number; - id: number; -}; +const JsonQuoteSchema = z.object({ + text: z.string(), + britishText: z.string().optional(), + approvedBy: z.string().optional(), + source: z.string(), + length: z.number(), + id: z.number(), +}); -type QuoteData = { - language: string; - quotes: JsonQuote[]; - groups: [number, number][]; -}; +const QuoteDataSchema = z.object({ + language: z.string(), + quotes: z.array(JsonQuoteSchema), + groups: z.array(z.tuple([z.number(), z.number()])), +}); const PATH_TO_REPO = "../../../../monkeytype-new-quotes"; @@ -86,7 +89,10 @@ export async function add( let similarityScore = -1; if (existsSync(fileDir)) { const quoteFile = await readFile(fileDir); - const quoteFileJSON = JSON.parse(quoteFile.toString()) as QuoteData; + const quoteFileJSON = parseJsonWithSchema( + quoteFile.toString(), + QuoteDataSchema + ); quoteFileJSON.quotes.every((old) => { if (compareTwoStrings(old.text, quote.text) > 0.9) { duplicateId = old.id; @@ -156,6 +162,7 @@ export async function approve( source: editSource ?? targetQuote.source, length: targetQuote.text.length, approvedBy: name, + id: -1, }; let message = ""; @@ -170,7 +177,10 @@ export async function approve( await git.pull("upstream", "master"); if (existsSync(fileDir)) { const quoteFile = await readFile(fileDir); - const quoteObject = JSON.parse(quoteFile.toString()) as QuoteData; + const quoteObject = parseJsonWithSchema( + quoteFile.toString(), + QuoteDataSchema + ); quoteObject.quotes.every((old) => { if (compareTwoStrings(old.text, quote.text) > 0.8) { throw new MonkeyError(409, "Duplicate quote"); @@ -183,7 +193,12 @@ export async function approve( } }); quote.id = maxid + 1; - quoteObject.quotes.push(quote as JsonQuote); + + if (quote.id === -1) { + throw new MonkeyError(500, "Failed to get max id"); + } + + quoteObject.quotes.push(quote); writeFileSync(fileDir, JSON.stringify(quoteObject, null, 2)); message = `Added quote to ${language}.json.`; } else { diff --git a/backend/src/dal/user.ts b/backend/src/dal/user.ts index 1315d64c8..e89951a7b 100644 --- a/backend/src/dal/user.ts +++ b/backend/src/dal/user.ts @@ -1103,11 +1103,7 @@ export async function updateStreak( if (isYesterday(streak.lastResultTimestamp, streak.hourOffset ?? 0)) { streak.length += 1; } else if (!isToday(streak.lastResultTimestamp, streak.hourOffset ?? 0)) { - void addImportantLog( - "streak_lost", - JSON.parse(JSON.stringify(streak)) as Record, - uid - ); + void addImportantLog("streak_lost", streak, uid); streak.length = 1; } diff --git a/backend/src/init/redis.ts b/backend/src/init/redis.ts index feeeb1c4f..8b816d3aa 100644 --- a/backend/src/init/redis.ts +++ b/backend/src/init/redis.ts @@ -1,11 +1,47 @@ import fs from "fs"; import _ from "lodash"; import { join } from "path"; -import IORedis from "ioredis"; +import IORedis, { Redis } from "ioredis"; import Logger from "../utils/logger"; import { isDevEnvironment } from "../utils/misc"; import { getErrorMessage } from "../utils/error"; +// Define Redis connection with custom methods for type safety +export type RedisConnectionWithCustomMethods = Redis & { + addResult: ( + keyCount: number, + scoresKey: string, + resultsKey: string, + maxResults: number, + expirationTime: number, + uid: string, + score: number, + data: string + ) => Promise; + addResultIncrement: ( + keyCount: number, + scoresKey: string, + resultsKey: string, + expirationTime: number, + uid: string, + score: number, + data: string + ) => Promise; + getResults: ( + keyCount: number, + scoresKey: string, + resultsKey: string, + minRank: number, + maxRank: number, + withScores: string + ) => Promise<[string[], string[]]>; + purgeResults: ( + keyCount: number, + uid: string, + namespace: string + ) => Promise; +}; + let connection: IORedis.Redis; let connected = false; @@ -73,11 +109,11 @@ export function isConnected(): boolean { return connected; } -export function getConnection(): IORedis.Redis | undefined { +export function getConnection(): RedisConnectionWithCustomMethods | null { const status = connection?.status; if (connection === undefined || status !== "ready") { - return undefined; + return null; } - return connection; + return connection as RedisConnectionWithCustomMethods; } diff --git a/backend/src/server.ts b/backend/src/server.ts index b4415dcc4..7570078a0 100644 --- a/backend/src/server.ts +++ b/backend/src/server.ts @@ -47,7 +47,7 @@ async function bootServer(port: number): Promise { Logger.info("Initializing queues..."); queues.forEach((queue) => { - queue.init(connection); + queue.init(connection ?? undefined); }); Logger.success( `Queues initialized: ${queues @@ -57,11 +57,11 @@ async function bootServer(port: number): Promise { Logger.info("Initializing workers..."); workers.forEach(async (worker) => { - await worker(connection).run(); + await worker(connection ?? undefined).run(); }); Logger.success( `Workers initialized: ${workers - .map((worker) => worker(connection).name) + .map((worker) => worker(connection ?? undefined).name) .join(", ")}` ); } diff --git a/backend/src/services/weekly-xp-leaderboard.ts b/backend/src/services/weekly-xp-leaderboard.ts index b02e04f94..70f7064b5 100644 --- a/backend/src/services/weekly-xp-leaderboard.ts +++ b/backend/src/services/weekly-xp-leaderboard.ts @@ -1,24 +1,20 @@ import { Configuration } from "@monkeytype/contracts/schemas/configuration"; import * as RedisClient from "../init/redis"; import LaterQueue from "../queues/later-queue"; -import { XpLeaderboardEntry } from "@monkeytype/contracts/schemas/leaderboards"; +import { + RedisXpLeaderboardEntry, + RedisXpLeaderboardEntrySchema, + RedisXpLeaderboardScore, + XpLeaderboardEntry, +} from "@monkeytype/contracts/schemas/leaderboards"; import { getCurrentWeekTimestamp } from "@monkeytype/util/date-and-time"; import MonkeyError from "../utils/error"; import { omit } from "lodash"; +import { parseWithSchema as parseJsonWithSchema } from "@monkeytype/util/json"; type AddResultOpts = { - entry: Pick< - XpLeaderboardEntry, - | "uid" - | "name" - | "discordId" - | "discordAvatar" - | "badgeId" - | "lastActivityTimestamp" - | "isPremium" - >; - xpGained: number; - timeTypedSeconds: number; + entry: RedisXpLeaderboardEntry; + xpGained: RedisXpLeaderboardScore; }; const weeklyXpLeaderboardLeaderboardNamespace = @@ -59,7 +55,7 @@ export class WeeklyXpLeaderboard { weeklyXpLeaderboardConfig: Configuration["leaderboards"]["weeklyXp"], opts: AddResultOpts ): Promise { - const { entry, xpGained, timeTypedSeconds } = opts; + const { entry, xpGained } = opts; const connection = RedisClient.getConnection(); if (!connection || !weeklyXpLeaderboardConfig.enabled) { @@ -89,16 +85,14 @@ export class WeeklyXpLeaderboard { const currentEntryTimeTypedSeconds = currentEntry !== null - ? (JSON.parse(currentEntry) as { timeTypedSeconds: number | undefined }) + ? parseJsonWithSchema(currentEntry, RedisXpLeaderboardEntrySchema) ?.timeTypedSeconds : undefined; const totalTimeTypedSeconds = - timeTypedSeconds + (currentEntryTimeTypedSeconds ?? 0); + entry.timeTypedSeconds + (currentEntryTimeTypedSeconds ?? 0); const [rank] = await Promise.all([ - // @ts-expect-error we are doing some weird file to function mapping, thats why its any - // eslint-disable-next-line @typescript-eslint/no-unsafe-call connection.addResultIncrement( 2, weeklyXpLeaderboardScoresKey, @@ -106,8 +100,13 @@ export class WeeklyXpLeaderboard { weeklyXpLeaderboardExpirationTimeInSeconds, entry.uid, xpGained, - JSON.stringify({ ...entry, timeTypedSeconds: totalTimeTypedSeconds }) - ) as Promise, + JSON.stringify( + RedisXpLeaderboardEntrySchema.parse({ + ...entry, + timeTypedSeconds: totalTimeTypedSeconds, + }) + ) + ), LaterQueue.scheduleForNextWeek( "weekly-xp-leaderboard-results", "weekly-xp" @@ -138,10 +137,8 @@ export class WeeklyXpLeaderboard { const { weeklyXpLeaderboardScoresKey, weeklyXpLeaderboardResultsKey } = this.getThisWeeksXpLeaderboardKeys(); - // @ts-expect-error we are doing some weird file to function mapping, thats why its any - // eslint-disable-next-line @typescript-eslint/no-unsafe-call const [results, scores] = (await connection.getResults( - 2, // How many of the arguments are redis keys (https://redis.io/docs/manual/programmability/lua-api/) + 2, weeklyXpLeaderboardScoresKey, weeklyXpLeaderboardResultsKey, minRank, @@ -163,14 +160,32 @@ export class WeeklyXpLeaderboard { const resultsWithRanks: XpLeaderboardEntry[] = results.map( (resultJSON: string, index: number) => { - //TODO parse with zod? - const parsed = JSON.parse(resultJSON) as XpLeaderboardEntry; + try { + const parsed = parseJsonWithSchema( + resultJSON, + RedisXpLeaderboardEntrySchema + ); + const scoreValue = scores[index]; - return { - ...parsed, - rank: minRank + index + 1, - totalXp: parseInt(scores[index] as string, 10), - }; + if (typeof scoreValue !== "string") { + throw new Error( + `Invalid score value at index ${index}: ${scoreValue}` + ); + } + + return { + ...parsed, + rank: minRank + index + 1, + totalXp: parseInt(scoreValue, 10), + }; + } catch (error) { + throw new MonkeyError( + 500, + `Failed to parse leaderboard entry at index ${index}: ${ + error instanceof Error ? error.message : String(error) + }` + ); + } } ); @@ -187,15 +202,12 @@ export class WeeklyXpLeaderboard { ): Promise { const connection = RedisClient.getConnection(); if (!connection || !weeklyXpLeaderboardConfig.enabled) { - throw new MonkeyError(500, "Redis connnection is unavailable"); + throw new MonkeyError(500, "Redis connection is unavailable"); } const { weeklyXpLeaderboardScoresKey, weeklyXpLeaderboardResultsKey } = this.getThisWeeksXpLeaderboardKeys(); - // eslint-disable-next-line @typescript-eslint/no-unused-expressions - connection.set; - const [[, rank], [, totalXp], [, _count], [, result]] = (await connection .multi() .zrevrank(weeklyXpLeaderboardScoresKey, uid) @@ -213,11 +225,21 @@ export class WeeklyXpLeaderboard { return null; } - //TODO parse with zod? - const parsed = JSON.parse((result as string) ?? "null") as Omit< - XpLeaderboardEntry, - "rank" | "count" | "totalXp" - >; + // safely parse the result with error handling + let parsed: RedisXpLeaderboardEntry; + try { + parsed = parseJsonWithSchema( + result ?? "null", + RedisXpLeaderboardEntrySchema + ); + } catch (error) { + throw new MonkeyError( + 500, + `Failed to parse leaderboard entry: ${ + error instanceof Error ? error.message : String(error) + }` + ); + } return { ...parsed, @@ -261,8 +283,6 @@ export async function purgeUserFromXpLeaderboards( return; } - // @ts-expect-error we are doing some weird file to function mapping, thats why its any - // eslint-disable-next-line @typescript-eslint/no-unsafe-call await connection.purgeResults( 0, uid, diff --git a/backend/src/utils/daily-leaderboards.ts b/backend/src/utils/daily-leaderboards.ts index 4af312c24..038885d47 100644 --- a/backend/src/utils/daily-leaderboards.ts +++ b/backend/src/utils/daily-leaderboards.ts @@ -2,11 +2,16 @@ import _, { omit } from "lodash"; import * as RedisClient from "../init/redis"; import LaterQueue from "../queues/later-queue"; import { matchesAPattern, kogascore } from "./misc"; +import { parseWithSchema as parseJsonWithSchema } from "@monkeytype/util/json"; import { Configuration, ValidModeRule, } from "@monkeytype/contracts/schemas/configuration"; -import { LeaderboardEntry } from "@monkeytype/contracts/schemas/leaderboards"; +import { + LeaderboardEntry, + RedisDailyLeaderboardEntry, + RedisDailyLeaderboardEntrySchema, +} from "@monkeytype/contracts/schemas/leaderboards"; import MonkeyError from "./error"; import { Mode, Mode2 } from "@monkeytype/contracts/schemas/shared"; import { getCurrentDayTimestamp } from "@monkeytype/util/date-and-time"; @@ -50,7 +55,7 @@ export class DailyLeaderboard { } public async addResult( - entry: Omit, + entry: RedisDailyLeaderboardEntry, dailyLeaderboardsConfig: Configuration["dailyLeaderboards"] ): Promise { const connection = RedisClient.getConnection(); @@ -72,9 +77,7 @@ export class DailyLeaderboard { const resultScore = kogascore(entry.wpm, entry.acc, entry.timestamp); - // @ts-expect-error we are doing some weird file to function mapping, thats why its any - // eslint-disable-next-line @typescript-eslint/no-unsafe-call - const rank = (await connection.addResult( + const rank = await connection.addResult( 2, leaderboardScoresKey, leaderboardResultsKey, @@ -83,7 +86,7 @@ export class DailyLeaderboard { entry.uid, resultScore, JSON.stringify(entry) - )) as number; + ); if ( isValidModeRule( @@ -126,16 +129,14 @@ export class DailyLeaderboard { const { leaderboardScoresKey, leaderboardResultsKey } = this.getTodaysLeaderboardKeys(); - // @ts-expect-error we are doing some weird file to function mapping, thats why its any - // eslint-disable-next-line @typescript-eslint/no-unsafe-call - const [results, _] = (await connection.getResults( + const [results, _] = await connection.getResults( 2, leaderboardScoresKey, leaderboardResultsKey, minRank, maxRank, "false" - )) as [string[], string[]]; + ); if (results === undefined) { throw new Error( @@ -145,13 +146,24 @@ export class DailyLeaderboard { const resultsWithRanks: LeaderboardEntry[] = results.map( (resultJSON, index) => { - // TODO: parse with zod? - const parsed = JSON.parse(resultJSON) as LeaderboardEntry; + try { + const parsed = parseJsonWithSchema( + resultJSON, + RedisDailyLeaderboardEntrySchema + ); - return { - ...parsed, - rank: minRank + index + 1, - }; + return { + ...parsed, + rank: minRank + index + 1, + }; + } catch (error) { + throw new MonkeyError( + 500, + `Failed to parse leaderboard entry at index ${index}: ${ + error instanceof Error ? error.message : String(error) + }` + ); + } } ); @@ -191,7 +203,7 @@ export class DailyLeaderboard { ): Promise { const connection = RedisClient.getConnection(); if (!connection || !dailyLeaderboardsConfig.enabled) { - throw new MonkeyError(500, "Redis connnection is unavailable"); + throw new MonkeyError(500, "Redis connection is unavailable"); } const { leaderboardScoresKey, leaderboardResultsKey } = @@ -214,16 +226,28 @@ export class DailyLeaderboard { return null; } - return { - ...(JSON.parse(result ?? "null") as LeaderboardEntry), - rank: rank + 1, - }; + try { + return { + ...parseJsonWithSchema( + result ?? "null", + RedisDailyLeaderboardEntrySchema + ), + rank: rank + 1, + }; + } catch (error) { + throw new MonkeyError( + 500, + `Failed to parse leaderboard entry: ${ + error instanceof Error ? error.message : String(error) + }` + ); + } } public async getCount(): Promise { const connection = RedisClient.getConnection(); if (!connection) { - throw new MonkeyError(500, "Redis connnection is unavailable"); + throw new MonkeyError(500, "Redis connection is unavailable"); } const { leaderboardScoresKey } = this.getTodaysLeaderboardKeys(); @@ -241,8 +265,6 @@ export async function purgeUserFromDailyLeaderboards( return; } - // @ts-expect-error we are doing some weird file to function mapping, thats why its any - // eslint-disable-next-line @typescript-eslint/no-unsafe-call await connection.purgeResults(0, uid, dailyLeaderboardNamespace); } diff --git a/packages/contracts/src/schemas/leaderboards.ts b/packages/contracts/src/schemas/leaderboards.ts index 6af7500c0..5d0fe58c3 100644 --- a/packages/contracts/src/schemas/leaderboards.ts +++ b/packages/contracts/src/schemas/leaderboards.ts @@ -16,19 +16,40 @@ export const LeaderboardEntrySchema = z.object({ }); export type LeaderboardEntry = z.infer; +export const RedisDailyLeaderboardEntrySchema = LeaderboardEntrySchema.omit({ + rank: true, +}); +export type RedisDailyLeaderboardEntry = z.infer< + typeof RedisDailyLeaderboardEntrySchema +>; + export const DailyLeaderboardRankSchema = LeaderboardEntrySchema; export type DailyLeaderboardRank = z.infer; -export const XpLeaderboardEntrySchema = z.object({ +export const RedisXpLeaderboardEntrySchema = z.object({ uid: z.string(), name: z.string(), + lastActivityTimestamp: z.number().int().nonnegative(), + timeTypedSeconds: z.number().nonnegative(), + // optionals discordId: z.string().optional(), discordAvatar: z.string().optional(), badgeId: z.number().int().optional(), - lastActivityTimestamp: z.number().int().nonnegative(), - timeTypedSeconds: z.number().nonnegative(), - rank: z.number().nonnegative().int(), - totalXp: z.number().nonnegative().int(), isPremium: z.boolean().optional(), }); +export type RedisXpLeaderboardEntry = z.infer< + typeof RedisXpLeaderboardEntrySchema +>; + +export const RedisXpLeaderboardScoreSchema = z.number().int().nonnegative(); +export type RedisXpLeaderboardScore = z.infer< + typeof RedisXpLeaderboardScoreSchema +>; + +export const XpLeaderboardEntrySchema = RedisXpLeaderboardEntrySchema.extend({ + //based on another redis collection + totalXp: RedisXpLeaderboardScoreSchema, + // dynamically added when generating response on the backend + rank: z.number().nonnegative().int(), +}); export type XpLeaderboardEntry = z.infer; diff --git a/packages/contracts/src/schemas/quotes.ts b/packages/contracts/src/schemas/quotes.ts index 1987ec0d7..d0dc28802 100644 --- a/packages/contracts/src/schemas/quotes.ts +++ b/packages/contracts/src/schemas/quotes.ts @@ -9,7 +9,7 @@ export const QuoteIdSchema = z export type QuoteId = z.infer; export const ApproveQuoteSchema = z.object({ - id: QuoteIdSchema.optional(), + id: QuoteIdSchema, text: z.string(), source: z.string(), length: z.number().int().positive(),