From 3b27bb376e5c56e12128365f3400ee2d01d5bbc3 Mon Sep 17 00:00:00 2001 From: Tom Pallister Date: Fri, 24 Nov 2017 21:10:03 +0000 Subject: [PATCH] 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 --- .../Creator/FileOcelotConfigurationCreator.cs | 27 +--- .../Creator/IOcelotConfigurationCreator.cs | 1 - src/Ocelot/Errors/Error.cs | 5 + .../Middleware/OcelotMiddlewareExtensions.cs | 126 ++++++++++++------ .../CannotStartOcelotTests.cs | 58 ++++++++ .../FileConfigurationCreatorTests.cs | 77 +++++++---- test/Ocelot.UnitTests/Errors/ErrorTests.cs | 18 +++ 7 files changed, 227 insertions(+), 85 deletions(-) create mode 100644 test/Ocelot.AcceptanceTests/CannotStartOcelotTests.cs create mode 100644 test/Ocelot.UnitTests/Errors/ErrorTests.cs diff --git a/src/Ocelot/Configuration/Creator/FileOcelotConfigurationCreator.cs b/src/Ocelot/Configuration/Creator/FileOcelotConfigurationCreator.cs index 3aba7c208..f8e2d506e 100644 --- a/src/Ocelot/Configuration/Creator/FileOcelotConfigurationCreator.cs +++ b/src/Ocelot/Configuration/Creator/FileOcelotConfigurationCreator.cs @@ -66,35 +66,20 @@ IHttpHandlerOptionsCreator httpHandlerOptionsCreator _fileReRouteOptionsCreator = fileReRouteOptionsCreator; _httpHandlerOptionsCreator = httpHandlerOptionsCreator; } - - public async Task> Create() - { - var config = await SetUpConfiguration(_options.Value); - - return new OkResponse(config); - } - + public async Task> Create(FileConfiguration fileConfiguration) { var config = await SetUpConfiguration(fileConfiguration); - - return new OkResponse(config); + return config; } - private async Task SetUpConfiguration(FileConfiguration fileConfiguration) + private async Task> SetUpConfiguration(FileConfiguration fileConfiguration) { var response = await _configurationValidator.IsValid(fileConfiguration); if (response.Data.IsError) { - var errorBuilder = new StringBuilder(); - - foreach (var error in response.Errors) - { - errorBuilder.AppendLine(error.Message); - } - - throw new Exception($"Unable to start Ocelot..configuration, errors were {errorBuilder}"); + return new ErrorResponse(response.Data.Errors); } var reRoutes = new List(); @@ -107,7 +92,9 @@ private async Task SetUpConfiguration(FileConfiguration fi 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(config); } private ReRoute SetUpReRoute(FileReRoute fileReRoute, FileGlobalConfiguration globalConfiguration) diff --git a/src/Ocelot/Configuration/Creator/IOcelotConfigurationCreator.cs b/src/Ocelot/Configuration/Creator/IOcelotConfigurationCreator.cs index 99d1d39da..66a2d8aeb 100644 --- a/src/Ocelot/Configuration/Creator/IOcelotConfigurationCreator.cs +++ b/src/Ocelot/Configuration/Creator/IOcelotConfigurationCreator.cs @@ -6,7 +6,6 @@ namespace Ocelot.Configuration.Creator { public interface IOcelotConfigurationCreator { - Task> Create(); Task> Create(FileConfiguration fileConfiguration); } } \ No newline at end of file diff --git a/src/Ocelot/Errors/Error.cs b/src/Ocelot/Errors/Error.cs index 25a9f5d48..9c2145411 100644 --- a/src/Ocelot/Errors/Error.cs +++ b/src/Ocelot/Errors/Error.cs @@ -10,5 +10,10 @@ protected Error(string message, OcelotErrorCode code) public string Message { get; private set; } public OcelotErrorCode Code { get; private set; } + + public override string ToString() + { + return Message; + } } } \ No newline at end of file diff --git a/src/Ocelot/Middleware/OcelotMiddlewareExtensions.cs b/src/Ocelot/Middleware/OcelotMiddlewareExtensions.cs index c89a5b4e4..4929f9b2b 100644 --- a/src/Ocelot/Middleware/OcelotMiddlewareExtensions.cs +++ b/src/Ocelot/Middleware/OcelotMiddlewareExtensions.cs @@ -23,6 +23,7 @@ namespace Ocelot.Middleware { using System; + using System.Linq; using System.Threading.Tasks; using Authorisation.Middleware; using Microsoft.AspNetCore.Hosting; @@ -150,61 +151,108 @@ public static async Task UseOcelot(this IApplicationBuilder private static async Task CreateConfiguration(IApplicationBuilder builder) { - var fileConfig = (IOptions)builder.ApplicationServices.GetService(typeof(IOptions)); - - var configSetter = (IFileConfigurationSetter)builder.ApplicationServices.GetService(typeof(IFileConfigurationSetter)); - - var configProvider = (IOcelotConfigurationProvider)builder.ApplicationServices.GetService(typeof(IOcelotConfigurationProvider)); + var deps = GetDependencies(builder); - var ocelotConfiguration = await configProvider.Get(); + var ocelotConfiguration = await deps.provider.Get(); - if (ocelotConfiguration == null || ocelotConfiguration.Data == null || ocelotConfiguration.IsError) + if (ConfigurationNotSetUp(ocelotConfiguration)) { - Response config = null; - 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 (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 + var response = await SetConfig(builder, deps.fileConfiguration, deps.setter, deps.provider, deps.repo); + + if (UnableToSetConfig(response)) { - 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."); - } + return await GetOcelotConfigAndReturn(deps.provider); + } + + private static async Task SetConfig(IApplicationBuilder builder, IOptions 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 ocelotConfiguration) + { + return ocelotConfiguration == null || ocelotConfiguration.Data == null || ocelotConfiguration.IsError; + } + + private static (IOptions fileConfiguration, IFileConfigurationSetter setter, IOcelotConfigurationProvider provider, IFileConfigurationRepository repo) GetDependencies(IApplicationBuilder builder) + { + var fileConfiguration = (IOptions)builder.ApplicationServices.GetService(typeof(IOptions)); + + var setter = (IFileConfigurationSetter)builder.ApplicationServices.GetService(typeof(IFileConfigurationSetter)); + + var provider = (IOcelotConfigurationProvider)builder.ApplicationServices.GetService(typeof(IOcelotConfigurationProvider)); - ocelotConfiguration = await configProvider.Get(); + var repo = (IFileConfigurationRepository)builder.ApplicationServices.GetService(typeof(IFileConfigurationRepository)); + + return (fileConfiguration, setter, provider, repo); + } + + private static async Task 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 SetUpConfigFromConsul(IApplicationBuilder builder, IFileConfigurationRepository consulFileConfigRepo, IFileConfigurationSetter setter, IOptions 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)); diff --git a/test/Ocelot.AcceptanceTests/CannotStartOcelotTests.cs b/test/Ocelot.AcceptanceTests/CannotStartOcelotTests.cs new file mode 100644 index 000000000..25a9565cc --- /dev/null +++ b/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 + { + 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(); + } + } +} \ No newline at end of file diff --git a/test/Ocelot.UnitTests/Configuration/FileConfigurationCreatorTests.cs b/test/Ocelot.UnitTests/Configuration/FileConfigurationCreatorTests.cs index 094895234..975f71da8 100644 --- a/test/Ocelot.UnitTests/Configuration/FileConfigurationCreatorTests.cs +++ b/test/Ocelot.UnitTests/Configuration/FileConfigurationCreatorTests.cs @@ -15,6 +15,7 @@ namespace Ocelot.UnitTests.Configuration { + using Ocelot.Errors; using Ocelot.UnitTests.TestData; public class FileConfigurationCreatorTests @@ -114,19 +115,6 @@ public void should_call_region_creator() .BDDfy(); } - private void GivenTheFollowingRegionIsReturned(string region) - { - _regionCreator - .Setup(x => x.Create(It.IsAny())) - .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 @@ public void should_call_httpHandler_creator() .BDDfy(); } - private void GivenTheFollowingHttpHandlerOptionsAreReturned(HttpHandlerOptions httpHandlerOptions) - { - _httpHandlerOptionsCreator.Setup(x => x.Create(It.IsAny())) - .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 @@ public void should_create_with_authentication_properties(FileConfiguration fileC .BDDfy(); } + [Fact] + public void should_return_validation_errors() + { + var errors = new List {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 errors) + { + _validator + .Setup(x => x.IsValid(It.IsAny())) + .ReturnsAsync(new OkResponse(new ConfigurationValidationResult(true, errors))); + } + + private void ThenTheErrorsAreReturned(List errors) + { + _config.IsError.ShouldBeTrue(); + _config.Errors[0].ShouldBe(errors[0]); + } + private void GivenTheFollowingOptionsAreReturned(ReRouteOptions fileReRouteOptions) { _fileReRouteOptionsCreator @@ -553,7 +555,7 @@ private void GivenTheConfigIs(FileConfiguration fileConfiguration) private void WhenICreateTheConfig() { - _config = _ocelotConfigurationCreator.Create().Result; + _config = _ocelotConfigurationCreator.Create(_fileConfiguration).Result; } private void ThenTheReRoutesAre(List expectedReRoutes) @@ -651,5 +653,30 @@ private void GivenTheFollowingIsReturned(ServiceProviderConfiguration servicePro _serviceProviderConfigCreator .Setup(x => x.Create(It.IsAny())).Returns(serviceProviderConfiguration); } + + + private void GivenTheFollowingRegionIsReturned(string region) + { + _regionCreator + .Setup(x => x.Create(It.IsAny())) + .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())) + .Returns(httpHandlerOptions); + } + + private void ThenTheHttpHandlerOptionsCreatorIsCalledCorrectly() + { + _httpHandlerOptionsCreator.Verify(x => x.Create(_fileConfiguration.ReRoutes[0]), Times.Once()); + } } } diff --git a/test/Ocelot.UnitTests/Errors/ErrorTests.cs b/test/Ocelot.UnitTests/Errors/ErrorTests.cs new file mode 100644 index 000000000..69f2d15f5 --- /dev/null +++ b/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"); + } + } +} \ No newline at end of file