Refactor integration validation logic with a cleaner interface (opens…

…earch-project#943)

* Refactor validation logic with a deeper interface

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Remove redundant test.

This test is unneeded after 12c4bcf

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Add tests for new validators

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Make better failure mode for invalid objects

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

* Convert validator methods to use result types

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

---------

Signed-off-by: Simeon Widdis <sawiddis@amazon.com>

server/adaptors/integrations/__test__/local_repository.test.ts

@@ -31,14 +31,4 @@ describe('The local repository', () => {
     const integrations: Integration[] = await repository.getIntegrationList();
     await Promise.all(integrations.map((i) => expect(i.deepCheck()).resolves.toBeTruthy()));
   });
-
-  it('Should not have a type that is not imported in the config', async () => {
-    const repository: Repository = new Repository(path.join(__dirname, '../__data__/repository'));
-    const integrations: Integration[] = await repository.getIntegrationList();
-    for (const integration of integrations) {
-      const config = await integration.getConfig();
-      const components = config!.components.map((x) => x.name);
-      expect(components).toContain(config!.type);
-    }
-  });
 });

server/adaptors/integrations/__test__/validators.test.ts

@@ -0,0 +1,84 @@
+/*
+ * Copyright OpenSearch Contributors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import { validateTemplate, validateInstance, ValidationResult } from '../validators';
+
+const validTemplate: IntegrationTemplate = {
+  name: 'test',
+  version: '1.0.0',
+  license: 'Apache-2.0',
+  type: 'logs',
+  components: [
+    {
+      name: 'logs',
+      version: '1.0.0',
+    },
+  ],
+  assets: {},
+};
+
+const validInstance: IntegrationInstance = {
+  name: 'test',
+  templateName: 'test',
+  dataSource: 'test',
+  creationDate: new Date(0).toISOString(),
+  assets: [],
+};
+
+describe('validateTemplate', () => {
+  it('Returns a success value for a valid Integration Template', () => {
+    const result: ValidationResult<IntegrationTemplate> = validateTemplate(validTemplate);
+    expect(result.ok).toBe(true);
+    expect((result as any).value).toBe(validTemplate);
+  });
+
+  it('Returns a failure value if a template is missing a license', () => {
+    const sample: any = structuredClone(validTemplate);
+    sample.license = undefined;
+
+    const result: ValidationResult<IntegrationTemplate> = validateTemplate(sample);
+
+    expect(result.ok).toBe(false);
+    expect((result as any).error).toBeInstanceOf(Error);
+  });
+
+  it('Returns a failure if a template has an invalid type', () => {
+    const sample: any = structuredClone(validTemplate);
+    sample.components[0].name = 'not-logs';
+
+    const result: ValidationResult<IntegrationTemplate> = validateTemplate(sample);
+
+    expect(result.ok).toBe(false);
+    expect((result as any).error).toBeInstanceOf(Error);
+  });
+
+  it("Doesn't crash if given a non-object", () => {
+    // May happen in some user-provided JSON parsing scenarios.
+    expect(validateTemplate([] as any).ok).toBe(false);
+  });
+});
+
+describe('validateInstance', () => {
+  it('Returns true for a valid Integration Instance', () => {
+    const result: ValidationResult<IntegrationInstance> = validateInstance(validInstance);
+    expect(result.ok).toBe(true);
+    expect((result as any).value).toBe(validInstance);
+  });
+
+  it('Returns false if an instance is missing a template', () => {
+    const sample: any = structuredClone(validInstance);
+    sample.templateName = undefined;
+
+    const result: ValidationResult<IntegrationInstance> = validateInstance(sample);
+
+    expect(result.ok).toBe(false);
+    expect((result as any).error).toBeInstanceOf(Error);
+  });
+
+  it("Doesn't crash if given a non-object", () => {
+    // May happen in some user-provided JSON parsing scenarios.
+    expect(validateInstance([] as any).ok).toBe(false);
+  });
+});

server/adaptors/integrations/repository/__test__/integration.test.ts

@@ -16,8 +16,13 @@ describe('Integration', () => {
     name: 'sample',
     version: '2.0.0',
     license: 'Apache-2.0',
-    type: '',
-    components: [],
+    type: 'logs',
+    components: [
+      {
+        name: 'logs',
+        version: '1.0.0',
+      },
+    ],
     assets: {
       savedObjects: {
         name: 'sample',
@@ -105,7 +110,7 @@ describe('Integration', () => {
       const result = await integration.getConfig(sampleIntegration.version);
 
       expect(result).toBeNull();
-      expect(logValidationErrorsMock).toHaveBeenCalledWith(expect.any(String), expect.any(Array));
+      expect(logValidationErrorsMock).toHaveBeenCalled();
     });
 
     it('should return null and log syntax errors if the config file has syntax errors', async () => {

server/adaptors/integrations/repository/integration.ts

@@ -5,8 +5,7 @@
 
 import * as fs from 'fs/promises';
 import path from 'path';
-import { ValidateFunction } from 'ajv';
-import { templateValidator } from '../validators';
+import { validateTemplate } from '../validators';
 
 /**
  * Helper function to compare version numbers.
@@ -49,18 +48,6 @@ async function isDirectory(dirPath: string): Promise<boolean> {
   }
 }
 
-/**
- * Helper function to log validation errors.
- * Relies on the `ajv` package for validation error logs..
- *
- * @param integration The name of the component that failed validation.
- * @param validator A failing ajv validator.
- */
-function logValidationErrors(integration: string, validator: ValidateFunction<any>) {
-  const errors = validator.errors?.map((e) => e.message);
-  console.error(`Validation errors in ${integration}`, errors);
-}
-
 /**
  * The Integration class represents the data for Integration Templates.
  * It is backed by the repository file system.
@@ -164,13 +151,12 @@ export class Integration {
     try {
       const config = await fs.readFile(configPath, { encoding: 'utf-8' });
       const possibleTemplate = JSON.parse(config);
-
-      if (!templateValidator(possibleTemplate)) {
-        logValidationErrors(configFile, templateValidator);
-        return null;
+      const template = validateTemplate(possibleTemplate);
+      if (template.ok) {
+        return template.value;
       }
-
-      return possibleTemplate;
+      console.error(template.error);
+      return null;
     } catch (err: any) {
       if (err instanceof SyntaxError) {
         console.error(`Syntax errors in ${configFile}`, err);

server/adaptors/integrations/validators.ts

@@ -5,6 +5,8 @@
 
 import Ajv, { JSONSchemaType } from 'ajv';
 
+export type ValidationResult<T, E = Error> = { ok: true; value: T } | { ok: false; error: E };
+
 const ajv = new Ajv();
 
 const staticAsset: JSONSchemaType<StaticAsset> = {
@@ -107,5 +109,53 @@ const instanceSchema: JSONSchemaType<IntegrationInstance> = {
   required: ['name', 'templateName', 'dataSource', 'creationDate', 'assets'],
 };
 
-export const templateValidator = ajv.compile(templateSchema);
-export const instanceValidator = ajv.compile(instanceSchema);
+const templateValidator = ajv.compile(templateSchema);
+const instanceValidator = ajv.compile(instanceSchema);
+
+/**
+ * Validates an integration template against a predefined schema using the AJV library.
+ * Since AJV validators use side effects for errors,
+ * this is a more conventional wrapper that simplifies calling.
+ *
+ * @param data The data to be validated as an IntegrationTemplate.
+ * @return A ValidationResult indicating whether the validation was successful or not.
+ *         If validation succeeds, returns an object with 'ok' set to true and the validated data.
+ *         If validation fails, returns an object with 'ok' set to false and an Error object describing the validation error.
+ */
+export const validateTemplate = (data: unknown): ValidationResult<IntegrationTemplate> => {
+  if (!templateValidator(data)) {
+    return { ok: false, error: new Error(ajv.errorsText(templateValidator.errors)) };
+  }
+  // We assume an invariant that the type of an integration is connected with its component.
+  if (data.components.findIndex((x) => x.name === data.type) < 0) {
+    return {
+      ok: false,
+      error: new Error(`The integration type '${data.type}' must be included as a component`),
+    };
+  }
+  return {
+    ok: true,
+    value: data,
+  };
+};
+
+/**
+ * Validates an integration instance against a predefined schema using the AJV library.
+ *
+ * @param data The data to be validated as an IntegrationInstance.
+ * @return A ValidationResult indicating whether the validation was successful or not.
+ *         If validation succeeds, returns an object with 'ok' set to true and the validated data.
+ *         If validation fails, returns an object with 'ok' set to false and an Error object describing the validation error.
+ */
+export const validateInstance = (data: unknown): ValidationResult<IntegrationInstance> => {
+  if (!instanceValidator(data)) {
+    return {
+      ok: false,
+      error: new Error(ajv.errorsText(instanceValidator.errors)),
+    };
+  }
+  return {
+    ok: true,
+    value: data,
+  };
+};