Skip to content

Commit

Permalink
Fix PR Comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@AlyHKafoury
AlyHKafoury committed Jan 10, 2024
1 parent e35966a commit 1d185aa
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 31 deletions.
10 changes: 8 additions & 2 deletions src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ namespace Ocelot.Configuration.Creator
{
public class UpstreamTemplatePatternCreator : IUpstreamTemplatePatternCreator
{
private const string RegExMatchZeroOrMoreOfEverything = ".*";
public const string RegExMatchZeroOrMoreOfEverything = ".*";
private const string RegExMatchOneOrMoreOfEverythingUntilNextForwardSlash = "[^/]+";
private const string RegExMatchEndString = "$";
private const string RegExIgnoreCase = "(?i)";
Expand Down Expand Up @@ -55,7 +55,13 @@ public UpstreamPathTemplate Create(IRoute route)
{
upstreamTemplate = upstreamTemplate.Replace(placeholders[i], RegExMatchOneOrMoreOfEverythingUntilNextForwardSlash);
}
}
}

var indexOfLastForwardSlash = upstreamTemplate.LastIndexOf('/');
if (indexOfLastForwardSlash < (upstreamTemplate.Length - 1) && upstreamTemplate.ElementAt(indexOfLastForwardSlash + 1) == '.')
{
upstreamTemplate = upstreamTemplate.Substring(0, indexOfLastForwardSlash) + "(?:|/" + upstreamTemplate.Substring(indexOfLastForwardSlash + 1) + ")";
}

if (upstreamTemplate == "/")
{
Expand Down
20 changes: 12 additions & 8 deletions src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,16 +50,11 @@ public Response<List<PlaceholderNameAndValue>> Find(string path, string query, s

return new OkResponse<List<PlaceholderNameAndValue>>(placeHolderNameAndValues);
}
else if (IsCatchAll(path, counterForPath, pathTemplate) || (pathTemplate[counterForTemplate] == '{') && NoMoreForwardSlash(pathTemplate, counterForTemplate) && NotPassedQueryString(pathTemplate, pathTemplate.Length))
else if (IsCatchAll(path, counterForPath, pathTemplate) || IsCatchAllAfterOtherPlaceholders(pathTemplate, counterForTemplate))
{
var endOfPlaceholder = GetNextCounterPosition(pathTemplate, counterForTemplate, '}');

if (pathTemplate[counterForTemplate] == '/')
{
counterForTemplate++;
}

var placeholderName = GetPlaceholderName(pathTemplate, counterForTemplate);
var placeholderName = GetPlaceholderName(pathTemplate, counterForTemplate + 1);

if (NothingAfterFirstForwardSlash(path))
{
Expand Down Expand Up @@ -102,14 +97,23 @@ private static bool IsCatchAll(string path, int counterForPath, string pathTempl
&& pathTemplate.IndexOf('}') == pathTemplate.Length - 1;
}

private static bool IsCatchAllAfterOtherPlaceholders(string pathTemplate, int counterForTemplate)
{
return (pathTemplate[counterForTemplate] == '/')
&& (counterForTemplate < pathTemplate.Length - 1)
&& (pathTemplate[counterForTemplate + 1] == '{')
&& NoMoreForwardSlash(pathTemplate, counterForTemplate + 1)
&& NotPassedQueryString(pathTemplate, pathTemplate.Length);
}

private static bool NothingAfterFirstForwardSlash(string path)
{
return path.Length == 1 || path.Length == 0;
}

private static string GetPlaceholderValue(string urlPathTemplate, string query, string variableName, string urlPath, int counterForUrl, char delimiter)
{
if(counterForUrl > urlPath.Length)
if (counterForUrl > urlPath.Length)
{
return "";
}
Expand Down
10 changes: 8 additions & 2 deletions src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ public DownstreamUrlCreatorMiddleware(

public async Task Invoke(HttpContext httpContext)
{
var downstreamRoute = httpContext.Items.DownstreamRoute();
var downstreamRoute = httpContext.Items.DownstreamRoute();
var upstreamPath = httpContext.Request.Path.ToString();
var placeholders = httpContext.Items.TemplatePlaceholderNameAndValues();
var response = _replacer.Replace(downstreamRoute.DownstreamPathTemplate.Value, placeholders);
var downstreamRequest = httpContext.Items.DownstreamRequest();
Expand All @@ -43,7 +44,12 @@ public async Task Invoke(HttpContext httpContext)

httpContext.Items.UpsertErrors(response.Errors);
return;
}
}

if (response.Data.Value.EndsWith("/") && !upstreamPath.EndsWith("/"))
{
response = new OkResponse<DownstreamPath>(new DownstreamPath(response.Data.Value.TrimEnd('/')));
}

if (!string.IsNullOrEmpty(downstreamRoute.DownstreamScheme))
{
Expand Down
31 changes: 18 additions & 13 deletions test/Ocelot.AcceptanceTests/RoutingTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -558,8 +558,9 @@ public void should_return_not_found_when_upstream_url_ends_with_forward_slash_bu
.BDDfy();
}

[Fact]
public void should_return_200_found()
[Theory]
[InlineDataAttribute("/products", "/products/{productId}", "/products/")]
public void should_return_200_found(string downstreamPathTemplate, string upstreamPathTemplate, string requestURL)
{
var port = PortFinder.GetRandomPort();

Expand All @@ -569,7 +570,7 @@ public void should_return_200_found()
{
new()
{
DownstreamPathTemplate = "/products",
DownstreamPathTemplate = downstreamPathTemplate,
DownstreamScheme = "http",
DownstreamHostAndPorts = new List<FileHostAndPort>
{
Expand All @@ -579,18 +580,18 @@ public void should_return_200_found()
Port = port,
},
},
UpstreamPathTemplate = "/products/{productId}",
UpstreamPathTemplate = upstreamPathTemplate,
UpstreamHttpMethod = new List<string> { "Get" },
},
},
};

this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}", "/products", 200, "Hello from Laura"))
this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}", downstreamPathTemplate, 200, "Hello from Laura"))
.And(x => _steps.GivenThereIsAConfiguration(configuration))
.And(x => _steps.GivenOcelotIsRunning())
.When(x => _steps.WhenIGetUrlOnTheApiGateway("/products/"))
.When(x => _steps.WhenIGetUrlOnTheApiGateway(requestURL))
.Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK))
.Then(x => ThenTheDownstreamUrlPathShouldBe("/products"))
.Then(x => ThenTheDownstreamUrlPathShouldBe(downstreamPathTemplate))
.BDDfy();
}

