From dd821e0a69ee6d5833b0cd169f88cb0cff5df1ee Mon Sep 17 00:00:00 2001 From: zkldi <20380519+zkldi@users.noreply.github.com> Date: Tue, 30 Aug 2022 13:30:56 +0100 Subject: [PATCH 1/2] refactor: use tagged unions instead of instanceof Instanceof appears to be somewhat broken in typescript. Still looking into it. --- .../framework/common/converter-failures.ts | 38 ++-- .../score-import/framework/orphans/orphans.ts | 33 +-- .../score-importing/score-importing.ts | 190 +++++++++--------- 3 files changed, 135 insertions(+), 126 deletions(-) diff --git a/server/src/lib/score-import/framework/common/converter-failures.ts b/server/src/lib/score-import/framework/common/converter-failures.ts index ae29b5c6a..d5df881f3 100644 --- a/server/src/lib/score-import/framework/common/converter-failures.ts +++ b/server/src/lib/score-import/framework/common/converter-failures.ts @@ -3,14 +3,23 @@ import type { ImportTypeContextMap, ImportTypeDataMap } from "../../import-types/common/types"; import type { ImportTypes } from "tachi-common"; +export type FailureTypes = "Internal" | "InvalidScore" | "KTDataNotFound" | "SkipScore"; + export class ConverterFailure extends Error { message: string; + failureType: FailureTypes; - constructor(message: string) { + constructor(message: string, failureType: FailureTypes) { super(); this.message = message; - Object.setPrototypeOf(this, ConverterFailure.prototype); + // @hack + // Typescript sometimes decides to "compile out" prototype chains like this. + // This creates problems for things like `instanceof`, which will + // (seemingly randomly) stop working properly. Since we can't use instanceof + // to stably assert what kind of error was thrown, we use a tagged string + // instead. + this.failureType = failureType; } } @@ -21,14 +30,7 @@ export class ConverterFailure extends Error { */ export class SkipScoreFailure extends ConverterFailure { constructor(message: string) { - super(message); - - // @hack - // Typescript sometimes decides to "compile out" prototype chains like this - // we have to *enforce* that these classes have the right inheritance, - // because we do instanceof checks to determine what kind of error was - // thrown. We could switch to a tagged union approach, but this seems more stable. - Object.setPrototypeOf(this, SkipScoreFailure.prototype); + super(message, "SkipScore"); } } @@ -48,13 +50,11 @@ export class KTDataNotFoundFailure extends ConverterFailu data: ImportTypeDataMap[T], context: ImportTypeContextMap[T] ) { - super(message); + super(message, "KTDataNotFound"); this.importType = importType; this.data = data; this.converterContext = context; - - Object.setPrototypeOf(this, KTDataNotFoundFailure.prototype); } } @@ -64,9 +64,7 @@ export class KTDataNotFoundFailure extends ConverterFailu */ export class InvalidScoreFailure extends ConverterFailure { constructor(message: string) { - super(message); - - Object.setPrototypeOf(this, InvalidScoreFailure.prototype); + super(message, "InvalidScore"); } } @@ -76,8 +74,10 @@ export class InvalidScoreFailure extends ConverterFailure { */ export class InternalFailure extends ConverterFailure { constructor(message: string) { - super(message); - - Object.setPrototypeOf(this, InternalFailure.prototype); + super(message, "Internal"); } } + +export function IsConverterFailure(err: ConverterFailure | Error): err is ConverterFailure { + return "failureType" in err; +} diff --git a/server/src/lib/score-import/framework/orphans/orphans.ts b/server/src/lib/score-import/framework/orphans/orphans.ts index 7c8071233..c169a8ab6 100644 --- a/server/src/lib/score-import/framework/orphans/orphans.ts +++ b/server/src/lib/score-import/framework/orphans/orphans.ts @@ -1,21 +1,17 @@ import { Converters } from "../../import-types/converters"; -import { - ConverterFailure, - InternalFailure, - KTDataNotFoundFailure, -} from "../common/converter-failures"; import { HandlePostImportSteps } from "../score-importing/score-import-main"; import { ProcessSuccessfulConverterReturn } from "../score-importing/score-importing"; import db from "external/mongo/db"; import fjsh from "fast-json-stable-hash"; import { GetUserWithID } from "utils/user"; import type { - ConverterFunction, ConverterFnReturnOrFailure, + ConverterFunction, ImportTypeContextMap, ImportTypeDataMap, OrphanScoreDocument, } from "../../import-types/common/types"; +import type { ConverterFailure } from "../common/converter-failures"; import type { KtLogger } from "lib/logger/logger"; import type { Game, ImportTypes, integer } from "tachi-common"; @@ -96,10 +92,12 @@ export async function ReprocessOrphan( try { res = await ConverterFunction(orphan.data, orphan.context, orphan.importType, logger); - } catch (err) { + } catch (e) { + const err = e as ConverterFailure | Error; + // this is impossible to test, so we're going to ignore it /* istanbul ignore next */ - if (!(err instanceof ConverterFailure)) { + if (!("failureType" in err)) { logger.error(`Converter function ${orphan.importType} returned unexpected error.`, { err, }); @@ -111,15 +109,18 @@ export async function ReprocessOrphan( res = err; } - // If the data still can't be found, we do nothing about it. - if (res instanceof KTDataNotFoundFailure) { - logger.debug(`Unorphaning ${orphan.orphanID} failed. (${res.message})`); - return false; - } else if (res instanceof InternalFailure) { - logger.error(`Orphan Internal Failure - ${res.message}, OrphanID ${orphan.orphanID}`); + if ("failureType" in res) { + // If the data still can't be found, we do nothing about it. + if (res.failureType === "KTDataNotFound") { + logger.debug(`Unorphaning ${orphan.orphanID} failed. (${res.message})`); + return false; + } else if (res.failureType === "Internal") { + logger.error(`Orphan Internal Failure - ${res.message}, OrphanID ${orphan.orphanID}`); - return false; - } else if (res instanceof ConverterFailure) { + return false; + } + + // otherwise, it's another converterfailure we don't need to specifically handle. logger.warn( `received ConverterFailure ${res.message} on orphan ${orphan.orphanID}. Removing orphan.` ); diff --git a/server/src/lib/score-import/framework/score-importing/score-importing.ts b/server/src/lib/score-import/framework/score-importing/score-importing.ts index ed7f9f346..4cc66843b 100644 --- a/server/src/lib/score-import/framework/score-importing/score-importing.ts +++ b/server/src/lib/score-import/framework/score-importing/score-importing.ts @@ -1,21 +1,12 @@ import { HydrateScore } from "./hydrate-score"; import { GetScoreQueueMaybe, InsertQueue, QueueScoreInsert } from "./insert-score"; import { CreateScoreID } from "./score-id"; -import { - ConverterFailure, - InternalFailure, - InvalidScoreFailure, - KTDataNotFoundFailure, - SkipScoreFailure, -} from "../common/converter-failures"; +import { IsConverterFailure } from "../common/converter-failures"; import { OrphanScore } from "../orphans/orphans"; import db from "external/mongo/db"; import { AppendLogCtx } from "lib/logger/logger"; -import type { - ConverterFnReturnOrFailure, - ConverterFnSuccessReturn, - ConverterFunction, -} from "../../import-types/common/types"; +import type { ConverterFnSuccessReturn, ConverterFunction } from "../../import-types/common/types"; +import type { ConverterFailure, KTDataNotFoundFailure } from "../common/converter-failures"; import type { DryScore } from "../common/types"; import type { KtLogger } from "lib/logger/logger"; import type { ScoreImportJob } from "lib/score-import/worker/types"; @@ -128,104 +119,121 @@ export async function ImportIterableDatapoint( logger: KtLogger ): Promise { // Converter Function Return - let cfnReturn: ConverterFnReturnOrFailure; + let cfnReturn: ConverterFnSuccessReturn; try { cfnReturn = await ConverterFunction(data, context, importType, logger); - } catch (err) { - cfnReturn = err as ConverterFailure | Error; - } + } catch (e) { + const err = e as ConverterFailure | Error; - // if this conversion failed, return it in the proper format - if (cfnReturn instanceof ConverterFailure) { - if (cfnReturn instanceof KTDataNotFoundFailure) { - logger.info(`KTDataNotFoundFailure: ${cfnReturn.message}`, { - cfnReturn, - hideFromConsole: ["cfnReturn"], + // if this isn't a converterFailure, it's just a general error. + // Some sort of internal issue? + if (!IsConverterFailure(err)) { + logger.error(`Unknown error thrown from converter, Ignoring.`, { + err, }); + return { + success: false, + type: "InternalError", + message: "An internal service error has occured.", + content: {}, + }; + } - logger.debug("Inserting orphan...", { cfnReturn }); + // otherwise, let's handle all the error types. + // Originally, we handled this by using `instanceof` to check what class + // instance the failure type was. This was neat, but typescript has some + // questionable bugs with respect to maintaining prototype chains. + // Even though it's uglier, we instead use a stringly-typed union. + switch (err.failureType) { + case "KTDataNotFound": { + const dnfErr = err as KTDataNotFoundFailure; - const insertOrphan = await OrphanScore( - cfnReturn.importType, - userID, - cfnReturn.data, - cfnReturn.converterContext, - cfnReturn.message, - game, - logger - ); + logger.info(`KTDataNotFoundFailure: ${dnfErr.message}`, { + cfnReturn: dnfErr, + hideFromConsole: ["cfnReturn"], + }); + + logger.debug("Inserting orphan...", { cfnReturn: dnfErr }); + + const insertOrphan = await OrphanScore( + dnfErr.importType, + userID, + dnfErr.data, + dnfErr.converterContext, + dnfErr.message, + game, + logger + ); + + if (insertOrphan.success) { + logger.debug("Orphan inserted successfully.", { + orphanID: insertOrphan.orphanID, + }); + return { + success: false, + type: "KTDataNotFound", + message: dnfErr.message, + content: { + context: dnfErr.converterContext, + data: dnfErr.data, + orphanID: insertOrphan.orphanID, + }, + }; + } + + logger.debug(`Orphan already exists.`, { orphanID: insertOrphan.orphanID }); - if (insertOrphan.success) { - logger.debug("Orphan inserted successfully.", { orphanID: insertOrphan.orphanID }); return { success: false, - type: "KTDataNotFound", - message: cfnReturn.message, + type: "OrphanExists", + message: err.message, content: { - context: cfnReturn.converterContext, - data: cfnReturn.data, orphanID: insertOrphan.orphanID, }, }; } - logger.debug(`Orphan already exists.`, { orphanID: insertOrphan.orphanID }); + case "InvalidScore": { + logger.info(`InvalidScoreFailure: ${err.message}`, { + cfnReturn: err, + hideFromConsole: ["cfnReturn"], + }); + return { + success: false, + type: "InvalidDatapoint", + message: err.message, + content: {}, + }; + } - return { - success: false, - type: "OrphanExists", - message: cfnReturn.message, - content: { - orphanID: insertOrphan.orphanID, - }, - }; - } else if (cfnReturn instanceof InvalidScoreFailure) { - logger.info(`InvalidScoreFailure: ${cfnReturn.message}`, { - cfnReturn, - hideFromConsole: ["cfnReturn"], - }); - return { - success: false, - type: "InvalidDatapoint", - message: cfnReturn.message, - content: {}, - }; - } else if (cfnReturn instanceof InternalFailure) { - logger.error(`Internal error occured.`, { cfnReturn }); - return { - success: false, - type: "InternalError", + case "Internal": { + logger.error(`Internal error occured.`, { cfnReturn: err }); + return { + success: false, + type: "InternalError", - // could return cfnReturn.message here, but we might want to hide the details of the crash. - message: "An internal error has occured.", - content: {}, - }; - } else if (cfnReturn instanceof SkipScoreFailure) { - return null; + // could return cfnReturn.message here, but we might want to hide the details of the crash. + message: "An internal error has occured.", + content: {}, + }; + } + + case "SkipScore": + return null; + + default: { + logger.warn(`Unknown error returned as ConverterFailure, Ignoring.`, { + err, + }); + return { + success: false, + type: "InternalError", + message: "An internal service error has occured.", + content: {}, + }; + } } - - logger.warn(`Unknown error returned as ConverterFailure, Ignoring.`, { - err: cfnReturn, - }); - return { - success: false, - type: "InternalError", - message: "An internal service error has occured.", - content: {}, - }; - } - - if (cfnReturn instanceof Error) { - logger.error(`Unknown error thrown from converter, Ignoring.`, { - err: cfnReturn, - }); - return { - success: false, - type: "InternalError", - message: "An internal service error has occured.", - content: {}, - }; } return ProcessSuccessfulConverterReturn(userID, cfnReturn, blacklist, logger); From 84387ba78b93c2e4b9769bab5910e893936c8e14 Mon Sep 17 00:00:00 2001 From: zkldi <20380519+zkldi@users.noreply.github.com> Date: Wed, 31 Aug 2022 01:40:35 +0100 Subject: [PATCH 2/2] fix: improper logging of classes in score import --- .../framework/score-importing/score-importing.ts | 9 +++++---- server/src/utils/misc.ts | 10 ++++++++++ 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/server/src/lib/score-import/framework/score-importing/score-importing.ts b/server/src/lib/score-import/framework/score-importing/score-importing.ts index 4cc66843b..47257f681 100644 --- a/server/src/lib/score-import/framework/score-importing/score-importing.ts +++ b/server/src/lib/score-import/framework/score-importing/score-importing.ts @@ -5,6 +5,7 @@ import { IsConverterFailure } from "../common/converter-failures"; import { OrphanScore } from "../orphans/orphans"; import db from "external/mongo/db"; import { AppendLogCtx } from "lib/logger/logger"; +import { ClassToObject } from "utils/misc"; import type { ConverterFnSuccessReturn, ConverterFunction } from "../../import-types/common/types"; import type { ConverterFailure, KTDataNotFoundFailure } from "../common/converter-failures"; import type { DryScore } from "../common/types"; @@ -150,7 +151,7 @@ export async function ImportIterableDatapoint( const dnfErr = err as KTDataNotFoundFailure; logger.info(`KTDataNotFoundFailure: ${dnfErr.message}`, { - cfnReturn: dnfErr, + err: ClassToObject(dnfErr), hideFromConsole: ["cfnReturn"], }); @@ -196,7 +197,7 @@ export async function ImportIterableDatapoint( case "InvalidScore": { logger.info(`InvalidScoreFailure: ${err.message}`, { - cfnReturn: err, + err: ClassToObject(err), hideFromConsole: ["cfnReturn"], }); return { @@ -208,7 +209,7 @@ export async function ImportIterableDatapoint( } case "Internal": { - logger.error(`Internal error occured.`, { cfnReturn: err }); + logger.error(`Internal error occured.`, { err: ClassToObject(err) }); return { success: false, type: "InternalError", @@ -224,7 +225,7 @@ export async function ImportIterableDatapoint( default: { logger.warn(`Unknown error returned as ConverterFailure, Ignoring.`, { - err, + err: ClassToObject(err), }); return { success: false, diff --git a/server/src/utils/misc.ts b/server/src/utils/misc.ts index 5d8088bb5..b0324c41f 100644 --- a/server/src/utils/misc.ts +++ b/server/src/utils/misc.ts @@ -283,3 +283,13 @@ export function WrapScriptPromise(promise: Promise, logger: KtLogger) { }); }); } + +/** + * By default, winston won't serialise classes properly. + * + * This utility function ensures that an instance of a class becomes a json-valid + * object, that can be logged. + */ +export function ClassToObject(cls: unknown) { + return JSON.parse(JSON.stringify(cls)) as unknown; +}