#2002 Early removal of a replaced placeholder parameter in `DownstreamUrlCreatorMiddleware` #2003

bbenameur · 2024-03-22T13:50:41Z

Fixes #2002

Regression at DownstreamUrlCreatorMiddleware #2002

Proposed Changes

Remove only added query to placeholders.

raman-m · 2024-03-22T16:56:04Z

I answered you in #2002, see link!
This PR fixes your rare user case when you wanted just use the same names of placeholder and query param.
So, you wanted to use userId placeholder in query string like ?userId={userId}.
Ocelot has a list of other features in Placeholders logic. And probably your PR will break them...
Placeholders logic is quite complex and stable now, and we don't want to change the logic at all!
Read the docs for correct usage.

As a hotfix DevOps you need to rollback to version 21.0 as I suggested to you.

If you need something more, then just fork Ocelot repo and develop whatever you want!
Good luck!

raman-m · 2024-03-22T17:22:31Z

Strange... the build 3669 is 🟢
That means the unit tests were passed.
So, that means there is no much sense in moving of the code block...

raman-m · 2024-03-22T17:30:29Z

Once again...
In #2002, I previously explained how to utilize the new version 22.0+. It appears that your route templates are currently invalid. Instead, consider leveraging the new behavior of the Placeholders feature. While I don’t observe any regressions when used correctly, it’s possible that we may have introduced a breaking change without adequately notifying the community. As a result, I’ll make a note of this incident to ensure that we always include Breaking Changes in the Release Notes. 📝

bbenameur · 2024-03-25T15:32:17Z

Strange... the build 3669 is 🟢 That means the unit tests were passed. So, that means there is no much sense in moving of the code block...

The build 3669 is green because the new code have no regression and work correctly, but we need some new test that cover the case where parameter name and value are some like userId=userId

raman-m · 2024-03-28T16:31:13Z

but we need some new test that cover the case where param name and value are some like
userId=userId

🆗 Work more!
Our dev process requires tests for sure. Include unit tests and one acceptance test please.
But you must understand that requirement userId={userId} is wrong because param will be removed from query string according to current Placeholders logic!
I've already explained you in 2002 that you have to change your routes like: userId={user} which allows the param be kept in the query.
If you will adapt your route templates then you will get exactly what you need.

src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs

ggnaegi

I have added an alternate method, up to you.

src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs

raman-m · 2024-04-20T08:36:05Z

@ggnaegi left a comment

Do you approve or not?

raman-m

The code changes can break functionality of Query String Placeholders feature of placeholder replacements in query string. I need to understand why old tests green and final decision will be made after realizing that published functionality of Restrictions on use still work well.
Now I realized that the name of this docs section confuses community but actually this is the feature which will not be removed from the docs, but I will give the new name to the section.

raman-m · 2024-04-20T09:16:48Z

test/Ocelot.AcceptanceTests/RoutingWithQueryStringTests.cs

+                        DownstreamPathTemplate = "/api/units/{subscriptionId}/{unitId}/updates",
+                        DownstreamScheme = "http",
+                        DownstreamHostAndPorts =
+                        [
+                            new()
+                            {
+                                Host = "localhost",
+                                Port = port,
+                            },
+                        ],
+                        UpstreamPathTemplate = "/api/subscriptions/{subscriptionId}/updates?unitId={unitId}",


This is copied and pasted test (user scenario) of old tests.
You should not copy old tests at all!
You have to show us that your user scenario is fixed by your code improvement!

raman-m · 2024-04-20T09:20:07Z

test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs

+                            new("{userId}", "webley"),
+                            new("{personId}", "12345"),


Again! 😬

This is copied user scenario from old tests!
Where is testing of your user scenario ❓

raman-m · 2024-04-20T09:22:38Z

test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs

+                .When(x => x.WhenICallTheMiddleware())
+                .Then(x => x.ThenTheDownstreamRequestUriIs($"http://localhost:5000/persons?groupId=6789&personId=12345&userId=webley"))
+                .And(x => ThenTheQueryStringIs("?groupId=6789&personId=12345&userId=webley"))
+                .BDDfy();


We don't use BDDfy for new unit tests anymore! See UnitTest class constructor!
BDDfy will be removed from all unit tests soon... This is absolutely useless framework for unit tests. xUnit works well...

raman-m · 2024-04-20T09:43:03Z

test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs

+                    .WithOriginalValue("/users?userId={userId}&personId={personId}&groupId={groupId}").Build())
+                .WithDownstreamPathTemplate("/persons?personId={personId}&userId={userId}")


I wonder if this scenario and test is green now...
We have a conflict with documentation now.
Published feature will not be removed from docs: see Query String Placeholders and Restrictions on use sections.
You may not like it, but it works well, and fixes a lot of old bugs.
Restrictions on use section will be renamed...

Co-authored-by: Raynald Messié <redbird_project@yahoo.fr>

Please use language version 12.0 or greater.

bbenameur changed the base branch from develop to release/24.0 March 22, 2024 13:53

bbenameur changed the base branch from release/24.0 to develop March 22, 2024 13:58

raman-m added the Routing Ocelot feature: Routing label Mar 22, 2024

raman-m assigned raman-m and bbenameur Mar 22, 2024

