Skip to content

Commit

Permalink
ThreeMammals#2212 Incorrect calculation of placeholders, fixing the i…
Browse files Browse the repository at this point in the history
…ssue and adding unit and acceptance tests verifying the behavior.
  • Loading branch information
ggnaegi committed Nov 22, 2024
1 parent e9a8bfc commit 61c54fd
Show file tree
Hide file tree
Showing 3 changed files with 89 additions and 1 deletion.
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);
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}>[^/|&]*)";
})}";

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")]
[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

0 comments on commit 61c54fd

Please sign in to comment.