Skip to content

Commit

Permalink
fix: Improve filter component error handling (n8n-io#8832)
Browse files Browse the repository at this point in the history
  • Loading branch information
elsmr authored Mar 8, 2024
1 parent 5d52bda commit 76fe960
Show file tree
Hide file tree
Showing 7 changed files with 46 additions and 49 deletions.
11 changes: 10 additions & 1 deletion packages/editor-ui/src/components/FilterConditions/Condition.vue
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { type FilterOperatorId } from './constants';
import {
getFilterOperator,
handleOperatorChange,
isEmptyInput,
operatorTypeToNodeProperty,
resolveCondition,
} from './utils';
Expand Down Expand Up @@ -54,12 +55,20 @@ const operatorId = computed<FilterOperatorId>(() => {
});
const operator = computed(() => getFilterOperator(operatorId.value));
const isEmpty = computed(() => {
if (operator.value.singleValue) {
return isEmptyInput(condition.value.leftValue);
}
return isEmptyInput(condition.value.leftValue) && isEmptyInput(condition.value.rightValue);
});
const conditionResult = computed(() =>
resolveCondition({ condition: condition.value, options: props.options }),
);
const allIssues = computed(() => {
if (conditionResult.value.status === 'validation_error') {
if (conditionResult.value.status === 'validation_error' && !isEmpty.value) {
return [conditionResult.value.error];
}
Expand Down
4 changes: 4 additions & 0 deletions packages/editor-ui/src/components/FilterConditions/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,10 @@ export const handleOperatorChange = ({
return condition;
};

export const isEmptyInput = (value: unknown): boolean => {
return value === '' || value === '=';
};

export const resolveCondition = ({
condition,
options,
Expand Down
2 changes: 1 addition & 1 deletion packages/nodes-base/nodes/Filter/V2/FilterV2.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export class FilterV2 implements INodeType {
set(
error,
'description',
"Try to change the operator, switch ON the option 'Less Strict Type Validation', or change the type with an expression",
"Try changing the type of comparison. Alternatively you can enable 'Less Strict Type Validation' in the options.",
);
}
set(error, 'context.itemIndex', itemIndex);
Expand Down
2 changes: 1 addition & 1 deletion packages/nodes-base/nodes/If/V2/IfV2.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export class IfV2 implements INodeType {
set(
error,
'description',
"Try to change the operator, switch ON the option 'Less Strict Type Validation', or change the type with an expression",
"Try changing the type of comparison. Alternatively you can enable 'Less Strict Type Validation' in the options.",
);
}
set(error, 'context.itemIndex', itemIndex);
Expand Down
2 changes: 1 addition & 1 deletion packages/nodes-base/nodes/Switch/V3/SwitchV3.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ export class SwitchV3 implements INodeType {
} catch (error) {
if (!options.looseTypeValidation) {
error.description =
"Try to change the operator, switch ON the option 'Less Strict Type Validation', or change the type with an expression";
"Try changing the type of comparison. Alternatively you can enable 'Less Strict Type Validation' in the options.";
}
set(error, 'context.itemIndex', itemIndex);
set(error, 'node', this.getNode());
Expand Down
62 changes: 23 additions & 39 deletions packages/workflow/src/NodeParameters/FilterParameter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,16 @@ function parseSingleFilterValue(
type: FilterOperatorType,
strict = false,
): ValidationResult {
return type === 'any' || value === null || value === undefined || value === ''
return type === 'any' || value === null || value === undefined
? ({ valid: true, newValue: value } as ValidationResult)
: validateFieldType('filter', value, type, { strict, parseStrings: true });
}

const withIndefiniteArticle = (noun: string): string => {
const article = 'aeiou'.includes(noun.charAt(0)) ? 'an' : 'a';
return `${article} ${noun}`;
};

function parseFilterConditionValues(
condition: FilterConditionValue,
options: FilterOptionsValue,
Expand All @@ -63,50 +68,29 @@ function parseFilterConditionValues(
condition.rightValue.startsWith('='));
const leftValueString = String(condition.leftValue);
const rightValueString = String(condition.rightValue);
const errorDescription = 'Try to change the operator, or change the type with an expression';
const inCondition = errorFormat === 'full' ? ` in condition ${index + 1} ` : ' ';
const itemSuffix = `[item ${itemIndex}]`;

if (!leftValid && !rightValid) {
const providedValues = 'The provided values';
let types = `'${operator.type}'`;
if (rightType !== operator.type) {
types = `'${operator.type}' and '${rightType}' respectively`;
}
if (strict) {
return {
ok: false,
error: new FilterError(
`${providedValues} '${leftValueString}' and '${rightValueString}'${inCondition}are not of the expected type ${types} ${itemSuffix}`,
errorDescription,
),
};
}

return {
ok: false,
error: new FilterError(
`${providedValues} '${leftValueString}' and '${rightValueString}'${inCondition}cannot be converted to the expected type ${types} ${itemSuffix}`,
errorDescription,
),
};
}

const composeInvalidTypeMessage = (field: 'left' | 'right', type: string, value: string) => {
const fieldNumber = field === 'left' ? 1 : 2;
const suffix =
errorFormat === 'full' ? `[condition ${index}, item ${itemIndex}]` : `[item ${itemIndex}]`;

const composeInvalidTypeMessage = (type: string, fromType: string, value: string) => {
fromType = fromType.toLocaleLowerCase();
if (strict) {
return `The provided value ${fieldNumber} '${value}'${inCondition}is not of the expected type '${type}' ${itemSuffix}`;
return `Wrong type: '${value}' is ${withIndefiniteArticle(
fromType,
)} but was expecting ${withIndefiniteArticle(type)} ${suffix}`;
}
return `The provided value ${fieldNumber} '${value}'${inCondition}cannot be converted to the expected type '${type}' ${itemSuffix}`;
return `Conversion error: the ${fromType} '${value}' can't be converted to ${withIndefiniteArticle(
type,
)} ${suffix}`;
};

const invalidTypeDescription = 'Try changing the type of comparison.';

if (!leftValid) {
return {
ok: false,
error: new FilterError(
composeInvalidTypeMessage('left', operator.type, leftValueString),
errorDescription,
composeInvalidTypeMessage(operator.type, typeof condition.leftValue, leftValueString),
invalidTypeDescription,
),
};
}
Expand All @@ -115,8 +99,8 @@ function parseFilterConditionValues(
return {
ok: false,
error: new FilterError(
composeInvalidTypeMessage('right', rightType, rightValueString),
errorDescription,
composeInvalidTypeMessage(rightType, typeof condition.rightValue, rightValueString),
invalidTypeDescription,
),
};
}
Expand Down Expand Up @@ -301,7 +285,7 @@ export function executeFilterCondition(

switch (condition.operator.operation) {
case 'empty':
return !!left && Object.keys(left).length === 0;
return !left || Object.keys(left).length === 0;
case 'notEmpty':
return !!left && Object.keys(left).length !== 0;
}
Expand Down
12 changes: 6 additions & 6 deletions packages/workflow/test/FilterParameter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ describe('FilterParameter', () => {
}),
),
).toThrowError(
"The provided values '15' and 'true' in condition 1 are not of the expected type 'number' [item 0]",
"Wrong type: '15' is a string but was expecting a number [condition 0, item 0]",
);
});

Expand All @@ -136,7 +136,7 @@ describe('FilterParameter', () => {
}),
),
).toThrowError(
"The provided value 1 '[]' in condition 1 is not of the expected type 'array' [item 0]",
"Wrong type: '[]' is a string but was expecting an array [condition 0, item 0]",
);
});

Expand All @@ -155,7 +155,7 @@ describe('FilterParameter', () => {
}),
),
).toThrowError(
"The provided value 1 '{}' in condition 1 is not of the expected type 'object' [item 0]",
"Wrong type: '{}' is a string but was expecting an object [condition 0, item 0]",
);
});
});
Expand Down Expand Up @@ -201,7 +201,7 @@ describe('FilterParameter', () => {
}),
),
).toThrowError(
"The provided values 'a string' and '15' in condition 1 cannot be converted to the expected type 'boolean'",
"Conversion error: the string 'a string' can't be converted to a boolean [condition 0, item 0]",
);
});
});
Expand Down Expand Up @@ -1011,8 +1011,8 @@ describe('FilterParameter', () => {
it.each([
{ left: {}, expected: true },
{ left: { foo: 'bar' }, expected: false },
{ left: undefined, expected: false },
{ left: null, expected: false },
{ left: undefined, expected: true },
{ left: null, expected: true },
])('object:empty($left) === $expected', ({ left, expected }) => {
const result = executeFilter(
filterFactory({
Expand Down

0 comments on commit 76fe960

Please sign in to comment.