From 9438c68a794e23df9cbc35cf0a0a2db7430480a2 Mon Sep 17 00:00:00 2001 From: Ben Stone Date: Mon, 17 Aug 2020 16:41:12 +0100 Subject: [PATCH] CON-2182 Refactor content service to use decorator pattern --- .../GetContentBanner/WhenIGetContentBanner.cs | 31 ------- .../WhenIGetContentWithCaching.cs | 93 +++++++++++++++++++ .../DependencyResolution/ServicesRegistry.cs | 2 + .../GetClientContentRequestHandler.cs | 20 +--- .../ClientContentServiceWithCaching.cs | 49 ++++++++++ .../GetClientContent/WhenIGetContent.cs | 39 -------- .../WhenIGetContentDataWithCaching.cs | 90 ++++++++++++++++++ .../DependencyResolution/IoC.cs | 1 + .../ContentApiClientRegistry.cs | 6 -- .../DependencyResolution/ServicesRegistry.cs | 20 ++++ .../GetClientContentRequestHandler.cs | 20 +--- .../Services/ClientContentService.cs | 4 +- .../ClientContentServiceWithCaching.cs | 50 ++++++++++ 13 files changed, 314 insertions(+), 111 deletions(-) create mode 100644 src/SFA.DAS.EmployerAccounts.UnitTests/Services/ContentBanner/WhenIGetContentWithCaching.cs create mode 100644 src/SFA.DAS.EmployerAccounts/Services/ClientContentServiceWithCaching.cs create mode 100644 src/SFA.DAS.EmployerFinance.UnitTests/Services/ClientContentServiceTests/ClientContentServiceWithCachingTests/WhenIGetContentDataWithCaching.cs create mode 100644 src/SFA.DAS.EmployerFinance/DependencyResolution/ServicesRegistry.cs create mode 100644 src/SFA.DAS.EmployerFinance/Services/ClientContentServiceWithCaching.cs diff --git a/src/SFA.DAS.EmployerAccounts.UnitTests/Queries/GetContentBanner/WhenIGetContentBanner.cs b/src/SFA.DAS.EmployerAccounts.UnitTests/Queries/GetContentBanner/WhenIGetContentBanner.cs index 27a5026dc0..6492e6a473 100644 --- a/src/SFA.DAS.EmployerAccounts.UnitTests/Queries/GetContentBanner/WhenIGetContentBanner.cs +++ b/src/SFA.DAS.EmployerAccounts.UnitTests/Queries/GetContentBanner/WhenIGetContentBanner.cs @@ -70,37 +70,6 @@ public override async Task ThenIfTheMessageIsValidTheValueIsReturnedInTheRespons Assert.AreEqual(ContentBanner, response.Content); } - [Test, RecursiveMoqAutoData] - public async Task Check_Cache_ReturnIfExists(GetClientContentRequest query1, string contentBanner1, - Mock cacheStorageService1, - GetClientContentRequestHandler requestHandler1, - Mock> requestValidator1, - Mock logger, - Mock clientMockontentService) - { - query1.ContentType = "Banner"; - query1.UseLegacyStyles = false; - - var key = EmployerAccountsConfiguration.ApplicationId; - - requestValidator1.Setup(r => r.Validate(query1)).Returns(new ValidationResult()); - - clientMockontentService.Setup(c => c.Get("banner", key)); - - requestHandler1 = new GetClientContentRequestHandler(requestValidator1.Object, logger.Object, clientMockontentService.Object, - cacheStorageService1.Object, EmployerAccountsConfiguration); - - var cacheKey = key + "_banner"; - cacheStorageService1.Setup(c => c.TryGet(cacheKey, out contentBanner1)) - .Returns(true); - //Act - var result = await requestHandler1.Handle(query1); - - //assert - Assert.AreEqual(result.Content, contentBanner1); - cacheStorageService1.Verify(x => x.TryGet(cacheKey, out contentBanner1), Times.Once); - } - [Test, RecursiveMoqAutoData] public async Task Check_Cache_ReturnNull_CallFromClient(GetClientContentRequest query1, string contentBanner1, Mock cacheStorageService1, diff --git a/src/SFA.DAS.EmployerAccounts.UnitTests/Services/ContentBanner/WhenIGetContentWithCaching.cs b/src/SFA.DAS.EmployerAccounts.UnitTests/Services/ContentBanner/WhenIGetContentWithCaching.cs new file mode 100644 index 0000000000..1c6ed859d5 --- /dev/null +++ b/src/SFA.DAS.EmployerAccounts.UnitTests/Services/ContentBanner/WhenIGetContentWithCaching.cs @@ -0,0 +1,93 @@ + + +using Moq; +using NUnit.Framework; +using SFA.DAS.EmployerAccounts.Configuration; +using SFA.DAS.EmployerAccounts.Interfaces; +using SFA.DAS.EmployerAccounts.Services; +using System.Threading.Tasks; + +namespace SFA.DAS.EmployerAccounts.UnitTests.Services.ContentBanner +{ + public class WhenIGetContentWithCaching + { + private string _contentType; + private string _clientId; + + private Mock MockClientContentService; + private Mock MockCacheStorageService; + + private string CacheKey; + private string ContentFromCache; + private string ContentFromApi; + private EmployerAccountsConfiguration EmployerAccountsConfig; + + private IClientContentService ClientContentServiceWithCaching; + + [SetUp] + public void Arrange() + { + _clientId = "eas-fin"; + _contentType = "banner"; + + EmployerAccountsConfig = new EmployerAccountsConfiguration() + { + ApplicationId = "eas-fin", + DefaultCacheExpirationInMinutes = 1 + }; + ContentFromCache = "

Example content from cache

"; + ContentFromApi = "

Example content from api

"; + CacheKey = EmployerAccountsConfig.ApplicationId + "_banner"; + + MockClientContentService = new Mock(); + MockCacheStorageService = new Mock(); + + ClientContentServiceWithCaching = new ClientContentServiceWithCaching(MockClientContentService.Object, MockCacheStorageService.Object, EmployerAccountsConfig); + } + + [Test] + public async Task AND_ItIsStoredInTheCache_THEN_ReturnContent() + { + StoredInCacheSetup(); + + var result = await ClientContentServiceWithCaching.Get(_contentType, EmployerAccountsConfig.ApplicationId); + + Assert.AreEqual(ContentFromCache, result); + MockCacheStorageService.Verify(c => c.TryGet(CacheKey, out ContentFromCache), Times.Once); + } + + [Test] + public async Task AND_ItIsStoredInTheCache_THEN_ContentApiIsNotCalled() + { + StoredInCacheSetup(); + + var result = await ClientContentServiceWithCaching.Get(_contentType, EmployerAccountsConfig.ApplicationId); + + MockClientContentService.Verify(c => c.Get(_contentType, _clientId), Times.Never); + } + + [Test] + public async Task AND_ItIsNotStoredInTheCache_THEN_CallFromClient() + { + NotStoredInCacheSetup(); + + var result = await ClientContentServiceWithCaching.Get(_contentType, EmployerAccountsConfig.ApplicationId); + + MockClientContentService.Verify(c => c.Get(_contentType, _clientId), Times.Once); + Assert.AreEqual(ContentFromApi, result); + } + private void StoredInCacheSetup() + { + MockCacheStorageService.Setup(c => c.TryGet(CacheKey, out ContentFromCache)).Returns(true); + MockClientContentService.Setup(c => c.Get("banner", CacheKey)); + } + + private void NotStoredInCacheSetup() + { + + MockCacheStorageService.Setup(c => c.TryGet(CacheKey, out ContentFromCache)).Returns(false); + MockClientContentService.Setup(c => c.Get("banner", EmployerAccountsConfig.ApplicationId)) + .ReturnsAsync(ContentFromApi); + } + } +} diff --git a/src/SFA.DAS.EmployerAccounts/DependencyResolution/ServicesRegistry.cs b/src/SFA.DAS.EmployerAccounts/DependencyResolution/ServicesRegistry.cs index f6de582a38..5304c69af1 100644 --- a/src/SFA.DAS.EmployerAccounts/DependencyResolution/ServicesRegistry.cs +++ b/src/SFA.DAS.EmployerAccounts/DependencyResolution/ServicesRegistry.cs @@ -18,6 +18,8 @@ public ServicesRegistry() For().DecorateAllWith(); For().Use(); For().DecorateAllWith(); + For().Use(); + For().DecorateAllWith(); } } } \ No newline at end of file diff --git a/src/SFA.DAS.EmployerAccounts/Queries/GetClientContent/GetClientContentRequestHandler.cs b/src/SFA.DAS.EmployerAccounts/Queries/GetClientContent/GetClientContentRequestHandler.cs index 3ab9410f29..c7251327dc 100644 --- a/src/SFA.DAS.EmployerAccounts/Queries/GetClientContent/GetClientContentRequestHandler.cs +++ b/src/SFA.DAS.EmployerAccounts/Queries/GetClientContent/GetClientContentRequestHandler.cs @@ -37,25 +37,13 @@ public async Task Handle(GetClientContentRequest messa throw new InvalidRequestException(validationResult.ValidationDictionary); } - var applicationId = message.UseLegacyStyles? _employerAccountsConfiguration.ApplicationId + "-legacy" : _employerAccountsConfiguration.ApplicationId; - var cacheKey = $"{applicationId}_{message.ContentType}".ToLowerInvariant(); + var applicationId = message.UseLegacyStyles? _employerAccountsConfiguration.ApplicationId + "-legacy" : _employerAccountsConfiguration.ApplicationId; try - { - if (_cacheStorageService.TryGet(cacheKey, out string cachedContentBanner)) - { - return new GetClientContentResponse - { - Content = cachedContentBanner - }; - } - + { var contentBanner = await _service.Get(message.ContentType, applicationId); - if (contentBanner != null) - { - await _cacheStorageService.Save(cacheKey, contentBanner, 1); - } + return new GetClientContentResponse { Content = contentBanner @@ -63,7 +51,7 @@ public async Task Handle(GetClientContentRequest messa } catch (Exception ex) { - _logger.Error(ex, $"Failed to get Content for {cacheKey}"); + _logger.Error(ex, $"Failed to get Content {message.ContentType} for {applicationId}"); return new GetClientContentResponse { diff --git a/src/SFA.DAS.EmployerAccounts/Services/ClientContentServiceWithCaching.cs b/src/SFA.DAS.EmployerAccounts/Services/ClientContentServiceWithCaching.cs new file mode 100644 index 0000000000..e6f9e3f8c4 --- /dev/null +++ b/src/SFA.DAS.EmployerAccounts/Services/ClientContentServiceWithCaching.cs @@ -0,0 +1,49 @@ +using SFA.DAS.EmployerAccounts.Configuration; +using SFA.DAS.EmployerAccounts.Interfaces; +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace SFA.DAS.EmployerAccounts.Services +{ + public class ClientContentServiceWithCaching : IClientContentService + { + private readonly IClientContentService _contentService; + private readonly ICacheStorageService _cacheStorageService; + private readonly EmployerAccountsConfiguration _employerAccountsConfiguration; + + public ClientContentServiceWithCaching(IClientContentService contentService, ICacheStorageService cacheStorageService, EmployerAccountsConfiguration employerAccountsConfiguration) + { + _contentService = contentService; + _cacheStorageService = cacheStorageService; + _employerAccountsConfiguration = employerAccountsConfiguration; + } + public async Task Get(string type, string applicationId) + { + var cacheKey = $"{applicationId}_{type}".ToLowerInvariant(); + + try + { + if (_cacheStorageService.TryGet(cacheKey, out string cachedContentBanner)) + { + return cachedContentBanner; + } + + var content = await _contentService.Get(type, applicationId); + + if (content != null) + { + await _cacheStorageService.Save(cacheKey, content, _employerAccountsConfiguration.DefaultCacheExpirationInMinutes); + } + + return content; + } + catch + { + throw new ArgumentException($"Failed to get content for {cacheKey}"); + } + } + } +} diff --git a/src/SFA.DAS.EmployerFinance.UnitTests/Queries/GetClientContent/WhenIGetContent.cs b/src/SFA.DAS.EmployerFinance.UnitTests/Queries/GetClientContent/WhenIGetContent.cs index d30efb2120..c70e48b93c 100644 --- a/src/SFA.DAS.EmployerFinance.UnitTests/Queries/GetClientContent/WhenIGetContent.cs +++ b/src/SFA.DAS.EmployerFinance.UnitTests/Queries/GetClientContent/WhenIGetContent.cs @@ -83,47 +83,8 @@ public override async Task ThenIfTheMessageIsValidTheValueIsReturnedInTheRespons MockClientContentService.Verify(x => x.Get(_contentType, _clientId), Times.Once); } - [Test] - public async Task AND_ItIsStoredInTheCache_THEN_ReturnContent() - { - StoredInCacheSetup(); - - var result = await RequestHandler.Handle(Query); - - Assert.AreEqual(Content, result.Content); - MockCacheStorageService.Verify(c => c.TryGet(CacheKey, out Content), Times.Once); - } - - [Test] - public async Task AND_ItIsStoredInTheCache_THEN_ContentApiIsNotCalled() - { - StoredInCacheSetup(); - - var result = await RequestHandler.Handle(Query); - - MockClientContentService.Verify(c => c.Get(_contentType, _clientId), Times.Never); - } - - [Test] - public async Task AND_ItIsNotStoredInTheCache_THEN_CallFromClient() - { - NotStoredInCacheSetup(); - - var result = await RequestHandler.Handle(Query); - - Assert.AreEqual(Content, result.Content); - } - - private void StoredInCacheSetup() - { - // RequestValidator.Setup(r => r.Validate(Query)).Returns(new ValidationResult()); - MockCacheStorageService.Setup(c => c.TryGet(CacheKey, out Content)).Returns(true); - MockClientContentService.Setup(c => c.Get("banner", CacheKey)); - } - private void NotStoredInCacheSetup() { - //RequestValidator.Setup(r => r.Validate(Query)).Returns(new ValidationResult()); MockCacheStorageService.Setup(c => c.TryGet(CacheKey, out Content)).Returns(false); MockClientContentService.Setup(c => c.Get("banner", CacheKey)) .ReturnsAsync(Content); diff --git a/src/SFA.DAS.EmployerFinance.UnitTests/Services/ClientContentServiceTests/ClientContentServiceWithCachingTests/WhenIGetContentDataWithCaching.cs b/src/SFA.DAS.EmployerFinance.UnitTests/Services/ClientContentServiceTests/ClientContentServiceWithCachingTests/WhenIGetContentDataWithCaching.cs new file mode 100644 index 0000000000..ad490e2062 --- /dev/null +++ b/src/SFA.DAS.EmployerFinance.UnitTests/Services/ClientContentServiceTests/ClientContentServiceWithCachingTests/WhenIGetContentDataWithCaching.cs @@ -0,0 +1,90 @@ +using Moq; +using NUnit.Framework; +using SFA.DAS.EmployerFinance.Configuration; +using SFA.DAS.EmployerFinance.Interfaces; +using SFA.DAS.EmployerFinance.Services; +using System.Threading.Tasks; + +namespace SFA.DAS.EmployerFinance.UnitTests.Services.ClientContentServiceTests.ClientContentServiceWithCachingTests +{ + class WhenIGetContentDataWithCaching + { + private string _contentType; + private string _clientId; + + private Mock MockClientContentService; + private Mock MockCacheStorageService; + + private string CacheKey; + private string ContentFromCache; + private string ContentFromApi; + private EmployerFinanceConfiguration EmployerFinanceConfig; + + private IClientContentService ClientContentServiceWithCaching; + + [SetUp] + public void Arrange() + { + _clientId = "eas-fin"; + _contentType = "banner"; + + EmployerFinanceConfig = new EmployerFinanceConfiguration() + { + ApplicationId = "eas-fin", + DefaultCacheExpirationInMinutes = 1 + }; + ContentFromCache = "

Example content from cache

"; + ContentFromApi = "

Example content from api

"; + CacheKey = EmployerFinanceConfig.ApplicationId + "_banner"; + + MockClientContentService = new Mock(); + MockCacheStorageService = new Mock(); + + ClientContentServiceWithCaching = new ClientContentServiceWithCaching(MockClientContentService.Object, MockCacheStorageService.Object, EmployerFinanceConfig); + } + [Test] + public async Task AND_ItIsStoredInTheCache_THEN_ReturnContent() + { + StoredInCacheSetup(); + + var result = await ClientContentServiceWithCaching.Get(_contentType, EmployerFinanceConfig.ApplicationId); + + Assert.AreEqual(ContentFromCache, result); + MockCacheStorageService.Verify(c => c.TryGet(CacheKey, out ContentFromCache), Times.Once); + } + + [Test] + public async Task AND_ItIsStoredInTheCache_THEN_ContentApiIsNotCalled() + { + StoredInCacheSetup(); + + var result = await ClientContentServiceWithCaching.Get(_contentType, EmployerFinanceConfig.ApplicationId); + + MockClientContentService.Verify(c => c.Get(_contentType, _clientId), Times.Never); + } + + [Test] + public async Task AND_ItIsNotStoredInTheCache_THEN_CallFromClient() + { + NotStoredInCacheSetup(); + + var result = await ClientContentServiceWithCaching.Get(_contentType, EmployerFinanceConfig.ApplicationId); + + MockClientContentService.Verify(c => c.Get(_contentType, _clientId), Times.Once); + Assert.AreEqual(ContentFromApi, result); + } + private void StoredInCacheSetup() + { + MockCacheStorageService.Setup(c => c.TryGet(CacheKey, out ContentFromCache)).Returns(true); + MockClientContentService.Setup(c => c.Get("banner", CacheKey)); + } + + private void NotStoredInCacheSetup() + { + + MockCacheStorageService.Setup(c => c.TryGet(CacheKey, out ContentFromCache)).Returns(false); + MockClientContentService.Setup(c => c.Get("banner", EmployerFinanceConfig.ApplicationId)) + .ReturnsAsync(ContentFromApi); + } + } +} diff --git a/src/SFA.DAS.EmployerFinance.Web/DependencyResolution/IoC.cs b/src/SFA.DAS.EmployerFinance.Web/DependencyResolution/IoC.cs index f1fcf86e0b..f6f53e4349 100644 --- a/src/SFA.DAS.EmployerFinance.Web/DependencyResolution/IoC.cs +++ b/src/SFA.DAS.EmployerFinance.Web/DependencyResolution/IoC.cs @@ -38,6 +38,7 @@ public static IContainer Initialize() c.AddRegistry(); c.AddRegistry(); c.AddRegistry(); + c.AddRegistry(); }); } } diff --git a/src/SFA.DAS.EmployerFinance/DependencyResolution/ContentApiClientRegistry.cs b/src/SFA.DAS.EmployerFinance/DependencyResolution/ContentApiClientRegistry.cs index d4fb2a922a..1e68ffcf2b 100644 --- a/src/SFA.DAS.EmployerFinance/DependencyResolution/ContentApiClientRegistry.cs +++ b/src/SFA.DAS.EmployerFinance/DependencyResolution/ContentApiClientRegistry.cs @@ -5,12 +5,7 @@ using SFA.DAS.Http.TokenGenerators; using SFA.DAS.NLog.Logger.Web.MessageHandlers; using StructureMap; -using System; -using System.Collections.Generic; -using System.Linq; using System.Net.Http; -using System.Text; -using System.Threading.Tasks; namespace SFA.DAS.EmployerFinance.DependencyResolution { @@ -35,6 +30,5 @@ private HttpClient CreateClient(IContext context) return httpClient; } - } } diff --git a/src/SFA.DAS.EmployerFinance/DependencyResolution/ServicesRegistry.cs b/src/SFA.DAS.EmployerFinance/DependencyResolution/ServicesRegistry.cs new file mode 100644 index 0000000000..609c38c219 --- /dev/null +++ b/src/SFA.DAS.EmployerFinance/DependencyResolution/ServicesRegistry.cs @@ -0,0 +1,20 @@ +using SFA.DAS.EmployerFinance.Interfaces; +using SFA.DAS.EmployerFinance.Services; +using StructureMap; +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace SFA.DAS.EmployerFinance.DependencyResolution +{ + public class ServicesRegistry : Registry + { + public ServicesRegistry() + { + For().Use(); + For().DecorateAllWith(); + } + } +} diff --git a/src/SFA.DAS.EmployerFinance/Queries/GetClientContent/GetClientContentRequestHandler.cs b/src/SFA.DAS.EmployerFinance/Queries/GetClientContent/GetClientContentRequestHandler.cs index 83e73756b1..d4b13b31ba 100644 --- a/src/SFA.DAS.EmployerFinance/Queries/GetClientContent/GetClientContentRequestHandler.cs +++ b/src/SFA.DAS.EmployerFinance/Queries/GetClientContent/GetClientContentRequestHandler.cs @@ -44,32 +44,18 @@ public async Task Handle(GetClientContentRequest messa var applicationId = message.UseLegacyStyles ? _employerFinanceConfiguration.ApplicationId + "-legacy" : _employerFinanceConfiguration.ApplicationId; - var cacheKey = $"{applicationId}_{message.ContentType}".ToLowerInvariant(); - try { - if (_cacheStorageService.TryGet(cacheKey, out string cachedContentBanner)) - { - return new GetClientContentResponse - { - Content = cachedContentBanner - }; - } + var content = await _service.Get(message.ContentType, applicationId); - var contentBanner = await _service.Get(message.ContentType, applicationId); - - if (contentBanner != null) - { - await _cacheStorageService.Save(cacheKey, contentBanner, 1); - } return new GetClientContentResponse { - Content = contentBanner + Content = content }; } catch (Exception ex) { - _logger.Error(ex, $"Failed to get Content for {cacheKey}"); + _logger.Error(ex, $"Failed to get Content {message.ContentType} for {applicationId}"); return new GetClientContentResponse { diff --git a/src/SFA.DAS.EmployerFinance/Services/ClientContentService.cs b/src/SFA.DAS.EmployerFinance/Services/ClientContentService.cs index c2ae59ca02..9f1db79acc 100644 --- a/src/SFA.DAS.EmployerFinance/Services/ClientContentService.cs +++ b/src/SFA.DAS.EmployerFinance/Services/ClientContentService.cs @@ -14,9 +14,9 @@ public ClientContentService(IClientContentApiClient client) public async Task Get(string type, string applicationId) { - var cachedContent = await _client.Get(type, applicationId); + var content = await _client.Get(type, applicationId); - return cachedContent; + return content; } } } diff --git a/src/SFA.DAS.EmployerFinance/Services/ClientContentServiceWithCaching.cs b/src/SFA.DAS.EmployerFinance/Services/ClientContentServiceWithCaching.cs new file mode 100644 index 0000000000..72a5a4e0aa --- /dev/null +++ b/src/SFA.DAS.EmployerFinance/Services/ClientContentServiceWithCaching.cs @@ -0,0 +1,50 @@ +using SFA.DAS.EmployerFinance.Configuration; +using SFA.DAS.EmployerFinance.Interfaces; +using SFA.DAS.NLog.Logger; +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; + +namespace SFA.DAS.EmployerFinance.Services +{ + public class ClientContentServiceWithCaching : IClientContentService + { + private readonly IClientContentService _contentService; + private readonly ICacheStorageService _cacheStorageService; + private readonly EmployerFinanceConfiguration _employerFinanceConfiguration; + + public ClientContentServiceWithCaching(IClientContentService contentService, ICacheStorageService cacheStorageService, EmployerFinanceConfiguration employerFinanceConfiguration) + { + _contentService = contentService; + _cacheStorageService = cacheStorageService; + _employerFinanceConfiguration = employerFinanceConfiguration; + } + public async Task Get(string type, string applicationId) + { + var cacheKey = $"{applicationId}_{type}".ToLowerInvariant(); + + try + { + if (_cacheStorageService.TryGet(cacheKey, out string cachedContentBanner)) + { + return cachedContentBanner; + } + + var content = await _contentService.Get(type, applicationId); + + if (content != null) + { + await _cacheStorageService.Save(cacheKey, content, _employerFinanceConfiguration.DefaultCacheExpirationInMinutes); + } + + return content; + } + catch + { + throw new ArgumentException($"Failed to get content for {cacheKey}"); + } + } + } +}