#2248 Ensure correct mapping of RouteKeysConfig arrays in aggregates

[NandanDevHub]

Fixes #2248

• Aggregates + RouteKeysConfig where the array of values does not work correctly #2248
  Aggregated route failing to resolve nested userId parameter during downstream calls

This PR fixes an issue where aggregated routes failed to correctly resolve nested route parameters when the downstream route expected a placeholder (for example, user/{id}) derived from the first service response.

Previously, the aggregator was not correctly substituting the JsonPath parameter value into the downstream request, resulting in empty or incorrect downstream calls during multi-service aggregation.

Root Cause

• The AggregatesCreator logic did not properly bind the JsonPath extracted value to the downstream route template.
• The MultiplexingMiddleware was not maintaining consistent parameter mapping when multiple downstream responses were aggregated sequentially.

Proposed Changes

• Updated AggregatesCreator to correctly propagate extracted route parameters (JsonPath → Parameter) into the downstream template.
• Adjusted MultiplexingMiddleware to ensure each downstream request uses the resolved parameter before invocation.

Verified via

• Verified locally using the provided test setup (comments and user mock services via Ocelot gateway - reproduced locally).
• Gateway now correctly returns the aggregated response:

{
  "comments": [
    { "id": 1, "userId": 1 },
    { "id": 2, "userId": 2 }
  ],
  "user": [
    { "id": 1, "name": "Artem" },
    { "id": 2, "name": "Ivan" }
  ]
}

• Also validated through Postman to confirm downstream aggregation works as expected.
• All aggregate related acceptance tests passed except for a few port-collision test cases, which were environmental (macOS multiple test runners binding the same ports).

[coveralls]

coverage: 91.737% (-0.02%) from 91.759%
when pulling 4dc2d1f on NandanDevHub:fix/aggregate-routekeys-nandan
into e3ad51c on ThreeMammals:develop.

[raman-m]

@NandanDevHub Hi Nandan!
Thanks for your interest in Ocelot! 🐯
I noticed several fictitious changes stemming from the EOL Gotchas issue. I'll help you resolve them a bit later, as I know the best way to fix them. To be fair, reviewing the actual code is challenging with the fictitious changes, so I'll revisit the PR later as well.

The current CI build 268 is green, which means nothing was broken. However, since no tests were attached and there are only one or two tests related to complex aggregation, we definitely need more thorough tests. A must-have is writing bug #2248 repro acceptance tests to confirm that the fix resolves the original issue. While manual testing in Postman is a useful part of development, it's not enough for final delivery.

FYI, I won't mark this PR as draft because I believe it should be straightforward for you to handle the required testing given our development process. It's very likely this PR will be included in the next release for version 25.0, allowing this bug fix to be rolled out.

P.S. The current CI status is red due to a failed Coveralls check that says, "Please write more unit tests." 😉 Additionally, Coveralls provided this status update.

[raman-m]

@ggnaegi Guil, would you mind taking the lead and pair-programming on this PR?
If you're okay with it, please assign yourself as an Assignee.
I'll take on the role of supervisor, so let me focus and complete the current hot release v24.1.

[NandanDevHub]

Okay @raman-m Thenk you for the detailed feedback!
Got it about the EOL part, I’ll leave that as is for now, and will check locally again regarding EOL, and will soon add on the acceptance test for #2248.
I’ll commit the test shortly so it’s easier to review the actual fix.

[NandanDevHub]

@raman-m I’ve added the acceptance test for #2248 covering the RouteKeys array case.
It builds fine on my local setup for both net8.0 and net9.0.
Please let me know if anything looks off or if I should adjust the test structure or assertions.

[raman-m]

Avoid using AI tools to generate code❗

I noticed that the code in commit 19668d4 appears to have been generated by AI 😠
Code created this way will not be merged, and it won't help you understand the Ocelot codebase or successfully complete and deliver features or bug fixes. Moreover, the test is failing, dude 🙄

The test is currently failing❗

• Check the failed build here

[raman-m]

