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

fix(editor): Improve filter component error handling #8832

Merged
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
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.",
);
}
throw error;
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.",
);
}
throw error;
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 @@ -349,7 +349,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.";
}
throw error;
}
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
Loading