Skip to content

Commit

Permalink
N21-1488 Fix optional external tool parameter with regex (#4558)
Browse files Browse the repository at this point in the history
  • Loading branch information
@MarvinOehlerkingCap
MarvinOehlerkingCap authored Nov 15, 2023
1 parent b3b4cee commit 8f1aea3
Show file tree
Hide file tree
Showing 10 changed files with 212 additions and 163 deletions.
178 changes: 91 additions & 87 deletions apps/server/src/modules/tool/common/service/common-tool-validation.service.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,104 +109,93 @@ describe('CommonToolValidationService', () => {
});
});

describe('checkForDuplicateParameters', () => {
describe('when given parameters has a case sensitive duplicate', () => {
const setup = () => {
const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build({
parameters: [
{ name: 'nameDuplicate', value: 'value' },
{ name: 'nameDuplicate', value: 'value' },
],
});
describe('checkCustomParameterEntries', () => {
const createTools = (
externalToolMock?: Partial<ExternalTool>,
schoolExternalToolMock?: Partial<SchoolExternalTool>,
contextExternalToolMock?: Partial<ContextExternalTool>
) => {
const externalTool: ExternalTool = new ExternalTool({
...externalToolFactory.buildWithId(),
...externalToolMock,
});
const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build({
...schoolExternalToolFactory.buildWithId(),
...schoolExternalToolMock,
});
const schoolExternalToolId = schoolExternalTool.id as string;
const contextExternalTool: ContextExternalTool = contextExternalToolFactory.build({
...contextExternalToolFactory.buildWithId(),
...contextExternalToolMock,
});

return {
schoolExternalTool,
};
return {
externalTool,
schoolExternalTool,
schoolExternalToolId,
contextExternalTool,
};
};

it('should throw error', () => {
const { schoolExternalTool } = setup();

const func = () => service.checkForDuplicateParameters(schoolExternalTool);

expect(func).toThrow('tool_param_duplicate');
});
});

describe('when given parameters has case insensitive duplicate', () => {
describe('when a parameter is a duplicate', () => {
const setup = () => {
const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build({
const externalTool: ExternalTool = externalToolFactory.buildWithId({
parameters: [
{ name: 'nameDuplicate', value: 'value' },
{ name: 'nameduplicate', value: 'value' },
customParameterFactory.build({
name: 'duplicate',
scope: CustomParameterScope.SCHOOL,
type: CustomParameterType.STRING,
isOptional: true,
}),
],
});
const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({
toolId: externalTool.id,
parameters: [
{ name: 'duplicate', value: undefined },
{ name: 'duplicate', value: undefined },
],
});

return {
externalTool,
schoolExternalTool,
};
};

it('should throw error when given parameters has case insensitive duplicate', () => {
const { schoolExternalTool } = setup();
it('should throw error', () => {
const { externalTool, schoolExternalTool } = setup();

const func = () => service.checkForDuplicateParameters(schoolExternalTool);
const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool);

expect(func).toThrowError('tool_param_duplicate');
});
});

describe('when given parameters has no duplicates', () => {
describe('when a parameter is unknown', () => {
const setup = () => {
const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build({
parameters: [
{ name: 'nameNoDuplicate1', value: 'value' },
{ name: 'nameNoDuplicate2', value: 'value' },
],
const externalTool: ExternalTool = externalToolFactory.buildWithId({
parameters: [],
});
const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.buildWithId({
toolId: externalTool.id,
parameters: [{ name: 'unknownParameter', value: undefined }],
});

return {
externalTool,
schoolExternalTool,
};
};

it('when given parameters has no duplicates should return without error', () => {
const { schoolExternalTool } = setup();
it('should throw error', () => {
const { externalTool, schoolExternalTool } = setup();

const func = () => service.checkForDuplicateParameters(schoolExternalTool);
const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool);

expect(func).not.toThrowError('tool_param_duplicate');
expect(func).toThrowError('tool_param_unknown');
});
});
});

describe('checkCustomParameterEntries', () => {
const createTools = (
externalToolMock?: Partial<ExternalTool>,
schoolExternalToolMock?: Partial<SchoolExternalTool>,
contextExternalToolMock?: Partial<ContextExternalTool>
) => {
const externalTool: ExternalTool = new ExternalTool({
...externalToolFactory.buildWithId(),
...externalToolMock,
});
const schoolExternalTool: SchoolExternalTool = schoolExternalToolFactory.build({
...schoolExternalToolFactory.buildWithId(),
...schoolExternalToolMock,
});
const schoolExternalToolId = schoolExternalTool.id as string;
const contextExternalTool: ContextExternalTool = contextExternalToolFactory.build({
...contextExternalToolFactory.buildWithId(),
...contextExternalToolMock,
});

return {
externalTool,
schoolExternalTool,
schoolExternalToolId,
contextExternalTool,
};
};

describe('when checking parameter is required', () => {
describe('and given parameter is not optional and parameter value is empty', () => {
Expand Down Expand Up @@ -280,16 +269,23 @@ describe('CommonToolValidationService', () => {

describe('when parameter is not school or context', () => {
const setup = () => {
const notSchoolParam: CustomParameter = customParameterFactory.build({
name: 'notSchoolParam',
scope: CustomParameterScope.GLOBAL,
type: CustomParameterType.BOOLEAN,
});

const { externalTool, schoolExternalTool } = createTools(
{ parameters: [notSchoolParam] },
{
parameters: [{ name: 'name', value: 'true' }],
parameters: [
customParameterFactory.build({
name: 'notSchoolParam',
scope: CustomParameterScope.GLOBAL,
type: CustomParameterType.BOOLEAN,
}),
customParameterFactory.build({
name: 'schoolParam',
scope: CustomParameterScope.SCHOOL,
type: CustomParameterType.BOOLEAN,
}),
],
},
{
parameters: [{ name: 'schoolParam', value: 'true' }],
}
);

Expand Down Expand Up @@ -320,7 +316,7 @@ describe('CommonToolValidationService', () => {
const { externalTool, schoolExternalTool } = createTools(
{ parameters: [missingParam] },
{
parameters: [{ name: 'anotherParam', value: 'value' }],
parameters: [],
}
);

Expand All @@ -339,18 +335,26 @@ describe('CommonToolValidationService', () => {
});
});

describe('when parameter is optional but is missing on params', () => {
describe('when parameter is optional and was not defined', () => {
const setup = () => {
const param: CustomParameter = customParameterFactory.build({
name: 'notChecked',
scope: CustomParameterScope.SCHOOL,
isOptional: true,
});

const { externalTool, schoolExternalTool } = createTools(
{ parameters: [param] },
{
parameters: [{ name: 'anotherParam', value: 'value' }],
parameters: [
customParameterFactory.build({
name: 'optionalParameter',
scope: CustomParameterScope.SCHOOL,
isOptional: true,
}),
customParameterFactory.build({
name: 'requiredParameter',
scope: CustomParameterScope.SCHOOL,
type: CustomParameterType.STRING,
isOptional: false,
}),
],
},
{
parameters: [{ name: 'requiredParameter', value: 'value' }],
}
);

Expand All @@ -360,7 +364,7 @@ describe('CommonToolValidationService', () => {
};
};

it('should return without error ', () => {
it('should return without error', () => {
const { externalTool, schoolExternalTool } = setup();

const func = () => service.checkCustomParameterEntries(externalTool, schoolExternalTool);
Expand All @@ -385,7 +389,7 @@ describe('CommonToolValidationService', () => {
},
undefined,
{
parameters: [{ name: 'anotherParam', value: 'value' }],
parameters: [],
}
);

Expand Down
46 changes: 32 additions & 14 deletions apps/server/src/modules/tool/common/service/common-tool-validation.service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,41 +29,59 @@ export class CommonToolValidationService {
return isValid;
}

public checkForDuplicateParameters(validatableTool: ValidatableTool): void {
const caseInsensitiveNames: string[] = validatableTool.parameters.map(({ name }: CustomParameterEntry) =>
name.toLowerCase()
public checkCustomParameterEntries(loadedExternalTool: ExternalTool, validatableTool: ValidatableTool): void {
this.checkForDuplicateParameters(validatableTool);

const parametersForScope: CustomParameter[] = (loadedExternalTool.parameters ?? []).filter(
(param: CustomParameter) =>
(validatableTool instanceof SchoolExternalTool && param.scope === CustomParameterScope.SCHOOL) ||
(validatableTool instanceof ContextExternalTool && param.scope === CustomParameterScope.CONTEXT)
);

this.checkForUnknownParameters(validatableTool, parametersForScope);

this.checkValidityOfParameters(validatableTool, parametersForScope);
}

private checkForDuplicateParameters(validatableTool: ValidatableTool): void {
const caseInsensitiveNames: string[] = validatableTool.parameters.map(({ name }: CustomParameterEntry) => name);

const uniqueNames: Set<string> = new Set(caseInsensitiveNames);

if (uniqueNames.size !== validatableTool.parameters.length) {
throw new ValidationError(
`tool_param_duplicate: The tool ${validatableTool.id ?? ''} contains multiple of the same custom parameters.`
);
}
}

public checkCustomParameterEntries(loadedExternalTool: ExternalTool, validatableTool: ValidatableTool): void {
if (loadedExternalTool.parameters) {
for (const param of loadedExternalTool.parameters) {
this.checkScopeAndValidateParameter(validatableTool, param);
private checkForUnknownParameters(validatableTool: ValidatableTool, parametersForScope: CustomParameter[]): void {
for (const entry of validatableTool.parameters) {
const foundParameter: CustomParameter | undefined = parametersForScope.find(
({ name }: CustomParameter): boolean => name === entry.name
);

if (!foundParameter) {
throw new ValidationError(
`tool_param_unknown: The parameter with name ${entry.name} is not part of this tool.`
);
}
}
}

private checkScopeAndValidateParameter(validatableTool: ValidatableTool, param: CustomParameter): void {
const foundEntry: CustomParameterEntry | undefined = validatableTool.parameters.find(
({ name }: CustomParameterEntry): boolean => name.toLowerCase() === param.name.toLowerCase()
);
private checkValidityOfParameters(validatableTool: ValidatableTool, parametersForScope: CustomParameter[]): void {
for (const param of parametersForScope) {
const foundEntry: CustomParameterEntry | undefined = validatableTool.parameters.find(
({ name }: CustomParameterEntry): boolean => name === param.name
);

if (param.scope === CustomParameterScope.SCHOOL && validatableTool instanceof SchoolExternalTool) {
this.validateParameter(param, foundEntry);
} else if (param.scope === CustomParameterScope.CONTEXT && validatableTool instanceof ContextExternalTool) {
this.validateParameter(param, foundEntry);
}
}

private validateParameter(param: CustomParameter, foundEntry: CustomParameterEntry | undefined): void {
this.checkOptionalParameter(param, foundEntry);

if (foundEntry) {
this.checkParameterType(foundEntry, param);
this.checkParameterRegex(foundEntry, param);
Expand Down
Loading

0 comments on commit 8f1aea3

Please sign in to comment.