From 12c4bcf8b72a2a067827b3958da4ca24595b9985 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Mon, 21 Aug 2023 11:36:02 -0700 Subject: [PATCH 1/5] Refactor validation logic with a deeper interface Signed-off-by: Simeon Widdis --- .../repository/__test__/integration.test.ts | 11 ++++-- .../integrations/repository/integration.ts | 21 +--------- server/adaptors/integrations/validators.ts | 39 ++++++++++++++++++- 3 files changed, 47 insertions(+), 24 deletions(-) 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..1350cb6537 100644 --- a/server/adaptors/integrations/repository/integration.ts +++ b/server/adaptors/integrations/repository/integration.ts @@ -6,7 +6,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 +49,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. @@ -165,12 +153,7 @@ export class Integration { const config = await fs.readFile(configPath, { encoding: 'utf-8' }); const possibleTemplate = JSON.parse(config); - if (!templateValidator(possibleTemplate)) { - logValidationErrors(configFile, templateValidator); - return null; - } - - return possibleTemplate; + return validateTemplate(possibleTemplate, true) ? possibleTemplate : 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..ab51c17cb9 100644 --- a/server/adaptors/integrations/validators.ts +++ b/server/adaptors/integrations/validators.ts @@ -107,5 +107,40 @@ 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); + +// AJV validators use side effects for errors, so we provide a more conventional wrapper. +// The wrapper optionally handles error logging with the `logErrors` parameter. +export const validateTemplate = (data: { name?: unknown }, logErrors?: true): boolean => { + if (!templateValidator(data)) { + if (logErrors) { + console.error( + `The integration '${data.name ?? 'config'}' is invalid:`, + ajv.errorsText(templateValidator.errors) + ); + } + return false; + } + // 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) { + if (logErrors) { + console.error(`The integration type '${data.type}' must be included as a component`); + } + return false; + } + return true; +}; + +export const validateInstance = (data: { name?: unknown }, logErrors?: true): boolean => { + if (!instanceValidator(data)) { + if (logErrors) { + console.error( + `The integration '${data.name ?? 'instance'} is invalid:`, + ajv.errorsText(instanceValidator.errors) + ); + } + return false; + } + return true; +}; From 412480e74ab4cfbcf2cdf42a689f46d0c8eb3487 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Mon, 21 Aug 2023 12:02:27 -0700 Subject: [PATCH 2/5] Remove redundant test. This test is unneeded after 12c4bcf Signed-off-by: Simeon Widdis --- .../integrations/__test__/local_repository.test.ts | 10 ---------- 1 file changed, 10 deletions(-) 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); - } - }); }); From 6d2bad103cb35ec5934e3e614b022040b61a6931 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Mon, 21 Aug 2023 14:12:44 -0700 Subject: [PATCH 3/5] Add tests for new validators Signed-off-by: Simeon Widdis --- .../integrations/__test__/validators.test.ts | 79 +++++++++++++++++++ .../integrations/repository/integration.ts | 1 - 2 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 server/adaptors/integrations/__test__/validators.test.ts diff --git a/server/adaptors/integrations/__test__/validators.test.ts b/server/adaptors/integrations/__test__/validators.test.ts new file mode 100644 index 0000000000..f998324af0 --- /dev/null +++ b/server/adaptors/integrations/__test__/validators.test.ts @@ -0,0 +1,79 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import { validateTemplate, validateInstance } 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 true for a valid Integration Template', () => { + expect(validateTemplate(validTemplate)).toBe(true); + }); + + it('Returns false if a template is missing a license', () => { + const sample: any = structuredClone(validTemplate); + sample.license = undefined; + expect(validateTemplate(sample)).toBe(false); + }); + + it('Returns false if a template has an invalid type', () => { + const sample: any = structuredClone(validTemplate); + sample.components[0].name = 'not-logs'; + expect(validateTemplate(sample)).toBe(false); + }); + + it('Respects logErrors', () => { + const logValidationErrorsMock = jest.spyOn(console, 'error'); + const sample1: any = structuredClone(validTemplate); + sample1.license = undefined; + const sample2: any = structuredClone(validTemplate); + sample2.components[0].name = 'not-logs'; + + expect(validateTemplate(sample1, true)).toBe(false); + expect(validateTemplate(sample2, true)).toBe(false); + expect(logValidationErrorsMock).toBeCalledTimes(2); + }); +}); + +describe('validateInstance', () => { + it('Returns true for a valid Integration Instance', () => { + expect(validateInstance(validInstance)).toBe(true); + }); + + it('Returns false if an instance is missing a template', () => { + const sample: any = structuredClone(validInstance); + sample.templateName = undefined; + expect(validateInstance(sample)).toBe(false); + }); + + it('Respects logErrors', () => { + const logValidationErrorsMock = jest.spyOn(console, 'error'); + const sample1: any = structuredClone(validInstance); + sample1.templateName = undefined; + + expect(validateInstance(sample1, true)).toBe(false); + expect(logValidationErrorsMock).toBeCalled(); + }); +}); diff --git a/server/adaptors/integrations/repository/integration.ts b/server/adaptors/integrations/repository/integration.ts index 1350cb6537..7c20db6e49 100644 --- a/server/adaptors/integrations/repository/integration.ts +++ b/server/adaptors/integrations/repository/integration.ts @@ -5,7 +5,6 @@ import * as fs from 'fs/promises'; import path from 'path'; -import { ValidateFunction } from 'ajv'; import { validateTemplate } from '../validators'; /** From 9749d25d1d9c21755528830f8523ea36e3b25d24 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Mon, 21 Aug 2023 14:22:35 -0700 Subject: [PATCH 4/5] Make better failure mode for invalid objects Signed-off-by: Simeon Widdis --- .../adaptors/integrations/__test__/validators.test.ts | 10 ++++++++++ server/adaptors/integrations/validators.ts | 4 ++-- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/server/adaptors/integrations/__test__/validators.test.ts b/server/adaptors/integrations/__test__/validators.test.ts index f998324af0..b52128d540 100644 --- a/server/adaptors/integrations/__test__/validators.test.ts +++ b/server/adaptors/integrations/__test__/validators.test.ts @@ -55,6 +55,11 @@ describe('validateTemplate', () => { expect(validateTemplate(sample2, true)).toBe(false); expect(logValidationErrorsMock).toBeCalledTimes(2); }); + + it("Doesn't crash if given a non-object", () => { + // May happen in some user-provided JSON parsing scenarios. + expect(validateTemplate([] as any, true)).toBe(false); + }); }); describe('validateInstance', () => { @@ -76,4 +81,9 @@ describe('validateInstance', () => { expect(validateInstance(sample1, true)).toBe(false); expect(logValidationErrorsMock).toBeCalled(); }); + + it("Doesn't crash if given a non-object", () => { + // May happen in some user-provided JSON parsing scenarios. + expect(validateInstance([] as any, true)).toBe(false); + }); }); diff --git a/server/adaptors/integrations/validators.ts b/server/adaptors/integrations/validators.ts index ab51c17cb9..7621cf7176 100644 --- a/server/adaptors/integrations/validators.ts +++ b/server/adaptors/integrations/validators.ts @@ -116,7 +116,7 @@ export const validateTemplate = (data: { name?: unknown }, logErrors?: true): bo if (!templateValidator(data)) { if (logErrors) { console.error( - `The integration '${data.name ?? 'config'}' is invalid:`, + `The integration config '${data.name ?? data}' is invalid:`, ajv.errorsText(templateValidator.errors) ); } @@ -136,7 +136,7 @@ export const validateInstance = (data: { name?: unknown }, logErrors?: true): bo if (!instanceValidator(data)) { if (logErrors) { console.error( - `The integration '${data.name ?? 'instance'} is invalid:`, + `The integration instance '${data.name ?? data}' is invalid:`, ajv.errorsText(instanceValidator.errors) ); } From 85041c0235fa84f80bb6ee219cab9c37f6caae35 Mon Sep 17 00:00:00 2001 From: Simeon Widdis Date: Tue, 22 Aug 2023 11:44:49 -0700 Subject: [PATCH 5/5] Convert validator methods to use result types Signed-off-by: Simeon Widdis --- .../integrations/__test__/validators.test.ts | 51 +++++++-------- .../integrations/repository/integration.ts | 8 ++- server/adaptors/integrations/validators.ts | 63 ++++++++++++------- 3 files changed, 68 insertions(+), 54 deletions(-) diff --git a/server/adaptors/integrations/__test__/validators.test.ts b/server/adaptors/integrations/__test__/validators.test.ts index b52128d540..ba573c4c47 100644 --- a/server/adaptors/integrations/__test__/validators.test.ts +++ b/server/adaptors/integrations/__test__/validators.test.ts @@ -3,7 +3,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { validateTemplate, validateInstance } from '../validators'; +import { validateTemplate, validateInstance, ValidationResult } from '../validators'; const validTemplate: IntegrationTemplate = { name: 'test', @@ -28,62 +28,57 @@ const validInstance: IntegrationInstance = { }; describe('validateTemplate', () => { - it('Returns true for a valid Integration Template', () => { - expect(validateTemplate(validTemplate)).toBe(true); + 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 false if a template is missing a license', () => { + it('Returns a failure value if a template is missing a license', () => { const sample: any = structuredClone(validTemplate); sample.license = undefined; - expect(validateTemplate(sample)).toBe(false); + + const result: ValidationResult = validateTemplate(sample); + + expect(result.ok).toBe(false); + expect((result as any).error).toBeInstanceOf(Error); }); - it('Returns false if a template has an invalid type', () => { + it('Returns a failure if a template has an invalid type', () => { const sample: any = structuredClone(validTemplate); sample.components[0].name = 'not-logs'; - expect(validateTemplate(sample)).toBe(false); - }); - it('Respects logErrors', () => { - const logValidationErrorsMock = jest.spyOn(console, 'error'); - const sample1: any = structuredClone(validTemplate); - sample1.license = undefined; - const sample2: any = structuredClone(validTemplate); - sample2.components[0].name = 'not-logs'; + const result: ValidationResult = validateTemplate(sample); - expect(validateTemplate(sample1, true)).toBe(false); - expect(validateTemplate(sample2, true)).toBe(false); - expect(logValidationErrorsMock).toBeCalledTimes(2); + 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, true)).toBe(false); + expect(validateTemplate([] as any).ok).toBe(false); }); }); describe('validateInstance', () => { it('Returns true for a valid Integration Instance', () => { - expect(validateInstance(validInstance)).toBe(true); + 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; - expect(validateInstance(sample)).toBe(false); - }); - it('Respects logErrors', () => { - const logValidationErrorsMock = jest.spyOn(console, 'error'); - const sample1: any = structuredClone(validInstance); - sample1.templateName = undefined; + const result: ValidationResult = validateInstance(sample); - expect(validateInstance(sample1, true)).toBe(false); - expect(logValidationErrorsMock).toBeCalled(); + 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, true)).toBe(false); + expect(validateInstance([] as any).ok).toBe(false); }); }); diff --git a/server/adaptors/integrations/repository/integration.ts b/server/adaptors/integrations/repository/integration.ts index 7c20db6e49..21f187bdec 100644 --- a/server/adaptors/integrations/repository/integration.ts +++ b/server/adaptors/integrations/repository/integration.ts @@ -151,8 +151,12 @@ export class Integration { try { const config = await fs.readFile(configPath, { encoding: 'utf-8' }); const possibleTemplate = JSON.parse(config); - - return validateTemplate(possibleTemplate, true) ? possibleTemplate : null; + const template = validateTemplate(possibleTemplate); + if (template.ok) { + return template.value; + } + 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 7621cf7176..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 = { @@ -110,37 +112,50 @@ const instanceSchema: JSONSchemaType = { const templateValidator = ajv.compile(templateSchema); const instanceValidator = ajv.compile(instanceSchema); -// AJV validators use side effects for errors, so we provide a more conventional wrapper. -// The wrapper optionally handles error logging with the `logErrors` parameter. -export const validateTemplate = (data: { name?: unknown }, logErrors?: true): boolean => { +/** + * 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)) { - if (logErrors) { - console.error( - `The integration config '${data.name ?? data}' is invalid:`, - ajv.errorsText(templateValidator.errors) - ); - } - return false; + 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) { - if (logErrors) { - console.error(`The integration type '${data.type}' must be included as a component`); - } - return false; + return { + ok: false, + error: new Error(`The integration type '${data.type}' must be included as a component`), + }; } - return true; + return { + ok: true, + value: data, + }; }; -export const validateInstance = (data: { name?: unknown }, logErrors?: true): boolean => { +/** + * 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)) { - if (logErrors) { - console.error( - `The integration instance '${data.name ?? data}' is invalid:`, - ajv.errorsText(instanceValidator.errors) - ); - } - return false; + return { + ok: false, + error: new Error(ajv.errorsText(instanceValidator.errors)), + }; } - return true; + return { + ok: true, + value: data, + }; };