Upstream route templates that end in a slash are not 'found'. #649

PureKrome · 2018-10-02T08:23:25Z

Expected Behavior / New Feature

When I map a specific route and it ends with a / I expect it to match to the same route if there was no ending /.

Actual Behavior / Motivation for New Feature

Given two upstream routes where one differ's only with the last character being an / then the Router fails to map to the route and returns a 404.

Steps to Reproduce the Problem

First, lets define a sample route configuration:

{
    "ReRoutes": [
        {
            "UpstreamPathTemplate": "/account/authenticate/",  <-- Notice the ENDING slash
            "DownstreamPathTemplate": "/authenticate",
            "DownstreamScheme": "http",
            "DownstreamHostAndPorts": [
                {
                    "Host": "accounts.api",
                    "Port": 80
                }
            ]
        }
    ],
    "GlobalConfiguration": {
        "BaseUrl": "http://localhost:5002"
    }
}

When using Postman, try to hit the following routes:

HTTP POST http://localhost:5002/account/authenticate <--- Succeeds
HTTP POST http://localhost:5002/account/authenticate/ <-- FAILS but expected to succeed.

There's no documentation about this, if this is a 'by design' feature.

Specifications

Version: 11.0.3

C:\Users\PK>dotnet --info
.NET Core SDK (reflecting any global.json):
 Version:   2.1.402
 Commit:    3599f217f4

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.17134
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\2.1.402\

Host (useful for support):
  Version: 2.1.4
  Commit:  85255dde3e

.NET Core SDKs installed:
  2.1.202 [C:\Program Files\dotnet\sdk]
  2.1.301 [C:\Program Files\dotnet\sdk]
  2.1.400 [C:\Program Files\dotnet\sdk]
  2.1.401 [C:\Program Files\dotnet\sdk]
  2.1.402 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.1.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.0 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.1 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.2 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.4 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.0.9 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.1 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.2 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.3-servicing-26724-03 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.4 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]

To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download

The text was updated successfully, but these errors were encountered:

TomPallister · 2018-10-02T17:39:31Z

@PureKrome thanks for your interest in Ocelot, we can take a look at making this work but for now I would suggest just adding the same ReRoute with a / at the end!

joaopgrassi · 2020-05-05T15:07:54Z

This is not great.. and I don't think the suggestion above is good enough.

For example with this re-route:

{
  "Description": "My service",
  "UpstreamHttpMethod": [ "GET", "POST", "PUT" ],
  "UpstreamPathTemplate": "/services/tags/{everything}",
  "DownstreamPathTemplate": "api/{version}/tags/{everything}",
  "DownstreamScheme": "http",
  "DownstreamHostAndPorts": [
    {
        "Host": "localhost",
        "Port": 5014
    }
  ]
}

This works:
GET: https://localhost:44321/services/v1/tags/id

This doesn't (because of the /{everything})
POST: https://localhost:44321/services/v1/tags

raman-m · 2023-11-29T22:01:56Z

@PureKrome commented on Oct 2, 2018
@joaopgrassi commented on May 5, 2020

Hello guys!
Could you upgrade to latest version v22+ and confirm that this bug is still active, please?

I would say, I remember this problem is described in some other PR's and issues. So, I will find them all to consolidate, but now please confirm the bug existence...

raman-m · 2023-11-29T22:03:41Z

@ks1990cn Could you help to test these user scenarios please? But for latest v22 only!

PureKrome · 2023-11-30T03:04:07Z

Hi @raman-m - appreciate the feedback. I won't have a chance to test this because I'm not using this anymore and have moved on (at least, for now). Sincerely sorry. Good luck!

joaopgrassi · 2023-11-30T08:37:50Z

Hi @raman-m - appreciate the feedback. I won't have a chance to test this because I'm not using this anymore and have moved on (at least, for now). Sincerely sorry. Good luck!

Same for me unfortunately!

