#952 #1174 Merge query strings without duplicate values (#1182)

* Fix issue  #952 and #1174

* Fix compiling errors

* Fix warnings

* Fix errors

* Remove and Sort Usings

* CA1845 Use span-based 'string.Concat' and 'AsSpan' instead of 'Substring'.
Use 'AsSpan' with 'string.Concat'

* IDE1006 Naming rule violation: These words must begin with upper case characters: {should_*}.
Fix name violation

* Add namespace

* Fix build errors

* Test class should match the name of tested class

* Simplify too long class names, and they should match

* Move to the parent folder which was empty

* Fix warnings

* Process dictionaries using LINQ to Objects approach

* Fix code review issues from @RaynaldM

* Remove tiny private helper with one reference

* Fix warning & messages

* Define theory instead of 2 facts

* Add unit test for issue #952

* Add additional unit test for #952 to keep param

* Add tests for issue #1174

* Remove unnecessary parameter

* Copy routing.rst from released version

* Refactor the middleware body for query params

* Update routing.rst: Describe query string user scenarios

---------

Co-authored-by: Stjepan Majdak <stjepan.majdak@a1.hr>
Co-authored-by: raman-m <dotnet044@gmail.com>

src/Ocelot/DependencyInjection/OcelotBuilder.cs

@@ -16,7 +16,7 @@
 using Ocelot.Configuration.Validator;
 using Ocelot.DownstreamRouteFinder.Finder;
 using Ocelot.DownstreamRouteFinder.UrlMatcher;
-using Ocelot.DownstreamUrlCreator.UrlTemplateReplacer;
+using Ocelot.DownstreamUrlCreator;
 using Ocelot.Headers;
 using Ocelot.Infrastructure;
 using Ocelot.Infrastructure.Claims.Parser;
