From a9f08fc5baebee2b7e481267c92f7e3f2d95d7ad Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E0=A4=95=E0=A4=BE=E0=A4=B0=E0=A4=A4=E0=A5=8B=E0=A4=AB?= =?UTF-8?q?=E0=A5=8D=E0=A4=AB=E0=A5=87=E0=A4=B2=E0=A4=B8=E0=A5=8D=E0=A4=95?= =?UTF-8?q?=E0=A5=8D=E0=A4=B0=E0=A4=BF=E0=A4=AA=E0=A5=8D=E0=A4=9F=E2=84=A2?= Date: Wed, 15 Feb 2023 16:09:53 +0100 Subject: [PATCH] fix(core): Fix issues with community node installation (no-changelog) (#5481) This fixes the following issues: * After a community node is installed, we were not calling `postProcessLoaders`, which was causing a bunch of unexpected behaviors, and sometimes even crashes. This was only happening in the session where the package was installed. After a crash, the restarted service was working without these issues. * After a community node is installed, the icon for the nodes and credentials were missing in the UI, as we were creating one icons route per installed package at startup, and this did not handle newly installed packages. restarting the service fixes this issue as well. Fixes https://community.n8n.io/t/showing-weird-count-on-community-nodes/23035 --- .../cli/src/CommunityNodes/packageModel.ts | 33 ++++---- packages/cli/src/LoadNodesAndCredentials.ts | 81 ++++++------------- packages/cli/src/ReloadNodesAndCredentials.ts | 13 ++- packages/cli/src/Server.ts | 25 +++--- packages/core/src/DirectoryLoader.ts | 2 +- 5 files changed, 65 insertions(+), 89 deletions(-) diff --git a/packages/cli/src/CommunityNodes/packageModel.ts b/packages/cli/src/CommunityNodes/packageModel.ts index 947fc4364d..522dbc8dbc 100644 --- a/packages/cli/src/CommunityNodes/packageModel.ts +++ b/packages/cli/src/CommunityNodes/packageModel.ts @@ -1,5 +1,5 @@ -import type { INodeTypeData, INodeTypeNameVersion } from 'n8n-workflow'; import { LoggerProxy } from 'n8n-workflow'; +import type { PackageDirectoryLoader } from 'n8n-core'; import * as Db from '@/Db'; import { InstalledNodes } from '@db/entities/InstalledNodes'; import { InstalledPackages } from '@db/entities/InstalledPackages'; @@ -12,8 +12,9 @@ export async function findInstalledPackage(packageName: string): Promise { - const installedPackage = await findInstalledPackage(packageName); - return installedPackage !== null; + return Db.collections.InstalledPackages.exist({ + where: { packageName }, + }); } export async function getAllInstalledPackages(): Promise { @@ -27,13 +28,11 @@ export async function removePackageFromDatabase( } export async function persistInstalledPackageData( - installedPackageName: string, - installedPackageVersion: string, - installedNodes: INodeTypeNameVersion[], - loadedNodeTypes: INodeTypeData, - authorName?: string, - authorEmail?: string, + packageLoader: PackageDirectoryLoader, ): Promise { + const { packageJson, nodeTypes, loadedNodes } = packageLoader; + const { name: packageName, version: installedVersion, author } = packageJson; + let installedPackage: InstalledPackages; try { @@ -41,21 +40,21 @@ export async function persistInstalledPackageData( const promises = []; const installedPackagePayload = Object.assign(new InstalledPackages(), { - packageName: installedPackageName, - installedVersion: installedPackageVersion, - authorName, - authorEmail, + packageName, + installedVersion, + authorName: author?.name, + authorEmail: author?.email, }); installedPackage = await transactionManager.save(installedPackagePayload); installedPackage.installedNodes = []; promises.push( - ...installedNodes.map(async (loadedNode) => { + ...loadedNodes.map(async (loadedNode) => { const installedNodePayload = Object.assign(new InstalledNodes(), { - name: loadedNodeTypes[loadedNode.name].type.description.displayName, + name: nodeTypes[loadedNode.name].type.description.displayName, type: loadedNode.name, latestVersion: loadedNode.version, - package: installedPackageName, + package: packageName, }); installedPackage.installedNodes.push(installedNodePayload); return transactionManager.save(installedNodePayload); @@ -70,7 +69,7 @@ export async function persistInstalledPackageData( LoggerProxy.error('Failed to save installed packages and nodes', { // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment error, - packageName: installedPackageName, + packageName, }); throw error; } diff --git a/packages/cli/src/LoadNodesAndCredentials.ts b/packages/cli/src/LoadNodesAndCredentials.ts index f44c131ad9..0f0f8f2c4b 100644 --- a/packages/cli/src/LoadNodesAndCredentials.ts +++ b/packages/cli/src/LoadNodesAndCredentials.ts @@ -22,7 +22,6 @@ import { access as fsAccess, mkdir, readdir as fsReaddir, stat as fsStat } from import path from 'path'; import config from '@/config'; import type { InstalledPackages } from '@db/entities/InstalledPackages'; -import type { InstalledNodes } from '@db/entities/InstalledNodes'; import { executeCommand } from '@/CommunityNodes/helpers'; import { CLI_DIR, @@ -198,23 +197,13 @@ export class LoadNodesAndCredentialsClass implements INodesAndCredentials { const finalNodeUnpackedPath = path.join(downloadFolder, 'node_modules', packageName); - const { loadedNodes, packageJson } = await this.runDirectoryLoader( - PackageDirectoryLoader, - finalNodeUnpackedPath, - ); + const loader = await this.runDirectoryLoader(PackageDirectoryLoader, finalNodeUnpackedPath); - if (loadedNodes.length > 0) { + if (loader.loadedNodes.length > 0) { // Save info to DB try { - const installedPackage = await persistInstalledPackageData( - packageJson.name, - packageJson.version, - loadedNodes, - this.loaded.nodes, - packageJson.author?.name, - packageJson.author?.email, - ); - this.attachNodesToNodeTypes(installedPackage.installedNodes); + const installedPackage = await persistInstalledPackageData(loader); + await this.postProcessLoaders(); await this.generateTypesForFrontend(); return installedPackage; } catch (error) { @@ -242,9 +231,13 @@ export class LoadNodesAndCredentialsClass implements INodesAndCredentials { await removePackageFromDatabase(installedPackage); - await this.generateTypesForFrontend(); + if (packageName in this.loaders) { + this.loaders[packageName].reset(); + delete this.loaders[packageName]; + } - this.unloadNodes(installedPackage.installedNodes); + await this.postProcessLoaders(); + await this.generateTypesForFrontend(); } async updateNpmModule( @@ -264,33 +257,17 @@ export class LoadNodesAndCredentialsClass implements INodesAndCredentials { throw error; } - this.unloadNodes(installedPackage.installedNodes); - const finalNodeUnpackedPath = path.join(downloadFolder, 'node_modules', packageName); - const { loadedNodes, packageJson } = await this.runDirectoryLoader( - PackageDirectoryLoader, - finalNodeUnpackedPath, - ); + const loader = await this.runDirectoryLoader(PackageDirectoryLoader, finalNodeUnpackedPath); - if (loadedNodes.length > 0) { + if (loader.loadedNodes.length > 0) { // Save info to DB try { await removePackageFromDatabase(installedPackage); - - const newlyInstalledPackage = await persistInstalledPackageData( - packageJson.name, - packageJson.version, - loadedNodes, - this.loaded.nodes, - packageJson.author?.name, - packageJson.author?.email, - ); - - this.attachNodesToNodeTypes(newlyInstalledPackage.installedNodes); - + const newlyInstalledPackage = await persistInstalledPackageData(loader); + await this.postProcessLoaders(); await this.generateTypesForFrontend(); - return newlyInstalledPackage; } catch (error) { LoggerProxy.error('Failed to save installed packages and nodes', { @@ -363,20 +340,6 @@ export class LoadNodesAndCredentialsClass implements INodesAndCredentials { }); } - private unloadNodes(installedNodes: InstalledNodes[]): void { - installedNodes.forEach((installedNode) => { - delete this.loaded.nodes[installedNode.type]; - }); - } - - private attachNodesToNodeTypes(installedNodes: InstalledNodes[]): void { - const loadedNodes = this.loaded.nodes; - installedNodes.forEach((installedNode) => { - const { type, sourcePath } = loadedNodes[installedNode.type]; - loadedNodes[installedNode.type] = { type, sourcePath }; - }); - } - /** * Run a loader of source files of nodes and credentials in a directory. */ @@ -386,16 +349,18 @@ export class LoadNodesAndCredentialsClass implements INodesAndCredentials { ) { const loader = new constructor(dir, this.excludeNodes, this.includeNodes); await loader.loadAll(); - this.loaders[dir] = loader; + this.loaders[loader.packageName] = loader; return loader; } async postProcessLoaders() { - this.types.nodes = []; - this.types.credentials = []; - for (const [dir, loader] of Object.entries(this.loaders)) { + this.known = { nodes: {}, credentials: {} }; + this.loaded = { nodes: {}, credentials: {} }; + this.types = { nodes: [], credentials: [] }; + + for (const loader of Object.values(this.loaders)) { // list of node & credential types that will be sent to the frontend - const { types } = loader; + const { types, directory } = loader; this.types.nodes = this.types.nodes.concat(types.nodes); this.types.credentials = this.types.credentials.concat(types.credentials); @@ -416,7 +381,7 @@ export class LoadNodesAndCredentialsClass implements INodesAndCredentials { const { className, sourcePath } = known.nodes[type]; this.known.nodes[type] = { className, - sourcePath: path.join(dir, sourcePath), + sourcePath: path.join(directory, sourcePath), }; } @@ -424,7 +389,7 @@ export class LoadNodesAndCredentialsClass implements INodesAndCredentials { const { className, sourcePath, nodesToTestWith } = known.credentials[type]; this.known.credentials[type] = { className, - sourcePath: path.join(dir, sourcePath), + sourcePath: path.join(directory, sourcePath), nodesToTestWith: nodesToTestWith?.map((nodeName) => `${packageName}.${nodeName}`), }; } diff --git a/packages/cli/src/ReloadNodesAndCredentials.ts b/packages/cli/src/ReloadNodesAndCredentials.ts index d7cc36c05e..f0fde3ba3a 100644 --- a/packages/cli/src/ReloadNodesAndCredentials.ts +++ b/packages/cli/src/ReloadNodesAndCredentials.ts @@ -1,5 +1,5 @@ import path from 'path'; -import { realpath } from 'fs/promises'; +import { realpath, access } from 'fs/promises'; import type { LoadNodesAndCredentialsClass } from '@/LoadNodesAndCredentials'; import type { NodeTypesClass } from '@/NodeTypes'; @@ -15,8 +15,15 @@ export const reloadNodesAndCredentials = async ( // eslint-disable-next-line import/no-extraneous-dependencies const { watch } = await import('chokidar'); - Object.entries(loadNodesAndCredentials.loaders).forEach(async ([dir, loader]) => { - const realModulePath = path.join(await realpath(dir), path.sep); + Object.values(loadNodesAndCredentials.loaders).forEach(async (loader) => { + try { + await access(loader.directory); + } catch { + // If directory doesn't exist, there is nothing to watch + return; + } + + const realModulePath = path.join(await realpath(loader.directory), path.sep); const reloader = debounce(async () => { const modulesToUnload = Object.keys(require.cache).filter((filePath) => filePath.startsWith(realModulePath), diff --git a/packages/cli/src/Server.ts b/packages/cli/src/Server.ts index b898f62a79..c653001316 100644 --- a/packages/cli/src/Server.ts +++ b/packages/cli/src/Server.ts @@ -1261,18 +1261,23 @@ class Server extends AbstractServer { }, }; - for (const [dir, loader] of Object.entries(this.loadNodesAndCredentials.loaders)) { - const pathPrefix = `/icons/${loader.packageName}`; - this.app.use(`${pathPrefix}/*/*.(svg|png)`, async (req, res) => { - const filePath = pathResolve(dir, req.originalUrl.substring(pathPrefix.length + 1)); + this.app.use('/icons/:packageName/*/*.(svg|png)', async (req, res) => { + const { packageName } = req.params; + const loader = this.loadNodesAndCredentials.loaders[packageName]; + if (loader) { + const pathPrefix = `/icons/${packageName}/`; + const filePath = pathResolve( + loader.directory, + req.originalUrl.substring(pathPrefix.length), + ); try { await fsAccess(filePath); - res.sendFile(filePath); - } catch { - res.sendStatus(404); - } - }); - } + return res.sendFile(filePath); + } catch {} + } + + res.sendStatus(404); + }); this.app.use( '/', diff --git a/packages/core/src/DirectoryLoader.ts b/packages/core/src/DirectoryLoader.ts index adf6888c2b..5709efa819 100644 --- a/packages/core/src/DirectoryLoader.ts +++ b/packages/core/src/DirectoryLoader.ts @@ -45,7 +45,7 @@ export abstract class DirectoryLoader { types: Types = { nodes: [], credentials: [] }; constructor( - protected readonly directory: string, + readonly directory: string, protected readonly excludeNodes: string[] = [], protected readonly includeNodes: string[] = [], ) {}