#658 Change upstream Regex to account for blank placeholders

[AndyConlisk]

Fixes #658

• #658

Proposed Changes

• To fix the issue mentioned in #658 this adds an additional Regex and to account for blank last placeholder.

[raman-m]

@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]

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

[AndyConlisk]

@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]

@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]

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. :smile:

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.

1. request to http://localhost/api works ✅
2. request to http://localhost/api/ does not work ❌
   • error message: Failed to match Route configuration for upstream path: /api/, verb: GET.
3. request to http://localhost/api/foo works ✅
4. 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]

@AndyConlisk commented on Aug 5

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

[AndyConlisk]

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

[raman-m]

@skyflyer commented on Aug 6:

2. 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]

@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]

Related to

• #145
• #158
• 9ba57f8ba654f66d5515fb96b7500ec67bf846c7

[raman-m]

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

[AndyConlisk]

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]

@AndyConlisk commented on Nov 7

Lazy bones! :wink:

[raman-m]

Duplicate of #1911

[raman-m]

Original issue #658 was fixed by #1911