ks1990cn · 2023-11-30T08:57:19Z

@ks1990cn Could you help to test these user scenarios please? But for latest v22 only!

Sure

ks1990cn · 2023-11-30T15:47:11Z

@raman-m Working for me on latest.

  {
      "DownstreamHostAndPorts": [
        {
          "Host": "localhost",
          "Port": 5022
        }
      ],
      "UpstreamPathTemplate": "/lol/",
      "UpstreamHttpMethod": [ "GET" ],
      "DownstreamPathTemplate": "/WeatherForecast/name",
      "DownstreamScheme": "http"
    }

raman-m · 2023-12-04T17:20:41Z

@ks1990cn commented on Nov 30

I am too busy to search through Ocelot unit tests and acceptance tests...
I believe some similar cases were covered but it is hard to find them in tests... It seems the bug is not reproducible now...

Do you want to write a couple of acceptance tests to verify user scenarios above plz?
And open small PR to link this issue.
If acceptance test(s) will show that the bug is not reproducible then merging your PR will close this issue automatically.
I will merge to develop with high priority without any planning of release...

Will you have some time for tests coding challenge? 😉

ks1990cn · 2023-12-04T17:23:14Z

Sure, will give a try here

raman-m · 2023-12-04T17:35:35Z

@joaopgrassi commented on May 5, 2020

This works:
GET: https://localhost:44321/services/v1/tags/id

This doesn't (because of the /{everything})
POST: https://localhost:44321/services/v1/tags

I see your user scenario differs from author's one... We will focus on the initial problem without {everything} placeholder. I saw a lot of issues which describe {everything} placeholder problems... But reporters should understand that in 99% of cases they can define more abstract {everything} route to include these cases. Just moving all path prefixes into {everything}! 😄
For example:

host.com/path/prefix/ your concern, it doesn't work, right?
host.com/path/prefix/{everything} It should work

Let's merge this cases to one definition

host.com/path/{everything} so, prefix/ is included and host.com/path/prefix/ and host.com/path/prefix should work perfectly.
Sounds good? 😉

ks1990cn · 2023-12-04T21:05:50Z

@raman-m In manual testing api, I saw same here!!

Everything is tested okay already in routingtests.cs

raman-m · 2023-12-05T15:34:57Z

@ks1990cn
Could you share some links to the code in repo plz? Not screenshots, plz!

ks1990cn · 2023-12-05T15:55:43Z

Here is link for file- containing all tese cases. @raman-m

Ocelot/test/Ocelot.AcceptanceTests/RoutingTests.cs

Lines 324 to 374 in d7754af

    
           public void Bug() 
        
           { 
        
               var port = PortFinder.GetRandomPort(); 
        
               var configuration = new FileConfiguration 
        
               { 
        
                   Routes = new List<FileRoute> 
        
                   { 
        
                       new() 
        
                       { 
        
                           DownstreamPathTemplate = "/api/v1/vacancy", 
        
                           DownstreamScheme = "http", 
        
                           DownstreamHostAndPorts = new List<FileHostAndPort> 
        
                           { 
        
                               new() 
        
                               { 
        
                                   Host = "localhost", 
        
                                   Port = port, 
        
                               }, 
        
                           }, 
        
                           UpstreamPathTemplate = "/vacancy/", 
        
                           UpstreamHttpMethod = new List<string> { "Options",  "Put", "Get", "Post", "Delete" }, 
        
                           LoadBalancerOptions = new FileLoadBalancerOptions { Type = "LeastConnection" }, 
        
                       }, 
        
                       new() 
        
                       { 
        
                           DownstreamPathTemplate = "/api/v1/vacancy/{vacancyId}", 
        
                           DownstreamScheme = "http", 
        
                           DownstreamHostAndPorts = new List<FileHostAndPort> 
        
                           { 
        
                               new() 
        
                               { 
        
                                   Host = "localhost", 
        
                                   Port = port, 
        
                               }, 
        
                           }, 
        
                           UpstreamPathTemplate = "/vacancy/{vacancyId}", 
        
                           UpstreamHttpMethod = new List<string> { "Options",  "Put", "Get", "Post", "Delete" }, 
        
                           LoadBalancerOptions = new FileLoadBalancerOptions { Type = "LeastConnection" }, 
        
                       }, 
        
                   }, 
        
               }; 
        
               this.Given(x => x.GivenThereIsAServiceRunningOn($"http://localhost:{port}", "/api/v1/vacancy/1", 200, "Hello from Laura")) 
        
                   .And(x => _steps.GivenThereIsAConfiguration(configuration)) 
        
                   .And(x => _steps.GivenOcelotIsRunning()) 
        
                   .When(x => _steps.WhenIGetUrlOnTheApiGateway("/vacancy/1")) 
        
                   .Then(x => _steps.ThenTheStatusCodeShouldBe(HttpStatusCode.OK)) 
        
                   .And(x => _steps.ThenTheResponseBodyShouldBe("Hello from Laura")) 
        
                   .BDDfy(); 
        
           }