Expand Down Expand Up @@ -809,8 +810,12 @@ public void should_return_response_200_with_placeholder_for_final_url_path()
.BDDfy();
}

[Fact]
public void should_return_correct_downstream_when_omitting_ending_placeholder()
[Theory]
[InlineData("/downstream/test/{testId}", "/upstream/test/{testId}", "/upstream/test/1", "/downstream/test/1")]
[InlineData("/downstream/test/{testId}", "/upstream/test/{testId}", "/upstream/test/", "/downstream/test/")]
[InlineData("/downstream/test/{testId}", "/upstream/test/{testId}", "/upstream/test", "/downstream/test")]
[InlineData("/downstream/test/{testId}", "/upstream/test/{testId}", "/upstream/test123", null)]
public void should_return_correct_downstream_when_omitting_ending_placeholder(string downstreamPathTemplate, string upstreamPathTemplate, string requestURL, string downstreamURL)
{
var port = PortFinder.GetRandomPort();

Expand All @@ -820,7 +825,7 @@ public void should_return_correct_downstream_when_omitting_ending_placeholder()
{
new()
{
DownstreamPathTemplate = "/downstream/test/{testId}",
DownstreamPathTemplate = downstreamPathTemplate,
DownstreamScheme = "http",
DownstreamHostAndPorts = new List<FileHostAndPort>
{
Expand All @@ -830,7 +835,7 @@ public void should_return_correct_downstream_when_omitting_ending_placeholder()
Port = port,
},
},
UpstreamPathTemplate = "/upstream/test/{testId}",
UpstreamPathTemplate = upstreamPathTemplate,
UpstreamHttpMethod = new List<string> { "Get" },
},
},
Expand All @@ -839,8 +844,8 @@ public void should_return_correct_downstream_when_omitting_ending_placeholder()
this.Given(x => GivenThereIsAServiceRunningOn($"http://localhost:{port}", "/", 200, "Test Body"))
.And(x => _steps.GivenThereIsAConfiguration(configuration))
.And(x => _steps.GivenOcelotIsRunning())
.When(x => _steps.WhenIGetUrlOnTheApiGateway("/upstream/test/"))
.Then(x => ThenTheDownstreamUrlPathShouldBe("/downstream/test/"))
.When(x => _steps.WhenIGetUrlOnTheApiGateway(requestURL))
.Then(x => ThenTheDownstreamUrlPathShouldBe(downstreamURL))
.BDDfy();
}

