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.

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

Empty placeholders: Failed to match Route configuration for upstream path #748

Closed
satano opened this issue Jan 14, 2019 · 11 comments · Fixed by #1911
Closed

Empty placeholders: Failed to match Route configuration for upstream path #748

satano opened this issue Jan 14, 2019 · 11 comments · Fixed by #1911
Assignees
Labels
bug Identified as a potential bug merged Issue has been merged to dev and is waiting for the next release Nov'23 November 2023 release

Comments

@satano
Copy link

satano commented Jan 14, 2019

Expected Behavior

Match upstream path with empty placeholder and map it correctly to downstream.

Actual Behavior / Motivation for New Feature

Upstream path with placeholder is not working as expected when placeholder is empty. It fails with error:

UnableToFindDownstreamRouteError Message: Failed to match ReRoute configuration for upstream path: /test

Steps to Reproduce the Problem

This special catch all configuration of ReRoute works as expected

"DownstreamPathTemplate": "/api/invoices/{url}"
"UpstreamPathTemplate": "/{url}"

It maps URLs this way:

/ → /api/invoices/
/123 → /api/invoices/123

Now this mapping (downstream is the same):

"DownstreamPathTemplate": "/api/invoices/{url}"
"UpstreamPathTemplate": "/test/{url}"

It works correctly when {url} is specified: /test/123 → /api/invoices/123

But it fails, when {url} is empty. I would expect upstream URL /test/ to reroute to downstream URL /api/invoices/. The error is:

dbug: Ocelot.DownstreamRouteFinder.Middleware.DownstreamRouteFinderMiddleware[0]
      requestId: 0HLJQAG2RQPKO:00000005, previousRequestId: no previous request id, message: Upstream url path is /test/
warn: Ocelot.DownstreamRouteFinder.Middleware.DownstreamRouteFinderMiddleware[0]
      requestId: 0HLJQAG2RQPKO:00000005, previousRequestId: no previous request id, message: DownstreamRouteFinderMiddleware setting pipeline errors. IDownstreamRouteFinder returned Error Code: UnableToFindDownstreamRouteError Message: Failed to match ReRoute configuration for upstream path: /test/, verb: GET.
warn: Ocelot.DownstreamRouteFinder.Middleware.DownstreamRouteFinderMiddleware[0]
      requestId: 0HLJQAG2RQPKO:00000005, previousRequestId: no previous request id, message: Failed to match ReRoute configuration for upstream path: /test/, verb: GET.
warn: Ocelot.Responder.Middleware.ResponderMiddleware[0]
      requestId: 0HLJQAG2RQPKO:00000005, previousRequestId: no previous request id, message: Error Code: UnableToFindDownstreamRouteError Message: Failed to match ReRoute configuration for upstream path: /test/, verb: GET. errors found in ResponderMiddleware. Setting error response for request path:/test/, request method: GET
dbug: Ocelot.Errors.Middleware.ExceptionHandlerMiddleware[0]
      requestId: 0HLJQAG2RQPKO:00000005, previousRequestId: no previous request id, message: ocelot pipeline finished

As a special case of this, I would also expect routing to ignore trailing slash, because from WEB/API wiew, usually the address with the slash at the end is the same as without it. So upstream URL /test would map to downstream URL /api/invoices. Maybe this is not exact from the computers point of view, but for me it makes sense from human point of view of configuration. It does not work this way even if the routing is configured without the slashes:

"DownstreamPathTemplate": "/api/invoices{url}"
"UpstreamPathTemplate": "/test{url}"

It correctly maps non epmty {url} which means, it works with the slash at the end, because the slash is the value of the {url}. But still not without it:

/test/123 → /api/invoices/123
/test/ → /api/invoices/
/test → Failed to match ReRoute configuration for upstream path: /test

To make work everything what I want, I need three reroutes (or at least two, if the first one will be without slashes):

{
    "DownstreamPathTemplate": "/api/invoices/{url}"
    "UpstreamPathTemplate": "/test/{url}"
},
{
    "DownstreamPathTemplate": "/api/invoices/"
    "UpstreamPathTemplate": "/test/"
},
{
    "DownstreamPathTemplate": "/api/invoices"
    "UpstreamPathTemplate": "/test"
}

