Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aggregates + RouteKeysConfig where the array of values does not work correctly #2248

Open
ArtyomZhuravlyov opened this issue Jan 14, 2025 · 13 comments
Assignees
Labels
accepted Bug or feature would be accepted as a PR or is being worked on Aggregation Ocelot feature: Request Aggregation bug Identified as a potential bug

Comments

@ArtyomZhuravlyov
Copy link

ArtyomZhuravlyov commented Jan 14, 2025

Expected Behavior / New Feature

My json

{
  "Routes": [
    {
      "DownstreamPathTemplate": "/comments",
      "DownstreamHostAndPorts": [
        {
          "Host": "localhost",
          "Port": 5002
        }
      ],
      "UpstreamPathTemplate": "/comments",
      "UpstreamHttpMethod": [
        "GET"
      ],
      "Key": "comments"
    },
    {
      "DownstreamPathTemplate": "/users/{id}",
      "DownstreamHostAndPorts": [
        {
          "Host": "localhost",
          "Port": 5001
        }
      ],
      "UpstreamPathTemplate": "/users/{id}",
      "UpstreamHttpMethod": [
        "GET"
      ],
      "Key": "user"
    }
  ],
  "Aggregates": [
    {
      "UpstreamPathTemplate": "/aggregatecommentuser",
      "RouteKeys": [
        "comments",
        "user"
      ],
      "RouteKeysConfig": [
        {
          "RouteKey": "user",
          "JsonPath": "$[*].userId",
          "Parameter": "id"
        }
      ]
    }
  ],
  "GlobalConfiguration": {
    "BaseUrl": "https://localhost:5003"
  }
}

I'm accessing the aggregating url: http://localhost:5003/aggregateСommentUser

The first request occurs for http://localhost:5002/comments
If my first query in the aggregation returns a response

"comments": [
        {
            "id": 1,
            "userId": 1
        },
        {
            "id": 2,
            "userId": 2
        }
    ],

Then when I request information about users, I should get a response.

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

And the requests themselves should look like this:
1. http://localhost:5002/comments
2. http://localhost:5001/users/1
3. http://localhost:5001/users/2

Actual Behavior / Motivation for New Feature

I'm accessing the aggregating url: http://localhost:5003/aggregateСommentUser

The first request occurs for http://localhost:5002/comments
my first query in the aggregation returns a response

 "comments": [
        {
            "id": 1,
            "userId": 1
        },
        {
            "id": 2,
            "userId": 2
        }
    ],

I receive a response from the user service

      "user": [
        {
            "id": 1,
            "name": "Artem"
        },
        {
            "id": 1,
            "name": "Artem"
        }
    ]

Because my requests from Ocelot look like this:
1. http://localhost:5002/comments
2. http://localhost:5001/users/1
3. http://localhost:5001/users/1

Link to the repository: https://github.com/ArtyomZhuravlyov/OcelotAggregate

image
image

Specifications

  • Version: 23.4.2
  • Platform: .NET 8
  • Subsystem:
@raman-m raman-m added the Aggregation Ocelot feature: Request Aggregation label Jan 14, 2025
@raman-m
Copy link
Member

raman-m commented Jan 14, 2025

Hello, Artyom!
Your Users service returns the following response
image
image

Yes, you have 2 requests because your downstream service returned 2 user items with the same ID which is 1. Thus, Ocelot makes 2 requests with the same URL which is /user/1.
I am struggling to understand you: why do you report this issue?
Please fix your downstream service and enjoy Ocelot Aggregation feature!
Anything else?

@raman-m raman-m added wontfix No plan to include this issue in the Ocelot core functionality. needs validation Issue has not been replicated or verified yet needs feedback Issue is waiting on feedback before acceptance question Initially seen a question could become a new feature or bug or closed ;) labels Jan 14, 2025
@raman-m
Copy link
Member

raman-m commented Jan 14, 2025

Artyom,
Please be aware that Complex Aggregation is a pilot feature that may contain bugs, and it could be improved. Additionally, this feature is not well-documented: it was released in version 23.1.0 with draft section for complex aggregating. Therefore, consider it as "under development", although the basic functionality should work.

If you need consultation, our teammate will assist you. However, it would be better if you develop your own aggregator first, without a Complex Aggregation logic.

@raman-m raman-m self-assigned this Jan 14, 2025
@ArtyomZhuravlyov
Copy link
Author

No, no, you don't understand me.

I have the following specified in json

"RouteKeysConfig": [
     {
       "RouteKey": "user",
       "JsonPath": "$[*].userId",
       "Parameter": "id"
     }
   ]

This means that this request must be executed at the beginning.
http://localhost:5002/comments

The response always comes from it.

[
        {
            "id": 1,
            "userId": 1
        },
        {
            "id": 2,
            "userId": 2
        }
    ]

This is what the controller looks like


[ApiController]
[Route("[controller]")]
public class CommentsController : ControllerBase
{
    [HttpGet]
    public Comment[] GetAll()
    {
        return new Comment[] { new Comment() { Id = 1, UserId = 1 }, new Comment() { Id = 2, UserId = 2 } };
    }
}

So Ocelot should

  1. take User Id = 1 and create request http://localhost:5001/users/1
  2. take UserId = 2 and create request http://localhost:5001/users/2

Then I will get a response from the first request.

 {
            "id": 1,
            "name": "Artem"
        }

and the response from the second request

