From 432ac1da59e173ce4c0f2abbc416743d9953ba70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 2 Aug 2024 12:01:10 +0200 Subject: [PATCH 01/98] fix(core): Surface enterprise trial error message (#10267) --- packages/cli/src/license/license.controller.ts | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/cli/src/license/license.controller.ts b/packages/cli/src/license/license.controller.ts index 1c0ca8c0d68ed..945f3650ea340 100644 --- a/packages/cli/src/license/license.controller.ts +++ b/packages/cli/src/license/license.controller.ts @@ -1,6 +1,8 @@ import { Get, Post, RestController, GlobalScope } from '@/decorators'; import { AuthenticatedRequest, LicenseRequest } from '@/requests'; import { LicenseService } from './license.service'; +import { BadRequestError } from '@/errors/response-errors/bad-request.error'; +import type { AxiosError } from 'axios'; @RestController('/license') export class LicenseController { @@ -14,7 +16,18 @@ export class LicenseController { @Post('/enterprise/request_trial') @GlobalScope('license:manage') async requestEnterpriseTrial(req: AuthenticatedRequest) { - await this.licenseService.requestEnterpriseTrial(req.user); + try { + await this.licenseService.requestEnterpriseTrial(req.user); + } catch (error: unknown) { + if (error instanceof Error) { + const errorMsg = + (error as AxiosError<{ message: string }>).response?.data?.message ?? error.message; + + throw new BadRequestError(errorMsg); + } else { + throw new BadRequestError('Failed to request trial'); + } + } } @Post('/activate') From dc8c94d036ca6ee5f3fbed839770ce5a902cca11 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 2 Aug 2024 12:01:42 +0200 Subject: [PATCH 02/98] ci: Introduce lint rule `no-type-unsafe-event-emitter` (no-changelog) (#10254) --- packages/@n8n_io/eslint-config/local-rules.js | 30 +++++++++++++++++++ packages/cli/.eslintrc.js | 7 +++++ .../MessageEventBus/MessageEventBus.ts | 2 ++ 3 files changed, 39 insertions(+) diff --git a/packages/@n8n_io/eslint-config/local-rules.js b/packages/@n8n_io/eslint-config/local-rules.js index c9b533a032e1e..152415e14c8d0 100644 --- a/packages/@n8n_io/eslint-config/local-rules.js +++ b/packages/@n8n_io/eslint-config/local-rules.js @@ -448,6 +448,36 @@ module.exports = { }; }, }, + + 'no-type-unsafe-event-emitter': { + meta: { + type: 'problem', + docs: { + description: 'Disallow extending from `EventEmitter`, which is not type-safe.', + recommended: 'error', + }, + messages: { + noExtendsEventEmitter: 'Extend from the type-safe `TypedEmitter` class instead.', + }, + }, + create(context) { + return { + ClassDeclaration(node) { + if ( + node.superClass && + node.superClass.type === 'Identifier' && + node.superClass.name === 'EventEmitter' && + node.id.name !== 'TypedEmitter' + ) { + context.report({ + node: node.superClass, + messageId: 'noExtendsEventEmitter', + }); + } + }, + }; + }, + }, }; const isJsonParseCall = (node) => diff --git a/packages/cli/.eslintrc.js b/packages/cli/.eslintrc.js index ac66574887551..bb1041607ddcd 100644 --- a/packages/cli/.eslintrc.js +++ b/packages/cli/.eslintrc.js @@ -21,6 +21,7 @@ module.exports = { rules: { 'n8n-local-rules/no-dynamic-import-template': 'error', 'n8n-local-rules/misplaced-n8n-typeorm-import': 'error', + 'n8n-local-rules/no-type-unsafe-event-emitter': 'error', complexity: 'error', // TODO: Remove this @@ -44,6 +45,12 @@ module.exports = { 'n8n-local-rules/misplaced-n8n-typeorm-import': 'off', }, }, + { + files: ['./test/**/*.ts'], + rules: { + 'n8n-local-rules/no-type-unsafe-event-emitter': 'off', + }, + }, { files: ['./src/decorators/**/*.ts'], rules: { diff --git a/packages/cli/src/eventbus/MessageEventBus/MessageEventBus.ts b/packages/cli/src/eventbus/MessageEventBus/MessageEventBus.ts index 758eeb5ae590f..eeb868798bf6b 100644 --- a/packages/cli/src/eventbus/MessageEventBus/MessageEventBus.ts +++ b/packages/cli/src/eventbus/MessageEventBus/MessageEventBus.ts @@ -52,6 +52,8 @@ export interface MessageEventBusInitializeOptions { } @Service() +// TODO: Convert to TypedEventEmitter +// eslint-disable-next-line n8n-local-rules/no-type-unsafe-event-emitter export class MessageEventBus extends EventEmitter { private isInitialized = false; From 489ce100634c3af678fb300e9a39d273042542e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 2 Aug 2024 12:02:05 +0200 Subject: [PATCH 03/98] feat(core): Support create, read, update, delete projects in Public API (#10269) --- .../v1/handlers/projects/projects.handler.ts | 65 +++ .../spec/paths/projects.projectId.yml | 43 ++ .../handlers/projects/spec/paths/projects.yml | 40 ++ .../spec/schemas/parameters/projectId.yml | 6 + .../projects/spec/schemas/project.yml | 13 + .../projects/spec/schemas/projectList.yml | 11 + packages/cli/src/PublicApi/v1/openapi.yml | 6 + .../integration/publicApi/projects.test.ts | 401 ++++++++++++++++++ .../test/integration/shared/db/projects.ts | 4 + .../cli/test/integration/shared/db/users.ts | 6 +- 10 files changed, 594 insertions(+), 1 deletion(-) create mode 100644 packages/cli/src/PublicApi/v1/handlers/projects/projects.handler.ts create mode 100644 packages/cli/src/PublicApi/v1/handlers/projects/spec/paths/projects.projectId.yml create mode 100644 packages/cli/src/PublicApi/v1/handlers/projects/spec/paths/projects.yml create mode 100644 packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/parameters/projectId.yml create mode 100644 packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/project.yml create mode 100644 packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/projectList.yml create mode 100644 packages/cli/test/integration/publicApi/projects.test.ts diff --git a/packages/cli/src/PublicApi/v1/handlers/projects/projects.handler.ts b/packages/cli/src/PublicApi/v1/handlers/projects/projects.handler.ts new file mode 100644 index 0000000000000..e61e808daf301 --- /dev/null +++ b/packages/cli/src/PublicApi/v1/handlers/projects/projects.handler.ts @@ -0,0 +1,65 @@ +import { globalScope, isLicensed, validCursor } from '../../shared/middlewares/global.middleware'; +import type { Response } from 'express'; +import type { ProjectRequest } from '@/requests'; +import type { PaginatedRequest } from '@/PublicApi/types'; +import Container from 'typedi'; +import { ProjectController } from '@/controllers/project.controller'; +import { ProjectRepository } from '@/databases/repositories/project.repository'; +import { encodeNextCursor } from '../../shared/services/pagination.service'; + +type Create = ProjectRequest.Create; +type Update = ProjectRequest.Update; +type Delete = ProjectRequest.Delete; +type GetAll = PaginatedRequest; + +export = { + createProject: [ + isLicensed('feat:projectRole:admin'), + globalScope('project:create'), + async (req: Create, res: Response) => { + const project = await Container.get(ProjectController).createProject(req); + + return res.status(201).json(project); + }, + ], + updateProject: [ + isLicensed('feat:projectRole:admin'), + globalScope('project:update'), + async (req: Update, res: Response) => { + await Container.get(ProjectController).updateProject(req); + + return res.status(204).send(); + }, + ], + deleteProject: [ + isLicensed('feat:projectRole:admin'), + globalScope('project:delete'), + async (req: Delete, res: Response) => { + await Container.get(ProjectController).deleteProject(req); + + return res.status(204).send(); + }, + ], + getProjects: [ + isLicensed('feat:projectRole:admin'), + globalScope('project:list'), + validCursor, + async (req: GetAll, res: Response) => { + const { offset = 0, limit = 100 } = req.query; + + const [projects, count] = await Container.get(ProjectRepository).findAndCount({ + skip: offset, + take: limit, + }); + + return res.json({ + data: projects, + nextCursor: encodeNextCursor({ + offset, + limit, + numberOfTotalRecords: count, + }), + }); + }, + ], +}; diff --git a/packages/cli/src/PublicApi/v1/handlers/projects/spec/paths/projects.projectId.yml b/packages/cli/src/PublicApi/v1/handlers/projects/spec/paths/projects.projectId.yml new file mode 100644 index 0000000000000..a5aab19b3d6ed --- /dev/null +++ b/packages/cli/src/PublicApi/v1/handlers/projects/spec/paths/projects.projectId.yml @@ -0,0 +1,43 @@ +delete: + x-eov-operation-id: deleteProject + x-eov-operation-handler: v1/handlers/projects/projects.handler + tags: + - Projects + summary: Delete a project + description: Delete a project from your instance. + parameters: + - $ref: '../schemas/parameters/projectId.yml' + responses: + '204': + description: Operation successful. + '401': + $ref: '../../../../shared/spec/responses/unauthorized.yml' + '403': + $ref: '../../../../shared/spec/responses/forbidden.yml' + '404': + $ref: '../../../../shared/spec/responses/notFound.yml' +put: + x-eov-operation-id: updateProject + x-eov-operation-handler: v1/handlers/projects/projects.handler + tags: + - Project + summary: Update a project + description: Update a project. + requestBody: + description: Updated project object. + content: + application/json: + schema: + $ref: '../schemas/project.yml' + required: true + responses: + '204': + description: Operation successful. + '400': + $ref: '../../../../shared/spec/responses/badRequest.yml' + '401': + $ref: '../../../../shared/spec/responses/unauthorized.yml' + '403': + $ref: '../../../../shared/spec/responses/forbidden.yml' + '404': + $ref: '../../../../shared/spec/responses/notFound.yml' diff --git a/packages/cli/src/PublicApi/v1/handlers/projects/spec/paths/projects.yml b/packages/cli/src/PublicApi/v1/handlers/projects/spec/paths/projects.yml new file mode 100644 index 0000000000000..1babd3dd6ea03 --- /dev/null +++ b/packages/cli/src/PublicApi/v1/handlers/projects/spec/paths/projects.yml @@ -0,0 +1,40 @@ +post: + x-eov-operation-id: createProject + x-eov-operation-handler: v1/handlers/projects/projects.handler + tags: + - Projects + summary: Create a project + description: Create a project in your instance. + requestBody: + description: Payload for project to create. + content: + application/json: + schema: + $ref: '../schemas/project.yml' + required: true + responses: + '201': + description: Operation successful. + '400': + $ref: '../../../../shared/spec/responses/badRequest.yml' + '401': + $ref: '../../../../shared/spec/responses/unauthorized.yml' +get: + x-eov-operation-id: getProjects + x-eov-operation-handler: v1/handlers/projects/projects.handler + tags: + - Projects + summary: Retrieve projects + description: Retrieve projects from your instance. + parameters: + - $ref: '../../../../shared/spec/parameters/limit.yml' + - $ref: '../../../../shared/spec/parameters/cursor.yml' + responses: + '200': + description: Operation successful. + content: + application/json: + schema: + $ref: '../schemas/projectList.yml' + '401': + $ref: '../../../../shared/spec/responses/unauthorized.yml' diff --git a/packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/parameters/projectId.yml b/packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/parameters/projectId.yml new file mode 100644 index 0000000000000..32961f46016f9 --- /dev/null +++ b/packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/parameters/projectId.yml @@ -0,0 +1,6 @@ +name: projectId +in: path +description: The ID of the project. +required: true +schema: + type: string diff --git a/packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/project.yml b/packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/project.yml new file mode 100644 index 0000000000000..7a4d2ec432bdd --- /dev/null +++ b/packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/project.yml @@ -0,0 +1,13 @@ +type: object +additionalProperties: false +required: + - name +properties: + id: + type: string + readOnly: true + name: + type: string + type: + type: string + readOnly: true diff --git a/packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/projectList.yml b/packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/projectList.yml new file mode 100644 index 0000000000000..7d88be72fb008 --- /dev/null +++ b/packages/cli/src/PublicApi/v1/handlers/projects/spec/schemas/projectList.yml @@ -0,0 +1,11 @@ +type: object +properties: + data: + type: array + items: + $ref: './project.yml' + nextCursor: + type: string + description: Paginate through projects by setting the cursor parameter to a nextCursor attribute returned by a previous request. Default value fetches the first "page" of the collection. + nullable: true + example: MTIzZTQ1NjctZTg5Yi0xMmQzLWE0NTYtNDI2NjE0MTc0MDA diff --git a/packages/cli/src/PublicApi/v1/openapi.yml b/packages/cli/src/PublicApi/v1/openapi.yml index fd3b286a17fb5..e49bfb71c5668 100644 --- a/packages/cli/src/PublicApi/v1/openapi.yml +++ b/packages/cli/src/PublicApi/v1/openapi.yml @@ -32,6 +32,8 @@ tags: description: Operations about source control - name: Variables description: Operations about variables + - name: Projects + description: Operations about projects paths: /audit: @@ -72,6 +74,10 @@ paths: $ref: './handlers/variables/spec/paths/variables.yml' /variables/{id}: $ref: './handlers/variables/spec/paths/variables.id.yml' + /projects: + $ref: './handlers/projects/spec/paths/projects.yml' + /projects/{projectId}: + $ref: './handlers/projects/spec/paths/projects.projectId.yml' components: schemas: $ref: './shared/spec/schemas/_index.yml' diff --git a/packages/cli/test/integration/publicApi/projects.test.ts b/packages/cli/test/integration/publicApi/projects.test.ts new file mode 100644 index 0000000000000..0554fd6f4116f --- /dev/null +++ b/packages/cli/test/integration/publicApi/projects.test.ts @@ -0,0 +1,401 @@ +import { setupTestServer } from '@test-integration/utils'; +import { createMember, createOwner } from '@test-integration/db/users'; +import * as testDb from '../shared/testDb'; +import { FeatureNotLicensedError } from '@/errors/feature-not-licensed.error'; +import { createTeamProject, getProjectByNameOrFail } from '@test-integration/db/projects'; +import { mockInstance } from '@test/mocking'; +import { Telemetry } from '@/telemetry'; + +describe('Projects in Public API', () => { + const testServer = setupTestServer({ endpointGroups: ['publicApi'] }); + mockInstance(Telemetry); + + beforeAll(async () => { + await testDb.init(); + }); + + beforeEach(async () => { + await testDb.truncate(['Project', 'User']); + }); + + describe('GET /projects', () => { + it('if licensed, should return all projects with pagination', async () => { + /** + * Arrange + */ + testServer.license.setQuota('quota:maxTeamProjects', -1); + testServer.license.enable('feat:projectRole:admin'); + const owner = await createOwner({ withApiKey: true }); + const projects = await Promise.all([ + createTeamProject(), + createTeamProject(), + createTeamProject(), + ]); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).get('/projects'); + + /** + * Assert + */ + expect(response.status).toBe(200); + expect(response.body).toHaveProperty('data'); + expect(response.body).toHaveProperty('nextCursor'); + expect(Array.isArray(response.body.data)).toBe(true); + expect(response.body.data.length).toBe(projects.length + 1); // +1 for the owner's personal project + + projects.forEach(({ id, name }) => { + expect(response.body.data).toContainEqual(expect.objectContaining({ id, name })); + }); + }); + + it('if not authenticated, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: false }); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).get('/projects'); + + /** + * Assert + */ + expect(response.status).toBe(401); + expect(response.body).toHaveProperty('message', "'X-N8N-API-KEY' header required"); + }); + + it('if not licensed, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: true }); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).get('/projects'); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty( + 'message', + new FeatureNotLicensedError('feat:projectRole:admin').message, + ); + }); + + it('if missing scope, should reject', async () => { + /** + * Arrange + */ + testServer.license.setQuota('quota:maxTeamProjects', -1); + testServer.license.enable('feat:projectRole:admin'); + const owner = await createMember({ withApiKey: true }); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).get('/projects'); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty('message', 'Forbidden'); + }); + }); + + describe('POST /projects', () => { + it('if licensed, should create a new project', async () => { + /** + * Arrange + */ + testServer.license.setQuota('quota:maxTeamProjects', -1); + testServer.license.enable('feat:projectRole:admin'); + const owner = await createOwner({ withApiKey: true }); + const projectPayload = { name: 'some-project' }; + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(owner) + .post('/projects') + .send(projectPayload); + + /** + * Assert + */ + expect(response.status).toBe(201); + expect(response.body).toEqual({ + name: 'some-project', + type: 'team', + id: expect.any(String), + createdAt: expect.any(String), + updatedAt: expect.any(String), + role: 'project:admin', + scopes: expect.any(Array), + }); + await expect(getProjectByNameOrFail(projectPayload.name)).resolves.not.toThrow(); + }); + + it('if not authenticated, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: false }); + const projectPayload = { name: 'some-project' }; + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(owner) + .post('/projects') + .send(projectPayload); + + /** + * Assert + */ + expect(response.status).toBe(401); + expect(response.body).toHaveProperty('message', "'X-N8N-API-KEY' header required"); + }); + + it('if not licensed, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: true }); + const projectPayload = { name: 'some-project' }; + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(owner) + .post('/projects') + .send(projectPayload); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty( + 'message', + new FeatureNotLicensedError('feat:projectRole:admin').message, + ); + }); + + it('if missing scope, should reject', async () => { + /** + * Arrange + */ + testServer.license.setQuota('quota:maxTeamProjects', -1); + testServer.license.enable('feat:projectRole:admin'); + const member = await createMember({ withApiKey: true }); + const projectPayload = { name: 'some-project' }; + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(member) + .post('/projects') + .send(projectPayload); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty('message', 'Forbidden'); + }); + }); + + describe('DELETE /projects/:id', () => { + it('if licensed, should delete a project', async () => { + /** + * Arrange + */ + testServer.license.setQuota('quota:maxTeamProjects', -1); + testServer.license.enable('feat:projectRole:admin'); + const owner = await createOwner({ withApiKey: true }); + const project = await createTeamProject(); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).delete(`/projects/${project.id}`); + + /** + * Assert + */ + expect(response.status).toBe(204); + await expect(getProjectByNameOrFail(project.id)).rejects.toThrow(); + }); + + it('if not authenticated, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: false }); + const project = await createTeamProject(); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).delete(`/projects/${project.id}`); + + /** + * Assert + */ + expect(response.status).toBe(401); + expect(response.body).toHaveProperty('message', "'X-N8N-API-KEY' header required"); + }); + + it('if not licensed, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: true }); + const project = await createTeamProject(); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).delete(`/projects/${project.id}`); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty( + 'message', + new FeatureNotLicensedError('feat:projectRole:admin').message, + ); + }); + + it('if missing scope, should reject', async () => { + /** + * Arrange + */ + testServer.license.setQuota('quota:maxTeamProjects', -1); + testServer.license.enable('feat:projectRole:admin'); + const member = await createMember({ withApiKey: true }); + const project = await createTeamProject(); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(member).delete(`/projects/${project.id}`); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty('message', 'Forbidden'); + }); + }); + + describe('PUT /projects/:id', () => { + it('if licensed, should update a project', async () => { + /** + * Arrange + */ + testServer.license.setQuota('quota:maxTeamProjects', -1); + testServer.license.enable('feat:projectRole:admin'); + const owner = await createOwner({ withApiKey: true }); + const project = await createTeamProject('old-name'); + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(owner) + .put(`/projects/${project.id}`) + .send({ name: 'new-name' }); + + /** + * Assert + */ + expect(response.status).toBe(204); + await expect(getProjectByNameOrFail('new-name')).resolves.not.toThrow(); + }); + + it('if not authenticated, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: false }); + const project = await createTeamProject(); + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(owner) + .put(`/projects/${project.id}`) + .send({ name: 'new-name' }); + + /** + * Assert + */ + expect(response.status).toBe(401); + expect(response.body).toHaveProperty('message', "'X-N8N-API-KEY' header required"); + }); + + it('if not licensed, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: true }); + const project = await createTeamProject(); + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(owner) + .put(`/projects/${project.id}`) + .send({ name: 'new-name' }); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty( + 'message', + new FeatureNotLicensedError('feat:projectRole:admin').message, + ); + }); + + it('if missing scope, should reject', async () => { + /** + * Arrange + */ + testServer.license.setQuota('quota:maxTeamProjects', -1); + testServer.license.enable('feat:projectRole:admin'); + const member = await createMember({ withApiKey: true }); + const project = await createTeamProject(); + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(member) + .put(`/projects/${project.id}`) + .send({ name: 'new-name' }); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty('message', 'Forbidden'); + }); + }); +}); diff --git a/packages/cli/test/integration/shared/db/projects.ts b/packages/cli/test/integration/shared/db/projects.ts index 60548575b362b..3de7de5bb95f9 100644 --- a/packages/cli/test/integration/shared/db/projects.ts +++ b/packages/cli/test/integration/shared/db/projects.ts @@ -34,6 +34,10 @@ export const linkUserToProject = async (user: User, project: Project, role: Proj ); }; +export async function getProjectByNameOrFail(name: string) { + return await Container.get(ProjectRepository).findOneOrFail({ where: { name } }); +} + export const getPersonalProject = async (user: User): Promise => { return await Container.get(ProjectRepository).findOneOrFail({ where: { diff --git a/packages/cli/test/integration/shared/db/users.ts b/packages/cli/test/integration/shared/db/users.ts index f125f5ccded4f..98626bc549d9e 100644 --- a/packages/cli/test/integration/shared/db/users.ts +++ b/packages/cli/test/integration/shared/db/users.ts @@ -86,7 +86,11 @@ export async function createOwner({ withApiKey } = { withApiKey: false }) { return await createUser({ role: 'global:owner' }); } -export async function createMember() { +export async function createMember({ withApiKey } = { withApiKey: false }) { + if (withApiKey) { + return await addApiKey(await createUser({ role: 'global:member' })); + } + return await createUser({ role: 'global:member' }); } From 07d7b247f02a9d7185beca7817deb779a3d665dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 2 Aug 2024 12:02:38 +0200 Subject: [PATCH 04/98] feat(core): Allow transferring credentials in Public API (#10259) --- packages/cli/src/PublicApi/types.ts | 6 +++ .../credentials/credentials.handler.ts | 16 ++++++ .../spec/paths/credentials.id.transfer.yml | 31 ++++++++++++ .../spec/schemas/parameters/credentialId.yml | 6 +++ packages/cli/src/PublicApi/v1/openapi.yml | 2 + .../integration/publicApi/credentials.test.ts | 50 ++++++++++++++++++- .../integration/publicApi/workflows.test.ts | 2 +- .../test/integration/shared/db/credentials.ts | 19 +++++-- 8 files changed, 127 insertions(+), 5 deletions(-) create mode 100644 packages/cli/src/PublicApi/v1/handlers/credentials/spec/paths/credentials.id.transfer.yml create mode 100644 packages/cli/src/PublicApi/v1/handlers/credentials/spec/schemas/parameters/credentialId.yml diff --git a/packages/cli/src/PublicApi/types.ts b/packages/cli/src/PublicApi/types.ts index f58b521e04a88..831dd03ebfb9c 100644 --- a/packages/cli/src/PublicApi/types.ts +++ b/packages/cli/src/PublicApi/types.ts @@ -142,6 +142,12 @@ export declare namespace CredentialRequest { >; type Delete = AuthenticatedRequest<{ id: string }, {}, {}, Record>; + + type Transfer = AuthenticatedRequest< + { workflowId: string }, + {}, + { destinationProjectId: string } + >; } export type OperationID = 'getUsers' | 'getUser'; diff --git a/packages/cli/src/PublicApi/v1/handlers/credentials/credentials.handler.ts b/packages/cli/src/PublicApi/v1/handlers/credentials/credentials.handler.ts index 4da7635831340..57b6bd66f6841 100644 --- a/packages/cli/src/PublicApi/v1/handlers/credentials/credentials.handler.ts +++ b/packages/cli/src/PublicApi/v1/handlers/credentials/credentials.handler.ts @@ -19,6 +19,8 @@ import { toJsonSchema, } from './credentials.service'; import { Container } from 'typedi'; +import { z } from 'zod'; +import { EnterpriseCredentialsService } from '@/credentials/credentials.service.ee'; export = { createCredential: [ @@ -44,6 +46,20 @@ export = { } }, ], + transferCredential: [ + projectScope('credential:move', 'credential'), + async (req: CredentialRequest.Transfer, res: express.Response) => { + const body = z.object({ destinationProjectId: z.string() }).parse(req.body); + + await Container.get(EnterpriseCredentialsService).transferOne( + req.user, + req.params.workflowId, + body.destinationProjectId, + ); + + res.status(204).send(); + }, + ], deleteCredential: [ projectScope('credential:delete', 'credential'), async ( diff --git a/packages/cli/src/PublicApi/v1/handlers/credentials/spec/paths/credentials.id.transfer.yml b/packages/cli/src/PublicApi/v1/handlers/credentials/spec/paths/credentials.id.transfer.yml new file mode 100644 index 0000000000000..a9e9c5cf7c229 --- /dev/null +++ b/packages/cli/src/PublicApi/v1/handlers/credentials/spec/paths/credentials.id.transfer.yml @@ -0,0 +1,31 @@ +put: + x-eov-operation-id: transferCredential + x-eov-operation-handler: v1/handlers/credentials/credentials.handler + tags: + - Workflow + summary: Transfer a credential to another project. + description: Transfer a credential to another project. + parameters: + - $ref: '../schemas/parameters/credentialId.yml' + requestBody: + description: Destination project for the credential transfer. + content: + application/json: + schema: + type: object + properties: + destinationProjectId: + type: string + description: The ID of the project to transfer the credential to. + required: + - destinationProjectId + required: true + responses: + '200': + description: Operation successful. + '400': + $ref: '../../../../shared/spec/responses/badRequest.yml' + '401': + $ref: '../../../../shared/spec/responses/unauthorized.yml' + '404': + $ref: '../../../../shared/spec/responses/notFound.yml' diff --git a/packages/cli/src/PublicApi/v1/handlers/credentials/spec/schemas/parameters/credentialId.yml b/packages/cli/src/PublicApi/v1/handlers/credentials/spec/schemas/parameters/credentialId.yml new file mode 100644 index 0000000000000..f16676ce0b96c --- /dev/null +++ b/packages/cli/src/PublicApi/v1/handlers/credentials/spec/schemas/parameters/credentialId.yml @@ -0,0 +1,6 @@ +name: id +in: path +description: The ID of the credential. +required: true +schema: + type: string diff --git a/packages/cli/src/PublicApi/v1/openapi.yml b/packages/cli/src/PublicApi/v1/openapi.yml index e49bfb71c5668..1d4e14079f541 100644 --- a/packages/cli/src/PublicApi/v1/openapi.yml +++ b/packages/cli/src/PublicApi/v1/openapi.yml @@ -62,6 +62,8 @@ paths: $ref: './handlers/workflows/spec/paths/workflows.id.deactivate.yml' /workflows/{id}/transfer: $ref: './handlers/workflows/spec/paths/workflows.id.transfer.yml' + /credentials/{id}/transfer: + $ref: './handlers/credentials/spec/paths/credentials.id.transfer.yml' /workflows/{id}/tags: $ref: './handlers/workflows/spec/paths/workflows.id.tags.yml' /users: diff --git a/packages/cli/test/integration/publicApi/credentials.test.ts b/packages/cli/test/integration/publicApi/credentials.test.ts index dfa6c44dd4037..b5ed2bfb31d82 100644 --- a/packages/cli/test/integration/publicApi/credentials.test.ts +++ b/packages/cli/test/integration/publicApi/credentials.test.ts @@ -9,9 +9,10 @@ import { randomApiKey, randomName } from '../shared/random'; import * as utils from '../shared/utils/'; import type { CredentialPayload, SaveCredentialFunction } from '../shared/types'; import * as testDb from '../shared/testDb'; -import { affixRoleToSaveCredential } from '../shared/db/credentials'; +import { affixRoleToSaveCredential, createCredentials } from '../shared/db/credentials'; import { addApiKey, createUser, createUserShell } from '../shared/db/users'; import type { SuperAgentTest } from '../shared/types'; +import { createTeamProject } from '@test-integration/db/projects'; let owner: User; let member: User; @@ -256,6 +257,53 @@ describe('GET /credentials/schema/:credentialType', () => { }); }); +describe('PUT /credentials/:id/transfer', () => { + test('should transfer credential to project', async () => { + /** + * Arrange + */ + const [firstProject, secondProject] = await Promise.all([ + createTeamProject('first-project', owner), + createTeamProject('second-project', owner), + ]); + + const credentials = await createCredentials( + { name: 'Test', type: 'test', data: '' }, + firstProject, + ); + + /** + * Act + */ + const response = await authOwnerAgent.put(`/credentials/${credentials.id}/transfer`).send({ + destinationProjectId: secondProject.id, + }); + + /** + * Assert + */ + expect(response.statusCode).toBe(204); + }); + + test('if no destination project, should reject', async () => { + /** + * Arrange + */ + const project = await createTeamProject('first-project', member); + const credentials = await createCredentials({ name: 'Test', type: 'test', data: '' }, project); + + /** + * Act + */ + const response = await authOwnerAgent.put(`/credentials/${credentials.id}/transfer`).send({}); + + /** + * Assert + */ + expect(response.statusCode).toBe(400); + }); +}); + const credentialPayload = (): CredentialPayload => ({ name: randomName(), type: 'githubApi', diff --git a/packages/cli/test/integration/publicApi/workflows.test.ts b/packages/cli/test/integration/publicApi/workflows.test.ts index 855e69e3df12a..1e292af64d92f 100644 --- a/packages/cli/test/integration/publicApi/workflows.test.ts +++ b/packages/cli/test/integration/publicApi/workflows.test.ts @@ -1493,7 +1493,7 @@ describe('PUT /workflows/:id/transfer', () => { * Arrange */ const firstProject = await createTeamProject('first-project', member); - const secondProject = await createTeamProject('secon-project', member); + const secondProject = await createTeamProject('second-project', member); const workflow = await createWorkflow({}, firstProject); /** diff --git a/packages/cli/test/integration/shared/db/credentials.ts b/packages/cli/test/integration/shared/db/credentials.ts index 046d27db261a0..588fee6b51196 100644 --- a/packages/cli/test/integration/shared/db/credentials.ts +++ b/packages/cli/test/integration/shared/db/credentials.ts @@ -38,11 +38,24 @@ export async function createManyCredentials( ); } -export async function createCredentials(attributes: Partial = emptyAttributes) { +export async function createCredentials( + attributes: Partial = emptyAttributes, + project?: Project, +) { const credentialsRepository = Container.get(CredentialsRepository); - const entity = credentialsRepository.create(attributes); + const credentials = await credentialsRepository.save(credentialsRepository.create(attributes)); + + if (project) { + await Container.get(SharedCredentialsRepository).save( + Container.get(SharedCredentialsRepository).create({ + project, + credentials, + role: 'credential:owner', + }), + ); + } - return await credentialsRepository.save(entity); + return credentials; } /** From a5339166285b896a8ac2723723ca9544e0d2e6cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 2 Aug 2024 12:05:06 +0200 Subject: [PATCH 05/98] refactor(core): Decouple post workflow execute event from internal hooks (no-changelog) (#10280) --- packages/cli/src/InternalHooks.ts | 158 +----------------- .../cli/src/WorkflowExecuteAdditionalData.ts | 15 +- packages/cli/src/WorkflowRunner.ts | 12 +- .../audit-event-relay.service.test.ts | 34 ++-- .../src/eventbus/audit-event-relay.service.ts | 16 +- packages/cli/src/eventbus/event.types.ts | 6 +- .../executions/execution-recovery.service.ts | 16 +- .../telemetry-event-relay.service.ts | 141 ++++++++++++++++ 8 files changed, 179 insertions(+), 219 deletions(-) diff --git a/packages/cli/src/InternalHooks.ts b/packages/cli/src/InternalHooks.ts index fda2c3f21dfc7..46637106b7a47 100644 --- a/packages/cli/src/InternalHooks.ts +++ b/packages/cli/src/InternalHooks.ts @@ -1,23 +1,10 @@ import { Service } from 'typedi'; import { snakeCase } from 'change-case'; -import { get as pslGet } from 'psl'; -import type { - ExecutionStatus, - INodesGraphResult, - IRun, - ITelemetryTrackProperties, - IWorkflowBase, -} from 'n8n-workflow'; -import { TelemetryHelpers } from 'n8n-workflow'; - -import { N8N_VERSION } from '@/constants'; +import type { ITelemetryTrackProperties } from 'n8n-workflow'; import type { AuthProviderType } from '@db/entities/AuthIdentity'; import type { User } from '@db/entities/User'; -import { SharedWorkflowRepository } from '@db/repositories/sharedWorkflow.repository'; -import { determineFinalExecutionStatus } from '@/executionLifecycleHooks/shared/sharedHookFunctions'; -import type { ITelemetryUserDeletionData, IExecutionTrackProperties } from '@/Interfaces'; +import type { ITelemetryUserDeletionData } from '@/Interfaces'; import { WorkflowStatisticsService } from '@/services/workflow-statistics.service'; -import { NodeTypes } from '@/NodeTypes'; import { Telemetry } from '@/telemetry'; import { MessageEventBus } from './eventbus/MessageEventBus/MessageEventBus'; @@ -30,8 +17,6 @@ import { MessageEventBus } from './eventbus/MessageEventBus/MessageEventBus'; export class InternalHooks { constructor( private readonly telemetry: Telemetry, - private readonly nodeTypes: NodeTypes, - private readonly sharedWorkflowRepository: SharedWorkflowRepository, workflowStatisticsService: WorkflowStatisticsService, // Can't use @ts-expect-error because only dev time tsconfig considers this as an error, but not build time // eslint-disable-next-line @typescript-eslint/ban-ts-comment @@ -64,145 +49,6 @@ export class InternalHooks { this.telemetry.track('User responded to personalization questions', personalizationSurveyData); } - // eslint-disable-next-line complexity - async onWorkflowPostExecute( - _executionId: string, - workflow: IWorkflowBase, - runData?: IRun, - userId?: string, - ) { - if (!workflow.id) { - return; - } - - if (runData?.status === 'waiting') { - // No need to send telemetry or logs when the workflow hasn't finished yet. - return; - } - - const telemetryProperties: IExecutionTrackProperties = { - workflow_id: workflow.id, - is_manual: false, - version_cli: N8N_VERSION, - success: false, - }; - - if (userId) { - telemetryProperties.user_id = userId; - } - - if (runData?.data.resultData.error?.message?.includes('canceled')) { - runData.status = 'canceled'; - } - - telemetryProperties.success = !!runData?.finished; - - // const executionStatus: ExecutionStatus = runData?.status ?? 'unknown'; - const executionStatus: ExecutionStatus = runData - ? determineFinalExecutionStatus(runData) - : 'unknown'; - - if (runData !== undefined) { - telemetryProperties.execution_mode = runData.mode; - telemetryProperties.is_manual = runData.mode === 'manual'; - - let nodeGraphResult: INodesGraphResult | null = null; - - if (!telemetryProperties.success && runData?.data.resultData.error) { - telemetryProperties.error_message = runData?.data.resultData.error.message; - let errorNodeName = - 'node' in runData?.data.resultData.error - ? runData?.data.resultData.error.node?.name - : undefined; - telemetryProperties.error_node_type = - 'node' in runData?.data.resultData.error - ? runData?.data.resultData.error.node?.type - : undefined; - - if (runData.data.resultData.lastNodeExecuted) { - const lastNode = TelemetryHelpers.getNodeTypeForName( - workflow, - runData.data.resultData.lastNodeExecuted, - ); - - if (lastNode !== undefined) { - telemetryProperties.error_node_type = lastNode.type; - errorNodeName = lastNode.name; - } - } - - if (telemetryProperties.is_manual) { - nodeGraphResult = TelemetryHelpers.generateNodesGraph(workflow, this.nodeTypes); - telemetryProperties.node_graph = nodeGraphResult.nodeGraph; - telemetryProperties.node_graph_string = JSON.stringify(nodeGraphResult.nodeGraph); - - if (errorNodeName) { - telemetryProperties.error_node_id = nodeGraphResult.nameIndices[errorNodeName]; - } - } - } - - if (telemetryProperties.is_manual) { - if (!nodeGraphResult) { - nodeGraphResult = TelemetryHelpers.generateNodesGraph(workflow, this.nodeTypes); - } - - let userRole: 'owner' | 'sharee' | undefined = undefined; - if (userId) { - const role = await this.sharedWorkflowRepository.findSharingRole(userId, workflow.id); - if (role) { - userRole = role === 'workflow:owner' ? 'owner' : 'sharee'; - } - } - - const manualExecEventProperties: ITelemetryTrackProperties = { - user_id: userId, - workflow_id: workflow.id, - status: executionStatus, - executionStatus: runData?.status ?? 'unknown', - error_message: telemetryProperties.error_message as string, - error_node_type: telemetryProperties.error_node_type, - node_graph_string: telemetryProperties.node_graph_string as string, - error_node_id: telemetryProperties.error_node_id as string, - webhook_domain: null, - sharing_role: userRole, - }; - - if (!manualExecEventProperties.node_graph_string) { - nodeGraphResult = TelemetryHelpers.generateNodesGraph(workflow, this.nodeTypes); - manualExecEventProperties.node_graph_string = JSON.stringify(nodeGraphResult.nodeGraph); - } - - if (runData.data.startData?.destinationNode) { - const telemetryPayload = { - ...manualExecEventProperties, - node_type: TelemetryHelpers.getNodeTypeForName( - workflow, - runData.data.startData?.destinationNode, - )?.type, - node_id: nodeGraphResult.nameIndices[runData.data.startData?.destinationNode], - }; - - this.telemetry.track('Manual node exec finished', telemetryPayload); - } else { - nodeGraphResult.webhookNodeNames.forEach((name: string) => { - const execJson = runData.data.resultData.runData[name]?.[0]?.data?.main?.[0]?.[0] - ?.json as { headers?: { origin?: string } }; - if (execJson?.headers?.origin && execJson.headers.origin !== '') { - manualExecEventProperties.webhook_domain = pslGet( - execJson.headers.origin.replace(/^https?:\/\//, ''), - ); - } - }); - - this.telemetry.track('Manual workflow exec finished', manualExecEventProperties); - } - } - } - - this.telemetry.trackWorkflowExecution(telemetryProperties); - } - onWorkflowSharingUpdate(workflowId: string, userId: string, userList: string[]) { const properties: ITelemetryTrackProperties = { workflow_id: workflowId, diff --git a/packages/cli/src/WorkflowExecuteAdditionalData.ts b/packages/cli/src/WorkflowExecuteAdditionalData.ts index f4acd61b6aa55..9538ec91988fd 100644 --- a/packages/cli/src/WorkflowExecuteAdditionalData.ts +++ b/packages/cli/src/WorkflowExecuteAdditionalData.ts @@ -52,7 +52,6 @@ import { Push } from '@/push'; import * as WorkflowHelpers from '@/WorkflowHelpers'; import { findSubworkflowStart, isWorkflowIdValid } from '@/utils'; import { PermissionChecker } from './UserManagement/PermissionChecker'; -import { InternalHooks } from '@/InternalHooks'; import { ExecutionRepository } from '@db/repositories/execution.repository'; import { WorkflowStatisticsService } from '@/services/workflow-statistics.service'; import { SecretsHelper } from './SecretsHelpers'; @@ -548,7 +547,6 @@ function hookFunctionsSave(): IWorkflowExecuteHooks { */ function hookFunctionsSaveWorker(): IWorkflowExecuteHooks { const logger = Container.get(Logger); - const internalHooks = Container.get(InternalHooks); const workflowStatisticsService = Container.get(WorkflowStatisticsService); const eventService = Container.get(EventService); return { @@ -644,13 +642,9 @@ function hookFunctionsSaveWorker(): IWorkflowExecuteHooks { async function (this: WorkflowHooks, runData: IRun): Promise { const { executionId, workflowData: workflow } = this; - void internalHooks.onWorkflowPostExecute(executionId, workflow, runData); eventService.emit('workflow-post-execute', { - workflowId: workflow.id, - workflowName: workflow.name, + workflow, executionId, - success: runData.status === 'success', - isManual: runData.mode === 'manual', runData, }); }, @@ -787,7 +781,6 @@ async function executeWorkflow( parentCallbackManager?: CallbackManager; }, ): Promise | IWorkflowExecuteProcess> { - const internalHooks = Container.get(InternalHooks); const externalHooks = Container.get(ExternalHooks); await externalHooks.init(); @@ -933,13 +926,9 @@ async function executeWorkflow( await externalHooks.run('workflow.postExecute', [data, workflowData, executionId]); - void internalHooks.onWorkflowPostExecute(executionId, workflowData, data, additionalData.userId); eventService.emit('workflow-post-execute', { - workflowId: workflowData.id, - workflowName: workflowData.name, + workflow: workflowData, executionId, - success: data.status === 'success', - isManual: data.mode === 'manual', userId: additionalData.userId, runData: data, }); diff --git a/packages/cli/src/WorkflowRunner.ts b/packages/cli/src/WorkflowRunner.ts index 3318dd283cd60..f51a44cc4d2a3 100644 --- a/packages/cli/src/WorkflowRunner.ts +++ b/packages/cli/src/WorkflowRunner.ts @@ -34,7 +34,6 @@ import * as WorkflowHelpers from '@/WorkflowHelpers'; import * as WorkflowExecuteAdditionalData from '@/WorkflowExecuteAdditionalData'; import { generateFailedExecutionFromError } from '@/WorkflowHelpers'; import { PermissionChecker } from '@/UserManagement/PermissionChecker'; -import { InternalHooks } from '@/InternalHooks'; import { Logger } from '@/Logger'; import { WorkflowStaticDataService } from '@/workflows/workflowStaticData.service'; import { EventService } from './eventbus/event.service'; @@ -160,18 +159,9 @@ export class WorkflowRunner { const postExecutePromise = this.activeExecutions.getPostExecutePromise(executionId); postExecutePromise .then(async (executionData) => { - void Container.get(InternalHooks).onWorkflowPostExecute( - executionId, - data.workflowData, - executionData, - data.userId, - ); this.eventService.emit('workflow-post-execute', { - workflowId: data.workflowData.id, - workflowName: data.workflowData.name, + workflow: data.workflowData, executionId, - success: executionData?.status === 'success', - isManual: data.executionMode === 'manual', userId: data.userId, runData: executionData, }); diff --git a/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts b/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts index 52b86b58e1b6b..44277f1de4d0d 100644 --- a/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts +++ b/packages/cli/src/eventbus/__tests__/audit-event-relay.service.test.ts @@ -141,27 +141,31 @@ describe('AuditEventRelay', () => { it('should log on `workflow-post-execute` for successful execution', () => { const payload = mock({ executionId: 'some-id', - success: true, userId: 'some-id', - workflowId: 'some-id', - isManual: true, - workflowName: 'some-name', - metadata: {}, - runData: mock({ data: { resultData: {} } }), + workflow: mock({ id: 'some-id', name: 'some-name' }), + runData: mock({ status: 'success', mode: 'manual', data: { resultData: {} } }), }); eventService.emit('workflow-post-execute', payload); - const { runData: _, ...rest } = payload; + const { runData: _, workflow: __, ...rest } = payload; expect(eventBus.sendWorkflowEvent).toHaveBeenCalledWith({ eventName: 'n8n.workflow.success', - payload: rest, + payload: { + ...rest, + success: true, + isManual: true, + workflowName: 'some-name', + workflowId: 'some-id', + }, }); }); - it('should handle `workflow-post-execute` event for unsuccessful execution', () => { + it('should log on `workflow-post-execute` event for unsuccessful execution', () => { const runData = mock({ + status: 'error', + mode: 'manual', data: { resultData: { lastNodeExecuted: 'some-node', @@ -177,23 +181,23 @@ describe('AuditEventRelay', () => { const event = { executionId: 'some-id', - success: false, userId: 'some-id', - workflowId: 'some-id', - isManual: true, - workflowName: 'some-name', - metadata: {}, + workflow: mock({ id: 'some-id', name: 'some-name' }), runData, }; eventService.emit('workflow-post-execute', event); - const { runData: _, ...rest } = event; + const { runData: _, workflow: __, ...rest } = event; expect(eventBus.sendWorkflowEvent).toHaveBeenCalledWith({ eventName: 'n8n.workflow.failed', payload: { ...rest, + success: false, + isManual: true, + workflowName: 'some-name', + workflowId: 'some-id', lastNodeExecuted: 'some-node', errorNodeType: 'some-type', errorMessage: 'some-message', diff --git a/packages/cli/src/eventbus/audit-event-relay.service.ts b/packages/cli/src/eventbus/audit-event-relay.service.ts index f8a95a3ebf359..dcefeac0bd317 100644 --- a/packages/cli/src/eventbus/audit-event-relay.service.ts +++ b/packages/cli/src/eventbus/audit-event-relay.service.ts @@ -122,12 +122,20 @@ export class AuditEventRelay { } private workflowPostExecute(event: Event['workflow-post-execute']) { - const { runData, ...rest } = event; + const { runData, workflow, ...rest } = event; - if (event.success) { + const payload = { + ...rest, + success: runData?.status === 'success', + isManual: runData?.mode === 'manual', + workflowId: workflow.id, + workflowName: workflow.name, + }; + + if (payload.success) { void this.eventBus.sendWorkflowEvent({ eventName: 'n8n.workflow.success', - payload: rest, + payload, }); return; @@ -136,7 +144,7 @@ export class AuditEventRelay { void this.eventBus.sendWorkflowEvent({ eventName: 'n8n.workflow.failed', payload: { - ...rest, + ...payload, lastNodeExecuted: runData?.data.resultData.lastNodeExecuted, errorNodeType: runData?.data.resultData.error && 'node' in runData?.data.resultData.error diff --git a/packages/cli/src/eventbus/event.types.ts b/packages/cli/src/eventbus/event.types.ts index b62d3bc141031..bcca98b919637 100644 --- a/packages/cli/src/eventbus/event.types.ts +++ b/packages/cli/src/eventbus/event.types.ts @@ -44,12 +44,8 @@ export type Event = { 'workflow-post-execute': { executionId: string; - success: boolean; userId?: string; - workflowId: string; - isManual: boolean; - workflowName: string; - metadata?: Record; + workflow: IWorkflowBase; runData?: IRun; }; diff --git a/packages/cli/src/executions/execution-recovery.service.ts b/packages/cli/src/executions/execution-recovery.service.ts index b72fc490dddbb..615c65ea6ef5f 100644 --- a/packages/cli/src/executions/execution-recovery.service.ts +++ b/packages/cli/src/executions/execution-recovery.service.ts @@ -3,7 +3,6 @@ import { Push } from '@/push'; import { jsonStringify, sleep } from 'n8n-workflow'; import { ExecutionRepository } from '@db/repositories/execution.repository'; import { getWorkflowHooksMain } from '@/WorkflowExecuteAdditionalData'; // @TODO: Dependency cycle -import { InternalHooks } from '@/InternalHooks'; // @TODO: Dependency cycle if injected import type { DateTime } from 'luxon'; import type { IRun, ITaskData } from 'n8n-workflow'; import type { EventMessageTypes } from '../eventbus/EventMessageClasses'; @@ -280,22 +279,9 @@ export class ExecutionRecoveryService { private async runHooks(execution: IExecutionResponse) { execution.data ??= { resultData: { runData: {} } }; - await Container.get(InternalHooks).onWorkflowPostExecute(execution.id, execution.workflowData, { - data: execution.data, - finished: false, - mode: execution.mode, - waitTill: execution.waitTill, - startedAt: execution.startedAt, - stoppedAt: execution.stoppedAt, - status: execution.status, - }); - this.eventService.emit('workflow-post-execute', { - workflowId: execution.workflowData.id, - workflowName: execution.workflowData.name, + workflow: execution.workflowData, executionId: execution.id, - success: execution.status === 'success', - isManual: execution.mode === 'manual', runData: execution, }); diff --git a/packages/cli/src/telemetry/telemetry-event-relay.service.ts b/packages/cli/src/telemetry/telemetry-event-relay.service.ts index 9077abdf9ce23..f92b38987b1e3 100644 --- a/packages/cli/src/telemetry/telemetry-event-relay.service.ts +++ b/packages/cli/src/telemetry/telemetry-event-relay.service.ts @@ -8,10 +8,14 @@ import { License } from '@/License'; import { GlobalConfig } from '@n8n/config'; import { N8N_VERSION } from '@/constants'; import { WorkflowRepository } from '@/databases/repositories/workflow.repository'; +import type { ExecutionStatus, INodesGraphResult, ITelemetryTrackProperties } from 'n8n-workflow'; +import { get as pslGet } from 'psl'; import { TelemetryHelpers } from 'n8n-workflow'; import { NodeTypes } from '@/NodeTypes'; import { SharedWorkflowRepository } from '@/databases/repositories/sharedWorkflow.repository'; import { ProjectRelationRepository } from '@/databases/repositories/projectRelation.repository'; +import type { IExecutionTrackProperties } from '@/Interfaces'; +import { determineFinalExecutionStatus } from '@/executionLifecycleHooks/shared/sharedHookFunctions'; @Service() export class TelemetryEventRelay { @@ -118,6 +122,9 @@ export class TelemetryEventRelay { this.eventService.on('workflow-saved', async (event) => { await this.workflowSaved(event); }); + this.eventService.on('workflow-post-execute', async (event) => { + await this.workflowPostExecute(event); + }); } private teamProjectUpdated({ userId, role, members, projectId }: Event['team-project-updated']) { @@ -584,4 +591,138 @@ export class TelemetryEventRelay { earliest_workflow_created: firstWorkflow?.createdAt, }); } + + // eslint-disable-next-line complexity + private async workflowPostExecute({ workflow, runData, userId }: Event['workflow-post-execute']) { + if (!workflow.id) { + return; + } + + if (runData?.status === 'waiting') { + // No need to send telemetry or logs when the workflow hasn't finished yet. + return; + } + + const telemetryProperties: IExecutionTrackProperties = { + workflow_id: workflow.id, + is_manual: false, + version_cli: N8N_VERSION, + success: false, + }; + + if (userId) { + telemetryProperties.user_id = userId; + } + + if (runData?.data.resultData.error?.message?.includes('canceled')) { + runData.status = 'canceled'; + } + + telemetryProperties.success = !!runData?.finished; + + // const executionStatus: ExecutionStatus = runData?.status ?? 'unknown'; + const executionStatus: ExecutionStatus = runData + ? determineFinalExecutionStatus(runData) + : 'unknown'; + + if (runData !== undefined) { + telemetryProperties.execution_mode = runData.mode; + telemetryProperties.is_manual = runData.mode === 'manual'; + + let nodeGraphResult: INodesGraphResult | null = null; + + if (!telemetryProperties.success && runData?.data.resultData.error) { + telemetryProperties.error_message = runData?.data.resultData.error.message; + let errorNodeName = + 'node' in runData?.data.resultData.error + ? runData?.data.resultData.error.node?.name + : undefined; + telemetryProperties.error_node_type = + 'node' in runData?.data.resultData.error + ? runData?.data.resultData.error.node?.type + : undefined; + + if (runData.data.resultData.lastNodeExecuted) { + const lastNode = TelemetryHelpers.getNodeTypeForName( + workflow, + runData.data.resultData.lastNodeExecuted, + ); + + if (lastNode !== undefined) { + telemetryProperties.error_node_type = lastNode.type; + errorNodeName = lastNode.name; + } + } + + if (telemetryProperties.is_manual) { + nodeGraphResult = TelemetryHelpers.generateNodesGraph(workflow, this.nodeTypes); + telemetryProperties.node_graph = nodeGraphResult.nodeGraph; + telemetryProperties.node_graph_string = JSON.stringify(nodeGraphResult.nodeGraph); + + if (errorNodeName) { + telemetryProperties.error_node_id = nodeGraphResult.nameIndices[errorNodeName]; + } + } + } + + if (telemetryProperties.is_manual) { + if (!nodeGraphResult) { + nodeGraphResult = TelemetryHelpers.generateNodesGraph(workflow, this.nodeTypes); + } + + let userRole: 'owner' | 'sharee' | undefined = undefined; + if (userId) { + const role = await this.sharedWorkflowRepository.findSharingRole(userId, workflow.id); + if (role) { + userRole = role === 'workflow:owner' ? 'owner' : 'sharee'; + } + } + + const manualExecEventProperties: ITelemetryTrackProperties = { + user_id: userId, + workflow_id: workflow.id, + status: executionStatus, + executionStatus: runData?.status ?? 'unknown', + error_message: telemetryProperties.error_message as string, + error_node_type: telemetryProperties.error_node_type, + node_graph_string: telemetryProperties.node_graph_string as string, + error_node_id: telemetryProperties.error_node_id as string, + webhook_domain: null, + sharing_role: userRole, + }; + + if (!manualExecEventProperties.node_graph_string) { + nodeGraphResult = TelemetryHelpers.generateNodesGraph(workflow, this.nodeTypes); + manualExecEventProperties.node_graph_string = JSON.stringify(nodeGraphResult.nodeGraph); + } + + if (runData.data.startData?.destinationNode) { + const telemetryPayload = { + ...manualExecEventProperties, + node_type: TelemetryHelpers.getNodeTypeForName( + workflow, + runData.data.startData?.destinationNode, + )?.type, + node_id: nodeGraphResult.nameIndices[runData.data.startData?.destinationNode], + }; + + this.telemetry.track('Manual node exec finished', telemetryPayload); + } else { + nodeGraphResult.webhookNodeNames.forEach((name: string) => { + const execJson = runData.data.resultData.runData[name]?.[0]?.data?.main?.[0]?.[0] + ?.json as { headers?: { origin?: string } }; + if (execJson?.headers?.origin && execJson.headers.origin !== '') { + manualExecEventProperties.webhook_domain = pslGet( + execJson.headers.origin.replace(/^https?:\/\//, ''), + ); + } + }); + + this.telemetry.track('Manual workflow exec finished', manualExecEventProperties); + } + } + } + + this.telemetry.trackWorkflowExecution(telemetryProperties); + } } From 84efbd9b9c51f536b21a4f969ab607d277bef692 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 2 Aug 2024 12:06:17 +0200 Subject: [PATCH 06/98] feat(core): Support create, delete, edit role for users in Public API (#10279) --- .../users/spec/paths/users.id.role.yml | 31 +++ .../v1/handlers/users/spec/paths/users.id.yml | 18 ++ .../v1/handlers/users/spec/paths/users.yml | 50 ++++ .../v1/handlers/users/users.handler.ee.ts | 33 +++ packages/cli/src/PublicApi/v1/openapi.yml | 2 + .../test/integration/publicApi/users.test.ts | 252 ++++++++++++++++++ 6 files changed, 386 insertions(+) create mode 100644 packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.id.role.yml create mode 100644 packages/cli/test/integration/publicApi/users.test.ts diff --git a/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.id.role.yml b/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.id.role.yml new file mode 100644 index 0000000000000..92993adf7f9d4 --- /dev/null +++ b/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.id.role.yml @@ -0,0 +1,31 @@ +patch: + x-eov-operation-id: changeRole + x-eov-operation-handler: v1/handlers/users/users.handler.ee + tags: + - User + summary: Change a user's global role + description: Change a user's global role + parameters: + - $ref: '../schemas/parameters/userIdentifier.yml' + requestBody: + description: New role for the user + required: true + content: + application/json: + schema: + type: object + properties: + newRoleName: + type: string + enum: [global:admin, global:member] + required: + - newRoleName + responses: + '200': + description: Operation successful. + '401': + $ref: '../../../../shared/spec/responses/unauthorized.yml' + '403': + $ref: '../../../../shared/spec/responses/forbidden.yml' + '404': + $ref: '../../../../shared/spec/responses/notFound.yml' diff --git a/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.id.yml b/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.id.yml index 0d3c86c4cef97..f3dcae00534c4 100644 --- a/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.id.yml +++ b/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.id.yml @@ -17,3 +17,21 @@ get: $ref: '../schemas/user.yml' '401': $ref: '../../../../shared/spec/responses/unauthorized.yml' +delete: + x-eov-operation-id: deleteUser + x-eov-operation-handler: v1/handlers/users/users.handler.ee + tags: + - User + summary: Delete a user + description: Delete a user from your instance. + parameters: + - $ref: '../schemas/parameters/userIdentifier.yml' + responses: + '204': + description: Operation successful. + '401': + $ref: '../../../../shared/spec/responses/unauthorized.yml' + '403': + $ref: '../../../../shared/spec/responses/forbidden.yml' + '404': + $ref: '../../../../shared/spec/responses/notFound.yml' diff --git a/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.yml b/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.yml index 1d618435531b5..7778a08e35c60 100644 --- a/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.yml +++ b/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.yml @@ -18,3 +18,53 @@ get: $ref: '../schemas/userList.yml' '401': $ref: '../../../../shared/spec/responses/unauthorized.yml' +post: + x-eov-operation-id: createUser + x-eov-operation-handler: v1/handlers/users/users.handler.ee + tags: + - User + summary: Create multiple users + description: Create one or more users. + requestBody: + description: Array of users to be created. + required: true + content: + application/json: + schema: + type: array + items: + type: object + properties: + email: + type: string + format: email + role: + type: string + enum: [global:admin, global:member] + required: + - email + responses: + '200': + description: Operation successful. + content: + application/json: + schema: + type: object + properties: + user: + type: object + properties: + id: + type: string + email: + type: string + inviteAcceptUrl: + type: string + emailSent: + type: boolean + error: + type: string + '401': + $ref: '../../../../shared/spec/responses/unauthorized.yml' + '403': + $ref: '../../../../shared/spec/responses/forbidden.yml' diff --git a/packages/cli/src/PublicApi/v1/handlers/users/users.handler.ee.ts b/packages/cli/src/PublicApi/v1/handlers/users/users.handler.ee.ts index 0df22ec4aa9ef..cbfc43c0d100f 100644 --- a/packages/cli/src/PublicApi/v1/handlers/users/users.handler.ee.ts +++ b/packages/cli/src/PublicApi/v1/handlers/users/users.handler.ee.ts @@ -6,11 +6,19 @@ import { clean, getAllUsersAndCount, getUser } from './users.service.ee'; import { encodeNextCursor } from '../../shared/services/pagination.service'; import { globalScope, + isLicensed, validCursor, validLicenseWithUserQuota, } from '../../shared/middlewares/global.middleware'; import type { UserRequest } from '@/requests'; import { InternalHooks } from '@/InternalHooks'; +import type { Response } from 'express'; +import { InvitationController } from '@/controllers/invitation.controller'; +import { UsersController } from '@/controllers/users.controller'; + +type Create = UserRequest.Invite; +type Delete = UserRequest.Delete; +type ChangeRole = UserRequest.ChangeRole; export = { getUser: [ @@ -68,4 +76,29 @@ export = { }); }, ], + createUser: [ + globalScope('user:create'), + async (req: Create, res: Response) => { + const usersInvited = await Container.get(InvitationController).inviteUser(req); + + return res.status(201).json(usersInvited); + }, + ], + deleteUser: [ + globalScope('user:delete'), + async (req: Delete, res: Response) => { + await Container.get(UsersController).deleteUser(req); + + return res.status(204).send(); + }, + ], + changeRole: [ + isLicensed('feat:advancedPermissions'), + globalScope('user:changeRole'), + async (req: ChangeRole, res: Response) => { + await Container.get(UsersController).changeGlobalRole(req); + + return res.status(204).send(); + }, + ], }; diff --git a/packages/cli/src/PublicApi/v1/openapi.yml b/packages/cli/src/PublicApi/v1/openapi.yml index 1d4e14079f541..30c3a73bde0c7 100644 --- a/packages/cli/src/PublicApi/v1/openapi.yml +++ b/packages/cli/src/PublicApi/v1/openapi.yml @@ -70,6 +70,8 @@ paths: $ref: './handlers/users/spec/paths/users.yml' /users/{id}: $ref: './handlers/users/spec/paths/users.id.yml' + /users/{id}/role: + $ref: './handlers/users/spec/paths/users.id.role.yml' /source-control/pull: $ref: './handlers/sourceControl/spec/paths/sourceControl.yml' /variables: diff --git a/packages/cli/test/integration/publicApi/users.test.ts b/packages/cli/test/integration/publicApi/users.test.ts new file mode 100644 index 0000000000000..6021ae01a35f5 --- /dev/null +++ b/packages/cli/test/integration/publicApi/users.test.ts @@ -0,0 +1,252 @@ +import { setupTestServer } from '@test-integration/utils'; +import * as testDb from '../shared/testDb'; +import { createMember, createOwner, getUserById } from '@test-integration/db/users'; +import { mockInstance } from '@test/mocking'; +import { Telemetry } from '@/telemetry'; +import { FeatureNotLicensedError } from '@/errors/feature-not-licensed.error'; + +describe('Users in Public API', () => { + const testServer = setupTestServer({ endpointGroups: ['publicApi'] }); + mockInstance(Telemetry); + + beforeAll(async () => { + await testDb.init(); + }); + + beforeEach(async () => { + await testDb.truncate(['User']); + }); + + describe('POST /users', () => { + it('if not authenticated, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: false }); + const payload = { email: 'test@test.com', role: 'global:admin' }; + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).post('/users').send(payload); + + /** + * Assert + */ + expect(response.status).toBe(401); + }); + + it('if missing scope, should reject', async () => { + /** + * Arrange + */ + testServer.license.enable('feat:advancedPermissions'); + const member = await createMember({ withApiKey: true }); + const payload = [{ email: 'test@test.com', role: 'global:admin' }]; + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(member).post('/users').send(payload); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty('message', 'Forbidden'); + }); + + it('should create a user', async () => { + /** + * Arrange + */ + testServer.license.enable('feat:advancedPermissions'); + const owner = await createOwner({ withApiKey: true }); + const payload = [{ email: 'test@test.com', role: 'global:admin' }]; + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).post('/users').send(payload); + + /** + * Assert + */ + expect(response.status).toBe(201); + + expect(response.body).toHaveLength(1); + + const [result] = response.body; + const { user: returnedUser, error } = result; + const payloadUser = payload[0]; + + expect(returnedUser).toHaveProperty('email', payload[0].email); + expect(typeof returnedUser.inviteAcceptUrl).toBe('string'); + expect(typeof returnedUser.emailSent).toBe('boolean'); + expect(error).toBe(''); + + const storedUser = await getUserById(returnedUser.id); + expect(returnedUser.id).toBe(storedUser.id); + expect(returnedUser.email).toBe(storedUser.email); + expect(returnedUser.email).toBe(payloadUser.email); + expect(storedUser.role).toBe(payloadUser.role); + }); + }); + + describe('DELETE /users/:id', () => { + it('if not authenticated, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: false }); + const member = await createMember(); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).delete(`/users/${member.id}`); + + /** + * Assert + */ + expect(response.status).toBe(401); + }); + + it('if missing scope, should reject', async () => { + /** + * Arrange + */ + testServer.license.enable('feat:advancedPermissions'); + const firstMember = await createMember({ withApiKey: true }); + const secondMember = await createMember(); + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(firstMember) + .delete(`/users/${secondMember.id}`); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty('message', 'Forbidden'); + }); + + it('should delete a user', async () => { + /** + * Arrange + */ + testServer.license.enable('feat:advancedPermissions'); + const owner = await createOwner({ withApiKey: true }); + const member = await createMember(); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).delete(`/users/${member.id}`); + + /** + * Assert + */ + expect(response.status).toBe(204); + await expect(getUserById(member.id)).rejects.toThrow(); + }); + }); + + describe('PATCH /users/:id/role', () => { + it('if not authenticated, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: false }); + const member = await createMember(); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).patch(`/users/${member.id}/role`); + + /** + * Assert + */ + expect(response.status).toBe(401); + }); + + it('if not licensed, should reject', async () => { + /** + * Arrange + */ + const owner = await createOwner({ withApiKey: true }); + const member = await createMember(); + const payload = { newRoleName: 'global:admin' }; + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(owner) + .patch(`/users/${member.id}/role`) + .send(payload); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty( + 'message', + new FeatureNotLicensedError('feat:advancedPermissions').message, + ); + }); + + it('if missing scope, should reject', async () => { + /** + * Arrange + */ + testServer.license.enable('feat:advancedPermissions'); + const firstMember = await createMember({ withApiKey: true }); + const secondMember = await createMember(); + const payload = { newRoleName: 'global:admin' }; + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(firstMember) + .patch(`/users/${secondMember.id}/role`) + .send(payload); + + /** + * Assert + */ + expect(response.status).toBe(403); + expect(response.body).toHaveProperty('message', 'Forbidden'); + }); + + it("should change a user's role", async () => { + /** + * Arrange + */ + testServer.license.enable('feat:advancedPermissions'); + const owner = await createOwner({ withApiKey: true }); + const member = await createMember(); + const payload = { newRoleName: 'global:admin' }; + + /** + * Act + */ + const response = await testServer + .publicApiAgentFor(owner) + .patch(`/users/${member.id}/role`) + .send(payload); + + /** + * Assert + */ + expect(response.status).toBe(204); + const storedUser = await getUserById(member.id); + expect(storedUser.role).toBe(payload.newRoleName); + }); + }); +}); From 47a68b0220ce3e908e1e353b9e275dbc84a62402 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: Fri, 2 Aug 2024 12:20:34 +0200 Subject: [PATCH 07/98] ci: Fix DB tests (no-changelog) (#10282) --- .../integration/prometheus-metrics.test.ts | 48 ++++++------------- 1 file changed, 15 insertions(+), 33 deletions(-) diff --git a/packages/cli/test/integration/prometheus-metrics.test.ts b/packages/cli/test/integration/prometheus-metrics.test.ts index 4c00a98f74a49..9f6a0fad6e89d 100644 --- a/packages/cli/test/integration/prometheus-metrics.test.ts +++ b/packages/cli/test/integration/prometheus-metrics.test.ts @@ -5,45 +5,27 @@ import request, { type Response } from 'supertest'; import { N8N_VERSION } from '@/constants'; import { PrometheusMetricsService } from '@/metrics/prometheus-metrics.service'; import { setupTestServer } from './shared/utils'; -import { mockInstance } from '@test/mocking'; import { GlobalConfig } from '@n8n/config'; jest.unmock('@/eventbus/MessageEventBus/MessageEventBus'); const toLines = (response: Response) => response.text.trim().split('\n'); -mockInstance(GlobalConfig, { - database: { - type: 'sqlite', - sqlite: { - database: 'database.sqlite', - enableWAL: false, - executeVacuumOnStartup: false, - poolSize: 0, - }, - logging: { - enabled: false, - maxQueryExecutionTime: 0, - options: 'error', - }, - tablePrefix: '', - }, - endpoints: { - metrics: { - prefix: 'n8n_test_', - includeDefaultMetrics: true, - includeApiEndpoints: true, - includeCacheMetrics: true, - includeMessageEventBusMetrics: true, - includeCredentialTypeLabel: false, - includeNodeTypeLabel: false, - includeWorkflowIdLabel: false, - includeApiPathLabel: true, - includeApiMethodLabel: true, - includeApiStatusCodeLabel: true, - }, - }, -}); +const globalConfig = Container.get(GlobalConfig); +// @ts-expect-error `metrics` is a readonly property +globalConfig.endpoints.metrics = { + prefix: 'n8n_test_', + includeDefaultMetrics: true, + includeApiEndpoints: true, + includeCacheMetrics: true, + includeMessageEventBusMetrics: true, + includeCredentialTypeLabel: false, + includeNodeTypeLabel: false, + includeWorkflowIdLabel: false, + includeApiPathLabel: true, + includeApiMethodLabel: true, + includeApiStatusCodeLabel: true, +}; const server = setupTestServer({ endpointGroups: ['metrics'] }); const agent = request.agent(server.app); From c3e2e84065d6a5e55439450070243881b83c6aa6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 2 Aug 2024 12:25:57 +0200 Subject: [PATCH 08/98] refactor(core): Mark schema env vars used by cloud hooks (no-changelog) (#10283) --- packages/cli/src/config/schema.ts | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/packages/cli/src/config/schema.ts b/packages/cli/src/config/schema.ts index 068317e4918f6..dc63f53099659 100644 --- a/packages/cli/src/config/schema.ts +++ b/packages/cli/src/config/schema.ts @@ -4,6 +4,7 @@ import { Container } from 'typedi'; import { InstanceSettings } from 'n8n-core'; import { LOG_LEVELS } from 'n8n-workflow'; import { ensureStringArray } from './utils'; +import { GlobalConfig } from '@n8n/config'; convict.addFormat({ name: 'comma-separated-list', @@ -381,12 +382,17 @@ export const schema = { default: 0, env: 'N8N_USER_MANAGEMENT_JWT_REFRESH_TIMEOUT_HOURS', }, + + /** + * @important Do not remove until after cloud hooks are updated to stop using convict config. + */ isInstanceOwnerSetUp: { // n8n loads this setting from DB on startup doc: "Whether the instance owner's account has been set up", format: Boolean, default: false, }, + authenticationMethod: { doc: 'How to authenticate users (e.g. "email", "ldap", "saml")', format: ['email', 'ldap', 'saml'] as const, @@ -691,6 +697,19 @@ export const schema = { }, }, + /** + * @important Do not remove until after cloud hooks are updated to stop using convict config. + */ + endpoints: { + rest: { + format: String, + default: Container.get(GlobalConfig).endpoints.rest, + }, + }, + + /** + * @important Do not remove until after cloud hooks are updated to stop using convict config. + */ ai: { enabled: { doc: 'Whether AI features are enabled', From ae50bb95a8e5bf1cdbf9483da54b84094b82e260 Mon Sep 17 00:00:00 2001 From: Tomi Turtiainen <10324676+tomi@users.noreply.github.com> Date: Fri, 2 Aug 2024 13:46:35 +0300 Subject: [PATCH 09/98] fix(core): Make execution and its data creation atomic (#10276) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: कारतोफ्फेलस्क्रिप्ट™ --- packages/cli/src/ActiveExecutions.ts | 5 ++-- .../repositories/execution.repository.ts | 28 ++++++++++++------ .../repositories/executionData.repository.ts | 19 ++++++++++++ .../repositories/execution.repository.test.ts | 29 +++++++++++++++++++ 4 files changed, 69 insertions(+), 12 deletions(-) diff --git a/packages/cli/src/ActiveExecutions.ts b/packages/cli/src/ActiveExecutions.ts index 97313d5cb2f07..c1a6e8ffd65a9 100644 --- a/packages/cli/src/ActiveExecutions.ts +++ b/packages/cli/src/ActiveExecutions.ts @@ -12,6 +12,7 @@ import { ExecutionCancelledError, sleep, } from 'n8n-workflow'; +import { strict as assert } from 'node:assert'; import type { ExecutionPayload, @@ -74,9 +75,7 @@ export class ActiveExecutions { } executionId = await this.executionRepository.createNewExecution(fullExecutionData); - if (executionId === undefined) { - throw new ApplicationError('There was an issue assigning an execution id to the execution'); - } + assert(executionId); await this.concurrencyControl.throttle({ mode, executionId }); executionStatus = 'running'; diff --git a/packages/cli/src/databases/repositories/execution.repository.ts b/packages/cli/src/databases/repositories/execution.repository.ts index 1ebb22d8eb4bc..a8605147aa751 100644 --- a/packages/cli/src/databases/repositories/execution.repository.ts +++ b/packages/cli/src/databases/repositories/execution.repository.ts @@ -270,17 +270,27 @@ export class ExecutionRepository extends Repository { return rest; } + /** + * Insert a new execution and its execution data using a transaction. + */ async createNewExecution(execution: ExecutionPayload): Promise { - const { data, workflowData, ...rest } = execution; - const { identifiers: inserted } = await this.insert(rest); - const { id: executionId } = inserted[0] as { id: string }; - const { connections, nodes, name, settings } = workflowData ?? {}; - await this.executionDataRepository.insert({ - executionId, - workflowData: { connections, nodes, name, settings, id: workflowData.id }, - data: stringify(data), + return await this.manager.transaction(async (transactionManager) => { + const { data, workflowData, ...rest } = execution; + const insertResult = await transactionManager.insert(ExecutionEntity, rest); + const { id: executionId } = insertResult.identifiers[0] as { id: string }; + + const { connections, nodes, name, settings } = workflowData ?? {}; + await this.executionDataRepository.createExecutionDataForExecution( + { + executionId, + workflowData: { connections, nodes, name, settings, id: workflowData.id }, + data: stringify(data), + }, + transactionManager, + ); + + return String(executionId); }); - return String(executionId); } async markAsCrashed(executionIds: string | string[]) { diff --git a/packages/cli/src/databases/repositories/executionData.repository.ts b/packages/cli/src/databases/repositories/executionData.repository.ts index 5872f9888cd66..013453d998e42 100644 --- a/packages/cli/src/databases/repositories/executionData.repository.ts +++ b/packages/cli/src/databases/repositories/executionData.repository.ts @@ -1,13 +1,32 @@ import { Service } from 'typedi'; +import type { EntityManager } from '@n8n/typeorm'; +import type { IWorkflowBase } from 'n8n-workflow'; import { DataSource, In, Repository } from '@n8n/typeorm'; import { ExecutionData } from '../entities/ExecutionData'; +export interface CreateExecutionDataOpts extends Pick { + workflowData: Pick; +} + @Service() export class ExecutionDataRepository extends Repository { constructor(dataSource: DataSource) { super(ExecutionData, dataSource.manager); } + async createExecutionDataForExecution( + executionData: CreateExecutionDataOpts, + transactionManager: EntityManager, + ) { + const { data, executionId, workflowData } = executionData; + + return await transactionManager.insert(ExecutionData, { + executionId, + data, + workflowData, + }); + } + async findByExecutionIds(executionIds: string[]) { return await this.find({ select: ['workflowData'], diff --git a/packages/cli/test/integration/database/repositories/execution.repository.test.ts b/packages/cli/test/integration/database/repositories/execution.repository.test.ts index cfb897d627a56..e777f624299aa 100644 --- a/packages/cli/test/integration/database/repositories/execution.repository.test.ts +++ b/packages/cli/test/integration/database/repositories/execution.repository.test.ts @@ -52,5 +52,34 @@ describe('ExecutionRepository', () => { }); expect(executionData?.data).toEqual('[{"resultData":"1"},{}]'); }); + + it('should not create execution if execution data insert fails', async () => { + const executionRepo = Container.get(ExecutionRepository); + const executionDataRepo = Container.get(ExecutionDataRepository); + + const workflow = await createWorkflow({ settings: { executionOrder: 'v1' } }); + jest + .spyOn(executionDataRepo, 'createExecutionDataForExecution') + .mockRejectedValueOnce(new Error()); + + await expect( + async () => + await executionRepo.createNewExecution({ + workflowId: workflow.id, + data: { + //@ts-expect-error This is not needed for tests + resultData: {}, + }, + workflowData: workflow, + mode: 'manual', + startedAt: new Date(), + status: 'new', + finished: false, + }), + ).rejects.toThrow(); + + const executionEntities = await executionRepo.find(); + expect(executionEntities).toBeEmptyArray(); + }); }); }); From 7056e50b006bda665f64ce6234c5c1967891c415 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Iv=C3=A1n=20Ovejero?= Date: Fri, 2 Aug 2024 14:16:17 +0200 Subject: [PATCH 10/98] feat(core): Allow filtering executions and users by project in Public API (#10250) --- packages/cli/src/PublicApi/types.ts | 1 + .../handlers/executions/executions.handler.ts | 3 +- .../executions/spec/paths/executions.yml | 8 ++++ .../v1/handlers/users/spec/paths/users.yml | 8 ++++ .../v1/handlers/users/users.handler.ee.ts | 8 +++- .../v1/handlers/users/users.service.ee.ts | 7 ++- .../handlers/workflows/workflows.service.ts | 8 +++- .../projectRelation.repository.ts | 9 ++++ packages/cli/src/requests.ts | 2 +- .../src/workflows/workflowSharing.service.ts | 11 +++-- .../integration/publicApi/executions.test.ts | 38 ++++++++++++++++ .../integration/publicApi/users.ee.test.ts | 44 ++++++++++++++++++- 12 files changed, 138 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/PublicApi/types.ts b/packages/cli/src/PublicApi/types.ts index 831dd03ebfb9c..631a8c09d7236 100644 --- a/packages/cli/src/PublicApi/types.ts +++ b/packages/cli/src/PublicApi/types.ts @@ -29,6 +29,7 @@ export declare namespace ExecutionRequest { includeData?: boolean; workflowId?: string; lastId?: string; + projectId?: string; } >; diff --git a/packages/cli/src/PublicApi/v1/handlers/executions/executions.handler.ts b/packages/cli/src/PublicApi/v1/handlers/executions/executions.handler.ts index 16ca8093cb325..ed078b52d5249 100644 --- a/packages/cli/src/PublicApi/v1/handlers/executions/executions.handler.ts +++ b/packages/cli/src/PublicApi/v1/handlers/executions/executions.handler.ts @@ -95,9 +95,10 @@ export = { status = undefined, includeData = false, workflowId = undefined, + projectId, } = req.query; - const sharedWorkflowsIds = await getSharedWorkflowIds(req.user, ['workflow:read']); + const sharedWorkflowsIds = await getSharedWorkflowIds(req.user, ['workflow:read'], projectId); // user does not have workflows hence no executions // or the execution they are trying to access belongs to a workflow they do not own diff --git a/packages/cli/src/PublicApi/v1/handlers/executions/spec/paths/executions.yml b/packages/cli/src/PublicApi/v1/handlers/executions/spec/paths/executions.yml index 8d26492ec30bf..6fcdaf356e4bb 100644 --- a/packages/cli/src/PublicApi/v1/handlers/executions/spec/paths/executions.yml +++ b/packages/cli/src/PublicApi/v1/handlers/executions/spec/paths/executions.yml @@ -21,6 +21,14 @@ get: schema: type: string example: '1000' + - name: projectId + in: query + required: false + explode: false + allowReserved: true + schema: + type: string + example: VmwOO9HeTEj20kxM - $ref: '../../../../shared/spec/parameters/limit.yml' - $ref: '../../../../shared/spec/parameters/cursor.yml' responses: diff --git a/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.yml b/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.yml index 7778a08e35c60..e767ab33cc16b 100644 --- a/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.yml +++ b/packages/cli/src/PublicApi/v1/handlers/users/spec/paths/users.yml @@ -9,6 +9,14 @@ get: - $ref: '../../../../shared/spec/parameters/limit.yml' - $ref: '../../../../shared/spec/parameters/cursor.yml' - $ref: '../schemas/parameters/includeRole.yml' + - name: projectId + in: query + required: false + explode: false + allowReserved: true + schema: + type: string + example: VmwOO9HeTEj20kxM responses: '200': description: Operation successful. diff --git a/packages/cli/src/PublicApi/v1/handlers/users/users.handler.ee.ts b/packages/cli/src/PublicApi/v1/handlers/users/users.handler.ee.ts index cbfc43c0d100f..53c568ee69acd 100644 --- a/packages/cli/src/PublicApi/v1/handlers/users/users.handler.ee.ts +++ b/packages/cli/src/PublicApi/v1/handlers/users/users.handler.ee.ts @@ -12,6 +12,7 @@ import { } from '../../shared/middlewares/global.middleware'; import type { UserRequest } from '@/requests'; import { InternalHooks } from '@/InternalHooks'; +import { ProjectRelationRepository } from '@/databases/repositories/projectRelation.repository'; import type { Response } from 'express'; import { InvitationController } from '@/controllers/invitation.controller'; import { UsersController } from '@/controllers/users.controller'; @@ -51,12 +52,17 @@ export = { validCursor, globalScope(['user:list', 'user:read']), async (req: UserRequest.Get, res: express.Response) => { - const { offset = 0, limit = 100, includeRole = false } = req.query; + const { offset = 0, limit = 100, includeRole = false, projectId } = req.query; + + const _in = projectId + ? await Container.get(ProjectRelationRepository).findUserIdsByProjectId(projectId) + : undefined; const [users, count] = await getAllUsersAndCount({ includeRole, limit, offset, + in: _in, }); const telemetryData = { diff --git a/packages/cli/src/PublicApi/v1/handlers/users/users.service.ee.ts b/packages/cli/src/PublicApi/v1/handlers/users/users.service.ee.ts index f7bf6618168f0..e62c9465747ed 100644 --- a/packages/cli/src/PublicApi/v1/handlers/users/users.service.ee.ts +++ b/packages/cli/src/PublicApi/v1/handlers/users/users.service.ee.ts @@ -3,6 +3,8 @@ import { UserRepository } from '@db/repositories/user.repository'; import type { User } from '@db/entities/User'; import pick from 'lodash/pick'; import { validate as uuidValidate } from 'uuid'; +// eslint-disable-next-line n8n-local-rules/misplaced-n8n-typeorm-import +import { In } from '@n8n/typeorm'; export async function getUser(data: { withIdentifier: string; @@ -25,9 +27,12 @@ export async function getAllUsersAndCount(data: { includeRole?: boolean; limit?: number; offset?: number; + in?: string[]; }): Promise<[User[], number]> { + const { in: _in } = data; + const users = await Container.get(UserRepository).find({ - where: {}, + where: { ...(_in && { id: In(_in) }) }, skip: data.offset, take: data.limit, }); diff --git a/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts b/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts index d301e61c93966..3e3afc078ed92 100644 --- a/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts +++ b/packages/cli/src/PublicApi/v1/handlers/workflows/workflows.service.ts @@ -17,15 +17,21 @@ function insertIf(condition: boolean, elements: string[]): string[] { return condition ? elements : []; } -export async function getSharedWorkflowIds(user: User, scopes: Scope[]): Promise { +export async function getSharedWorkflowIds( + user: User, + scopes: Scope[], + projectId?: string, +): Promise { if (Container.get(License).isSharingEnabled()) { return await Container.get(WorkflowSharingService).getSharedWorkflowIds(user, { scopes, + projectId, }); } else { return await Container.get(WorkflowSharingService).getSharedWorkflowIds(user, { workflowRoles: ['workflow:owner'], projectRoles: ['project:personalOwner'], + projectId, }); } } diff --git a/packages/cli/src/databases/repositories/projectRelation.repository.ts b/packages/cli/src/databases/repositories/projectRelation.repository.ts index bddfd6e38d66f..00fc4de34a195 100644 --- a/packages/cli/src/databases/repositories/projectRelation.repository.ts +++ b/packages/cli/src/databases/repositories/projectRelation.repository.ts @@ -52,4 +52,13 @@ export class ProjectRelationRepository extends Repository { {} as Record, ); } + + async findUserIdsByProjectId(projectId: string): Promise { + const rows = await this.find({ + select: ['userId'], + where: { projectId }, + }); + + return [...new Set(rows.map((r) => r.userId))]; + } } diff --git a/packages/cli/src/requests.ts b/packages/cli/src/requests.ts index 1ea265f2394e7..4fe369857eb7a 100644 --- a/packages/cli/src/requests.ts +++ b/packages/cli/src/requests.ts @@ -306,7 +306,7 @@ export declare namespace UserRequest { { id: string; email: string; identifier: string }, {}, {}, - { limit?: number; offset?: number; cursor?: string; includeRole?: boolean } + { limit?: number; offset?: number; cursor?: string; includeRole?: boolean; projectId?: string } >; export type PasswordResetLink = AuthenticatedRequest<{ id: string }, {}, {}, {}>; diff --git a/packages/cli/src/workflows/workflowSharing.service.ts b/packages/cli/src/workflows/workflowSharing.service.ts index 93ef76438f7b0..add5bb31b8fc9 100644 --- a/packages/cli/src/workflows/workflowSharing.service.ts +++ b/packages/cli/src/workflows/workflowSharing.service.ts @@ -27,11 +27,16 @@ export class WorkflowSharingService { async getSharedWorkflowIds( user: User, options: - | { scopes: Scope[] } - | { projectRoles: ProjectRole[]; workflowRoles: WorkflowSharingRole[] }, + | { scopes: Scope[]; projectId?: string } + | { projectRoles: ProjectRole[]; workflowRoles: WorkflowSharingRole[]; projectId?: string }, ): Promise { + const { projectId } = options; + if (user.hasGlobalScope('workflow:read')) { - const sharedWorkflows = await this.sharedWorkflowRepository.find({ select: ['workflowId'] }); + const sharedWorkflows = await this.sharedWorkflowRepository.find({ + select: ['workflowId'], + ...(projectId && { where: { projectId } }), + }); return sharedWorkflows.map(({ workflowId }) => workflowId); } diff --git a/packages/cli/test/integration/publicApi/executions.test.ts b/packages/cli/test/integration/publicApi/executions.test.ts index 9961dac545036..8dac97fc22381 100644 --- a/packages/cli/test/integration/publicApi/executions.test.ts +++ b/packages/cli/test/integration/publicApi/executions.test.ts @@ -20,6 +20,8 @@ import { import type { SuperAgentTest } from '../shared/types'; import { mockInstance } from '@test/mocking'; import { Telemetry } from '@/telemetry'; +import { createTeamProject } from '@test-integration/db/projects'; +import type { ExecutionEntity } from '@/databases/entities/ExecutionEntity'; let owner: User; let user1: User; @@ -447,6 +449,42 @@ describe('GET /executions', () => { } }); + test('should return executions filtered by project ID', async () => { + /** + * Arrange + */ + const [firstProject, secondProject] = await Promise.all([ + createTeamProject(), + createTeamProject(), + ]); + const [firstWorkflow, secondWorkflow] = await Promise.all([ + createWorkflow({}, firstProject), + createWorkflow({}, secondProject), + ]); + const [firstExecution, secondExecution, _] = await Promise.all([ + createExecution({}, firstWorkflow), + createExecution({}, firstWorkflow), + createExecution({}, secondWorkflow), + ]); + + /** + * Act + */ + const response = await authOwnerAgent.get('/executions').query({ + projectId: firstProject.id, + }); + + /** + * Assert + */ + expect(response.statusCode).toBe(200); + expect(response.body.data.length).toBe(2); + expect(response.body.nextCursor).toBeNull(); + expect(response.body.data.map((execution: ExecutionEntity) => execution.id)).toEqual( + expect.arrayContaining([firstExecution.id, secondExecution.id]), + ); + }); + test('owner should retrieve all executions regardless of ownership', async () => { const [firstWorkflowForUser1, secondWorkflowForUser1] = await createManyWorkflows(2, {}, user1); await createManyExecutions(2, firstWorkflowForUser1, createSuccessfulExecution); diff --git a/packages/cli/test/integration/publicApi/users.ee.test.ts b/packages/cli/test/integration/publicApi/users.ee.test.ts index 6e57a629d7dc7..be23d8f45a519 100644 --- a/packages/cli/test/integration/publicApi/users.ee.test.ts +++ b/packages/cli/test/integration/publicApi/users.ee.test.ts @@ -7,8 +7,10 @@ import { mockInstance } from '../../shared/mocking'; import { randomApiKey } from '../shared/random'; import * as utils from '../shared/utils/'; import * as testDb from '../shared/testDb'; -import { createUser, createUserShell } from '../shared/db/users'; +import { createOwner, createUser, createUserShell } from '../shared/db/users'; import type { SuperAgentTest } from '../shared/types'; +import { createTeamProject, linkUserToProject } from '@test-integration/db/projects'; +import type { User } from '@/databases/entities/User'; mockInstance(License, { getUsersLimit: jest.fn().mockReturnValue(-1), @@ -84,6 +86,46 @@ describe('With license unlimited quota:users', () => { expect(updatedAt).toBeDefined(); } }); + + it('should return users filtered by project ID', async () => { + /** + * Arrange + */ + const [owner, firstMember, secondMember, thirdMember] = await Promise.all([ + createOwner({ withApiKey: true }), + createUser({ role: 'global:member' }), + createUser({ role: 'global:member' }), + createUser({ role: 'global:member' }), + ]); + + const [firstProject, secondProject] = await Promise.all([ + createTeamProject(), + createTeamProject(), + ]); + + await Promise.all([ + linkUserToProject(firstMember, firstProject, 'project:admin'), + linkUserToProject(secondMember, firstProject, 'project:viewer'), + linkUserToProject(thirdMember, secondProject, 'project:admin'), + ]); + + /** + * Act + */ + const response = await testServer.publicApiAgentFor(owner).get('/users').query({ + projectId: firstProject.id, + }); + + /** + * Assert + */ + expect(response.status).toBe(200); + expect(response.body.data.length).toBe(2); + expect(response.body.nextCursor).toBeNull(); + expect(response.body.data.map((user: User) => user.id)).toEqual( + expect.arrayContaining([firstMember.id, secondMember.id]), + ); + }); }); describe('GET /users/:id', () => { From 0faf46f4f87bc35406392f735e794e7aeceb6ead 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: Fri, 2 Aug 2024 15:18:33 +0200 Subject: [PATCH 11/98] refactor(core): Move instanceRole to InstanceSettings (no-changelog) (#10242) --- packages/cli/src/License.ts | 2 +- packages/cli/src/commands/start.ts | 4 ++-- packages/cli/src/config/schema.ts | 6 ----- .../execution-recovery.service.test.ts | 23 +++++++++---------- .../executions/execution-recovery.service.ts | 10 ++++---- .../cli/src/services/orchestration.service.ts | 10 +++++--- .../orchestration/main/MultiMainSetup.ee.ts | 14 ++++++----- packages/cli/src/services/pruning.service.ts | 5 ++-- .../test/integration/pruning.service.test.ts | 8 ++++--- packages/cli/test/unit/WaitTracker.test.ts | 2 +- .../services/orchestration.service.test.ts | 20 +++++++++------- packages/core/src/InstanceSettings.ts | 21 +++++++++++++++++ 12 files changed, 77 insertions(+), 48 deletions(-) diff --git a/packages/cli/src/License.ts b/packages/cli/src/License.ts index 9a2740ce7cbb7..643da18fd26e9 100644 --- a/packages/cli/src/License.ts +++ b/packages/cli/src/License.ts @@ -55,7 +55,7 @@ export class License { * This ensures the mains do not cause a 429 (too many requests) on license init. */ if (config.getEnv('multiMainSetup.enabled')) { - return autoRenewEnabled && config.getEnv('instanceRole') === 'leader'; + return autoRenewEnabled && this.instanceSettings.isLeader; } return autoRenewEnabled; diff --git a/packages/cli/src/commands/start.ts b/packages/cli/src/commands/start.ts index 4d7cd888b4cc0..63349ab6d1f0e 100644 --- a/packages/cli/src/commands/start.ts +++ b/packages/cli/src/commands/start.ts @@ -186,7 +186,7 @@ export class Start extends BaseCommand { await this.initOrchestration(); this.logger.debug('Orchestration init complete'); - if (!config.getEnv('license.autoRenewEnabled') && config.getEnv('instanceRole') === 'leader') { + if (!config.getEnv('license.autoRenewEnabled') && this.instanceSettings.isLeader) { this.logger.warn( 'Automatic license renewal is disabled. The license will not renew automatically, and access to licensed features may be lost!', ); @@ -210,7 +210,7 @@ export class Start extends BaseCommand { async initOrchestration() { if (config.getEnv('executions.mode') === 'regular') { - config.set('instanceRole', 'leader'); + this.instanceSettings.markAsLeader(); return; } diff --git a/packages/cli/src/config/schema.ts b/packages/cli/src/config/schema.ts index dc63f53099659..f99c14885ddf9 100644 --- a/packages/cli/src/config/schema.ts +++ b/packages/cli/src/config/schema.ts @@ -759,12 +759,6 @@ export const schema = { }, }, - instanceRole: { - doc: 'Always `leader` in single-main setup. `leader` or `follower` in multi-main setup.', - format: ['unset', 'leader', 'follower'] as const, - default: 'unset', // only until Start.initOrchestration - }, - multiMainSetup: { enabled: { doc: 'Whether to enable multi-main setup for queue mode (license required)', diff --git a/packages/cli/src/executions/__tests__/execution-recovery.service.test.ts b/packages/cli/src/executions/__tests__/execution-recovery.service.test.ts index 7e33c6ac74a8a..39a84d27a78c1 100644 --- a/packages/cli/src/executions/__tests__/execution-recovery.service.test.ts +++ b/packages/cli/src/executions/__tests__/execution-recovery.service.test.ts @@ -1,6 +1,7 @@ import Container from 'typedi'; import { stringify } from 'flatted'; import { randomInt } from 'n8n-workflow'; +import { InstanceSettings } from 'n8n-core'; import { mockInstance } from '@test/mocking'; import { createWorkflow } from '@test-integration/db/workflows'; @@ -21,38 +22,37 @@ import { EventMessageNode } from '@/eventbus/EventMessageClasses/EventMessageNod import { IN_PROGRESS_EXECUTION_DATA, OOM_WORKFLOW } from './constants'; import { setupMessages } from './utils'; -import type { EventService } from '@/eventbus/event.service'; import type { EventMessageTypes as EventMessage } from '@/eventbus/EventMessageClasses'; -import type { Logger } from '@/Logger'; describe('ExecutionRecoveryService', () => { - let push: Push; + const push = mockInstance(Push); + mockInstance(InternalHooks); + const instanceSettings = new InstanceSettings(); + let executionRecoveryService: ExecutionRecoveryService; let orchestrationService: OrchestrationService; let executionRepository: ExecutionRepository; beforeAll(async () => { await testDb.init(); - push = mockInstance(Push); executionRepository = Container.get(ExecutionRepository); orchestrationService = Container.get(OrchestrationService); - mockInstance(InternalHooks); executionRecoveryService = new ExecutionRecoveryService( - mock(), + mock(), + instanceSettings, push, executionRepository, orchestrationService, - mock(), + mock(), ); }); beforeEach(() => { - config.set('instanceRole', 'leader'); + instanceSettings.markAsLeader(); }); afterEach(async () => { - config.load(config.default); jest.restoreAllMocks(); await testDb.truncate(['Execution', 'ExecutionData', 'Workflow']); executionRecoveryService.shutdown(); @@ -69,7 +69,6 @@ describe('ExecutionRecoveryService', () => { * Arrange */ config.set('executions.mode', 'queue'); - jest.spyOn(orchestrationService, 'isLeader', 'get').mockReturnValue(true); const scheduleSpy = jest.spyOn(executionRecoveryService, 'scheduleQueueRecovery'); /** @@ -88,7 +87,7 @@ describe('ExecutionRecoveryService', () => { * Arrange */ config.set('executions.mode', 'queue'); - jest.spyOn(orchestrationService, 'isLeader', 'get').mockReturnValue(false); + instanceSettings.markAsFollower(); const scheduleSpy = jest.spyOn(executionRecoveryService, 'scheduleQueueRecovery'); /** @@ -130,7 +129,7 @@ describe('ExecutionRecoveryService', () => { /** * Arrange */ - config.set('instanceRole', 'follower'); + instanceSettings.markAsFollower(); // @ts-expect-error Private method const amendSpy = jest.spyOn(executionRecoveryService, 'amend'); const messages = setupMessages('123', 'Some workflow'); diff --git a/packages/cli/src/executions/execution-recovery.service.ts b/packages/cli/src/executions/execution-recovery.service.ts index 615c65ea6ef5f..152d4ad4f2964 100644 --- a/packages/cli/src/executions/execution-recovery.service.ts +++ b/packages/cli/src/executions/execution-recovery.service.ts @@ -5,6 +5,7 @@ import { ExecutionRepository } from '@db/repositories/execution.repository'; import { getWorkflowHooksMain } from '@/WorkflowExecuteAdditionalData'; // @TODO: Dependency cycle import type { DateTime } from 'luxon'; import type { IRun, ITaskData } from 'n8n-workflow'; +import { InstanceSettings } from 'n8n-core'; import type { EventMessageTypes } from '../eventbus/EventMessageClasses'; import type { IExecutionResponse } from '@/Interfaces'; import { NodeCrashedError } from '@/errors/node-crashed.error'; @@ -24,6 +25,7 @@ import { EventService } from '@/eventbus/event.service'; export class ExecutionRecoveryService { constructor( private readonly logger: Logger, + private readonly instanceSettings: InstanceSettings, private readonly push: Push, private readonly executionRepository: ExecutionRepository, private readonly orchestrationService: OrchestrationService, @@ -36,10 +38,10 @@ export class ExecutionRecoveryService { init() { if (config.getEnv('executions.mode') === 'regular') return; - const { isLeader, isMultiMainSetupEnabled } = this.orchestrationService; - + const { isLeader } = this.instanceSettings; if (isLeader) this.scheduleQueueRecovery(); + const { isMultiMainSetupEnabled } = this.orchestrationService; if (isMultiMainSetupEnabled) { this.orchestrationService.multiMainSetup .on('leader-takeover', () => this.scheduleQueueRecovery()) @@ -58,7 +60,7 @@ export class ExecutionRecoveryService { * Recover key properties of a truncated execution using event logs. */ async recoverFromLogs(executionId: string, messages: EventMessageTypes[]) { - if (this.orchestrationService.isFollower) return; + if (this.instanceSettings.isFollower) return; const amendedExecution = await this.amend(executionId, messages); @@ -319,7 +321,7 @@ export class ExecutionRecoveryService { private shouldScheduleQueueRecovery() { return ( config.getEnv('executions.mode') === 'queue' && - config.getEnv('instanceRole') === 'leader' && + this.instanceSettings.isLeader && !this.isShuttingDown ); } diff --git a/packages/cli/src/services/orchestration.service.ts b/packages/cli/src/services/orchestration.service.ts index bfc1d140fc6fa..283470f4d2114 100644 --- a/packages/cli/src/services/orchestration.service.ts +++ b/packages/cli/src/services/orchestration.service.ts @@ -7,11 +7,13 @@ import type { RedisServiceBaseCommand, RedisServiceCommand } from './redis/Redis import { RedisService } from './redis.service'; import { MultiMainSetup } from './orchestration/main/MultiMainSetup.ee'; import type { WorkflowActivateMode } from 'n8n-workflow'; +import { InstanceSettings } from 'n8n-core'; @Service() export class OrchestrationService { constructor( private readonly logger: Logger, + private readonly instanceSettings: InstanceSettings, private readonly redisService: RedisService, readonly multiMainSetup: MultiMainSetup, ) {} @@ -43,12 +45,14 @@ export class OrchestrationService { return config.getEnv('redis.queueModeId'); } + /** @deprecated use InstanceSettings.isLeader */ get isLeader() { - return config.getEnv('instanceRole') === 'leader'; + return this.instanceSettings.isLeader; } + /** @deprecated use InstanceSettings.isFollower */ get isFollower() { - return config.getEnv('instanceRole') !== 'leader'; + return this.instanceSettings.isFollower; } sanityCheck() { @@ -63,7 +67,7 @@ export class OrchestrationService { if (this.isMultiMainSetupEnabled) { await this.multiMainSetup.init(); } else { - config.set('instanceRole', 'leader'); + this.instanceSettings.markAsLeader(); } this.isInitialized = true; diff --git a/packages/cli/src/services/orchestration/main/MultiMainSetup.ee.ts b/packages/cli/src/services/orchestration/main/MultiMainSetup.ee.ts index cabcf5996bc08..89c6ef725ab45 100644 --- a/packages/cli/src/services/orchestration/main/MultiMainSetup.ee.ts +++ b/packages/cli/src/services/orchestration/main/MultiMainSetup.ee.ts @@ -1,6 +1,7 @@ import config from '@/config'; import { Service } from 'typedi'; import { TIME } from '@/constants'; +import { InstanceSettings } from 'n8n-core'; import { ErrorReporterProxy as EventReporter } from 'n8n-workflow'; import { Logger } from '@/Logger'; import { RedisServicePubSubPublisher } from '@/services/redis/RedisServicePubSubPublisher'; @@ -16,6 +17,7 @@ type MultiMainEvents = { export class MultiMainSetup extends TypedEmitter { constructor( private readonly logger: Logger, + private readonly instanceSettings: InstanceSettings, private readonly redisPublisher: RedisServicePubSubPublisher, private readonly redisClientService: RedisClientService, ) { @@ -50,7 +52,7 @@ export class MultiMainSetup extends TypedEmitter { async shutdown() { clearInterval(this.leaderCheckInterval); - const isLeader = config.getEnv('instanceRole') === 'leader'; + const { isLeader } = this.instanceSettings; if (isLeader) await this.redisPublisher.clear(this.leaderKey); } @@ -69,8 +71,8 @@ export class MultiMainSetup extends TypedEmitter { if (leaderId && leaderId !== this.instanceId) { this.logger.debug(`[Instance ID ${this.instanceId}] Leader is other instance "${leaderId}"`); - if (config.getEnv('instanceRole') === 'leader') { - config.set('instanceRole', 'follower'); + if (this.instanceSettings.isLeader) { + this.instanceSettings.markAsFollower(); this.emit('leader-stepdown'); // lost leadership - stop triggers, pollers, pruning, wait-tracking, queue recovery @@ -85,7 +87,7 @@ export class MultiMainSetup extends TypedEmitter { `[Instance ID ${this.instanceId}] Leadership vacant, attempting to become leader...`, ); - config.set('instanceRole', 'follower'); + this.instanceSettings.markAsFollower(); /** * Lost leadership - stop triggers, pollers, pruning, wait tracking, license renewal, queue recovery @@ -106,7 +108,7 @@ export class MultiMainSetup extends TypedEmitter { if (keySetSuccessfully) { this.logger.debug(`[Instance ID ${this.instanceId}] Leader is now this instance`); - config.set('instanceRole', 'leader'); + this.instanceSettings.markAsLeader(); await this.redisPublisher.setExpiration(this.leaderKey, this.leaderKeyTtl); @@ -115,7 +117,7 @@ export class MultiMainSetup extends TypedEmitter { */ this.emit('leader-takeover'); } else { - config.set('instanceRole', 'follower'); + this.instanceSettings.markAsFollower(); } } diff --git a/packages/cli/src/services/pruning.service.ts b/packages/cli/src/services/pruning.service.ts index 78c0aad03750c..7f824836ae4ee 100644 --- a/packages/cli/src/services/pruning.service.ts +++ b/packages/cli/src/services/pruning.service.ts @@ -1,5 +1,5 @@ import { Service } from 'typedi'; -import { BinaryDataService } from 'n8n-core'; +import { BinaryDataService, InstanceSettings } from 'n8n-core'; import { inTest, TIME } from '@/constants'; import config from '@/config'; import { ExecutionRepository } from '@db/repositories/execution.repository'; @@ -25,6 +25,7 @@ export class PruningService { constructor( private readonly logger: Logger, + private readonly instanceSettings: InstanceSettings, private readonly executionRepository: ExecutionRepository, private readonly binaryDataService: BinaryDataService, private readonly orchestrationService: OrchestrationService, @@ -56,7 +57,7 @@ export class PruningService { if ( config.getEnv('multiMainSetup.enabled') && config.getEnv('generic.instanceType') === 'main' && - config.getEnv('instanceRole') === 'follower' + this.instanceSettings.isFollower ) { return false; } diff --git a/packages/cli/test/integration/pruning.service.test.ts b/packages/cli/test/integration/pruning.service.test.ts index 09a43a5b5b123..37d218c09d438 100644 --- a/packages/cli/test/integration/pruning.service.test.ts +++ b/packages/cli/test/integration/pruning.service.test.ts @@ -1,5 +1,5 @@ import config from '@/config'; -import { BinaryDataService } from 'n8n-core'; +import { BinaryDataService, InstanceSettings } from 'n8n-core'; import type { ExecutionStatus } from 'n8n-workflow'; import Container from 'typedi'; @@ -15,10 +15,11 @@ import { mockInstance } from '../shared/mocking'; import { createWorkflow } from './shared/db/workflows'; import { createExecution, createSuccessfulExecution } from './shared/db/executions'; import { mock } from 'jest-mock-extended'; -import type { OrchestrationService } from '@/services/orchestration.service'; describe('softDeleteOnPruningCycle()', () => { let pruningService: PruningService; + const instanceSettings = new InstanceSettings(); + instanceSettings.markAsLeader(); const now = new Date(); const yesterday = new Date(Date.now() - TIME.DAY); @@ -29,9 +30,10 @@ describe('softDeleteOnPruningCycle()', () => { pruningService = new PruningService( mockInstance(Logger), + instanceSettings, Container.get(ExecutionRepository), mockInstance(BinaryDataService), - mock(), + mock(), ); workflow = await createWorkflow(); diff --git a/packages/cli/test/unit/WaitTracker.test.ts b/packages/cli/test/unit/WaitTracker.test.ts index 0f8464e18d14d..fb51d2e25bd30 100644 --- a/packages/cli/test/unit/WaitTracker.test.ts +++ b/packages/cli/test/unit/WaitTracker.test.ts @@ -10,7 +10,7 @@ jest.useFakeTimers(); describe('WaitTracker', () => { const executionRepository = mock(); const multiMainSetup = mock(); - const orchestrationService = new OrchestrationService(mock(), mock(), multiMainSetup); + const orchestrationService = new OrchestrationService(mock(), mock(), mock(), multiMainSetup); const execution = mock({ id: '123', diff --git a/packages/cli/test/unit/services/orchestration.service.test.ts b/packages/cli/test/unit/services/orchestration.service.test.ts index 75c7c06e40bb6..0b63f74344b57 100644 --- a/packages/cli/test/unit/services/orchestration.service.test.ts +++ b/packages/cli/test/unit/services/orchestration.service.test.ts @@ -1,4 +1,9 @@ import Container from 'typedi'; +import type Redis from 'ioredis'; +import { mock } from 'jest-mock-extended'; +import { InstanceSettings } from 'n8n-core'; +import type { WorkflowActivateMode } from 'n8n-workflow'; + import config from '@/config'; import { OrchestrationService } from '@/services/orchestration.service'; import type { RedisServiceWorkerResponseObject } from '@/services/redis/RedisServiceCommands'; @@ -13,11 +18,9 @@ import { Logger } from '@/Logger'; import { Push } from '@/push'; import { ActiveWorkflowManager } from '@/ActiveWorkflowManager'; import { mockInstance } from '../../shared/mocking'; -import type { WorkflowActivateMode } from 'n8n-workflow'; import { RedisClientService } from '@/services/redis/redis-client.service'; -import type Redis from 'ioredis'; -import { mock } from 'jest-mock-extended'; +const instanceSettings = Container.get(InstanceSettings); const redisClientService = mockInstance(RedisClientService); const mockRedisClient = mock(); redisClientService.createClient.mockReturnValue(mockRedisClient); @@ -72,6 +75,10 @@ describe('Orchestration Service', () => { queueModeId = config.get('redis.queueModeId'); }); + beforeEach(() => { + instanceSettings.markAsLeader(); + }); + afterAll(async () => { jest.mock('@/services/redis/RedisServicePubSubPublisher').restoreAllMocks(); jest.mock('@/services/redis/RedisServicePubSubSubscriber').restoreAllMocks(); @@ -141,13 +148,10 @@ describe('Orchestration Service', () => { ); expect(helpers.debounceMessageReceiver).toHaveBeenCalledTimes(2); expect(res1!.payload).toBeUndefined(); - expect((res2!.payload as { result: string }).result).toEqual('debounced'); + expect(res2!.payload).toEqual({ result: 'debounced' }); }); describe('shouldAddWebhooks', () => { - beforeEach(() => { - config.set('instanceRole', 'leader'); - }); test('should return true for init', () => { // We want to ensure that webhooks are populated on init // more https://github.com/n8n-io/n8n/pull/8830 @@ -169,7 +173,7 @@ describe('Orchestration Service', () => { }); test('should return false for update or activate when not leader', () => { - config.set('instanceRole', 'follower'); + instanceSettings.markAsFollower(); const modes = ['update', 'activate'] as WorkflowActivateMode[]; for (const mode of modes) { const result = os.shouldAddWebhooks(mode); diff --git a/packages/core/src/InstanceSettings.ts b/packages/core/src/InstanceSettings.ts index e8ab9aa553223..f75e1df712108 100644 --- a/packages/core/src/InstanceSettings.ts +++ b/packages/core/src/InstanceSettings.ts @@ -14,6 +14,8 @@ interface WritableSettings { type Settings = ReadOnlySettings & WritableSettings; +type InstanceRole = 'unset' | 'leader' | 'follower'; + const inTest = process.env.NODE_ENV === 'test'; @Service() @@ -38,6 +40,25 @@ export class InstanceSettings { readonly instanceId = this.generateInstanceId(); + /** Always `leader` in single-main setup. `leader` or `follower` in multi-main setup. */ + private instanceRole: InstanceRole = 'unset'; + + get isLeader() { + return this.instanceRole === 'leader'; + } + + markAsLeader() { + this.instanceRole = 'leader'; + } + + get isFollower() { + return this.instanceRole === 'follower'; + } + + markAsFollower() { + this.instanceRole = 'follower'; + } + get encryptionKey() { return this.settings.encryptionKey; } From 57d1c9a99e97308f2f1b8ae05ac3861a835e8e5a Mon Sep 17 00:00:00 2001 From: Eugene Date: Fri, 2 Aug 2024 16:00:09 +0200 Subject: [PATCH 12/98] feat(core): Show sub-node error on the logs pane. Open logs pane on sub-node error (#10248) --- cypress/constants.ts | 2 + .../e2e/233-AI-switch-to-logs-on-error.cy.ts | 279 ++++++++++++++++++ cypress/pages/ndv.ts | 1 + .../nodes/tools/ToolCode/ToolCode.node.ts | 4 +- .../src/components/Error/NodeErrorView.vue | 3 +- .../editor-ui/src/components/OutputPanel.vue | 32 +- packages/editor-ui/src/components/RunData.vue | 19 +- 7 files changed, 333 insertions(+), 7 deletions(-) create mode 100644 cypress/e2e/233-AI-switch-to-logs-on-error.cy.ts diff --git a/cypress/constants.ts b/cypress/constants.ts index 8439952ac7b81..8fdb03ef9cfba 100644 --- a/cypress/constants.ts +++ b/cypress/constants.ts @@ -37,6 +37,7 @@ export const INSTANCE_MEMBERS = [ export const MANUAL_TRIGGER_NODE_NAME = 'Manual Trigger'; export const MANUAL_TRIGGER_NODE_DISPLAY_NAME = 'When clicking ‘Test workflow’'; export const MANUAL_CHAT_TRIGGER_NODE_NAME = 'Chat Trigger'; +export const MANUAL_CHAT_TRIGGER_NODE_DISPLAY_NAME = 'When chat message received'; export const SCHEDULE_TRIGGER_NODE_NAME = 'Schedule Trigger'; export const CODE_NODE_NAME = 'Code'; export const SET_NODE_NAME = 'Set'; @@ -57,6 +58,7 @@ export const AI_TOOL_CODE_NODE_NAME = 'Code Tool'; export const AI_TOOL_WIKIPEDIA_NODE_NAME = 'Wikipedia'; export const AI_TOOL_HTTP_NODE_NAME = 'HTTP Request Tool'; export const AI_LANGUAGE_MODEL_OPENAI_CHAT_MODEL_NODE_NAME = 'OpenAI Chat Model'; +export const AI_MEMORY_POSTGRES_NODE_NAME = 'Postgres Chat Memory'; export const AI_OUTPUT_PARSER_AUTO_FIXING_NODE_NAME = 'Auto-fixing Output Parser'; export const WEBHOOK_NODE_NAME = 'Webhook'; diff --git a/cypress/e2e/233-AI-switch-to-logs-on-error.cy.ts b/cypress/e2e/233-AI-switch-to-logs-on-error.cy.ts new file mode 100644 index 0000000000000..78d4b61449c23 --- /dev/null +++ b/cypress/e2e/233-AI-switch-to-logs-on-error.cy.ts @@ -0,0 +1,279 @@ +import type { ExecutionError } from 'n8n-workflow/src'; +import { NDV, WorkflowPage as WorkflowPageClass } from '../pages'; +import { + addLanguageModelNodeToParent, + addMemoryNodeToParent, + addNodeToCanvas, + addToolNodeToParent, + navigateToNewWorkflowPage, + openNode, +} from '../composables/workflow'; +import { + AGENT_NODE_NAME, + AI_LANGUAGE_MODEL_OPENAI_CHAT_MODEL_NODE_NAME, + AI_MEMORY_POSTGRES_NODE_NAME, + AI_TOOL_CALCULATOR_NODE_NAME, + MANUAL_CHAT_TRIGGER_NODE_DISPLAY_NAME, + MANUAL_CHAT_TRIGGER_NODE_NAME, + MANUAL_TRIGGER_NODE_DISPLAY_NAME, + MANUAL_TRIGGER_NODE_NAME, +} from '../constants'; +import { + clickCreateNewCredential, + clickExecuteNode, + clickGetBackToCanvas, +} from '../composables/ndv'; +import { setCredentialValues } from '../composables/modals/credential-modal'; +import { + closeManualChatModal, + getManualChatMessages, + getManualChatModalLogs, + getManualChatModalLogsEntries, + sendManualChatMessage, +} from '../composables/modals/chat-modal'; +import { createMockNodeExecutionData, getVisibleSelect, runMockWorkflowExecution } from '../utils'; + +const ndv = new NDV(); +const WorkflowPage = new WorkflowPageClass(); + +function createRunDataWithError(inputMessage: string) { + return [ + createMockNodeExecutionData(MANUAL_CHAT_TRIGGER_NODE_NAME, { + jsonData: { + main: { input: inputMessage }, + }, + }), + createMockNodeExecutionData(AI_MEMORY_POSTGRES_NODE_NAME, { + jsonData: { + ai_memory: { + json: { + action: 'loadMemoryVariables', + values: { + input: inputMessage, + system_message: 'You are a helpful assistant', + formatting_instructions: + 'IMPORTANT: Always call `format_final_response` to format your final response!', + }, + }, + }, + }, + inputOverride: { + ai_memory: [ + [ + { + json: { + action: 'loadMemoryVariables', + values: { + input: inputMessage, + system_message: 'You are a helpful assistant', + formatting_instructions: + 'IMPORTANT: Always call `format_final_response` to format your final response!', + }, + }, + }, + ], + ], + }, + error: { + message: 'Internal error', + timestamp: 1722591723244, + name: 'NodeOperationError', + description: 'Internal error', + context: {}, + cause: { + name: 'error', + severity: 'FATAL', + code: '3D000', + file: 'postinit.c', + line: '885', + routine: 'InitPostgres', + } as unknown as Error, + } as ExecutionError, + }), + createMockNodeExecutionData(AGENT_NODE_NAME, { + executionStatus: 'error', + error: { + level: 'error', + tags: { + packageName: 'workflow', + }, + context: {}, + functionality: 'configuration-node', + name: 'NodeOperationError', + timestamp: 1722591723244, + node: { + parameters: { + notice: '', + sessionIdType: 'fromInput', + tableName: 'n8n_chat_histories', + }, + id: '6b9141da-0135-4e9d-94d1-2d658cbf48b5', + name: 'Postgres Chat Memory', + type: '@n8n/n8n-nodes-langchain.memoryPostgresChat', + typeVersion: 1, + position: [1140, 500], + credentials: { + postgres: { + id: 'RkyZetVpGsSfEAhQ', + name: 'Postgres account', + }, + }, + }, + messages: ['database "chat11" does not exist'], + description: 'Internal error', + message: 'Internal error', + } as unknown as ExecutionError, + metadata: { + subRun: [ + { + node: 'Postgres Chat Memory', + runIndex: 0, + }, + ], + }, + }), + ]; +} + +function setupTestWorkflow(chatTrigger: boolean = false) { + // Setup test workflow with AI Agent, Postgres Memory Node (source of error), Calculator Tool, and OpenAI Chat Model + if (chatTrigger) { + addNodeToCanvas(MANUAL_CHAT_TRIGGER_NODE_NAME, true); + } else { + addNodeToCanvas(MANUAL_TRIGGER_NODE_NAME, true); + } + + addNodeToCanvas(AGENT_NODE_NAME, true); + + if (!chatTrigger) { + // Remove chat trigger + WorkflowPage.getters + .canvasNodeByName(MANUAL_CHAT_TRIGGER_NODE_DISPLAY_NAME) + .find('[data-test-id="delete-node-button"]') + .click({ force: true }); + + // Set manual trigger to output standard pinned data + openNode(MANUAL_TRIGGER_NODE_DISPLAY_NAME); + ndv.actions.editPinnedData(); + ndv.actions.savePinnedData(); + ndv.actions.close(); + } + + // Calculator is added just to make OpenAI Chat Model work (tools can not be empty with OpenAI model) + addToolNodeToParent(AI_TOOL_CALCULATOR_NODE_NAME, AGENT_NODE_NAME); + clickGetBackToCanvas(); + + addMemoryNodeToParent(AI_MEMORY_POSTGRES_NODE_NAME, AGENT_NODE_NAME); + + clickCreateNewCredential(); + setCredentialValues({ + password: 'testtesttest', + }); + + ndv.getters.parameterInput('sessionIdType').click(); + getVisibleSelect().contains('Define below').click(); + ndv.getters.parameterInput('sessionKey').type('asdasd'); + + clickGetBackToCanvas(); + + addLanguageModelNodeToParent( + AI_LANGUAGE_MODEL_OPENAI_CHAT_MODEL_NODE_NAME, + AGENT_NODE_NAME, + true, + ); + + clickCreateNewCredential(); + setCredentialValues({ + apiKey: 'sk_test_123', + }); + clickGetBackToCanvas(); + + WorkflowPage.actions.zoomToFit(); +} + +function checkMessages(inputMessage: string, outputMessage: string) { + const messages = getManualChatMessages(); + messages.should('have.length', 2); + messages.should('contain', inputMessage); + messages.should('contain', outputMessage); + + getManualChatModalLogs().should('exist'); + getManualChatModalLogsEntries() + .should('have.length', 1) + .should('contain', AI_MEMORY_POSTGRES_NODE_NAME); +} + +describe("AI-233 Make root node's logs pane active in case of an error in sub-nodes", () => { + beforeEach(() => { + navigateToNewWorkflowPage(); + }); + + it('should open logs tab by default when there was an error', () => { + setupTestWorkflow(true); + + openNode(AGENT_NODE_NAME); + + const inputMessage = 'Test the code tool'; + + clickExecuteNode(); + runMockWorkflowExecution({ + trigger: () => sendManualChatMessage(inputMessage), + runData: createRunDataWithError(inputMessage), + lastNodeExecuted: AGENT_NODE_NAME, + }); + + checkMessages(inputMessage, '[ERROR: Internal error]'); + closeManualChatModal(); + + // Open the AI Agent node to see the logs + openNode(AGENT_NODE_NAME); + + // Finally check that logs pane is opened by default + ndv.getters.outputDataContainer().should('be.visible'); + + ndv.getters.aiOutputModeToggle().should('be.visible'); + ndv.getters + .aiOutputModeToggle() + .find('[role="radio"]') + .should('have.length', 2) + .eq(1) + .should('have.attr', 'aria-checked', 'true'); + + ndv.getters + .outputPanel() + .findChildByTestId('node-error-message') + .should('be.visible') + .should('contain', 'Error in sub-node'); + }); + + it('should switch to logs tab on error, when NDV is already opened', () => { + setupTestWorkflow(false); + + openNode(AGENT_NODE_NAME); + + const inputMessage = 'Test the code tool'; + + runMockWorkflowExecution({ + trigger: () => clickExecuteNode(), + runData: createRunDataWithError(inputMessage), + lastNodeExecuted: AGENT_NODE_NAME, + }); + + // Check that logs pane is opened by default + ndv.getters.outputDataContainer().should('be.visible'); + + ndv.getters.aiOutputModeToggle().should('be.visible'); + ndv.getters + .aiOutputModeToggle() + .find('[role="radio"]') + .should('have.length', 2) + .eq(1) + .should('have.attr', 'aria-checked', 'true'); + + ndv.getters + .outputPanel() + .findChildByTestId('node-error-message') + .should('be.visible') + .should('contain', 'Error in sub-node'); + }); +}); diff --git a/cypress/pages/ndv.ts b/cypress/pages/ndv.ts index 018ec43a5de9c..24e1922663371 100644 --- a/cypress/pages/ndv.ts +++ b/cypress/pages/ndv.ts @@ -24,6 +24,7 @@ export class NDV extends BasePage { editPinnedDataButton: () => cy.getByTestId('ndv-edit-pinned-data'), pinnedDataEditor: () => this.getters.outputPanel().find('.cm-editor .cm-scroller .cm-content'), runDataPaneHeader: () => cy.getByTestId('run-data-pane-header'), + aiOutputModeToggle: () => cy.getByTestId('ai-output-mode-select'), nodeOutputHint: () => cy.getByTestId('ndv-output-run-node-hint'), savePinnedDataButton: () => this.getters.runDataPaneHeader().find('button').filter(':visible').contains('Save'), diff --git a/packages/@n8n/nodes-langchain/nodes/tools/ToolCode/ToolCode.node.ts b/packages/@n8n/nodes-langchain/nodes/tools/ToolCode/ToolCode.node.ts index 492284b190c78..6ae5a8807539b 100644 --- a/packages/@n8n/nodes-langchain/nodes/tools/ToolCode/ToolCode.node.ts +++ b/packages/@n8n/nodes-langchain/nodes/tools/ToolCode/ToolCode.node.ts @@ -6,6 +6,7 @@ import type { SupplyData, ExecutionError, } from 'n8n-workflow'; + import { NodeConnectionType, NodeOperationError } from 'n8n-workflow'; import type { Sandbox } from 'n8n-nodes-base/dist/nodes/Code/Sandbox'; import { getSandboxContext } from 'n8n-nodes-base/dist/nodes/Code/Sandbox'; @@ -208,7 +209,7 @@ export class ToolCode implements INodeType { try { response = await runFunction(query); } catch (error: unknown) { - executionError = error as ExecutionError; + executionError = new NodeOperationError(this.getNode(), error as ExecutionError); response = `There was an error: "${executionError.message}"`; } @@ -229,6 +230,7 @@ export class ToolCode implements INodeType { } else { void this.addOutputData(NodeConnectionType.AiTool, index, [[{ json: { response } }]]); } + return response; }, }), diff --git a/packages/editor-ui/src/components/Error/NodeErrorView.vue b/packages/editor-ui/src/components/Error/NodeErrorView.vue index 5244371d0fc47..a0d6396df1416 100644 --- a/packages/editor-ui/src/components/Error/NodeErrorView.vue +++ b/packages/editor-ui/src/components/Error/NodeErrorView.vue @@ -21,6 +21,7 @@ import type { BaseTextKey } from '@/plugins/i18n'; type Props = { error: NodeError | NodeApiError | NodeOperationError; + compact?: boolean; }; const props = defineProps(); @@ -377,7 +378,7 @@ function copySuccess() { > -
+

{{ i18n.baseText('nodeErrorView.details.title') }} diff --git a/packages/editor-ui/src/components/OutputPanel.vue b/packages/editor-ui/src/components/OutputPanel.vue index 6d76c34295978..af1a7801f6652 100644 --- a/packages/editor-ui/src/components/OutputPanel.vue +++ b/packages/editor-ui/src/components/OutputPanel.vue @@ -28,6 +28,7 @@

+ + + + + diff --git a/packages/editor-ui/src/components/ButtonParameter/utils.ts b/packages/editor-ui/src/components/ButtonParameter/utils.ts new file mode 100644 index 0000000000000..14d3ca4d78334 --- /dev/null +++ b/packages/editor-ui/src/components/ButtonParameter/utils.ts @@ -0,0 +1,46 @@ +import type { Schema } from '@/Interface'; +import type { INodeExecutionData } from 'n8n-workflow'; +import { useWorkflowsStore } from '@/stores/workflows.store'; +import { useNDVStore } from '@/stores/ndv.store'; +import { useDataSchema } from '@/composables/useDataSchema'; +import { executionDataToJson } from '@/utils/nodeTypesUtils'; + +export function getParentNodes() { + const activeNode = useNDVStore().activeNode; + const { getCurrentWorkflow, getNodeByName } = useWorkflowsStore(); + const workflow = getCurrentWorkflow(); + + if (!activeNode || !workflow) return []; + + return workflow + .getParentNodesByDepth(activeNode?.name) + .filter(({ name }, i, nodes) => { + return name !== activeNode.name && nodes.findIndex((node) => node.name === name) === i; + }) + .map((n) => getNodeByName(n.name)) + .filter((n) => n !== null); +} + +export function getSchemas() { + const parentNodes = getParentNodes(); + const parentNodesNames = parentNodes.map((node) => node?.name); + const { getSchemaForExecutionData, getInputDataWithPinned } = useDataSchema(); + const parentNodesSchemas: Array<{ nodeName: string; schema: Schema }> = parentNodes + .map((node) => { + const inputData: INodeExecutionData[] = getInputDataWithPinned(node); + + return { + nodeName: node?.name || '', + schema: getSchemaForExecutionData(executionDataToJson(inputData), true), + }; + }) + .filter((node) => node.schema?.value.length > 0); + + const inputSchema = parentNodesSchemas.shift(); + + return { + parentNodesNames, + inputSchema, + parentNodesSchemas, + }; +} diff --git a/packages/editor-ui/src/components/CodeNodeEditor/CodeNodeEditor.vue b/packages/editor-ui/src/components/CodeNodeEditor/CodeNodeEditor.vue index d59a13e98ee60..f4ba1febf2471 100644 --- a/packages/editor-ui/src/components/CodeNodeEditor/CodeNodeEditor.vue +++ b/packages/editor-ui/src/components/CodeNodeEditor/CodeNodeEditor.vue @@ -60,13 +60,12 @@ import jsParser from 'prettier/plugins/babel'; import * as estree from 'prettier/plugins/estree'; import { type Ref, computed, nextTick, onBeforeUnmount, onMounted, ref, watch } from 'vue'; -import { ASK_AI_EXPERIMENT, CODE_NODE_TYPE } from '@/constants'; +import { CODE_NODE_TYPE } from '@/constants'; import { codeNodeEditorEventBus } from '@/event-bus'; import { useRootStore } from '@/stores/root.store'; import { usePostHog } from '@/stores/posthog.store'; import { useMessage } from '@/composables/useMessage'; -import { useSettingsStore } from '@/stores/settings.store'; import AskAI from './AskAI/AskAI.vue'; import { readOnlyEditorExtensions, writableEditorExtensions } from './baseExtensions'; import { useCompleter } from './completer'; @@ -114,7 +113,6 @@ const { autocompletionExtension } = useCompleter(() => props.mode, editor); const { createLinter } = useLinter(() => props.mode, editor); const rootStore = useRootStore(); -const settingsStore = useSettingsStore(); const posthog = usePostHog(); const i18n = useI18n(); const telemetry = useTelemetry(); @@ -191,13 +189,7 @@ onBeforeUnmount(() => { }); const aiEnabled = computed(() => { - const isAiExperimentEnabled = [ASK_AI_EXPERIMENT.gpt3, ASK_AI_EXPERIMENT.gpt4].includes( - (posthog.getVariant(ASK_AI_EXPERIMENT.name) ?? '') as string, - ); - - return ( - isAiExperimentEnabled && settingsStore.settings.ai.enabled && props.language === 'javaScript' - ); + return posthog.isAiEnabled() && props.language === 'javaScript'; }); const placeholder = computed(() => { diff --git a/packages/editor-ui/src/components/JsEditor/JsEditor.vue b/packages/editor-ui/src/components/JsEditor/JsEditor.vue index 40692ab32392c..5a5f2cb411878 100644 --- a/packages/editor-ui/src/components/JsEditor/JsEditor.vue +++ b/packages/editor-ui/src/components/JsEditor/JsEditor.vue @@ -1,5 +1,5 @@