Expand Down
12 changes: 6 additions & 6 deletions test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public void should_use_re_route_priority()

this.Given(x => x.GivenTheFollowingFileRoute(fileRoute))
.When(x => x.WhenICreateTheTemplatePattern())
.Then(x => x.ThenTheFollowingIsReturned("^(?i)/orders/.*$"))
.Then(x => x.ThenTheFollowingIsReturned($"^(?i)/orders(?:|/{UpstreamTemplatePatternCreator.RegExMatchZeroOrMoreOfEverything})$"))
.And(x => ThenThePriorityIs(0))
.BDDfy();
}
Expand Down Expand Up @@ -74,7 +74,7 @@ public void should_set_upstream_template_pattern_to_ignore_case_sensitivity()

this.Given(x => x.GivenTheFollowingFileRoute(fileRoute))
.When(x => x.WhenICreateTheTemplatePattern())
.Then(x => x.ThenTheFollowingIsReturned("^(?i)/PRODUCTS/.*$"))
.Then(x => x.ThenTheFollowingIsReturned($"^(?i)/PRODUCTS(?:|/{UpstreamTemplatePatternCreator.RegExMatchZeroOrMoreOfEverything})$"))
.And(x => ThenThePriorityIs(1))
.BDDfy();
}
Expand Down Expand Up @@ -105,7 +105,7 @@ public void should_set_upstream_template_pattern_to_respect_case_sensitivity()
};
this.Given(x => x.GivenTheFollowingFileRoute(fileRoute))
.When(x => x.WhenICreateTheTemplatePattern())
.Then(x => x.ThenTheFollowingIsReturned("^/PRODUCTS/.*$"))
.Then(x => x.ThenTheFollowingIsReturned($"^/PRODUCTS(?:|/{UpstreamTemplatePatternCreator.RegExMatchZeroOrMoreOfEverything})$"))
.And(x => ThenThePriorityIs(1))
.BDDfy();
}
Expand All @@ -121,7 +121,7 @@ public void should_create_template_pattern_that_matches_anything_to_end_of_strin

this.Given(x => x.GivenTheFollowingFileRoute(fileRoute))
.When(x => x.WhenICreateTheTemplatePattern())
.Then(x => x.ThenTheFollowingIsReturned("^/api/products/.*$"))
.Then(x => x.ThenTheFollowingIsReturned($"^/api/products(?:|/{UpstreamTemplatePatternCreator.RegExMatchZeroOrMoreOfEverything})$"))
.And(x => ThenThePriorityIs(1))
.BDDfy();
}
Expand All @@ -137,7 +137,7 @@ public void should_create_template_pattern_that_matches_more_than_one_placeholde

this.Given(x => x.GivenTheFollowingFileRoute(fileRoute))
.When(x => x.WhenICreateTheTemplatePattern())
.Then(x => x.ThenTheFollowingIsReturned("^/api/products/[^/]+/variants/.*$"))
.Then(x => x.ThenTheFollowingIsReturned($"^/api/products/[^/]+/variants(?:|/{UpstreamTemplatePatternCreator.RegExMatchZeroOrMoreOfEverything})$"))
.And(x => ThenThePriorityIs(1))
.BDDfy();
}
Expand Down Expand Up @@ -214,7 +214,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=.*$"))
.Then(x => x.ThenTheFollowingIsReturned($"^(?i)/api/subscriptions/[^/]+/updates\\?unitId={UpstreamTemplatePatternCreator.RegExMatchZeroOrMoreOfEverything}$"))
.And(x => ThenThePriorityIs(1))
.BDDfy();
}
Expand Down

0 comments on commit 1d185aa

Please sign in to comment.