Merge pull request #661 from TNG-dev/zkldi/no-instanceof

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<T extends ImportTypes> 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<T extends ImportTypes> 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;
+}

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.`
 		);

server/src/lib/score-import/framework/score-importing/score-importing.ts

@@ -1,21 +1,13 @@
 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 { 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";
 import type { KtLogger } from "lib/logger/logger";
 import type { ScoreImportJob } from "lib/score-import/worker/types";
@@ -128,104 +120,121 @@ export async function ImportIterableDatapoint<D, C>(
 	logger: KtLogger
 ): Promise<ImportProcessingInfo | null> {
 	// 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<ImportTypes>;
 
-			const insertOrphan = await OrphanScore(
-				cfnReturn.importType,
-				userID,
-				cfnReturn.data,
-				cfnReturn.converterContext,
-				cfnReturn.message,
-				game,
-				logger
-			);
+				logger.info(`KTDataNotFoundFailure: ${dnfErr.message}`, {
+					err: ClassToObject(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}`, {
+					err: ClassToObject(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.`, { err: ClassToObject(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: ClassToObject(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);

server/src/utils/misc.ts

@@ -283,3 +283,13 @@ export function WrapScriptPromise(promise: Promise<unknown>, 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;
+}