Ensure directives are preserved when serializing subgraph schemas in planner #568
Conversation
|
This PR targets the branch |
router-bridge/Cargo.toml
Outdated
| @@ -1,6 +1,6 @@ | |||
| [package] | |||
| name = "router-bridge" | |||
| version = "0.6.0-beta.0+v2.9.0-beta.0" | |||
| version = "0.6.0-beta.0+v2.9.0-beta.1" | |||
There was a problem hiding this comment.
(partial review, still looking at code) Note that this should be 0.6.0-beta.1+v2.9.0-beta.0, not 0.6.0-beta.0+v2.9.0-beta.1. This is assuming the build info here is meant to indicate the JS/federation repo package versions. (If it's meant to be the router-bridge JS package version, then it should be 0.6.0-beta.1+v2.9.0-beta.1, but my impression was that the JS router-bridgepackage was meant to be invisible.)
There was a problem hiding this comment.
Ok, I can update that. Per @SimonSapin's comment, should I rebase this and target main? Or is it ok to release a beta.1 from this branch?
There was a problem hiding this comment.
I think we've been releasing RCs off branches and not main (or at least, recently we have been).
There was a problem hiding this comment.
I realized I'm not super familiar with how we release this package. Is the branch name significant, or is it fine to merge here after approval and publish from this branch?
| @@ -281,7 +282,7 @@ export class BridgeQueryPlanner { | |||
| let result = new Map<string, string>(); | |||
|
|
|||
| subgraphs.names().forEach((name) => { | |||
| let sdl = printSchema(subgraphs.get(name).schema.toGraphQLJSSchema({})); | |||
| let sdl = printSchemaWithDirectives(subgraphs.get(name).schema); | |||
There was a problem hiding this comment.
(This comment is more of an FYI, nothing to fix.) Looking at the PR that introduced subgraphs(), it seems this method is used to generate subgraph schemas to validate subgraph responses against. Two things here:
- I'm guessing here that the presence of directive applications in the subgraph schema shouldn't impede this subgraph response validation.
- We're also presumably now asserting downstream that the subgraph schemas with directive applications are valid GraphQL (which they ought to be), but I checked and that's not a new assertion in router (we use the default of
shouldValidatewhen callingSupergraph.build()in router), so it should be fine (i.e., if there were bugs in how we generated/printed subgraph schemas, they wouldn't suddenly be triggered after this PR).
There was a problem hiding this comment.
I'm guessing here that the presence of directive applications in the subgraph schema shouldn't impede this subgraph response validation.
It shouldn't affect that part of the code. One thing I'm noticing when running the router tests against this change is that it changes the query hash in a few tests because the hash will include directives if they exist. So, this will bust the cache, but that happens anyway when we bump the federation version. I think this will still be correct.
There was a problem hiding this comment.
One thing I'm noticing when running the router tests against this change is that it changes the query hash in a few tests because the hash will include directives if they exist.
This presumably also would cause cache misses when just a subgraph schema's directive applications change, though if such caches are also keyed by the supergraph schema (which presumably hasn't had directive applications stripped away), then this cache miss was going to happen anyway, so it's fine.
It might be something for @Geal to chime in on once they're available, but it probably shouldn't block a new RC release.
| weight: Int! | ||
| ) on ARGUMENT_DEFINITION | ENUM | FIELD_DEFINITION | INPUT_FIELD_DEFINITION | OBJECT | SCALAR | ||
|
|
||
| directive @cost__listSize( |
There was a problem hiding this comment.
I'm confused why there's a directive definition for @cost__listSize here since @listSize was imported; the directive's name in schema should be @listSize instead of @cost__listSize.
There was a problem hiding this comment.
It's an artifact of composition when we have multiple directives defined in the same specification. I copied this over from other test cases to make sure it works. We can try to remove that extra definition later, but it's a composition change, so there's nothing I can do in this PR.
There was a problem hiding this comment.
@tninesling
The @link spec generally assumes that directives in a spec have one name in a schema (and code that processes a schema using @link can and often do assume this). This is technically fine in the example schema given because the extra definition gets ignored, but applications of this definition may also be ignored (and that's usually not permissible).
You're right that this isn't a problem with this PR specifically, so I'll go ahead and approve, and release the new RC. I'll see if I can get the composition issue fixed tonight (if so, I'll cut a 2.9.0-beta.1 RC).
sachindshinde
changed the base branch from
router-bridge@v0.6.0-beta.0+v2.9.0-beta.0
to
router-bridge@v0.6.0-beta.1+v2.9.0-beta.0
sachindshinde
merged commit 0f1ae38
into
router-bridge@v0.6.0-beta.1+v2.9.0-beta.0
No description provided.