Add TestHarness, and assorted API breaking changes by SimonSapin · Pull Request #1487 · apollographql/router

SimonSapin · 2022-08-09T17:11:38Z

This PR makes a number of changes, many of them breaking. Each commit should be self-contained, though. The highlights are:

A new TestHarness API that replaces both PluginTestHarness (which is removed) and PluggableRouterServiceBuilder (which becomes private)
The above allows a number of other items to be made private, as they are no longer needed outside of the crate or appear in public signatures
The services module was also made private. It is replaced in the public API by a new stages module that contains sub-modules for each pipeline stage (query_planner, execution, etc) which in turn contain items like Request, BoxService, etc.
Some other drive-by API changes, for polish towards 1.0

For more details, see NEXT_CHANGELOG.md, apollo-router/src/test_harness.rs, and apollo-router/src/stages.rs. Much of the other changes are very mechanical.

Many choices proposed in this PR are opinionated but everything is open to discussion.

It should probably have been multiple smaller PRs. I got a bit carried away, sorry. For review I suggest going commit-by-commit.

SimonSapin · 2022-08-09T17:16:21Z

For now CI will fail for (at least) two reasons:

Some tracing test fails. This failure will go away once this PR is rebased on top of remove telemetry configuration hot reloading #1463
A warning in apollo-router/src/plugins/telemetry/config.rs because MetricsCommon::delay_interval is never used. The warning is new because the struct and field used to be public, but it’s legitimate. Should we remove this configuration since it does nothing? Remove delay_interval in telemetry configuration #1498

o0Ignition0o · 2022-08-11T10:34:33Z

Not sure if it's in the scope of this pr, but it would be great if we tested our plugins against a yaml! configuration instead of

        let config = serde_json::json!({
            "plugins": {
                "my_example.catch_query_planner_error": {
                    "enabled": true ,
                }
            }
        });
        apollo_router::TestHarness::builder()
            .configuration_json(config)
            .unwrap()
            .build()
            .await
            .unwrap();
    }

This would allow us to catch yml related issues earlier

o0Ignition0o · 2022-08-11T10:54:51Z

                    let query_planner_response: Result<query_planner::Response, BoxError> =
                        query_planner_future.await;

@SimonSapin you spoiled me! I was expecting query_planner::Result to be available :D

o0Ignition0o · 2022-08-11T10:56:25Z

Any chance we can fix the requirement of an alias while we're at it ?

use apollo_router::layers::ServiceExt;
use tower::{ServiceExt as _};

SimonSapin · 2022-08-11T10:59:21Z

We could definitely add Result aliases!

Renaming ServiceExt too, but this PR is already big, let's do additional changes separately 😅

SimonSapin · 2022-08-11T11:02:58Z

We could have a convenience builder method for TestHarness that accepts YAML, but it would have to be as a &str (so typically in a Rust string literal without syntax highlighting). serde_yaml doesn't have a macro like serde_json::json! because YAML syntax is too different from Rust syntax. json! actually takes Rust tokens as inputs, I'm sure there are edge cases where the syntax is not quite the same. It just looks close enough.

The `Router` type already implements `Service` and `Clone`.

This uses BrynCooke/buildstructor#70

…ublic API

Oops

They alias Result<Response, BoxError>

o0Ignition0o

LGTM!

I'd like to take a moment to acknowledge the HUGE amount of work you had to do to come up with such a big, and so well documented PR.

This one touches everything, and the same level of care and attention to detail has been given to each file, and each function.

I know I have suffered review fatigue while reading this, so I can't imagine the amount of work and concentration preparing such a changeset required. And I won't even mention the number of rebases and merge conflicts you had to go through.

Again kudos for the very nice modules and functions documentation. This is really great work!

o0Ignition0o · 2022-08-12T14:49:26Z

NEXT_CHANGELOG.md

tremendous effort on the changelog, that's really GREAT WORK 🎉

NIT: you can now update the PR #

o0Ignition0o · 2022-08-12T14:56:45Z

