From 1524fd5a08df20d32c28d403175114a72d0b64f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Romuald=20Cz=C5=82onkowski?= Date: Fri, 5 Dec 2025 12:00:13 +0100 Subject: [PATCH] fix: address code review issues for PR #467 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add 5-minute TTL to version cache (prevents stale data after upgrades) - Add type validation for version string from settings response - Support 'v' prefix in version strings (e.g., v1.2.3) - Fix race condition in getVersion() using promise-based locking - Fix empty settings regression (Issue #431) - use minimal defaults 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/services/n8n-api-client.ts | 45 ++++++++++++++++++++++++-------- src/services/n8n-validation.ts | 12 +++++++-- src/services/n8n-version.ts | 47 +++++++++++++++++++++++----------- 3 files changed, 76 insertions(+), 28 deletions(-) diff --git a/src/services/n8n-api-client.ts b/src/services/n8n-api-client.ts index a697dbf..cf16768 100644 --- a/src/services/n8n-api-client.ts +++ b/src/services/n8n-api-client.ts @@ -43,7 +43,7 @@ export class N8nApiClient { private maxRetries: number; private baseUrl: string; private versionInfo: N8nVersionInfo | null = null; - private versionFetched = false; + private versionPromise: Promise | null = null; constructor(config: N8nApiClientConfig) { const { baseUrl, apiKey, timeout = 30000, maxRetries = 3 } = config; @@ -95,19 +95,42 @@ export class N8nApiClient { } /** - * Get the n8n version, fetching it if not already cached + * Get the n8n version, fetching it if not already cached. + * Uses promise-based locking to prevent concurrent requests. */ async getVersion(): Promise { - if (!this.versionFetched) { - // Check if already cached globally - this.versionInfo = getCachedVersion(this.baseUrl); - if (!this.versionInfo) { - // Fetch from server - this.versionInfo = await fetchN8nVersion(this.baseUrl); - } - this.versionFetched = true; + // If we already have version info, return it + if (this.versionInfo) { + return this.versionInfo; } - return this.versionInfo; + + // If a fetch is already in progress, wait for it + if (this.versionPromise) { + return this.versionPromise; + } + + // Start a new fetch with promise-based locking + this.versionPromise = this.fetchVersionOnce(); + try { + this.versionInfo = await this.versionPromise; + return this.versionInfo; + } finally { + // Clear the promise so future calls can retry if needed + this.versionPromise = null; + } + } + + /** + * Internal method to fetch version once + */ + private async fetchVersionOnce(): Promise { + // Check if already cached globally + let version = getCachedVersion(this.baseUrl); + if (!version) { + // Fetch from server + version = await fetchN8nVersion(this.baseUrl); + } + return version; } /** diff --git a/src/services/n8n-validation.ts b/src/services/n8n-validation.ts index 3fe3a15..12ccae4 100644 --- a/src/services/n8n-validation.ts +++ b/src/services/n8n-validation.ts @@ -181,9 +181,17 @@ export function cleanWorkflowForUpdate(workflow: Workflow): Partial { filteredSettings[key] = value; } } - cleanedWorkflow.settings = filteredSettings; + // If no valid properties remain after filtering, use minimal defaults + // Issue #431: n8n API rejects empty settings objects + if (Object.keys(filteredSettings).length > 0) { + cleanedWorkflow.settings = filteredSettings; + } else { + // Minimal valid settings - executionOrder v1 is the modern default + cleanedWorkflow.settings = { executionOrder: 'v1' as const }; + } } else { - cleanedWorkflow.settings = {}; + // No settings provided - use minimal valid defaults + cleanedWorkflow.settings = { executionOrder: 'v1' as const }; } return cleanedWorkflow; diff --git a/src/services/n8n-version.ts b/src/services/n8n-version.ts index 3c6f279..d36a9ec 100644 --- a/src/services/n8n-version.ts +++ b/src/services/n8n-version.ts @@ -20,8 +20,16 @@ import axios from 'axios'; import { logger } from '../utils/logger'; import { N8nVersionInfo, N8nSettingsResponse } from '../types/n8n-api'; -// Cache version info per base URL to avoid repeated API calls -const versionCache = new Map(); +// Cache version info per base URL with TTL to handle server upgrades +interface CachedVersion { + info: N8nVersionInfo; + fetchedAt: number; +} + +// Cache TTL: 5 minutes - allows for server upgrades without requiring restart +const VERSION_CACHE_TTL_MS = 5 * 60 * 1000; + +const versionCache = new Map(); // Settings properties supported by each n8n version range // These are CUMULATIVE - each version adds to the previous @@ -53,8 +61,9 @@ const SETTINGS_BY_VERSION = { * Parse version string into structured version info */ export function parseVersion(versionString: string): N8nVersionInfo | null { - // Handle formats like "1.119.0", "1.37.0-beta.1", "0.200.0" - const match = versionString.match(/^(\d+)\.(\d+)\.(\d+)/); + // Handle formats like "1.119.0", "1.37.0-beta.1", "0.200.0", "v1.2.3" + // Support optional 'v' prefix for robustness + const match = versionString.match(/^v?(\d+)\.(\d+)\.(\d+)/); if (!match) { return null; } @@ -111,11 +120,11 @@ export function getSupportedSettingsProperties(version: N8nVersionInfo): Set { - // Check cache first + // Check cache first (with TTL) const cached = versionCache.get(baseUrl); - if (cached) { - logger.debug(`Using cached n8n version for ${baseUrl}: ${cached.version}`); - return cached; + if (cached && Date.now() - cached.fetchedAt < VERSION_CACHE_TTL_MS) { + logger.debug(`Using cached n8n version for ${baseUrl}: ${cached.info.version}`); + return cached.info; } try { @@ -138,14 +147,18 @@ export async function fetchN8nVersion(baseUrl: string): Promise