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

Regression at DownstreamUrlCreatorMiddleware #2002

Open
bbenameur opened this issue Mar 22, 2024 · 6 comments · May be fixed by #2003
Open

Regression at DownstreamUrlCreatorMiddleware #2002

bbenameur opened this issue Mar 22, 2024 · 6 comments · May be fixed by #2003
Assignees
Labels
accepted Bug or feature would be accepted as a PR or is being worked on bug Identified as a potential bug Mar-Apr'24 March-April 2024 release Routing Ocelot feature: Routing

Comments

@bbenameur
Copy link

bbenameur commented Mar 22, 2024

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

  • Version: all version from 22.0.0+
@iam-black
Copy link

iam-black commented Mar 22, 2024

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
Copy link
Member

raman-m commented Mar 22, 2024

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 ae43f32)

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

  • Define {userName} and {groupName} if appropriate query parameters exist.
"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

  • Use Catch All approach and try to merge URL prefixes caring about {role} placeholder and roleId parameter only.
"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 raman-m added wontfix No plan to include this issue in the Ocelot core functionality. waiting Waiting for answer to question or feedback from issue raiser Routing Ocelot feature: Routing labels Mar 22, 2024
@raman-m raman-m self-assigned this Mar 22, 2024
@raman-m
Copy link
Member

raman-m commented Mar 22, 2024

@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
Copy link
Author

bbenameur commented Mar 25, 2024

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 : #2003 work correclty with a little code , mabay is not a
Regression but improvement or feature,

@raman-m
Copy link
Member

raman-m commented Apr 1, 2024

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 : #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 raman-m closed this as not planned Won't fix, can't repro, duplicate, stale Apr 1, 2024
@raman-m raman-m added the Mar-Apr'24 March-April 2024 release label Apr 20, 2024
@raman-m raman-m added this to the March-April'24 milestone Apr 20, 2024
@raman-m raman-m removed the waiting Waiting for answer to question or feedback from issue raiser label Apr 20, 2024
@raman-m raman-m added bug Identified as a potential bug accepted Bug or feature would be accepted as a PR or is being worked on and removed wontfix No plan to include this issue in the Ocelot core functionality. labels May 9, 2024
@raman-m
Copy link
Member

raman-m commented May 9, 2024

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

@raman-m raman-m reopened this May 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Bug or feature would be accepted as a PR or is being worked on bug Identified as a potential bug Mar-Apr'24 March-April 2024 release Routing Ocelot feature: Routing
Projects
None yet
3 participants