apollo-router/src/executable.rs

Interesting!

o0Ignition0o · 2022-08-12T14:57:56Z

apollo-router/src/layers/mod.rs

great indent catch!

o0Ignition0o · 2022-08-12T14:59:27Z

apollo-router/src/plugin/mod.rs

o0Ignition0o · 2022-08-12T15:05:47Z

apollo-router/src/plugin/test/service.rs

oh that s a leftover from the apollo-router apollo-router-core split!

o0Ignition0o · 2022-08-12T15:46:53Z

apollo-router/src/plugins/csrf.rs

o0Ignition0o · 2022-08-12T15:56:23Z

apollo-router/src/plugin/test/mod.rs

 //! Utilities which make it easy to test with [`crate::plugin`].

 mod mock;
+#[macro_use]


why do we need this ? 🤔

Because we removed #[macro_export] elsewhere

o0Ignition0o · 2022-08-12T15:56:42Z

apollo-router/src/test_harness.rs

+/// On the subgraph side, this test harness never makes network requests to subgraphs
+/// unless [`with_subgraph_network_requests`][Self::with_subgraph_network_requests] is called.


o0Ignition0o · 2022-08-12T16:08:23Z

examples/cookies-to-headers/src/main.rs

+                        *body = stream
+                            .map(move |mut resp| {
+                                resp.data = mock_data.clone();
+                                resp
+                            })
+                            .boxed();


is this why we need futures::StreamExt?

This feels a bit inconvenient, I m not sure I would assume plugin writers to be able to wield this, especially when learning rust :/

Yes, this was an ugly way to get this test to compile. I’ve added a helper in #1525

o0Ignition0o · 2022-08-12T16:18:24Z

apollo-router/src/router.rs

+    /// Returns a builder to start an HTTP server in a separate Tokio task.
+    ///
+    /// Builder methods:
+    ///
+    /// * `.schema(impl Into<`[`SchemaSource`]`>)`
+    ///   Required.
+    ///   Specifies where to find the supergraph schema definition.
+    ///   Some sources support hot-reloading.
+    ///
+    /// * `.configuration(impl Into<`[`ConfigurationSource`]`>)`
+    ///   Optional.
+    ///   Specifies where to find the router configuration.
+    ///   If not provided, the default configuration as with an empty YAML file.
+    ///
+    /// * `.shutdown(impl Into<`[`ShutdownSource`]`>)`
+    ///   Optional.
+    ///   Specifies when the server should gracefully shut down.
+    ///   If not provided, the default is [`ShutdownSource::CtrlC`].
+    ///
+    /// * `.start()`
+    ///   Finishes the builder,
+    ///   starts an HTTP server in a separate Tokio task,
+    ///   and returns a `RouterHttpServer` handle.
+    ///
+    /// The server handle can be used in multiple ways.
+    /// As a [`Future`], it resolves to `Result<(), `[`ApolloRouterError`]`>`
+    /// either when the server has finished gracefully shutting down
+    /// or when it encounters a fatal error that prevents it from starting.
+    ///
+    /// If the handle is dropped before being awaited as a future,
+    /// a graceful shutdown is triggered.
+    /// In order to wait until shutdown finishes,
+    /// use the [`shutdown`][Self::shutdown] method instead.


o0Ignition0o · 2022-08-12T16:36:39Z

@SimonSapin I didn't see you enabled automerge, let's followup on the comment's I ve written next week!

Follows up on #1487 (comment) Related to #1219

SimonSapin added the api/1.0 label Aug 9, 2022

SimonSapin assigned Geal, BrynCooke, abernix, bnjjj and o0Ignition0o Aug 9, 2022

This was referenced Aug 11, 2022

API 1.0 - Public API audit #1150

Closed

Error handling audit #81

Closed

API 1.0 - PluginTestHarness is not good enough #1149

Open

SimonSapin force-pushed the simon/test-harness branch 2 times, most recently from 501574b to fe7b712 Compare August 12, 2022 12:57

