From 2def300cd90b3596409e994a24436074574f8792 Mon Sep 17 00:00:00 2001 From: Aaron Arinder Date: Wed, 17 Dec 2025 15:39:03 -0500 Subject: [PATCH 1/2] fix(exec::Req): pub(crate) -> pub, fixes public api --- .../src/plugins/subscription/execution.rs | 7 ++++++- apollo-router/src/services/execution.rs | 19 +++++++++++++++++-- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/apollo-router/src/plugins/subscription/execution.rs b/apollo-router/src/plugins/subscription/execution.rs index f9ff577de4..45594e7ee9 100644 --- a/apollo-router/src/plugins/subscription/execution.rs +++ b/apollo-router/src/plugins/subscription/execution.rs @@ -119,7 +119,12 @@ where } } -pub(crate) struct SubscriptionTaskParams { +// WARN: you might be tempted to change this to pub(crate) because its fields are pub(crate) and +// there's no actual use in having it marked pub _apart_ from it being used in the +// execution::Request builder and, more importantly, the fake_builder we get from BuildStructor. +// Customers use that in unit tests for their plugins, and they'll fail to compile if we mark this +// pub(crate) +pub struct SubscriptionTaskParams { pub(crate) client_sender: tokio::sync::mpsc::Sender, pub(crate) subscription_handle: SubscriptionHandle, pub(crate) subscription_config: SubscriptionConfig, diff --git a/apollo-router/src/services/execution.rs b/apollo-router/src/services/execution.rs index bc229b8bf7..f679b5bb81 100644 --- a/apollo-router/src/services/execution.rs +++ b/apollo-router/src/services/execution.rs @@ -79,8 +79,23 @@ impl Request { /// This is the constructor (or builder) to use when constructing a "fake" ExecutionRequest. /// /// This does not enforce the provision of the data that is required for a fully functional - /// ExecutionRequest. It's usually enough for testing, when a fully consructed ExecutionRequest is - /// difficult to construct and not required for the pusposes of the test. + /// ExecutionRequest. It's usually enough for testing, when a fully constructed ExecutionRequest is + /// difficult to construct and not required for the purposes of the test. + /// + /// # Example + /// ``` + /// use apollo_router::services::execution; + /// use apollo_router::Context; + /// + /// // This builder must remain usable from external crates for plugin testing. + /// let request = execution::Request::fake_builder() + /// .context(Context::default()) + /// .build(); + /// ``` + // WARN: make sure to keep the example in the doc test; it gets compiled as an external crate, + // which is very useful in determining whether the public API has changed. In particular, we + // had an issue with SubscriptionTaskParams going from pub to pub(crate), preventing folks from + // compiling their unit tests for plugins because of the visibility change for the fake_builder #[builder(visibility = "pub")] fn fake_new( supergraph_request: Option>, From 94f0874d40f6a6257cc1b7788f913d0165b8602c Mon Sep 17 00:00:00 2001 From: Aaron Arinder Date: Wed, 17 Dec 2025 15:42:15 -0500 Subject: [PATCH 2/2] changeset --- .../fix_aaron_execution_request_builder_visibility.md | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changesets/fix_aaron_execution_request_builder_visibility.md diff --git a/.changesets/fix_aaron_execution_request_builder_visibility.md b/.changesets/fix_aaron_execution_request_builder_visibility.md new file mode 100644 index 0000000000..2202d14670 --- /dev/null +++ b/.changesets/fix_aaron_execution_request_builder_visibility.md @@ -0,0 +1,7 @@ +### Fix visibility of SubscriptionTaskParams + +We had SubscriptionTaskParams set as pub and folks started using a builder that has it as its argument (the fake_bulder off of execution::Request); with the change in visibility, they weren't able to compile unit tests for their plugins + +New docs test (compiled as an external crate so a good test of the public API's visibility) and a couple of WARNs to make sure we don't make that same mistake again + +By [@aaronarinder](https://github.com/aaronarinder) in https://github.com/apollographql/router/pull/8771