search

ThreeMammals / Ocelot

.NET API Gateway
https://www.nuget.org/packages/Ocelot
MIT License
8.28k stars 1.63k forks source link

Regression at DownstreamUrlCreatorMiddleware #2002

Closed bbenameur closed 2 months ago

bbenameur commented 5 months ago

Expected Behavior

There is a regression since version 22.0.0 and even in version 23.0.0.

"UpstreamPathTemplate": "/WeatherForecast/{roleId}/groups?something=9874565",
"UpstreamHttpMethod": ["Get"],
"DownstreamPathTemplate": "/account/{{username}}/groups/{{groupName}}/roles?roleId={roleId}",
"DownstreamScheme": "https",
"DownstreamHostAndPorts": [
  { "Host": "localhost", "Port": 7275 }
],

The correct behabior is :

Upstream: /WeatherForecast/123456/groups?something=9874565
Downstream: /account/{{username}}/groups/{{groupName}}/roles?roleid=123456&something=9874565

Actual Behavior / Motivation for New Feature

But currently all query from Downstream path was removed

Steps to Reproduce the Problem

Make sure that you have a query from DownstreamPathTemplate and UpstreamPathTemplate

"UpstreamPathTemplate": "/WeatherForecast/{roleId}/groups?something=9874565",
"UpstreamHttpMethod": ["Get"],
"DownstreamPathTemplate": "/account/{{username}}/groups/{{groupName}}/roles?roleId={roleId}",
"DownstreamScheme": "https",
"DownstreamHostAndPorts": [
  { "Host": "localhost", "Port": 7275 }
],

Specifications

iam-black commented 5 months ago

Same. api/v1/Documents?DocumentId=826933&IsSignedDocuments=true is being forwarded to downstream as api/v1/Documents?26933&IsSignedDocuments=true. Query params in my case are not hardcoded in the Template though.

raman-m commented 5 months ago

Hi @bbenameur ! Have you read our Routing docs carefully? You should pay attention when reading these sections:

This is not "regression"! This is new feature and bug fix of PR #1182 (commit https://github.com/ThreeMammals/Ocelot/commit/ae43f32fe32b322487623e4b1a22299c3d91bb73)

My recommendation is rolling back to version 21.0.0 until you understand how to use new feature. But what I see now is misconfiguration issue of your route JSON ❗

Let me to explain...

Steps to Reproduce the Problem

Make sure that you have a query from DownstreamPathTemplate and UpstreamPathTemplate

"UpstreamPathTemplate": "/WeatherForecast/{roleId}/groups?something=9874565",
"UpstreamHttpMethod": ["Get"],
"DownstreamPathTemplate": "/account/{{username}}/groups/{{groupName}}/roles?roleId={roleId}",
"DownstreamScheme": "https",
"DownstreamHostAndPorts": [
  { "Host": "localhost", "Port": 7275 }
],

Your route definition is invalid! I wonder that your app started at all. You should have some warnings and errors in logs.

Mistake 1: Double curly braces

We must use single { and } braces when defining Placeholders! I have no idea how Ocelot validates this invalid templates, but at least I expect warnings in logs. Valid definition will be:

"UpstreamPathTemplate": "/WeatherForecast/{roleId}/groups?something=9874565",
"DownstreamPathTemplate": "/account/{username}/groups/{groupName}/roles?roleId={roleId}",

