diff --git a/packages/cli/src/services/__tests__/ownership.service.test.ts b/packages/cli/src/services/__tests__/ownership.service.test.ts index ee6922dd76..6e79c83bb2 100644 --- a/packages/cli/src/services/__tests__/ownership.service.test.ts +++ b/packages/cli/src/services/__tests__/ownership.service.test.ts @@ -1,4 +1,5 @@ import { mock } from 'jest-mock-extended'; +import { v4 as uuid } from 'uuid'; import { Project } from '@/databases/entities/project'; import { ProjectRelation } from '@/databases/entities/project-relation'; @@ -13,12 +14,15 @@ import { OwnershipService } from '@/services/ownership.service'; import { mockCredential, mockProject } from '@test/mock-objects'; import { mockInstance } from '@test/mocking'; +import { CacheService } from '../cache/cache.service'; + describe('OwnershipService', () => { const userRepository = mockInstance(UserRepository); const sharedWorkflowRepository = mockInstance(SharedWorkflowRepository); const projectRelationRepository = mockInstance(ProjectRelationRepository); + const cacheService = mockInstance(CacheService); const ownershipService = new OwnershipService( - mock(), + cacheService, userRepository, mock(), projectRelationRepository, @@ -52,22 +56,22 @@ describe('OwnershipService', () => { }); }); - describe('getProjectOwnerCached()', () => { + describe('getPersonalProjectOwnerCached()', () => { test('should retrieve a project owner', async () => { - const mockProject = new Project(); - const mockOwner = new User(); - - const projectRelation = Object.assign(new ProjectRelation(), { - role: 'project:personalOwner', - project: mockProject, - user: mockOwner, - }); + // ARRANGE + const project = new Project(); + const owner = new User(); + const projectRelation = new ProjectRelation(); + projectRelation.role = 'project:personalOwner'; + (projectRelation.project = project), (projectRelation.user = owner); projectRelationRepository.getPersonalProjectOwners.mockResolvedValueOnce([projectRelation]); + // ACT const returnedOwner = await ownershipService.getPersonalProjectOwnerCached('some-project-id'); - expect(returnedOwner).toBe(mockOwner); + // ASSERT + expect(returnedOwner).toBe(owner); }); test('should not throw if no project owner found, should return null instead', async () => { @@ -77,6 +81,29 @@ describe('OwnershipService', () => { expect(owner).toBeNull(); }); + + test('should not use the repository if the owner was found in the cache', async () => { + // ARRANGE + const project = new Project(); + project.id = uuid(); + const owner = new User(); + owner.id = uuid(); + const projectRelation = new ProjectRelation(); + projectRelation.role = 'project:personalOwner'; + (projectRelation.project = project), (projectRelation.user = owner); + + cacheService.getHashValue.mockResolvedValueOnce(owner); + userRepository.create.mockReturnValueOnce(owner); + + // ACT + const foundOwner = await ownershipService.getPersonalProjectOwnerCached(project.id); + + // ASSERT + expect(cacheService.getHashValue).toHaveBeenCalledTimes(1); + expect(cacheService.getHashValue).toHaveBeenCalledWith('project-owner', project.id); + expect(projectRelationRepository.getPersonalProjectOwners).not.toHaveBeenCalled(); + expect(foundOwner).toEqual(owner); + }); }); describe('getProjectOwnerCached()', () => { diff --git a/packages/cli/src/services/ownership.service.ts b/packages/cli/src/services/ownership.service.ts index 65e83e53eb..d0ad442bc1 100644 --- a/packages/cli/src/services/ownership.service.ts +++ b/packages/cli/src/services/ownership.service.ts @@ -45,13 +45,9 @@ export class OwnershipService { * Personal project ownership is **immutable**. */ async getPersonalProjectOwnerCached(projectId: string): Promise { - const cachedValue = await this.cacheService.getHashValue( - 'project-owner', - projectId, - ); + const cachedValue = await this.cacheService.getHashValue('project-owner', projectId); - if (cachedValue) this.userRepository.create(cachedValue); - if (cachedValue === null) return null; + if (cachedValue) return this.userRepository.create(cachedValue); const ownerRel = await this.projectRelationRepository.getPersonalProjectOwners([projectId]); const owner = ownerRel[0]?.user ?? null; diff --git a/packages/cli/src/subworkflows/subworkflow-policy-checker.service.ts b/packages/cli/src/subworkflows/subworkflow-policy-checker.service.ts index cf9b122e72..6c64fc0b3a 100644 --- a/packages/cli/src/subworkflows/subworkflow-policy-checker.service.ts +++ b/packages/cli/src/subworkflows/subworkflow-policy-checker.service.ts @@ -1,6 +1,5 @@ import { GlobalConfig } from '@n8n/config'; import { type Workflow, type INode, type WorkflowSettings } from 'n8n-workflow'; -import { strict as assert } from 'node:assert'; import { Service } from 'typedi'; import type { Project } from '@/databases/entities/project'; @@ -68,11 +67,9 @@ export class SubworkflowPolicyChecker { const owner = await this.ownershipService.getPersonalProjectOwnerCached(subworkflowProject.id); - assert(owner !== null); // only `null` if not personal - return { hasReadAccess, - ownerName: owner.firstName + ' ' + owner.lastName, + ownerName: owner ? owner.firstName + ' ' + owner.lastName : 'No owner (team project)', }; }