From 3af60f11faa63f886b3b404640ac445bc95069ba Mon Sep 17 00:00:00 2001 From: Thomas Rittson <31796059+eliykat@users.noreply.github.com> Date: Wed, 16 Aug 2023 13:42:09 +1000 Subject: [PATCH] [AC-1597] Revert GetByUserIdWithPolicyDetailsAsync changes to unblock SQL CPU (#3203) * Revert "[PM-3007] Caching user policies on PolicyService variable (#3117)" This reverts commit 78588d0246d21cfbd8bb01b36d6cec380647c9d2. * Don't delete old migration script * Add migration to revert sproc (cherry picked from commit fc814ff352437151e29d59090605c080225b7dbd) --- .../IOrganizationUserRepository.cs | 2 +- .../Services/Implementations/PolicyService.cs | 17 +++------ .../OrganizationUserRepository.cs | 4 +-- .../OrganizationUserRepository.cs | 5 +-- ...tionUser_ReadByUserIdWithPolicyDetails.sql | 30 +++++++++------- test/Core.Test/Services/PolicyServiceTests.cs | 10 ++++-- .../OrganizationUserRepositoryTests.cs | 2 +- ...ationUserReadByUserIdWithPolicyDetails.sql | 35 +++++++++++++++++++ 8 files changed, 71 insertions(+), 34 deletions(-) create mode 100644 util/Migrator/DbScripts/2023-08-16_00_HotfixRevertOrganizationUserReadByUserIdWithPolicyDetails.sql diff --git a/src/Core/Repositories/IOrganizationUserRepository.cs b/src/Core/Repositories/IOrganizationUserRepository.cs index fb012c62362f..f9dfa12c2cbf 100644 --- a/src/Core/Repositories/IOrganizationUserRepository.cs +++ b/src/Core/Repositories/IOrganizationUserRepository.cs @@ -39,6 +39,6 @@ Task GetDetailsByUserAsync(Guid userId, Gui Task> GetManyByMinimumRoleAsync(Guid organizationId, OrganizationUserType minRole); Task RevokeAsync(Guid id); Task RestoreAsync(Guid id, OrganizationUserStatusType status); - Task> GetByUserIdWithPolicyDetailsAsync(Guid userId); + Task> GetByUserIdWithPolicyDetailsAsync(Guid userId, PolicyType policyType); Task GetOccupiedSmSeatCountByOrganizationIdAsync(Guid organizationId); } diff --git a/src/Core/Services/Implementations/PolicyService.cs b/src/Core/Services/Implementations/PolicyService.cs index 595a798e7307..6b1009093c1c 100644 --- a/src/Core/Services/Implementations/PolicyService.cs +++ b/src/Core/Services/Implementations/PolicyService.cs @@ -20,8 +20,6 @@ public class PolicyService : IPolicyService private readonly IMailService _mailService; private readonly GlobalSettings _globalSettings; - private IEnumerable _cachedOrganizationUserPolicyDetails; - public PolicyService( IEventService eventService, IOrganizationRepository organizationRepository, @@ -196,25 +194,18 @@ public async Task AnyPoliciesApplicableToUserAsync(Guid userId, PolicyType return result.Any(); } - private async Task> QueryOrganizationUserPolicyDetailsAsync(Guid userId, PolicyType? policyType, OrganizationUserStatusType minStatus = OrganizationUserStatusType.Accepted) + private async Task> QueryOrganizationUserPolicyDetailsAsync(Guid userId, PolicyType policyType, OrganizationUserStatusType minStatus = OrganizationUserStatusType.Accepted) { - // Check if the cached policies are available - if (_cachedOrganizationUserPolicyDetails == null) - { - // Cached policies not available, retrieve from the repository - _cachedOrganizationUserPolicyDetails = await _organizationUserRepository.GetByUserIdWithPolicyDetailsAsync(userId); - } - + var organizationUserPolicyDetails = await _organizationUserRepository.GetByUserIdWithPolicyDetailsAsync(userId, policyType); var excludedUserTypes = GetUserTypesExcludedFromPolicy(policyType); - return _cachedOrganizationUserPolicyDetails.Where(o => - (policyType == null || o.PolicyType == policyType) && + return organizationUserPolicyDetails.Where(o => o.PolicyEnabled && !excludedUserTypes.Contains(o.OrganizationUserType) && o.OrganizationUserStatus >= minStatus && !o.IsProvider); } - private OrganizationUserType[] GetUserTypesExcludedFromPolicy(PolicyType? policyType) + private OrganizationUserType[] GetUserTypesExcludedFromPolicy(PolicyType policyType) { switch (policyType) { diff --git a/src/Infrastructure.Dapper/Repositories/OrganizationUserRepository.cs b/src/Infrastructure.Dapper/Repositories/OrganizationUserRepository.cs index 27410fb3f7a1..008242c26c2b 100644 --- a/src/Infrastructure.Dapper/Repositories/OrganizationUserRepository.cs +++ b/src/Infrastructure.Dapper/Repositories/OrganizationUserRepository.cs @@ -505,13 +505,13 @@ public async Task RestoreAsync(Guid id, OrganizationUserStatusType status) } } - public async Task> GetByUserIdWithPolicyDetailsAsync(Guid userId) + public async Task> GetByUserIdWithPolicyDetailsAsync(Guid userId, PolicyType policyType) { using (var connection = new SqlConnection(ConnectionString)) { var results = await connection.QueryAsync( $"[{Schema}].[{Table}_ReadByUserIdWithPolicyDetails]", - new { UserId = userId }, + new { UserId = userId, PolicyType = policyType }, commandType: CommandType.StoredProcedure); return results.ToList(); diff --git a/src/Infrastructure.EntityFramework/Repositories/OrganizationUserRepository.cs b/src/Infrastructure.EntityFramework/Repositories/OrganizationUserRepository.cs index f5ba2b9c1b66..8256696d9686 100644 --- a/src/Infrastructure.EntityFramework/Repositories/OrganizationUserRepository.cs +++ b/src/Infrastructure.EntityFramework/Repositories/OrganizationUserRepository.cs @@ -588,7 +588,7 @@ public async Task RestoreAsync(Guid id, OrganizationUserStatusType status) } } - public async Task> GetByUserIdWithPolicyDetailsAsync(Guid userId) + public async Task> GetByUserIdWithPolicyDetailsAsync(Guid userId, PolicyType policyType) { using (var scope = ServiceScopeFactory.CreateScope()) { @@ -604,7 +604,8 @@ on pu.ProviderId equals po.ProviderId join ou in dbContext.OrganizationUsers on p.OrganizationId equals ou.OrganizationId let email = dbContext.Users.Find(userId).Email // Invited orgUsers do not have a UserId associated with them, so we have to match up their email - where ou.UserId == userId || ou.Email == email + where p.Type == policyType && + (ou.UserId == userId || ou.Email == email) select new OrganizationUserPolicyDetails { OrganizationUserId = ou.Id, diff --git a/src/Sql/dbo/Stored Procedures/OrganizationUser_ReadByUserIdWithPolicyDetails.sql b/src/Sql/dbo/Stored Procedures/OrganizationUser_ReadByUserIdWithPolicyDetails.sql index b4968e2cd9d8..c2bc690a2787 100644 --- a/src/Sql/dbo/Stored Procedures/OrganizationUser_ReadByUserIdWithPolicyDetails.sql +++ b/src/Sql/dbo/Stored Procedures/OrganizationUser_ReadByUserIdWithPolicyDetails.sql @@ -1,5 +1,6 @@ CREATE PROCEDURE [dbo].[OrganizationUser_ReadByUserIdWithPolicyDetails] - @UserId UNIQUEIDENTIFIER + @UserId UNIQUEIDENTIFIER, + @PolicyType TINYINT AS BEGIN SET NOCOUNT ON @@ -12,19 +13,22 @@ SELECT OU.[Type] AS OrganizationUserType, OU.[Status] AS OrganizationUserStatus, OU.[Permissions] AS OrganizationUserPermissionsData, - CASE WHEN PU.[ProviderId] IS NOT NULL THEN 1 ELSE 0 END AS IsProvider + CASE WHEN EXISTS ( + SELECT 1 + FROM [dbo].[ProviderUserView] PU + INNER JOIN [dbo].[ProviderOrganizationView] PO ON PO.[ProviderId] = PU.[ProviderId] + WHERE PU.[UserId] = OU.[UserId] AND PO.[OrganizationId] = P.[OrganizationId] + ) THEN 1 ELSE 0 END AS IsProvider FROM [dbo].[PolicyView] P INNER JOIN [dbo].[OrganizationUserView] OU ON P.[OrganizationId] = OU.[OrganizationId] -LEFT JOIN [dbo].[ProviderUserView] PU - ON PU.[UserId] = OU.[UserId] -LEFT JOIN [dbo].[ProviderOrganizationView] PO - ON PO.[ProviderId] = PU.[ProviderId] AND PO.[OrganizationId] = P.[OrganizationId] -WHERE - (OU.[Status] != 0 AND OU.[UserId] = @UserId) -- OrgUsers who have accepted their invite and are linked to a UserId - OR EXISTS ( - SELECT 1 - FROM [dbo].[UserView] U - WHERE U.[Id] = @UserId AND OU.[Email] = U.[Email] AND OU.[Status] = 0 -- 'Invited' OrgUsers are not linked to a UserId yet, so we have to look up their email +WHERE P.[Type] = @PolicyType AND + ( + (OU.[Status] != 0 AND OU.[UserId] = @UserId) -- OrgUsers who have accepted their invite and are linked to a UserId + OR EXISTS ( + SELECT 1 + FROM [dbo].[UserView] U + WHERE U.[Id] = @UserId AND OU.[Email] = U.[Email] AND OU.[Status] = 0 -- 'Invited' OrgUsers are not linked to a UserId yet, so we have to look up their email + ) ) -END +END \ No newline at end of file diff --git a/test/Core.Test/Services/PolicyServiceTests.cs b/test/Core.Test/Services/PolicyServiceTests.cs index a85c2a7c491b..6c1218c84a7f 100644 --- a/test/Core.Test/Services/PolicyServiceTests.cs +++ b/test/Core.Test/Services/PolicyServiceTests.cs @@ -624,12 +624,18 @@ private static void SetupOrg(SutProvider sutProvider, Guid organi private static void SetupUserPolicies(Guid userId, SutProvider sutProvider) { sutProvider.GetDependency() - .GetByUserIdWithPolicyDetailsAsync(userId) + .GetByUserIdWithPolicyDetailsAsync(userId, PolicyType.RequireSso) .Returns(new List { new() { OrganizationId = Guid.NewGuid(), PolicyType = PolicyType.RequireSso, PolicyEnabled = false, OrganizationUserType = OrganizationUserType.Owner, OrganizationUserStatus = OrganizationUserStatusType.Confirmed, IsProvider = false}, new() { OrganizationId = Guid.NewGuid(), PolicyType = PolicyType.RequireSso, PolicyEnabled = true, OrganizationUserType = OrganizationUserType.Owner, OrganizationUserStatus = OrganizationUserStatusType.Confirmed, IsProvider = false }, - new() { OrganizationId = Guid.NewGuid(), PolicyType = PolicyType.RequireSso, PolicyEnabled = true, OrganizationUserType = OrganizationUserType.Owner, OrganizationUserStatus = OrganizationUserStatusType.Confirmed, IsProvider = true }, + new() { OrganizationId = Guid.NewGuid(), PolicyType = PolicyType.RequireSso, PolicyEnabled = true, OrganizationUserType = OrganizationUserType.Owner, OrganizationUserStatus = OrganizationUserStatusType.Confirmed, IsProvider = true } + }); + + sutProvider.GetDependency() + .GetByUserIdWithPolicyDetailsAsync(userId, PolicyType.DisableSend) + .Returns(new List + { new() { OrganizationId = Guid.NewGuid(), PolicyType = PolicyType.DisableSend, PolicyEnabled = true, OrganizationUserType = OrganizationUserType.User, OrganizationUserStatus = OrganizationUserStatusType.Invited, IsProvider = false }, new() { OrganizationId = Guid.NewGuid(), PolicyType = PolicyType.DisableSend, PolicyEnabled = true, OrganizationUserType = OrganizationUserType.User, OrganizationUserStatus = OrganizationUserStatusType.Invited, IsProvider = true } }); diff --git a/test/Infrastructure.EFIntegration.Test/Repositories/OrganizationUserRepositoryTests.cs b/test/Infrastructure.EFIntegration.Test/Repositories/OrganizationUserRepositoryTests.cs index b1ee1cbc9bee..4f3b912b6ec0 100644 --- a/test/Infrastructure.EFIntegration.Test/Repositories/OrganizationUserRepositoryTests.cs +++ b/test/Infrastructure.EFIntegration.Test/Repositories/OrganizationUserRepositoryTests.cs @@ -274,7 +274,7 @@ SqlRepo.ProviderUserRepository sqlProviderUserRepo } // Act - var result = await orgUserRepos[i].GetByUserIdWithPolicyDetailsAsync(savedUser.Id); + var result = await orgUserRepos[i].GetByUserIdWithPolicyDetailsAsync(savedUser.Id, policy.Type); results.Add(result.FirstOrDefault()); } diff --git a/util/Migrator/DbScripts/2023-08-16_00_HotfixRevertOrganizationUserReadByUserIdWithPolicyDetails.sql b/util/Migrator/DbScripts/2023-08-16_00_HotfixRevertOrganizationUserReadByUserIdWithPolicyDetails.sql new file mode 100644 index 000000000000..7da87f4aef0b --- /dev/null +++ b/util/Migrator/DbScripts/2023-08-16_00_HotfixRevertOrganizationUserReadByUserIdWithPolicyDetails.sql @@ -0,0 +1,35 @@ +CREATE OR ALTER PROCEDURE [dbo].[OrganizationUser_ReadByUserIdWithPolicyDetails] + @UserId UNIQUEIDENTIFIER, + @PolicyType TINYINT +AS +BEGIN + SET NOCOUNT ON +SELECT + OU.[Id] AS OrganizationUserId, + P.[OrganizationId], + P.[Type] AS PolicyType, + P.[Enabled] AS PolicyEnabled, + P.[Data] AS PolicyData, + OU.[Type] AS OrganizationUserType, + OU.[Status] AS OrganizationUserStatus, + OU.[Permissions] AS OrganizationUserPermissionsData, + CASE WHEN EXISTS ( + SELECT 1 + FROM [dbo].[ProviderUserView] PU + INNER JOIN [dbo].[ProviderOrganizationView] PO ON PO.[ProviderId] = PU.[ProviderId] + WHERE PU.[UserId] = OU.[UserId] AND PO.[OrganizationId] = P.[OrganizationId] + ) THEN 1 ELSE 0 END AS IsProvider +FROM [dbo].[PolicyView] P +INNER JOIN [dbo].[OrganizationUserView] OU + ON P.[OrganizationId] = OU.[OrganizationId] +WHERE P.[Type] = @PolicyType AND + ( + (OU.[Status] != 0 AND OU.[UserId] = @UserId) -- OrgUsers who have accepted their invite and are linked to a UserId + OR EXISTS ( + SELECT 1 + FROM [dbo].[UserView] U + WHERE U.[Id] = @UserId AND OU.[Email] = U.[Email] AND OU.[Status] = 0 -- 'Invited' OrgUsers are not linked to a UserId yet, so we have to look up their email + ) + ) +END +GO \ No newline at end of file