Entity cache fix: update the cache key with private info on the first call

.changesets/fix_geal_test_private_info_caching.md

@@ -0,0 +1,5 @@
+### Entity cache fix: update the cache key with private info on the first call ([PR #5599](https://github.com/apollographql/router/pull/5599))
+
+This adds a test for private information caching and fixes an issue where private data was stored at the wrong key, so it did not appear to be cached
+
+By [@Geal](https://github.com/Geal) in https://github.com/apollographql/router/pull/5599

apollo-router/src/plugins/cache/entity.rs

@@ -590,11 +590,13 @@ impl InnerCacheService {
                             // we did not know in advance that this was a query with a private scope, so we update the cache key
                             if !is_known_private {
                                 self.private_queries.write().await.insert(query.to_string());
+
+                                if let Some(s) = private_id.as_ref() {
+                                    root_cache_key = format!("{root_cache_key}:{s}");
+                                }
                             }
 
-                            if let Some(s) = private_id.as_ref() {
-                                root_cache_key = format!("{root_cache_key}:{s}");
-                            } else {
+                            if private_id.is_none() {
                                 // the response has a private scope but we don't have a way to differentiate users, so we do not store the response in cache
                                 return Ok(response);
                             }

apollo-router/tests/common.rs

@@ -491,6 +491,7 @@ impl IntegrationTest {
         self.execute_query_internal(
             &json!({"query":"query {topProducts{name}}","variables":{}}),
             None,
+            None,
         )
     }
 
@@ -499,34 +500,44 @@ impl IntegrationTest {
         &self,
         query: &Value,
     ) -> impl std::future::Future<Output = (TraceId, reqwest::Response)> {
-        self.execute_query_internal(query, None)
+        self.execute_query_internal(query, None, None)
     }
 
     #[allow(dead_code)]
     pub fn execute_bad_query(
         &self,
     ) -> impl std::future::Future<Output = (TraceId, reqwest::Response)> {
-        self.execute_query_internal(&json!({"garbage":{}}), None)
+        self.execute_query_internal(&json!({"garbage":{}}), None, None)
     }
 
     #[allow(dead_code)]
     pub fn execute_huge_query(
         &self,
     ) -> impl std::future::Future<Output = (TraceId, reqwest::Response)> {
-        self.execute_query_internal(&json!({"query":"query {topProducts{name, name, name, name, name, name, name, name, name, name}}","variables":{}}), None)
+        self.execute_query_internal(&json!({"query":"query {topProducts{name, name, name, name, name, name, name, name, name, name}}","variables":{}}), None, None)
     }
 
     #[allow(dead_code)]
     pub fn execute_bad_content_type(
         &self,
     ) -> impl std::future::Future<Output = (TraceId, reqwest::Response)> {
-        self.execute_query_internal(&json!({"garbage":{}}), Some("garbage"))
+        self.execute_query_internal(&json!({"garbage":{}}), Some("garbage"), None)
+    }
+
+    #[allow(dead_code)]
+    pub fn execute_query_with_headers(
+        &self,
+        query: &Value,
+        headers: HashMap<String, String>,
+    ) -> impl std::future::Future<Output = (TraceId, reqwest::Response)> {
+        self.execute_query_internal(query, None, Some(headers))
     }
 
     fn execute_query_internal(
         &self,
         query: &Value,
         content_type: Option<&'static str>,
+        headers: Option<HashMap<String, String>>,
     ) -> impl std::future::Future<Output = (TraceId, reqwest::Response)> {
         assert!(
             self.router.is_some(),
@@ -544,7 +555,7 @@ impl IntegrationTest {
             async move {
                 let client = reqwest::Client::new();
 
-                let mut request = client
+                let mut builder = client
                     .post(url)
                     .header(
                         CONTENT_TYPE,
@@ -553,10 +564,15 @@ impl IntegrationTest {
                     .header("apollographql-client-name", "custom_name")
                     .header("apollographql-client-version", "1.0")
                     .header("x-my-header", "test")
-                    .header("head", "test")
-                    .json(&query)
-                    .build()
-                    .unwrap();
+                    .header("head", "test");
+
+                if let Some(headers) = headers {
+                    for (name, value) in headers {
+                        builder = builder.header(name, value);
+                    }
+                }
+
+                let mut request = builder.json(&query).build().unwrap();
                 telemetry.inject_context(&mut request);
                 dbg!(&request.headers());
                 request.headers_mut().remove(ACCEPT);

apollo-router/tests/samples/enterprise/entity-cache/private/README.md

@@ -0,0 +1,3 @@
+# Entity cache: private data caching
+
+This tests private data caching in the entity cache:

apollo-router/tests/samples/enterprise/entity-cache/private/configuration.yaml

@@ -0,0 +1,21 @@
+override_subgraph_url:
+  products: http://localhost:4005
+include_subgraph_errors:
+  all: true
+
+rhai:
+  scripts: "tests/samples/enterprise/entity-cache/private"
+  main: "private.rhai"
+
+preview_entity_cache:
+  enabled: true
+  redis:
+    urls:
+      ["redis://localhost:6379",]
+  subgraph:
+    all:
+      enabled: true
+      ttl: 10s
+    subgraphs:
+      accounts:
+        private_id: "user"

apollo-router/tests/samples/enterprise/entity-cache/private/plan.json

@@ -0,0 +1,125 @@
+{
+    "enterprise": true,
+    "redis": true,
+    "actions": [
+        {
+            "type": "Start",
+            "schema_path": "./supergraph.graphql",
+            "configuration_path": "./configuration.yaml",
+            "subgraphs": {
+                "accounts": {
+                    "requests": [
+                        {
+                            "request": {
+                                "body": {"query":"query private__accounts__0{me{name}}"}
+                            },
+                            "response": {
+                                "headers": {
+                                    "Cache-Control": "private, max-age=10",
+                                    "Content-Type": "application/json"
+                                },
+                                "body": {"data": { "me": { "name": "test" } } }
+                            }
+                        }
+                    ]
+                }
+            }
+        },
+        {
+            "type": "Request",
+            "request": {
+                "query": "query private { me { name } }"
+            },
+            "headers": {
+                "x-user": "1"
+            },
+            "expected_response": {
+                "data":{
+                    "me":{
+                        "name":"test"
+                    }
+                }
+            }
+        },
+        {
+            "type": "ReloadSubgraphs",
+            "subgraphs": {
+                "accounts": {
+                    "requests": [
+                        {
+                            "request": {
+                                "body": {"query":"query private__accounts__0{me{name}}"}
+                            },
+                            "response": {
+                                "headers": {
+                                    "Cache-Control": "private, max-age=10",
+                                    "Content-Type": "application/json"
+                                },
+                                "body": {"data": { "me": { "name": "test2" } } }
+                            }
+                        }
+                    ]
+                }
+            }
+        },
+        {
+            "type": "Request",
+            "request": {
+                "query": "query private { me { name } }"
+            },
+            "headers": {
+                "x-user": "2"
+            },
+            "expected_response": {
+                "data":{
+                    "me":{
+                        "name":"test2"
+                    }
+                }
+            }
+        },
+        {
+            "type": "ReloadSubgraphs",
+            "subgraphs": {
+                "accounts": {
+                    "requests": []
+                }
+            }
+        },
+        {
+            "type": "Request",
+            "request": {
+                "query": "query private { me { name } }"
+            },
+            "headers": {
+                "x-user": "1"
+            },
+            "expected_response": {
+                "data":{
+                    "me":{
+                        "name":"test"
+                    }
+                }
+            }
+        },
+        {
+            "type": "Request",
+            "request": {
+                "query": "query private { me { name } }"
+            },
+            "headers": {
+                "x-user": "2"
+            },
+            "expected_response": {
+                "data":{
+                    "me":{
+                        "name":"test2"
+                    }
+                }
+            }
+        },
+        {
+            "type": "Stop"
+        }
+    ]
+}

apollo-router/tests/samples/enterprise/entity-cache/private/private.rhai

@@ -0,0 +1,21 @@
+fn supergraph_service(service) {
+    const request_callback = Fn("process_request");
+    service.map_request(request_callback);
+}
+
+// This will convert all cookie pairs into headers.
+// If you only wish to convert certain cookies, you
+// can add logic to modify the processing.
+fn process_request(request) {
+
+print(`headers: ${request.headers}`);
+    // Find our cookies
+    if "x-user" in request.headers {
+        let user = request.headers["x-user"];
+        print(`found user {user}`);
+
+        request.context["user"] = user;
+    } else {
+        print("no user found");
+    }
+}

apollo-router/tests/samples/enterprise/entity-cache/private/supergraph.graphql

@@ -0,0 +1,91 @@
+
+schema
+  @core(feature: "https://specs.apollo.dev/core/v0.2"),
+  @core(feature: "https://specs.apollo.dev/join/v0.1", for: EXECUTION)
+  @core(feature: "https://specs.apollo.dev/inaccessible/v0.1", for: SECURITY)
+{
+  query: Query
+  mutation: Mutation
+}
+
+directive @core(as: String, feature: String!, for: core__Purpose) repeatable on SCHEMA
+
+directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet) on FIELD_DEFINITION
+
+directive @join__type(graph: join__Graph!, key: join__FieldSet) repeatable on OBJECT | INTERFACE
+
+directive @join__owner(graph: join__Graph!) on OBJECT | INTERFACE
+
+directive @join__graph(name: String!, url: String!) on ENUM_VALUE
+
+directive @tag(name: String!) repeatable on FIELD_DEFINITION | INTERFACE | OBJECT | UNION
+
+directive @inaccessible on OBJECT | FIELD_DEFINITION | INTERFACE | UNION
+
+enum core__Purpose {
+  """
+  `EXECUTION` features provide metadata necessary to for operation execution.
+  """
+  EXECUTION
+
+  """
+  `SECURITY` features provide metadata necessary to securely resolve fields.
+  """
+  SECURITY
+}
+
+scalar join__FieldSet
+
+enum join__Graph {
+  ACCOUNTS @join__graph(name: "accounts", url: "https://accounts.demo.starstuff.dev")
+  INVENTORY @join__graph(name: "inventory", url: "https://inventory.demo.starstuff.dev")
+  PRODUCTS @join__graph(name: "products", url: "https://products.demo.starstuff.dev")
+  REVIEWS @join__graph(name: "reviews", url: "https://reviews.demo.starstuff.dev")
+}
+type Mutation {
+  updateMyAccount: User @join__field(graph: ACCOUNTS)
+  createProduct(name: String, upc: ID!): Product @join__field(graph: PRODUCTS)
+  createReview(body: String, id: ID!, upc: ID!): Review @join__field(graph: REVIEWS)
+}
+
+type Product
+  @join__owner(graph: PRODUCTS)
+  @join__type(graph: PRODUCTS, key: "upc")
+  @join__type(graph: INVENTORY, key: "upc")
+  @join__type(graph: REVIEWS, key: "upc")
+{
+  inStock: Boolean @join__field(graph: INVENTORY) @tag(name: "private") @inaccessible
+  name: String @join__field(graph: PRODUCTS)
+  price: Int @join__field(graph: PRODUCTS)
+  reviews: [Review] @join__field(graph: REVIEWS)
+  reviewsForAuthor(authorID: ID!): [Review] @join__field(graph: REVIEWS)
+  shippingEstimate: Int @join__field(graph: INVENTORY, requires: "price weight")
+  upc: String! @join__field(graph: PRODUCTS)
+  weight: Int @join__field(graph: PRODUCTS)
+}
+
+type Query {
+  me: User @join__field(graph: ACCOUNTS)
+  topProducts(first: Int = 5): [Product] @join__field(graph: PRODUCTS)
+}
+
+type Review
+  @join__owner(graph: REVIEWS)
+  @join__type(graph: REVIEWS, key: "id")
+{
+  author: User @join__field(graph: REVIEWS, provides: "username")
+  body: String @join__field(graph: REVIEWS)
+  id: ID! @join__field(graph: REVIEWS)
+  product: Product @join__field(graph: REVIEWS)
+}
+
+type User
+  @join__owner(graph: ACCOUNTS)
+  @join__type(graph: ACCOUNTS, key: "id")
+  @join__type(graph: REVIEWS, key: "id")
+{
+  id: ID! @join__field(graph: ACCOUNTS)
+  name: String @join__field(graph: ACCOUNTS)
+  reviews: [Review] @join__field(graph: REVIEWS)
+  username: String @join__field(graph: ACCOUNTS)
+}