Added validation plan after apply in plugins. This plan will check that noop is returned for the same plan. Added tests. Changed constructor for plugin to be Plugin.create(). Added handling for apply validation errors. Changed canModify to modifyOnChange.

kevinwang5658 · kevinwang5658 · commit 9c7c7657c362 · 2024-06-03T08:36:32.000-04:00
diff --git a/src/entities/errors.ts b/src/entities/errors.ts
@@ -1,3 +1,6 @@
+import { Plan } from './plan.js';
+import { StringIndexedObject } from 'codify-schemas';
+
 export class SudoError extends Error {
   command: string;
 
@@ -6,3 +9,17 @@ export class SudoError extends Error {
     this.command = command;
   }
 }
+
+export class ApplyValidationError<T extends StringIndexedObject> extends Error {
+  desiredPlan: Plan<T>;
+  validatedPlan: Plan<T>;
+
+  constructor(
+    desiredPlan: Plan<T>,
+    validatedPlan: Plan<T>
+  ) {
+    super();
+    this.desiredPlan = desiredPlan;
+    this.validatedPlan = validatedPlan;
+  }
+}
diff --git a/src/entities/plan-types.ts b/src/entities/plan-types.ts
@@ -5,7 +5,7 @@ export interface ParameterOptions {
   /**
    * Chose if the resource should be re-created or modified if this parameter is changed. Defaults to false (re-creates resource on change).
    */
-  canModify?: boolean;
+  modifyOnChange?: 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]?.canModify) {
-            newOperation = parameterOptions[curr.name].canModify ? ResourceOperation.MODIFY : ResourceOperation.RECREATE;
+          } else if (parameterOptions[curr.name]?.modifyOnChange) {
+            newOperation = parameterOptions[curr.name].modifyOnChange ? ResourceOperation.MODIFY : ResourceOperation.RECREATE;
           } else {
             newOperation = ResourceOperation.RECREATE; // Default to Re-create. Should handle the majority of use cases
           }
@@ -75,7 +75,7 @@ export class Plan<T extends StringIndexedObject> {
     return this.resourceMetadata.type
   }
 