raman-m · 2023-12-06T13:30:42Z

@ks1990cn
Thanks for the link!
Not exactly the same user case: Bug tests another upstream: .When(x => _steps.WhenIGetUrlOnTheApiGateway("/vacancy/1")) with placeholder, so the 2nd route has tested: UpstreamPathTemplate = "/vacancy/{vacancyId}",

Our current upstream is "UpstreamPathTemplate": "/account/authenticate/" which ends with slash.

raman-m · 2023-12-06T13:41:58Z

@ks1990cn
Yeah! Your screenshots showed a good candidates of the Ocelot.AcceptanceTests.RoutingTests class:

the 1st test is about OK status
But the 2nd test is about NotFound status

We are searching for the Config like this

UpstreamPathTemplate = "/products/",
DownstreamPathTemplate = "/products",

raman-m · 2023-12-06T13:51:14Z

HTTP POST http://localhost:5002/account/authenticate <--- Succeeds

This case is covered by should_return_ok_when_upstream_url_ends_with_forward_slash_but_template_does_not because it uses URL .When(x => _steps.WhenIGetUrlOnTheApiGateway("/products"))

ks1990cn · 2023-12-06T13:55:16Z

@raman-m Sorry my screen shot was on different line and I shared was different line. Didn't saw this.

But yes its tested ok

raman-m · 2023-12-06T14:01:49Z

HTTP POST http://localhost:5002/account/authenticate/ <-- FAILS but expected to succeed.

The second test should_return_not_found_when_upstream_url_ends_with_foward_slash_but_template_does_not is not the same because the route config differs: UpstreamPathTemplate = "/products",. So it is without a slash.

So, the first test is better...
We need to copy should_return_ok_when_upstream_url_ends_with_forward_slash_but_template_does_not test and change setup a little bit...

raman-m · 2023-12-06T14:32:50Z

@ks1990cn
Could you confirm plz that your manual tests in Postman have passed?

raman-m · 2023-12-06T18:41:15Z

Hello @ks1990cn
I've checked. The bug is not reproducible.
Here is the feature branch: bug-649 with acceptance tests...

Would you like to open PR ? 😉
Sync your fork and create a PR from the feature branch in your repo. And link this issue plz!
You will be able to contribute right to current release! 🤩

ks1990cn · 2023-12-07T11:03:27Z

Yes will open PR for it, thank you

* Changes related to test case of issue 649 * #649 test cases --------- Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

TomPallister added feature A new feature help wanted Not actively being worked on. If you plan to contribute, please drop a note. medium effort Likely a few days of development effort labels Oct 2, 2018

ig-sinicyn mentioned this issue Jun 27, 2020

[Proposal] Final placeholder must work as a catch-all mask #1275

Closed

raman-m added the bug Identified as a potential bug label Nov 29, 2023

