fix: cleanup defer handling within the QP

[dariuszkuc]

If user explicitly opt-out of the @defer support, deferred queries will be rejected by the router with GraphQL validation error (unknown directive). It is not possible to plan a query with a @defer in it when defer support is disabled so we can simplify this logic.

---

Checklist

Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.

• Changes are compatible¹
• Documentation² completed
• Performance impact assessed and acceptable
• Tests added and passing³
  • Unit Tests
  • Integration Tests
  • Manual Tests

Exceptions

Note any exceptions here

Notes

Footnotes

1. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩

2. Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩

3. Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩

[svc-apollo-docs]

✅ Docs Preview Ready

No new or changed pages found.

[github-actions]

@dariuszkuc, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

[router-perf]

CI performance tests

• connectors-const - Connectors stress test that runs with a constant number of users
• const - Basic stress test that runs with a constant number of users
• demand-control-instrumented - A copy of the step test, but with demand control monitoring and metrics enabled
• demand-control-uninstrumented - A copy of the step test, but with demand control monitoring enabled
• enhanced-signature - Enhanced signature enabled
• events - Stress test for events with a lot of users and deduplication ENABLED
• events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
• events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
• events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
• events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
• events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
• extended-reference-mode - Extended reference mode enabled
• large-request - Stress test with a 1 MB request payload
• no-tracing - Basic stress test, no tracing
• reload - Reload test over a long period of time at a constant rate of users
• step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
• step-local-metrics - Field stats that are generated from the router rather than FTV1
• step-with-prometheus - A copy of the step test with the Prometheus metrics exporter enabled
• step - Basic stress test that steps up the number of users over time
• xlarge-request - Stress test with 10 MB request payload
• xxlarge-request - Stress test with 100 MB request payload

[dariuszkuc]

NOTE: this change is unrelated to the defer cleanup. Those tests were not implemented and their JS counterpart were covering general GraphQL validations that are already handled by apollo-rs and reuse fragment logic.

[dariuszkuc]

those tests were verifying without_defer functionality

[dariuszkuc]

Was going to remove QueryPlannerConfig#incremental_delivery but that is actually used to generate proper API schema. Comment on QueryPlanIncrementalDeliveryConfig seems reasonable for "future" use case (i.e. when subgraphs add support for @defer/@stream) .... but it might be a while before it happens so I'd just drop it and reintroduce it back WHEN we need it.

We generate API schema twice -> once by the router and once by the QP. Maybe we should update our QueryPlanner::new signature to accept Schema from the router instead (it contains both the supergraph and api schema)?

[dariuszkuc]

This was lifted out of the old DeferFilter. We don't need to pass schema anymore as at this stage all @defer applications will already have labels normalized.

[dariuszkuc]

We'll tackle the duplicate API schema generation issue and cleanup the overall API in other PR.

[duckki]

Looks good!

[TylerBloom]

Other than the one comment about a test being removed, looks good to me.

[TylerBloom]

Is there a reason this was removed? It was ported directly from JS, and, IMO, is testing behavior that we care about.

[duckki]

The changed behavior does not allow @defer directive if defer is enabled, while previously @defer was removed silently. So, the tested scenario can no longer happen.

However, I just noticed the test would still be able to run. I think this function need to be updated:

router/apollo-federation/tests/query_plan/build_query_plan_support.rs

Line 98 in 6342406

include_defer: true,

pub(crate) fn api_schema_and_planner(
    function_path: &'static str,
    config: QueryPlannerConfig,
    subgraph_names_and_schemas: &[(&str, &str)],
) -> (ValidFederationSchema, QueryPlanner) {
    let supergraph = compose(function_path, subgraph_names_and_schemas);
    let supergraph = apollo_federation::Supergraph::new(&supergraph).unwrap();
    let planner = QueryPlanner::new(&supergraph, config).unwrap();
    let api_schema_config = apollo_federation::ApiSchemaOptions {
        include_defer: true,
        include_stream: false,
    };
    let api_schema = supergraph.to_api_schema(api_schema_config).unwrap();
    (api_schema, planner)
}

The config should control ApiSchemaOptions's include_defer.

[dariuszkuc]

thats the thing -> outside of tests it is not possible for this scenario to ever reach QP. Old test was verifying the behavior that we would strip out @defer from incoming requests if it was disabled.... but if user explicitly opts out of @defer but still send a query with @defer it will be rejected during GraphQL validation by apollo-rs (as that directive won't exist in the API schema).

So real use cases are

• defer enabled
  • query with defer (need to normalize it)
  • query without defer
• defer disabled
  • query without defer

[dariuszkuc]

Fixed the test api schema config in 6d7a1ca