Move the ParsedDocument hack (#7610)

Author: goto-bus-stop

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

@@ -146,7 +146,7 @@ pub(crate) async fn from_supergraph_mock_callback_and_configuration(
         supergraph_service
     });
 
-    let (_, supergraph_creator) = crate::TestHarness::builder()
+    let (_, _, supergraph_creator) = crate::TestHarness::builder()
         .configuration(configuration.clone())
         .supergraph_hook(move |_| supergraph_service.clone().boxed())
         .build_common()
@@ -196,7 +196,7 @@ pub(crate) async fn empty() -> impl Service<
         .expect_clone()
         .returning(MockSupergraphService::new);
 
-    let (_, supergraph_creator) = crate::TestHarness::builder()
+    let (_, _, supergraph_creator) = crate::TestHarness::builder()
         .configuration(Default::default())
         .supergraph_hook(move |_| supergraph_service.clone().boxed())
         .build_common()

apollo-router/src/services/router/snapshots/apollo_router__services__router__tests__invalid_input_enum.snap

@@ -0,0 +1,21 @@
+---
+source: apollo-router/src/services/router/tests.rs
+expression: response
+snapshot_kind: text
+---
+{
+  "errors": [
+    {
+      "message": "Value \"C\" does not exist in \"InputEnum\" enum.",
+      "locations": [
+        {
+          "line": 1,
+          "column": 21
+        }
+      ],
+      "extensions": {
+        "code": "GRAPHQL_VALIDATION_FAILED"
+      }
+    }
+  ]
+}

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

@@ -832,3 +832,74 @@ fn batch_with_three_unique_queries() -> Request<RouterBody> {
             router::body::from_bytes(result)
         })
 }
+
+const ENUM_SCHEMA: &str = r#"schema
+    @link(url: "https://specs.apollo.dev/link/v1.0")
+    @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) {
+      query: Query
+   }
+   directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA
+   directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE
+   directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION
+   directive @join__graph(name: String!, url: String!) on ENUM_VALUE
+   directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE
+   directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR
+   directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION
+
+   scalar link__Import
+
+   enum link__Purpose {
+     SECURITY
+     EXECUTION
+   }
+
+   scalar join__FieldSet
+
+   enum join__Graph {
+       USER @join__graph(name: "user", url: "http://localhost:4001/graphql")
+       ORGA @join__graph(name: "orga", url: "http://localhost:4002/graphql")
+   }
+   type Query @join__type(graph: USER) @join__type(graph: ORGA){
+      test(input: InputEnum): String @join__field(graph: USER)
+   }
+
+   enum InputEnum @join__type(graph: USER) @join__type(graph: ORGA) {
+    A
+    B
+  }"#;
+
+// Companion test: services::supergraph::tests::invalid_input_enum
+#[tokio::test]
+async fn invalid_input_enum() {
+    let service = crate::TestHarness::builder()
+        .configuration_json(serde_json::json!({
+            "include_subgraph_errors": {
+                "all": true,
+            },
+        }))
+        .unwrap()
+        .schema(ENUM_SCHEMA)
+        .build_router()
+        .await
+        .unwrap();
+
+    let request = supergraph::Request::fake_builder()
+        .query("query { test(input: C) }")
+        .build()
+        .unwrap()
+        .try_into()
+        .unwrap();
+    let response = service
+        .clone()
+        .oneshot(request)
+        .await
+        .unwrap()
+        .next_response()
+        .await
+        .expect("should have one response")
+        .unwrap();
+
+    let response: serde_json::Value = serde_json::from_slice(&response).unwrap();
+
+    insta::assert_json_snapshot!(response);
+}

apollo-router/src/services/supergraph/service.rs

