Follow up #1670: Fix StyleCop issues and improve the code (#1711)

* Code refactoring and improvements

* Fix unit tests for ConsulServiceDiscoveryProvider

* Add default port

* Throw NullRef exception if no client

* Fix 'should_not_get' unit test

* Add AgentService extensions

* Use current naming conventions for unit tests

* Fix code smell with methos signature

* Encapsulate warning string creation.
Create expected value from a hardcoded string in unit tests

Author: raman-m

samples/OcelotServiceDiscovery/ApiGateway/ServiceDiscovery/MyServiceDiscoveryProvider.cs

@@ -20,7 +20,7 @@ public MyServiceDiscoveryProvider(IServiceProvider serviceProvider, ServiceProvi
         _downstreamRoute = downstreamRoute;
     }
 
-    public Task<List<Service>> Get()
+    public Task<List<Service>> GetAsync()
     {
 
         // Returns a list of service(s) that match the downstream route passed to the provider

src/Ocelot.Provider.Consul/Consul.cs

@@ -14,20 +14,21 @@ public class Consul : IServiceDiscoveryProvider
 
     public Consul(ConsulRegistryConfiguration config, IOcelotLoggerFactory factory, IConsulClientFactory clientFactory)
     {
-        _logger = factory.CreateLogger<Consul>();
         _config = config;
         _consul = clientFactory.Get(_config);
+        _logger = factory.CreateLogger<Consul>();
     }
 
-    public async Task<List<Service>> Get()
+    public async Task<List<Service>> GetAsync()
     {
         var queryResult = await _consul.Health.Service(_config.KeyOfServiceInConsul, string.Empty, true);
 
         var services = new List<Service>();
 
         foreach (var serviceEntry in queryResult.Response)
         {
-            if (IsValid(serviceEntry))
+            var service = serviceEntry.Service;
+            if (IsValid(service))
             {
                 var nodes = await _consul.Catalog.Nodes();
                 if (nodes.Response == null)
@@ -36,14 +37,14 @@ public async Task<List<Service>> Get()
                 }
                 else
                 {
-                    var serviceNode = nodes.Response.FirstOrDefault(n => n.Address == serviceEntry.Service.Address);
+                    var serviceNode = nodes.Response.FirstOrDefault(n => n.Address == service.Address);
                     services.Add(BuildService(serviceEntry, serviceNode));
                 }
             }
             else
             {
                 _logger.LogWarning(
-                    $"Unable to use service Address: {serviceEntry.Service.Address} and Port: {serviceEntry.Service.Port} as it is invalid. Address must contain host only e.g. localhost and port must be greater than 0");
+                    $"Unable to use service address: '{service.Address}' and port: {service.Port} as it is invalid for the service: '{service.Service}'. Address must contain host only e.g. 'localhost', and port must be greater than 0.");
             }
         }
 
@@ -52,27 +53,24 @@ public async Task<List<Service>> Get()
 
     private static Service BuildService(ServiceEntry serviceEntry, Node serviceNode)
     {
+        var service = serviceEntry.Service;
         return new Service(
-            serviceEntry.Service.Service,
-            new ServiceHostAndPort(serviceNode == null ? serviceEntry.Service.Address : serviceNode.Name,
-                serviceEntry.Service.Port),
-            serviceEntry.Service.ID,
-            GetVersionFromStrings(serviceEntry.Service.Tags),
-            serviceEntry.Service.Tags ?? Enumerable.Empty<string>());
+            service.Service,
+            new ServiceHostAndPort(
+                serviceNode == null ? service.Address : serviceNode.Name,
+                service.Port),
+            service.ID,
+            GetVersionFromStrings(service.Tags),
+            service.Tags ?? Enumerable.Empty<string>());
     }
 
-    private static bool IsValid(ServiceEntry serviceEntry)
-    {
-        return !string.IsNullOrEmpty(serviceEntry.Service.Address)
-               && !serviceEntry.Service.Address.Contains("http://")
-               && !serviceEntry.Service.Address.Contains("https://")
-               && serviceEntry.Service.Port > 0;
-    }
+    private static bool IsValid(AgentService service)
+        => !string.IsNullOrEmpty(service.Address)
+        && !service.Address.Contains($"{Uri.UriSchemeHttp}://")
+        && !service.Address.Contains($"{Uri.UriSchemeHttps}://")
+        && service.Port > 0;
 
     private static string GetVersionFromStrings(IEnumerable<string> strings)
