impr: add fallback and migrate to parsejsonwithschema (@miodec) (#6518)

!nuf

frontend/__tests__/utils/local-storage-with-schema.spec.ts

@@ -68,14 +68,16 @@ describe("local-storage-with-schema.ts", () => {
       expect(res).toEqual(defaultObject);
     });
 
-    it("should revert to the fallback value and remove if localstorage json is malformed", () => {
+    it("should revert to the fallback value if localstorage json is malformed", () => {
       getItemMock.mockReturnValue("badjson");
 
       const res = ls.get();
 
       expect(localStorage.getItem).toHaveBeenCalledWith("config");
-      expect(localStorage.removeItem).toHaveBeenCalledWith("config");
-      expect(localStorage.setItem).not.toHaveBeenCalled();
+      expect(localStorage.setItem).toHaveBeenCalledWith(
+        "config",
+        JSON.stringify(defaultObject)
+      );
       expect(res).toEqual(defaultObject);
     });
 
@@ -132,8 +134,7 @@ describe("local-storage-with-schema.ts", () => {
       expect(localStorage.getItem).toHaveBeenCalledWith("config");
       expect(migrateFnMock).toHaveBeenCalledWith(
         existingValue,
-        expect.any(Array),
-        defaultObject
+        expect.any(Array)
       );
       expect(localStorage.setItem).toHaveBeenCalledWith(
         "config",
@@ -167,8 +168,7 @@ describe("local-storage-with-schema.ts", () => {
       expect(localStorage.getItem).toHaveBeenCalledWith("config");
       expect(migrateFnMock).toHaveBeenCalledWith(
         existingValue,
-        expect.any(Array),
-        defaultObject
+        expect.any(Array)
       );
       expect(localStorage.setItem).toHaveBeenCalledWith(
         "config",

frontend/__tests__/utils/url-handler.spec.ts

@@ -251,16 +251,7 @@ describe("url-handler", () => {
 
       //THEN
       expect(addNotificationMock).toHaveBeenCalledWith(
-        `Failed to load test settings from URL: \"0\" Invalid enum value. Expected 'time' | 'words' | 'quote' | 'custom' | 'zen', received 'invalidMode'
-\"1\" Needs to be a number or a number represented as a string e.g. \"10\".
-\"2.text\" Expected array, received string
-\"2.mode\" Invalid enum value. Expected 'repeat' | 'random' | 'shuffle', received 'invalid'
-\"2.limit\" Expected object, received string
-\"2.pipeDelimiter\" Expected boolean, received string
-\"3\" Expected boolean, received string
-\"4\" Expected boolean, received string
-\"6\" Invalid enum value. Expected 'normal' | 'expert' | 'master', received 'invalid'
-\"7\" Invalid input`,
+        `Failed to load test settings from URL: JSON does not match schema: \"0\" invalid enum value. expected 'time' | 'words' | 'quote' | 'custom' | 'zen', received 'invalidmode', \"1\" needs to be a number or a number represented as a string e.g. \"10\"., \"2.text\" expected array, received string, \"2.mode\" invalid enum value. expected 'repeat' | 'random' | 'shuffle', received 'invalid', \"2.limit\" expected object, received string, \"2.pipeDelimiter\" expected boolean, received string, \"3\" expected boolean, received string, \"4\" expected boolean, received string, \"6\" invalid enum value. expected 'normal' | 'expert' | 'master', received 'invalid', \"7\" invalid input`,
         0
       );
     });

frontend/src/ts/commandline/lists.ts

@@ -94,21 +94,17 @@ import * as Misc from "../utils/misc";
 import * as JSONData from "../utils/json-data";
 import { randomizeTheme } from "../controllers/theme-controller";
 import * as CustomTextPopup from "../modals/custom-text";
-import * as Settings from "../pages/settings";
 import * as Notifications from "../elements/notifications";
 import * as VideoAdPopup from "../popups/video-ad-popup";
 import * as ShareTestSettingsPopup from "../modals/share-test-settings";
 import * as TestStats from "../test/test-stats";
 import * as QuoteSearchModal from "../modals/quote-search";
 import * as FPSCounter from "../elements/fps-counter";
-import { migrateConfig } from "../utils/config";
 import {
   CustomBackgroundSchema,
   CustomLayoutFluid,
-  PartialConfigSchema,
 } from "@monkeytype/contracts/schemas/configs";
 import { Command, CommandsSubgroup } from "./types";
-import { parseWithSchema as parseJsonWithSchema } from "@monkeytype/util/json";
 import * as TestLogic from "../test/test-logic";
 import * as ActivePage from "../states/active-page";
 
@@ -378,21 +374,7 @@ export const commands: CommandsSubgroup = {
       input: true,
       exec: async ({ input }): Promise<void> => {
         if (input === undefined || input === "") return;
-        try {
-          const parsedConfig = parseJsonWithSchema(
-            input,
-            PartialConfigSchema.strip()
-          );
-          await UpdateConfig.apply(migrateConfig(parsedConfig));
-          UpdateConfig.saveFullConfigToLocalStorage();
-          void Settings.update();
-          Notifications.add("Done", 1);
-        } catch (e) {
-          Notifications.add(
-            "An error occured while importing settings: " + e,
-            -1
-          );
-        }
+        await UpdateConfig.applyFromJson(input);
       },
     },
     {

frontend/src/ts/config.ts

@@ -14,6 +14,7 @@ import {
   canSetFunboxWithConfig,
 } from "./test/funbox/funbox-validation";
 import {
+  createErrorMessage,
   isDevEnvironment,
   isObject,
   promiseWithResolvers,
@@ -28,6 +29,7 @@ import { LocalStorageWithSchema } from "./utils/local-storage-with-schema";
 import { migrateConfig } from "./utils/config";
 import { roundTo1 } from "@monkeytype/util/numbers";
 import { getDefaultConfig } from "./constants/default-config";
+import { parseWithSchema as parseJsonWithSchema } from "@monkeytype/util/json";
 
 const configLS = new LocalStorageWithSchema({
   key: "config",
@@ -2132,6 +2134,30 @@ export function getConfigChanges(): Partial<Config> {
   return configChanges;
 }
 
+export async function applyFromJson(json: string): Promise<void> {
+  try {
+    const parsedConfig = parseJsonWithSchema(
+      json,
+      ConfigSchemas.PartialConfigSchema.strip(),
+      {
+        migrate: (value) => {
+          if (Array.isArray(value)) {
+            throw new Error("Invalid config");
+          }
+          return migrateConfig(value);
+        },
+      }
+    );
+    await apply(parsedConfig);
+    saveFullConfigToLocalStorage();
+    Notifications.add("Done", 1);
+  } catch (e) {
+    const msg = createErrorMessage(e, "Failed to import settings");
+    console.error(msg);
+    Notifications.add(msg, -1);
+  }
+}
+
 const { promise: loadPromise, resolve: loadDone } = promiseWithResolvers();
 
 export { loadPromise };

frontend/src/ts/modals/import-export-settings.ts

@@ -1,9 +1,5 @@
-import { PartialConfigSchema } from "@monkeytype/contracts/schemas/configs";
 import * as UpdateConfig from "../config";
-import * as Notifications from "../elements/notifications";
 import AnimatedModal from "../utils/animated-modal";
-import { migrateConfig } from "../utils/config";
-import { parseWithSchema as parseJsonWithSchema } from "@monkeytype/util/json";
 
 type State = {
   mode: "import" | "export";
@@ -47,23 +43,7 @@ const modal = new AnimatedModal({
         void modal.hide();
         return;
       }
-      try {
-        const parsedConfig = parseJsonWithSchema(
-          state.value,
-          PartialConfigSchema.strip()
-        );
-        await UpdateConfig.apply(migrateConfig(parsedConfig));
-      } catch (e) {
-        Notifications.add(
-          "Failed to import settings: incorrect data schema",
-          0
-        );
-        console.error(e);
-        void modal.hide();
-        return;
-      }
-      Notifications.add("Settings imported", 1);
-      UpdateConfig.saveFullConfigToLocalStorage();
+      await UpdateConfig.applyFromJson(state.value);
       void modal.hide();
     });
   },

frontend/src/ts/test/custom-text.ts

@@ -7,6 +7,7 @@ import {
 import { LocalStorageWithSchema } from "../utils/local-storage-with-schema";
 import { z } from "zod";
 import { CustomTextDataWithTextLen } from "@monkeytype/contracts/schemas/results";
+import { deepClone } from "../utils/misc";
 
 const CustomTextObjectSchema = z.record(z.string(), z.string());
 type CustomTextObject = z.infer<typeof CustomTextObjectSchema>;
@@ -51,7 +52,9 @@ const customTextSettings = new LocalStorageWithSchema({
   key: "customTextSettings",
   schema: CustomTextSettingsSchema,
   fallback: defaultCustomTextSettings,
-  migrate: (oldData, _zodIssues, fallback) => {
+  migrate: (oldData, _zodIssues) => {
+    const fallback = deepClone(defaultCustomTextSettings);
+
     if (typeof oldData !== "object" || oldData === null) {
       return fallback;
     }
@@ -60,7 +63,7 @@ const customTextSettings = new LocalStorageWithSchema({
       "text" in oldData &&
       z.array(z.string()).safeParse(migratedData.text).success
     ) {
-      migratedData.text = oldData.text as string[];
+      migratedData.text = oldData["text"] as string[];
     }
     return migratedData;
   },

frontend/src/ts/utils/config.ts

@@ -14,6 +14,8 @@ import { getDefaultConfig } from "../constants/default-config";
  * @returns
  */
 export function migrateConfig(config: PartialConfig | object): Config {
+  //todo this assumes config is matching all schemas
+  //i think we should go through each value and validate
   return mergeWithDefaultConfig(replaceLegacyValues(config));
 }
 

frontend/src/ts/utils/local-storage-with-schema.ts

@@ -1,20 +1,26 @@
 import { ZodIssue } from "zod";
-import { deepClone } from "./misc";
 import { isZodError } from "@monkeytype/util/zod";
 import * as Notifications from "../elements/notifications";
 import { tryCatchSync } from "@monkeytype/util/trycatch";
+import { parseWithSchema as parseJsonWithSchema } from "@monkeytype/util/json";
 
 export class LocalStorageWithSchema<T> {
   private key: string;
   private schema: Zod.Schema<T>;
   private fallback: T;
-  private migrate?: (value: unknown, zodIssues: ZodIssue[], fallback: T) => T;
+  private migrate?: (
+    value: Record<string, unknown> | unknown[],
+    zodIssues?: ZodIssue[]
+  ) => T;
 
   constructor(options: {
     key: string;
     schema: Zod.Schema<T>;
     fallback: T;
-    migrate?: (value: unknown, zodIssues: ZodIssue[], fallback: T) => T;
+    migrate?: (
+      value: Record<string, unknown> | unknown[],
+      zodIssues?: ZodIssue[]
+    ) => T;
   }) {
     this.key = options.key;
     this.schema = options.schema;
@@ -29,49 +35,31 @@ export class LocalStorageWithSchema<T> {
       return this.fallback;
     }
 
-    const { data: jsonParsed, error } = tryCatchSync(
-      () => JSON.parse(value) as unknown
+    let migrated = false;
+    const { data: parsed, error } = tryCatchSync(() =>
+      parseJsonWithSchema(value, this.schema, {
+        fallback: this.fallback,
+        migrate: (oldData, zodIssues) => {
+          migrated = true;
+          if (this.migrate) {
+            return this.migrate(oldData, zodIssues);
+          } else {
+            return this.fallback;
+          }
+        },
+      })
     );
+
     if (error) {
-      console.log(
-        `Value from localStorage ${this.key} was not a valid JSON, using fallback`,
-        error
-      );
-      window.localStorage.removeItem(this.key);
+      window.localStorage.setItem(this.key, JSON.stringify(this.fallback));
       return this.fallback;
     }
 
-    const schemaParsed = this.schema.safeParse(jsonParsed);
-
-    if (schemaParsed.success) {
-      return schemaParsed.data;
+    if (migrated || parsed === this.fallback) {
+      window.localStorage.setItem(this.key, JSON.stringify(parsed));
     }
 
-    console.log(
-      `Value from localStorage ${this.key} failed schema validation, migrating`,
-      schemaParsed.error.issues
-    );
-
-    let newValue = this.fallback;
-    if (this.migrate) {
-      const migrated = this.migrate(
-        jsonParsed,
-        schemaParsed.error.issues,
-        deepClone(this.fallback)
-      );
-      const parse = this.schema.safeParse(migrated);
-      if (parse.success) {
-        newValue = migrated;
-      } else {
-        console.error(
-          `Value from localStorage ${this.key} failed schema validation after migration! This is very bad!`,
-          parse.error.issues
-        );
-      }
-    }
-
-    window.localStorage.setItem(this.key, JSON.stringify(newValue));
-    return newValue;
+    return parsed;
   }
 
   public set(data: T): boolean {

packages/util/__test__/json.spec.ts

@@ -8,6 +8,13 @@ describe("json", () => {
       name: z.string(),
       nested: z.object({ foo: z.string() }).strict().optional(),
     });
+    it("should throw with invalid json", () => {
+      expect(() => parseWithSchema("blah", schema)).toThrowError(
+        new Error(
+          `Invalid JSON: Unexpected token 'b', "blah" is not valid JSON`
+        )
+      );
+    });
     it("should parse", () => {
       const json = `{
         "test":true,
@@ -24,7 +31,7 @@ describe("json", () => {
         nested: { foo: "bar" },
       });
     });
-    it("should fail with invalid schema", () => {
+    it("should throw with invalid schema", () => {
       const json = `{
             "test":"yes",
             "nested":{
@@ -34,9 +41,106 @@ describe("json", () => {
 
       expect(() => parseWithSchema(json, schema)).toThrowError(
         new Error(
-          `"test" Expected boolean, received string\n"name" Required\n"nested.foo" Expected string, received number`
+          `JSON does not match schema: "test" expected boolean, received string, "name" required, "nested.foo" expected string, received number`
         )
       );
     });
+    it("should migrate if valid json", () => {
+      const json = `{
+          "name": 1
+          }`;
+
+      const result = parseWithSchema(json, schema, {
+        migrate: () => {
+          return {
+            name: "migrated",
+            test: false,
+          };
+        },
+      });
+
+      expect(result).toStrictEqual({
+        name: "migrated",
+        test: false,
+      });
+    });
+    it("should revert to fallback if invalid json", () => {
+      const json = `blah`;
+
+      const result = parseWithSchema(json, schema, {
+        fallback: {
+          name: "migrated",
+          test: false,
+        },
+      });
+
+      expect(result).toStrictEqual({
+        name: "migrated",
+        test: false,
+      });
+    });
+    it("should throw if migration fails", () => {
+      const json = `{
+          "name": 1
+          }`;
+
+      expect(() => {
+        parseWithSchema(json, schema, {
+          //@ts-expect-error need to test migration failure
+          migrate: () => {
+            return {
+              name: null,
+              test: "Hi",
+            };
+          },
+        });
+      }).toThrowError(
+        new Error(
+          `Migrated value does not match schema: "test" expected boolean, received string, "name" expected string, received null`
+        )
+      );
+    });
+    it("should revert to fallback if migration fails", () => {
+      const json = `{
+          "name": 1
+          }`;
+
+      const result = parseWithSchema(json, schema, {
+        fallback: {
+          name: "fallback",
+          test: false,
+        },
+        //@ts-expect-error need to test migration failure
+        migrate: () => {
+          return {
+            name: null,
+            test: "Hi",
+          };
+        },
+      });
+
+      expect(result).toStrictEqual({
+        name: "fallback",
+        test: false,
+      });
+    });
+    it("migrate function should receive value", () => {
+      const json = `{
+        "test":"test"
+      }`;
+
+      const result = parseWithSchema(json, schema, {
+        migrate: (value) => {
+          expect(value).toEqual({ test: "test" });
+          return {
+            name: "valid",
+          };
+        },
+      });
+
+      expect(result).toStrictEqual({
+        name: "valid",
+      });
+    });
   });
 });

packages/util/src/json.ts

@@ -1,32 +1,75 @@
 import { z, ZodIssue } from "zod";
-import { isZodError } from "./zod";
+import { tryCatchSync } from "./trycatch";
 
 /**
  * Parse a JSON string into an object and validate it against a schema
  * @param json  JSON string
  * @param schema  Zod schema to validate the JSON against
+ * @param fallbackAndMigrate  Optional object containing optional fallback value and optional migration function
+ * @throws Error if JSON is invalid and no fallback is provided
+ * @throws Error if JSON does not match schema and no migration function is provided
  * @returns  The parsed JSON object
  */
 export function parseWithSchema<T extends z.ZodTypeAny>(
   json: string,
-  schema: T
-): z.infer<T> {
-  try {
-    const jsonParsed = JSON.parse(json) as unknown;
-    // hits is fine to ignore
-    // eslint-disable-next-line @typescript-eslint/no-unsafe-return
-    return schema.parse(jsonParsed) as z.infer<T>;
-  } catch (error) {
-    if (isZodError(error)) {
-      throw new Error(error.issues.map(prettyErrorMessage).join("\n"));
-    } else {
-      throw error;
-    }
+  schema: T,
+  fallbackAndMigrate?: {
+    fallback?: z.infer<T>;
+    migrate?: (
+      value: Record<string, unknown> | unknown[],
+      zodIssues?: ZodIssue[]
+    ) => z.infer<T>;
   }
+): z.infer<T> {
+  const { fallback, migrate } = fallbackAndMigrate ?? {};
+
+  const { data: jsonParsed, error } = tryCatchSync(
+    () => JSON.parse(json) as Record<string, unknown>
+  );
+
+  if (error) {
+    if (fallback === undefined) {
+      throw new Error(`Invalid JSON: ` + error.message);
+    }
+    // todo fix me
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-return
+    return fallback as z.infer<T>;
+  }
+
+  const safeParse = schema.safeParse(jsonParsed);
+  if (safeParse.success) {
+    return safeParse.data as T;
+  }
+
+  if (migrate === undefined) {
+    throw new Error(
+      `JSON does not match schema: ${safeParse.error.issues
+        .map(prettyErrorMessage)
+        .join(", ")}`
+    );
+  }
+
+  const migrated = migrate(jsonParsed, safeParse.error.issues);
+  const safeParseMigrated = schema.safeParse(migrated);
+
+  if (!safeParseMigrated.success) {
+    if (fallback === undefined) {
+      throw new Error(
+        `Migrated value does not match schema: ${safeParseMigrated.error.issues
+          .map(prettyErrorMessage)
+          .join(", ")}`
+      );
+    }
+    // todo fix me
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-return
+    return fallback as z.infer<T>;
+  }
+
+  return safeParseMigrated.data as T;
 }
 
 function prettyErrorMessage(issue: ZodIssue | undefined): string {
   if (issue === undefined) return "";
   const path = issue.path.length > 0 ? `"${issue.path.join(".")}" ` : "";
-  return `${path}${issue.message}`;
+  return `${path}${issue.message.toLowerCase()}`;
 }