refactor(core): Decouple Insights from CLI and TypeORM (#40214)

This commit is contained in:
Charlie Kolb 2026-10-05 09:59:39 +00:00 committed by GitHub
parent 32d7c8ebf4
commit f47a2fe235
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
12 changed files with 113 additions and 128 deletions

View File

@ -351,7 +351,6 @@ export default defineConfig(
'./src/modules/dynamic-credentials.ee/services/credential-resolver.service.ts',
'./src/modules/external-secrets.ee/secrets-providers-connections.service.ee.ts',
'./src/modules/favorites/favorites.service.ts',
'./src/modules/insights/insights-collection.service.ts',
'./src/modules/instance-ai/instance-ai.adapter.service.ts',
'./src/modules/instance-ai/mcp/instance-ai-mcp-registry.service.ts',
'./src/modules/instance-ai/storage/typeorm-agent-checkpoint-store.ts',

View File

@ -2,7 +2,7 @@ import { createTeamProject, createWorkflow, testDb, testModules } from '@n8n/bac
import { Container } from '@n8n/di';
import { DateTime } from 'luxon';
import { InsightsRawRepository } from '@/modules/insights/database/repositories/insights-raw.repository';
import { InsightsRawRepository } from '../database/repositories/insights-raw.repository';
import {
createRawInsightsEvent,

View File

@ -8,17 +8,15 @@ import {
import type { Project, WorkflowEntity, IWorkflowDb, SharedWorkflowRepository } from '@n8n/db';
import type { WorkflowExecuteAfterContext } from '@n8n/decorators';
import { Container } from '@n8n/di';
import { In } from '@n8n/typeorm';
import { createDeferredPromise } from '@n8n/utils/promise/deferred-promise';
import { DateTime } from 'luxon';
import { type ExecutionStatus, type IRun, type WorkflowExecuteMode } from 'n8n-workflow';
import assert from 'node:assert';
import { mock } from 'vitest-mock-extended';
import type { TypeUnit } from '@/modules/insights/database/entities/insights-shared';
import { InsightsMetadataRepository } from '@/modules/insights/database/repositories/insights-metadata.repository';
import { InsightsRawRepository } from '@/modules/insights/database/repositories/insights-raw.repository';
import type { TypeUnit } from '../database/entities/insights-shared';
import { InsightsMetadataRepository } from '../database/repositories/insights-metadata.repository';
import { InsightsRawRepository } from '../database/repositories/insights-raw.repository';
import { InsightsCollectionService } from '../insights-collection.service';
import { InsightsConfig } from '../insights.config';
@ -283,9 +281,9 @@ describe('workflowExecuteAfterHandler - cacheMetadata', () => {
// Mock the repositories functions
const repositoryMocks = {
find: vi.fn(),
findBy: vi.fn(),
upsert: vi.fn(),
findOwnerProjectsByWorkflowIds: vi.fn(),
findByWorkflowIds: vi.fn(),
upsertWorkflowMetadata: vi.fn(),
insert: vi.fn(),
};
const sharedWorkflowRepositoryMock = mock<SharedWorkflowRepository>(repositoryMocks);
@ -319,15 +317,12 @@ describe('workflowExecuteAfterHandler - cacheMetadata', () => {
project = await createTeamProject();
workflow = await createWorkflow({}, project);
repositoryMocks.find = vi.fn().mockResolvedValue([
{
workflow,
workflowId: workflow.id,
projectId: 'project-id',
project: { name: 'project-name' },
},
]);
repositoryMocks.findBy = vi.fn().mockResolvedValue([
repositoryMocks.findOwnerProjectsByWorkflowIds.mockReset();
repositoryMocks.findOwnerProjectsByWorkflowIds.mockResolvedValue(
new Map([[workflow.id, mock<Project>({ id: 'project-id', name: 'project-name' })]]),
);
repositoryMocks.findByWorkflowIds.mockReset();
repositoryMocks.findByWorkflowIds.mockResolvedValue([
{
metaId: 'meta-id',
workflowId: workflow.id,
@ -336,6 +331,8 @@ describe('workflowExecuteAfterHandler - cacheMetadata', () => {
projectName: 'project-name',
},
]);
repositoryMocks.upsertWorkflowMetadata.mockClear();
repositoryMocks.insert.mockClear();
});
test('reuses cached metadata for subsequent executions of the same workflow', async () => {
@ -350,11 +347,8 @@ describe('workflowExecuteAfterHandler - cacheMetadata', () => {
await insightsCollectionService.flushEvents();
// ASSERT
expect(repositoryMocks.find).toHaveBeenCalledWith({
where: { workflowId: In([workflow.id]), role: 'workflow:owner' },
relations: { project: true },
});
expect(repositoryMocks.upsert).toHaveBeenCalledWith(
expect(repositoryMocks.findOwnerProjectsByWorkflowIds).toHaveBeenCalledWith([workflow.id]);
expect(repositoryMocks.upsertWorkflowMetadata).toHaveBeenCalledWith(
expect.arrayContaining([
{
workflowId: workflow.id,
@ -363,18 +357,21 @@ describe('workflowExecuteAfterHandler - cacheMetadata', () => {
projectName: 'project-name',
},
]),
['workflowId'],
);
repositoryMocks.findOwnerProjectsByWorkflowIds.mockClear();
repositoryMocks.findByWorkflowIds.mockClear();
repositoryMocks.upsertWorkflowMetadata.mockClear();
// ACT AGAIN with the same workflow
await insightsCollectionService.handleWorkflowExecuteAfter(ctx);
await insightsCollectionService.flushEvents();
// ASSERT AGAIN
repositoryMocks.find.mockClear();
repositoryMocks.upsert.mockClear();
expect(repositoryMocks.find).not.toHaveBeenCalled();
expect(repositoryMocks.upsert).not.toHaveBeenCalled();
expect(repositoryMocks.findOwnerProjectsByWorkflowIds).toHaveBeenCalledOnce();
expect(repositoryMocks.findOwnerProjectsByWorkflowIds).toHaveBeenCalledWith([workflow.id]);
expect(repositoryMocks.findByWorkflowIds).not.toHaveBeenCalled();
expect(repositoryMocks.upsertWorkflowMetadata).not.toHaveBeenCalled();
});
test('updates cached metadata if workflow details change', async () => {
@ -386,8 +383,8 @@ describe('workflowExecuteAfterHandler - cacheMetadata', () => {
await insightsCollectionService.flushEvents();
// ASSERT
expect(repositoryMocks.find).toHaveBeenCalled();
expect(repositoryMocks.upsert).toHaveBeenCalled();
expect(repositoryMocks.findOwnerProjectsByWorkflowIds).toHaveBeenCalled();
expect(repositoryMocks.upsertWorkflowMetadata).toHaveBeenCalled();
// Change the workflow name
workflow.name = 'new-workflow-name';
@ -397,11 +394,8 @@ describe('workflowExecuteAfterHandler - cacheMetadata', () => {
await insightsCollectionService.flushEvents();
// ASSERT AGAIN
expect(repositoryMocks.find).toHaveBeenCalledWith({
where: { workflowId: In([workflow.id]), role: 'workflow:owner' },
relations: { project: true },
});
expect(repositoryMocks.upsert).toHaveBeenCalledWith(
expect(repositoryMocks.findOwnerProjectsByWorkflowIds).toHaveBeenCalledWith([workflow.id]);
expect(repositoryMocks.upsertWorkflowMetadata).toHaveBeenCalledWith(
expect.arrayContaining([
{
workflowId: workflow.id,
@ -410,7 +404,6 @@ describe('workflowExecuteAfterHandler - cacheMetadata', () => {
projectName: 'project-name',
},
]),
['workflowId'],
);
});
});
@ -427,11 +420,11 @@ describe('workflowExecuteAfterHandler - flushEvents', () => {
insertInsightsRaw: vi.fn(),
};
const sharedWorkflowRepositoryMock = mock<SharedWorkflowRepository>({
find: repoMocks.findSharedWorkflowRepositoryMock,
findOwnerProjectsByWorkflowIds: repoMocks.findSharedWorkflowRepositoryMock,
});
const metadataRepositoryMock = mock<InsightsMetadataRepository>({
findBy: repoMocks.findByMetadata,
upsert: repoMocks.upsertMetadata,
findByWorkflowIds: repoMocks.findByMetadata,
upsertWorkflowMetadata: repoMocks.upsertMetadata,
});
const insightsRawRepositoryMock = mock<InsightsRawRepository>({
insert: repoMocks.insertInsightsRaw,
@ -459,14 +452,9 @@ describe('workflowExecuteAfterHandler - flushEvents', () => {
beforeEach(async () => {
project = await createTeamProject();
workflow = await createWorkflow({ settings: { timeSavedPerExecution: 1 } }, project);
repoMocks.findSharedWorkflowRepositoryMock.mockResolvedValue([
{
workflow,
workflowId: workflow.id,
projectId: 'project-id',
project: { name: 'project-name' },
},
]);
repoMocks.findSharedWorkflowRepositoryMock.mockResolvedValue(
new Map([[workflow.id, mock<Project>({ id: 'project-id', name: 'project-name' })]]),
);
repoMocks.findByMetadata.mockResolvedValue([
{
metaId: 'meta-id',

View File

@ -1,11 +1,5 @@
import { mockLogger } from '@n8n/backend-test-utils';
import type {
Project,
SharedWorkflow,
SharedWorkflowRepository,
IWorkflowDb,
WorkflowEntity,
} from '@n8n/db';
import type { Project, SharedWorkflowRepository, IWorkflowDb, WorkflowEntity } from '@n8n/db';
import type { WorkflowExecuteAfterContext } from '@n8n/decorators';
import { DateTime } from 'luxon';
import type { IRun } from 'n8n-workflow';
@ -90,15 +84,11 @@ describe('initialization safeguards', () => {
});
// mock shared workflow repository for the flushing process
sharedWorkflowRepository.find.mockResolvedValueOnce([
mock<SharedWorkflow>({
workflow,
project: mock<Project>(),
role: 'workflow:editor',
}),
]);
sharedWorkflowRepository.findOwnerProjectsByWorkflowIds.mockResolvedValueOnce(
new Map([[workflow.id, mock<Project>()]]),
);
// mock insights metadata repository for the flushing process
insightsMetadataRepository.findBy.mockResolvedValueOnce([
insightsMetadataRepository.findByWorkflowIds.mockResolvedValueOnce([
mock<InsightsMetadata>({ workflowId: ctx.workflow.id, metaId: 1 }),
]);
@ -285,15 +275,10 @@ describe('flushEvents with an insight that has no shared workflow', () => {
service.init();
// Only "known-workflow" has an owner row, so "gone-workflow" gets no metadata.
sharedWorkflowRepository.find.mockResolvedValue([
mock<SharedWorkflow>({
workflowId: 'known-workflow',
projectId: 'project-id',
project: mock<Project>({ name: 'Project' }),
role: 'workflow:owner',
}),
]);
insightsMetadataRepository.findBy.mockResolvedValue([
sharedWorkflowRepository.findOwnerProjectsByWorkflowIds.mockResolvedValue(
new Map([['known-workflow', mock<Project>({ id: 'project-id', name: 'Project' })]]),
);
insightsMetadataRepository.findByWorkflowIds.mockResolvedValue([
mock<InsightsMetadata>({ workflowId: 'known-workflow', metaId: 1 }),
]);
});
@ -326,8 +311,8 @@ describe('flushEvents with an insight that has no shared workflow', () => {
});
test('does not re-buffer the event on the next flush', async () => {
sharedWorkflowRepository.find.mockResolvedValue([]);
insightsMetadataRepository.findBy.mockResolvedValue([]);
sharedWorkflowRepository.findOwnerProjectsByWorkflowIds.mockResolvedValue(new Map());
insightsMetadataRepository.findByWorkflowIds.mockResolvedValue([]);
bufferEvent('gone-workflow');
await service.flushEvents();

View File

@ -5,7 +5,7 @@ import { Container } from '@n8n/di';
import { DateTime } from 'luxon';
import { mock } from 'vitest-mock-extended';
import { InsightsRawRepository } from '@/modules/insights/database/repositories/insights-raw.repository';
import { InsightsRawRepository } from '../database/repositories/insights-raw.repository';
import {
createMetadata,

View File

@ -48,6 +48,7 @@ describe('InsightsModule', () => {
mockInstanceSettings,
Container.get(Logger),
mock(),
mock(),
),
);
insightsModule = Container.get(InsightsModule);

View File

@ -19,7 +19,7 @@ import type { MockProxy } from 'vitest-mock-extended';
import { mock } from 'vitest-mock-extended';
import { ForbiddenError } from '@n8n/errors';
import type { WorkflowSharingService } from '@n8n/backend-services';
import type { ProjectScopeService, WorkflowSharingService } from '@n8n/backend-services';
import { createMember } from '@test-integration/db/users';
import { createCompactedInsightsEvent } from '../database/entities/__tests__/db-utils';
@ -69,6 +69,7 @@ describe('InsightsService (Integration)', () => {
instanceSettings,
mockLogger(),
mock<WorkflowSharingService>(),
mock<ProjectScopeService>(),
);
// Get the real service from the container and spy on it
@ -1629,6 +1630,7 @@ describe('InsightsService (Integration)', () => {
mock<InstanceSettings>(),
mockLogger(),
mock<WorkflowSharingService>(),
mock<ProjectScopeService>(),
);
});
@ -1728,6 +1730,7 @@ describe('InsightsService (Integration)', () => {
mock<InstanceSettings>({ instanceType: 'main' }),
mockLogger(),
mock<WorkflowSharingService>(),
mock<ProjectScopeService>(),
);
});

View File

@ -5,17 +5,12 @@ import type { InstanceSettings } from 'n8n-core';
import type { MockProxy } from 'vitest-mock-extended';
import { mock } from 'vitest-mock-extended';
import { userHasScopes } from '@/permissions.ee/check-access';
import type { WorkflowSharingService } from '@n8n/backend-services';
import type { ProjectScopeService, WorkflowSharingService } from '@n8n/backend-services';
import { TypeToNumber, type TypeUnitNumber } from '../database/entities/insights-shared';
import type { InsightsByPeriodRepository } from '../database/repositories/insights-by-period.repository';
import { InsightsService } from '../insights.service';
vi.mock('@/permissions.ee/check-access', () => ({
userHasScopes: vi.fn(),
}));
const user = mock<User>({ id: 'user-1' });
describe('InsightsService', () => {
@ -25,6 +20,7 @@ describe('InsightsService', () => {
let mockLicenseState: MockProxy<LicenseState>;
let mockInstanceSettings: MockProxy<InstanceSettings>;
let mockWorkflowSharingService: MockProxy<WorkflowSharingService>;
let mockProjectScopeService: MockProxy<ProjectScopeService>;
beforeEach(() => {
vi.clearAllMocks();
@ -33,7 +29,8 @@ describe('InsightsService', () => {
mockLicenseState = mock<LicenseState>();
mockInstanceSettings = mock<InstanceSettings>();
mockWorkflowSharingService = mock<WorkflowSharingService>();
vi.mocked(userHasScopes).mockResolvedValue(true);
mockProjectScopeService = mock<ProjectScopeService>();
mockProjectScopeService.getProjectIds.mockResolvedValue(null);
insightsService = new InsightsService(
mockInsightsByPeriodRepository,
@ -41,6 +38,7 @@ describe('InsightsService', () => {
mockInstanceSettings,
mockLogger(),
mockWorkflowSharingService,
mockProjectScopeService,
);
});
@ -869,11 +867,11 @@ describe('InsightsService', () => {
it('should not check project access when no project is requested', async () => {
await insightsService.getInsightsSummary({ user, startDate, endDate });
expect(userHasScopes).not.toHaveBeenCalled();
expect(mockProjectScopeService.getProjectIds).not.toHaveBeenCalled();
});
it('should throw a forbidden error when the requested project is not accessible', async () => {
vi.mocked(userHasScopes).mockResolvedValue(false);
mockProjectScopeService.getProjectIds.mockResolvedValue([]);
await expect(
insightsService.getInsightsSummary({ user, startDate, endDate, projectId: 'project-1' }),
@ -892,9 +890,7 @@ describe('InsightsService', () => {
projectId: 'project-1',
});
expect(userHasScopes).toHaveBeenCalledWith(user, ['workflow:read'], false, {
projectId: 'project-1',
});
expect(mockProjectScopeService.getProjectIds).toHaveBeenCalledWith(user, ['workflow:read']);
expect(
mockInsightsByPeriodRepository.getPreviousAndCurrentPeriodTypeAggregates,
).toHaveBeenCalledWith(expect.objectContaining({ projectId: 'project-1' }));
@ -1045,11 +1041,11 @@ describe('InsightsService', () => {
it('should not check project access when no project is requested', async () => {
await insightsService.getInsightsByWorkflow({ user, startDate, endDate });
expect(userHasScopes).not.toHaveBeenCalled();
expect(mockProjectScopeService.getProjectIds).not.toHaveBeenCalled();
});
it('should throw a forbidden error when the requested project is not accessible', async () => {
vi.mocked(userHasScopes).mockResolvedValue(false);
mockProjectScopeService.getProjectIds.mockResolvedValue([]);
await expect(
insightsService.getInsightsByWorkflow({
@ -1106,11 +1102,11 @@ describe('InsightsService', () => {
it('should not check project access when no project is requested', async () => {
await insightsService.getInsightsByTime({ user, startDate, endDate });
expect(userHasScopes).not.toHaveBeenCalled();
expect(mockProjectScopeService.getProjectIds).not.toHaveBeenCalled();
});
it('should throw a forbidden error when the requested project is not accessible', async () => {
vi.mocked(userHasScopes).mockResolvedValue(false);
mockProjectScopeService.getProjectIds.mockResolvedValue([]);
await expect(
insightsService.getInsightsByTime({

View File

@ -14,7 +14,7 @@ import { DataSource } from '@n8n/typeorm';
import { sleep } from '@n8n/utils/sleep';
import { DateTime } from 'luxon';
import { InsightsConfig } from '@/modules/insights/insights.config';
import { InsightsConfig } from '../../../insights.config';
import { createMember } from '@test-integration/db/users';
import {

View File

@ -1,5 +1,5 @@
import { Service } from '@n8n/di';
import { DataSource, Repository } from '@n8n/typeorm';
import { DataSource, In, Repository } from '@n8n/typeorm';
import { InsightsMetadata } from '../entities/insights-metadata';
@ -8,4 +8,16 @@ export class InsightsMetadataRepository extends Repository<InsightsMetadata> {
constructor(dataSource: DataSource) {
super(InsightsMetadata, dataSource.manager);
}
async upsertWorkflowMetadata(metadata: InsightsMetadata[]) {
if (metadata.length === 0) return;
await this.upsert(metadata, ['workflowId']);
}
async findByWorkflowIds(workflowIds: string[]) {
if (workflowIds.length === 0) return [];
return await this.findBy({ workflowId: In(workflowIds) });
}
}

View File

@ -3,13 +3,11 @@ import { isBillableExecution } from '@n8n/backend-services';
import { SharedWorkflowRepository } from '@n8n/db';
import { OnLifecycleEvent, type WorkflowExecuteAfterContext } from '@n8n/decorators';
import { Service } from '@n8n/di';
import { In } from '@n8n/typeorm';
import { DateTime } from 'luxon';
import { IRun, type ExecutionStatus, type WorkflowExecuteMode } from 'n8n-workflow';
import { InsightsMetadata } from '@/modules/insights/database/entities/insights-metadata';
import { InsightsRaw } from '@/modules/insights/database/entities/insights-raw';
import { InsightsMetadata } from './database/entities/insights-metadata';
import { InsightsRaw } from './database/entities/insights-raw';
import { InsightsMetadataRepository } from './database/repositories/insights-metadata.repository';
import { InsightsRawRepository } from './database/repositories/insights-raw.repository';
import { InsightsConfig } from './insights.config';
@ -233,40 +231,44 @@ export class InsightsCollectionService {
workflowIdNames.set(event.workflowId, event.workflowName);
}
const sharedWorkflows = await this.sharedWorkflowRepository.find({
where: { workflowId: In([...workflowIdNames.keys()]), role: 'workflow:owner' },
relations: { project: true },
});
const ownerProjects = await this.sharedWorkflowRepository.findOwnerProjectsByWorkflowIds([
...workflowIdNames.keys(),
]);
// Upsert metadata for the workflows that are not already in the cache or have
// different project or workflow names
const metadataToUpsert = sharedWorkflows.reduce((acc, workflow) => {
const cachedMetadata = this.cachedMetadata.get(workflow.workflowId);
const metadataToUpsert: InsightsMetadata[] = [];
for (const [workflowId, workflowName] of workflowIdNames) {
const project = ownerProjects.get(workflowId);
if (!project) continue;
const cachedMetadata = this.cachedMetadata.get(workflowId);
if (
!cachedMetadata ||
cachedMetadata.projectId !== workflow.projectId ||
cachedMetadata.projectName !== workflow.project.name ||
cachedMetadata.workflowName !== workflowIdNames.get(workflow.workflowId)
cachedMetadata.projectId !== project.id ||
cachedMetadata.projectName !== project.name ||
cachedMetadata.workflowName !== workflowName
) {
const metadata = new InsightsMetadata();
metadata.projectId = workflow.projectId;
metadata.projectName = workflow.project.name;
metadata.workflowId = workflow.workflowId;
metadata.workflowName = workflowIdNames.get(workflow.workflowId)!;
metadata.projectId = project.id;
metadata.projectName = project.name;
metadata.workflowId = workflowId;
metadata.workflowName = workflowName;
acc.push(metadata);
metadataToUpsert.push(metadata);
}
return acc;
}, [] as InsightsMetadata[]);
}
this.logger.debug(`Saving ${metadataToUpsert.length} insights metadata for workflows`);
await this.insightsMetadataRepository.upsert(metadataToUpsert, ['workflowId']);
if (metadataToUpsert.length > 0) {
this.logger.debug(`Saving ${metadataToUpsert.length} insights metadata for workflows`);
await this.insightsMetadataRepository.upsertWorkflowMetadata(metadataToUpsert);
const upsertMetadata = await this.insightsMetadataRepository.findBy({
workflowId: In(metadataToUpsert.map((m) => m.workflowId)),
});
for (const metadata of upsertMetadata) {
this.cachedMetadata.set(metadata.workflowId, metadata);
const upsertMetadata = await this.insightsMetadataRepository.findByWorkflowIds(
metadataToUpsert.map((metadata) => metadata.workflowId),
);
for (const metadata of upsertMetadata) {
this.cachedMetadata.set(metadata.workflowId, metadata);
}
}
const events: InsightsRaw[] = [];

View File

@ -4,7 +4,7 @@ import {
type RestrictedInsightsByTime,
} from '@n8n/api-types';
import { LicenseState, Logger } from '@n8n/backend-common';
import { WorkflowSharingService } from '@n8n/backend-services';
import { ProjectScopeService, WorkflowSharingService } from '@n8n/backend-services';
import type { User } from '@n8n/db';
import { Container, Service } from '@n8n/di';
import { DateTime } from 'luxon';
@ -12,8 +12,6 @@ import { InstanceSettings } from 'n8n-core';
import { UserError } from 'n8n-workflow';
import { ForbiddenError } from '@n8n/errors';
import { userHasScopes } from '@/permissions.ee/check-access';
import type { PeriodUnit, TypeUnit, ByTimeInsightType } from './database/entities/insights-shared';
import { NumberToType } from './database/entities/insights-shared';
import type { InsightsAccessFilter } from './database/repositories/insights-by-period.repository';
@ -42,6 +40,7 @@ export class InsightsService {
private readonly instanceSettings: InstanceSettings,
private readonly logger: Logger,
private readonly workflowSharingService: WorkflowSharingService,
private readonly projectScopeService: ProjectScopeService,
) {
this.logger = this.logger.scoped('insights');
}
@ -85,10 +84,10 @@ export class InsightsService {
projectId?: string,
): Promise<InsightsAccessFilter | undefined> {
if (projectId) {
const userHasRequiredProjectScopes = await userHasScopes(user, ['workflow:read'], false, {
projectId,
});
if (!userHasRequiredProjectScopes) {
const accessibleProjectIds = await this.projectScopeService.getProjectIds(user, [
'workflow:read',
]);
if (accessibleProjectIds !== null && !accessibleProjectIds.includes(projectId)) {
throw new ForbiddenError('You do not have access to insights for this project.');
}
}