fix: address code review feedback for generic fixedCollection validator

- Fixed node type casing inconsistencies (compareDatasets -> comparedatasets, httpRequest -> httprequest)
- Improved error handling in hasInvalidStructure method with null/array checks
- Replaced all 'any' types with proper TypeScript types (NodeConfig, NodeConfigValue)
- Fixed potential memory leak in getAllPatterns by creating deep copies
- Added circular reference protection using WeakSet in hasInvalidStructure

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
czlonkowski
2025-08-02 09:20:56 +02:00
parent 066e7fc668
commit a5c60ddde1
2 changed files with 201 additions and 77 deletions

Binary file not shown.

View File

@@ -3,6 +3,13 @@
* Prevents the "propertyValues[itemName] is not iterable" error * Prevents the "propertyValues[itemName] is not iterable" error
*/ */
// Type definitions for node configurations
export type NodeConfigValue = string | number | boolean | null | undefined | NodeConfig | NodeConfigValue[];
export interface NodeConfig {
[key: string]: NodeConfigValue;
}
export interface FixedCollectionPattern { export interface FixedCollectionPattern {
nodeType: string; nodeType: string;
property: string; property: string;
@@ -18,10 +25,33 @@ export interface FixedCollectionValidationResult {
message: string; message: string;
fix: string; fix: string;
}>; }>;
autofix?: any; autofix?: NodeConfig | NodeConfigValue[];
} }
export class FixedCollectionValidator { export class FixedCollectionValidator {
/**
* Type guard to check if value is a NodeConfig
*/
private static isNodeConfig(value: NodeConfigValue): value is NodeConfig {
return typeof value === 'object' && value !== null && !Array.isArray(value);
}
/**
* Safely get nested property value
*/
private static getNestedValue(obj: NodeConfig, path: string): NodeConfigValue | undefined {
const parts = path.split('.');
let current: NodeConfigValue = obj;
for (const part of parts) {
if (!this.isNodeConfig(current)) {
return undefined;
}
current = current[part];
}
return current;
}
/** /**
* Known problematic patterns for various n8n nodes * Known problematic patterns for various n8n nodes
*/ */
@@ -106,11 +136,17 @@ export class FixedCollectionValidator {
/** /**
* Validate a node configuration for fixedCollection issues * Validate a node configuration for fixedCollection issues
* Includes protection against circular references
*/ */
static validate( static validate(
nodeType: string, nodeType: string,
config: Record<string, any> config: NodeConfig
): FixedCollectionValidationResult { ): FixedCollectionValidationResult {
// Early return for non-object configs
if (typeof config !== 'object' || config === null || Array.isArray(config)) {
return { isValid: true, errors: [] };
}
const normalizedNodeType = this.normalizeNodeType(nodeType); const normalizedNodeType = this.normalizeNodeType(nodeType);
const pattern = this.getPatternForNode(normalizedNodeType); const pattern = this.getPatternForNode(normalizedNodeType);
@@ -147,14 +183,19 @@ export class FixedCollectionValidator {
* Apply autofix to a configuration * Apply autofix to a configuration
*/ */
static applyAutofix( static applyAutofix(
config: Record<string, any>, config: NodeConfig,
pattern: FixedCollectionPattern pattern: FixedCollectionPattern
): Record<string, any> { ): NodeConfig | NodeConfigValue[] {
const fixedConfig = this.generateAutofix(config, pattern); const fixedConfig = this.generateAutofix(config, pattern);
// For If/Filter nodes, the autofix might return just the values array // For If/Filter nodes, the autofix might return just the values array
if (pattern.nodeType === 'if' || pattern.nodeType === 'filter') { if (pattern.nodeType === 'if' || pattern.nodeType === 'filter') {
if (config.conditions?.values) { const conditions = config.conditions;
return config.conditions.values; if (conditions && typeof conditions === 'object' && !Array.isArray(conditions) && 'values' in conditions) {
const values = conditions.values;
if (values !== undefined && values !== null &&
(Array.isArray(values) || typeof values === 'object')) {
return values as NodeConfig | NodeConfigValue[];
}
} }
} }
return fixedConfig; return fixedConfig;
@@ -180,19 +221,46 @@ export class FixedCollectionValidator {
/** /**
* Check if configuration has an invalid structure * Check if configuration has an invalid structure
* Includes circular reference protection
*/ */
private static hasInvalidStructure( private static hasInvalidStructure(
config: Record<string, any>, config: NodeConfig,
pattern: string pattern: string
): boolean { ): boolean {
const parts = pattern.split('.'); const parts = pattern.split('.');
let current = config; let current: NodeConfigValue = config;
const visited = new WeakSet<object>();
for (const part of parts) { for (const part of parts) {
if (!current || typeof current !== 'object' || !current[part]) { // Check for null/undefined
if (current === null || current === undefined) {
return false; return false;
} }
current = current[part];
// Check if it's an object (but not an array for property access)
if (typeof current !== 'object' || Array.isArray(current)) {
return false;
}
// Check for circular reference
if (visited.has(current)) {
return false; // Circular reference detected, invalid structure
}
visited.add(current);
// Check if property exists (using hasOwnProperty to avoid prototype pollution)
if (!Object.prototype.hasOwnProperty.call(current, part)) {
return false;
}
const nextValue = (current as NodeConfig)[part];
if (typeof nextValue !== 'object' || nextValue === null) {
// If we have more parts to traverse but current value is not an object, invalid structure
if (parts.indexOf(part) < parts.length - 1) {
return false;
}
}
current = nextValue as NodeConfig;
} }
return true; return true;
@@ -233,106 +301,158 @@ export class FixedCollectionValidator {
* Generate autofix for invalid structures * Generate autofix for invalid structures
*/ */
private static generateAutofix( private static generateAutofix(
config: Record<string, any>, config: NodeConfig,
pattern: FixedCollectionPattern pattern: FixedCollectionPattern
): any { ): NodeConfig | NodeConfigValue[] {
const fixedConfig = { ...config }; const fixedConfig = { ...config };
switch (pattern.nodeType) { switch (pattern.nodeType) {
case 'switch': case 'switch': {
if (config.rules?.conditions?.values) { const rules = config.rules;
fixedConfig.rules = { if (this.isNodeConfig(rules)) {
values: Array.isArray(config.rules.conditions.values) const conditions = rules.conditions;
? config.rules.conditions.values.map((condition: any, index: number) => ({ if (this.isNodeConfig(conditions) && 'values' in conditions) {
conditions: condition, const values = conditions.values;
outputKey: `output${index + 1}` fixedConfig.rules = {
})) values: Array.isArray(values)
: [{ ? values.map((condition, index) => ({
conditions: config.rules.conditions.values, conditions: condition,
outputKey: 'output1' outputKey: `output${index + 1}`
}] }))
}; : [{
} else if (config.rules?.conditions) { conditions: values,
fixedConfig.rules = { outputKey: 'output1'
values: [{ }]
conditions: config.rules.conditions, };
outputKey: 'output1' } else if (conditions) {
}] fixedConfig.rules = {
}; values: [{
conditions: conditions,
outputKey: 'output1'
}]
};
}
} }
break; break;
}
case 'if': case 'if':
case 'filter': case 'filter': {
if (config.conditions?.values) { const conditions = config.conditions;
return config.conditions.values; if (this.isNodeConfig(conditions) && 'values' in conditions) {
const values = conditions.values;
if (values !== undefined && values !== null &&
(Array.isArray(values) || typeof values === 'object')) {
return values as NodeConfig | NodeConfigValue[];
}
} }
break; break;
}
case 'summarize': case 'summarize': {
if (config.fieldsToSummarize?.values?.values) { const fieldsToSummarize = config.fieldsToSummarize;
fixedConfig.fieldsToSummarize = { if (this.isNodeConfig(fieldsToSummarize)) {
values: config.fieldsToSummarize.values.values const values = fieldsToSummarize.values;
}; if (this.isNodeConfig(values) && 'values' in values) {
fixedConfig.fieldsToSummarize = {
values: values.values
};
}
} }
break; break;
}
case 'comparedatasets': case 'comparedatasets': {
if (config.mergeByFields?.values?.values) { const mergeByFields = config.mergeByFields;
fixedConfig.mergeByFields = { if (this.isNodeConfig(mergeByFields)) {
values: config.mergeByFields.values.values const values = mergeByFields.values;
}; if (this.isNodeConfig(values) && 'values' in values) {
fixedConfig.mergeByFields = {
values: values.values
};
}
} }
break; break;
}
case 'sort': case 'sort': {
if (config.sortFieldsUi?.sortField?.values) { const sortFieldsUi = config.sortFieldsUi;
fixedConfig.sortFieldsUi = { if (this.isNodeConfig(sortFieldsUi)) {
sortField: config.sortFieldsUi.sortField.values const sortField = sortFieldsUi.sortField;
}; if (this.isNodeConfig(sortField) && 'values' in sortField) {
fixedConfig.sortFieldsUi = {
sortField: sortField.values
};
}
} }
break; break;
}
case 'aggregate': case 'aggregate': {
if (config.fieldsToAggregate?.fieldToAggregate?.values) { const fieldsToAggregate = config.fieldsToAggregate;
fixedConfig.fieldsToAggregate = { if (this.isNodeConfig(fieldsToAggregate)) {
fieldToAggregate: config.fieldsToAggregate.fieldToAggregate.values const fieldToAggregate = fieldsToAggregate.fieldToAggregate;
}; if (this.isNodeConfig(fieldToAggregate) && 'values' in fieldToAggregate) {
fixedConfig.fieldsToAggregate = {
fieldToAggregate: fieldToAggregate.values
};
}
} }
break; break;
}
case 'set': case 'set': {
if (config.fields?.values?.values) { const fields = config.fields;
fixedConfig.fields = { if (this.isNodeConfig(fields)) {
values: config.fields.values.values const values = fields.values;
}; if (this.isNodeConfig(values) && 'values' in values) {
fixedConfig.fields = {
values: values.values
};
}
} }
break; break;
}
case 'html': case 'html': {
if (config.extractionValues?.values?.values) { const extractionValues = config.extractionValues;
fixedConfig.extractionValues = { if (this.isNodeConfig(extractionValues)) {
values: config.extractionValues.values.values const values = extractionValues.values;
}; if (this.isNodeConfig(values) && 'values' in values) {
fixedConfig.extractionValues = {
values: values.values
};
}
} }
break; break;
}
case 'httprequest': case 'httprequest': {
if (config.body?.parameters?.values) { const body = config.body;
fixedConfig.body = { if (this.isNodeConfig(body)) {
...config.body, const parameters = body.parameters;
parameters: config.body.parameters.values if (this.isNodeConfig(parameters) && 'values' in parameters) {
}; fixedConfig.body = {
...body,
parameters: parameters.values
};
}
} }
break; break;
}
case 'airtable': case 'airtable': {
if (config.sort?.sortField?.values) { const sort = config.sort;
fixedConfig.sort = { if (this.isNodeConfig(sort)) {
sortField: config.sort.sortField.values const sortField = sort.sortField;
}; if (this.isNodeConfig(sortField) && 'values' in sortField) {
fixedConfig.sort = {
sortField: sortField.values
};
}
} }
break; break;
}
} }
return fixedConfig; return fixedConfig;
@@ -340,9 +460,13 @@ export class FixedCollectionValidator {
/** /**
* Get all known patterns (for testing and documentation) * Get all known patterns (for testing and documentation)
* Returns a deep copy to prevent external modifications
*/ */
static getAllPatterns(): FixedCollectionPattern[] { static getAllPatterns(): FixedCollectionPattern[] {
return [...this.KNOWN_PATTERNS]; return this.KNOWN_PATTERNS.map(pattern => ({
...pattern,
invalidPatterns: [...pattern.invalidPatterns]
}));
} }
/** /**