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

@@ -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<string>("RequestId");
+
+            if (requestId == null || requestId.IsError)
+            {
+                return $"{message} : OcelotRequestId - not set";
+            }
+
+            return $"{message} : OcelotRequestId - {requestId.Data}";
+        }
+    }
+}

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<T>()
+        {
+            var logger = _loggerFactory.CreateLogger<T>();
+            return new AspDotNetLogger(logger, _scopedDataRepository);
+        }
+    }
+}

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<T>();
     }
-
+    /// <summary>
-    public class AspDotNetLoggerFactory : IOcelotLoggerFactory
+    /// Thin wrapper around the DotNet core logging framework, used to allow the scopedDataRepository to be injected giving access to the Ocelot RequestId
-    {
+    /// </summary>
-        private readonly ILoggerFactory _loggerFactory;
-        private readonly IRequestScopedDataRepository _scopedDataRepository;
-
-        public AspDotNetLoggerFactory(ILoggerFactory loggerFactory, IRequestScopedDataRepository scopedDataRepository)
-        {
-            _loggerFactory = loggerFactory;
-            _scopedDataRepository = scopedDataRepository;
-        }
-
-        public IOcelotLogger CreateLogger<T>()
-        {
-            var logger = _loggerFactory.CreateLogger<T>();
-            return new AspDotNetLogger(logger, _scopedDataRepository);
-        }
-    }
-
     public interface IOcelotLogger
     {
         void LogTrace(string message, params object[] args);
         void LogDebug(string message, params object[] args);
         void LogError(string message, Exception exception);
-    }
+        void LogError(string message, params object[] args);
-
-    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<string>("RequestId");
-
-            if (requestId != null && !requestId.IsError)
-            {
-                return $"{message} : OcelotRequestId - {requestId.Data}";
-                
-            }
-            return $"{message} : OcelotRequestId - not set";
-        }
     }
 }

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);
             }

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<string>("key"))
+                .Then(x => x.ThenTheResultIsAnOkResponse<string>("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<string>("keyDoesNotExist"))
+                .Then(x => x.ThenTheResultIsAnErrorReposnse<string>("string1"))
+                .BDDfy();
+        }
+
+        private void GivenAHttpContextContaining(string key, object o)
+        {
+            _httpContext.Items.Add(key, o);
+        }
+
+        private void GetIsCalledWithKey<T>(string key)
+        {
+            _result = _httpDataRepository.Get<T>(key);
+        }
+
+        private void ThenTheResultIsAnErrorReposnse<T>(object resultValue)
+        {
+            _result.ShouldBeOfType<ErrorResponse<T>>();
+            ((ErrorResponse<T>) _result).Data.ShouldBeNull();
+            ((ErrorResponse<T>)_result).IsError.ShouldBe(true);
+            ((ErrorResponse<T>) _result).Errors.ShouldHaveSingleItem()
+                .ShouldBeOfType<CannotFindDataError>()
+                .Message.ShouldStartWith("Unable to find data for key: ");
+        }
+
+        private void ThenTheResultIsAnOkResponse<T>(object resultValue)
+        {
+            _result.ShouldBeOfType<OkResponse<T>>();
+            ((OkResponse<T>)_result).Data.ShouldBe(resultValue);
+        }
+
+    }
+}