raman-m changed the title ~~issues/2002 : Regression at DownstreamUrlCreatorMiddleware~~ #2002 Regression at DownstreamUrlCreatorMiddleware Mar 22, 2024

bbenameur mentioned this pull request Mar 25, 2024

Regression at DownstreamUrlCreatorMiddleware #2002

Open

raman-m requested review from raman-m, RaynaldM and ggnaegi March 28, 2024 16:28

bbenameur force-pushed the fix/issues(2002)-RegressionatDownstreamUrlCreatorMiddleware branch from a69f646 to a87ef17 Compare March 31, 2024 23:53

raman-m linked an issue Apr 1, 2024 that may be closed by this pull request

Regression at DownstreamUrlCreatorMiddleware #2002

Open

raman-m changed the title ~~#2002 Regression at DownstreamUrlCreatorMiddleware~~ #2002 Early removal of a replaced placeholder parameter in DownstreamUrlCreatorMiddleware Apr 1, 2024

RaynaldM approved these changes Apr 2, 2024

View reviewed changes

src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs Outdated Show resolved Hide resolved

ggnaegi reviewed Apr 2, 2024

View reviewed changes

src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs Outdated Show resolved Hide resolved

bbenameur force-pushed the fix/issues(2002)-RegressionatDownstreamUrlCreatorMiddleware branch 2 times, most recently from 7f07a5f to 9c956a0 Compare April 4, 2024 08:23

raman-m added the Mar-Apr'24 March-April 2024 release label Apr 4, 2024

raman-m added this to the March-April'24 milestone Apr 4, 2024

raman-m force-pushed the fix/issues(2002)-RegressionatDownstreamUrlCreatorMiddleware branch from 6ffe2be to 8a32b67 Compare April 5, 2024 15:35

raman-m force-pushed the fix/issues(2002)-RegressionatDownstreamUrlCreatorMiddleware branch from 8a32b67 to a90b160 Compare April 20, 2024 08:25

raman-m requested changes Apr 20, 2024

View reviewed changes

raman-m force-pushed the fix/issues(2002)-RegressionatDownstreamUrlCreatorMiddleware branch 2 times, most recently from 0b0f29c to efe3927 Compare April 25, 2024 17:51

bbenameur and others added 6 commits May 7, 2024 14:20

issues/2002 : Regression at DownstreamUrlCreatorMiddleware

7572d17

issues/2002:: add some unit tests and acceptance test

92a333e

issues/2002: fix test

3b7da60

Apply suggestions from code review

fc24279

Co-authored-by: Raynald Messié <redbird_project@yahoo.fr>

issues/2002: fix build

4480d3f

CS8936 Feature 'collection expressions' is not available in C# 10.0.

42e39c1

Please use language version 12.0 or greater.

raman-m force-pushed the fix/issues(2002)-RegressionatDownstreamUrlCreatorMiddleware branch from efe3927 to 42e39c1 Compare May 7, 2024 11:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2002 Early removal of a replaced placeholder parameter in `DownstreamUrlCreatorMiddleware` #2003

#2002 Early removal of a replaced placeholder parameter in `DownstreamUrlCreatorMiddleware` #2003

bbenameur commented Mar 22, 2024 •

edited by raman-m

raman-m commented Mar 22, 2024

raman-m commented Mar 22, 2024

raman-m commented Mar 22, 2024

bbenameur commented Mar 25, 2024 •

edited by raman-m

raman-m commented Mar 28, 2024 •

edited

ggnaegi left a comment

raman-m commented Apr 20, 2024

raman-m left a comment

raman-m Apr 20, 2024

raman-m Apr 20, 2024

raman-m Apr 20, 2024

raman-m Apr 20, 2024 •

edited

		.WithOriginalValue("/users?userId={userId}&personId={personId}&groupId={groupId}").Build())
		.WithDownstreamPathTemplate("/persons?personId={personId}&userId={userId}")

#2002 Early removal of a replaced placeholder parameter in DownstreamUrlCreatorMiddleware #2003

Are you sure you want to change the base?

#2002 Early removal of a replaced placeholder parameter in DownstreamUrlCreatorMiddleware #2003

Conversation

bbenameur commented Mar 22, 2024 • edited by raman-m

Fixes #2002

Proposed Changes

raman-m commented Mar 22, 2024

raman-m commented Mar 22, 2024

raman-m commented Mar 22, 2024

bbenameur commented Mar 25, 2024 • edited by raman-m

raman-m commented Mar 28, 2024 • edited

ggnaegi left a comment

Choose a reason for hiding this comment

raman-m commented Apr 20, 2024

raman-m left a comment

Choose a reason for hiding this comment

raman-m Apr 20, 2024

Choose a reason for hiding this comment

raman-m Apr 20, 2024

Choose a reason for hiding this comment

Again! 😬

raman-m Apr 20, 2024

Choose a reason for hiding this comment

raman-m Apr 20, 2024 • edited

Choose a reason for hiding this comment

#2002 Early removal of a replaced placeholder parameter in `DownstreamUrlCreatorMiddleware` #2003

#2002 Early removal of a replaced placeholder parameter in `DownstreamUrlCreatorMiddleware` #2003

bbenameur commented Mar 22, 2024 •

edited by raman-m

bbenameur commented Mar 25, 2024 •

edited by raman-m

raman-m commented Mar 28, 2024 •

edited

raman-m Apr 20, 2024 •

edited