raman-m added the needs validation Issue has not been replicated or verified yet label Dec 4, 2023

raman-m assigned ks1990cn Dec 4, 2023

ks1990cn mentioned this issue Dec 7, 2023

#649 Acceptance tests #1846

Merged

raman-m closed this as completed in #1846 Dec 8, 2023

raman-m added a commit that referenced this issue Dec 8, 2023

#649 Acceptance tests (#1846)

6b1f332

* Changes related to test case of issue 649 * #649 test cases --------- Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

raman-m added merged Issue has been merged to dev and is waiting for the next release Nov'23 November 2023 release and removed accepted Bug or feature would be accepted as a PR or is being worked on labels Dec 8, 2023

raman-m added this to the November'23 milestone Dec 8, 2023

raman-m mentioned this issue Feb 13, 2024

Release 23.0 | Artifacts | +semver: breaking #1962

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upstream route templates that end in a slash are not 'found'. #649

Upstream route templates that end in a slash are not 'found'. #649

PureKrome commented Oct 2, 2018 •

edited by raman-m

TomPallister commented Oct 2, 2018

joaopgrassi commented May 5, 2020 •

edited

raman-m commented Nov 29, 2023

raman-m commented Nov 29, 2023

PureKrome commented Nov 30, 2023

joaopgrassi commented Nov 30, 2023

ks1990cn commented Nov 30, 2023

ks1990cn commented Nov 30, 2023

raman-m commented Dec 4, 2023

ks1990cn commented Dec 4, 2023

raman-m commented Dec 4, 2023

ks1990cn commented Dec 4, 2023 •

edited

raman-m commented Dec 5, 2023

ks1990cn commented Dec 5, 2023

raman-m commented Dec 6, 2023 •

edited

raman-m commented Dec 6, 2023 •

edited

raman-m commented Dec 6, 2023

ks1990cn commented Dec 6, 2023

raman-m commented Dec 6, 2023 •

edited

raman-m commented Dec 6, 2023

raman-m commented Dec 6, 2023 •

edited

ks1990cn commented Dec 7, 2023 •

edited

Upstream route templates that end in a slash are not 'found'. #649

Upstream route templates that end in a slash are not 'found'. #649

Comments

PureKrome commented Oct 2, 2018 • edited by raman-m

Expected Behavior / New Feature

Actual Behavior / Motivation for New Feature

Steps to Reproduce the Problem

Specifications

TomPallister commented Oct 2, 2018

joaopgrassi commented May 5, 2020 • edited

raman-m commented Nov 29, 2023

raman-m commented Nov 29, 2023

PureKrome commented Nov 30, 2023

joaopgrassi commented Nov 30, 2023

ks1990cn commented Nov 30, 2023

ks1990cn commented Nov 30, 2023

raman-m commented Dec 4, 2023

ks1990cn commented Dec 4, 2023

raman-m commented Dec 4, 2023

ks1990cn commented Dec 4, 2023 • edited

raman-m commented Dec 5, 2023

ks1990cn commented Dec 5, 2023

raman-m commented Dec 6, 2023 • edited

raman-m commented Dec 6, 2023 • edited

raman-m commented Dec 6, 2023

ks1990cn commented Dec 6, 2023

raman-m commented Dec 6, 2023 • edited

raman-m commented Dec 6, 2023

raman-m commented Dec 6, 2023 • edited

ks1990cn commented Dec 7, 2023 • edited

PureKrome commented Oct 2, 2018 •

edited by raman-m

joaopgrassi commented May 5, 2020 •

edited

ks1990cn commented Dec 4, 2023 •

edited

raman-m commented Dec 6, 2023 •

edited

raman-m commented Dec 6, 2023 •

edited

raman-m commented Dec 6, 2023 •

edited

raman-m commented Dec 6, 2023 •

edited

ks1990cn commented Dec 7, 2023 •

edited