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: Filter component - improve errors #10456

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
14 changes: 5 additions & 9 deletions packages/editor-ui/src/components/Error/NodeErrorView.vue
Original file line number Diff line number Diff line change
Expand Up @@ -216,17 +216,13 @@ function getErrorDescription(): string {
function addItemIndexSuffix(message: string): string {
let itemIndexSuffix = '';

const ITEM_INDEX_SUFFIX_TEXT = '[item ';

if (
hasManyInputItems.value &&
!message.includes(ITEM_INDEX_SUFFIX_TEXT) &&
props.error?.context?.itemIndex !== undefined
) {
itemIndexSuffix = ` [item ${props.error.context.itemIndex}]`;
if (hasManyInputItems.value && props.error?.context?.itemIndex !== undefined) {
itemIndexSuffix = `item ${props.error.context.itemIndex}`;
}

return message + itemIndexSuffix;
if (message.includes(itemIndexSuffix)) return message;

return `${message} [${itemIndexSuffix}]`;
}

function getErrorMessage(): string {
Expand Down
2 changes: 2 additions & 0 deletions packages/nodes-base/nodes/Filter/Filter.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ export class Filter extends VersionedNodeType {
iconColor: 'light-blue',
group: ['transform'],
description: 'Remove items matching a condition',
defaultVersion: 2.1,
};

const nodeVersions: IVersionedNodeType['nodeVersions'] = {
1: new FilterV1(baseDescription),
2: new FilterV2(baseDescription),
2.1: new FilterV2(baseDescription),
};

super(nodeVersions, baseDescription);
Expand Down
31 changes: 23 additions & 8 deletions packages/nodes-base/nodes/Filter/V2/FilterV2.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ import {
type INodeTypeDescription,
} from 'n8n-workflow';
import { ENABLE_LESS_STRICT_TYPE_VALIDATION } from '../../../utils/constants';
import { getTypeValidationParameter, getTypeValidationStrictness } from '../../If/V2/utils';
import { looseTypeValidationProperty } from '../../../utils/descriptions';

export class FilterV2 implements INodeType {
description: INodeTypeDescription;

constructor(baseDescription: INodeTypeBaseDescription) {
this.description = {
...baseDescription,
version: 2,
version: [2, 2.1],
defaults: {
name: 'Filter',
color: '#229eff',
Expand All @@ -35,7 +37,16 @@ export class FilterV2 implements INodeType {
typeOptions: {
filter: {
caseSensitive: '={{!$parameter.options.ignoreCase}}',
typeValidation: '={{$parameter.options.looseTypeValidation ? "loose" : "strict"}}',
typeValidation: getTypeValidationStrictness(2.1),
},
},
},
{
...looseTypeValidationProperty,
default: false,
displayOptions: {
show: {
'@version': [{ _cnd: { gte: 2.1 } }],
},
},
},
Expand All @@ -54,11 +65,12 @@ export class FilterV2 implements INodeType {
default: true,
},
{
displayName: 'Less Strict Type Validation',
description: 'Whether to try casting value types based on the selected operator',
name: 'looseTypeValidation',
type: 'boolean',
default: true,
...looseTypeValidationProperty,
displayOptions: {
show: {
'@version': [{ _cnd: { lt: 2.1 } }],
},
},
},
],
},
Expand All @@ -82,7 +94,10 @@ export class FilterV2 implements INodeType {
extractValue: true,
}) as boolean;
} catch (error) {
if (!options.looseTypeValidation && !error.description) {
if (
!getTypeValidationParameter(2.1)(this, itemIndex, options.looseTypeValidation) &&
!error.description
) {
set(error, 'description', ENABLE_LESS_STRICT_TYPE_VALIDATION);
}
set(error, 'context.itemIndex', itemIndex);
Expand Down
3 changes: 2 additions & 1 deletion packages/nodes-base/nodes/If/If.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,12 +13,13 @@ export class If extends VersionedNodeType {
iconColor: 'green',
group: ['transform'],
description: 'Route items to different branches (true/false)',
defaultVersion: 2,
defaultVersion: 2.1,
};

const nodeVersions: IVersionedNodeType['nodeVersions'] = {
1: new IfV1(baseDescription),
2: new IfV2(baseDescription),
2.1: new IfV2(baseDescription),
};

super(nodeVersions, baseDescription);
Expand Down
31 changes: 23 additions & 8 deletions packages/nodes-base/nodes/If/V2/IfV2.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,16 @@ import {
type INodeTypeDescription,
} from 'n8n-workflow';
import { ENABLE_LESS_STRICT_TYPE_VALIDATION } from '../../../utils/constants';
import { looseTypeValidationProperty } from '../../../utils/descriptions';
import { getTypeValidationParameter, getTypeValidationStrictness } from './utils';

export class IfV2 implements INodeType {
description: INodeTypeDescription;

constructor(baseDescription: INodeTypeBaseDescription) {
this.description = {
...baseDescription,
version: 2,
version: [2, 2.1],
defaults: {
name: 'If',
color: '#408000',
Expand All @@ -35,7 +37,16 @@ export class IfV2 implements INodeType {
typeOptions: {
filter: {
caseSensitive: '={{!$parameter.options.ignoreCase}}',
typeValidation: '={{$parameter.options.looseTypeValidation ? "loose" : "strict"}}',
typeValidation: getTypeValidationStrictness(2.1),
},
},
},
{
...looseTypeValidationProperty,
default: false,
displayOptions: {
show: {
'@version': [{ _cnd: { gte: 2.1 } }],
},
},
},
Expand All @@ -54,11 +65,12 @@ export class IfV2 implements INodeType {
default: true,
},
{
displayName: 'Less Strict Type Validation',
description: 'Whether to try casting value types based on the selected operator',
name: 'looseTypeValidation',
type: 'boolean',
default: true,
...looseTypeValidationProperty,
displayOptions: {
show: {
'@version': [{ _cnd: { lt: 2.1 } }],
},
},
},
],
},
Expand All @@ -82,7 +94,10 @@ export class IfV2 implements INodeType {
extractValue: true,
}) as boolean;
} catch (error) {
if (!options.looseTypeValidation && !error.description) {
if (
!getTypeValidationParameter(2.1)(this, itemIndex, options.looseTypeValidation) &&
!error.description
) {
set(error, 'description', ENABLE_LESS_STRICT_TYPE_VALIDATION);
}
set(error, 'context.itemIndex', itemIndex);
Expand Down
15 changes: 15 additions & 0 deletions packages/nodes-base/nodes/If/V2/utils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import type { IExecuteFunctions } from 'n8n-workflow';

export const getTypeValidationStrictness = (version: number) => {
return `={{ ($nodeVersion < ${version} ? $parameter.options.looseTypeValidation : $parameter.looseTypeValidation) ? "loose" : "strict" }}`;
};

export const getTypeValidationParameter = (version: number) => {
return (context: IExecuteFunctions, itemIndex: number, option: boolean | undefined) => {
if (context.getNode().typeVersion < version) {
return option;
} else {
return context.getNodeParameter('looseTypeValidation', itemIndex, false) as boolean;
}
};
};
3 changes: 2 additions & 1 deletion packages/nodes-base/nodes/Switch/Switch.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,14 @@ export class Switch extends VersionedNodeType {
iconColor: 'light-blue',
group: ['transform'],
description: 'Route items depending on defined expression or rules',
defaultVersion: 3,
defaultVersion: 3.1,
};

const nodeVersions: IVersionedNodeType['nodeVersions'] = {
1: new SwitchV1(baseDescription),
2: new SwitchV2(baseDescription),
3: new SwitchV3(baseDescription),
3.1: new SwitchV3(baseDescription),
};

super(nodeVersions, baseDescription);
Expand Down
36 changes: 27 additions & 9 deletions packages/nodes-base/nodes/Switch/V3/SwitchV3.node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { ApplicationError, NodeConnectionType, NodeOperationError } from 'n8n-wo
import set from 'lodash/set';
import { capitalize } from '@utils/utilities';
import { ENABLE_LESS_STRICT_TYPE_VALIDATION } from '../../../utils/constants';
import { looseTypeValidationProperty } from '../../../utils/descriptions';
import { getTypeValidationParameter, getTypeValidationStrictness } from '../../If/V2/utils';

const configuredOutputs = (parameters: INodeParameters) => {
const mode = parameters.mode as string;
Expand Down Expand Up @@ -48,7 +50,7 @@ export class SwitchV3 implements INodeType {
this.description = {
...baseDescription,
subtitle: `=mode: {{(${capitalize})($parameter["mode"])}}`,
version: [3],
version: [3, 3.1],
defaults: {
name: 'Switch',
color: '#506000',
Expand Down Expand Up @@ -157,8 +159,7 @@ export class SwitchV3 implements INodeType {
multipleValues: false,
filter: {
caseSensitive: '={{!$parameter.options.ignoreCase}}',
typeValidation:
'={{$parameter.options.looseTypeValidation ? "loose" : "strict"}}',
typeValidation: getTypeValidationStrictness(3.1),
},
},
},
Expand All @@ -184,6 +185,15 @@ export class SwitchV3 implements INodeType {
},
],
},
{
...looseTypeValidationProperty,
default: false,
displayOptions: {
show: {
'@version': [{ _cnd: { gte: 3.1 } }],
},
},
},
{
displayName: 'Options',
name: 'options',
Expand Down Expand Up @@ -218,11 +228,12 @@ export class SwitchV3 implements INodeType {
default: true,
},
{
displayName: 'Less Strict Type Validation',
description: 'Whether to try casting value types based on the selected operator',
name: 'looseTypeValidation',
type: 'boolean',
default: true,
...looseTypeValidationProperty,
displayOptions: {
show: {
'@version': [{ _cnd: { lt: 3.1 } }],
},
},
},
{
displayName: 'Rename Fallback Output',
Expand Down Expand Up @@ -349,7 +360,14 @@ export class SwitchV3 implements INodeType {
},
) as boolean;
} catch (error) {
if (!options.looseTypeValidation && !error.description) {
if (
!getTypeValidationParameter(3.1)(
this,
itemIndex,
options.looseTypeValidation as boolean,
) &&
!error.description
) {
error.description = ENABLE_LESS_STRICT_TYPE_VALIDATION;
}
set(error, 'context.itemIndex', itemIndex);
Expand Down
8 changes: 8 additions & 0 deletions packages/nodes-base/utils/descriptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,14 @@ export const returnAllOrLimit: INodeProperties[] = [
},
];

export const looseTypeValidationProperty: INodeProperties = {
displayName: 'Less Strict Type Validation',
description: 'Whether to try casting value types based on the selected operator',
name: 'looseTypeValidation',
type: 'boolean',
default: true,
};

export const encodeDecodeOptions: INodePropertyOptions[] = [
{
name: 'armscii8',
Expand Down
53 changes: 50 additions & 3 deletions packages/workflow/src/NodeParameters/FilterParameter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,14 +94,61 @@ function parseFilterConditionValues(
)} ${suffix}`;
};

const invalidTypeDescription = 'Try changing the type of comparison.';
const getTypeDescription = (isStrict: boolean) => {
if (isStrict)
return 'Try changing the type of the comparison, or enabling less strict type validation.';
return 'Try changing the type of the comparison.';
};

const composeInvalidTypeDescription = (
type: string,
fromType: string,
valuePosition: 'first' | 'second',
) => {
fromType = fromType.toLocaleLowerCase();
const expectedType = withIndefiniteArticle(type);

let convertionFunction = '';
if (type === 'string') {
convertionFunction = '.toString()';
} else if (type === 'number') {
convertionFunction = '.toNumber()';
} else if (type === 'boolean') {
convertionFunction = '.toBoolean()';
}

if (strict && convertionFunction) {
const suggestFunction = ` by adding <code>${convertionFunction}</code>`;
return `
<p>Try either:</p>
<ol>
<li>Enabling less strict type validation</li>
<li>Converting the ${valuePosition} field to ${expectedType}${suggestFunction}</li>
</ol>
`;
}

return getTypeDescription(strict);
};

if (!leftValid && !rightValid && typeof condition.leftValue === typeof condition.rightValue) {
return {
ok: false,
error: new FilterError(
`Comparison type expects ${withIndefiniteArticle(operator.type)} but both fields are ${withIndefiniteArticle(
typeof condition.leftValue,
)}`,
getTypeDescription(strict),
),
};
}

if (!leftValid) {
return {
ok: false,
error: new FilterError(
composeInvalidTypeMessage(operator.type, typeof condition.leftValue, leftValueString),
invalidTypeDescription,
composeInvalidTypeDescription(operator.type, typeof condition.leftValue, 'first'),
),
};
}
Expand All @@ -111,7 +158,7 @@ function parseFilterConditionValues(
ok: false,
error: new FilterError(
composeInvalidTypeMessage(rightType, typeof condition.rightValue, rightValueString),
invalidTypeDescription,
composeInvalidTypeDescription(rightType, typeof condition.rightValue, 'second'),
),
};
}
Expand Down
Loading