diff --git a/apollo-router/src/plugins/response_cache/plugin.rs b/apollo-router/src/plugins/response_cache/plugin.rs index fcf24575da..f92afcd15f 100644 --- a/apollo-router/src/plugins/response_cache/plugin.rs +++ b/apollo-router/src/plugins/response_cache/plugin.rs @@ -1504,41 +1504,39 @@ async fn cache_lookup_entities( .iter() .map(|k| k.cache_key.as_str()) .collect::>(); - let cache_result = cache.fetch_multiple(&cache_keys, &name).await; Span::current().set_span_dyn_attribute( "cache.keys".into(), opentelemetry::Value::Array(Array::String( cache_keys - .into_iter() + .iter() .map(|ck| StringValue::from(ck.to_string())) .collect(), )), ); - if cache_control.is_some_and(|c| c.is_no_cache()) { - // skip cache lookup if no-cache is set - we have no means of revalidating entries without - // just performing the query, so there's no benefit to hitting the cache - return Ok(ControlFlow::Continue(( - request, - ResponseCacheResults::default(), - ))); - } + // When no-cache is set, skip using any cached values: treat every entity as a cache miss + // so that all representations are fetched fresh from the subgraph. We still build the + // IntermediateResult list (all with cache_entry = None) so that insert_entities_in_result + // can properly assemble the response in the correct order. + let cache_result: Vec> = if cache_control.is_some_and(|c| c.is_no_cache()) { + std::iter::repeat_n(None, keys_len).collect() + } else { + match cache.fetch_multiple(&cache_keys, &name).await { + Ok(res) => res + .into_iter() + .map(|v| match v { + Some(v) if v.control.can_use() => Some(v), + _ => None, + }) + .collect(), + Err(err) => { + if !err.is_row_not_found() { + let span = Span::current(); + span.mark_as_error(format!("cannot get cache entry: {err}")); + } - let cache_result: Vec> = match cache_result { - Ok(res) => res - .into_iter() - .map(|v| match v { - Some(v) if v.control.can_use() => Some(v), - _ => None, - }) - .collect(), - Err(err) => { - if !err.is_row_not_found() { - let span = Span::current(); - span.mark_as_error(format!("cannot get cache entry: {err}")); + std::iter::repeat_n(None, keys_len).collect() } - - std::iter::repeat_n(None, keys_len).collect() } }; let body = request.subgraph_request.body_mut(); diff --git a/apollo-router/src/plugins/response_cache/tests.rs b/apollo-router/src/plugins/response_cache/tests.rs index 47cea496d4..fb1ecc9312 100644 --- a/apollo-router/src/plugins/response_cache/tests.rs +++ b/apollo-router/src/plugins/response_cache/tests.rs @@ -1482,6 +1482,144 @@ async fn no_store_from_request() { assert_eq!(invalidations_by_subgraph.into_values().sum::(), 0); } +// Regression test for ROUTER-1689: +// When `cache-control: no-cache` is sent by the client and response_cache is enabled, +// entity fields resolved via `_entities` queries must not be discarded. +// Previously, the no-cache fast-path returned an empty IntermediateResult list, +// causing insert_entities_in_result to produce `_entities: []` and entity fields to be null. +#[tokio::test] +async fn no_cache_from_request() { + let valid_schema = Arc::new(Schema::parse_and_validate(SCHEMA, "test.graphql").unwrap()); + let query = "query { currentUser { activeOrganization { id creatorUser { __typename id } } } }"; + + let subgraphs = serde_json::json!({ + "user": { + "query": { + "currentUser": { + "activeOrganization": { + "__typename": "Organization", + "id": "1", + } + } + } + }, + "orga": { + "entities": [ + { + "__typename": "Organization", + "id": "1", + "creatorUser": { + "__typename": "User", + "id": 2 + } + } + ] + }, + }); + + let (drop_tx, drop_rx) = tokio::sync::broadcast::channel(2); + let storage = Storage::new(&Config::test(false, &Uuid::new_v4().to_string()), drop_rx) + .await + .unwrap(); + let response_cache = ResponseCache::for_test( + storage.clone(), + Default::default(), + valid_schema.clone(), + false, + drop_tx, + ) + .await + .unwrap(); + + // Phase 1: Warm up the cache with a normal request (no cache-control header) + let service = TestHarness::builder() + .configuration_json(serde_json::json!({"include_subgraph_errors": { "all": true }, "experimental_mock_subgraphs": subgraphs.clone(), "headers": { + "all": { + "request": [{ + "propagate": { + "named": "cache-control" + } + }] + } + } })) + .unwrap() + .schema(SCHEMA) + .extra_private_plugin(response_cache.clone()) + .build_supergraph() + .await + .unwrap(); + + let request = supergraph::Request::fake_builder() + .query(query) + .context(Context::new()) + .build() + .unwrap(); + let mut response = service.oneshot(request).await.unwrap(); + let response = response.next_response().await.unwrap(); + + // Sanity-check: normal request returns entity data + insta::assert_json_snapshot!(response, @r#" + { + "data": { + "currentUser": { + "activeOrganization": { + "id": "1", + "creatorUser": { + "__typename": "User", + "id": 2 + } + } + } + } + } + "#); + + // Phase 2: Request with `no-cache` — cache must be bypassed for lookup but entity data + // from the subgraph must still be returned correctly (regression for ROUTER-1689). + let service = TestHarness::builder() + .configuration_json(serde_json::json!({"include_subgraph_errors": { "all": true }, "experimental_mock_subgraphs": subgraphs.clone(), "headers": { + "all": { + "request": [{ + "propagate": { + "named": "cache-control" + } + }] + } + } })) + .unwrap() + .schema(SCHEMA) + .extra_private_plugin(response_cache.clone()) + .build_supergraph() + .await + .unwrap(); + + let request = supergraph::Request::fake_builder() + .query(query) + .context(Context::new()) + .header(CACHE_CONTROL, HeaderValue::from_static("no-cache")) + .build() + .unwrap(); + let mut response = service.oneshot(request).await.unwrap(); + let response = response.next_response().await.unwrap(); + + // Entity fields must NOT be null — this was the regression + insta::assert_json_snapshot!(response, @r#" + { + "data": { + "currentUser": { + "activeOrganization": { + "id": "1", + "creatorUser": { + "__typename": "User", + "id": 2 + } + } + } + } + } + "#); +} + #[tokio::test] async fn private_only() { async {