-    {
-        return strings
-            ?.FirstOrDefault(x => x.StartsWith(VersionPrefix, StringComparison.Ordinal))
+        => strings?.FirstOrDefault(x => x.StartsWith(VersionPrefix, StringComparison.Ordinal))
             .TrimStart(VersionPrefix);
-    }
 }

src/Ocelot.Provider.Consul/ConsulClientFactory.cs

@@ -3,12 +3,15 @@
 public class ConsulClientFactory : IConsulClientFactory
 {
     public IConsulClient Get(ConsulRegistryConfiguration config)
+        => new ConsulClient(c => OverrideConfig(c, config));
+
+    private static void OverrideConfig(ConsulClientConfiguration to, ConsulRegistryConfiguration from)
     {
-        return new ConsulClient(c =>
-        {
-            c.Address = new Uri($"{config.Scheme}://{config.Host}:{config.Port}");
+        to.Address = new Uri($"{from.Scheme}://{from.Host}:{from.Port}");
 
-            if (!string.IsNullOrEmpty(config?.Token)) c.Token = config.Token;
-        });
+        if (!string.IsNullOrEmpty(from?.Token))
+        {
+            to.Token = from.Token;
+        }
     }
 }

src/Ocelot.Provider.Consul/ConsulFileConfigurationRepository.cs

@@ -1,11 +1,12 @@
-using System.Text;
-using Microsoft.Extensions.Options;
+using Microsoft.Extensions.Options;
 using Newtonsoft.Json;
 using Ocelot.Cache;
+using Ocelot.Configuration;
 using Ocelot.Configuration.File;
 using Ocelot.Configuration.Repository;
 using Ocelot.Logging;
 using Ocelot.Responses;
+using System.Text;
 
 namespace Ocelot.Provider.Consul;
 
@@ -25,37 +26,32 @@ public ConsulFileConfigurationRepository(
         _logger = loggerFactory.CreateLogger<ConsulFileConfigurationRepository>();
         _cache = cache;
 
-        var serviceDiscoveryProvider = fileConfiguration.Value.GlobalConfiguration.ServiceDiscoveryProvider;
-        _configurationKey = string.IsNullOrWhiteSpace(serviceDiscoveryProvider.ConfigurationKey)
-            ? "InternalConfiguration"
-            : serviceDiscoveryProvider.ConfigurationKey;
-
-        var config = new ConsulRegistryConfiguration(serviceDiscoveryProvider.Scheme, serviceDiscoveryProvider.Host,
-            serviceDiscoveryProvider.Port, _configurationKey, serviceDiscoveryProvider.Token);
+        var provider = fileConfiguration.Value.GlobalConfiguration.ServiceDiscoveryProvider;
+        _configurationKey = string.IsNullOrWhiteSpace(provider.ConfigurationKey)
+            ? nameof(InternalConfiguration)
+            : provider.ConfigurationKey;
 
+        var config = new ConsulRegistryConfiguration(provider.Scheme, provider.Host,
+            provider.Port, _configurationKey, provider.Token);
         _consul = factory.Get(config);
     }
 
     public async Task<Response<FileConfiguration>> Get()
     {
         var config = _cache.Get(_configurationKey, _configurationKey);
-
         if (config != null)
         {
             return new OkResponse<FileConfiguration>(config);
         }
 
         var queryResult = await _consul.KV.Get(_configurationKey);
-
         if (queryResult.Response == null)
         {
             return new OkResponse<FileConfiguration>(null);
         }
 
         var bytes = queryResult.Response.Value;
-
         var json = Encoding.UTF8.GetString(bytes);
-
         var consulConfig = JsonConvert.DeserializeObject<FileConfiguration>(json);
 
         return new OkResponse<FileConfiguration>(consulConfig);
@@ -64,16 +60,13 @@ public async Task<Response<FileConfiguration>> Get()
     public async Task<Response> Set(FileConfiguration ocelotConfiguration)
     {
         var json = JsonConvert.SerializeObject(ocelotConfiguration, Formatting.Indented);
-
         var bytes = Encoding.UTF8.GetBytes(json);
-
         var kvPair = new KVPair(_configurationKey)
         {
             Value = bytes,
         };
 
         var result = await _consul.KV.Put(kvPair);
-
         if (result.Response)
         {
             _cache.AddAndDelete(_configurationKey, ocelotConfiguration, TimeSpan.FromSeconds(3), _configurationKey);
@@ -82,6 +75,6 @@ public async Task<Response> Set(FileConfiguration ocelotConfiguration)
         }
 
         return new ErrorResponse(new UnableToSetConfigInConsulError(
-            $"Unable to set FileConfiguration in consul, response status code from consul was {result.StatusCode}"));
+            $"Unable to set {nameof(FileConfiguration)} in {nameof(Consul)}, response status code from {nameof(Consul)} was {result.StatusCode}"));
     }
 }

