expose query plan in extensions in GraphQL Response#1470
Conversation
bnjjj
requested review from
BrynCooke,
Geal,
SimonSapin,
garypen and
o0Ignition0o
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
abernix
left a comment
abernix
left a comment
There was a problem hiding this comment.
Nice! No implementation review from me, just some naming things that I've left as comments. 😉
Thanks for getting this together!
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
|
thanks @abernix it was exactly what I was looking for when asking for review :) |
o0Ignition0o
left a comment
o0Ignition0o
left a comment
There was a problem hiding this comment.
LGTM overall,
Saw a couple of nits here and there, and I believe we can (and should!) iterate on the tests once #1487 lands. Maybe this should have been an integration test.
| where | ||
| K: Into<String>, | ||
| { | ||
| self.entries.get(&key.into()).map(|v| v.value().clone()) |
There was a problem hiding this comment.
Hmm I'm not sure to understand what you mean. I think it's not an and_then but I might be wrong
| req.context.insert(ENABLED_CONTEXT_KEY, true).unwrap(); | ||
| } | ||
| (req.originating_request.body().query.clone(), is_enabled) | ||
| }, move |(query, is_enabled): (Option<String>, bool), f| async move { |
Signed-off-by: Benjamin Coenen <5719034+bnjjj@users.noreply.github.com>
abernix
left a comment
abernix
left a comment
There was a problem hiding this comment.
I found that this actually didn't quite hit the requirements in the original issue so there's a slight adjustment that needs to be made. I could have probably made that more clear in the original issue but I think there's also a couple patterns that we've adopted (more generally) that make this subtle nuance harder to catch. I've left comments within, so let me know what y'all think!
| { | ||
| first | ||
| .extensions | ||
| .insert("apolloQueryPlan", json!({ "object": { "kind": "QueryPlan", "node": plan, "text": query } })); |
There was a problem hiding this comment.
Whoops! This turns out to have been not quite what was needed from the original requirements which actually ask for object and text to be at the same level — not nested within each other.
| .insert("apolloQueryPlan", json!({ "object": { "kind": "QueryPlan", "node": plan, "text": query } })); | |
| .insert("apolloQueryPlan", json!({ "object": { "kind": "QueryPlan", "node": plan }, "text": query })); |
There was a problem hiding this comment.
As an observation, I think this would have been easier to detect in review against the requirements if the JSON blob wasn't wrapped onto one line! (Something like this, but I wrote this on GitHub, so don't it's not necessarily perfect.)
| .insert("apolloQueryPlan", json!({ "object": { "kind": "QueryPlan", "node": plan, "text": query } })); | |
| .insert("apolloQueryPlan", json!({ | |
| "object": { | |
| "kind": "QueryPlan", | |
| "node": plan | |
| }, | |
| "text": query | |
| })); |
| @@ -0,0 +1 @@ | |||
| {"data":{"topProducts":[{"upc":"1","name":"Table","reviews":[{"id":"1","product":{"name":"Table"},"author":{"id":"1","name":"Ada Lovelace"}},{"id":"4","product":{"name":"Table"},"author":{"id":"2","name":"Alan Turing"}}]},{"upc":"2","name":"Couch","reviews":[{"id":"2","product":{"name":"Couch"},"author":{"id":"1","name":"Ada Lovelace"}}]}]},"extensions":{"apolloQueryPlan":{"object":{"kind":"QueryPlan","node":{"kind":"Sequence","nodes":[{"kind":"Fetch","serviceName":"products","variableUsages":["first"],"operation":"query TopProducts__products__0($first:Int){topProducts(first:$first){__typename upc name}}","operationName":"TopProducts__products__0","operationKind":"query","id":null},{"kind":"Flatten","path":["topProducts","@"],"node":{"kind":"Fetch","serviceName":"reviews","requires":[{"kind":"InlineFragment","typeCondition":"Product","selections":[{"kind":"Field","name":"__typename"},{"kind":"Field","name":"upc"}]}],"variableUsages":[],"operation":"query TopProducts__reviews__1($representations:[_Any!]!){_entities(representations:$representations){...on Product{reviews{id product{__typename upc}author{__typename id}}}}}","operationName":"TopProducts__reviews__1","operationKind":"query","id":null}},{"kind":"Parallel","nodes":[{"kind":"Flatten","path":["topProducts","@","reviews","@","product"],"node":{"kind":"Fetch","serviceName":"products","requires":[{"kind":"InlineFragment","typeCondition":"Product","selections":[{"kind":"Field","name":"__typename"},{"kind":"Field","name":"upc"}]}],"variableUsages":[],"operation":"query TopProducts__products__2($representations:[_Any!]!){_entities(representations:$representations){...on Product{name}}}","operationName":"TopProducts__products__2","operationKind":"query","id":null}},{"kind":"Flatten","path":["topProducts","@","reviews","@","author"],"node":{"kind":"Fetch","serviceName":"accounts","requires":[{"kind":"InlineFragment","typeCondition":"User","selections":[{"kind":"Field","name":"__typename"},{"kind":"Field","name":"id"}]}],"variableUsages":[],"operation":"query TopProducts__accounts__3($representations:[_Any!]!){_entities(representations:$representations){...on User{name}}}","operationName":"TopProducts__accounts__3","operationKind":"query","id":null}}]}]},"text":"query TopProducts($first: Int) { topProducts(first: $first) { upc name reviews { id product { name } author { id name } } } }"}}}} No newline at end of file | |||
There was a problem hiding this comment.
At a meta level, these too would be so much simpler to review and get the requirements right and avoid the subtle bug missed during implementation and review if they were formatted JSON!
4 participants
close #1075