diff --git a/.changeset/user-content-scopes-count-dataloader.md b/.changeset/user-content-scopes-count-dataloader.md new file mode 100644 index 00000000000..f7653a65345 --- /dev/null +++ b/.changeset/user-content-scopes-count-dataloader.md @@ -0,0 +1,7 @@ +--- +"@comet/cms-api": patch +--- + +Improve performance of the `contentScopesCount` field when querying the users list + +When resolving `contentScopesCount` for a list of users, the available content scopes were recomputed (and deduplicated) once per row. They are now resolved a single time per request through a request-scoped `DataLoader`, which noticeably speeds up the users list when many content scopes are configured. diff --git a/packages/api/cms-api/src/user-permissions/user-content-scopes-loader.service.ts b/packages/api/cms-api/src/user-permissions/user-content-scopes-loader.service.ts new file mode 100644 index 00000000000..2ea6b5a82f3 --- /dev/null +++ b/packages/api/cms-api/src/user-permissions/user-content-scopes-loader.service.ts @@ -0,0 +1,21 @@ +import { Injectable, Scope } from "@nestjs/common"; +import DataLoader from "dataloader"; + +import { ContentScope } from "./interfaces/content-scope.interface"; +import { User } from "./interfaces/user"; +import { UserPermissionsService } from "./user-permissions.service"; + +@Injectable({ scope: Scope.REQUEST }) +export class UserContentScopesLoaderService { + private dataLoader: DataLoader; + + constructor(private readonly userPermissionsService: UserPermissionsService) { + this.dataLoader = new DataLoader((users) => this.userPermissionsService.getContentScopesForUsers([...users]), { + cacheKeyFn: (user) => user.id, + }); + } + + load(user: User): Promise { + return this.dataLoader.load(user); + } +} diff --git a/packages/api/cms-api/src/user-permissions/user-permissions.module.ts b/packages/api/cms-api/src/user-permissions/user-permissions.module.ts index 1552a16c905..2fb09b63eac 100644 --- a/packages/api/cms-api/src/user-permissions/user-permissions.module.ts +++ b/packages/api/cms-api/src/user-permissions/user-permissions.module.ts @@ -10,6 +10,7 @@ import { UserContentScopes } from "./entities/user-content-scopes.entity"; import { UserPermission } from "./entities/user-permission.entity"; import { UserResolver } from "./user.resolver"; import { UserContentScopesResolver } from "./user-content-scopes.resolver"; +import { UserContentScopesLoaderService } from "./user-content-scopes-loader.service"; import { UserPermissionResolver } from "./user-permission.resolver"; import { ACCESS_CONTROL_SERVICE, USER_PERMISSIONS_OPTIONS, USER_PERMISSIONS_USER_SERVICE } from "./user-permissions.constants"; import { UserPermissionsPublicService } from "./user-permissions.public.service"; @@ -32,6 +33,7 @@ import { UserResolver, UserPermissionResolver, UserContentScopesResolver, + UserContentScopesLoaderService, ContentScopeService, { provide: APP_GUARD, diff --git a/packages/api/cms-api/src/user-permissions/user-permissions.service.spec.ts b/packages/api/cms-api/src/user-permissions/user-permissions.service.spec.ts new file mode 100644 index 00000000000..240404b08a4 --- /dev/null +++ b/packages/api/cms-api/src/user-permissions/user-permissions.service.spec.ts @@ -0,0 +1,60 @@ +import type { DiscoveryService } from "@golevelup/nestjs-discovery"; +import type { EntityRepository } from "@mikro-orm/postgresql"; +import { describe, expect, it, vi } from "vitest"; + +import type { UserContentScopes } from "./entities/user-content-scopes.entity"; +import type { UserPermission } from "./entities/user-permission.entity"; +import type { ContentScope } from "./interfaces/content-scope.interface"; +import type { User } from "./interfaces/user"; +import { UserPermissionsService } from "./user-permissions.service"; +import { type AccessControlServiceInterface, UserPermissions, type UserPermissionsOptions } from "./user-permissions.types"; + +function createService({ + availableContentScopes, + getContentScopesForUser, +}: { + availableContentScopes: ContentScope[]; + getContentScopesForUser?: AccessControlServiceInterface["getContentScopesForUser"]; +}) { + const options: UserPermissionsOptions = { availableContentScopes }; + const accessControlService: AccessControlServiceInterface = { isAllowed: () => true, getContentScopesForUser }; + const contentScopeRepository = { findOne: vi.fn().mockResolvedValue(null) } as unknown as EntityRepository; + const permissionRepository = {} as EntityRepository; + + return new UserPermissionsService(options, undefined, accessControlService, permissionRepository, contentScopeRepository, {} as DiscoveryService); +} + +describe("UserPermissionsService", () => { + const scopeA: ContentScope = { domain: "main", language: "en" }; + const scopeB: ContentScope = { domain: "secondary", language: "de" }; + + const userA: User = { id: "a", name: "User A", email: "a@example.com" }; + const userB: User = { id: "b", name: "User B", email: "b@example.com" }; + + describe("getContentScopesForUsers", () => { + it("computes the available content scopes only once for all users", async () => { + const service = createService({ + availableContentScopes: [scopeA, scopeB], + getContentScopesForUser: () => UserPermissions.allContentScopes, + }); + const getAvailableContentScopes = vi.spyOn(service, "getAvailableContentScopes"); + + const result = await service.getContentScopesForUsers([userA, userB]); + + expect(getAvailableContentScopes).toHaveBeenCalledTimes(1); + expect(result).toEqual([ + [scopeA, scopeB], + [scopeA, scopeB], + ]); + }); + + it("returns each user's content scopes in the order of the passed users", async () => { + const service = createService({ + availableContentScopes: [scopeA, scopeB], + getContentScopesForUser: (user) => (user.id === "a" ? [scopeA] : [scopeB]), + }); + + await expect(service.getContentScopesForUsers([userA, userB])).resolves.toEqual([[scopeA], [scopeB]]); + }); + }); +}); diff --git a/packages/api/cms-api/src/user-permissions/user-permissions.service.ts b/packages/api/cms-api/src/user-permissions/user-permissions.service.ts index 55acfa3136c..cb656395a59 100644 --- a/packages/api/cms-api/src/user-permissions/user-permissions.service.ts +++ b/packages/api/cms-api/src/user-permissions/user-permissions.service.ts @@ -206,8 +206,28 @@ export class UserPermissionsService { } async getContentScopes(user: User, includeContentScopesManual = true): Promise { - const contentScopes: ContentScope[] = []; const availableContentScopes = (await this.getAvailableContentScopes()).map((cs) => cs.scope); + return this.filterContentScopesForUser({ user, availableContentScopes, includeContentScopesManual }); + } + + // Resolves the content scopes for many users while computing the (shared) available content scopes only once. + // Used by the request-scoped UserContentScopesLoaderService so the contentScopesCount field resolver doesn't + // recompute the full available-scope list once per returned user. + async getContentScopesForUsers(users: User[], includeContentScopesManual = true): Promise { + const availableContentScopes = (await this.getAvailableContentScopes()).map((cs) => cs.scope); + return Promise.all(users.map((user) => this.filterContentScopesForUser({ user, availableContentScopes, includeContentScopesManual }))); + } + + private async filterContentScopesForUser({ + user, + availableContentScopes, + includeContentScopesManual, + }: { + user: User; + availableContentScopes: ContentScope[]; + includeContentScopesManual: boolean; + }): Promise { + const contentScopes: ContentScope[] = []; if (this.accessControlService.getContentScopesForUser) { const userContentScopes = await this.accessControlService.getContentScopesForUser(user); diff --git a/packages/api/cms-api/src/user-permissions/user.resolver.ts b/packages/api/cms-api/src/user-permissions/user.resolver.ts index d950065e306..5cb00f25b7d 100644 --- a/packages/api/cms-api/src/user-permissions/user.resolver.ts +++ b/packages/api/cms-api/src/user-permissions/user.resolver.ts @@ -9,6 +9,7 @@ import { CurrentUser } from "./dto/current-user"; import { FindUsersArgs, PermissionFilter } from "./dto/paginated-user-list"; import { UserPermissionsUser } from "./dto/user"; import { User } from "./interfaces/user"; +import { UserContentScopesLoaderService } from "./user-content-scopes-loader.service"; import { UserPermissionsService } from "./user-permissions.service"; @ObjectType() @@ -19,7 +20,10 @@ class UserPermissionPaginatedUserList extends PaginatedResponseFactory.create(Us export class UserResolver { private readonly logger = new Logger(UserResolver.name); - constructor(private readonly userService: UserPermissionsService) {} + constructor( + private readonly userService: UserPermissionsService, + private readonly userContentScopesLoader: UserContentScopesLoaderService, + ) {} @Query(() => UserPermissionsUser) async userPermissionsUserById(@Args("id", { type: () => String }) id: string): Promise { @@ -115,7 +119,7 @@ export class UserResolver { @ResolveField(() => Int) async contentScopesCount(@Parent() user: UserPermissionsUser): Promise { - return (await this.userService.getContentScopes(user)).length; + return (await this.userContentScopesLoader.load(user)).length; } @ResolveField(() => Boolean)