-
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 7 commits
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 | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -166,11 +166,13 @@ private IEnumerable<Task<HttpContext>> ProcessRouteWithComplexAggregation(Aggreg | |||||||||
| 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(); | ||||||||||
|
||||||||||
| var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Select(s => s.ToString()).Distinct().ToList(); | |
| var values = jObject.SelectTokens(matchAdvancedAgg.JsonPath).Select(s => s.ToString()).Distinct().ToArray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The 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();There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Uh oh!
There was an error while loading. Please reload this page.