-
Notifications
You must be signed in to change notification settings - Fork 258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Undefined variable reference in generated subgraph query #3112
Comments
Note that the error is repeated. When running older versions of Router, there was not a validation error, but invalid subgraph queries where generated. So it may be that the validation errors are coming from validating the query plan queries, not the main query itself. In those versions, you can also see that the |
Running this with router v1.45.1 gives a different error. I've included the query plan here too:
|
Interesting, thanks for the report and the detailed reproduction! This looks like a bug in the JS query planner. The generated subgraph query is the one that doesn't have the right variables defined. |
We have seen this in v1.33.2, but not sure about earlier versions. What would be one to test? |
Sorry, does that mean it worked in 1.33.2, or that it already didn't work in 1.33.2? 😄 I'm just looking to find out if this is a recent regression in query planning or if it's something that has existed for a long time. |
That means we are seeing this bug in v1.33.2. it is easily reproducible in my test repo with whatever version you care to try. |
OK perfect, thank you. It looks similar to an issue we had in the work-in-progress Rust query planner that has since been fixed. Moved the issue to the federation repo so the JS query planner team can have a look at prioritising this. |
I agree this looks similar to the Rust QP bug that @goto-bus-stop fixed recently. |
Hey @carldunham, can you confirm that this is not an issue if you have query fragment generation turned on? i.e. the following in your router yaml: supergraph:
generate_query_fragments: true Not sure if that's an option for you while waiting for the fix. |
Hi, Chris. I see the same behavior with that setting. Tried setting it to |
Fixes #3112 Before creating an Operation, we call `collectUsedVariables`, which will only pull values from the selection set and not from fragments. This isn't great, because a variable used in a fragment won't be collected, and it doesn't make sense to collect variables from fragments because it's before they are optimized and many will be unused. The inelegant solution I came up with is to pass in available variables in calls to `optimize` or `generateQueryFragments` for an operation where we can add back in the unused variables. This should be ok, because we are guaranteed that exactly one of them will get called by `toPlanNode`. Pretty sure there won't be too much overhead added because we'll only call this once per subgraph fetch.
Describe the bug
This repo includes schema and a sample query that fails validation using the new Rust validation code.
To Reproduce
Steps to reproduce the behavior:
make run
query.graphql
, provide a value for "time" (can be any enum value).Expected behavior
The query should succeed.
Output
yet
$time
is clearly defined and used.Desktop (please complete the following information):
Additional context
Router version v1.52.0
Rover version 0.12.2
The text was updated successfully, but these errors were encountered: