Merge pull request #20750 from mbenson/each-schema-validation

Each schema validation
2023-11-08 15:01:45 -06:00
parent d811f85923 cc6765ecc7
commit b2df411b7a
5 changed files with 164 additions and 87 deletions
@@ -0,0 +1,5 @@
+---
+'@backstage/plugin-scaffolder-backend': patch
+---
+
+Refactoring the runner to generate minimally informative task log per iteration and properly validate iterated actions.
@@ -113,6 +113,7 @@
    "esbuild": "^0.19.0",
    "jest-when": "^3.1.0",
    "msw": "^1.0.0",
+    "strip-ansi": "^7.1.0",
    "supertest": "^6.1.3",
    "wait-for-expect": "^3.0.2",
    "yaml": "^2.0.0"
@@ -34,12 +34,14 @@ import {
 } from '@backstage/plugin-permission-common';
 import { RESOURCE_TYPE_SCAFFOLDER_ACTION } from '@backstage/plugin-scaffolder-common/alpha';
 import { createMockDirectory } from '@backstage/backend-test-utils';
+import stripAnsi from 'strip-ansi';

-describe('DefaultWorkflowRunner', () => {
+describe('NunjucksWorkflowRunner', () => {
  const logger = getVoidLogger();
  let actionRegistry = new TemplateActionRegistry();
  let runner: NunjucksWorkflowRunner;
  let fakeActionHandler: jest.Mock;
+  let fakeTaskLog: jest.Mock;

  const mockDir = createMockDirectory();

@@ -65,17 +67,24 @@ describe('DefaultWorkflowRunner', () => {
    isDryRun,
    complete: async () => {},
    done: false,
-    emitLog: async () => {},
+    emitLog: fakeTaskLog,
    cancelSignal: new AbortController().signal,
    getWorkspaceName: () => Promise.resolve('test-workspace'),
  });

+  function expectTaskLog(message: string) {
+    expect(fakeTaskLog.mock.calls.map(args => stripAnsi(args[0]))).toContain(
+      message,
+    );
+  }
+
  beforeEach(() => {
    mockDir.clear();

    jest.resetAllMocks();
    actionRegistry = new TemplateActionRegistry();
    fakeActionHandler = jest.fn();
+    fakeTaskLog = jest.fn();

    actionRegistry.register({
      id: 'jest-mock-action',
@@ -554,6 +563,7 @@ describe('DefaultWorkflowRunner', () => {

  describe('each', () => {
    it('should run a step repeatedly - flat values', async () => {
+      const colors = ['blue', 'green', 'red'];
      const task = createMockTaskWithSpec({
        apiVersion: 'scaffolder.backstage.io/v1beta3',
        steps: [
@@ -567,21 +577,19 @@ describe('DefaultWorkflowRunner', () => {
        ],
        output: {},
        parameters: {
-          colors: ['blue', 'green', 'red'],
+          colors,
        },
      });
-
      await runner.execute(task);

-      expect(fakeActionHandler).toHaveBeenCalledWith(
-        expect.objectContaining({ input: { color: 'blue' } }),
-      );
-      expect(fakeActionHandler).toHaveBeenCalledWith(
-        expect.objectContaining({ input: { color: 'green' } }),
-      );
-      expect(fakeActionHandler).toHaveBeenCalledWith(
-        expect.objectContaining({ input: { color: 'red' } }),
-      );
+      colors.forEach((color, idx) => {
+        expectTaskLog(
+          `info: Running step each: {"key":"${idx}","value":"${color}"}`,
+        );
+        expect(fakeActionHandler).toHaveBeenCalledWith(
+          expect.objectContaining({ input: { color } }),
+        );
+      });
    });

    it('should run a step repeatedly - object list', async () => {
@@ -604,9 +612,11 @@ describe('DefaultWorkflowRunner', () => {
          settings: [{ color: 'blue' }],
        },
      });
-
      await runner.execute(task);

+      expectTaskLog(
+        'info: Running step each: {"key":"0","value":"[object Object]"}',
+      );
      expect(fakeActionHandler).toHaveBeenCalledWith(
        expect.objectContaining({
          input: { key: '0', value: { color: 'blue' } },
@@ -615,6 +625,10 @@ describe('DefaultWorkflowRunner', () => {
    });

    it('should run a step repeatedly - object', async () => {
+      const settings = {
+        color: 'blue',
+        transparent: 'yes',
+      };
      const task = createMockTaskWithSpec({
        apiVersion: 'scaffolder.backstage.io/v1beta3',
        steps: [
@@ -628,22 +642,81 @@ describe('DefaultWorkflowRunner', () => {
        ],
        output: {},
        parameters: {
-          settings: { color: 'blue', transparent: 'yes' },
+          settings,
        },
      });
-
      await runner.execute(task);

-      expect(fakeActionHandler).toHaveBeenCalledWith(
-        expect.objectContaining({
-          input: { key: 'color', value: 'blue' },
-        }),
-      );
-      expect(fakeActionHandler).toHaveBeenCalledWith(
-        expect.objectContaining({
-          input: { key: 'transparent', value: 'yes' },
-        }),
+      for (const [key, value] of Object.entries(settings)) {
+        expectTaskLog(
+          `info: Running step each: {"key":"${key}","value":"${value}"}`,
+        );
+        expect(fakeActionHandler).toHaveBeenCalledWith(
+          expect.objectContaining({
+            input: { key, value },
+          }),
+        );
+      }
+    });
+
+    it('should run a step repeatedly with validation of single-expression value', async () => {
+      const numbers = [5, 7, 9];
+      const task = createMockTaskWithSpec({
+        apiVersion: 'scaffolder.backstage.io/v1beta3',
+        steps: [
+          {
+            id: 'test',
+            name: 'name',
+            each: '${{parameters.numbers}}',
+            action: 'jest-validated-action',
+            input: { foo: '${{each.value}}' },
+          },
+        ],
+        output: {},
+        parameters: {
+          numbers,
+        },
+      });
+      await runner.execute(task);
+
+      numbers.forEach((foo, idx) => {
+        expectTaskLog(
+          `info: Running step each: {"key":"${idx}","value":"${foo}"}`,
+        );
+        expect(fakeActionHandler).toHaveBeenCalledWith(
+          expect.objectContaining({
+            input: { foo },
+          }),
+        );
+      });
+    });
+
+    it('should validate each action iteration', async () => {
+      const task = createMockTaskWithSpec({
+        apiVersion: 'scaffolder.backstage.io/v1beta3',
+        steps: [
+          {
+            id: 'test',
+            name: 'name',
+            each: '${{parameters.data}}',
+            action: 'jest-validated-action',
+            input: { foo: '${{each.value.foo}}' },
+          },
+        ],
+        output: {},
+        parameters: {
+          data: [
+            {
+              foo: 0,
+            },
+            {},
+          ],
+        },
+      });
+      await expect(runner.execute(task)).rejects.toThrow(
+        'Invalid input passed to action jest-validated-action[1], instance requires property "foo"',
      );
+      expect(fakeActionHandler).not.toHaveBeenCalled();
    });
  });

@@ -276,74 +276,71 @@ export class NunjucksWorkflowRunner implements WorkflowRunner {
          return;
        }
      }
+      const iterations = (
+        step.each
+          ? Object.entries(this.render(step.each, context, renderTemplate)).map(
+              ([key, value]) => ({
+                each: { key, value },
+              }),
+            )
+          : [{}]
+      ).map(i => ({
+        ...i,
+        // Secrets are only passed when templating the input to actions for security reasons
+        input: step.input
+          ? this.render(
+              step.input,
+              { ...context, ...i, ...task },
+              renderTemplate,
+            )
+          : {},
+      }));
+      for (const iteration of iterations) {
+        const actionId =
+          action.id + (iteration.each ? `[${iteration.each.key}]` : '');

-      // Secrets are only passed when templating the input to actions for security reasons
-      const input =
-        (step.input &&
-          this.render(
-            step.input,
-            { ...context, secrets: task.secrets ?? {} },
-            renderTemplate,
-          )) ??
-        {};
-
-      if (action.schema?.input) {
-        const validateResult = validateJsonSchema(input, action.schema.input);
-        if (!validateResult.valid) {
-          const errors = validateResult.errors.join(', ');
-          throw new InputError(
-            `Invalid input passed to action ${action.id}, ${errors}`,
+        if (action.schema?.input) {
+          const validateResult = validateJsonSchema(
+            iteration.input,
+            action.schema.input,
+          );
+          if (!validateResult.valid) {
+            const errors = validateResult.errors.join(', ');
+            throw new InputError(
+              `Invalid input passed to action ${actionId}, ${errors}`,
+            );
+          }
+        }
+        if (
+          !isActionAuthorized(decision, {
+            action: action.id,
+            input: iteration.input,
+          })
+        ) {
+          throw new NotAllowedError(
+            `Unauthorized action: ${actionId}. The action is not allowed. Input: ${JSON.stringify(
+              iteration.input,
+              null,
+              2,
+            )}`,
          );
        }
      }
-
-      if (!isActionAuthorized(decision, { action: action.id, input })) {
-        throw new NotAllowedError(
-          `Unauthorized action: ${
-            action.id
-          }. The action is not allowed. Input: ${JSON.stringify(
-            input,
-            null,
-            2,
-          )}`,
-        );
-      }
-
      const tmpDirs = new Array<string>();
      const stepOutput: { [outputName: string]: JsonValue } = {};

-      const iterations = new Array<JsonValue>();
-      if (step.each) {
-        const each = await this.render(step.each, context, renderTemplate);
-        iterations.push(
-          ...Object.keys(each).map((key: any) => {
-            return { key: key, value: each[key] };
-          }),
-        );
-      } else {
-        iterations.push({});
-      }
-
-      let actionInput = input;
      for (const iteration of iterations) {
-        if (step.each) {
-          taskLogger.info(`Running step each: ${iteration}`);
-          const iterationContext = {
-            ...context,
-            each: iteration,
-          };
-          // re-render input with the modified context that includes each
-          actionInput =
-            (step.input &&
-              this.render(
-                step.input,
-                { ...iterationContext, secrets: task.secrets ?? {} },
-                renderTemplate,
-              )) ??
-            {};
+        if (iteration.each) {
+          taskLogger.info(
+            `Running step each: ${JSON.stringify(
+              iteration.each,
+              (k, v) => (k ? v.toString() : v),
+              0,
+            )}`,
+          );
        }
        await action.handler({
-          input: actionInput,
+          input: iteration.input,
          secrets: task.secrets ?? {},
          logger: taskLogger,
          logStream: streamLogger,
@@ -8900,6 +8900,7 @@ __metadata:
    p-limit: ^3.1.0
    p-queue: ^6.6.2
    prom-client: ^14.0.1
+    strip-ansi: ^7.1.0
    supertest: ^6.1.3
    uuid: ^8.2.0
    wait-for-expect: ^3.0.2
@@ -42809,12 +42810,12 @@ __metadata:
  languageName: node
  linkType: hard

-"strip-ansi@npm:^7.0.1":
-  version: 7.0.1
-  resolution: "strip-ansi@npm:7.0.1"
+"strip-ansi@npm:^7.0.1, strip-ansi@npm:^7.1.0":
+  version: 7.1.0
+  resolution: "strip-ansi@npm:7.1.0"
  dependencies:
    ansi-regex: ^6.0.1
-  checksum: 257f78fa433520e7f9897722731d78599cb3fce29ff26a20a5e12ba4957463b50a01136f37c43707f4951817a75e90820174853d6ccc240997adc5df8f966039
+  checksum: 859c73fcf27869c22a4e4d8c6acfe690064659e84bef9458aa6d13719d09ca88dcfd40cbf31fd0be63518ea1a643fe070b4827d353e09533a5b0b9fd4553d64d
  languageName: node
  linkType: hard