#683 Validate placeholder duplicates in path templates (#1927)

* add validation for downstream path also * add similiar route placeholders * Add unit tests * Refactor unit tests * IDE1006: Naming rule violation * IDE0028: Collection Initialization can be simplified * Merge into one theory * Less `IEnumerable<T>` usage to have less `IEnumerator<T>` objects in favor of the collection one * Refactor validation of duplicated placeholders * Finish unit testing * Publish hidden Service Fabric feature * Update acceptance tests * Update integration tests * Update release notes --------- The author: Aly Kafoury <@AlyHKafoury> Co-authored-by: Raman Maksimchuk <[email protected]>
ThreeMammals · Mar 26, 2024 · 4f0e483 · 4f0e483
1 parent ded4d7e
commit 4f0e483
Show file tree

Hide file tree

Showing 9 changed files with 374 additions and 198 deletions.
diff --git a/ReleaseNotes.md b/ReleaseNotes.md
@@ -1,39 +1,20 @@
-## January 2024 (version {0}) aka [Hornussen](https://www.myswitzerland.com/en-ch/planning/about-switzerland/custom-and-tradition/hornussen-where-the-nouss-flies-from-the-ramp-and-into-the-playing-field/) release
-> Codenamed as **[Hornussen Sport](https://www.youtube.com/results?search_query=Hornussen)**
-> Read the Docs: [Ocelot 23.1](https://ocelot.readthedocs.io/en/23.1.0/)
+## February 2024 (version {0}) aka [February'24](https://github.com/ThreeMammals/Ocelot/milestone/5) release
+> Codenamed as **[February'24](https://github.com/ThreeMammals/Ocelot/milestone/5)**
+> Read the Docs: [Ocelot 23.2](https://ocelot.readthedocs.io/en/23.2.0/)
 
 ### Focus On
 
 <details>
-  <summary><b>Multiplexing middleware</b> aka <a href="https://ocelot.readthedocs.io/en/latest/features/requestaggregation.html">Request Aggregation</a> feature</summary>
+  <summary><b>New features of</b>: Service Fabric and ...</summary>
 
-- Significant refactoring and design review of the [Multiplexer](https://github.com/ThreeMammals/Ocelot/tree/develop/src/Ocelot/Multiplexer)
-- Optimizing multiplexer performance: `HttpContext` is not copied when there is only one downstream route, and etc.
-- Fixed [the bug](https://github.com/ThreeMammals/Ocelot/pull/1462) in the multiplexer: `HttpContext.User` information was not copied if there was more than one downstream request.
-</details>
-
-<details>
-  <summary><b>System routing</b>. Content streaming when <a href="https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding">Transfer-Encoding</a>: 'chunked'</summary>
-
-  - Correction of [the bug](https://github.com/ThreeMammals/Ocelot/pull/1972) when creating requests: The header [Transfer-Encoding](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Transfer-Encoding): `chunked` was present even when there was no content or the request body size was 0. These cases are now addressed.
-</details>
-
-<details>
-  <summary><b>Updates of the features</b>: QoS, Load Balancer and Error Status Codes</summary>
-
-- [Quality of Service](https://ocelot.readthedocs.io/en/latest/features/qualityofservice.html): Possibility of implementation of custom Polly v8.2 providers. New `AddPolly` extension methods.
-- [Load Balancer](https://ocelot.readthedocs.io/en/latest/features/loadbalancer.html): Extension of the route key format, ensuring that the key remains unique for cases of **UpstreamHost** route property and **ServiceName** vs **ServiceNamespace** properties in Consul setup.
-- [Error Status Codes](https://ocelot.readthedocs.io/en/latest/features/errorcodes.html): When [413 Content Too Large](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/413), Ocelot now returns a 413 `PayloadTooLargeError` (Ocelot error code `41`).
-</details>
-
-<details>
-  <summary>Documentation for <b>Request Aggregation</b></summary>
-
-  - [Request Aggregation](https://ocelot.readthedocs.io/en/latest/features/requestaggregation.html)
+- **[Service Fabric](https://ocelot.readthedocs.io/en/latest/features/servicefabric.html)**: Published old undocumented "[Placeholders in Service Name](https://ocelot.readthedocs.io/en/23.2.0/features/servicefabric.html#placeholders-in-service-name)" feature of [Service Fabric](https://ocelot.readthedocs.io/en/23.2.0/features/servicefabric.html) [service discovery provider](https://ocelot.readthedocs.io/en/23.2.0/search.html?q=ServiceDiscoveryProvider). This feature is available starting from version [13.0.0](https://github.com/ThreeMammals/Ocelot/releases/tag/13.0.0).
 </details>
 
 <details>
   <summary><b>Stabilization</b> aka bug fixing</summary>
 
-  - See [all bugs](https://github.com/ThreeMammals/Ocelot/issues?q=is%3Aissue+is%3Aclosed+label%3Abug+milestone%3AJanuary%2724) of the [January'24](https://github.com/ThreeMammals/Ocelot/milestone/4) milestone
+  - [683](https://github.com/ThreeMammals/Ocelot/issues/683) by PR [1927](https://github.com/ThreeMammals/Ocelot/pull/1927)
+    Ocelot configuration validation logic has updated with [new rules](https://github.com/search?q=repo%3AThreeMammals%2FOcelot+IsPlaceholderNotDuplicatedIn+IsUpstreamPlaceholderDefinedInDownstream+IsDownstreamPlaceholderDefinedInUpstream&type=code) to search for placeholder duplicates in path templates.
+    See more in the [FileConfigurationFluentValidator](https://github.com/search?q=repo%3AThreeMammals%2FOcelot%20FileConfigurationFluentValidator&type=code) class.
+  - See [all bugs](https://github.com/ThreeMammals/Ocelot/issues?q=is%3Aissue+is%3Aclosed+label%3Abug+milestone%3AFebruary%2724) of the [February'24](https://github.com/ThreeMammals/Ocelot/milestone/5) milestone
 </details>
diff --git a/docs/features/servicefabric.rst b/docs/features/servicefabric.rst
@@ -9,7 +9,7 @@ We also need to set up the **ServiceDiscoveryProvider** in **GlobalConfiguration
 The example here shows a typical configuration.
 It assumes *Service Fabric* is running on ``localhost`` and that the naming service is on port ``19081``.
 
-The example below is taken from the `samples/OcelotServiceFabric <https://github.com/ThreeMammals/Ocelot/tree/main/samples/OcelotServiceFabric>`_ folder so please check it if this doesn't make sense!
+The example below is taken from the `OcelotServiceFabric <https://github.com/ThreeMammals/Ocelot/tree/main/samples/OcelotServiceFabric>`_ sample, so please check it if this doesn't make sense!
 
 .. code-block:: json
 
@@ -24,6 +24,7 @@ The example below is taken from the `samples/OcelotServiceFabric <https://github
       }
     ],
     "GlobalConfiguration": {
+      "BaseUrl": "https://ocelot.com"
       "RequestIdKey": "OcRequestId",
       "ServiceDiscoveryProvider": {
         "Host": "localhost",
@@ -36,6 +37,61 @@ The example below is taken from the `samples/OcelotServiceFabric <https://github
 If you are using stateless / guest exe services, Ocelot will be able to proxy through the naming service without anything else.
 However, if you are using statefull / actor services, you must send the **PartitionKind** and **PartitionKey** query string values with the client request e.g.
 
-    GET http://ocelot.com/EquipmentInterfaces?PartitionKind=xxx&PartitionKey=xxx
+    GET ``http://ocelot.com/EquipmentInterfaces?PartitionKind=xxx&PartitionKey=xxx``
 
 There is no way for Ocelot to work these out for you.
+
+.. _sf-placeholders:
+
+Placeholders in Service Name [#f1]_
+-----------------------------------
+
+In Ocelot, you can insert placeholders for variables into your ``UpstreamPathTemplate`` and ``ServiceName`` using the format ``{something}``.
+
+It's important to note that the placeholder variable must exist in both the (**DownstreamPathTemplate** vs **ServiceName**) and the **UpstreamPathTemplate**.
+The **UpstreamPathTemplate** should include all placeholders from the **DownstreamPathTemplate** and **ServiceName**;
+otherwise, Ocelot will not start due to validation errors, which are logged.
+
+Once the validation stage is cleared, Ocelot will replace the placeholder values in the **UpstreamPathTemplate** with those in the **DownstreamPathTemplate** and/or **ServiceName** for each processed request.
+Thus, the :ref:`sf-placeholders` behave similarly to the :ref:`routing-placeholders` feature, but with the **ServiceName** property considered during the processing.
+
+
+Placeholders example
+^^^^^^^^^^^^^^^^^^^^
+
+Here is the example of variable ``version`` in *Service Fabric* service name.
+
+**Given** you have the following `ocelot.json`_:
+
+.. code-block:: json
+
+  {
+    "Routes": [
+      {
+        "UpstreamPathTemplate": "/api/{version}/{endpoint}",
+        "DownstreamPathTemplate": "/{endpoint}",
+        "ServiceName": "Service_{version}/Api",
+      }
+    ],
+    "GlobalConfiguration": {
+      "BaseUrl": "https://ocelot.com"
+      "ServiceDiscoveryProvider": {
+        "Host": "localhost",
+        "Port": 19081,
+        "Type": "ServiceFabric"
+      }
+    }
+  }
+
+**When** you make a request: GET ``https://ocelot.com/api/1.0/products``
+
+**Then** the *Service Fabric* request: GET ``http://localhost:19081/Service_1.0/Api/products``
+
+""""
+
+.. [#f1] ":ref:`sf-placeholders`" feature was requested in issue `721`_ and delivered by PR `722`_ as a part of the version `13.0.0`_.
+
+.. _ocelot.json: https://github.com/ThreeMammals/Ocelot/blob/main/test/Ocelot.ManualTest/ocelot.json
+.. _721: https://github.com/ThreeMammals/Ocelot/issues/721
+.. _722: https://github.com/ThreeMammals/Ocelot/pull/722
+.. _13.0.0: https://github.com/ThreeMammals/Ocelot/releases/tag/13.0.0
diff --git a/src/Ocelot/Configuration/Validator/FileConfigurationFluentValidator.cs b/src/Ocelot/Configuration/Validator/FileConfigurationFluentValidator.cs
@@ -7,10 +7,13 @@
 
 namespace Ocelot.Configuration.Validator
 {
-    public class FileConfigurationFluentValidator : AbstractValidator<FileConfiguration>, IConfigurationValidator
+    /// <summary>
+    /// Validation of a <see cref="FileConfiguration"/> objects.
+    /// </summary>
+    public partial class FileConfigurationFluentValidator : AbstractValidator<FileConfiguration>, IConfigurationValidator
     {
         private const string Servicefabric = "servicefabric";
-        private readonly List<ServiceDiscoveryFinderDelegate> _serviceDiscoveryFinderDelegates;
+        private readonly List<ServiceDiscoveryFinderDelegate> _serviceDiscoveryFinderDelegates;
 
         public FileConfigurationFluentValidator(IServiceProvider provider, RouteFluentValidator routeFluentValidator, FileGlobalConfigurationFluentValidator fileGlobalConfigurationFluentValidator)
         {
@@ -34,7 +37,17 @@ public FileConfigurationFluentValidator(IServiceProvider provider, RouteFluentVa
 
             RuleForEach(configuration => configuration.Routes)
                 .Must((_, route) => IsPlaceholderNotDuplicatedIn(route.UpstreamPathTemplate))
-                .WithMessage((_, route) => $"{nameof(route)} {route.UpstreamPathTemplate} has duplicated placeholder");
+                .WithMessage((_, route) => $"{nameof(route.UpstreamPathTemplate)} '{route.UpstreamPathTemplate}' has duplicated placeholder");
+            RuleForEach(configuration => configuration.Routes)
+                .Must((_, route) => IsPlaceholderNotDuplicatedIn(route.DownstreamPathTemplate))
+                .WithMessage((_, route) => $"{nameof(route.DownstreamPathTemplate)} '{route.DownstreamPathTemplate}' has duplicated placeholder");
+
+            RuleForEach(configuration => configuration.Routes)
+                .Must(IsUpstreamPlaceholderDefinedInDownstream)
+                .WithMessage((_, route) => $"{nameof(route.UpstreamPathTemplate)} '{route.UpstreamPathTemplate}' doesn't contain the same placeholders in {nameof(route.DownstreamPathTemplate)} '{route.DownstreamPathTemplate}'");
+            RuleForEach(configuration => configuration.Routes)
+                .Must(IsDownstreamPlaceholderDefinedInUpstream)
+                .WithMessage((_, route) => $"{nameof(route.DownstreamPathTemplate)} '{route.DownstreamPathTemplate}' doesn't contain the same placeholders in {nameof(route.UpstreamPathTemplate)} '{route.UpstreamPathTemplate}'");
 
             RuleFor(configuration => configuration.GlobalConfiguration.ServiceDiscoveryProvider)
                 .Must(HaveServiceDiscoveryProviderRegistered)
@@ -93,14 +106,52 @@ private static bool AllRoutesForAggregateExist(FileAggregateRoute fileAggregateR
 
             return routesForAggregate.Count() == fileAggregateRoute.RouteKeys.Count;
         }
-
-        private static bool IsPlaceholderNotDuplicatedIn(string upstreamPathTemplate)
+
+#if NET7_0_OR_GREATER
+        [GeneratedRegex(@"\{\w+\}", RegexOptions.IgnoreCase | RegexOptions.Singleline, "en-US")]
+        private static partial Regex PlaceholderRegex();
+#else
+        private static readonly Regex PlaceholderRegexVar = new(@"\{\w+\}", RegexOptions.IgnoreCase | RegexOptions.Singleline, TimeSpan.FromMilliseconds(1000));
+        private static Regex PlaceholderRegex() => PlaceholderRegexVar;
+#endif
+
+        private static bool IsPlaceholderNotDuplicatedIn(string pathTemplate)
         {
-            var regExPlaceholder = new Regex("{[^}]+}");
-            var matches = regExPlaceholder.Matches(upstreamPathTemplate);
-            var upstreamPathPlaceholders = matches.Select(m => m.Value);
-            return upstreamPathPlaceholders.Count() == upstreamPathPlaceholders.Distinct().Count();
-        }
+            var placeholders = PlaceholderRegex().Matches(pathTemplate)
+                .Select(m => m.Value).ToList();
+            return placeholders.Count == placeholders.Distinct().Count();
+        }
+
+        private static bool IsServiceFabricWithServiceName(FileConfiguration configuration, FileRoute route)
+            => Servicefabric.Equals(configuration?.GlobalConfiguration?.ServiceDiscoveryProvider?.Type, StringComparison.InvariantCultureIgnoreCase)
+                && !string.IsNullOrEmpty(route?.ServiceName) && PlaceholderRegex().IsMatch(route.ServiceName);
+
+        private bool IsUpstreamPlaceholderDefinedInDownstream(FileConfiguration configuration, FileRoute route)
+            => IsServiceFabricWithServiceName(configuration, route)
+                ? IsPlaceholderDefinedInBothTemplates(route.UpstreamPathTemplate, route.ServiceName + route.DownstreamPathTemplate)
+                : IsPlaceholderDefinedInBothTemplates(route.UpstreamPathTemplate, route.DownstreamPathTemplate);
+
+        private bool IsDownstreamPlaceholderDefinedInUpstream(FileConfiguration configuration, FileRoute route)
+            => IsServiceFabricWithServiceName(configuration, route)
+                ? IsPlaceholderDefinedInBothTemplates(route.ServiceName + route.DownstreamPathTemplate, route.UpstreamPathTemplate)
+                : IsPlaceholderDefinedInBothTemplates(route.DownstreamPathTemplate, route.UpstreamPathTemplate);
+
+        private static bool IsPlaceholderDefinedInBothTemplates(string firstPathTemplate, string secondPathTemplate)
+        {
+            var firstPlaceholders = PlaceholderRegex().Matches(firstPathTemplate)
+                .Select(m => m.Value).ToList();
+            var secondPlaceholders = PlaceholderRegex().Matches(secondPathTemplate)
+                .Select(m => m.Value).ToList();
+            foreach (var placeholder in firstPlaceholders)
+            {
+                if (!secondPlaceholders.Contains(placeholder))
+                {
+                    return false;
+                }
+            }
+
+            return true;
+        }
 
         private static bool DoesNotContainRoutesWithSpecificRequestIdKeys(FileAggregateRoute fileAggregateRoute,
             IEnumerable<FileRoute> routes)
@@ -131,7 +182,7 @@ private static bool IsNotDuplicateIn(FileRoute route,
 
             var duplicateSpecificVerbs = matchingRoutes.SelectMany(x => x.UpstreamHttpMethod).GroupBy(x => x.ToLower()).SelectMany(x => x.Skip(1)).Any();
 
-            if (duplicateAllowAllVerbs || duplicateSpecificVerbs || (allowAllVerbs && specificVerbs))
+            if (duplicateAllowAllVerbs || duplicateSpecificVerbs || allowAllVerbs && specificVerbs)
             {
                 return false;
             }
@@ -155,6 +206,6 @@ private static bool IsNotDuplicateIn(FileAggregateRoute route, IEnumerable<FileA
             var matchingRoutes = aggregateRoutes
                 .Where(r => r.UpstreamPathTemplate == route.UpstreamPathTemplate & r.UpstreamHost == route.UpstreamHost);
             return matchingRoutes.Count() <= 1;
-        }
+        }
     }
 }
diff --git a/test/Ocelot.AcceptanceTests/AggregateTests.cs b/test/Ocelot.AcceptanceTests/AggregateTests.cs
@@ -187,7 +187,7 @@ public void Should_return_response_200_with_advanced_aggregate_configs()
                                 Port = port2,
                             },
                         ],
-                        UpstreamPathTemplate = "/UserDetails",
+                        UpstreamPathTemplate = "/UserDetails/{userId}",
                         UpstreamHttpMethod = ["Get"],
                         Key = "UserDetails",
                     },
@@ -203,7 +203,7 @@ public void Should_return_response_200_with_advanced_aggregate_configs()
                                 Port = port3,
                             },
                         ],
-                        UpstreamPathTemplate = "/PostDetails",
+                        UpstreamPathTemplate = "/PostDetails/{postId}",
                         UpstreamHttpMethod = ["Get"],
                         Key = "PostDetails",
                     },

diff --git a/test/Ocelot.AcceptanceTests/ClaimsToDownstreamPathTests.cs b/test/Ocelot.AcceptanceTests/ClaimsToDownstreamPathTests.cs
@@ -61,7 +61,7 @@ public void should_return_200_and_change_downstream_path()
                                },
                            },
                            DownstreamScheme = "http",
-                           UpstreamPathTemplate = "/users",
+                           UpstreamPathTemplate = "/users/{userId}",
                            UpstreamHttpMethod = new List<string> { "Get" },
                            AuthenticationOptions = new FileAuthenticationOptions
                            {