#2346 Fix problem with downstream URLs where query parameter names get messed up if they contain a placeholder that has a non-empty value

README.md

@@ -2,8 +2,8 @@
 
 [![Release Status](https://github.com/ThreeMammals/Ocelot/actions/workflows/release.yml/badge.svg)](https://github.com/ThreeMammals/Ocelot/actions/workflows/release.yml)
 [![Development Status](https://github.com/ThreeMammals/Ocelot/actions/workflows/develop.yml/badge.svg)](https://github.com/ThreeMammals/Ocelot/actions/workflows/develop.yml)
-[![ReadTheDocs](https://readthedocs.org/projects/ocelot/badge/?version=latest&style=flat-square)](https://app.readthedocs.org/projects/ocelot/builds/?version__slug=latest)
-[![Coveralls](https://coveralls.io/repos/github/ThreeMammals/Ocelot/badge.svg?branch=main)](https://coveralls.io/github/ThreeMammals/Ocelot?branch=main)
+[![ReadTheDocs](https://readthedocs.org/projects/ocelot/badge/?version=develop&style=flat-square)](https://app.readthedocs.org/projects/ocelot/builds/?version__slug=develop)
+[![Coveralls](https://coveralls.io/repos/github/ThreeMammals/Ocelot/badge.svg?branch=develop)](https://coveralls.io/github/ThreeMammals/Ocelot?branch=develop)
 [![License: MIT](https://img.shields.io/badge/License-MIT-yellow.svg)](https://github.com/ThreeMammals/Ocelot/blob/main/LICENSE.md)
 [![NuGet](https://img.shields.io/nuget/v/Ocelot?logo=nuget&label=NuGet&color=blue)](https://www.nuget.org/packages/Ocelot/ "Download Ocelot from NuGet.org")
 [![Downloads](https://img.shields.io/nuget/dt/Ocelot?logo=nuget&label=Downloads)](https://www.nuget.org/packages/Ocelot/ "Total Ocelot downloads from NuGet.org")

src/Ocelot/DownstreamRouteFinder/UrlMatcher/PlaceholderNameAndValue.cs

@@ -1,7 +1,12 @@
+using Ocelot.Infrastructure;
+
 namespace Ocelot.DownstreamRouteFinder.UrlMatcher;
 
 public class PlaceholderNameAndValue
 {
+    private const char OpeningBrace = Placeholders.OpeningBrace;
+    private const char ClosingBrace = Placeholders.ClosingBrace;
+
     public PlaceholderNameAndValue(string name, string value)
     {
         Name = name;
@@ -11,5 +16,6 @@ public PlaceholderNameAndValue(string name, string value)
     public string Name { get; }
     public string Value { get; }
 
-    public override string ToString() => $"[{{{Name}}}={Value}]";
+    public string Key { get => Name.Trim(OpeningBrace, ClosingBrace); }
+    public override string ToString() => $"[{Name}={Value}]";
 }

src/Ocelot/DownstreamUrlCreator/DownstreamUrlCreatorMiddleware.cs

@@ -1,7 +1,7 @@
 using Microsoft.AspNetCore.Http;
+using Microsoft.AspNetCore.WebUtilities;
 using Ocelot.Configuration;
 using Ocelot.DownstreamRouteFinder.UrlMatcher;
-using Ocelot.Infrastructure;
 using Ocelot.Infrastructure.Extensions;
 using Ocelot.Logging;
 using Ocelot.Middleware;
@@ -19,8 +19,6 @@ public class DownstreamUrlCreatorMiddleware : OcelotMiddleware
 
     private const char Ampersand = '&';
     private const char QuestionMark = '?';
-    private const char OpeningBrace = Placeholders.OpeningBrace;
-    private const char ClosingBrace = Placeholders.ClosingBrace;
     protected const char Slash = '/';
 
     public DownstreamUrlCreatorMiddleware(
@@ -80,8 +78,8 @@ public async Task Invoke(HttpContext context)
             }
             else
             {
-                RemoveQueryStringParametersThatHaveBeenUsedInTemplate(downstreamRequest, placeholders);
                 downstreamRequest.AbsolutePath = dsPath;
+                downstreamRequest.Query = RemoveQueryStringParametersThatHaveBeenUsedInTemplate(downstreamRequest, placeholders);
             }
         }
 
@@ -102,50 +100,41 @@ protected static string MergeQueryStringsWithoutDuplicateValues(string queryStri
         var newQueries = HttpUtility.ParseQueryString(newQueryString);
 
         // Remove old replaced query parameters
-        var placeholderNames = new HashSet<string>(placeholders.Select(p => p.Name.Trim(OpeningBrace, ClosingBrace)));
-        foreach (var queryKey in queries.AllKeys.Where(placeholderNames.Contains))
+        var placeholderKeys = new HashSet<string>(placeholders.Select(p => p.Key));
+        foreach (var queryKey in queries.AllKeys.Where(placeholderKeys.Contains))
         {
             queries.Remove(queryKey);
         }
 
         var parameters = newQueries.AllKeys
-            .Where(key => !string.IsNullOrEmpty(key))
+            .Where(key => key.IsNotEmpty())
             .ToDictionary(key => key, key => newQueries[key]);
 
         _ = queries.AllKeys
-            .Where(key => !string.IsNullOrEmpty(key) && !parameters.ContainsKey(key))
+            .Where(key => key.IsNotEmpty() && !parameters.ContainsKey(key))
             .All(key => parameters.TryAdd(key, queries[key]));
 
-        return QuestionMark + string.Join(Ampersand, parameters.Select(MapQueryParameter));
+        return QueryHelpers.AddQueryString(string.Empty, parameters);
     }
 
-    protected static string MapQueryParameter(KeyValuePair<string, string> pair) => $"{pair.Key}={pair.Value}";
-
     /// <summary>
-    /// Feature <see href="https://github.com/ThreeMammals/Ocelot/pull/467">467</see>:
-    /// Added support for query string parameters in upstream path template.
+    /// Feature 467: <see href="https://github.com/ThreeMammals/Ocelot/pull/467">Added support for query string parameters in upstream path template</see>.
     /// </summary>
-    protected static void RemoveQueryStringParametersThatHaveBeenUsedInTemplate(DownstreamRequest downstreamRequest, List<PlaceholderNameAndValue> templatePlaceholders)
+    /// <returns>A <see cref="string"/> object without wanted parameters.</returns>
+    protected static string RemoveQueryStringParametersThatHaveBeenUsedInTemplate(DownstreamRequest request, List<PlaceholderNameAndValue> templatePlaceholders)
     {
-        var builder = new StringBuilder();
-        foreach (var nAndV in templatePlaceholders)
+        if (templatePlaceholders.Count == 0 || request.Query.IsEmpty())
         {
-            var name = nAndV.Name.Trim(OpeningBrace, ClosingBrace);
-            var parameter = $"{name}={nAndV.Value}";
-            if (!downstreamRequest.Query.Contains(parameter))
-            {
-                continue;
-            }
+            return request.Query;
+        }
 
-            int questionMarkOrAmpersand = downstreamRequest.Query.IndexOf(name, StringComparison.Ordinal);
-            builder.Clear()
-                .Append(downstreamRequest.Query)
-                .Replace(parameter, string.Empty)
-                .Remove(--questionMarkOrAmpersand, 1);
-            downstreamRequest.Query = builder.Length > 0
-                ? builder.Remove(0, 1).Insert(0, QuestionMark).ToString()
-                : string.Empty;
+        var query = QueryHelpers.ParseQuery(request.Query);
+        foreach (var placeholder in templatePlaceholders.Where(p => query.ContainsKey(p.Key)))
+        {
+            query.Remove(placeholder.Key);
         }
+
+        return QueryHelpers.AddQueryString(string.Empty, query);
     }
 
     protected static ReadOnlySpan<char> GetPath(ReadOnlySpan<char> downstreamPath)

test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs

@@ -345,13 +345,13 @@ public void Should_return_response_200_with_placeholder_for_final_url_path2(stri
     }
 
     [Theory]
-    [Trait("Bug", "748")]
-    [InlineData("/downstream/test/{everything}", "/upstream/test/{everything}", "/upstream/test/1", "/downstream/test/1", "?p1=v1&p2=v2&something-else")]
-    [InlineData("/downstream/test/{everything}", "/upstream/test/{everything}", "/upstream/test/", "/downstream/test/", "?p1=v1&p2=v2&something-else")]
-    [InlineData("/downstream/test/{everything}", "/upstream/test/{everything}", "/upstream/test", "/downstream/test", "?p1=v1&p2=v2&something-else")]
+    [Trait("Bug", "748")] // https://github.com/ThreeMammals/Ocelot/issues/748
+    [InlineData("/downstream/test/{everything}", "/upstream/test/{everything}", "/upstream/test/1", "/downstream/test/1", "?p1=v1&p2=v2&something-else=")]
+    [InlineData("/downstream/test/{everything}", "/upstream/test/{everything}", "/upstream/test/", "/downstream/test/", "?p1=v1&p2=v2&something-else=")]
+    [InlineData("/downstream/test/{everything}", "/upstream/test/{everything}", "/upstream/test", "/downstream/test", "?p1=v1&p2=v2&something-else=")]
     [InlineData("/downstream/test/{everything}", "/upstream/test/{everything}", "/upstream/test123", null, null)]
-    [InlineData("/downstream/{version}/test/{everything}", "/upstream/{version}/test/{everything}", "/upstream/v1/test/123", "/downstream/v1/test/123", "?p1=v1&p2=v2&something-else")]
-    [InlineData("/downstream/{version}/test", "/upstream/{version}/test", "/upstream/v1/test", "/downstream/v1/test", "?p1=v1&p2=v2&something-else")]
+    [InlineData("/downstream/{version}/test/{everything}", "/upstream/{version}/test/{everything}", "/upstream/v1/test/123", "/downstream/v1/test/123", "?p1=v1&p2=v2&something-else=")]
+    [InlineData("/downstream/{version}/test", "/upstream/{version}/test", "/upstream/v1/test", "/downstream/v1/test", "?p1=v1&p2=v2&something-else=")]
     [InlineData("/downstream/{version}/test", "/upstream/{version}/test", "/upstream/test", null, null)]
     public void Should_return_correct_downstream_when_omitting_ending_placeholder(string downstreamPathTemplate, string upstreamPathTemplate, string requestURL, string downstreamURL, string queryString)
     {