Specifications

  • Version: Ocelot 13.0.0
  • Platform: .NET Core 2.2 on Windows
@margaale
Copy link
Contributor

I always thought that it was designed to work this way, let's see if we can get some clarification...

@margaale margaale added the question Initially seen a question could become a new feature or bug or closed ;) label Jan 14, 2019
@margaale margaale self-assigned this Jan 14, 2019
@satano
Copy link
Author

satano commented Jan 15, 2019

New day has come and the new ideas with it. :)

Well. I thought about it and it is not as simple as I wrote in the first post. The problem is, that empty placeholder is not allowed in upstream URLs - special exception is catch all upstream /{everything}. And it really makes sense. But for me, it makes sense only for placeholders in the middle of the path.

So for example, I would not allow empty placeholder in this configuration.

"DownstreamPathTemplate": "/api/{url}/list"
"UpstreamPathTemplate": "/test/v2/{url}/list"

Because the most probably I do not want it to catch upstream URL /test/v2//list. This URL is OK and it is the same as /test/v2/list - the number of consecutive slashes does not matter.

So I would formulate my proposal this way.

If the same placeholder is at the end of both upstream and downstream path after the slash, it should work with empty value. Furthermore, when the placeholder is empty, this upstream route should match URL without the trailing slash.

To clarify this - for me, it is the same as catch all route /{everything}, but with some path prefix. So some examples:

Simple catch all URL

"DownstreamPathTemplate": "/api/test/{url}"
"UpstreamPathTemplate": "/test/{url}"

Will reroute:

/test/123 → /api/test/123
/test/ → /api/test/
/test → /api/test

Multiple placeholders

"DownstreamPathTemplate": "/api/{version}/test/{url}"
"UpstreamPathTemplate": "/test/{version}/{url}"

Will reroute:

/test/v1/123 → /api/v1/test/123
/test/v1/ → /api/v1/test/
/test/v1 → /api/v1/test

Empty value for {version} is not allowed, so upstream URL /test/123 will not match.

Placeholder in the middle of the path

"DownstreamPathTemplate": "/api/{url}/test"
"UpstreamPathTemplate": "/test/{url}"

Or

"DownstreamPathTemplate": "/api/test/{url}"
"UpstreamPathTemplate": "/test/{url}/api"

Empty value for {url} is not allowed. The placeholder must be at the end of both paths.

Different placeholders at the ends

"DownstreamPathTemplate": "/api/{url2}/{url1}"
"UpstreamPathTemplate": "/test/{url1}/{url2}"

Empty value is not allowed for any of the placeholders. The placeholder at the end must be the same.

Conclusion

So what I am thinking about is like extending current behavior of catch all URL /{url}. In mentioned conditions, the catch all will work for any path, not just simple /.

@philproctor philproctor self-assigned this Jan 16, 2019
@philproctor
Copy link
Contributor

#737 changes the way the matching templates are created and used, I did not change this behavior as it appeared to be by design. A few days ago I tried having the catch all catch 0 or more if it's the last segment of the pattern, but this caused some acceptance tests to fail which were explicitly verifying that did not happen. As such, I believe this is an intended behavior, but I think as part of #737 we may have some workarounds, particularly, using a grouping match instead of a placeholder. I will discuss with the team.

@upadhyaychandan
Copy link

Hi Team,
I want to validate Placeholder value how we can do that
Example:-
Api/test1/{Id}
api/test1/{Id}/test
api/test1/{id}/test/{id2}

Request Url:-
api/test1/{EmptyValue/test
So in Placeholder it is coming {EmptyValue}/test , it should be only {emptyvalue}

@i02coroj
Copy link

i02coroj commented Apr 6, 2022

Hi.
I see this ticket is still open and I would be quite interested to know if there's a plan to get this implemented.
The behavior I see with version 18.0.0 of the package it that it still doesn't match the upstream when the placeholder at the end of the path is empty.
so

@raman-m raman-m added bug Identified as a potential bug help wanted Not actively being worked on. If you plan to contribute, please drop a note. good first issue Should be pretty easy to do and removed question Initially seen a question could become a new feature or bug or closed ;) labels Jan 7, 2024
@AlyHKafoury
Copy link
Contributor

