Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changesets/fix_aaron_execution_request_builder_visibility.md
Original file line number Diff line number Diff line change
@@ -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
7 changes: 6 additions & 1 deletion apollo-router/src/plugins/subscription/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Response>,
pub(crate) subscription_handle: SubscriptionHandle,
pub(crate) subscription_config: SubscriptionConfig,
Expand Down
19 changes: 17 additions & 2 deletions apollo-router/src/services/execution.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<http::Request<graphql::Request>>,
Expand Down