Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#658 Change upstream Regex to account for blank placeholders #1333

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

#658 Change upstream Regex to account for blank placeholders #1333

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
94f9abe
Change upstream regex to account for blank placeholders
AndyConlisk Sep 4, 2020
285e07a
code formatting
raman-m Aug 5, 2023
58c5d5f
IDE0057 Substring can be simplified
raman-m Aug 5, 2023
295f7b9
code formatting
raman-m Aug 5, 2023
687c869
Use expression body for method
raman-m Aug 5, 2023
4c23837
CA1822 Member 'TemplateDoesNotEndInForwardSlash' does not access inst…
raman-m Aug 5, 2023
2c19c3d
CA1822 Member 'IsLastPlaceholder' does not access instance data and c…
raman-m Aug 5, 2023
8877c90
IDE1006 Naming rule violation: These words must begin with upper case…
raman-m Aug 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ namespace Ocelot.Configuration.Creator
public class UpstreamTemplatePatternCreator : IUpstreamTemplatePatternCreator
{
private const string RegExMatchOneOrMoreOfEverything = ".+";
private const string RegExMatchLastPlaceHolderZeroOrMoreOfEverything = "(|/.+|/[\\?&#].+)";
Copy link
Member

Choose a reason for hiding this comment

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

  1. What about URL encoded queries including everything case?
    Should we add % to the set of chars [\\?&#]?

  2. The 2nd case of having something after last slash char (everything-case) is defined by expression which matches by .+ quantifier. Can we replace all reg-expression like that: (/[\\?&#].*) ?

private const string RegExMatchOneOrMoreOfEverythingUntilNextForwardSlash = "[^/]+";
private const string RegExMatchEndString = "$";
private const string RegExIgnoreCase = "(?i)";
Expand Down Expand Up @@ -49,7 +50,15 @@ public UpstreamPathTemplate Create(IRoute route)
var indexOfNextForwardSlash = upstreamTemplate.IndexOf("/", indexOfPlaceholder, StringComparison.Ordinal);
if (indexOfNextForwardSlash < indexOfPlaceholder || (containsQueryString && upstreamTemplate.IndexOf('?', StringComparison.Ordinal) < upstreamTemplate.IndexOf(placeholders[i], StringComparison.Ordinal)))
{
upstreamTemplate = upstreamTemplate.Replace(placeholders[i], RegExMatchOneOrMoreOfEverything);
if (upstreamTemplate[indexOfPlaceholder - 1] == '/')
Copy link
Member

Choose a reason for hiding this comment

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

Could you look at line 77 please?
Is not the same case when the string ends with slash?

{
upstreamTemplate = upstreamTemplate.Remove(indexOfPlaceholder - 1, 1);
upstreamTemplate = upstreamTemplate.Replace(placeholders[i], RegExMatchLastPlaceHolderZeroOrMoreOfEverything);
}
else
{
upstreamTemplate = upstreamTemplate.Replace(placeholders[i], RegExMatchOneOrMoreOfEverything);
}
}
else
{
Expand All @@ -76,7 +85,7 @@ public UpstreamPathTemplate Create(IRoute route)

private static bool ForwardSlashAndOnePlaceHolder(string upstreamTemplate, List<string> placeholders, int postitionOfPlaceHolderClosingBracket)
{
if (upstreamTemplate.Substring(0, 2) == "/{" && placeholders.Count == 1 && upstreamTemplate.Length == postitionOfPlaceHolderClosingBracket + 1)
if (upstreamTemplate[..2] == "/{" && placeholders.Count == 1 && upstreamTemplate.Length == postitionOfPlaceHolderClosingBracket + 1)
{
return true;
}
Expand Down
32 changes: 32 additions & 0 deletions src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
using Ocelot.Responses;
using System;
using System.Collections.Generic;

namespace Ocelot.DownstreamRouteFinder.UrlMatcher
{
Expand Down Expand Up @@ -68,6 +70,29 @@ public Response<List<PlaceholderNameAndValue>> Find(string path, string query, s

counterForTemplate = endOfPlaceholder;
}
else if (TemplateDoesNotEndInForwardSlash(pathTemplate) && IsLastPlaceholder(path, counterForPath, pathTemplate))
{
Comment on lines +73 to +74
Copy link
Member

Choose a reason for hiding this comment

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

The code for this if-block was copied from lines 26-46.
Based on the function of code snippet, why not to define private helper-method to be reused in both places?

//should_find_multiple_query_string make test pass
if (PassedQueryString(pathTemplate, counterForTemplate))
{
delimiter = '&';
nextDelimiter = '&';
Comment on lines +78 to +79
Copy link
Member

Choose a reason for hiding this comment

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

Can be one line assignment

Suggested change
delimiter = '&';
nextDelimiter = '&';
delimiter = nextDelimiter = '&';

}

//should_find_multiple_query_string_and_path makes test pass
if (NotPassedQueryString(pathTemplate, counterForTemplate) && NoMoreForwardSlash(pathTemplate, counterForTemplate))
{
delimiter = '?';
nextDelimiter = '?';
Comment on lines +85 to +86
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
delimiter = '?';
nextDelimiter = '?';
delimiter = nextDelimiter = '?';

}

var placeholderName = GetPlaceholderName(pathTemplate, counterForTemplate);
var placeholderValue = GetPlaceholderValue(pathTemplate, query, placeholderName, path, counterForPath, delimiter);
placeHolderNameAndValues.Add(new PlaceholderNameAndValue(placeholderName, placeholderValue));

var endOfPlaceholder = GetNextCounterPosition(pathTemplate, counterForTemplate, '}');
counterForTemplate = endOfPlaceholder;
}

counterForPath++;
}
Expand Down Expand Up @@ -142,5 +167,12 @@ private static bool ContinueScanningUrl(int counterForUrl, int urlLength)
}

private static bool IsPlaceholder(char character) => character == '{';

private static bool IsLastPlaceholder(string path, int counterForPath, string pathTemplate)
=> path.Length <= counterForPath && pathTemplate.Length > 1
&& pathTemplate.Substring(counterForPath, 2) == "/{"
&& pathTemplate.IndexOf('}') == pathTemplate.Length - 1;

private static bool TemplateDoesNotEndInForwardSlash(string pathTemplate) => !pathTemplate.EndsWith('/');
}
}
38 changes: 19 additions & 19 deletions test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ public UpstreamTemplatePatternCreatorTests()
}

[Fact]
public void should_match_up_to_next_slash()
public void Should_match_up_to_next_slash()
{
var fileRoute = new FileRoute
{
Expand All @@ -32,7 +32,7 @@ public void should_match_up_to_next_slash()
}

[Fact]
public void should_use_re_route_priority()
public void Should_use_re_route_priority()
{
var fileRoute = new FileRoute
{
Expand All @@ -42,13 +42,13 @@ 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(|/.+|/[\\?&#].+)$"))
.And(x => ThenThePriorityIs(0))
.BDDfy();
}

[Fact]
public void should_use_zero_priority()
public void Should_use_zero_priority()
{
var fileRoute = new FileRoute
{
Expand All @@ -64,7 +64,7 @@ public void should_use_zero_priority()
}

[Fact]
public void should_set_upstream_template_pattern_to_ignore_case_sensitivity()
public void Should_set_upstream_template_pattern_to_ignore_case_sensitivity()
{
var fileRoute = new FileRoute
{
Expand All @@ -74,13 +74,13 @@ 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(|/.+|/[\\?&#].+)$"))
.And(x => ThenThePriorityIs(1))
.BDDfy();
}

[Fact]
public void should_match_forward_slash_or_no_forward_slash_if_template_end_with_forward_slash()
public void Should_match_forward_slash_or_no_forward_slash_if_template_end_with_forward_slash()
{
var fileRoute = new FileRoute
{
Expand All @@ -96,7 +96,7 @@ public void should_match_forward_slash_or_no_forward_slash_if_template_end_with_
}

[Fact]
public void should_set_upstream_template_pattern_to_respect_case_sensitivity()
public void Should_set_upstream_template_pattern_to_respect_case_sensitivity()
{
var fileRoute = new FileRoute
{
Expand All @@ -105,13 +105,13 @@ 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(|/.+|/[\\?&#].+)$"))
.And(x => ThenThePriorityIs(1))
.BDDfy();
}

[Fact]
public void should_create_template_pattern_that_matches_anything_to_end_of_string()
public void Should_create_template_pattern_that_matches_anything_to_end_of_string()
{
var fileRoute = new FileRoute
{
Expand All @@ -121,13 +121,13 @@ 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(|/.+|/[\\?&#].+)$"))
.And(x => ThenThePriorityIs(1))
.BDDfy();
}

[Fact]
public void should_create_template_pattern_that_matches_more_than_one_placeholder()
public void Should_create_template_pattern_that_matches_more_than_one_placeholder()
{
var fileRoute = new FileRoute
{
Expand All @@ -137,13 +137,13 @@ 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(|/.+|/[\\?&#].+)$"))
.And(x => ThenThePriorityIs(1))
.BDDfy();
}

[Fact]
public void should_create_template_pattern_that_matches_more_than_one_placeholder_with_trailing_slash()
public void Should_create_template_pattern_that_matches_more_than_one_placeholder_with_trailing_slash()
{
var fileRoute = new FileRoute
{
Expand All @@ -159,7 +159,7 @@ public void should_create_template_pattern_that_matches_more_than_one_placeholde
}

[Fact]
public void should_create_template_pattern_that_matches_to_end_of_string()
public void Should_create_template_pattern_that_matches_to_end_of_string()
{
var fileRoute = new FileRoute
{
Expand All @@ -174,7 +174,7 @@ public void should_create_template_pattern_that_matches_to_end_of_string()
}

[Fact]
public void should_create_template_pattern_that_matches_to_end_of_string_when_slash_and_placeholder()
public void Should_create_template_pattern_that_matches_to_end_of_string_when_slash_and_placeholder()
{
var fileRoute = new FileRoute
{
Expand All @@ -189,7 +189,7 @@ public void should_create_template_pattern_that_matches_to_end_of_string_when_sl
}

[Fact]
public void should_create_template_pattern_that_starts_with_placeholder_then_has_another_later()
public void Should_create_template_pattern_that_starts_with_placeholder_then_has_another_later()
{
var fileRoute = new FileRoute
{
Expand All @@ -205,7 +205,7 @@ public void should_create_template_pattern_that_starts_with_placeholder_then_has
}

[Fact]
public void should_create_template_pattern_that_matches_query_string()
public void Should_create_template_pattern_that_matches_query_string()
{
var fileRoute = new FileRoute
{
Expand All @@ -220,7 +220,7 @@ public void should_create_template_pattern_that_matches_query_string()
}

[Fact]
public void should_create_template_pattern_that_matches_query_string_with_multiple_params()
public void Should_create_template_pattern_that_matches_query_string_with_multiple_params()
{
var fileRoute = new FileRoute
{
Expand Down