#2002 Early removal of a replaced placeholder parameter in DownstreamUrlCreatorMiddleware

[bbenameur]

Fixes #2002

• Regression at DownstreamUrlCreatorMiddleware #2002

Proposed Changes

Remove only added query to placeholders.

[raman-m]

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]

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]

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]

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]

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.

[ggnaegi]

I have added an alternate method, up to you.

[raman-m]

@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]

Again! 😬

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

[raman-m]

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]

Ready for delivery ✅

• Code review ✔️✔️
• Team approvals ✔️✔️
• Unit testing ✔️ → DownstreamUrlCreatorMiddlewareTests → link1
• Acceptance testing ✔️ → RoutingWithQueryStringTests → link2
• Updated docs ✔️ → routing.rst → link3