@@ -103,7 +103,7 @@ public OcelotBuilder(IServiceCollection services, IConfiguration configurationRo
             Services.TryAddSingleton<IClaimsParser, ClaimsParser>();
             Services.TryAddSingleton<IUrlPathToUrlTemplateMatcher, RegExUrlMatcher>();
             Services.TryAddSingleton<IPlaceholderNameAndValueFinder, UrlPathPlaceholderNameAndValueFinder>();
-            Services.TryAddSingleton<IDownstreamPathPlaceholderReplacer, DownstreamTemplatePathPlaceholderReplacer>();
+            Services.TryAddSingleton<IDownstreamPathPlaceholderReplacer, DownstreamPathPlaceholderReplacer>();
             Services.AddSingleton<IDownstreamRouteProvider, DownstreamRouteFinder.Finder.DownstreamRouteFinder>();
             Services.AddSingleton<IDownstreamRouteProvider, DownstreamRouteCreator>();
             Services.TryAddSingleton<IDownstreamRouteProviderFactory, DownstreamRouteProviderFactory>();

src/Ocelot/DownstreamUrlCreator/UrlTemplateReplacer/DownstreamTemplatePathPlaceholderReplacer.cs → src/Ocelot/DownstreamUrlCreator/DownstreamPathPlaceholderReplacer.cs

@@ -2,9 +2,9 @@
 using Ocelot.Responses;
 using Ocelot.Values;
 
-namespace Ocelot.DownstreamUrlCreator.UrlTemplateReplacer
+namespace Ocelot.DownstreamUrlCreator
 {
-    public class DownstreamTemplatePathPlaceholderReplacer : IDownstreamPathPlaceholderReplacer
+    public class DownstreamPathPlaceholderReplacer : IDownstreamPathPlaceholderReplacer
     {
         public Response<DownstreamPath> Replace(string downstreamPathTemplate,
             List<PlaceholderNameAndValue> urlPathPlaceholderNameAndValues)

src/Ocelot/DownstreamUrlCreator/UrlTemplateReplacer/IDownstreamPathPlaceholderReplacer.cs → src/Ocelot/DownstreamUrlCreator/IDownstreamPathPlaceholderReplacer.cs

@@ -2,7 +2,7 @@
 using Ocelot.Responses;
 using Ocelot.Values;
 
-namespace Ocelot.DownstreamUrlCreator.UrlTemplateReplacer
+namespace Ocelot.DownstreamUrlCreator
 {
     public interface IDownstreamPathPlaceholderReplacer
     {

src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs

@@ -1,24 +1,29 @@
 using Microsoft.AspNetCore.Http;
 using Ocelot.Configuration;
 using Ocelot.DownstreamRouteFinder.UrlMatcher;
-using Ocelot.DownstreamUrlCreator.UrlTemplateReplacer;
-using Ocelot.Logging;
-using Ocelot.Middleware;
+using Ocelot.Logging;
+using Ocelot.Middleware;
 using Ocelot.Request.Middleware;
-using Ocelot.Responses;
+using Ocelot.Responses;
 using Ocelot.Values;
-
+using System.Web;
+
 namespace Ocelot.DownstreamUrlCreator.Middleware
 {
     public class DownstreamUrlCreatorMiddleware : OcelotMiddleware
     {
         private readonly RequestDelegate _next;
         private readonly IDownstreamPathPlaceholderReplacer _replacer;
-
-        public DownstreamUrlCreatorMiddleware(RequestDelegate next,
+
+        private const char Ampersand = '&';
+        private const char QuestionMark = '?';
+        private const char OpeningBrace = '{';
+        private const char ClosingBrace = '}';
+
+        public DownstreamUrlCreatorMiddleware(
+            RequestDelegate next,
             IOcelotLoggerFactory loggerFactory,
-            IDownstreamPathPlaceholderReplacer replacer
-            )
+            IDownstreamPathPlaceholderReplacer replacer)
                 : base(loggerFactory.CreateLogger<DownstreamUrlCreatorMiddleware>())
         {
             _next = next;
@@ -28,17 +33,13 @@ IDownstreamPathPlaceholderReplacer replacer
         public async Task Invoke(HttpContext httpContext)
         {
             var downstreamRoute = httpContext.Items.DownstreamRoute();
-
-            var templatePlaceholderNameAndValues = httpContext.Items.TemplatePlaceholderNameAndValues();
-
-            var response = _replacer
-                .Replace(downstreamRoute.DownstreamPathTemplate.Value, templatePlaceholderNameAndValues);
-
+            var placeholders = httpContext.Items.TemplatePlaceholderNameAndValues();
+            var response = _replacer.Replace(downstreamRoute.DownstreamPathTemplate.Value, placeholders);
             var downstreamRequest = httpContext.Items.DownstreamRequest();
 
             if (response.IsError)
             {
-                Logger.LogDebug("IDownstreamPathPlaceholderReplacer returned an error, setting pipeline error");
+                Logger.LogDebug($"{nameof(IDownstreamPathPlaceholderReplacer)} returned an error, setting pipeline error");
 
                 httpContext.Items.UpsertErrors(response.Errors);
                 return;
@@ -54,7 +55,7 @@ public async Task Invoke(HttpContext httpContext)
 
             if (ServiceFabricRequest(internalConfiguration, downstreamRoute))
             {
-                var (path, query) = CreateServiceFabricUri(downstreamRequest, downstreamRoute, templatePlaceholderNameAndValues, response);
+                var (path, query) = CreateServiceFabricUri(downstreamRequest, downstreamRoute, placeholders, response);
 
                 //todo check this works again hope there is a test..
                 downstreamRequest.AbsolutePath = path;
@@ -63,23 +64,17 @@ public async Task Invoke(HttpContext httpContext)
             else
             {
                 var dsPath = response.Data;
-
-                if (ContainsQueryString(dsPath))
+                if (dsPath.Value.Contains(QuestionMark))
                 {
                     downstreamRequest.AbsolutePath = GetPath(dsPath);
-
-                    if (string.IsNullOrEmpty(downstreamRequest.Query))
-                    {
-                        downstreamRequest.Query = GetQueryString(dsPath);
-                    }
-                    else
-                    {
-                        downstreamRequest.Query += GetQueryString(dsPath).Replace('?', '&');
-                    }
+                    var newQuery = GetQueryString(dsPath);
+                    downstreamRequest.Query = string.IsNullOrEmpty(downstreamRequest.Query)
+                        ? newQuery
+                        : MergeQueryStringsWithoutDuplicateValues(downstreamRequest.Query, newQuery, placeholders);
                 }
                 else
                 {
-                    RemoveQueryStringParametersThatHaveBeenUsedInTemplate(downstreamRequest, templatePlaceholderNameAndValues);
+                    RemoveQueryStringParametersThatHaveBeenUsedInTemplate(downstreamRequest, placeholders);
 
                     downstreamRequest.AbsolutePath = dsPath.Value;
                 }
@@ -90,11 +85,35 @@ public async Task Invoke(HttpContext httpContext)
             await _next.Invoke(httpContext);
         }
 
+        private static string MergeQueryStringsWithoutDuplicateValues(string queryString, string newQueryString, List<PlaceholderNameAndValue> placeholders)
+        {
+            newQueryString = newQueryString.Replace(QuestionMark, Ampersand);
+            var queries = HttpUtility.ParseQueryString(queryString);
+            var newQueries = HttpUtility.ParseQueryString(newQueryString);
+
+            var parameters = newQueries.AllKeys
+                .Where(key => !string.IsNullOrEmpty(key))
+                .ToDictionary(key => key, key => newQueries[key]);
+
+            _ = queries.AllKeys
+                .Where(key => !string.IsNullOrEmpty(key) && !parameters.ContainsKey(key))
+                .All(key => parameters.TryAdd(key, queries[key]));
+
+            // Remove old replaced query parameters
+            foreach (var placeholder in placeholders)
+            {
+                parameters.Remove(placeholder.Name.Trim(OpeningBrace, ClosingBrace));
+            }
+
+            var orderedParams = parameters.OrderBy(x => x.Key).Select(x => $"{x.Key}={x.Value}");
+            return QuestionMark + string.Join(Ampersand, orderedParams);
+        }
+
         private static void RemoveQueryStringParametersThatHaveBeenUsedInTemplate(DownstreamRequest downstreamRequest, List<PlaceholderNameAndValue> templatePlaceholderNameAndValues)
         {
             foreach (var nAndV in templatePlaceholderNameAndValues)
             {
-                var name = nAndV.Name.Replace("{", string.Empty).Replace("}", string.Empty);
+                var name = nAndV.Name.Trim(OpeningBrace, ClosingBrace);
 
                 var rgx = new Regex($@"\b{name}={nAndV.Value}\b");
 
@@ -106,29 +125,24 @@ private static void RemoveQueryStringParametersThatHaveBeenUsedInTemplate(Downst
 
                     if (!string.IsNullOrEmpty(downstreamRequest.Query))
                     {
-                        downstreamRequest.Query = string.Concat("?", downstreamRequest.Query.AsSpan(1));
+                        downstreamRequest.Query = QuestionMark + downstreamRequest.Query[1..];
                     }
                 }
-            }
+            } 
         }
 
         private static string GetPath(DownstreamPath dsPath)
         {
-            int length = dsPath.Value.IndexOf('?', StringComparison.Ordinal);
+            int length = dsPath.Value.IndexOf(QuestionMark, StringComparison.Ordinal);
             return dsPath.Value[..length];
         }
 
         private static string GetQueryString(DownstreamPath dsPath)
         {
-            int startIndex = dsPath.Value.IndexOf('?', StringComparison.Ordinal);
+            int startIndex = dsPath.Value.IndexOf(QuestionMark, StringComparison.Ordinal);
             return dsPath.Value[startIndex..];
         }
 
-        private static bool ContainsQueryString(DownstreamPath dsPath)
-        {
-            return dsPath.Value.Contains('?');
-        }
-
         private (string Path, string Query) CreateServiceFabricUri(DownstreamRequest downstreamRequest, DownstreamRoute downstreamRoute, List<PlaceholderNameAndValue> templatePlaceholderNameAndValues, Response<DownstreamPath> dsPath)
         {
             var query = downstreamRequest.Query;
@@ -138,7 +152,7 @@ private static bool ContainsQueryString(DownstreamPath dsPath)
         }
 
         private static bool ServiceFabricRequest(IInternalConfiguration config, DownstreamRoute downstreamRoute)
-        {
+        {
             return config.ServiceProviderConfiguration.Type?.ToLower() == "servicefabric" && downstreamRoute.UseServiceDiscovery;
         }
     }