```
 {
        "id": 2,
        "name": "Ivan"
    }

[ApiController]
[Route("[controller]")]
public class UsersController : ControllerBase
{
[HttpGet("{id:int}")]
public User? GetById(int id)
{

    var users = new User[] { new User() { Id = 1, Name = "Artem" }, new User() { Id = 2, Name = "Ivan" } };
    return users.SingleOrDefault(x => x.Id == id);
}

}


But Ocelot ignores UserId = 2
And makes two identical requests:
http://localhost:5001/users/1
http://localhost:5001/users/1

        

@raman-m raman-m added bug Identified as a potential bug and removed question Initially seen a question could become a new feature or bug or closed ;) wontfix No plan to include this issue in the Ocelot core functionality. needs feedback Issue is waiting on feedback before acceptance labels Jan 14, 2025
@raman-m
Copy link
Member

raman-m commented Jan 14, 2025

🆗 Fair enough!
I have examined the controller in the solution. It could be a potential bug.

Could you please debug the ProcessRouteWithComplexAggregation method to locate issue?
We need to replicate this bug in debug mode to comprehend the problem.

@ArtyomZhuravlyov
Copy link
Author

ArtyomZhuravlyov commented Jan 14, 2025

I have debugged, attached screenshots of control values where value(userId) = 1, value(userId) = 2

Снимок
Снимок2

It is noticeable that HttpContext stores the value value in TemplatePlaceholderNameAndValues at each iteration.
After the first request, the HttpContext stores {[{{userId}}=1]}.
And when the second request is executed, HttpContext already stores {[{{userId}}=1]} and {[{{userId}}=2]} and tPnv, respectively, also stores two values {[{{userId}}=1]} and {[{{userId}}=2]} with a single userId key.

The problem is that tPnv is a reference from HttpContext.items, and when you add Placeholder to tPnv, Placeholder is added to HttpContext, and each time the number of parameters with one userId key increases, although only the last value should be recorded.

Most likely, only the first value (1) with one userId key per url is always sent. http://localhost:5001/users /{id}

Simply addition of ToList() removes the problem because we are already working with another reference object.
var tPnv = httpContext.Items.TemplatePlaceholderNameAndValues().ToList();

After .ToList();
image

@raman-m
Copy link
Member

raman-m commented Jan 14, 2025

@ArtyomZhuravlyov Thank you for debugging. Do you intend to open a PR to fix this?
As I've previously mentioned, the Complex Aggregation feature is a pilot, and it will be enhanced possibly with redesign. This feature was initially hidden and unpublished, but in version 23.1.0, we redeveloped it by refactoring the Multiplexer and finally published the documentation. However, it remains a pilot feature. It is commendable that you have identified the bug. Thanks!

@ggnaegi What do you think? Anyway, you will be key reviewer 😉

@raman-m raman-m added accepted Bug or feature would be accepted as a PR or is being worked on and removed needs validation Issue has not been replicated or verified yet labels Jan 14, 2025
@ggnaegi
Copy link
Member

ggnaegi commented Jan 14, 2025

@ArtyomZhuravlyov @raman-m well, what we could do, it's just pass the PlaceHolderNameAndValue object to CopyItemsToNewContext. That would be cleaner, and would leave the door open to further changes in the future.

@ArtyomZhuravlyov
Copy link
Author

ArtyomZhuravlyov commented Jan 15, 2025

@raman-m, @ggnaegi I think you know better how to do better.

I think it would be better if you fix it yourself and you open a PR. I have little experience with this.

And one more thing, why not work with a lot of parameters here?

var matchAdvancedAgg = routeKeysConfigs.FirstOrDefault(q => q.RouteKey == downstreamRoute.Key);

For example, I want to pass two parameters to http://localhost:5001/users / and would like to specify

"RouteKeysConfig": [
        {
          "RouteKey": "user",
          "JsonPath": "$[*].userId",
          "Parameter": "id"
        },
         {
          "RouteKey": "user",
          "JsonPath": "$[*].otherParameterId",
          "Parameter": "otherParameter"
        }
      ]

Now because routeKeysConfigs is specified.FirstOrDefault I can't do this.

@ArtyomZhuravlyov
Copy link
Author

@ggnaegi Yes, why not? Why set limits?

For example, I need to pass the following parameters to the second request
api/v2/directories/techplaces/companies/{companyId}/rfmanufactures/{rfmanufactureId}/subdivisions/{subdivisionId}/rfassetlocations

And in my case, I can't get the necessary rfassetlocations by subdivisionId alone, I need the whole chain CompanyId -> rfmanufactureId -> subdivisionId.

@ggnaegi
Copy link
Member

ggnaegi commented Jan 15, 2025

@ArtyomZhuravlyov Ok, let's check this. But first, fix the issue.

@raman-m
Copy link
Member

raman-m commented Jan 15, 2025

@ArtyomZhuravlyov commented on Jan 15

I think it would be better if you fix it yourself and you open a PR. I have little experience with this.

Yes, my dear, we will address this ourselves: the bug has been accepted. We will assign the appropriate priority label based on our internal team capacity, as it appears you will not be contributing.
I plan to fix the bug in the next upcoming release, but not current one.

@raman-m
Copy link
Member

raman-m commented Jan 15, 2025

@ArtyomZhuravlyov commented on Jan 15

OK, but you are asking for too much! This is a new feature proposal, and you are mixing both the new feature and the current bug. If you really want the new feature, please go to the official issue template and use it to propose the new feature and describe the new feature requirements completely.

@raman-m
Copy link
Member

raman-m commented Jan 15, 2025

@ggnaegi commented on Jan 15

Let's discuss and plan this issue internally.
How critical is this bug for Multiplexer? Please mark the issue with appropriate Priority label, in your opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Bug or feature would be accepted as a PR or is being worked on Aggregation Ocelot feature: Request Aggregation bug Identified as a potential bug
Projects
None yet
Development

No branches or pull requests

3 participants