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 2 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);
}
});
});
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
21 changes: 2 additions & 19 deletions server/adaptors/integrations/repository/integration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -49,18 +49,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 @@ -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);
Expand Down
39 changes: 37 additions & 2 deletions server/adaptors/integrations/validators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -107,5 +107,40 @@ 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);

// 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 => {
Swiddis marked this conversation as resolved.
Show resolved Hide resolved
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;
};
Loading