Maybe you used double {{ curly braces to emphasize the path parts. But please note, route validator can generate errors because of that.

Mistake 2: Redundant placeholders or absent ones

Both templates must have the same placeholders. Otherwise Ocelot validator generates at least warnings in logs! More valid definition is

"UpstreamPathTemplate": "/WeatherForecast/{roleId}/groups?something=9874565",
"DownstreamPathTemplate": "/account/username/groups/groupName/roles?roleId={roleId}",

If you have to have both {username} and {groupName} placeholders then see Solution 1 below 👇

Mistake 3: Ignoring Catch All approach

As a team, we recommend always to define Catch All routes. But more concrete URLs need to be defined for transformations like your user case of Placeholders for query string. I guess your route definition can be shorter:

"UpstreamPathTemplate": "/WeatherForecast/{roleId}/groups?{everything}",
"DownstreamPathTemplate": "/account/username/groups/groupName/roles?roleId={roleId}&{everything}",

Mistake 4: Ignoring Restrictions on use

Query string manipulation has Restrictions on use when defining template with query strings. Read the 2nd case in Warning for which states:

So, both {userId} placeholder and userId parameter names are the same! Finally, the userId parameter is removed.

In your case the problem with {roleId} path placeholder to be mapped to roleId={roleId}. Based on #1182 feature the roleId parameter will be removed after merging stage. To keep the parameter in query string you have to rename {roleId} placeholder to {role} and make corrections of templates like that:

"UpstreamPathTemplate": "/WeatherForecast/{role}/groups?{everything}",
"DownstreamPathTemplate": "/account/username/groups/groupName/roles?roleId={role}&{everything}",

So, your {role} placeholder will be mapped as roleId parameter and it will be saved during merging stage.

Now let's improve your templates.

Originally logically you wanted to define such generic route:

"UpstreamPathTemplate": "/WeatherForecast/{role}/groups?{everything}",
"DownstreamPathTemplate": "/account/{userName}/groups/{groupName}/roles?roleId={role}&{everything}",

But as I said, {userName} and {groupName} are not presented in upstream! So, Ocelot's validator will generate warnings and errors in logs. To fix this, you can go with two solutions

Solution 1

"UpstreamPathTemplate": "/WeatherForecast/{role}/groups?userId={user}&groupId={group}&{everything}",
"DownstreamPathTemplate": "/account/{user}/groups/{group}/roles?roleId={role}&{everything}",

We believe this is the most correct scenario and it gives exactly what you want.

Solution 2

"UpstreamPathTemplate": "/WeatherForecast/{role}/groups?{everything}",
"DownstreamPathTemplate": "/account/user1/groups/group1/roles?roleId={role}&{everything}",

But this solution requires probably to make a lot of routes. So, that's why to develop and use Solution 1.

Hope it helps!

raman-m commented 5 months ago

@iam-black commented

It is hard to give an advice if no routes definitions, only some URLs! Provide route definition JSON if you need a recommendation on fixing!

bbenameur commented 5 months ago

Hello @raman-m Thank you very much for this detailed explanation. Dont care about Mistake 1 and 2. In my example route I'm only interested in the transformation of query string your solution is work correctly, but it's not perfect to change the placeholder, is not so clean, for you example 

"UpstreamPathTemplate": "/WeatherForecast/{role}/groups?{everything}",
"DownstreamPathTemplate": "/account/{userName}/groups/{groupName}/roles?roleId={role}&{everything}",

/WeatherForecast/{role}/groups?{everything I'm really waiting a roleId or no role And when we look at the REST API standards, no sense in this URL "/WeatherForecast/roles/{role}/groups?{everything}, // logically I am waiting for a RoleId "/account/{userName}/groups/{groupName}/roles?roleId={role}&{everything}" , // logically I am waiting for a RoleId

We have a very large BFF Ocelot and the developer will be able to easily identify the palceholder query string, no need to change.

The code is the MR : https://github.com/ThreeMammals/Ocelot/pull/2003 work correclty with a little code , mabay is not a Regression but improvement or feature,

raman-m commented 4 months ago

I'm really waiting a roleId or no role

I've explained you above and provided you solutions how to keep the parameter in query not breaking behavior of downstream service. Yes, you have to review ocelot.json!

We have a very large BFF Ocelot and the developer will be able to easily identify the palceholder query string, no need to change.

I don't understand you! "No need to change"... Well... Before you realized that new versions have new behavior you had to read Release Notes for version 22.0. After 4 months you came back and you want change and improve something. It is a little bit late!

The code is the MR : https://github.com/ThreeMammals/Ocelot/pull/2003 work correclty with a little code , mabay is not a Regression but improvement or feature,

I don't see any features. The Placeholders logic has a lot of old implemented features and bug fixes! To be fair, it will be very difficult to improve here not breaking and changing old logic and bug fixes. Pay your attention to the fact that removal of replaced placeholders from query string is old feature and it will not be changed. Now I see that you don't like it, but this feature will not be changed.

raman-m commented 3 months ago

I will double check that we had a regression...

sberwal commented 2 months ago

Hi raman

I understand that the fix is pending for this issue due to missing and outdated test cases.

Do you believe this issue needs to be there in 23.3 as it’s delaying other critical issues which already have been fixed from getting released?

So can I request to please move this issue to next release 23.4/24.1 or 24.0 so that 23.3 can go through!

If this is doable though as per the criticality of this issue.

It’s just a request to reconsider!

Thanks, Suraj Berwal