From 7af94ec35e4614ac89549a544352fd2ceba1ba42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bruno=20Zori=C4=87?= Date: Fri, 27 Oct 2023 09:16:54 +0200 Subject: [PATCH] fix(api-headless-cms): models and group loaders (#3643) --- .../contentAPI/contentModel.crud.test.ts | 4 ++ .../src/crud/contentModel.crud.ts | 63 +++++++++++++------ .../src/crud/contentModelGroup.crud.ts | 60 +++++++++++------- packages/api-headless-cms/src/types.ts | 1 - .../api-headless-cms/src/utils/filterAsync.ts | 20 +++--- packages/api-security/src/createSecurity.ts | 3 + packages/api-security/src/types.ts | 2 + 7 files changed, 98 insertions(+), 55 deletions(-) diff --git a/packages/api-headless-cms/__tests__/contentAPI/contentModel.crud.test.ts b/packages/api-headless-cms/__tests__/contentAPI/contentModel.crud.test.ts index 61ffc91baa1..8456c5c0a26 100644 --- a/packages/api-headless-cms/__tests__/contentAPI/contentModel.crud.test.ts +++ b/packages/api-headless-cms/__tests__/contentAPI/contentModel.crud.test.ts @@ -965,6 +965,10 @@ describe("content model test", () => { const { listContentModelsQuery: listModels } = useGraphQLHandler({ ...manageHandlerOpts, + identity: { + ...helpers.identity, + id: "identityWithSpecificModelPermissions" + }, permissions: createPermissions({ models: [createdContentModels[0].modelId] }) }); diff --git a/packages/api-headless-cms/src/crud/contentModel.crud.ts b/packages/api-headless-cms/src/crud/contentModel.crud.ts index 1db85bcdbf9..dd2a6a75334 100644 --- a/packages/api-headless-cms/src/crud/contentModel.crud.ts +++ b/packages/api-headless-cms/src/crud/contentModel.crud.ts @@ -104,10 +104,12 @@ export const createModelsCrud = (params: CreateModelsCrudParams): CmsModelContex }) }; + const listModelsCache = new Map>(); const clearModelsCache = (): void => { for (const loader of Object.values(loaders)) { loader.clearAll(); } + listModelsCache.clear(); }; const managers = new Map(); @@ -183,30 +185,49 @@ export const createModelsCrud = (params: CreateModelsCrudParams): CmsModelContex const modelsList = async (): Promise => { const databaseModels: CmsModel[] = await loaders.listModels.load("listModels"); - const pluginsModels = getModelsAsPlugins(); - return databaseModels.concat(pluginsModels); }; - + /** + * The list models cache is a key -> Promise pair so it the listModels() can be called multiple times but executed only once. + */ const listModels = async () => { - return context.benchmark.measure("headlessCms.crud.models.listModels", async () => { - const models = await modelsList(); - return filterAsync(models, async model => { - const ownsModel = await modelsPermissions.ensure( - { owns: model.createdBy }, - { throw: false } - ); - - if (!ownsModel) { - return false; - } + /** + * Maybe we can cache based on permissions, not the identity id? + * + * TODO: @adrian please check if possible. + */ + const key = JSON.stringify({ + tenant: getTenant().id, + locale: getLocale().code, + identity: context.security.isAuthorizationEnabled() ? getIdentity()?.id : undefined + }); + if (listModelsCache.has(key)) { + return listModelsCache.get(key) as Promise; + } + const cachedModelList = async () => { + return context.benchmark.measure("headlessCms.crud.models.listModels", async () => { + const models = await modelsList(); + return filterAsync(models, async model => { + const ownsModel = await modelsPermissions.ensure( + { owns: model.createdBy }, + { throw: false } + ); + + if (!ownsModel) { + return false; + } - return modelsPermissions.canAccessModel({ - model + return modelsPermissions.canAccessModel({ + model + }); }); }); - }); + }; + + listModelsCache.set(key, cachedModelList()); + + return listModelsCache.get(key) as Promise; }; const getModel = async (modelId: string): Promise => { @@ -375,7 +396,7 @@ export const createModelsCrud = (params: CreateModelsCrudParams): CmsModelContex model }); - loaders.listModels.clearAll(); + clearModelsCache(); await updateManager(context, model); @@ -507,7 +528,7 @@ export const createModelsCrud = (params: CreateModelsCrudParams): CmsModelContex await updateManager(context, resultModel); - loaders.listModels.clearAll(); + clearModelsCache(); await onModelAfterUpdate.publish({ input: {} as CmsModelUpdateInput, @@ -595,7 +616,7 @@ export const createModelsCrud = (params: CreateModelsCrudParams): CmsModelContex model }); - loaders.listModels.clearAll(); + clearModelsCache(); await updateManager(context, model); @@ -641,6 +662,8 @@ export const createModelsCrud = (params: CreateModelsCrudParams): CmsModelContex ); } + clearModelsCache(); + await onModelAfterDelete.publish({ model }); diff --git a/packages/api-headless-cms/src/crud/contentModelGroup.crud.ts b/packages/api-headless-cms/src/crud/contentModelGroup.crud.ts index aae7868d9bb..e5d441170cb 100644 --- a/packages/api-headless-cms/src/crud/contentModelGroup.crud.ts +++ b/packages/api-headless-cms/src/crud/contentModelGroup.crud.ts @@ -4,7 +4,6 @@ import { CmsContext, CmsGroup, CmsGroupContext, - CmsGroupListParams, HeadlessCmsStorageOperations, OnGroupAfterCreateTopicParams, OnGroupAfterDeleteTopicParams, @@ -76,6 +75,7 @@ export const createModelGroupsCrud = (params: CreateModelGroupsCrudParams): CmsG }) }; + const listGroupsCache = new Map>(); const clearGroupsCache = (): void => { for (const loader of Object.values(dataLoaders)) { loader.clearAll(); @@ -124,15 +124,12 @@ export const createModelGroupsCrud = (params: CreateModelGroupsCrudParams): CmsG return group; }; - const listGroupsViaDataLoader = async (params: CmsGroupListParams) => { - const { where } = params || {}; - + const listGroupsViaDataLoader = async () => { try { return await dataLoaders.listGroups.load("listGroups"); } catch (ex) { throw new WebinyError(ex.message, ex.code || "LIST_ERROR", { - ...(ex.data || {}), - where + ...(ex.data || {}) }); } }; @@ -191,6 +188,7 @@ export const createModelGroupsCrud = (params: CreateModelGroupsCrudParams): CmsG return group; }; + const listGroups: CmsGroupContext["listGroups"] = async params => { const { where } = params || {}; @@ -198,30 +196,44 @@ export const createModelGroupsCrud = (params: CreateModelGroupsCrudParams): CmsG await modelGroupsPermissions.ensure({ rwd: "r" }); - const response = await listGroupsViaDataLoader({ - ...(params || {}), - where: { - ...(where || {}), - tenant: tenant || getTenant().id, - locale: locale || getLocale().code - } + /** + * Maybe we can cache based on permissions, not the identity id? + * + * TODO: @adrian please check if possible. + */ + const key = JSON.stringify({ + tenant: tenant || getTenant().id, + locale: locale || getLocale().code, + identity: context.security.isAuthorizationEnabled() ? getIdentity()?.id : undefined }); + if (listGroupsCache.has(key)) { + return listGroupsCache.get(key) as Promise; + } - return filterAsync(response, async group => { - const ownsGroup = await modelGroupsPermissions.ensure( - { owns: group.createdBy }, - { throw: false } - ); + const cached = async () => { + const response = await listGroupsViaDataLoader(); - if (!ownsGroup) { - return false; - } + return filterAsync(response, async group => { + const ownsGroup = await modelGroupsPermissions.ensure( + { owns: group.createdBy }, + { throw: false } + ); - return await modelGroupsPermissions.canAccessGroup({ - group + if (!ownsGroup) { + return false; + } + + return await modelGroupsPermissions.canAccessGroup({ + group + }); }); - }); + }; + + listGroupsCache.set(key, cached()); + + return listGroupsCache.get(key) as Promise; }; + const createGroup: CmsGroupContext["createGroup"] = async input => { await modelGroupsPermissions.ensure({ rwd: "w" }); diff --git a/packages/api-headless-cms/src/types.ts b/packages/api-headless-cms/src/types.ts index 8b41896d619..7a41f280fcb 100644 --- a/packages/api-headless-cms/src/types.ts +++ b/packages/api-headless-cms/src/types.ts @@ -1096,7 +1096,6 @@ export interface CmsGroupListParams { where: { tenant: string; locale: string; - [key: string]: any; }; } diff --git a/packages/api-headless-cms/src/utils/filterAsync.ts b/packages/api-headless-cms/src/utils/filterAsync.ts index 7ff828201e5..9e1ac09e79e 100644 --- a/packages/api-headless-cms/src/utils/filterAsync.ts +++ b/packages/api-headless-cms/src/utils/filterAsync.ts @@ -2,15 +2,15 @@ export const filterAsync = async >( items: T[], predicate: (param: T) => Promise ): Promise => { - const filteredItems = []; + const filteredItems = await Promise.all( + items.map(async item => { + const valid = await predicate(item); + if (!valid) { + return null; + } + return item; + }) + ); - for (let i = 0; i < items.length; i++) { - const item = items[i]; - const valid = await predicate(item); - if (valid) { - filteredItems.push(item); - } - } - - return filteredItems; + return filteredItems.filter(Boolean) as T[]; }; diff --git a/packages/api-security/src/createSecurity.ts b/packages/api-security/src/createSecurity.ts index bdee73b092b..93f37be8aef 100644 --- a/packages/api-security/src/createSecurity.ts +++ b/packages/api-security/src/createSecurity.ts @@ -86,6 +86,9 @@ export const createSecurity = async (config: SecurityConfig): Promise authentication.setIdentity(identity); this.onIdentity.publish({ identity }); }, + isAuthorizationEnabled: () => { + return performAuthorization; + }, async withoutAuthorization(cb: () => Promise): Promise { const isAuthorizationEnabled = performAuthorization; performAuthorization = false; diff --git a/packages/api-security/src/types.ts b/packages/api-security/src/types.ts index d5a01a7c488..c5a249fcac8 100644 --- a/packages/api-security/src/types.ts +++ b/packages/api-security/src/types.ts @@ -85,6 +85,8 @@ export interface Security extends Authentication(cb: () => Promise): Promise; /**