Feature/fix #156 (#160)

* change config creator to not throw exception in there is an error......lord i hate this config creator code I need to sort it out.

* Remove method that we are not using anymore..

* throw exception and add errors to message

* train hacking and some refactoring

* bs test for code coverage

* actually return the errors in the exception

src/Ocelot/Configuration/Creator/FileOcelotConfigurationCreator.cs

@@ -67,34 +67,19 @@ namespace Ocelot.Configuration.Creator
             _httpHandlerOptionsCreator = httpHandlerOptionsCreator;
         }
         
-        public async Task<Response<IOcelotConfiguration>> Create()
-        {     
-            var config = await SetUpConfiguration(_options.Value);
-
-            return new OkResponse<IOcelotConfiguration>(config);
-        }
-
         public async Task<Response<IOcelotConfiguration>> Create(FileConfiguration fileConfiguration)
         {     
             var config = await SetUpConfiguration(fileConfiguration);
-
+            return config;
-            return new OkResponse<IOcelotConfiguration>(config);
         }
 
-        private async Task<IOcelotConfiguration> SetUpConfiguration(FileConfiguration fileConfiguration)
+        private async Task<Response<IOcelotConfiguration>> SetUpConfiguration(FileConfiguration fileConfiguration)
         {
             var response = await _configurationValidator.IsValid(fileConfiguration);
 
             if (response.Data.IsError)
             {
-                var errorBuilder = new StringBuilder();
+                return new ErrorResponse<IOcelotConfiguration>(response.Data.Errors);
-
-                foreach (var error in response.Errors)
-                {
-                    errorBuilder.AppendLine(error.Message);
-                }
-
-                throw new Exception($"Unable to start Ocelot..configuration, errors were {errorBuilder}");
             }
 
             var reRoutes = new List<ReRoute>();
@@ -107,7 +92,9 @@ namespace Ocelot.Configuration.Creator
 
             var serviceProviderConfiguration = _serviceProviderConfigCreator.Create(fileConfiguration.GlobalConfiguration);
             
-            return new OcelotConfiguration(reRoutes, fileConfiguration.GlobalConfiguration.AdministrationPath, serviceProviderConfiguration);
+            var config = new OcelotConfiguration(reRoutes, fileConfiguration.GlobalConfiguration.AdministrationPath, serviceProviderConfiguration);
+
+            return new OkResponse<IOcelotConfiguration>(config);
         }
 
         private ReRoute SetUpReRoute(FileReRoute fileReRoute, FileGlobalConfiguration globalConfiguration)

src/Ocelot/Configuration/Creator/IOcelotConfigurationCreator.cs

@@ -6,7 +6,6 @@ namespace Ocelot.Configuration.Creator
 {
     public interface IOcelotConfigurationCreator
     {
-        Task<Response<IOcelotConfiguration>> Create();
         Task<Response<IOcelotConfiguration>> Create(FileConfiguration fileConfiguration);
     }
 }

src/Ocelot/Errors/Error.cs

@@ -10,5 +10,10 @@ namespace Ocelot.Errors
 
         public string Message { get; private set; }
         public OcelotErrorCode Code { get; private set; }
+
+        public override string ToString()
+        {
+            return Message;
+        }
     }
 }

src/Ocelot/Middleware/OcelotMiddlewareExtensions.cs

