Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor integration validation logic with a cleaner interface #943

Merged
merged 5 commits into from
Aug 22, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 0 additions & 10 deletions server/adaptors/integrations/__test__/local_repository.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
});
});
84 changes: 84 additions & 0 deletions server/adaptors/integrations/__test__/validators.test.ts
Original file line number Diff line number Diff line change
@@ -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);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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 () => {
Expand Down
26 changes: 6 additions & 20 deletions server/adaptors/integrations/repository/integration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down
54 changes: 52 additions & 2 deletions server/adaptors/integrations/validators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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> = {
Expand Down Expand Up @@ -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,
};
};
Loading