Something unusual is happening after I reverted the MultiplexingMiddleware code in commit bb83253. This is the exact copy of upstream develop branch.
This file still includes a few fictious changes 👇

If it's alright with you, I'll rebase the branch onto the upstream develop. This should help clean up any fictitious changes from the commit history... So, going to rebase...

[raman-m]

@NandanDevHub Now the changes look good. The test has been moved to the AggregateTests class.
I think we can begin the code review now.

[raman-m]

Ready for Code Review

@NandanDevHub After you complete the test and demonstrate that it effectively covers the bug's scenario, I will review this PR again.

[NandanDevHub]

@raman-m sorry man for the trouble, especially fixing the EOL issues, cleaning up the branch, and rebasing everything. thanks man appreciate the effort you put in to make it right,

and will add the test in same tests for test to cover the bug scenario.

[NandanDevHub]

@raman-m The new test Should_expand_jsonpath_array_into_multiple_parameterized_calls checks that when a downstream service returns an array for example [{"userId":1},{"userId":2}], so that ocelot expands those values into multiple parameterized calls to /users/{userId} and aggregates the responses as expected.

I ran the test locally twice once all and again with the two 2248 related tests do passes

[RaynaldM]

Suggested change

	var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Select(s => s.ToString()).Distinct().ToList();
	var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Select(s => s.ToString()).Distinct().ToArray();

[raman-m]

To be fair, creating a list or another collection isn't necessary since the foreach operator already processes IEnumerable object 🤣 LoL →

Ocelot/src/Ocelot/Multiplexer/MultiplexingMiddleware.cs

Lines 169 to 170 in f758ba7

	var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Select(s => s.ToString()).Distinct();
	foreach (var value in values)

@NandanDevHub Please avoid using ToList() to create the list, as it consumes a small amount of heap and adds unnecessary pressure on the garbage collector!
FYI, Distinct() is essentially a wrapper algorithm applied to a collection generated by multiple Select() operations.

[raman-m]

My suggestion:

-        var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Select(s => s.ToString()).Distinct();
+        var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Select(ToString).Distinct();

[NandanDevHub]

@raman-m I've kept the explicit lambda here since SelectTokens() returns JToken items, i felt this way each tokens value is safely converted to string.

Also in below cmmit i confirmed the EOL formatting is fine now.

[raman-m]

OK, fair enough!
Could the code line be like this?
My suggestion:

var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Distinct().Select(s => s.ToString());

The idea is to skip creating the collection and applying the Distinct algorithm afterward. Instead, it's more efficient to search for distinct objects directly and then perform the casting to strings.
I'm not sure about the correctness because Distinct might return all J-tokens.

[raman-m]

Lack of Unit Testing ❗

@NandanDevHub I can't approve this PR because there are no unit tests. This issue is highlighted by the failed Coveralls build, which shows no unit tests and a decrease in the coverage factor. Please focus on adding unit tests for MultiplexingMiddleware.
Unit tests should be added to MultiplexingMiddlewareTests class.

[raman-m]

@RaynaldM approved these changes on Oct 28

Ray, thanks for your approval. I know your suggestions have been applied, but I think we need to wait for Gui's approval since he is the key reviewer and the master of Multiplexer.

[raman-m]

@ggnaegi Are you planning to manage this PR and assist the contributor in delivering it for the current release?
I'm unable to do so because I'm busy, as you know what I'm currently working on. I would really appreciate it if you could handle this bug fix alongside the author.

[NandanDevHub]

@raman-m I have added a new unit test inside MultiplexingMiddlewareTests that exercises the ProcessRouteWithComplexAggregation path in MultiplexingMiddleware

[raman-m]

Sorry, I’m tied up with a hot release and can’t assist right now. Please avoid mentioning me directly, as you already have an assigned mentor. Reach out to Guillaume for further help.

FYI

Thanks for adding the commit 4dc2d1f, but which lines did you actually cover?
The unit testing Coveralls build has failed, showing a 2% decrease in code coverage. You can find the details here → Coveralls Build:

So, I'm concluding that the unit test you added doesn't actually cover the new code!