src/Ocelot.Provider.Consul/ConsulMiddlewareConfigurationProvider.cs

@@ -11,7 +11,9 @@ namespace Ocelot.Provider.Consul;
 
 public static class ConsulMiddlewareConfigurationProvider
 {
-    public static OcelotMiddlewareConfigurationDelegate Get = async builder =>
+    public static OcelotMiddlewareConfigurationDelegate Get { get; } = GetAsync;
+
+    private static async Task GetAsync(IApplicationBuilder builder)
     {
         var fileConfigRepo = builder.ApplicationServices.GetService<IFileConfigurationRepository>();
         var fileConfig = builder.ApplicationServices.GetService<IOptionsMonitor<FileConfiguration>>();
@@ -22,34 +24,30 @@ public static class ConsulMiddlewareConfigurationProvider
         {
             await SetFileConfigInConsul(builder, fileConfigRepo, fileConfig, internalConfigCreator, internalConfigRepo);
         }
-    };
+    }
 
     private static bool UsingConsul(IFileConfigurationRepository fileConfigRepo)
-    {
-        return fileConfigRepo.GetType() == typeof(ConsulFileConfigurationRepository);
-    }
+        => fileConfigRepo.GetType() == typeof(ConsulFileConfigurationRepository);
 
     private static async Task SetFileConfigInConsul(IApplicationBuilder builder,
         IFileConfigurationRepository fileConfigRepo, IOptionsMonitor<FileConfiguration> fileConfig,
         IInternalConfigurationCreator internalConfigCreator, IInternalConfigurationRepository internalConfigRepo)
     {
-        // get the config from consul.
+        // Get the config from Consul
         var fileConfigFromConsul = await fileConfigRepo.Get();
-
         if (IsError(fileConfigFromConsul))
         {
             ThrowToStopOcelotStarting(fileConfigFromConsul);
         }
         else if (ConfigNotStoredInConsul(fileConfigFromConsul))
         {
-            //there was no config in consul set the file in config in consul
+            // there was no config in Consul set the file in config in Consul
             await fileConfigRepo.Set(fileConfig.CurrentValue);
         }
         else
         {
-            // create the internal config from consul data
+            // Create the internal config from Consul data
             var internalConfig = await internalConfigCreator.Create(fileConfigFromConsul.Data);
-
             if (IsError(internalConfig))
             {
                 ThrowToStopOcelotStarting(internalConfig);
@@ -58,7 +56,6 @@ private static async Task SetFileConfigInConsul(IApplicationBuilder builder,
             {
                 // add the internal config to the internal repo
                 var response = internalConfigRepo.AddOrReplace(internalConfig.Data);
-
                 if (IsError(response))
                 {
                     ThrowToStopOcelotStarting(response);
@@ -73,18 +70,11 @@ private static async Task SetFileConfigInConsul(IApplicationBuilder builder,
     }
 
     private static void ThrowToStopOcelotStarting(Response config)
-    {
-        throw new Exception(
-            $"Unable to start Ocelot, errors are: {string.Join(',', config.Errors.Select(x => x.ToString()))}");
-    }
+        => throw new Exception($"Unable to start Ocelot, errors are: {string.Join(',', config.Errors.Select(x => x.ToString()))}");
 
     private static bool IsError(Response response)
-    {
-        return response == null || response.IsError;
-    }
+        => response == null || response.IsError;
 
     private static bool ConfigNotStoredInConsul(Response<FileConfiguration> fileConfigFromConsul)
-    {
-        return fileConfigFromConsul.Data == null;
-    }
+        => fileConfigFromConsul.Data == null;
 }

src/Ocelot.Provider.Consul/ConsulProviderFactory.cs

@@ -8,7 +8,7 @@ namespace Ocelot.Provider.Consul;
 public static class ConsulProviderFactory
 {
     /// <summary>
-    ///     String constant used for provider type definition.
+    /// String constant used for provider type definition.
     /// </summary>
     public const string PollConsul = nameof(Provider.Consul.PollConsul);
 
@@ -32,17 +32,14 @@ private static IServiceDiscoveryProvider CreateProvider(IServiceProvider provide
         {
             lock (LockObject)
             {
-                var discoveryProvider =
-                    ServiceDiscoveryProviders.FirstOrDefault(x => x.ServiceName == route.ServiceName);
+                var discoveryProvider = ServiceDiscoveryProviders.FirstOrDefault(x => x.ServiceName == route.ServiceName);
                 if (discoveryProvider != null)
                 {
                     return discoveryProvider;
                 }
 
-                discoveryProvider = new PollConsul(
-                    config.PollingInterval, route.ServiceName, factory, consulProvider);
+                discoveryProvider = new PollConsul(config.PollingInterval, route.ServiceName, factory, consulProvider);
                 ServiceDiscoveryProviders.Add(discoveryProvider);
-
                 return discoveryProvider;
             }
         }

src/Ocelot.Provider.Consul/ConsulRegistryConfiguration.cs

@@ -2,11 +2,19 @@
 
 public class ConsulRegistryConfiguration
 {
+    /// <summary>
+    /// Consul HTTP client default port.
+    /// <para>
+    /// HashiCorp Developer docs: <see href="https://developer.hashicorp.com/consul">Consul</see> | <see href="https://developer.hashicorp.com/consul/docs/install/ports">Required Ports</see>.
+    /// </para>
+    /// </summary>
+    public const int DefaultHttpPort = 8500;
+
     public ConsulRegistryConfiguration(string scheme, string host, int port, string keyOfServiceInConsul, string token)
     {
         Host = string.IsNullOrEmpty(host) ? "localhost" : host;
-        Port = port > 0 ? port : 8500;
-        Scheme = string.IsNullOrEmpty(scheme) ? "http" : scheme;
+        Port = port > 0 ? port : DefaultHttpPort;
+        Scheme = string.IsNullOrEmpty(scheme) ? Uri.UriSchemeHttp : scheme;
         KeyOfServiceInConsul = keyOfServiceInConsul;
         Token = token;
     }

src/Ocelot.Provider.Consul/OcelotBuilderExtensions.cs

@@ -9,18 +9,20 @@ public static class OcelotBuilderExtensions
 {
     public static IOcelotBuilder AddConsul(this IOcelotBuilder builder)
     {
-        builder.Services.AddSingleton(ConsulProviderFactory.Get);
-        builder.Services.AddSingleton<IConsulClientFactory, ConsulClientFactory>();
-        builder.Services.RemoveAll(typeof(IFileConfigurationPollerOptions));
-        builder.Services.AddSingleton<IFileConfigurationPollerOptions, ConsulFileConfigurationPollerOption>();
+        builder.Services
+            .AddSingleton(ConsulProviderFactory.Get)
+            .AddSingleton<IConsulClientFactory, ConsulClientFactory>()
+            .RemoveAll(typeof(IFileConfigurationPollerOptions))
+            .AddSingleton<IFileConfigurationPollerOptions, ConsulFileConfigurationPollerOption>();
         return builder;
     }
 
     public static IOcelotBuilder AddConfigStoredInConsul(this IOcelotBuilder builder)
     {
-        builder.Services.AddSingleton(ConsulMiddlewareConfigurationProvider.Get);
-        builder.Services.AddHostedService<FileConfigurationPoller>();
-        builder.Services.AddSingleton<IFileConfigurationRepository, ConsulFileConfigurationRepository>();
+        builder.Services
+            .AddSingleton(ConsulMiddlewareConfigurationProvider.Get)
+            .AddHostedService<FileConfigurationPoller>()
+            .AddSingleton<IFileConfigurationRepository, ConsulFileConfigurationRepository>();
         return builder;
     }
 }