From 871108e54ecc76ca83397886e2f8c616e38143d0 Mon Sep 17 00:00:00 2001 From: Elias Meire Date: Wed, 6 Mar 2024 19:49:25 +0100 Subject: [PATCH 1/2] Improve filter component error handling + fix object:isEmpty + null --- .../components/FilterConditions/Condition.vue | 11 +++- .../src/components/FilterConditions/utils.ts | 4 ++ .../nodes/Filter/V2/FilterV2.node.ts | 2 +- packages/nodes-base/nodes/If/V2/IfV2.node.ts | 2 +- .../nodes/Switch/V3/SwitchV3.node.ts | 2 +- .../src/NodeParameters/FilterParameter.ts | 51 +++++-------------- .../workflow/test/FilterParameter.test.ts | 4 +- 7 files changed, 32 insertions(+), 44 deletions(-) diff --git a/packages/editor-ui/src/components/FilterConditions/Condition.vue b/packages/editor-ui/src/components/FilterConditions/Condition.vue index b7f284da65ec6..f14c55add3c5e 100644 --- a/packages/editor-ui/src/components/FilterConditions/Condition.vue +++ b/packages/editor-ui/src/components/FilterConditions/Condition.vue @@ -16,6 +16,7 @@ import { type FilterOperatorId } from './constants'; import { getFilterOperator, handleOperatorChange, + isEmptyInput, operatorTypeToNodeProperty, resolveCondition, } from './utils'; @@ -54,12 +55,20 @@ const operatorId = computed(() => { }); 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]; } diff --git a/packages/editor-ui/src/components/FilterConditions/utils.ts b/packages/editor-ui/src/components/FilterConditions/utils.ts index d26bcc74d1d9e..cbaee87e9695d 100644 --- a/packages/editor-ui/src/components/FilterConditions/utils.ts +++ b/packages/editor-ui/src/components/FilterConditions/utils.ts @@ -56,6 +56,10 @@ export const handleOperatorChange = ({ return condition; }; +export const isEmptyInput = (value: unknown): boolean => { + return value === '' || value === '='; +}; + export const resolveCondition = ({ condition, options, diff --git a/packages/nodes-base/nodes/Filter/V2/FilterV2.node.ts b/packages/nodes-base/nodes/Filter/V2/FilterV2.node.ts index c74498e5053a8..e8e0fbba2d0ae 100644 --- a/packages/nodes-base/nodes/Filter/V2/FilterV2.node.ts +++ b/packages/nodes-base/nodes/Filter/V2/FilterV2.node.ts @@ -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; diff --git a/packages/nodes-base/nodes/If/V2/IfV2.node.ts b/packages/nodes-base/nodes/If/V2/IfV2.node.ts index 3cf0df38680c6..9898db2c4e31c 100644 --- a/packages/nodes-base/nodes/If/V2/IfV2.node.ts +++ b/packages/nodes-base/nodes/If/V2/IfV2.node.ts @@ -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; diff --git a/packages/nodes-base/nodes/Switch/V3/SwitchV3.node.ts b/packages/nodes-base/nodes/Switch/V3/SwitchV3.node.ts index eceefdd79747b..ba2131f66c52a 100644 --- a/packages/nodes-base/nodes/Switch/V3/SwitchV3.node.ts +++ b/packages/nodes-base/nodes/Switch/V3/SwitchV3.node.ts @@ -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; } diff --git a/packages/workflow/src/NodeParameters/FilterParameter.ts b/packages/workflow/src/NodeParameters/FilterParameter.ts index 4f09b49948967..ae0d7198c4e2b 100644 --- a/packages/workflow/src/NodeParameters/FilterParameter.ts +++ b/packages/workflow/src/NodeParameters/FilterParameter.ts @@ -32,7 +32,7 @@ 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 }); } @@ -63,50 +63,25 @@ 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 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`; - } + const composeInvalidTypeMessage = (type: string, fromType: string, value: string) => { + fromType = fromType.toLocaleLowerCase(); if (strict) { - return { - ok: false, - error: new FilterError( - `${providedValues} '${leftValueString}' and '${rightValueString}'${inCondition}are not of the expected type ${types} ${itemSuffix}`, - errorDescription, - ), - }; + return `Wrong type: expected ${type} ${inCondition}, received '${value}' (${fromType}) ${itemSuffix}`; } - - 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; - - if (strict) { - return `The provided value ${fieldNumber} '${value}'${inCondition}is not of the expected type '${type}' ${itemSuffix}`; - } - return `The provided value ${fieldNumber} '${value}'${inCondition}cannot be converted to the expected type '${type}' ${itemSuffix}`; + return `Conversion error: '${value}' (${fromType}) cannot be converted to type '${type}' ${inCondition} ${itemSuffix}`; }; + 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, ), }; } @@ -115,8 +90,8 @@ function parseFilterConditionValues( return { ok: false, error: new FilterError( - composeInvalidTypeMessage('right', rightType, rightValueString), - errorDescription, + composeInvalidTypeMessage(rightType, typeof condition.rightValue, rightValueString), + invalidTypeDescription, ), }; } @@ -301,7 +276,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; } diff --git a/packages/workflow/test/FilterParameter.test.ts b/packages/workflow/test/FilterParameter.test.ts index 37ffe4c57c9bd..1554ffaff6716 100644 --- a/packages/workflow/test/FilterParameter.test.ts +++ b/packages/workflow/test/FilterParameter.test.ts @@ -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({ From 5908ad0e723006414e8834f7adc3ab998b1ef6b0 Mon Sep 17 00:00:00 2001 From: Elias Meire Date: Thu, 7 Mar 2024 11:14:17 +0100 Subject: [PATCH 2/2] Improve error messages, update test --- .../src/NodeParameters/FilterParameter.ts | 17 +++++++++++++---- packages/workflow/test/FilterParameter.test.ts | 8 ++++---- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/packages/workflow/src/NodeParameters/FilterParameter.ts b/packages/workflow/src/NodeParameters/FilterParameter.ts index ae0d7198c4e2b..5bbeaea569d63 100644 --- a/packages/workflow/src/NodeParameters/FilterParameter.ts +++ b/packages/workflow/src/NodeParameters/FilterParameter.ts @@ -37,6 +37,11 @@ function parseSingleFilterValue( : 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, @@ -63,15 +68,19 @@ function parseFilterConditionValues( condition.rightValue.startsWith('=')); const leftValueString = String(condition.leftValue); const rightValueString = String(condition.rightValue); - const inCondition = errorFormat === 'full' ? `in condition ${index + 1}` : ' '; - const itemSuffix = `[item ${itemIndex}]`; + const suffix = + errorFormat === 'full' ? `[condition ${index}, item ${itemIndex}]` : `[item ${itemIndex}]`; const composeInvalidTypeMessage = (type: string, fromType: string, value: string) => { fromType = fromType.toLocaleLowerCase(); if (strict) { - return `Wrong type: expected ${type} ${inCondition}, received '${value}' (${fromType}) ${itemSuffix}`; + return `Wrong type: '${value}' is ${withIndefiniteArticle( + fromType, + )} but was expecting ${withIndefiniteArticle(type)} ${suffix}`; } - return `Conversion error: '${value}' (${fromType}) cannot be converted to type '${type}' ${inCondition} ${itemSuffix}`; + return `Conversion error: the ${fromType} '${value}' can't be converted to ${withIndefiniteArticle( + type, + )} ${suffix}`; }; const invalidTypeDescription = 'Try changing the type of comparison.'; diff --git a/packages/workflow/test/FilterParameter.test.ts b/packages/workflow/test/FilterParameter.test.ts index 1554ffaff6716..b8cd8bac7eb5a 100644 --- a/packages/workflow/test/FilterParameter.test.ts +++ b/packages/workflow/test/FilterParameter.test.ts @@ -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]", ); }); @@ -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]", ); }); @@ -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]", ); }); }); @@ -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]", ); }); });