Improved error handling (#4)

kevinwang5658 · web-flow · commit 28d28e65190c · 2024-05-27T15:27:59.000-04:00
* Added improved error handling. All errors from the plugin (including validation errors) are caught now and returned as status: error

* Improved try catch block. Added isDebug check to return error stack instead of just the message. Fixed tests

* Fixed transform value bug

* Bug fixes for create plan not using the transformed desired parameters

* Modified parameter to canModify
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "codify-plugin-lib",
-  "version": "1.0.55",
+  "version": "1.0.57",
   "description": "",
   "main": "dist/index.js",
   "typings": "dist/index.d.ts",
diff --git a/src/entities/plan-types.ts b/src/entities/plan-types.ts
@@ -1,13 +1,11 @@
-import { ResourceOperation } from 'codify-schemas';
-
 /**
  * Customize properties for specific parameters. This will alter the way the library process changes to the parameter.
  */
 export interface ParameterOptions {
   /**
-   * Chose if the resource should be re-created or modified if this parameter is changed. Defaults to re-create.
+   * Chose if the resource should be re-created or modified if this parameter is changed. Defaults to false (re-creates resource on change).
    */
-  planOperation?: ResourceOperation.MODIFY | ResourceOperation.RECREATE;
+  canModify?: boolean;
   /**
    * Customize the equality comparison for a parameter.
    * @param a
diff --git a/src/entities/plan.ts b/src/entities/plan.ts
@@ -55,8 +55,8 @@ export class Plan<T extends StringIndexedObject> {
           let newOperation: ResourceOperation;
           if (statefulParameterNames.has(curr.name)) {
             newOperation = ResourceOperation.MODIFY // All stateful parameters are modify only
-          } else if (parameterOptions[curr.name]?.planOperation) {
-            newOperation = parameterOptions[curr.name].planOperation!;
+          } else if (parameterOptions[curr.name]?.canModify) {
+            newOperation = parameterOptions[curr.name].canModify ? ResourceOperation.MODIFY : ResourceOperation.RECREATE;
           } else {
             newOperation = ResourceOperation.RECREATE; // Default to Re-create. Should handle the majority of use cases
           }
diff --git a/src/entities/plugin.ts b/src/entities/plugin.ts
@@ -15,6 +15,7 @@ export class Plugin {
   planStorage: Map<string, Plan<ResourceConfig>>;
 
   constructor(
+    public name: string,
     public resources: Map<string, Resource<ResourceConfig>>
   ) {
     this.planStorage = new Map();
diff --git a/src/entities/resource-parameters.test.ts b/src/entities/resource-parameters.test.ts
@@ -80,7 +80,7 @@ describe('Resource parameter tests', () => {
           type: 'resource',
           parameterOptions: {
             propA: { statefulParameter: statefulParameterSpy },
-            propB: { planOperation: ResourceOperation.MODIFY },
+            propB: { canModify: true },
           }
         });
       }
diff --git a/src/entities/resource-types.ts b/src/entities/resource-types.ts
@@ -1,15 +1,13 @@
-import { ResourceOperation } from 'codify-schemas';
-
 export type ErrorMessage = string;
 
 /**
  * Customize properties for specific parameters. This will alter the way the library process changes to the parameter.
  */
 export interface ResourceParameterOptions {
   /**
-   * Chose if the resource should be re-created or modified if this parameter is changed. Defaults to re-create.
+   * Chose if the resource should be re-created or modified if this parameter is changed. Defaults to false (re-create).
    */
-  planOperation?: ResourceOperation.MODIFY | ResourceOperation.RECREATE;
+  canModify?: boolean;
   /**
    * Customize the equality comparison for a parameter.
    * @param desired
@@ -19,7 +17,7 @@ export interface ResourceParameterOptions {
   /**
    * Default value for the parameter. If a value is not provided in the config, the library will use this value.
    */
-  default?: unknown,
+  default?: unknown;
 }
 
 /**
diff --git a/src/entities/resource.test.ts b/src/entities/resource.test.ts
@@ -195,8 +195,8 @@ describe('Resource tests', () => {
         super({
           type: 'resource',
           parameterOptions: {
-            propA: { planOperation: ResourceOperation.MODIFY },
-            propB: { planOperation: ResourceOperation.MODIFY },
+            propA: { canModify: true },
+            propB: { canModify: true },
           }
         });
       }
@@ -244,7 +244,7 @@ describe('Resource tests', () => {
           type: 'type',
           dependencies: ['homebrew', 'python'],
           parameterOptions: {
-            propA: { planOperation: ResourceOperation.MODIFY },
+            propA: { canModify: true },
             propB: { statefulParameter },
             propC: { isEqual: (a, b) => true },
           }
@@ -281,7 +281,7 @@ describe('Resource tests', () => {
           type: 'type',
           dependencies: ['homebrew', 'python'],
           parameterOptions: {
-            propA: { planOperation: ResourceOperation.MODIFY },
+            propA: { canModify: true },
             propB: { statefulParameter },
             propC: { isEqual: (a, b) => true },
           }
diff --git a/src/entities/resource.ts b/src/entities/resource.ts
@@ -101,7 +101,7 @@ export abstract class Resource<T extends StringIndexedObject> {
 
     // Short circuit here. If the resource is non-existent, there's no point checking stateful parameters
     if (currentParameters == null) {
-      return Plan.create(desiredParameters, null, resourceMetadata, planOptions);
+      return Plan.create(resourceParameters, null, resourceMetadata, planOptions);
     }
 
     // Refresh stateful parameters. These parameters have state external to the resource
@@ -161,7 +161,7 @@ export abstract class Resource<T extends StringIndexedObject> {
 
     for (const pc of statelessParameterChanges) {
       // TODO: When stateful mode is added in the future. Dynamically choose if deletes are allowed
-      await this.applyModify(pc.name, pc.newValue, pc.previousValue, false, plan);
+      await this.applyModify(pc, plan);
     }
 
     const statefulParameterChanges = parameterChanges
@@ -228,21 +228,17 @@ Additional: ${[...refreshKeys].filter(k => !desiredKeys.has(k))};`
     const orderedEntries = [...Object.entries(transformParameters)]
       .sort(([keyA], [keyB]) => this.transformParameterOrder.get(keyA)! - this.transformParameterOrder.get(keyB)!)
 
-    for (const [key] of orderedEntries) {
-      if (desired[key] !== null) {
-        const transformedValue = await this.transformParameters.get(key)!.transform(desired[key]);
+    for (const [key, value] of orderedEntries) {
+      const transformedValue = await this.transformParameters.get(key)!.transform(value);
 
-        if (Object.keys(transformedValue).some((k) => desired[k] !== undefined)) {
-          throw new Error(`Transform parameter ${key as string} is attempting to override existing value ${desired[key]}`);
-        }
-
-        Object.entries(transformedValue).forEach(([tvKey, tvValue]) => {
-          // @ts-ignore
-          desired[tvKey] = tvValue;
-        })
-
-        delete desired[key];
+      if (Object.keys(transformedValue).some((k) => desired[k] !== undefined)) {
+        throw new Error(`Transform parameter ${key as string} is attempting to override existing values ${JSON.stringify(transformedValue, null, 2)}`);
       }
+
+      Object.entries(transformedValue).forEach(([tvKey, tvValue]) => {
+        // @ts-ignore
+        desired[tvKey] = tvValue;
+      })
     }
   }
 
@@ -314,7 +310,7 @@ Additional: ${[...refreshKeys].filter(k => !desiredKeys.has(k))};`
 
   abstract applyCreate(plan: Plan<T>): Promise<void>;
 
-  async applyModify(parameterName: keyof T, newValue: unknown, previousValue: unknown, allowDeletes: boolean, plan: Plan<T>): Promise<void> {};
+  async applyModify(pc: ParameterChange<T>, plan: Plan<T>): Promise<void> {};
 
   abstract applyDestroy(plan: Plan<T>): Promise<void>;
 }
diff --git a/src/entities/transform-parameter.ts b/src/entities/transform-parameter.ts
@@ -1,5 +1,11 @@
 import { StringIndexedObject } from 'codify-schemas';
 
+/**
+ * Transform parameters convert the provided value into
+ * other parameters. Transform parameters will not show up
+ * in the refresh or the plan. Transform parameters get processed after
+ * default values.
+ */
 export abstract class TransformParameter<T extends StringIndexedObject> {
 
   abstract transform(value: any): Promise<Partial<T>>
diff --git a/src/index.ts b/src/index.ts
@@ -16,4 +16,3 @@ export async function runPlugin(plugin: Plugin) {
   const messageHandler = new MessageHandler(plugin);
   process.on('message', (message) => messageHandler.onMessage(message))
 }
-export { ErrorMessage } from './entities/resource-types.js';
diff --git a/src/messages/handlers.test.ts b/src/messages/handlers.test.ts
@@ -1,14 +1,25 @@
 import { MessageHandler } from './handlers.js';
 import { Plugin } from '../entities/plugin.js';
-import { describe, it, expect } from 'vitest';
-import { vi } from 'vitest'
+import { describe, expect, it } from 'vitest';
 import { mock } from 'vitest-mock-extended'
+import { Resource } from '../entities/resource.js';
+import { Plan } from '../entities/plan.js';
+import { MessageStatus, ResourceOperation } from 'codify-schemas';
 
 describe('Message handler tests', () => {
   it('handles plan requests', async () => {
     const plugin = mock<Plugin>();
     const handler = new MessageHandler(plugin);
 
+    process.send = (message) => {
+      expect(message).toMatchObject({
+        cmd: 'plan_Response',
+        status: MessageStatus.SUCCESS,
+      });
+
+      return true;
+    }
+
     // Message handler also validates the response. That part does not need to be tested
     try {
       await handler.onMessage({
@@ -23,12 +34,23 @@ describe('Message handler tests', () => {
     } catch (e) {}
 
     expect(plugin.plan.mock.calls.length).to.eq(1);
+    process.send = undefined;
   })
 
   it('rejects bad plan requests', async () => {
     const plugin = mock<Plugin>();
     const handler = new MessageHandler(plugin);
 
+    process.send = (message) => {
+      console.log(message);
+      expect(message).toMatchObject({
+        cmd: 'plan_Response',
+        status: MessageStatus.ERROR,
+      });
+
+      return true;
+    }
+
     // Message handler also validates the response. That part does not need to be tested
     try {
       await handler.onMessage({
@@ -39,9 +61,12 @@ describe('Message handler tests', () => {
           prop2: 'B',
         }
       })
-    } catch (e) {}
+    } catch (e) {
+      console.log(e);
+    }
 
     expect(plugin.plan.mock.calls.length).to.eq(0);
+    process.send = undefined;
   })
 
   it('handles apply requests', async () => {
@@ -110,15 +135,123 @@ describe('Message handler tests', () => {
     const plugin = mock<Plugin>();
     const handler = new MessageHandler(plugin);
 
+    process.send = () => true;
+
     // Message handler also validates the response. That part does not need to be tested
-    try {
-      await handler.onMessage({
-        cmd: 'validate',
-        data: {}
-      })
-    } catch (e) {}
+    // This should not throw
+    expect(await handler.onMessage({
+      cmd: 'validate',
+      data: {}
+    })).to.eq(undefined);
 
     expect(plugin.apply.mock.calls.length).to.be.eq(0);
   })
 
+  it('handles errors for plan', async () => {
+    const resource= testResource();
+    const plugin = testPlugin(resource);
+
+    const handler = new MessageHandler(plugin);
+
+    process.send = (message) => {
+      expect(message).toMatchObject({
+        cmd: 'plan_Response',
+        status: MessageStatus.ERROR,
+        data: 'Refresh error',
+      })
+      return true;
+    }
+
+    expect(async () => await handler.onMessage({
+      cmd: 'plan',
+      data: {
+        type: 'resourceA'
+      }
+    })).rejects.to.not.throw;
+
+    process.send = undefined;
+  })
+
+  it('handles errors for apply (create)', async () => {
+    const resource= testResource();
+    const plugin = testPlugin(resource);
+
+    const handler = new MessageHandler(plugin);
+
+    process.send = (message) => {
+      expect(message).toMatchObject({
+        cmd: 'apply_Response',
+        status: MessageStatus.ERROR,
+        data: 'Create error',
+      })
+      return true;
+    }
+
+    expect(async () => await handler.onMessage({
+      cmd: 'apply',
+      data: {
+        plan: {
+          resourceType: 'resourceA',
+          operation: ResourceOperation.CREATE,
+          parameters: []
+        }
+      }
+    })).rejects.to.not.throw;
+  })
+
+  it('handles errors for apply (destroy)', async () => {
+    const resource= testResource();
+    const plugin = testPlugin(resource);
+
+    const handler = new MessageHandler(plugin);
+
+    process.send = (message) => {
+      expect(message).toMatchObject({
+        cmd: 'apply_Response',
+        status: MessageStatus.ERROR,
+        data: 'Destroy error',
+      })
+      return true;
+    }
+
+    expect(async () => await handler.onMessage({
+      cmd: 'apply',
+      data: {
+        plan: {
+          resourceType: 'resourceA',
+          operation: ResourceOperation.DESTROY,
+          parameters: []
+        }
+      }
+    })).rejects.to.not.throw;
+  })
+
+
+  const testResource = () => new class extends Resource<any> {
+    constructor() {
+      super({ type: 'resourceA' });
+    }
+
+    async refresh(keys: Map<keyof any, any>): Promise<Partial<any> | null> {
+      throw new Error('Refresh error');
+    }
+
+    applyCreate(plan: Plan<any>): Promise<void> {
+      throw new Error('Create error');
+    }
+
+    applyDestroy(plan: Plan<any>): Promise<void> {
+      throw new Error('Destroy error');
+    }
+  }
+
+  const testPlugin = (resource: Resource<any>) => new class extends Plugin {
+    constructor() {
+      const map = new Map();
+      map.set('resourceA', resource);
+
+      super('name', map);
+    }
+  }
+
 });
diff --git a/src/messages/handlers.ts b/src/messages/handlers.ts
diff --git a/src/utils/test-utils.test.ts b/src/utils/test-utils.test.ts

Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"name": "codify-plugin-lib",`
`3`		`- "version": "1.0.55",`
	`3`	`+ "version": "1.0.57",`
`4`	`4`	`"description": "",`
`5`	`5`	`"main": "dist/index.js",`
`6`	`6`	`"typings": "dist/index.d.ts",`
Original file line number	Diff line number	Diff line change
`@@ -80,7 +80,7 @@ describe('Resource parameter tests', () => {`
`80`	`80`	`type: 'resource',`
`81`	`81`	`parameterOptions: {`
`82`	`82`	`propA: { statefulParameter: statefulParameterSpy },`
`83`		`- propB: { planOperation: ResourceOperation.MODIFY },`
	`83`	`+ propB: { canModify: true },`
`84`	`84`	`}`
`85`	`85`	`});`
`86`	`86`	`}`
Original file line number	Diff line number	Diff line change
`@@ -16,4 +16,3 @@ export async function runPlugin(plugin: Plugin) {`
`16`	`16`	`const messageHandler = new MessageHandler(plugin);`
`17`	`17`	`process.on('message', (message) => messageHandler.onMessage(message))`
`18`	`18`	`}`
`19`		`-export { ErrorMessage } from './entities/resource-types.js';`