Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,12 @@ public Response<List<PlaceholderNameAndValue>> Find(string path, string query, s
private static List<Group> FindGroups(string path, string query, string template)
{
template = EscapeExceptBraces(template);
var regexPattern = $"^{RegexPlaceholders().Replace(template, match => $"(?<{match.Groups[1].Value}>[^&]*)")}";
var regexPattern = GenerateRegexPattern(template);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this is dynamic Regex, so the pattern is dynamically generated, but the previous version aka RegexPlaceholders() returns static object.
Well... is it possible to convert somehow to static object to improve the performance?

var testedPath = ShouldSkipQuery(query, template) ? path : $"{path}{query}";

var match = Regex.Match(testedPath, regexPattern);
var foundGroups = match.Groups.Cast<Group>().Skip(1).ToList();

if (foundGroups.Count > 0 || !IsCatchAllPath(template))
{
return foundGroups;
Expand All @@ -80,6 +82,33 @@ private static List<Group> FindGroups(string path, string query, string template
match = Regex.Match($"{testedPath}/", regexPattern);
return match.Groups.Cast<Group>().Skip(1).ToList();
}

/// <summary>
/// The placeholders that are not placed at the end of the template
/// are delimited by forward slashes, only the last one, the catch-all can match
/// more segments.
/// </summary>
/// <param name="escapedTemplate">The escaped path template.</param>
/// <returns>The pattern for values replacement.</returns>
private static string GenerateRegexPattern(string escapedTemplate)
{
// first we count the matches
var placeHoldersCountMatch = RegexPlaceholders().Matches(escapedTemplate);
int placeHoldersCount = placeHoldersCountMatch.Count;

int index = 0;

// we know that the replace process will be started from the beginning of the url
// so we can use a simple counter to determine the last placeholder
var regexPattern = $@"^{RegexPlaceholders().Replace(escapedTemplate, match =>
{
var groupName = match.Groups[1].Value;
index++;
return index == placeHoldersCount ? $"(?<{groupName}>[^&]*)" : $"(?<{groupName}>[^/|&]*)";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this [^/|&] negative matching group filters out all slashes / after closing brace }, before catch all placeholder, right?
And, the last RX-match, for the last Catch-All, just accepts uncounted number of slashes before query string, right?
Query string processing is quite unclear... What's the final generated pattern?

})}";

return regexPattern;
}

private const int CatchAllQueryMilliseconds = 300;
#if NET7_0_OR_GREATER
Expand Down
24 changes: 24 additions & 0 deletions test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,30 @@ public void ShouldNotMatchComplexQueriesCaseSensitive(string downstream, string
.BDDfy();
}

[Theory]
[Trait("Bug", "2212")]
[InlineData("/data-registers/{version}/it/{everything}", "/dati-registri/{version}/{everything}", "/dati-registri/v1.0/operatore/R80QQ5J9600/valida", "/data-registers/v1.0/it/operatore/R80QQ5J9600/valida")]
Copy link
Member

@raman-m raman-m Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this test data from #2212, right?

[InlineData("/files/{version}/uploads/{everything}", "/data/{version}/storage/{everything}", "/data/v2.0/storage/images/photos/nature", "/files/v2.0/uploads/images/photos/nature")]
[InlineData("/resources/{area}/details/{everything}", "/api/resources/{area}/info/{everything}", "/api/resources/global/info/stats/2024/data", "/resources/global/details/stats/2024/data")]
[InlineData("/users/{userId}/logs/{everything}", "/data/users/{userId}/activity/{everything}", "/data/users/12345/activity/session/login/2024", "/users/12345/logs/session/login/2024")]
[InlineData("/orders/{orderId}/items/{everything}", "/ecommerce/{orderId}/details/{everything}", "/ecommerce/98765/details/category/electronics/phone", "/orders/98765/items/category/electronics/phone")]
[InlineData("/tasks/{taskId}/subtasks/{everything}", "/work/{taskId}/breakdown/{everything}", "/work/56789/breakdown/phase/3/step/2", "/tasks/56789/subtasks/phase/3/step/2")]
[InlineData("/configs/{env}/overrides/{everything}", "/settings/{env}/{everything}", "/settings/prod/feature/toggles", "/configs/prod/overrides/feature/toggles")]
public void OnlyTheLastPlaceholderShouldMatchSeveralSegments(string downstream, string upstream, string requestUrl, string downstreamPath)
{
var port = PortFinder.GetRandomPort();
var route = GivenRoute(port, upstream, downstream);
var configuration = GivenConfiguration(route);
this.Given(x => GivenThereIsAServiceRunningOn(port, downstreamPath, HttpStatusCode.OK, "Hello from Guillaume"))
.And(x => GivenThereIsAConfiguration(configuration))
.And(x => GivenOcelotIsRunning())
.When(x => WhenIGetUrlOnTheApiGateway(requestUrl))
.Then(x => ThenTheDownstreamUrlPathShouldBe(downstreamPath))
.And(x => ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
.And(x => ThenTheResponseBodyShouldBe("Hello from Guillaume"))
.BDDfy();
}

[Fact]
[Trait("Feat", "91, 94")]
public void Should_return_response_201_with_simple_url_and_multiple_upstream_http_method()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,41 @@ public void Find_CaseInsensitive_CannotMatchPlaceholders(string template, string
// Assert;
ThenTheExpectedVariablesCantBeFound(expectedTemplates.ToArray());
}

[Theory]
[Trait("Bug", "2212")]
[InlineData("/dati-registri/{version}/{everything}", "/dati-registri/v1.0/operatore/R80QQ5J9600/valida", "{version}", "v1.0", "{everything}", "operatore/R80QQ5J9600/valida")]
[InlineData("/api/invoices/{invoiceId}/{url}", "/api/invoices/1", "{invoiceId}", "1", "{url}", "")]
[InlineData("/api/{version}/{type}/{everything}", "/api/v1.0/items/details/12345", "{version}", "v1.0", "{type}", "items", "{everything}", "details/12345")]
[InlineData("/resources/{area}/{id}/{details}", "/resources/europe/56789/info/about", "{area}", "europe", "{id}", "56789", "{details}", "info/about")]
[InlineData("/data/{version}/{category}/{subcategory}/{rest}", "/data/2.1/sales/reports/weekly/summary", "{version}", "2.1", "{category}", "sales", "{subcategory}", "reports", "{rest}", "weekly/summary")]
[InlineData("/users/{region}/{team}/{userId}/{details}", "/users/north/eu/12345/activities/list", "{region}", "north", "{team}", "eu", "{userId}", "12345", "{details}", "activities/list")]
public void Match_CatchAll_OnlyTheLastPlaceholderCanContainSlashes(string template, string path,
string placeholderName1, string placeholderValue1, string placeholderName2, string placeholderValue2,
string placeholderName3 = null, string placeholderValue3 = null, string placeholderName4 = null, string placeholderValue4 = null)
{
var expectedTemplates = new List<PlaceholderNameAndValue>
{
new(placeholderName1, placeholderValue1),
new(placeholderName2, placeholderValue2),
};

if (!string.IsNullOrEmpty(placeholderName3))
{
expectedTemplates.Add(new(placeholderName3, placeholderValue3));
}

if (!string.IsNullOrEmpty(placeholderName4))
{
expectedTemplates.Add(new(placeholderName4, placeholderValue4));
}

// Act
_result = _finder.Find(path, Empty, template);

// Assert
ThenTheTemplatesVariablesAre(expectedTemplates.ToArray());
}

private void ThenSinglePlaceholderIs(string expectedName, string expectedValue)
{
Expand Down