fix: Correctly flag incoming POST requests.

fixes #865

A regression happened during our switch to Axum which would mark incoming POST requests as GET, thus triggering our forbid_http_mutations layer, and preventing mutations on the router.
This PR fixes this, and introduces some tests. More integration or axum oriented tests should follow.

When writing a unit test to assert subgraph requests are POST, I noticed the test would never fail. this is because withf only acts as a request router, and panicking (or in our case asserting) within wouldnt cause the router to fail.

This commit rewrites a couple of subgraph tests so they properly notify whether they completed successfully or failed.

NEXT_CHANGELOG.md

@@ -28,7 +28,8 @@ This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm
 ## 🚀 Features
 
 ## 🐛 Fixes
-
+### Correctly flag incoming POST requests [#865](https://github.com/apollographql/router/issues/865)
+A regression happened during our recent switch to axum that would propagate incoming POST requests as GET requests. This has been fixed and we now have several regression tests, pending more integration tests.
 ## 🛠 Maintenance
 
 ## 📚 Documentation

apollo-router-core/src/query_planner/mod.rs

@@ -530,9 +530,11 @@ mod log {
 #[cfg(test)]
 mod tests {
     use super::*;
-    use std::collections::HashMap;
+    use http::Method;
     use std::str::FromStr;
+    use std::sync::atomic::Ordering;
     use std::sync::Arc;
+    use std::{collections::HashMap, sync::atomic::AtomicBool};
     use tower::{ServiceBuilder, ServiceExt};
     macro_rules! test_query_plan {
         () => {
@@ -569,16 +571,18 @@ mod tests {
             root: serde_json::from_str(test_query_plan!()).unwrap(),
         };
 
+        let succeeded: Arc<AtomicBool> = Default::default();
+        let inner_succeeded = Arc::clone(&succeeded);
+
         let mut mock_products_service = plugin_utils::MockSubgraphService::new();
         mock_products_service
             .expect_call()
             .times(1)
-            .withf(|request| {
-                assert_eq!(
-                    request.http_request.body().operation_name,
-                    Some("topProducts_product_0".into())
-                );
-                true
+            .withf(move |request| {
+                let matches = request.http_request.body().operation_name
+                    == Some("topProducts_product_0".into());
+                inner_succeeded.store(matches, Ordering::SeqCst);
+                matches
             });
         query_plan
             .execute(
@@ -592,5 +596,44 @@ mod tests {
                 &Schema::from_str(test_schema!()).unwrap(),
             )
             .await;
+
+        assert!(succeeded.load(Ordering::SeqCst), "incorrect operation name");
+    }
+
+    #[tokio::test]
+    async fn fetch_makes_post_requests() {
+        let query_plan: QueryPlan = QueryPlan {
+            root: serde_json::from_str(test_query_plan!()).unwrap(),
+        };
+
+        let succeeded: Arc<AtomicBool> = Default::default();
+        let inner_succeeded = Arc::clone(&succeeded);
+
+        let mut mock_products_service = plugin_utils::MockSubgraphService::new();
+        mock_products_service
+            .expect_call()
+            .times(1)
+            .withf(move |request| {
+                let matches = request.http_request.method() == Method::POST;
+                inner_succeeded.store(matches, Ordering::SeqCst);
+                matches
+            });
+        query_plan
+            .execute(
+                &Context::new().with_request(Arc::new(http_compat::Request::mock())),
+                &ServiceRegistry::new(HashMap::from([(
+                    "product".into(),
+                    ServiceBuilder::new()
+                        .buffer(1)
+                        .service(mock_products_service.build().boxed()),
+                )])),
+                &Schema::from_str(test_schema!()).unwrap(),
+            )
+            .await;
+
+        assert!(
+            succeeded.load(Ordering::SeqCst),
+            "subgraph requests must be http post"
+        );
     }
 }

apollo-router/src/axum_http_server_factory.rs

@@ -446,13 +446,12 @@ async fn run_graphql_operation(
     Extension(service): Extension<BufferedService>,
     header_map: HeaderMap,
 ) -> impl IntoResponse {
-    let mut http_request = Request::builder()
-        .uri(
-            Uri::from_str(&format!("http://{}{}", host, uri))
-                .expect("the URL is already valid because it comes from axum; qed"),
-        )
-        .body(request)
-        .expect("body has already been parsed; qed");
+    let mut http_request = Request::post(
+        Uri::from_str(&format!("http://{}{}", host, uri))
+            .expect("the URL is already valid because it comes from axum; qed"),
+    )
+    .body(request)
+    .expect("body has already been parsed; qed");
     *http_request.headers_mut() = header_map;
 
     run_graphql_request(service, http_request)

apollo-router/src/plugins/override_url.rs

@@ -63,13 +63,7 @@ mod tests {
         let mut mock_service = MockSubgraphService::new();
         mock_service
             .expect_call()
-            .withf(|req| {
-                assert_eq!(
-                    req.http_request.url(),
-                    &Uri::from_str("http://localhost:8001").unwrap()
-                );
-                true
-            })
+            .withf(|req| req.http_request.url() == &Uri::from_str("http://localhost:8001").unwrap())
             .times(1)
             .returning(move |req: SubgraphRequest| {
                 Ok(plugin_utils::SubgraphResponse::builder()

apollo-router/tests/integration_tests.rs

@@ -198,6 +198,41 @@ async fn queries_should_work_over_get() {
     assert_eq!(registry.totals(), expected_service_hits);
 }
 
+#[tokio::test]
+async fn queries_should_work_over_post() {
+    let request = graphql::Request::builder()
+        .query(r#"{ topProducts { upc name reviews {id product { name } author { id name } } } }"#)
+        .variables(Arc::new(
+            vec![
+                ("topProductsFirst".into(), 2.into()),
+                ("reviewsForAuthorAuthorId".into(), 1.into()),
+            ]
+            .into_iter()
+            .collect(),
+        ))
+        .build();
+
+    let expected_service_hits = hashmap! {
+        "products".to_string()=>2,
+        "reviews".to_string()=>1,
+        "accounts".to_string()=>1,
+    };
+
+    let http_request =
+        http_compat::RequestBuilder::new(Method::POST, Uri::from_str("http://test").unwrap())
+            .body(request)
+            .unwrap();
+
+    let request = graphql::RouterRequest {
+        context: graphql::Context::new().with_request(http_request),
+    };
+
+    let (actual, registry) = query_rust(request).await;
+
+    assert_eq!(0, actual.errors.len());
+    assert_eq!(registry.totals(), expected_service_hits);
+}
+
 #[tokio::test]
 async fn service_errors_should_be_propagated() {
     let expected_error =apollo_router_core::Error {
@@ -273,6 +308,54 @@ async fn mutation_should_not_work_over_get() {
     assert_eq!(registry.totals(), expected_service_hits);
 }
 
+#[tokio::test]
+async fn mutation_should_work_over_post() {
+    let request = graphql::Request::builder()
+        .query(
+            r#"mutation {
+                createProduct(upc:"8", name:"Bob") {
+                  upc
+                  name
+                  reviews {
+                    body
+                  }
+                }
+                createReview(upc: "8", id:"100", body: "Bif"){
+                  id
+                  body
+                }
+              }"#,
+        )
+        .variables(Arc::new(
+            vec![
+                ("topProductsFirst".into(), 2.into()),
+                ("reviewsForAuthorAuthorId".into(), 1.into()),
+            ]
+            .into_iter()
+            .collect(),
+        ))
+        .build();
+
+    let expected_service_hits = hashmap! {
+        "products".to_string()=>1,
+        "reviews".to_string()=>2,
+    };
+
+    let http_request =
+        http_compat::RequestBuilder::new(Method::POST, Uri::from_str("http://test").unwrap())
+            .body(request)
+            .unwrap();
+
+    let request = graphql::RouterRequest {
+        context: graphql::Context::new().with_request(http_request),
+    };
+
+    let (actual, registry) = query_rust(request).await;
+
+    assert_eq!(0, actual.errors.len());
+    assert_eq!(registry.totals(), expected_service_hits);
+}
+
 #[tokio::test]
 async fn automated_persisted_queries() {
     let (router, registry) = setup_router_and_registry().await;