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

PureKrome commented 6 years ago

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

TomPallister commented 6 years ago

@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 commented 4 years ago

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 commented 11 months ago

@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 commented 11 months ago

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

PureKrome commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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

Sure

ks1990cn commented 11 months ago

@raman-m Working for me on latest.

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

raman-m commented 11 months ago

@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 commented 11 months ago

Sure, will give a try here

raman-m commented 11 months ago

@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 commented 11 months ago

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

Everything is tested okay already in routingtests.cs

raman-m commented 11 months ago

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

ks1990cn commented 11 months ago

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

https://github.com/ThreeMammals/Ocelot/blob/d7754af609845cba0222870111842c1256c19122/test/Ocelot.AcceptanceTests/RoutingTests.cs#L324-L374

raman-m commented 11 months ago

@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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

@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 commented 11 months ago

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 commented 11 months ago

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

raman-m commented 11 months ago

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 commented 11 months ago

Yes will open PR for it, thank you

ThreeMammals / Ocelot