From b317352c44270b1a2e09612c70a8322793d0fe34 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Tue, 22 Aug 2023 13:46:53 -0700 Subject: [PATCH] Refactor integration validation logic with a cleaner interface (#943) * Refactor validation logic with a deeper interface Signed-off-by: Simeon Widdis * Remove redundant test. This test is unneeded after 12c4bcf Signed-off-by: Simeon Widdis * Add tests for new validators Signed-off-by: Simeon Widdis * Make better failure mode for invalid objects Signed-off-by: Simeon Widdis * Convert validator methods to use result types Signed-off-by: Simeon Widdis --------- Signed-off-by: Simeon Widdis --- .../__test__/local_repository.test.ts | 10 --- .../integrations/__test__/validators.test.ts | 84 +++++++++++++++++++ .../repository/__test__/integration.test.ts | 11 ++- .../integrations/repository/integration.ts | 26 ++---- server/adaptors/integrations/validators.ts | 54 +++++++++++- 5 files changed, 150 insertions(+), 35 deletions(-) create mode 100644 server/adaptors/integrations/__test__/validators.test.ts diff --git a/server/adaptors/integrations/__test__/local_repository.test.ts b/server/adaptors/integrations/__test__/local_repository.test.ts index 722711710b..6b0ddc72fd 100644 --- a/server/adaptors/integrations/__test__/local_repository.test.ts +++ b/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); - } - }); }); diff --git a/server/adaptors/integrations/__test__/validators.test.ts b/server/adaptors/integrations/__test__/validators.test.ts new file mode 100644 index 0000000000..ba573c4c47 --- /dev/null +++ b/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 = 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 = 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 = 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 = 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 = 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); + }); +}); diff --git a/server/adaptors/integrations/repository/__test__/integration.test.ts b/server/adaptors/integrations/repository/__test__/integration.test.ts index 4474fc48ff..2002ad04a9 100644 --- a/server/adaptors/integrations/repository/__test__/integration.test.ts +++ b/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 () => { diff --git a/server/adaptors/integrations/repository/integration.ts b/server/adaptors/integrations/repository/integration.ts index fe9ddd0ef3..21f187bdec 100644 --- a/server/adaptors/integrations/repository/integration.ts +++ b/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 { } } -/** - * 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) { - 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); diff --git a/server/adaptors/integrations/validators.ts b/server/adaptors/integrations/validators.ts index 3cb24212d2..b40871eefb 100644 --- a/server/adaptors/integrations/validators.ts +++ b/server/adaptors/integrations/validators.ts @@ -5,6 +5,8 @@ import Ajv, { JSONSchemaType } from 'ajv'; +export type ValidationResult = { ok: true; value: T } | { ok: false; error: E }; + const ajv = new Ajv(); const staticAsset: JSONSchemaType = { @@ -107,5 +109,53 @@ const instanceSchema: JSONSchemaType = { 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 => { + 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 => { + if (!instanceValidator(data)) { + return { + ok: false, + error: new Error(ajv.errorsText(instanceValidator.errors)), + }; + } + return { + ok: true, + value: data, + }; +};