#2064 2065 #2132 #2169 Ampersand vs Slash workaround in `UpstreamTemp…

…latePatternCreator` (#2225) * fixed bug in UpstreamTemplatePatternCreator * improved regex pattern + added unit tests * fixed upstream template tests * added acceptance test * pr adaptions * adopted regex related pr change requests * Code review by @raman-m --------- Co-authored-by: Raman Maksimchuk <[email protected]>
ThreeMammals · Dec 10, 2024 · 995b103 · 995b103
1 parent c2aa287
commit 995b103
Show file tree

Hide file tree

Showing 3 changed files with 86 additions and 4 deletions.
diff --git a/src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs b/src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs
@@ -40,7 +40,9 @@ public UpstreamPathTemplate Create(IRoute route)
         if (upstreamTemplate.Contains('?'))
         {
             containsQueryString = true;
-            upstreamTemplate = upstreamTemplate.Replace("?", "(|\\?)");
+            upstreamTemplate = upstreamTemplate.Replace(
+                upstreamTemplate.Contains("/?") ? "/?" : "?",
+                @"(/$|/\?|\?|$)");
         }
 
         for (var i = 0; i < placeholders.Count; i++)

diff --git a/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs b/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs
@@ -666,7 +666,48 @@ public void Should_change_downstream_path_by_upstream_path_when_path_contains_ma
             .And(x => ThenTheDownstreamUrlPathShouldBe(decodedDownstreamUrlPath))
             .BDDfy();
     }
-
+
+    [Fact]
+    [Trait("Bug", "2064")]
+    [Trait("Discus", "2065")]
+    public void Should_match_correct_route_when_placeholder_appears_after_query_start()
+    {
+        const string DownstreamPath = "/1/products/1";
+        var port = PortFinder.GetRandomPort();
+        var configuration = GivenConfiguration(
+            GivenRoute(port, "/{tenantId}/products?{everything}", "/{tenantId}/products?{everything}"), // This route should NOT BE matched
+            GivenRoute(port, "/{tenantId}/products/{everything}", "/{tenantId}/products/{everything}")); // This route should BE matched
+        this.Given(x => GivenThereIsAServiceRunningOn(port, DownstreamPath, HttpStatusCode.OK, "Hello from Finn"))
+            .And(x => GivenThereIsAConfiguration(configuration))
+            .And(x => GivenOcelotIsRunning())
+            .When(x => WhenIGetUrlOnTheApiGateway("/1/products/1"))
+            .Then(x => ThenTheDownstreamUrlPathShouldBe(DownstreamPath))
+            .And(x => ThenTheDownstreamUrlQueryStringShouldBe(string.Empty))
+            .And(x => ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
+            .And(x => ThenTheResponseBodyShouldBe("Hello from Finn"))
+            .BDDfy();
+    }
+
+    [Fact]
+    [Trait("Bug", "2132")]
+    public void Should_match_correct_route_when_a_configuration_exists_with_query_param_wildcard()
+    {
+        const string DownstreamPath = "/api/v1/apple";
+        var port = PortFinder.GetRandomPort();
+        var configuration = GivenConfiguration(
+            GivenRoute(port, "/api/v1/abc?{everything}",  "/api/v1/abc?{everything}"), // This route should NOT be matched
+            GivenRoute(port, "/api/v1/abc2/{everything}", "/api/v1/{everything}")); // This route should be matched
+        this.Given(x => GivenThereIsAServiceRunningOn(port, DownstreamPath, HttpStatusCode.OK, "Hello from Finn"))
+            .And(x => GivenThereIsAConfiguration(configuration))
+            .And(x => GivenOcelotIsRunning())
+            .When(x => WhenIGetUrlOnTheApiGateway("/api/v1/abc2/apple?isRequired=1"))
+            .Then(x => ThenTheDownstreamUrlPathShouldBe(DownstreamPath))
+            .And(x => ThenTheDownstreamUrlQueryStringShouldBe("?isRequired=1"))
+            .And(x => ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
+            .And(x => ThenTheResponseBodyShouldBe("Hello from Finn"))
+            .BDDfy();
+    }
+
     private void GivenThereIsAServiceRunningOn(int port, string basePath, HttpStatusCode statusCode, string responseBody)
     {
         var baseUrl = DownstreamUrl(port);

diff --git a/test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs b/test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs
@@ -1,6 +1,7 @@
 using Ocelot.Configuration.Creator;
 using Ocelot.Configuration.File;
 using Ocelot.Values;
+using System.Text.RegularExpressions;
 
 namespace Ocelot.UnitTests.Configuration;
 
@@ -215,7 +216,7 @@ public void should_create_template_pattern_that_matches_query_string()
 
         this.Given(x => x.GivenTheFollowingFileRoute(fileRoute))
             .When(x => x.WhenICreateTheTemplatePattern())
-            .Then(x => x.ThenTheFollowingIsReturned($"^(?i)/api/subscriptions/[^/]+/updates(|\\?)unitId={MatchEverything}$"))
+            .Then(x => x.ThenTheFollowingIsReturned($@"^(?i)/api/subscriptions/[^/]+/updates(/$|/\?|\?|$)unitId={MatchEverything}$"))
             .And(x => ThenThePriorityIs(1))
             .BDDfy();
     }
@@ -230,11 +231,49 @@ public void should_create_template_pattern_that_matches_query_string_with_multip
 
         this.Given(x => x.GivenTheFollowingFileRoute(fileRoute))
             .When(x => x.WhenICreateTheTemplatePattern())
-            .Then(x => x.ThenTheFollowingIsReturned($"^(?i)/api/subscriptions/[^/]+/updates(|\\?)unitId={MatchEverything}&productId={MatchEverything}$"))
+            .Then(x => x.ThenTheFollowingIsReturned($@"^(?i)/api/subscriptions/[^/]+/updates(/$|/\?|\?|$)unitId={MatchEverything}&productId={MatchEverything}$"))
             .And(x => ThenThePriorityIs(1))
             .BDDfy();
     }
 
+    [Theory]
+    [Trait("Bug", "2064")]
+    [InlineData("/{tenantId}/products?{everything}", "/1/products/1", false)]
+    [InlineData("/{tenantId}/products/{everything}", "/1/products/1", true)]
+    public void Should_not_match_when_placeholder_appears_after_query_start(string urlPathTemplate, string requestPath, bool shouldMatch)
+    {
+        // Arrange
+        GivenTheFollowingFileRoute(new() { UpstreamPathTemplate = urlPathTemplate });
+
+        // Act
+        WhenICreateTheTemplatePattern();
+
+        // Assert
+        ShouldMatchWithRegex(requestPath, shouldMatch);
+    }
+
+    [Theory]
+    [Trait("Bug", "2132")]
+    [InlineData("/api/v1/abc?{everything}", "/api/v1/abc2/apple", false)]
+    [InlineData("/api/v1/abc2/{everything}", "/api/v1/abc2/apple", true)]
+    public void Should_not_match_with_query_param_wildcard(string urlPathTemplate, string requestPath, bool shouldMatch)
+    {
+        // Arrange
+        GivenTheFollowingFileRoute(new() { UpstreamPathTemplate = urlPathTemplate });
+
+        // Act
+        WhenICreateTheTemplatePattern();
+
+        // Assert
+        ShouldMatchWithRegex(requestPath, shouldMatch);
+    }
+
+    private void ShouldMatchWithRegex(string requestPath, bool shouldMatch)
+    {
+        var match = Regex.Match(requestPath, _result.Template);
+        Assert.Equal(shouldMatch, match.Success);
+    }
+
     private void GivenTheFollowingFileRoute(FileRoute fileRoute)
     {
         _fileRoute = fileRoute;