expose query plan only works on the first query

[abernix]

If I run the router (from main as of this issue 77621dd) with the experimental feature in #1470 using the following config (using the starstuff getting started graph):

Config

plugins:
  experimental.expose_query_plan: true

It starts up with this:

2022-08-18T17:33:20.231639Z  INFO apollo_router::router_factory: list of plugins plugin_details=[("apollo.csrf", "apollo_router::plugins::csrf::Csrf"), ("apollo.telemetry", "apollo_router::plugins::telemetry::Telemetry"), ("experimental.expose_query_plan", "apollo_router::plugins::expose_query_plan::ExposeQueryPlan")]

And then if I make this request:

Request

curl -s --request POST \
    --header 'Apollo-Expose-Query-Plan: true' \
    --header 'content-type: application/json' \
    --url 'http://127.0.0.1:4000/' \
    --data '{"query":"query QuesoMakesTheWorld {  topProducts {  reviews { author { username } } } me { reviews { product {  name } } } }"}'

It successfully returns:

{
  "data": { /* actual data */ },
  "extensions": {
    "apolloQueryPlan": {
      /*... with the full query plan */
    }
  }
}

However, if I run the query again after that (no matter how many times), it no longer returns the query plan and only returns the data:

{
  "data": { /* actual data */ }
  /* Look ma, no plan */
}

[garypen]

That feels like a caching issue.

[o0Ignition0o]

It is indeed! We can't and shouldn't wrap all of the QueryPlanner plugins with CachingQueryPlanner, that would defeat the purpose of the plugins ^^'

[Geal]

Hmm no that was very intentional, the result of query planning and plugin application only depends on the query, operation name and options, so the result of plugin application should be cached. The issue here is not that the cache is in the wrong place, but that it does not cache the extensions field

[Geal]

That was done in #1484

[o0Ignition0o]

I don't think we should cache the extensions field. I think plugins should always run, because we cannot assume plugins to be stateless.

The specific bit i disagree with is this one The query planner service performs work that should only depend on the schema, the query and query plan options, so that the result of its (heavy) execution can be cached.

While this is true for the bridge_query_planner, it might not be for plugins, especially if they fiddle with the context.

For example let's pretend the first request doesn't set Apollo-Expose-Query-Plan to true, how can we retrieve it in subsequent calls? Caching the extensions field wouldn't change much, unless we maybe always store the plan?

[Geal]

ok so here it's not even the extensions field that should be cached, because it does not exist in the query planner service. The relevant code uses the context:

https://github.com/apollographql/router/pull/1470/files#diff-1ad31184d4599823f80eca03e0ef76b31f913284d898c7ddf302d6f830853638R56

my belief here is that the functionality to expose the query plan should not have been a plugin, but instead a special case inside the supergraph_service. Not everything fits neatly into a plugin, and for core functionality like this we should put it in the right place.

The query planner is a special place in the router, and is designed to be deterministic, so letting plugins introduce whatever they want there is dangerous. In particular, that's why we hid the query plan from the QueryPlannerResponse

router/apollo-router/src/services/mod.rs

Line 362 in 7710b99

pub(crate) content: QueryPlannerContent,

But the plugin to expose it actually ignores that constraint because it is part of the router's crate.

I propose we sleep on it and look for a better solution tomorrow?

[o0Ignition0o]

I'm going to update the PR in light of #1545 and our discussion

[abernix]

Confirmed this is fixed. Thank you!