-
Notifications
You must be signed in to change notification settings - Fork 1.7k
#2248 Ensure correct mapping of RouteKeysConfig arrays in aggregates
#2328
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
base: develop
Are you sure you want to change the base?
Changes from 1 commit
0c4e9a0
19668d4
830fb1f
3395986
a260be3
5817a7e
378ce02
5120304
46252ad
4dc2d1f
363a86f
ab8ec6c
65c53dc
dac7de7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -162,21 +162,25 @@ private Task MapResponsesAsync(HttpContext context, Route route, HttpContext mai | |||||||||
| /// <summary> | ||||||||||
| /// Processing a route with aggregation. | ||||||||||
| /// </summary> | ||||||||||
| private IEnumerable<Task<HttpContext>> ProcessRouteWithComplexAggregation(AggregateRouteConfig matchAdvancedAgg, | ||||||||||
| JToken jObject, HttpContext httpContext, DownstreamRoute downstreamRoute) | ||||||||||
| private IEnumerable<Task<HttpContext>> ProcessRouteWithComplexAggregation( | ||||||||||
| AggregateRouteConfig matchAdvancedAgg, | ||||||||||
| JToken jObject, | ||||||||||
| HttpContext httpContext, | ||||||||||
| DownstreamRoute downstreamRoute) | ||||||||||
| { | ||||||||||
| var processing = new List<Task<HttpContext>>(); | ||||||||||
| var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Select(s => s.ToString()).Distinct(); | ||||||||||
| var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Select(s => s.ToString()).Distinct().ToList(); | ||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To be fair, creating a list or another collection isn't necessary since the Ocelot/src/Ocelot/Multiplexer/MultiplexingMiddleware.cs Lines 169 to 170 in f758ba7
@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.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My suggestion: - var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Select(s => s.ToString()).Distinct();
+ var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Select(ToString).Distinct();
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @raman-m I've kept the explicit lambda here since Also in below cmmit i confirmed the EOL formatting is fine now.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, fair enough! 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. |
||||||||||
|
|
||||||||||
| foreach (var value in values) | ||||||||||
| { | ||||||||||
| var tPnv = httpContext.Items.TemplatePlaceholderNameAndValues(); | ||||||||||
| tPnv.Add(new PlaceholderNameAndValue('{' + matchAdvancedAgg.Parameter + '}', value)); | ||||||||||
| var tPnv = new List<PlaceholderNameAndValue>(httpContext.Items.TemplatePlaceholderNameAndValues()) | ||||||||||
| { | ||||||||||
| new PlaceholderNameAndValue('{' + matchAdvancedAgg.Parameter + '}', value) | ||||||||||
| }; | ||||||||||
| processing.Add(ProcessRouteAsync(httpContext, downstreamRoute, tPnv)); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return processing; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// <summary> | ||||||||||
| /// Process a downstream route asynchronously. | ||||||||||
| /// </summary> | ||||||||||
|
|
@@ -200,18 +204,18 @@ private static void CopyItemsToNewContext(HttpContext target, HttpContext source | |||||||||
| target.Items.SetIInternalConfiguration(source.Items.IInternalConfiguration()); | ||||||||||
| target.Items.UpsertTemplatePlaceholderNameAndValues(placeholders ?? | ||||||||||
| source.Items.TemplatePlaceholderNameAndValues()); | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| /// <summary> | ||||||||||
| /// Creates a new HttpContext based on the source. | ||||||||||
| /// </summary> | ||||||||||
| /// <param name="source">The base http context.</param> | ||||||||||
| /// <param name="source">The base http context.</param> | ||||||||||
| /// <param name="route">Downstream route.</param> | ||||||||||
| /// <returns>The cloned context.</returns> | ||||||||||
| protected virtual async Task<HttpContext> CreateThreadContextAsync(HttpContext source, DownstreamRoute route) | ||||||||||
| { | ||||||||||
| var from = source.Request; | ||||||||||
| var bodyStream = await CloneRequestBodyAsync(from, route, source.RequestAborted); | ||||||||||
| var from = source.Request; | ||||||||||
| var bodyStream = await CloneRequestBodyAsync(from, route, source.RequestAborted); | ||||||||||
| var target = new DefaultHttpContext | ||||||||||
| { | ||||||||||
| Request = | ||||||||||
|
|
@@ -241,12 +245,12 @@ protected virtual async Task<HttpContext> CreateThreadContextAsync(HttpContext s | |||||||||
| foreach (var header in from.Headers) | ||||||||||
| { | ||||||||||
| target.Request.Headers[header.Key] = header.Value.ToArray(); | ||||||||||
| } | ||||||||||
| // Once the downstream request is completed and the downstream response has been read, the downstream response object can dispose of the body's Stream object | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // Once the downstream request is completed and the downstream response has been read, the downstream response object can dispose of the body's Stream object | ||||||||||
| target.Response.RegisterForDisposeAsync(bodyStream); // manage Stream lifetime by HttpResponse object | ||||||||||
| return target; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| protected virtual Task MapAsync(HttpContext httpContext, Route route, List<HttpContext> contexts) | ||||||||||
| { | ||||||||||
|
|
@@ -255,31 +259,41 @@ protected virtual Task MapAsync(HttpContext httpContext, Route route, List<HttpC | |||||||||
| return Task.CompletedTask; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| // ensure each context retains its correct aggregate key for proper response mapping | ||||||||||
| if (route.DownstreamRouteConfig != null && route.DownstreamRouteConfig.Count > 0) | ||||||||||
| { | ||||||||||
| for (int i = 0; i < contexts.Count && i < route.DownstreamRouteConfig.Count; i++) | ||||||||||
| { | ||||||||||
| var key = route.DownstreamRouteConfig[i].RouteKey; | ||||||||||
| contexts[i].Items["CurrentAggregateRouteKey"] = key; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| var aggregator = _factory.Get(route); | ||||||||||
| return aggregator.Aggregate(route, httpContext, contexts); | ||||||||||
| } | ||||||||||
| protected virtual async Task<Stream> CloneRequestBodyAsync(HttpRequest request, DownstreamRoute route, CancellationToken aborted) | ||||||||||
| { | ||||||||||
| request.EnableBuffering(); | ||||||||||
| if (request.Body.Position != 0) | ||||||||||
| { | ||||||||||
| Logger.LogWarning(() => $"Ocelot does not support body copy without stream in initial position 0 for the route {route.Name()}."); | ||||||||||
| return request.Body; | ||||||||||
| } | ||||||||||
| var targetBuffer = new MemoryStream(); | ||||||||||
| if (request.ContentLength is not null) | ||||||||||
| { | ||||||||||
| await request.Body.CopyToAsync(targetBuffer, (int)request.ContentLength, aborted); | ||||||||||
| targetBuffer.Position = 0; | ||||||||||
| request.Body.Position = 0; | ||||||||||
| } | ||||||||||
| else | ||||||||||
| { | ||||||||||
| Logger.LogInformation(() => $"Aggregation does not support body copy without Content-Length header, skipping body copy for the route {route.Name()}."); | ||||||||||
| } | ||||||||||
| return targetBuffer; | ||||||||||
| } | ||||||||||
| } | ||||||||||
|
|
||||||||||
| protected virtual async Task<Stream> CloneRequestBodyAsync(HttpRequest request, DownstreamRoute route, CancellationToken aborted) | ||||||||||
| { | ||||||||||
| request.EnableBuffering(); | ||||||||||
| if (request.Body.Position != 0) | ||||||||||
| { | ||||||||||
| Logger.LogWarning(() => $"Ocelot does not support body copy without stream in initial position 0 for the route {route.Name()}."); | ||||||||||
| return request.Body; | ||||||||||
| } | ||||||||||
|
|
||||||||||
| var targetBuffer = new MemoryStream(); | ||||||||||
| if (request.ContentLength is not null) | ||||||||||
| { | ||||||||||
| await request.Body.CopyToAsync(targetBuffer, (int)request.ContentLength, aborted); | ||||||||||
| targetBuffer.Position = 0; | ||||||||||
| request.Body.Position = 0; | ||||||||||
| } | ||||||||||
| else | ||||||||||
| { | ||||||||||
| Logger.LogInformation(() => $"Aggregation does not support body copy without Content-Length header, skipping body copy for the route {route.Name()}."); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| return targetBuffer; | ||||||||||
| } | ||||||||||
| } | ||||||||||
Uh oh!
There was an error while loading. Please reload this page.