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

Issue with Aggregator in Ocelot: Requires At Least Two Routes to Execute #1941

Closed
camilocalderont opened this issue Jan 21, 2024 · 11 comments
Closed
Assignees
Labels
Aggregation Ocelot feature: Request Aggregation Delegating Handlers Ocelot feature: Delegating Handlers wontfix No plan to include this issue in the Ocelot core functionality.

Comments

@camilocalderont
Copy link

camilocalderont commented Jan 21, 2024

Expected Behavior / New Feature

Aggregators in Ocelot should execute correctly when configured with a single route, allowing modifications to the response as per the aggregator's logic.

Actual Behavior / Motivation for New Feature

The aggregator does not execute when it is configured with only one route. Instead, the request seems to be handled by another route with a wildcard, and the response from the microservice is returned without any modifications by the aggregator.

Steps to Reproduce the Problem

  1. Configure a route with a wildcard in ocelot.json.
  2. Configure an aggregator with only one route in ocelot.json.
  3. Make a request to the aggregator's route.
  4. Observe that the aggregator does not execute.

Specifications

  • Version: Ocelot 22.0.1
  • Platform: .NET 6
  • Subsystem: N/A
{
  "Routes": [
    {
      "SwaggerKey": "administrador",
      "UpstreamPathTemplate": "/api/administrador/v1/{everything}",
      "UpstreamHttpMethod": ["GET", "POST", "PUT", "DELETE"],
      "DownstreamHostAndPorts": [
        { "Host": "", "Port": 0 }
      ],
      "DownstreamPathTemplate": "/api/{everything}"
    }
  ]
}
{
  "Aggregates": [
    {
      "UpstreamPathTemplate": "/api/mix/v1/IniciarSesion",
      "RouteKeys": ["loginJWT"],
      "Aggregator": "LoginAggregator",
      "Description": "Crea el JWT luego de la respuesta de login de administrador."
    }
  ],
  "Routes": [
    {
      "UpstreamPathTemplate": "/api/administrador/v1/Usuario/login",
      "UpstreamHttpMethod": ["GET", "POST", "PUT", "DELETE"],
      "DownstreamHostAndPorts": [
        { "Host": "", "Port": 0 }
      ],
      "DownstreamPathTemplate": "/api/Usuario/login",
      "Key": "loginJWT",
      "SwaggerKey": "loginJWT"
    }  
  ]
}

Docs

@raman-m raman-m added Aggregation Ocelot feature: Request Aggregation Delegating Handlers Ocelot feature: Delegating Handlers proposal Proposal for a new functionality in Ocelot labels Jan 21, 2024
@raman-m
Copy link
Member

raman-m commented Jan 21, 2024

Hi @camilocalderont !
You needn't to develop aggregator classes.
Use Delegating Handlers feature!
But current restriction to have at least 2 routes is quite natural thing when combining 2 sources of data.
But, make sense to remove this restriction.
When developing aggregator you work with DownstreamResponse object.
When developing delegating handler you work with HttpRequestMessage and HttpResponseMessage objects.
But currently, you need to develop custom Http handler which allows you to transform content both of request and response, HttpRequestMessage and HttpResponseMessage objects.
In your user scenario, you want to convert data of the response, thus you have to work with HttpResponseMessage object!

Hope it helps!

@raman-m
Copy link
Member

raman-m commented Mar 4, 2024

@ggnaegi Oh, no!
We forgot to check this user scenario before the release 😏

@ggnaegi
Copy link
Member

ggnaegi commented Mar 4, 2024

@ggnaegi Oh, no!
We forgot to check this user scenario before the release 😏

Ouch...

@raman-m
Copy link
Member

raman-m commented Mar 5, 2024

@RaynaldM @ggnaegi If not joking... Do we accept it?
His feature request is processing single route by Aggregation (multiplexer) middleware.
So, he wants to process single route by IDefinedAggregator class to convert/analyze data of the response and return converted result to upstream finally.
Current restriction by validators (I guess) is having at least 2 routes with aggregation keys to define an aggregator in Aggregates section.
To be fair, as I recommended, such data conversions can be performed by custom delegating handler.
Is this sufficient argument to start development for multiplexer?
And, seems the author doesn't care about his issue anymore: so, it could be closed after our decision.

@raman-m
Copy link
Member

raman-m commented Mar 5, 2024

@camilocalderont Dear Camilo,
Where are you?

@raman-m raman-m added the waiting Waiting for answer to question or feedback from issue raiser label Mar 5, 2024
@RaynaldM
Copy link
Collaborator

RaynaldM commented Mar 8, 2024

if @camilocalderont wants to do it himself, why not?

@camilocalderont
Copy link
Author

Hi, @RaynaldM, @raman-m, I greatly appreciate your attention to my case. I'm a big fan of Ocelot and use it daily as an orchestrator service and even as an entry point.

Your alternative is very good, but in my case, I use a class to organize and dynamically change the host and port. Therefore, it was better for me to use Aggregators instead of Controllers, as it would duplicate an endpoint that is already in another .Net project being orchestrated. Thus, I had to use an Aggregator and define 2 routes, but only use the first one.

It would be nice to not have that restriction.

@raman-m
Copy link
Member

raman-m commented Mar 12, 2024

@camilocalderont

Thus, I had to use an Aggregator and define 2 routes, but only use the first one.

Sounds like a life hack! 🤣 You are true Ocelot fan! 😉
Imagine we will disable 2 routes validator for Aggregation. What's the goal? Just remove fake route from JSON?
My vote weight is 0.5. I neither refuse nor accept at this time.

@ggnaegi Your opinion?

Voting

Total 2.5 negative of 3

@ggnaegi
Copy link
Member

ggnaegi commented Mar 12, 2024

@raman-m @camilocalderont @RaynaldM By design, if only one route, then when don't make any further checks and process the downstream request. It's a bit straight forward, but that's the way it is at the moment. The problem is probably that the Aggregator has been placed at the wrong spot in the multiplexer and we are mixing concerns. From my side I would prefer moving the Aggregator somewhere else in the pipeline, renaming it, and calling it ResponseTransform instead.

@raman-m
Copy link
Member

raman-m commented Mar 18, 2024

@ggnaegi Your vote is negative, right? ➖
Regarding middleware name... It doesn't matter for me, and I don't see a benefit in renaming.
Regarding a position in the pipeline... It makes sense to find appropriate and right position 👍

@raman-m raman-m self-assigned this Mar 18, 2024
@raman-m raman-m added wontfix No plan to include this issue in the Ocelot core functionality. and removed proposal Proposal for a new functionality in Ocelot waiting Waiting for answer to question or feedback from issue raiser labels Mar 18, 2024
@raman-m
Copy link
Member

raman-m commented Mar 18, 2024

@camilocalderont
Dear Camilo!
Sorry, but we didn't accept the issue for development!
There are 2 reasons for that

  • In version 23.1 we enhanced Multiplexer (see release notes). And with one route Aggregation logic simply skips Aggregation main block and behaves as standard route. This change was introduced because of performance. So, your IDefinedAggregator will not work! It will not be called!
  • I guess your current life hack with 2nd fake route is acceptable solution. Fake route should return empty data or small data block which you will ignore for sure when parsing JSON data.

We will be glad to review your open PRs in future!
Stay with Ocelot! 🐯

@raman-m raman-m closed this as not planned Won't fix, can't repro, duplicate, stale Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aggregation Ocelot feature: Request Aggregation Delegating Handlers Ocelot feature: Delegating Handlers wontfix No plan to include this issue in the Ocelot core functionality.
Projects
None yet
Development

No branches or pull requests

4 participants