#2346 Fix problem with downstream URLs where query parameter names get messed up if they contain a placeholder that has a non-empty value

[bhargav-polara]

Fixes #2346

• Query parameter names containing id are corrupted when another route has {id} placeholder (Regression in 23.4.3) #2346

Background

After upgrading to Ocelot 23.4.3, query parameters whose names contain the substring id (e.g., customer_id) are incorrectly rewritten or corrupted if another route exists with a {id} path parameter. See #2346 for details and repro steps.

Problem

When Ocelot tries to remove consumed query string parameters from the downstream request, it was using a simple string Contains check. This caused substring matches (like the id in customer_id) to be incorrectly interpreted as route tokens—resulting in corruption (e.g., customer_id=5172980 becoming customer5172980).

Solution

Changed logic in RemoveQueryStringParametersThatHaveBeenUsedInTemplate to use a Regex match instead of string Contains.
This ensures only exact parameter matches are identified and removed, and substrings like id in customer_id are preserved correctly.

if (!Regex.IsMatch(
        downstreamRequest.Query,
        $@"(?<=^|\?|&){Regex.Escape(parameter)}",
        RegexOptions.IgnoreCase))
{
    continue;
}

---

How to Reproduce

See steps in #2346:

1. Define two routes:
   • One with /finance/v1/payment-methods/{id}
   • One with /finance/v1/payment-methods
2. Call GET /finance/v1/payment-methods?customer_id=5172980
3. The downstream request should forward the query string unmodified.

Testing

• Manually tested scenarios from 2346; parameters like customer_id=5172980 are now correctly forwarded downstream without corruption.
• All existing unit and integration tests pass (dotnet test).

Checklist

• Branch based on develop
• Code follows contribution guidelines
• Tests passing

[coveralls]

coverage: 93.509% (+0.006%) from 93.503%
when pulling cfb1441 on bhargav-polara:bug/2346-query-param-id-regression
into 9fc4e78 on ThreeMammals:develop.

[raman-m]

Problem

When Ocelot tries to remove consumed query string parameters from the downstream request, it was using a simple string Contains check. This caused substring matches (like the id in customer_id) to be incorrectly interpreted as route tokens—resulting in corruption (e.g., customer_id=5172980 becoming customer5172980).

Right! It was my fault when releasing the Ocelot 23.4.3 patch. Sometimes, when you solve a big problem, there’s a chance you might introduce a small, unexpected one 👶

My idea to use the Contains method works/worked well when we also check that both names are the same length. This means the Contains method requires both name lengths to be equal. However, we still need to parse the parameter in the downstreamRequest.Query string or verify its existence in some way. I think the easiest solution is to reuse the functionality of the HttpContext.Request.Query object 👇

IQueryCollection query = context.Request.Query;
bool exists = query.ContainsKey(name) && query.TryGetValue(name, out var pvalue) && pvalue == nAndV.Value;
if (!exists) // !downstreamRequest.Query.Contains(parameter)
{
    continue;
}

To do that, we just need an additional context argument, and that’s all. Parsing the parameter value doesn’t seem necessary, though it might depend. The algorithm following these lines could also be optimized.

Ocelot/src/Ocelot/DownstreamUrlCreator/DownstreamUrlCreatorMiddleware.cs

Line 135 in 9fc4e78

if (!downstreamRequest.Query.Contains(parameter))

[raman-m]

While this solution should fix the bug, it looks like we still need more thorough unit testing with different URLs. Additionally, the lack of acceptance tests is preventing me from approving this version of the bugfix.
Let’s work on finding the optimal solution, as this is a critical middleware for the routing feature. I recommend spending more time refining the approach.

[bhargav-polara]

@raman-m I had tried out this your suggestion but after applying other testcases are getting failed. I have refactored code and added more test Cases.

[raman-m]

Yahoo! The rodeo goes on! 🤠
Did you notice the CI build failed? No rush, though I'm planning to revert the last commit 21661a4.

You're a talented vibe-coder, and the new algorithm seems solid and will handle query strings properly, but instead of the old 10 lines, we now have 30 lines of code proposed by the AI coding agent. This version will heavily consume RAM due to two new collections in the algorithm. Honestly, I prefer the previous Regex-based version since it's better to use more CPU cycles than to increase RAM usage. Have you seen how expensive RAM modules are these days? 😉

I had tried out #2351 (comment) suggestion but after applying other testcases are getting failed. I have refactored code and added more test Cases.

You've added new unit tests generated by the AI coding agent but wrote no acceptance tests. And you didn't run the tests locally, so now the old Routing acceptance tests are failing 👇

Sorry, the last commit will be reverted because the old Regex version is more suitable.

[bhargav-polara]

@raman-m I have refactored the code based on your suggestions and also made a few additional changes to address the failing test cases that appeared after applying them.

[raman-m]

It is great if the unit tests and old acceptance tests are now passing.
Consider adding new acceptance tests to the RoutingWithQueryStringTests class. Be sure to include the test case for #2346 to confirm the bug fix works properly.
Please note, I will perform the final code review after writing the acceptance tests.

[bhargav-polara]

@raman-m Added new acceptance tests

[raman-m]

Ready for code review ✔️

[raman-m]

Ready for delivery ✅

• Code review ✔️
• Unit testing ✔️ ✔️
• Acceptance testing ✔️ ✔️