@@ -23,6 +23,7 @@ using Ocelot.RateLimit.Middleware;
 namespace Ocelot.Middleware
 {
     using System;
+    using System.Linq;
     using System.Threading.Tasks;
     using Authorisation.Middleware;
     using Microsoft.AspNetCore.Hosting;
@@ -150,61 +151,108 @@ namespace Ocelot.Middleware
 
         private static async Task<IOcelotConfiguration> CreateConfiguration(IApplicationBuilder builder)
         {
-            var fileConfig = (IOptions<FileConfiguration>)builder.ApplicationServices.GetService(typeof(IOptions<FileConfiguration>));
+            var deps = GetDependencies(builder);
 
-            var configSetter = (IFileConfigurationSetter)builder.ApplicationServices.GetService(typeof(IFileConfigurationSetter));
+            var ocelotConfiguration = await deps.provider.Get();
 
-            var configProvider = (IOcelotConfigurationProvider)builder.ApplicationServices.GetService(typeof(IOcelotConfigurationProvider));
+            if (ConfigurationNotSetUp(ocelotConfiguration))
-
-            var ocelotConfiguration = await configProvider.Get();
-
-            if (ocelotConfiguration == null || ocelotConfiguration.Data == null || ocelotConfiguration.IsError)
             {
-                Response config = null;
+                var response = await SetConfig(builder, deps.fileConfiguration, deps.setter, deps.provider, deps.repo);
-                var fileConfigRepo = builder.ApplicationServices.GetService(typeof(IFileConfigurationRepository));
-                if (fileConfigRepo.GetType() == typeof(ConsulFileConfigurationRepository))
-                {
-                    var consulFileConfigRepo = (ConsulFileConfigurationRepository) fileConfigRepo;
-                    var ocelotConfigurationRepository =
-                        (IOcelotConfigurationRepository) builder.ApplicationServices.GetService(
-                            typeof(IOcelotConfigurationRepository));
-                    var ocelotConfigurationCreator =
-                        (IOcelotConfigurationCreator) builder.ApplicationServices.GetService(
-                            typeof(IOcelotConfigurationCreator));
                 
-                    var fileConfigFromConsul = await consulFileConfigRepo.Get();
+                if (UnableToSetConfig(response))
-                    if (fileConfigFromConsul.Data == null)
-                    {
-                        config = await configSetter.Set(fileConfig.Value);
-                    }
-                    else
-                    {
-                        var ocelotConfig = await ocelotConfigurationCreator.Create(fileConfigFromConsul.Data);
-                        config = await ocelotConfigurationRepository.AddOrReplace(ocelotConfig.Data);
-                        var hack = builder.ApplicationServices.GetService(typeof(ConsulFileConfigurationPoller));
-                    }
-                }
-                else
                 {
-                    config = await configSetter.Set(fileConfig.Value);
+                    ThrowToStopOcelotStarting(response);
-                }
-
-                if (config == null || config.IsError)
-                {
-                    throw new Exception("Unable to start Ocelot: configuration was not set up correctly.");
                 }
             }
 
-            ocelotConfiguration = await configProvider.Get();
+            return await GetOcelotConfigAndReturn(deps.provider);
+        }
+
+        private static async Task<Response> SetConfig(IApplicationBuilder builder, IOptions<FileConfiguration> fileConfiguration, IFileConfigurationSetter setter, IOcelotConfigurationProvider provider, IFileConfigurationRepository repo)
+        {
+            if (UsingConsul(repo))
+            {
+                return await SetUpConfigFromConsul(builder, repo, setter, fileConfiguration);
+            }
+            
+            return await setter.Set(fileConfiguration.Value);
+        }
+
+        private static bool UnableToSetConfig(Response response)
+        {
+            return response == null || response.IsError;
+        }
+
+        private static bool ConfigurationNotSetUp(Response<IOcelotConfiguration> ocelotConfiguration)
+        {
+            return ocelotConfiguration == null || ocelotConfiguration.Data == null || ocelotConfiguration.IsError;
+        }
+
+        private static (IOptions<FileConfiguration> fileConfiguration, IFileConfigurationSetter setter, IOcelotConfigurationProvider provider, IFileConfigurationRepository repo) GetDependencies(IApplicationBuilder builder)
+        {
+            var fileConfiguration = (IOptions<FileConfiguration>)builder.ApplicationServices.GetService(typeof(IOptions<FileConfiguration>));
+            
+            var setter = (IFileConfigurationSetter)builder.ApplicationServices.GetService(typeof(IFileConfigurationSetter));
+            
+            var provider = (IOcelotConfigurationProvider)builder.ApplicationServices.GetService(typeof(IOcelotConfigurationProvider));
+
+            var repo = (IFileConfigurationRepository)builder.ApplicationServices.GetService(typeof(IFileConfigurationRepository));
+
+            return (fileConfiguration, setter, provider, repo);
+        }
+
+        private static async Task<IOcelotConfiguration> GetOcelotConfigAndReturn(IOcelotConfigurationProvider provider)
+        {
+            var ocelotConfiguration = await provider.Get();
 
             if(ocelotConfiguration == null || ocelotConfiguration.Data == null || ocelotConfiguration.IsError)
             {
-                throw new Exception("Unable to start Ocelot: ocelot configuration was not returned by provider.");
+                ThrowToStopOcelotStarting(ocelotConfiguration);
             }
 
             return ocelotConfiguration.Data;
         }
 
+        private static void ThrowToStopOcelotStarting(Response config)
+        {
+            throw new Exception($"Unable to start Ocelot, errors are: {string.Join(",", config.Errors.Select(x => x.ToString()))}");
+        }
+
+        private static bool UsingConsul(IFileConfigurationRepository fileConfigRepo)
+        {
+            return fileConfigRepo.GetType() == typeof(ConsulFileConfigurationRepository);
+        }
+
+        private static async Task<Response> SetUpConfigFromConsul(IApplicationBuilder builder, IFileConfigurationRepository consulFileConfigRepo, IFileConfigurationSetter setter, IOptions<FileConfiguration> fileConfig)
+        {
+            Response config = null;
+
+            var ocelotConfigurationRepository =
+                (IOcelotConfigurationRepository) builder.ApplicationServices.GetService(
+                    typeof(IOcelotConfigurationRepository));
+            var ocelotConfigurationCreator =
+                (IOcelotConfigurationCreator) builder.ApplicationServices.GetService(
+                    typeof(IOcelotConfigurationCreator));
+
+            var fileConfigFromConsul = await consulFileConfigRepo.Get();
+            if (fileConfigFromConsul.Data == null)
+            {
+                config = await setter.Set(fileConfig.Value);
+            }
+            else
+            {
+                var ocelotConfig = await ocelotConfigurationCreator.Create(fileConfigFromConsul.Data);
+                if(ocelotConfig.IsError)
+                {
+                    return new ErrorResponse(ocelotConfig.Errors);
+                }
+                config = await ocelotConfigurationRepository.AddOrReplace(ocelotConfig.Data);
+                var hack = builder.ApplicationServices.GetService(typeof(ConsulFileConfigurationPoller));
+            }
+
+            return new OkResponse();
+        }
+
         private static async Task CreateAdministrationArea(IApplicationBuilder builder, IOcelotConfiguration configuration)
         {
             var identityServerConfiguration = (IIdentityServerConfiguration)builder.ApplicationServices.GetService(typeof(IIdentityServerConfiguration));

test/Ocelot.AcceptanceTests/CannotStartOcelotTests.cs

@@ -0,0 +1,58 @@
+
+using System;
+using System.Collections.Generic;
+using Microsoft.AspNetCore.Hosting;
+using Ocelot.Configuration.File;
+using Shouldly;
+using TestStack.BDDfy;
+using Xunit;
+
+namespace Ocelot.AcceptanceTests
+{
+    public class CannotStartOcelotTests : IDisposable
+    {
+        private IWebHost _builder;
+        private readonly Steps _steps;
+        private string _downstreamPath;
+
+        public CannotStartOcelotTests()
+        {
+            _steps = new Steps();
+        }
+
+        [Fact]
+        public void should_throw_exception_if_cannot_start()
+        {
+            var invalidConfig = new FileConfiguration()
+            {
+                ReRoutes = new List<FileReRoute>
+                {
+                    new FileReRoute
+                    {
+                        UpstreamPathTemplate = "api",
+                        DownstreamPathTemplate = "test"
+                    }
+                }
+            };
+
+            Exception exception = null;
+            _steps.GivenThereIsAConfiguration(invalidConfig);
+            try
+            {
+                _steps.GivenOcelotIsRunning();
+            }
+            catch(Exception ex)
+            {
+                exception = ex;
+            }
+
+            exception.ShouldNotBeNull();
+        }
+        
+        public void Dispose()
+        {
+            _builder?.Dispose();
+            _steps.Dispose();
+        }
+    }
+}

test/Ocelot.UnitTests/Configuration/FileConfigurationCreatorTests.cs

@@ -15,6 +15,7 @@ using Xunit;
 
 namespace Ocelot.UnitTests.Configuration
 {
+    using Ocelot.Errors;
     using Ocelot.UnitTests.TestData;
 
     public class FileConfigurationCreatorTests
@@ -114,19 +115,6 @@ namespace Ocelot.UnitTests.Configuration
                 .BDDfy();
         }
 
-        private void GivenTheFollowingRegionIsReturned(string region)
-        {
-            _regionCreator
-                .Setup(x => x.Create(It.IsAny<FileReRoute>()))
-                .Returns(region);
-        }
-
-        private void ThenTheRegionCreatorIsCalledCorrectly(string expected)
-        {
-            _regionCreator
-                .Verify(x => x.Create(_fileConfiguration.ReRoutes[0]), Times.Once);
-        }
-
         [Fact]
         public void should_call_rate_limit_options_creator()
         {
@@ -441,17 +429,6 @@ namespace Ocelot.UnitTests.Configuration
                 .BDDfy();
         }
 
-        private void GivenTheFollowingHttpHandlerOptionsAreReturned(HttpHandlerOptions httpHandlerOptions)
-        {
-            _httpHandlerOptionsCreator.Setup(x => x.Create(It.IsAny<FileReRoute>()))
-                .Returns(httpHandlerOptions);
-        }
-
-        private void ThenTheHttpHandlerOptionsCreatorIsCalledCorrectly()
-        {
-            _httpHandlerOptionsCreator.Verify(x => x.Create(_fileConfiguration.ReRoutes[0]), Times.Once());
-        }
-
         [Theory]
         [MemberData(nameof(AuthenticationConfigTestData.GetAuthenticationData), MemberType = typeof(AuthenticationConfigTestData))]
         public void should_create_with_headers_to_extract(FileConfiguration fileConfig)
@@ -523,6 +500,31 @@ namespace Ocelot.UnitTests.Configuration
                 .BDDfy();
         }
 
+        [Fact]
+        public void should_return_validation_errors()
+        {
+            var errors = new List<Error> {new PathTemplateDoesntStartWithForwardSlash("some message")};
+
+            this.Given(x => x.GivenTheConfigIs(new FileConfiguration()))
+                .And(x => x.GivenTheConfigIsInvalid(errors))
+                .When(x => x.WhenICreateTheConfig())
+                .Then(x => x.ThenTheErrorsAreReturned(errors))
+                .BDDfy();
+        }
+
+        private void GivenTheConfigIsInvalid(List<Error> errors)
+        {
+            _validator
+                .Setup(x => x.IsValid(It.IsAny<FileConfiguration>()))
+                .ReturnsAsync(new OkResponse<ConfigurationValidationResult>(new ConfigurationValidationResult(true, errors)));
+        }
+
+        private void ThenTheErrorsAreReturned(List<Error> errors)
+        {
+            _config.IsError.ShouldBeTrue();
+            _config.Errors[0].ShouldBe(errors[0]);
+        }
+
         private void GivenTheFollowingOptionsAreReturned(ReRouteOptions fileReRouteOptions)
         {
             _fileReRouteOptionsCreator
@@ -553,7 +555,7 @@ namespace Ocelot.UnitTests.Configuration
 
         private void WhenICreateTheConfig()
         {
-            _config = _ocelotConfigurationCreator.Create().Result;
+            _config = _ocelotConfigurationCreator.Create(_fileConfiguration).Result;
         }
 
         private void ThenTheReRoutesAre(List<ReRoute> expectedReRoutes)
@@ -651,5 +653,30 @@ namespace Ocelot.UnitTests.Configuration
             _serviceProviderConfigCreator
                 .Setup(x => x.Create(It.IsAny<FileGlobalConfiguration>())).Returns(serviceProviderConfiguration);
         }
+
+        
+        private void GivenTheFollowingRegionIsReturned(string region)
+        {
+            _regionCreator
+                .Setup(x => x.Create(It.IsAny<FileReRoute>()))
+                .Returns(region);
+        }
+
+        private void ThenTheRegionCreatorIsCalledCorrectly(string expected)
+        {
+            _regionCreator
+                .Verify(x => x.Create(_fileConfiguration.ReRoutes[0]), Times.Once);
+        }
+        
+        private void GivenTheFollowingHttpHandlerOptionsAreReturned(HttpHandlerOptions httpHandlerOptions)
+        {
+            _httpHandlerOptionsCreator.Setup(x => x.Create(It.IsAny<FileReRoute>()))
+                .Returns(httpHandlerOptions);
+        }
+
+        private void ThenTheHttpHandlerOptionsCreatorIsCalledCorrectly()
+        {
+            _httpHandlerOptionsCreator.Verify(x => x.Create(_fileConfiguration.ReRoutes[0]), Times.Once());
+        }
     }
 }

test/Ocelot.UnitTests/Errors/ErrorTests.cs

@@ -0,0 +1,18 @@
+using Ocelot.Errors;
+using Ocelot.Infrastructure.RequestData;
+using Shouldly;
+using Xunit;
+
+namespace Ocelot.UnitTests.Errors
+{
+    public class ErrorTests
+    {
+        [Fact]
+        public void should_return_message()
+        {
+            var error = new CannotAddDataError("message");
+            var result = error.ToString();
+            result.ShouldBe("message");
+        }
+    }
+}