#2248 Ensure correct mapping of `RouteKeysConfig` arrays in aggregates

[NandanDevHub]

Fixes #2248

• #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: 93.396% (+0.005%) from 93.391% when pulling 65c53dcff8a48d38d180dc50c190ad4062a6728a on NandanDevHub:fix/aggregate-routekeys-nandan into 4730613e2dab8f851e75413c5c20d3cebe7a1aff on ThreeMammals:develop.

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

Something unusual is happening after I reverted the MultiplexingMiddleware code in commit https://github.com/ThreeMammals/Ocelot/pull/2328/commits/bb83253ef65e2669acbd5c8a9463708163a58670. 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

[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 https://github.com/ThreeMammals/Ocelot/pull/2328/commits/4dc2d1f209fd42ed385d4758fd6fa1ad172ac6da, 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!

[NandanDevHub]

Added two additional unit tests in MultiplexingMiddlewareTests to cover the missing branches in ProcessRoutesWithRouteKeysAsync and MapAsync.

@ggnaegi Let me know once you review the code

[NandanDevHub]

Fixed the mistake, updated the unit test which updated the code coverage