SimonSapin marked this pull request as ready for review August 12, 2022 12:57

SimonSapin requested a review from StephenBarlow as a code owner August 12, 2022 12:57

SimonSapin requested review from BrynCooke, Geal and o0Ignition0o and removed request for StephenBarlow August 12, 2022 13:03

SimonSapin added 7 commits August 12, 2022 16:45

Remove no-op use of make_service on axum’s Router

f3601a0

The `Router` type already implements `Service` and `Clone`.

Make constructors private when there’s a public builder

a66a665

This uses BrynCooke/buildstructor#70

Rename enums *Kind -> *Source

4ee35e7

Rename ApolloRouter to RouterHttpServer

194ec7c

Merge RouterHandle into RouterHttpServer

53dbad6

Replace router_builder_fn() with shutdown()

4f10bc0

Add RouterHttpServer::shutdown

772da4f

SimonSapin added 20 commits August 12, 2022 16:46

Remove deprecated type aliases

6e26603

RouterRequest::fake_builder defaults to Content-Type: application/json

64e54bb

Add apollo_router::TestHarness

472e315

Port rhai_tests to the new TestHarness

8b81aa5

Port integration_tests to the new TestHarness

5c65ca8

Port apollo-router-benchmarks to the new TestHarness

537979c

Port examples/embedded to the new TestHarness

7dae42c

Make some things private

f4b54a9

Add apollo_router::stages, replacing apollo_router::services in the p…

889326c

…ublic API

Make more things private

e559002

Add ad-hoc plugins with callbacks in TestHarness

9d65cf0

Actually implement TestHarness::with_subgraph_network_requests

f60586c

Oops

Add TestHarness::configuration_json

909c56f

Port apollo-router-scaffold to the new TestHarness

2c6b5d9

Port remaining examples/* to the new TestHarness

33326e7

Port telemetry::metrics::apollo to the new test harness

0a79e20

Remove PluginTestHarness, as it was replaced by TestHarness

3c17878

Make Schema actually private

d430813

Make plugins private

3499374

Add apollo_router::stages::*::Result aliases

56690db

They alias Result<Response, BoxError>

SimonSapin force-pushed the simon/test-harness branch from 88790c6 to 56690db Compare August 12, 2022 15:52

SimonSapin enabled auto-merge August 12, 2022 15:55

o0Ignition0o approved these changes Aug 12, 2022

View reviewed changes

SimonSapin merged commit 622d946 into main Aug 12, 2022

SimonSapin deleted the simon/test-harness branch August 12, 2022 16:22

SimonSapin added a commit that referenced this pull request Aug 17, 2022

Add {Router,Execution}Response::map_stream helper

08a7724

Follows up on #1487 (comment) Related to #1219

SimonSapin mentioned this pull request Aug 17, 2022

Add {Router,Execution}Response::map_stream helper #1525

Merged

SimonSapin added a commit that referenced this pull request Aug 17, 2022

Add {Router,Execution}Response::map_stream helper (#1525)

b4ebe6b

Follows up on #1487 (comment) Related to #1219

Geal mentioned this pull request Aug 22, 2022

release: v0.16.0 #1566

Merged

		/// On the subgraph side, this test harness never makes network requests to subgraphs
		/// unless [`with_subgraph_network_requests`][Self::with_subgraph_network_requests] is called.

Conversation

SimonSapin commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SimonSapin commented Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

o0Ignition0o commented Aug 11, 2022

Uh oh!

o0Ignition0o commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

o0Ignition0o commented Aug 11, 2022

Uh oh!

SimonSapin commented Aug 11, 2022

Uh oh!

SimonSapin commented Aug 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

o0Ignition0o left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

o0Ignition0o commented Aug 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SimonSapin commented Aug 9, 2022 •

edited

Loading

SimonSapin commented Aug 9, 2022 •

edited

Loading

o0Ignition0o commented Aug 11, 2022 •

edited

Loading

SimonSapin commented Aug 11, 2022 •

edited

Loading