First cut query planner calling from rust. #812
Conversation
BrynCooke
force-pushed
the
query-planer-js-bridge
branch
3 times, most recently
from
92a49f9
to
67409c5
Compare
BrynCooke
force-pushed
the
query-planer-js-bridge
branch
from
67409c5
to
10ffdce
Compare
Error handling needs to be improved. Pulled out common code for running Js to a separate module. Note that the existing `harmonize` method has been renamed to `compose::compose` to allow `plan:plan` to be added.
BrynCooke
force-pushed
the
query-planer-js-bridge
branch
from
10ffdce
to
02f2495
Compare
Allows use as a git dependency in cargo.
There was a problem hiding this comment.
The changes here look correct, but my high-level question is if we want to actually have both of these live together in the same published crate.
Currently, harmonizer (which was a play on the term "composition", originally) is used in rover, which only needs composition. [It's published to crates.io as harmonizer. While not enforced yet, in order to reduce the cognitive overhead of "what version of composition am I running?", the version of the harmonizer package was meant to track that of the @apollo/federation package. (e.g., harmonizer@0.2.2 crate = @apollo/federation@0.2.2 npm package)
I'd like to understand the motivation as my initial thought is that we're overloading it and that we should have separate "rust wrapper around composition" and "rust wrapper around query planning", since they are different packages in Node.js-land too and I worry that it would make it more difficult/complex to debug/consume or add cognitive overhead. While we might be duplicating some boilerplate Deno runtime stuff across the two different implementations (e.g., query planning and composition), I don't think we're going to get to the "rule of threes" here and I don't think we want to have more Rust-wrapping-JS packages than these two, so I wouldn't optimize for that.
Finally, since we're not coupling the Rust graph router to the evolution of the query planner, we may want to just further decouple these and just have a one-off packaged version of the current query planner live inside the new router repo, or just have a separate repository that automatically manages publishing these separate Rust crates, as the upstream versions of the underlying JS implementations evolve. e.g., new version of @apollo/federation = automatic new version published of harmonizer and, separately, new version of @apollo/query-planner = automatic new version published of name to be determined.
Thoughts?
| } | ||
|
|
||
| function done(result) { | ||
| Deno.core.opSync('op_composition_result', result); |
There was a problem hiding this comment.
This is a "composition" reference but is triggered through both composition and query planning.
| // need to be initialized as empty objects. | ||
| global = {}; | ||
| exports = {}; | ||
| queryPlanner = {}; |
There was a problem hiding this comment.
Could we put a comment here explaining the queryPlanner empty object requirement here? Is that something that could live in another file? (This file is intended to be some very Node.js-specific runtime requirement stuff rather than application specific code.
There was a problem hiding this comment.
(Further, I don't see where this is used after looking through the rest of the PR.)
There was a problem hiding this comment.
It's something required by the rollup and is not actually used by us directly. I didn't manage to figure out why it was needed, possibly it's something to do with query planner being a class.
|
There wasn't really any motivation to the original PR structure over and above having all the js-bridge code centralized. The rust router project doesn't actually require any published package as it uses a git dependency: Until we are ready to make the switch to to a full Rust query planner lets try and stay using this repo. We don't want to be doing manual syncronization of the repos and unless query planning is guaranteed to stay static. As this is currently not blocking dev on the router side let's leave this unmerged for now with the intention of splitting to a separate submodule. |
How was this even working?!
cecton
force-pushed
the
query-planer-js-bridge
branch
from
c6385dd
to
3450780
Compare
BrynCooke
force-pushed
the
query-planer-js-bridge
branch
from
3450780
to
58afba5
Compare
cecton
force-pushed
the
query-planer-js-bridge
branch
from
58afba5
to
3450780
Compare
BrynCooke
force-pushed
the
query-planer-js-bridge
branch
from
b0b30e8
to
3450780
Compare
…1079) This is a Rust-JS hybrid package that allows us to invoke some existing JavaScript functions via V8 + deno_core from Rust code. It's inspired by the existing harmonizer package, but makes this a real TypeScript package so we don't merely need to leverage JSDoc comments for types (which was fraught) and instead uses the TypeScript compiler as typically expected in this monorepo. The immediate motivation is to expose introspection capabilities via graphql-js to Rust, in order to (as a stop-gap shortcut) as an introspection provider for the Router while we build up the native functionality to do so in Rust via apollo-rs, which will need some time to build that out. A follow-up motivation – for building this as a general purpose router bridge — will be to land the changes of #812 (JS query planning) on in this router-bridge package, rather than in harmonizer. That's desirable since harmonizer is meant to just be composition for Rover. A couple notes as to how this differs from harmonizer at the moment — it will probably be worth applying similar changes to the harmonizer package, but those can be follow-ups to this PR: The JavaScript invocation can now return anything that implements DeserializeOwned, whereas it previously only returned Strings. The runtime.js file remains as just JS as it wasn't happy with assigning things to process and global — which we need to do since we are the runtime environment (and those things are typically provided by Node.js). This work-around definitely suggests there's an inadvertent inclusion of node types, but it didn't seem worth fighting since this (boilerplate) is quite static and doesn't need to change much. In a similar spirit, the tsconfig.json for the router-bridge disables "use strict"; emission. While not normally desirable, this is acceptable because these files still go through rollup (JS) afterward, which adds it anyhow at a higher level. Directories have been adjusted to be slightly more intuitive for a Rust/JS hybrid package. In particular: TypeScript source files are now in js-src/, rather than src/. Rust remains in src/ at the moment, but could theoretically be put in rs-src/ in a later commit. dist/ is now js-dist/ and is now emitted by TypeScript compiler, rather than (somewhat confusingly) the rollup bundler. The rollup-produced bundles are now in bundled! Renamed url_shim to url_polyfill, because that's what it is really! Actually has Jest tests wired up to the root TypeScript project that live in router-bridge. This should help ensure compatibility and act as a force-function to keep things aligned and working as the rest of the federation repository evolves. Lastly, not only is this a language hybrid but this PR is a contribution hybrid — with @o0Ignition0o doing the Rust work and @abernix on the TypeScript. 🎉 Co-authored-by: Jeremy Lempereur jeremy.lempereur@iomentum.com Co-authored-by: Jesse Rosenberger git@jro.cc
fixes #1088 This commit is built on top of @BrynCooke 's work on #812, and exposes a query planning functionality, which has been living in a separate branch for a while. A few things happened: General: - Remove jest tests from the router bridge (they weren't being run anyway, and they are now superseded by the cargo tests) - Add router-bridge js files to the prettier configuration for formatting and linting - Add a dependency to query-planner-js to the router-bridge TypeScript/Javascript: - Add `do_plan.ts` that calls the query-planner behind the scenes. - Use `new QueryPlanner(schema).buildQueryPlan` instead of the old `buildQueryPlan` function, that doesn't exist anymore (depending on how long we want to keep this code, we might be able to keep the QueryPlanner object around, and skip subsequent schema parsing / composing. - Move the type `OperationResult` to a utils file (so we can use it in both `do_plan.ts` and `do_introspect.ts` Rust: - Use &str instead of String for schema SDL, since apollo-rs provides as_str() on the Schema type. - Add a plan function, that is generic over it's return type ( `Result<T, PlanningErrors>` where `T: DeserializeOwned + 'static` so we can keep the `QueryPlan` structure on the router-core side. - Add plan() tests that call plan<serde_json::Value>() so we have snapshots over the plan result.
fixes #1088 This commit is built on top of @BrynCooke 's work on #812, and exposes a query planning functionality, which has been living in a separate branch for a while. A few things happened: General: - Remove jest tests from the router bridge (they weren't being run anyway, and they are now superseded by the cargo tests) - Add router-bridge js files to the prettier configuration for formatting and linting - Add a dependency to query-planner-js to the router-bridge TypeScript/Javascript: - Add `do_plan.ts` that calls the query-planner behind the scenes. - Use `new QueryPlanner(schema).buildQueryPlan` instead of the old `buildQueryPlan` function, that doesn't exist anymore (depending on how long we want to keep this code, we might be able to keep the QueryPlanner object around, and skip subsequent schema parsing / composing. - Move the type `OperationResult` to a utils file (so we can use it in both `do_plan.ts` and `do_introspect.ts` Rust: - Use &str instead of String for schema SDL, since apollo-rs provides as_str() on the Schema type. - Add a plan function, that is generic over it's return type ( `Result<T, PlanningErrors>` where `T: DeserializeOwned + 'static` so we can keep the `QueryPlan` structure on the router-core side. - Add plan() tests that call plan<serde_json::Value>() so we have snapshots over the plan result.
fixes #1088 This commit is built on top of @BrynCooke 's work on #812, and exposes a query planning functionality, which has been living in a separate branch for a while. A few things happened: General: - Remove jest tests from the router bridge (they weren't being run anyway, and they are now superseded by the cargo tests) - Add router-bridge js files to the prettier configuration for formatting and linting - Add a dependency to query-planner-js to the router-bridge TypeScript/Javascript: - Add `do_plan.ts` that calls the query-planner behind the scenes. - Use `new QueryPlanner(schema).buildQueryPlan` instead of the old `buildQueryPlan` function, that doesn't exist anymore (depending on how long we want to keep this code, we might be able to keep the QueryPlanner object around, and skip subsequent schema parsing / composing. - Move the type `OperationResult` to a utils file (so we can use it in both `do_plan.ts` and `do_introspect.ts` Rust: - Use &str instead of String for schema SDL, since apollo-rs provides as_str() on the Schema type. - Add a plan function, that is generic over it's return type ( `Result<T, PlanningErrors>` where `T: DeserializeOwned + 'static` so we can keep the `QueryPlan` structure on the router-core side. - Add plan() tests that call plan<serde_json::Value>() so we have snapshots over the plan result.
fixes #1088 This commit is built on top of @BrynCooke 's work on #812, and exposes a query planning functionality, which has been living in a separate branch for a while. A few things happened: General: - Remove jest tests from the router bridge (they weren't being run anyway, and they are now superseded by the cargo tests) - Add router-bridge js files to the prettier configuration for formatting and linting - Add a dependency to query-planner-js to the router-bridge TypeScript/Javascript: - Add `do_plan.ts` that calls the query-planner behind the scenes. - Use `new QueryPlanner(schema).buildQueryPlan` instead of the old `buildQueryPlan` function, that doesn't exist anymore (depending on how long we want to keep this code, we might be able to keep the QueryPlanner object around, and skip subsequent schema parsing / composing. - Move the type `OperationResult` to a utils file (so we can use it in both `do_plan.ts` and `do_introspect.ts` Rust: - Use &str instead of String for schema SDL, since apollo-rs provides as_str() on the Schema type. - Add a plan function, that is generic over it's return type ( `Result<T, PlanningErrors>` where `T: DeserializeOwned + 'static` so we can keep the `QueryPlan` structure on the router-core side. - Add plan() tests that call plan<serde_json::Value>() so we have snapshots over the plan result.
* chore: Introduce query planner to router-bridge fixes #1088 This commit is built on top of @BrynCooke 's work on #812, and exposes a query planning functionality, which has been living in a separate branch for a while. A few things happened: General: - Remove jest tests from the router bridge (they weren't being run anyway, and they are now superseded by the cargo tests) - Add router-bridge js files to the prettier configuration for formatting and linting - Add a dependency to query-planner-js to the router-bridge TypeScript/Javascript: - Add `do_plan.ts` that calls the query-planner behind the scenes. - Use `new QueryPlanner(schema).buildQueryPlan` instead of the old `buildQueryPlan` function, that doesn't exist anymore (depending on how long we want to keep this code, we might be able to keep the QueryPlanner object around, and skip subsequent schema parsing / composing. - Move the type `OperationResult` to a utils file (so we can use it in both `do_plan.ts` and `do_introspect.ts` Rust: - Use &str instead of String for schema SDL, since apollo-rs provides as_str() on the Schema type. - Add a plan function, that is generic over it's return type ( `Result<T, PlanningErrors>` where `T: DeserializeOwned + 'static` so we can keep the `QueryPlan` structure on the router-core side. - Add plan() tests that call plan<serde_json::Value>() so we have snapshots over the plan result. - Add a test to make sure validation happens during query planning, checking queries that are missing subfields, and queries to fields that don't exist
|
@BrynCooke let's close it and see if it still works :D |
|
I'm deleting the branch, too, since that might be what the cargo |
Error handling needs to be improved.
Pulled out common code for running Js to a separate module.
Note that the existing
harmonizemethod has been renamed tocompose::composeto allowplan:planto be added.