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

AndyConlisk · 2020-09-04T00:33:58Z

Fixes #658

Routing question: placeholders vs slash #658

Proposed Changes

To fix the issue mentioned in Routing question: placeholders vs slash #658 this adds an additional Regex and to account for blank last placeholder.

raman-m · 2023-08-05T15:38:13Z

@AndyConlisk
Thanks for the PR!
The feature branch (master) has been rebased onto ThreeMammals:develop!
And we have the green build.
So, we can go with code review now...

I don't see develop branch in your fork! Your fork is too old.

Could you Sync fork please? So, new develop branch will occur with all top commits!

Could you add me as collaborator to your forked repo please? I will create develop branch and make it default.

raman-m · 2023-08-05T15:40:27Z

@skyflyer Hi Miha!
As an issue author,
Could you review the code of this solution please?

AndyConlisk · 2023-08-05T16:50:34Z

@raman-m commented on Aug 5

I added you as a collaborator to my fork. Please let me know if there is anything else you need me to do.

Thank you

skyflyer · 2023-08-05T17:26:00Z

@raman-m commented on Aug 5

Hello @raman-m!

Unfortunately, I'm not really acquainted with the code base and won't be able to either reproduce the error or test the fix right now. Sorry about that.

But, I will have a look at reproducing the scenario later, if that helps.

skyflyer · 2023-08-06T13:48:20Z

So, I'm not sure if my rationale is sound, but this does not seem to work, at least not for the case I reported in the issue.

First, do understand, that we're not using Ocelot anymore (the project that was the consumer of this library unfortunately died some time ago). Also understand that I'm not in a position to really review the code, as I'm not really acquainted with the codebase nor the roadmap and the design of the library. So take the following comments with that in mind. 😄

The general idea was that ocelot lib would route requests coming to /api to some services running without the /api prefix. In other words, requests coming to http://localhost:5000/api would be routed (or proxied would be a better term, I think) to http://localhost:8000/ and by that logic, requests coming to http://localhost:5000/api/foo would be routed to http://localhost:8000/api/foo.

In order to verify this behaviour, I cloned the develop branch from the main repo and "squash merged" the PR 1333 into that, so the changes are applied on top of the latest develop bits.

I then used a simple config (see below) and tried to request the resources. I've setup python's simple http server (python3 -m http.server 8000) in the parent directory where Ocelot (the repository) and OcelotDemo (the test program) were hosted.

request to http://localhost/api works ✅
request to http://localhost/api/ does not work ❌
- error message: Failed to match Route configuration for upstream path: /api/, verb: GET.
request to http://localhost/api/foo works ✅
request to http://localhost/api/OcelotDemo/ocelot.json works ✅

Simple config

{
    "Routes": [
        {
        "DownstreamPathTemplate": "/{id}",
        "DownstreamScheme": "http",
        "DownstreamHostAndPorts": [
            {
                "Host": "localhost",
                "Port": 8000
            }
        ],
        "UpstreamPathTemplate": "/api/{id}",
        "UpstreamHttpMethod": [ "Get", "Post" ]
        }
    ],
    "GlobalConfiguration": {
        "BaseUrl": "https://localhost:5000"
    }
}

And here is the Program.cs if you wish to reproduce the complete test:

Program.cs

using Ocelot.Middleware;
using Ocelot.DependencyInjection;

var builder = WebApplication.CreateBuilder(args);

builder.Configuration.AddJsonFile("ocelot.json");

builder.Services.AddOcelot(builder.Configuration);

var app = builder.Build();

app.MapGet("/", () => "Hello World!");

app.UseOcelot().Wait();

app.Run();

raman-m · 2023-08-14T14:11:58Z

@AndyConlisk commented on Aug 5

Thanks!
All that's left to do is making it default. 👇

AndyConlisk · 2023-08-14T14:36:15Z

@raman-m I have made the develop branch to be the default branch.

raman-m · 2023-08-14T14:40:32Z

@skyflyer commented on Aug 6:

request to http://localhost/api/ does not work ❌
* error message: Failed to match Route configuration for upstream path: /api/, verb: GET.

Thanks for user scenarios.
Yeah, we will test this scenario too.
And, on my opinion, all user's scenarios from your issue should be tested, but they are not, based on current source code.

raman-m · 2023-08-14T14:46:40Z

@RaynaldM approved these changes on Aug 7

Ray, why did you approve the PR?
There are no unit tests at all! 🤣
Even a beautiful code without tests should not be approved. 😃

raman-m

@AndyConlisk
Although the code looks solid, but it can be improved. 👇

${\color{orange}There\ are\ no\ new\ tests!}$

The new code blocks are not covered by unit tests.
There are no unit tests for user scenarios from the linked issue.
Also, there are no acceptance tests to make sure Ocelot pipeline will handle all user scenarios correctly.

raman-m · 2023-08-14T15:07:50Z

src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs

@@ -9,6 +9,7 @@ namespace Ocelot.Configuration.Creator
    public class UpstreamTemplatePatternCreator : IUpstreamTemplatePatternCreator
    {
        private const string RegExMatchOneOrMoreOfEverything = ".+";
+        private const string RegExMatchLastPlaceHolderZeroOrMoreOfEverything = "(|/.+|/[\\?&#].+)";


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

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: (/[\\?&#].*) ?

raman-m · 2023-08-14T15:56:41Z

src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs

@@ -52,7 +53,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] == '/')


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

raman-m · 2023-08-14T15:58:12Z

src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs

+                        delimiter = '&';
+                        nextDelimiter = '&';


Can be one line assignment

Suggested change

delimiter = '&';

nextDelimiter = '&';

delimiter = nextDelimiter = '&';

raman-m · 2023-08-14T16:01:25Z

src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs

+                        delimiter = '?';
+                        nextDelimiter = '?';


Suggested change

delimiter = '?';

nextDelimiter = '?';

delimiter = nextDelimiter = '?';

raman-m · 2023-08-14T16:07:46Z

src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs

+                else if (TemplateDoesNotEndInForwardSlash(pathTemplate) && IsLastPlaceholder(path, counterForPath, pathTemplate))
+                {


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?

raman-m · 2023-08-14T16:21:29Z

Related to

Use range operator

…ance data and can be marked as static

…an be marked as static

… characters: should_*

raman-m · 2023-10-26T17:29:49Z

@AndyConlisk Do you have time to fix code review issues?
Do you have an intention to write unit tests at all?

AndyConlisk · 2023-11-07T00:08:52Z

I don't know when I will have time to write the new unit tests. I can try in the near future, but I can't give a solid commitment at this time.

raman-m · 2023-11-08T09:12:56Z

@AndyConlisk commented on Nov 7

Lazy bones! 😉

raman-m assigned AndyConlisk Jul 17, 2023

raman-m changed the title ~~Change upstream regex to account for blank placeholders~~ #658 Change upstream Regex to account for blank placeholders Jul 17, 2023

raman-m changed the base branch from master to develop July 17, 2023 18:36

raman-m self-requested a review July 17, 2023 18:47

raman-m force-pushed the master branch from 1f461a2 to 06722c8 Compare August 5, 2023 14:41

raman-m added bug Identified as a potential bug needs feedback Issue is waiting on feedback before acceptance labels Aug 5, 2023

raman-m mentioned this pull request Aug 5, 2023

Routing question: placeholders vs slash #658

Open

raman-m requested review from binarymash, jps, geffzhang, RaynaldM, philproctor, anktsrkr, sharpn, abooraja, wast, yangzhongke and thiagoloureiro August 5, 2023 15:44

RaynaldM approved these changes Aug 7, 2023

View reviewed changes

raman-m requested changes Aug 14, 2023

View reviewed changes

AndyConlisk and others added 8 commits October 26, 2023 20:06

Change upstream regex to account for blank placeholders

94f9abe

code formatting

285e07a

IDE0057 Substring can be simplified

58c5d5f

Use range operator

code formatting

295f7b9

Use expression body for method

687c869

CA1822 Member 'TemplateDoesNotEndInForwardSlash' does not access inst…

4c23837

…ance data and can be marked as static

CA1822 Member 'IsLastPlaceholder' does not access instance data and c…

2c19c3d

…an be marked as static

IDE1006 Naming rule violation: These words must begin with upper case…

8877c90

… characters: should_*

raman-m force-pushed the master branch from 00413c7 to 8877c90 Compare October 26, 2023 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

AndyConlisk commented Sep 4, 2020 •

edited by raman-m

raman-m commented Aug 5, 2023

raman-m commented Aug 5, 2023

AndyConlisk commented Aug 5, 2023 •

edited by raman-m

skyflyer commented Aug 5, 2023 •

edited by raman-m

skyflyer commented Aug 6, 2023 •

edited

raman-m commented Aug 14, 2023

AndyConlisk commented Aug 14, 2023

raman-m commented Aug 14, 2023

raman-m commented Aug 14, 2023

raman-m left a comment •

edited

raman-m Aug 14, 2023

raman-m Aug 14, 2023

raman-m Aug 14, 2023

raman-m Aug 14, 2023

raman-m Aug 14, 2023

raman-m commented Aug 14, 2023

raman-m commented Oct 26, 2023

AndyConlisk commented Nov 7, 2023

raman-m commented Nov 8, 2023

	delimiter = '&';
	nextDelimiter = '&';
	delimiter = nextDelimiter = '&';

	delimiter = '?';
	nextDelimiter = '?';
	delimiter = nextDelimiter = '?';

		else if (TemplateDoesNotEndInForwardSlash(pathTemplate) && IsLastPlaceholder(path, counterForPath, pathTemplate))
		{

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

Are you sure you want to change the base?

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

Conversation

AndyConlisk commented Sep 4, 2020 • edited by raman-m

Fixes #658

Proposed Changes

raman-m commented Aug 5, 2023

raman-m commented Aug 5, 2023

AndyConlisk commented Aug 5, 2023 • edited by raman-m

skyflyer commented Aug 5, 2023 • edited by raman-m

skyflyer commented Aug 6, 2023 • edited

raman-m commented Aug 14, 2023

AndyConlisk commented Aug 14, 2023

raman-m commented Aug 14, 2023

raman-m commented Aug 14, 2023

raman-m left a comment • edited

Choose a reason for hiding this comment

${\color{orange}There\ are\ no\ new\ tests!}$

raman-m Aug 14, 2023

Choose a reason for hiding this comment

raman-m Aug 14, 2023

Choose a reason for hiding this comment

raman-m Aug 14, 2023

Choose a reason for hiding this comment

raman-m Aug 14, 2023

Choose a reason for hiding this comment

raman-m Aug 14, 2023

Choose a reason for hiding this comment

raman-m commented Aug 14, 2023

Related to

raman-m commented Oct 26, 2023

AndyConlisk commented Nov 7, 2023

raman-m commented Nov 8, 2023

AndyConlisk commented Sep 4, 2020 •

edited by raman-m

AndyConlisk commented Aug 5, 2023 •

edited by raman-m

skyflyer commented Aug 5, 2023 •

edited by raman-m

skyflyer commented Aug 6, 2023 •

edited

raman-m left a comment •

edited