@@ -205,7 +205,6 @@ async fn service_call(
         planning,
         body.operation_name.clone(),
         context.clone(),
-        schema.clone(),
         // We cannot assume that the query is present as it may have been modified by coprocessors or plugins.
         // There is a deeper issue here in that query analysis is doing a bunch of stuff that it should not and
         // places the results in context. Therefore plugins that have modified the query won't actually take effect.
@@ -787,31 +786,8 @@ async fn plan_query(
     mut planning: CachingQueryPlanner<QueryPlannerService>,
     operation_name: Option<String>,
     context: Context,
-    schema: Arc<Schema>,
     query_str: String,
 ) -> Result<QueryPlannerResponse, CacheResolverError> {
-    // FIXME: we have about 80 tests creating a supergraph service and crafting a supergraph request for it
-    // none of those tests create an executable document to put it in the context, and the document cannot be created
-    // from inside the supergraph request fake builder, because it needs a schema matching the query.
-    // So while we are updating the tests to create a document manually, this here will make sure current
-    // tests will pass.
-    // During a regular request, `ParsedDocument` is already populated during query analysis.
-    // Some tests do populate the document, so we only do it if it's not already there.
-    if !context.extensions().with_lock(|lock| {
-        lock.contains_key::<crate::services::layers::query_analysis::ParsedDocument>()
-    }) {
-        let doc = crate::spec::Query::parse_document(
-            &query_str,
-            operation_name.as_deref(),
-            &schema,
-            &Configuration::default(),
-        )
-        .map_err(crate::error::QueryPlannerError::from)?;
-        context.extensions().with_lock(|lock| {
-            lock.insert::<crate::services::layers::query_analysis::ParsedDocument>(doc)
-        });
-    }
-
     let qpr = planning
         .call(
             query_planner::CachingRequest::builder()

apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__invalid_input_enum-2.snap

@@ -1,19 +1,15 @@
 ---
 source: apollo-router/src/services/supergraph/tests.rs
 expression: response
+snapshot_kind: text
 ---
 {
   "errors": [
     {
-      "message": "Value \"C\" does not exist in \"InputEnum\" enum.",
-      "locations": [
-        {
-          "line": 1,
-          "column": 21
-        }
-      ],
+      "message": "invalid variable `$input`: found JSON string for GraphQL type `InputEnum`",
       "extensions": {
-        "code": "GRAPHQL_VALIDATION_FAILED"
+        "name": "input",
+        "code": "VALIDATION_INVALID_TYPE_VARIABLE"
       }
     }
   ]

apollo-router/src/services/supergraph/snapshots/apollo_router__services__supergraph__tests__invalid_input_enum.snap

@@ -3634,41 +3634,30 @@ const ENUM_SCHEMA: &str = r#"schema
     B
   }"#;
 
+// Companion test: services::router::tests::invalid_input_enum
 #[tokio::test]
 async fn invalid_input_enum() {
     let service = TestHarness::builder()
-        .configuration_json(serde_json::json!({"include_subgraph_errors": { "all": true } }))
+        .configuration_json(serde_json::json!({
+            "include_subgraph_errors": {
+                "all": true,
+            },
+        }))
         .unwrap()
         .schema(ENUM_SCHEMA)
         //.extra_plugin(subgraphs)
         .build_supergraph()
         .await
         .unwrap();
 
-    let request = supergraph::Request::fake_builder()
-        .query("query { test(input: C) }")
-        .context(defer_context())
-        // Request building here
-        .build()
-        .unwrap();
-    let response = service
-        .clone()
-        .oneshot(request)
-        .await
-        .unwrap()
-        .next_response()
-        .await
-        .unwrap();
-
-    insta::assert_json_snapshot!(response);
-
     let request = supergraph::Request::fake_builder()
         .query("query($input: InputEnum) { test(input: $input) }")
         .variable("input", "INVALID")
         .context(defer_context())
         // Request building here
         .build()
         .unwrap();
+
     let response = service
         .oneshot(request)
         .await

apollo-router/src/services/supergraph/tests.rs

@@ -272,7 +272,7 @@ impl<'a> TestHarness<'a> {
 
     pub(crate) async fn build_common(
         self,
-    ) -> Result<(Arc<Configuration>, SupergraphCreator), BoxError> {
+    ) -> Result<(Arc<Configuration>, Arc<Schema>, SupergraphCreator), BoxError> {
         let builder = if self.schema.is_none() {
             self.subgraph_hook(|subgraph_name, default| match subgraph_name {
                 "products" => canned::products_subgraph().boxed(),
@@ -297,32 +297,57 @@ impl<'a> TestHarness<'a> {
         let supergraph_creator = YamlRouterFactory
             .inner_create_supergraph(
                 config.clone(),
-                schema,
+                schema.clone(),
                 None,
                 None,
                 Some(builder.extra_plugins),
                 Default::default(),
             )
             .await?;
 
-        Ok((config, supergraph_creator))
+        Ok((config, schema, supergraph_creator))
     }
 
     /// Builds the supergraph service
     pub async fn build_supergraph(self) -> Result<supergraph::BoxCloneService, BoxError> {
-        let (_config, supergraph_creator) = self.build_common().await?;
+        let (config, schema, supergraph_creator) = self.build_common().await?;
 
-        Ok(tower::service_fn(move |request| {
+        Ok(tower::service_fn(move |request: supergraph::Request| {
             let router = supergraph_creator.make();
 
+            // The supergraph service expects a ParsedDocument in the context. In the real world,
+            // that is always populated by the router service. For the testing harness, however,
+            // tests normally craft a supergraph request manually, and it's inconvenient to
+            // manually populate the ParsedDocument. Instead of doing it many different ways
+            // over and over in different tests, we simulate that part of the router service here.
+            let body = request.supergraph_request.body();
+            // If we don't have a query we definitely won't have a parsed document.
+            if let Some(query_str) = body.query.as_deref() {
+                let operation_name = body.operation_name.as_deref();
+                if !request.context.extensions().with_lock(|lock| {
+                    lock.contains_key::<crate::services::layers::query_analysis::ParsedDocument>()
+                }) {
+                    let doc = crate::spec::Query::parse_document(
+                        query_str,
+                        operation_name,
+                        &schema,
+                        &config,
+                    )
+                    .expect("parse error in test");
+                    request.context.extensions().with_lock(|lock| {
+                        lock.insert::<crate::services::layers::query_analysis::ParsedDocument>(doc)
+                    });
+                }
+            }
+
             async move { router.oneshot(request).await }
         })
         .boxed_clone())
     }
 
     /// Builds the router service
     pub async fn build_router(self) -> Result<router::BoxCloneService, BoxError> {
-        let (config, supergraph_creator) = self.build_common().await?;
+        let (config, _schema, supergraph_creator) = self.build_common().await?;
         let router_creator = RouterCreator::new(
             QueryAnalysisLayer::new(supergraph_creator.schema(), Arc::clone(&config)).await,
             Arc::new(PersistedQueryLayer::new(&config).await.unwrap()),
@@ -350,7 +375,7 @@ impl<'a> TestHarness<'a> {
         use crate::axum_factory::axum_http_server_factory::make_axum_router;
         use crate::router_factory::RouterFactory;
 
-        let (config, supergraph_creator) = self.build_common().await?;
+        let (config, _schema, supergraph_creator) = self.build_common().await?;
         let router_creator = RouterCreator::new(
             QueryAnalysisLayer::new(supergraph_creator.schema(), Arc::clone(&config)).await,
             Arc::new(PersistedQueryLayer::new(&config).await.unwrap()),

apollo-router/src/test_harness.rs

@@ -107,51 +107,6 @@ async fn two_operations() {
     "###);
 }
 
-#[tokio::test]
-async fn operation_name_error() {
-    let request = Request::fake_builder()
-        .query(
-            r#"
-                query ThisOp { me { id } }
-                query OtherOp { me { id } }
-            "#,
-        )
-        .build()
-        .unwrap();
-    let response = make_request(request).await;
-    insta::assert_json_snapshot!(response, @r###"
-    {
-      "errors": [
-        {
-          "message": "Must provide operation name if query contains multiple operations.",
-          "extensions": {
-            "code": "GRAPHQL_VALIDATION_FAILED"
-          }
-        }
-      ]
-    }
-    "###);
-
-    let request = Request::fake_builder()
-        .query("query ThisOp { me { id } }")
-        .operation_name("NonExistentOp")
-        .build()
-        .unwrap();
-    let response = make_request(request).await;
-    insta::assert_json_snapshot!(response, @r###"
-    {
-      "errors": [
-        {
-          "message": "Unknown operation named \"NonExistentOp\"",
-          "extensions": {
-            "code": "GRAPHQL_UNKNOWN_OPERATION_NAME"
-          }
-        }
-      ]
-    }
-    "###);
-}
-
 #[tokio::test]
 async fn mixed() {
     let request = Request::fake_builder()