From 0ad41aa3fac8fc69776ddf14beae0b3d4f3c8737 Mon Sep 17 00:00:00 2001 From: Marc Denman Date: Tue, 14 Mar 2017 11:52:44 +0000 Subject: [PATCH] Add LogError(string) to logger Added LogError(string) to interface as sometimes there isn't an exception to be logged. Additionally, split Logger and LoggerFactory implementations into seperate files just for tidiness. Additonally added some very basic unit tests to the HttpDataRepository as a bit of regression safety and to prove that Get never returns a null. Slightly refactored the logic within AspDotNetLogger under GetMessageWithOcelotRequestId so that the if statement is a little easier to read. Attempted to remove the requestId == null, however this broke numerous tests as the mocks don't set the behviour for dataReposioty getting the requestId --- src/Ocelot/Logging/AspDotNetLogger.cs | 49 +++++++++++++ src/Ocelot/Logging/AspDotNetLoggerFactory.cs | 23 ++++++ src/Ocelot/Logging/IOcelotLoggerFactory.cs | 64 +--------------- .../Middleware/ResponderMiddleware.cs | 6 +- .../Infrastructure/HttpDataRepositoryTests.cs | 73 +++++++++++++++++++ 5 files changed, 152 insertions(+), 63 deletions(-) create mode 100644 src/Ocelot/Logging/AspDotNetLogger.cs create mode 100644 src/Ocelot/Logging/AspDotNetLoggerFactory.cs create mode 100644 test/Ocelot.UnitTests/Infrastructure/HttpDataRepositoryTests.cs diff --git a/src/Ocelot/Logging/AspDotNetLogger.cs b/src/Ocelot/Logging/AspDotNetLogger.cs new file mode 100644 index 00000000..c4185c23 --- /dev/null +++ b/src/Ocelot/Logging/AspDotNetLogger.cs @@ -0,0 +1,49 @@ +using System; +using Microsoft.Extensions.Logging; +using Ocelot.Infrastructure.RequestData; + +namespace Ocelot.Logging +{ + public class AspDotNetLogger : IOcelotLogger + { + private readonly ILogger _logger; + private readonly IRequestScopedDataRepository _scopedDataRepository; + + public AspDotNetLogger(ILogger logger, IRequestScopedDataRepository scopedDataRepository) + { + _logger = logger; + _scopedDataRepository = scopedDataRepository; + } + + public void LogTrace(string message, params object[] args) + { + _logger.LogTrace(GetMessageWithOcelotRequestId(message), args); + } + + public void LogDebug(string message, params object[] args) + { + _logger.LogDebug(GetMessageWithOcelotRequestId(message), args); + } + public void LogError(string message, Exception exception) + { + _logger.LogError(GetMessageWithOcelotRequestId(message), exception); + } + + public void LogError(string message, params object[] args) + { + _logger.LogError(GetMessageWithOcelotRequestId(message), args); + } + + private string GetMessageWithOcelotRequestId(string message) + { + var requestId = _scopedDataRepository.Get("RequestId"); + + if (requestId == null || requestId.IsError) + { + return $"{message} : OcelotRequestId - not set"; + } + + return $"{message} : OcelotRequestId - {requestId.Data}"; + } + } +} \ No newline at end of file diff --git a/src/Ocelot/Logging/AspDotNetLoggerFactory.cs b/src/Ocelot/Logging/AspDotNetLoggerFactory.cs new file mode 100644 index 00000000..91435740 --- /dev/null +++ b/src/Ocelot/Logging/AspDotNetLoggerFactory.cs @@ -0,0 +1,23 @@ +using Microsoft.Extensions.Logging; +using Ocelot.Infrastructure.RequestData; + +namespace Ocelot.Logging +{ + public class AspDotNetLoggerFactory : IOcelotLoggerFactory + { + private readonly ILoggerFactory _loggerFactory; + private readonly IRequestScopedDataRepository _scopedDataRepository; + + public AspDotNetLoggerFactory(ILoggerFactory loggerFactory, IRequestScopedDataRepository scopedDataRepository) + { + _loggerFactory = loggerFactory; + _scopedDataRepository = scopedDataRepository; + } + + public IOcelotLogger CreateLogger() + { + var logger = _loggerFactory.CreateLogger(); + return new AspDotNetLogger(logger, _scopedDataRepository); + } + } +} \ No newline at end of file diff --git a/src/Ocelot/Logging/IOcelotLoggerFactory.cs b/src/Ocelot/Logging/IOcelotLoggerFactory.cs index 50e5b25e..98326648 100644 --- a/src/Ocelot/Logging/IOcelotLoggerFactory.cs +++ b/src/Ocelot/Logging/IOcelotLoggerFactory.cs @@ -1,6 +1,4 @@ using System; -using Microsoft.Extensions.Logging; -using Ocelot.Infrastructure.RequestData; namespace Ocelot.Logging { @@ -8,68 +6,14 @@ namespace Ocelot.Logging { IOcelotLogger CreateLogger(); } - - public class AspDotNetLoggerFactory : IOcelotLoggerFactory - { - private readonly ILoggerFactory _loggerFactory; - private readonly IRequestScopedDataRepository _scopedDataRepository; - - public AspDotNetLoggerFactory(ILoggerFactory loggerFactory, IRequestScopedDataRepository scopedDataRepository) - { - _loggerFactory = loggerFactory; - _scopedDataRepository = scopedDataRepository; - } - - public IOcelotLogger CreateLogger() - { - var logger = _loggerFactory.CreateLogger(); - return new AspDotNetLogger(logger, _scopedDataRepository); - } - } - + /// + /// Thin wrapper around the DotNet core logging framework, used to allow the scopedDataRepository to be injected giving access to the Ocelot RequestId + /// public interface IOcelotLogger { void LogTrace(string message, params object[] args); void LogDebug(string message, params object[] args); void LogError(string message, Exception exception); - } - - public class AspDotNetLogger : IOcelotLogger - { - private readonly ILogger _logger; - private readonly IRequestScopedDataRepository _scopedDataRepository; - - public AspDotNetLogger(ILogger logger, IRequestScopedDataRepository scopedDataRepository) - { - _logger = logger; - _scopedDataRepository = scopedDataRepository; - } - - public void LogTrace(string message, params object[] args) - { - _logger.LogTrace(GetMessageWithOcelotRequestId(message), args); - } - - public void LogDebug(string message, params object[] args) - { - _logger.LogDebug(GetMessageWithOcelotRequestId(message), args); - } - - public void LogError(string message, Exception exception) - { - _logger.LogError(GetMessageWithOcelotRequestId(message), exception); - } - - private string GetMessageWithOcelotRequestId(string message) - { - var requestId = _scopedDataRepository.Get("RequestId"); - - if (requestId != null && !requestId.IsError) - { - return $"{message} : OcelotRequestId - {requestId.Data}"; - - } - return $"{message} : OcelotRequestId - not set"; - } + void LogError(string message, params object[] args); } } diff --git a/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs b/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs index 2e6585d6..a15bb68b 100644 --- a/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs +++ b/src/Ocelot/Responder/Middleware/ResponderMiddleware.cs @@ -35,16 +35,16 @@ namespace Ocelot.Responder.Middleware public async Task Invoke(HttpContext context) { _logger.LogTrace($"entered {MiddlwareName}"); - _logger.LogDebug($"invoking next middleware from {MiddlwareName}"); + _logger.LogTrace($"invoking next middleware from {MiddlwareName}"); await _next.Invoke(context); - _logger.LogDebug($"returned to {MiddlwareName} after next middleware completed"); + _logger.LogTrace($"returned to {MiddlwareName} after next middleware completed"); if (PipelineError) { var errors = PipelineErrors; - _logger.LogDebug($"{errors.Count} pipeline errors found in {MiddlwareName}. Setting error response status code"); + _logger.LogError($"{errors.Count} pipeline errors found in {MiddlwareName}. Setting error response status code"); SetErrorResponse(context, errors); } diff --git a/test/Ocelot.UnitTests/Infrastructure/HttpDataRepositoryTests.cs b/test/Ocelot.UnitTests/Infrastructure/HttpDataRepositoryTests.cs new file mode 100644 index 00000000..0d7c9201 --- /dev/null +++ b/test/Ocelot.UnitTests/Infrastructure/HttpDataRepositoryTests.cs @@ -0,0 +1,73 @@ +using Microsoft.AspNetCore.Http; +using Ocelot.Infrastructure.RequestData; +using Ocelot.Responses; +using Shouldly; +using TestStack.BDDfy; +using Xunit; + +namespace Ocelot.UnitTests.Infrastructure +{ + public class HttpDataRepositoryTests + { + private HttpContext _httpContext; + private IHttpContextAccessor _httpContextAccessor; + private HttpDataRepository _httpDataRepository; + private object _result; + + public HttpDataRepositoryTests() + { + _httpContext = new DefaultHttpContext(); + _httpContextAccessor = new HttpContextAccessor{HttpContext = _httpContext}; + _httpDataRepository = new HttpDataRepository(_httpContextAccessor); + } + + //TODO - Additional tests -> Type mistmatch aka Add string, request int + //TODO - Additional tests -> HttpContent null. This should never happen + + [Fact] + public void Get_returns_correct_key_from_http_context() + { + + this.Given(x => x.GivenAHttpContextContaining("key", "string")) + .When(x => x.GetIsCalledWithKey("key")) + .Then(x => x.ThenTheResultIsAnOkResponse("string")) + .BDDfy(); + } + + [Fact] + public void Get_returns_error_response_if_the_key_is_not_found() //Therefore does not return null + { + this.Given(x => x.GivenAHttpContextContaining("key", "string")) + .When(x => x.GetIsCalledWithKey("keyDoesNotExist")) + .Then(x => x.ThenTheResultIsAnErrorReposnse("string1")) + .BDDfy(); + } + + private void GivenAHttpContextContaining(string key, object o) + { + _httpContext.Items.Add(key, o); + } + + private void GetIsCalledWithKey(string key) + { + _result = _httpDataRepository.Get(key); + } + + private void ThenTheResultIsAnErrorReposnse(object resultValue) + { + _result.ShouldBeOfType>(); + ((ErrorResponse) _result).Data.ShouldBeNull(); + ((ErrorResponse)_result).IsError.ShouldBe(true); + ((ErrorResponse) _result).Errors.ShouldHaveSingleItem() + .ShouldBeOfType() + .Message.ShouldStartWith("Unable to find data for key: "); + } + + private void ThenTheResultIsAnOkResponse(object resultValue) + { + _result.ShouldBeOfType>(); + ((OkResponse)_result).Data.ShouldBe(resultValue); + } + + } +}