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 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>,