@raman-m Can I work on this issue ?

@raman-m
Copy link
Member

raman-m commented Jan 8, 2024

@AlyHKafoury Yes, you can!
With this bug you can go to fix...
Pay attention you need to check a lot of current unit and acceptance tests, and add new ones for "empty placeholder" problem.

@raman-m raman-m added the needs validation Issue has not been replicated or verified yet label Jan 8, 2024
@raman-m
Copy link
Member

raman-m commented Jan 8, 2024

@AlyHKafoury First of all, please verify (validate, repro) this bug.
I've marked the issue with label: needs validation label.

@margaale margaale removed their assignment Jan 8, 2024
@raman-m raman-m assigned AlyHKafoury and unassigned philproctor Jan 8, 2024
@raman-m raman-m added the Jan'24 January 2024 release label Jan 8, 2024
@AlyHKafoury
Copy link
Contributor

@raman-m I am adding a unit test for it first and then will fix it, after I reproduce and create a unit test

@AlyHKafoury
Copy link
Contributor

@raman-m I can verify the error exists and I have a unit-test for it here https://github.com/ThreeMammals/Ocelot/pull/1911/files

@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed needs validation Issue has not been replicated or verified yet help wanted Not actively being worked on. If you plan to contribute, please drop a note. good first issue Should be pretty easy to do labels Jan 10, 2024
@raman-m
Copy link
Member

raman-m commented Jan 10, 2024

Accepted due to open PR #1911

@raman-m raman-m changed the title Empty placeholder: Failed to match ReRoute configuration for upstream path Empty placeholders: Failed to match Route configuration for upstream path Jan 10, 2024
@raman-m raman-m added Nov'23 November 2023 release and removed Jan'24 January 2024 release labels Jan 17, 2024
@raman-m raman-m modified the milestones: January'24, Nov-December'23 Jan 17, 2024
raman-m added a commit that referenced this issue Jan 18, 2024
…ll placeholders at the end of template (#1911)

* Update RoutingTests.cs

* Fix end of line empty placeholder

* Fix unit tests

* Fix PR Comments

* Update src/Ocelot/Configuration/Creator/UpstreamTemplatePatternCreator.cs

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* Update src/Ocelot/DownstreamRouteFinder/UrlMatcher/UrlPathPlaceholderNameAndValueFinder.cs

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* Update test/Ocelot.AcceptanceTests/RoutingTests.cs

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* Update test/Ocelot.AcceptanceTests/RoutingTests.cs

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* Update test/Ocelot.UnitTests/Configuration/UpstreamTemplatePatternCreatorTests.cs

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* compact the tests constant name

* remove constant

* Update RoutingTests.cs

* Update RoutingTests.cs

* Update RoutingTests.cs

* Update UrlPathPlaceholderNameAndValueFinderTests.cs

* Use range operator

* Use expression body for method

* Update src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>

* Update DownstreamUrlCreatorMiddleware.cs

* add extra unit tests

* Update RoutingTests.cs

* Clean code

* Update UrlPathPlaceholderNameAndValueFinderTests.cs

* Update DownstreamUrlCreatorMiddlewareTests.cs

* Update DownstreamUrlCreatorMiddlewareTests.cs

* Fix broken dsPath

* Review tests. Add query string scenarios

* Fix unit test and fix +1 issue

* Add final routing tests for Catch-All query string cases

* Fixed added unit tests

* Test traits

* Update docs of Routing feature

---------

Co-authored-by: Raman Maksimchuk <dotnet044@gmail.com>
@raman-m raman-m added merged Issue has been merged to dev and is waiting for the next release and removed accepted Bug or feature would be accepted as a PR or is being worked on labels Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug merged Issue has been merged to dev and is waiting for the next release Nov'23 November 2023 release
Projects
None yet
7 participants