Merge pull request #76 from bknd-io/feat/mm-revert-on-error

run schema mutations in safe proxy and revert to previous on error

README.md

@@ -18,10 +18,10 @@ bknd simplifies app development by providing a fully functional backend for data
 > and therefore full backward compatibility is not guaranteed before reaching v1.0.0.
 
 ## Size
-![gzipped size of bknd](https://img.badgesize.io/https://unpkg.com/bknd@0.6.1/dist/index.js?compression=gzip&label=bknd)
-![gzipped size of bknd/client](https://img.badgesize.io/https://unpkg.com/bknd@0.6.1/dist/ui/client/index.js?compression=gzip&label=bknd/client)
-![gzipped size of bknd/elements](https://img.badgesize.io/https://unpkg.com/bknd@0.6.1/dist/ui/elements/index.js?compression=gzip&label=bknd/elements)
-![gzipped size of bknd/ui](https://img.badgesize.io/https://unpkg.com/bknd@0.6.1/dist/ui/index.js?compression=gzip&label=bknd/ui)
+![gzipped size of bknd](https://img.badgesize.io/https://unpkg.com/bknd@0.7.1/dist/index.js?compression=gzip&label=bknd)
+![gzipped size of bknd/client](https://img.badgesize.io/https://unpkg.com/bknd@0.7.1/dist/ui/client/index.js?compression=gzip&label=bknd/client)
+![gzipped size of bknd/elements](https://img.badgesize.io/https://unpkg.com/bknd@0.7.1/dist/ui/elements/index.js?compression=gzip&label=bknd/elements)
+![gzipped size of bknd/ui](https://img.badgesize.io/https://unpkg.com/bknd@0.7.1/dist/ui/index.js?compression=gzip&label=bknd/ui)
 
 The size on npm is misleading, as the `bknd` package includes the backend, the ui components as well as the whole backend bundled into the cli including static assets. 
 

app/__test__/modules/ModuleManager.spec.ts

@@ -1,6 +1,7 @@
-import { describe, expect, test } from "bun:test";
-import { stripMark } from "../../src/core/utils";
+import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
+import { Type, disableConsoleLog, enableConsoleLog, stripMark } from "../../src/core/utils";
 import { entity, text } from "../../src/data";
+import { Module } from "../../src/modules/Module";
 import { ModuleManager, getDefaultConfig } from "../../src/modules/ModuleManager";
 import { CURRENT_VERSION, TABLE_NAME } from "../../src/modules/migrations";
 import { getDummyConnection } from "../helper";
@@ -252,4 +253,126 @@ describe("ModuleManager", async () => {
    });
 
    // @todo: add tests for migrations (check "backup" and new version)
+
+   describe("revert", async () => {
+      const failingModuleSchema = Type.Object({
+         value: Type.Optional(Type.Number())
+      });
+      class FailingModule extends Module<typeof failingModuleSchema> {
+         getSchema() {
+            return failingModuleSchema;
+         }
+
+         override async build() {
+            //console.log("building FailingModule", this.config);
+            if (this.config.value < 0) {
+               throw new Error("value must be positive");
+            }
+            this.setBuilt();
+         }
+      }
+      class TestModuleManager extends ModuleManager {
+         constructor(...args: ConstructorParameters<typeof ModuleManager>) {
+            super(...args);
+            const [, options] = args;
+            // @ts-ignore
+            const initial = options?.initial?.failing ?? {};
+            this.modules["failing"] = new FailingModule(initial, this.ctx());
+            this.modules["failing"].setListener(async (c) => {
+               // @ts-ignore
+               await this.onModuleConfigUpdated("failing", c);
+            });
+         }
+      }
+
+      beforeEach(() => disableConsoleLog(["log", "warn", "error"]));
+      afterEach(enableConsoleLog);
+
+      test("it builds", async () => {
+         const { dummyConnection } = getDummyConnection();
+         const mm = new TestModuleManager(dummyConnection);
+         expect(mm).toBeDefined();
+         await mm.build();
+         expect(mm.toJSON()).toBeDefined();
+      });
+
+      test("it accepts config", async () => {
+         const { dummyConnection } = getDummyConnection();
+         const mm = new TestModuleManager(dummyConnection, {
+            initial: {
+               // @ts-ignore
+               failing: { value: 2 }
+            }
+         });
+         await mm.build();
+         expect(mm.configs()["failing"].value).toBe(2);
+      });
+
+      test("it crashes on invalid", async () => {
+         const { dummyConnection } = getDummyConnection();
+         const mm = new TestModuleManager(dummyConnection, {
+            initial: {
+               // @ts-ignore
+               failing: { value: -1 }
+            }
+         });
+         expect(mm.build()).rejects.toThrow(/value must be positive/);
+         expect(mm.configs()["failing"].value).toBe(-1);
+      });
+
+      test("it correctly accepts valid", async () => {
+         const mockOnUpdated = mock(() => null);
+         const { dummyConnection } = getDummyConnection();
+         const mm = new TestModuleManager(dummyConnection, {
+            onUpdated: async () => {
+               mockOnUpdated();
+            }
+         });
+         await mm.build();
+         // @ts-ignore
+         const f = mm.mutateConfigSafe("failing");
+
+         expect(f.set({ value: 2 })).resolves.toBeDefined();
+         expect(mockOnUpdated).toHaveBeenCalled();
+      });
+
+      test("it reverts on safe mutate", async () => {
+         const mockOnUpdated = mock(() => null);
+         const { dummyConnection } = getDummyConnection();
+         const mm = new TestModuleManager(dummyConnection, {
+            initial: {
+               // @ts-ignore
+               failing: { value: 1 }
+            },
+            onUpdated: async () => {
+               mockOnUpdated();
+            }
+         });
+         await mm.build();
+         expect(mm.configs()["failing"].value).toBe(1);
+
+         // now safe mutate
+         // @ts-ignore
+         expect(mm.mutateConfigSafe("failing").set({ value: -2 })).rejects.toThrow(
+            /value must be positive/
+         );
+         expect(mm.configs()["failing"].value).toBe(1);
+         expect(mockOnUpdated).toHaveBeenCalled();
+      });
+
+      test("it only accepts schema mutating methods", async () => {
+         const { dummyConnection } = getDummyConnection();
+         const mm = new TestModuleManager(dummyConnection);
+         await mm.build();
+
+         // @ts-ignore
+         const f = mm.mutateConfigSafe("failing");
+
+         expect(() => f.has("value")).toThrow();
+         expect(() => f.bypass()).toThrow();
+         expect(() => f.clone()).toThrow();
+         expect(() => f.get()).toThrow();
+         expect(() => f.default()).toThrow();
+      });
+   });
 });

app/src/App.ts

@@ -58,13 +58,15 @@ export class App {
          onUpdated: async (key, config) => {
             // if the EventManager was disabled, we assume we shouldn't
             // respond to events, such as "onUpdated".
+            // this is important if multiple changes are done, and then build() is called manually
             if (!this.emgr.enabled) {
                console.warn("[APP] config updated, but event manager is disabled, skip.");
                return;
             }
 
             console.log("[APP] config updated", key);
-            await this.build({ sync: true, save: true });
+            // @todo: potentially double syncing
+            await this.build({ sync: true });
             await this.emgr.emit(new AppConfigUpdatedEvent({ app: this }));
          },
          onFirstBoot: async () => {
@@ -85,16 +87,10 @@ export class App {
       return this.modules.ctx().emgr;
    }
 
-   async build(options?: { sync?: boolean; drop?: boolean; save?: boolean }) {
+   async build(options?: { sync?: boolean }) {
+      if (options?.sync) this.modules.ctx().flags.sync_required = true;
       await this.modules.build();
 
-      if (options?.sync) {
-         const syncResult = await this.module.data.em
-            .schema()
-            .sync({ force: true, drop: options.drop });
-         //console.log("syncing", syncResult);
-      }
-
       const { guard, server } = this.modules.ctx();
 
       // load system controller
@@ -110,10 +106,6 @@ export class App {
 
       server.all("/api/*", async (c) => c.notFound());
 
-      if (options?.save) {
-         await this.modules.save();
-      }
-
       // first boot is set from ModuleManager when there wasn't a config table
       if (this.trigger_first_boot) {
          this.trigger_first_boot = false;
@@ -122,7 +114,7 @@ export class App {
    }
 
    mutateConfig<Module extends keyof Modules>(module: Module) {
-      return this.modules.get(module).schema();
+      return this.modules.mutateConfigSafe(module);
    }
 
    get server() {

app/src/modules/ModuleManager.ts

@@ -130,7 +130,7 @@ interface T_INTERNAL_EM {
 // @todo: cleanup old diffs on upgrade
 // @todo: cleanup multiple backups on upgrade
 export class ModuleManager {
-   private modules: Modules;
+   protected modules: Modules;
    // internal em for __bknd config table
    __em!: EntityManager<T_INTERNAL_EM>;
    // ctx for modules
@@ -433,44 +433,6 @@ export class ModuleManager {
       });
    }
 
-   private async buildModules(options?: { graceful?: boolean; ignoreFlags?: boolean }) {
-      this.logger.log("buildModules() triggered", options, this._built);
-      if (options?.graceful && this._built) {
-         this.logger.log("skipping build (graceful)");
-         return;
-      }
-
-      this.logger.log("building");
-      const ctx = this.ctx(true);
-      for (const key in this.modules) {
-         await this.modules[key].setContext(ctx).build();
-         this.logger.log("built", key);
-      }
-
-      this._built = true;
-      this.logger.log("modules built", ctx.flags);
-
-      if (options?.ignoreFlags !== true) {
-         if (ctx.flags.sync_required) {
-            ctx.flags.sync_required = false;
-            this.logger.log("db sync requested");
-
-            // sync db
-            await ctx.em.schema().sync({ force: true });
-            await this.save();
-         }
-
-         if (ctx.flags.ctx_reload_required) {
-            ctx.flags.ctx_reload_required = false;
-            this.logger.log("ctx reload requested");
-            this.ctx(true);
-         }
-      }
-
-      // reset all falgs
-      ctx.flags = Module.ctx_flags;
-   }
-
    async build() {
       this.logger.context("build").log("version", this.version());
       this.logger.log("booted with", this._booted_with);
@@ -503,8 +465,10 @@ export class ModuleManager {
             // it's up to date because we use default configs (no fetch result)
             this._version = CURRENT_VERSION;
             await this.syncConfigTable();
-            await this.buildModules();
-            await this.save();
+            const state = await this.buildModules();
+            if (!state.saved) {
+               await this.save();
+            }
 
             // run initial setup
             await this.setupInitial();
@@ -523,6 +487,60 @@ export class ModuleManager {
       return this;
    }
 
+   private async buildModules(options?: { graceful?: boolean; ignoreFlags?: boolean }) {
+      const state = {
+         built: false,
+         modules: [] as ModuleKey[],
+         synced: false,
+         saved: false,
+         reloaded: false
+      };
+
+      this.logger.log("buildModules() triggered", options, this._built);
+      if (options?.graceful && this._built) {
+         this.logger.log("skipping build (graceful)");
+         return state;
+      }
+
+      this.logger.log("building");
+      const ctx = this.ctx(true);
+      for (const key in this.modules) {
+         await this.modules[key].setContext(ctx).build();
+         this.logger.log("built", key);
+         state.modules.push(key as ModuleKey);
+      }
+
+      this._built = state.built = true;
+      this.logger.log("modules built", ctx.flags);
+
+      if (options?.ignoreFlags !== true) {
+         if (ctx.flags.sync_required) {
+            ctx.flags.sync_required = false;
+            this.logger.log("db sync requested");
+
+            // sync db
+            await ctx.em.schema().sync({ force: true });
+            state.synced = true;
+
+            // save
+            await this.save();
+            state.saved = true;
+         }
+
+         if (ctx.flags.ctx_reload_required) {
+            ctx.flags.ctx_reload_required = false;
+            this.logger.log("ctx reload requested");
+            this.ctx(true);
+            state.reloaded = true;
+         }
+      }
+
+      // reset all falgs
+      ctx.flags = Module.ctx_flags;
+
+      return state;
+   }
+
    protected async setupInitial() {
       const ctx = {
          ...this.ctx(),
@@ -538,6 +556,54 @@ export class ModuleManager {
       await this.options?.onFirstBoot?.();
    }
 
+   mutateConfigSafe<Module extends keyof Modules>(name: Module) {
+      const module = this.modules[name];
+      const copy = structuredClone(this.configs());
+
+      return new Proxy(module.schema(), {
+         get: (target, prop: string) => {
+            if (!["set", "patch", "overwrite", "remove"].includes(prop)) {
+               throw new Error(`Method ${prop} is not allowed`);
+            }
+
+            return async (...args) => {
+               console.log("[Safe Mutate]", name);
+               try {
+                  // overwrite listener to run build inside this try/catch
+                  module.setListener(async () => {
+                     await this.buildModules();
+                  });
+
+                  const result = await target[prop](...args);
+
+                  // revert to original listener
+                  module.setListener(async (c) => {
+                     await this.onModuleConfigUpdated(name, c);
+                  });
+
+                  // if there was an onUpdated listener, call it after success
+                  // e.g. App uses it to register module routes
+                  if (this.options?.onUpdated) {
+                     await this.options.onUpdated(name, module.config as any);
+                  }
+
+                  return result;
+               } catch (e) {
+                  console.error("[Safe Mutate] failed", e);
+
+                  // revert to previous config & rebuild using original listener
+                  this.setConfigs(copy);
+                  await this.onModuleConfigUpdated(name, module.config as any);
+                  console.log("[Safe Mutate] reverted");
+
+                  // make sure to throw the error
+                  throw e;
+               }
+            };
+         }
+      });
+   }
+
    get<K extends keyof Modules>(key: K): Modules[K] {
       if (!(key in this.modules)) {
          throw new Error(`Module "${key}" doesn't exist, cannot get`);

app/src/modules/server/AppServer.ts

@@ -1,10 +1,7 @@
-import { Exception, isDebug } from "core";
+import { Exception } from "core";
 import { type Static, StringEnum, Type } from "core/utils";
-import { Hono } from "hono";
 import { cors } from "hono/cors";
-import { timing } from "hono/timing";
 import { Module } from "modules/Module";
-import * as SystemPermissions from "modules/permissions";
 
 const serverMethods = ["GET", "POST", "PATCH", "PUT", "DELETE"];
 export const serverConfigSchema = Type.Object(

app/src/modules/server/SystemController.ts

@@ -256,17 +256,15 @@ export class SystemController extends Controller {
          tb(
             "query",
             Type.Object({
-               sync: Type.Optional(booleanLike),
-               drop: Type.Optional(booleanLike),
-               save: Type.Optional(booleanLike)
+               sync: Type.Optional(booleanLike)
             })
          ),
          async (c) => {
-            const { sync, drop, save } = c.req.valid("query") as Record<string, boolean>;
+            const { sync } = c.req.valid("query") as Record<string, boolean>;
             this.ctx.guard.throwUnlessGranted(SystemPermissions.build);
 
-            await this.app.build({ sync, drop, save });
-            return c.json({ success: true, options: { sync, drop, save } });
+            await this.app.build({ sync });
+            return c.json({ success: true, options: { sync } });
          }
       );