-  static fromResponse<T extends ResourceConfig>(data: ApplyRequestData['plan'], defaultValues: Partial<Record<keyof T, unknown>>): Plan<T> {
+  static fromResponse<T extends ResourceConfig>(data: ApplyRequestData['plan'], defaultValues?: Partial<Record<keyof T, unknown>>): Plan<T> {
     if (!data) {
       throw new Error('Data is empty');
     }
@@ -95,7 +95,7 @@ export class Plan<T extends StringIndexedObject> {
     );
 
    function addDefaultValues(): void {
-      Object.entries(defaultValues)
+      Object.entries(defaultValues ?? {})
         .forEach(([key, defaultValue]) => {
           const configValueExists = data!
             .parameters
diff --git a/src/entities/plugin.test.ts b/src/entities/plugin.test.ts
@@ -0,0 +1,96 @@
+import { describe, expect, it } from 'vitest';
+import { Plugin } from './plugin.js';
+import { ParameterOperation, ResourceOperation, StringIndexedObject } from 'codify-schemas';
+import { Resource } from './resource.js';
+import { Plan } from './plan.js';
+import { ValidationResult } from './resource-types.js';
+import { ApplyValidationError } from './errors.js';
+
+interface TestConfig extends StringIndexedObject {
+  propA: string;
+  propB: number;
+  propC?: string;
+}
+
+class TestResource extends Resource<TestConfig> {
+  constructor() {
+    super({
+      type: 'testResource'
+    });
+  }
+
+  applyCreate(plan: Plan<TestConfig>): Promise<void> {
+    return Promise.resolve(undefined);
+  }
+
+  applyDestroy(plan: Plan<TestConfig>): Promise<void> {
+    return Promise.resolve(undefined);
+  }
+
+  async refresh(keys: Map<string, unknown>): Promise<Partial<TestConfig> | null> {
+    return {
+      propA: 'a',
+      propB: 10,
+      propC: 'c',
+    };
+  }
+
+  async validateResource(config: unknown): Promise<ValidationResult> {
+    return {
+      isValid: true
+    }
+  }
+}
+
+describe('Plugin tests', () => {
+  it('Validates that applies were successfully applied', async () => {
+    const resource = new class extends TestResource {
+      async applyCreate(plan: Plan<TestConfig>): Promise<void> {
+      }
+
+      // Refresh has to line up with desired for the apply to go through
+      async refresh(keys: Map<string, unknown>): Promise<Partial<TestConfig> | null> {
+        return {
+          propA: 'abc'
+        }
+      }
+    }
+
+    const testPlugin = Plugin.create('testPlugin', [resource])
+
+    const desiredPlan = {
+      operation: ResourceOperation.CREATE,
+      resourceType: 'testResource',
+      parameters: [
+        { name: 'propA', operation: ParameterOperation.ADD, newValue: 'abc', previousValue: null },
+      ]
+    };
+
+    // If this doesn't throw then it passes the test
+    await testPlugin.apply({ plan: desiredPlan });
+  });
+
+  it('Validates that applies were successfully applied (error)', async () => {
+    const resource = new class extends TestResource {
+      async applyCreate(plan: Plan<TestConfig>): Promise<void> {
+      }
+
+      // Return null to indicate that the resource was not created
+      async refresh(keys: Map<string, unknown>): Promise<Partial<TestConfig> | null> {
+        return null;
+      }
+    }
+
+    const testPlugin = Plugin.create('testPlugin', [resource])
+
+    const desiredPlan = {
+      operation: ResourceOperation.CREATE,
+      resourceType: 'testResource',
+      parameters: [
+        { name: 'propA', operation: ParameterOperation.ADD, newValue: 'abc', previousValue: null },
+      ]
+    };
+
+    await expect(async () => testPlugin.apply({ plan: desiredPlan })).rejects.toThrowError(expect.any(ApplyValidationError));
+  });
+});
diff --git a/src/entities/plugin.ts b/src/entities/plugin.ts
@@ -5,14 +5,24 @@ import {
   PlanRequestData,
   PlanResponseData,
   ResourceConfig,
+  ResourceOperation,
   ValidateRequestData,
   ValidateResponseData
 } from 'codify-schemas';
 import { Plan } from './plan.js';
 import { splitUserConfig } from '../utils/utils.js';
+import { ApplyValidationError } from './errors.js';
 
 export class Plugin {
-  planStorage: Map<string, Plan<ResourceConfig>>;
+  planStorage: Map<string, Plan<any>>;
+
+  static create(name: string, resources: Resource<any>[]) {
+    const resourceMap = new Map<string, Resource<any>>(
+      resources.map((r) => [r.typeId, r] as const)
+    );
+
+    return new Plugin(name, resourceMap);
+  }
 
   constructor(
     public name: string,
@@ -82,6 +92,13 @@ export class Plugin {
     }
 
     await resource.apply(plan);
+
+    // Perform a validation check after to ensure that the plan was properly applied.
+    // Sometimes no errors are returned (exit code 0) but the apply was not successful
+    const validationPlan = await resource.plan({ ...plan.resourceMetadata, ...plan.desiredConfig });
+    if (validationPlan.changeSet.operation !== ResourceOperation.NOOP) {
+      throw new ApplyValidationError(plan, validationPlan);
+    }
   }
 
   private resolvePlan(data: ApplyRequestData): Plan<ResourceConfig> {
diff --git a/src/entities/resource-parameters.test.ts b/src/entities/resource-parameters.test.ts
@@ -119,7 +119,7 @@ describe('Resource parameter tests', () => {
           type: 'resource',
           parameterOptions: {
             propA: { statefulParameter: statefulParameterSpy },
-            propB: { canModify: true },
+            propB: { modifyOnChange: true },
           }
         });
       }
diff --git a/src/entities/resource-types.ts b/src/entities/resource-types.ts
@@ -7,7 +7,7 @@ export interface ResourceParameterOptions {
   /**
    * Chose if the resource should be re-created or modified if this parameter is changed. Defaults to false (re-create).
    */
-  canModify?: boolean;
+  modifyOnChange?: boolean;
   /**
    * Customize the equality comparison for a parameter.
    * @param desired
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: { canModify: true },
-            propB: { canModify: true },
+            propA: { modifyOnChange: true },
+            propB: { modifyOnChange: true },
           }
         });
       }
@@ -218,12 +218,6 @@ describe('Resource tests', () => {
 
   it('Validates the resource options correct (pass)', () => {
     const statefulParameter = new class extends StatefulParameter<TestConfig, string> {
-      constructor() {
-        super({
-          name: 'propC',
-        });
-      }
-
       async refresh(): Promise<string | null> {
         return null;
       }
@@ -244,7 +238,7 @@ describe('Resource tests', () => {
           type: 'type',
           dependencies: ['homebrew', 'python'],
           parameterOptions: {
-            propA: { canModify: true },
+            propA: { modifyOnChange: true },
             propB: { statefulParameter },
             propC: { isEqual: (a, b) => true },
           }
@@ -255,12 +249,6 @@ describe('Resource tests', () => {
 
   it('Validates the resource options correct (fail)', () => {
     const statefulParameter = new class extends StatefulParameter<TestConfig, string> {
-      constructor() {
-        super({
-          name: 'propC',
-        });
-      }
-
       async refresh(): Promise<string | null> {
         return null;
       }
@@ -281,7 +269,7 @@ describe('Resource tests', () => {
           type: 'type',
           dependencies: ['homebrew', 'python'],
           parameterOptions: {
-            propA: { canModify: true },
+            propA: { modifyOnChange: true },
             propB: { statefulParameter },
             propC: { isEqual: (a, b) => true },
           }
diff --git a/src/messages/handlers.ts b/src/messages/handlers.ts
@@ -15,7 +15,7 @@ import {
   ValidateResponseDataSchema
 } from 'codify-schemas';
 import Ajv2020, { SchemaObject, ValidateFunction } from 'ajv/dist/2020.js';
-import { SudoError } from '../entities/errors.js';
+import { ApplyValidationError, SudoError } from '../entities/errors.js';
 
 const SupportedRequests: Record<string, { requestValidator: SchemaObject; responseValidator: SchemaObject; handler: (plugin: Plugin, data: any) => Promise<unknown> }> = {
   'initialize': {
@@ -117,13 +117,28 @@ export class MessageHandler {
     const cmd = message.cmd + '_Response';
 
     if (e instanceof SudoError) {
-      process.send?.({
+      return process.send?.({
         cmd,
         status: MessageStatus.ERROR,
         data: `Plugin: '${this.plugin.name}'. Forbidden usage of sudo for command '${e.command}'. Please contact the plugin developer to fix this.`,
       })
     }
 
+    if (e instanceof ApplyValidationError) {
+      return process.send?.({
+        cmd,
+        status: MessageStatus.ERROR,
+        data: `Plugin: '${this.plugin.name}'. Apply validation was not successful (additional changes are needed to match the desired plan).
+
+Validation plan:
+${JSON.stringify(e.validatedPlan, null, 2)},
+        
+User desired plan:
+${JSON.stringify(e.desiredPlan, null, 2)}
+`
+      })
+    }
+
     const isDebug = process.env.DEBUG?.includes('*') ?? false;
 
     process.send?.({

Original file line number	Diff line number	Diff line change
`@@ -119,7 +119,7 @@ describe('Resource parameter tests', () => {`
`119`	`119`	`type: 'resource',`
`120`	`120`	`parameterOptions: {`
`121`	`121`	`propA: { statefulParameter: statefulParameterSpy },`
`122`		`- propB: { canModify: true },`
	`122`	`+ propB: { modifyOnChange: true },`
`123`	`123`	`}`
`124`	`124`	`});`
`125`	`125`	`}`