Conversation
Summary of ChangesHello @ardatan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the documentation for the Hive Router, specifically by adding a comprehensive guide on its native plugin system. The new guide empowers developers to extend the router's functionality using custom Rust plugins, covering everything from initial setup to advanced concepts like lifecycle hooks, context sharing, and configuration. This addition significantly improves the extensibility and customizability of the router for users. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📚 Storybook DeploymentThe latest changes are available as preview in: https://pr-7387.hive-storybook.pages.dev |
There was a problem hiding this comment.
Code Review
This pull request introduces a new comprehensive documentation guide titled "Extending the Router," detailing how to create and configure custom Rust plugins for the Hive Router. The guide covers development setup, the router's plugin lifecycle hooks (e.g., on_http_request, on_graphql_params, on_execute, on_subgraph_execute), methods for short-circuiting responses, overriding default behavior, sharing context data between hooks, and configuring/registering plugins. Additionally, the PR includes minor version updates for hive-apollo-router-plugin and hive-console-sdk in Cargo.lock. Review comments primarily focus on enhancing the new documentation's quality by addressing inconsistent heading levels, correcting typos (e.g., an incorrect Rust version, "seperated"), fixing grammatical errors (e.g., "different the end," "different than"), rectifying comment syntax, and critically, recommending the replacement of unwrap() calls with graceful error handling in code examples to promote safer coding practices.
packages/web/docs/src/content/router/guides/extending-the-router/index.mdx
Outdated
Show resolved
Hide resolved
|
|
||
| ### Development Setup | ||
|
|
||
| ## Create a new Rust project |
There was a problem hiding this comment.
The heading level is inconsistent with the document structure. This heading is h2 (##), but it's a subsection of ### Development Setup (h3). It should be h3 (###) to maintain the correct hierarchy. This also applies to the other sub-headings in this section on lines 56, 80, and 88.
### Create a new Rust project
| ## Create a new Rust project | ||
|
|
||
| First, ensure you have the necessary development environment set up for | ||
| [Rust 1.91.1 or later](https://rust-lang.org/tools/install/). Then, you need to create a new Rust |
| /// This is where you can register your custom plugins | ||
| let plugin_registry = PluginRegistry::new(); | ||
| /// Start the Hive Router with the plugin registry |
There was a problem hiding this comment.
In Rust, /// is used for documentation comments that are part of the public API documentation. Inside a function body, regular comments // are more appropriate.
// This is where you can register your custom plugins
let plugin_registry = PluginRegistry::new();
// Start the Hive Router with the plugin registry
| parameters body expected by GraphQL-over-HTTP spec. But we still need to parse the operation into | ||
| AST. | ||
|
|
||
| On the start of this hooks, you can do the following things for example; |
|
|
||
| On the end of this hook, you can do the following things for example; | ||
|
|
||
| - Prevent certain operations from being executed by checked the HTTP headers or other request |
| ready along with the coerced variables. So you can block the operation, manipulate the result, | ||
| variables, etc. | ||
|
|
||
| This is different the end of `on_query_plan` hook because we don't have all the parameters ready |
| variables, etc. | ||
|
|
||
| This is different the end of `on_query_plan` hook because we don't have all the parameters ready | ||
| like coerced variables, filling the introspection fields that are seperated from the actual planning |
| But we still don't have the actual "HTTP" request that would be sent to the subgraph. So this is | ||
| different than `on_subgraph_http_request` hook. So this is before the serialization to HTTP request. |
| let new_header_value = format!("Hello {}", context_data_entry.incoming_data); | ||
| payload.execution_request.headers.insert( | ||
| "x-hello", | ||
| http::HeaderValue::from_str(&new_header_value).unwrap(), |
There was a problem hiding this comment.
💻 Website PreviewThe latest changes are available as preview in: https://pr-7387.hive-landing-page.pages.dev |
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tag: |
|
🐋 This PR was built and pushed to the following Docker images: Targets: Platforms: Image Tag: |
| ### Create a new Rust project | ||
|
|
||
| First, ensure you have the necessary development environment set up for | ||
| [Rust 1.91.1 or later](https://rust-lang.org/tools/install/). Then, you need to create a new Rust |
There was a problem hiding this comment.
How did we decide 1.91.1, I mean, we don't have an official MSRV defined for the Crate (maybe we should?)
|
|
||
| ```toml | ||
| [dependencies] | ||
| hive-router = "0.17" |
There was a problem hiding this comment.
ntex dependency is missing here, and it needs to be added because we don't expose it. The main example below assumes that you have ntex and then it fails.
| You can use our example supergraph as a starting point; | ||
|
|
||
| ```bash | ||
| curl -sSL https://federation-demo.theguild.workers.dev/supergraph.graphql > supergraph.graphql |
There was a problem hiding this comment.
What about subgraphs here? Are they already running somewhere? add a note please
| curl -sSL https://federation-demo.theguild.workers.dev/supergraph.graphql > supergraph.graphql | ||
| ``` | ||
|
|
||
| Then point to that supergraph in your `router.config.yaml`: |
There was a problem hiding this comment.
Create a test/development config file to help you to easily run the router, call it router.config.yaml and place it in the root of the project:
| path: ./supergraph.graphql | ||
| ``` | ||
|
|
||
| Or you can use other ways to provide the supergraph, see |
There was a problem hiding this comment.
I think this is not really needed here, if someone already landed in the custom plugin system, they already know the docs and the fact they can use different sources.
| use hive_router::{PluginRegistry, router_entrypoint, BoxError}; | ||
|
|
||
| #[ntex::main] | ||
| async fn main() -> Result<(), BoxError> { |
There was a problem hiding this comment.
BoxError -> RouterInitError
Also missing import for RouterInitError
| ```rust | ||
| use hive_router::{PluginRegistry, router_entrypoint, BoxError}; | ||
|
|
||
| #[ntex::main] |
There was a problem hiding this comment.
In Hive Router, we are using a custom allocator, and users needs to know that and use it as well, otherwise it might cause unexpected runtime issues.
| Install `hive-router` as a dependency by adding it to your `Cargo.toml` file: | ||
|
|
||
| ```toml | ||
| [dependencies] |
There was a problem hiding this comment.
I'm wondering about what needs to be exported from hive-router crate in order to make it easy for users.
We already know that users needs 2 crucial things at the entry point, in order to be able to just create the first main.rs:
- ntex (v2)
- mimalloc
Should we export from hive-router some defaults in order to make it easier for users? I mean, it can be #[hive_router::main] that wraps ntex::main.
And it can be hive_router::Allocator to be used with #[global_allocator] for the sake of simplicty, or if we'll update this at some point in our codebase.
I can imagine that users who are using a custom build won't deal with custom allocator/http server that's used by Hive-Router, and they will not want to deal with changing/upgrading those depedencies.
| cargo run | ||
| ``` | ||
|
|
||
| ### Configure your plugins in `router.config.yaml` |
There was a problem hiding this comment.
Before this section, we need a dead simple example of creating struct MyPlugin and then implementing the trait for it.
Then, an example snippet on how to register a plugin in the registry.
Then, an example on how to add configuration to the plugin.
And only then, an example on how to configure the plugin the config file.
There was a problem hiding this comment.
Having a custom plugin involved 3 different parts and it should be very clear for the user that they need all 3:
- Create the plugin (actual implementation)
- Register the plugin (to make it available for the router to load)
- Enable the plugin and configure it
| header: 'x-client-id' | ||
| ``` | ||
|
|
||
| ## Registration of Plugins |
There was a problem hiding this comment.
Also, we need to clearly mention that registering a plugin doesn't mean it's activated.
Consider the following case: a plugin is registered but doesn't have configuration. How will it be enabled in the config file?
| specific operation name. So basically this plugin rejects anonymous operations. See the highlighted | ||
| code below; | ||
|
|
||
| ```rust filename="src/forbid_anon_ops.rs" {33} |
There was a problem hiding this comment.
While this example is great, it should be simpler. I would try to avoid copying a full plugin code here, and minimize the snippet so it will focus on the end_with_response call and how/when to use it.
|
|
||
| #[ntex::main] | ||
| async fn main() -> Result<(), BoxError> { | ||
| /// This is where you can register your custom plugins |
There was a problem hiding this comment.
missing init_rustls_crypto_provider
it fails with
Call CryptoProvider::install_default() before this point to select a provider manually, or make sure exactly one of the 'aws-lc-rs' and 'ring' features is enabled.
| - `get_mut<T>(&mut self) -> Option<&mut T>` - Retrieves a mutable reference to the data of type `T` | ||
| from the context. | ||
|
|
||
| ## Refresh State on Supergraph Reload |
There was a problem hiding this comment.
This section needs to be written differently in my opinion:
First, it needs to explain the lifecycle and the fact that a supergraph can change in background.
Then, show the hook and how to respond to it if needed.
And then a seperate section called Depending on Supergraph in your plugin state that explains the usage of ArcSwap and how it can help to avoid these syncing issues
|
|
||
| ```toml | ||
| [dependencies] | ||
| hive-router = "0.17" |
There was a problem hiding this comment.
Also, after a very short while, users need serde and Deserializable for the config. It's not mentioned anywhere on how to install it.
There was a problem hiding this comment.
Actually Kamil and I discussed about it before, we will probably re-export things to avoid users to add extra dependencies
There was a problem hiding this comment.
And it's not only to reduce the amount of dependencies, it's also to make it simpler for users to build plugins without maintaining the dependencies, or even knowing most of them.
There was a problem hiding this comment.
Avoiding extra dependencies is part of making it simpler
0172d0d to
2abb3f5
Compare
22ea8ca to
f7c919f
Compare
Documentation > graphql-hive/console#7387 Preview of the docs -> https://pr-7387.hive-landing-page.pages.dev/docs/router/guides/extending-the-router Example plugins -> https://github.com/graphql-hive/router/tree/plugin-system/plugin_examples TODO: - Document why API is implemented in this way and that way (payload.initialize_plugins, OnPluginInitResult type e.g.) - Documentation updates
Documentation of graphql-hive/router#482