Ensure error resolving an entity don't impact resolvable ones (for main branch)
#1306
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Maybe it's because it's a little late in the day for me, but I think I would probably greatly benefit from a walk through of this code. Would you maybe have time tomorrow? Either way I'll dig in deeper tomorrow. |
clenfest
reviewed
Dec 15, 2021
| @@ -489,7 +489,17 @@ function executeSelectionSet( | |||
| const selections = (selection as QueryPlanFieldNode).selections; | |||
|
|
|||
| if (typeof source[responseName] === 'undefined') { | |||
There was a problem hiding this comment.
should we simplify to
if (source[responseName] === undefined)There was a problem hiding this comment.
Oops, did you not touch this line? Never mind.
query-graphs-js/src/querygraph.ts
Outdated
| @@ -353,6 +360,16 @@ export class QueryGraph { | |||
| const indexes = this.typesToVertices.get(typeName); | |||
| return indexes == undefined ? [] : indexes.map(i => this.vertices[i]); | |||
| } | |||
|
|
|||
| externalTester(source: string) { | |||
query-planner-js/src/buildPlan.ts
Outdated
| @@ -1637,7 +1642,9 @@ function inputsForRequire(graph: QueryGraph, entityType: ObjectType, edge: Edge, | |||
| fullSelectionSet.add(new FieldSelection(new Field(entityType.typenameField()!))); | |||
| fullSelectionSet.mergeIn(edge.conditions!); | |||
| if (includeKeyInputs) { | |||
| fullSelectionSet.mergeIn(additionalKeyEdgeForRequireEdge(graph, edge).conditions!); | |||
| const keyCondition = getLocallySatisfiableKey(graph, edge.head); | |||
| assert(keyCondition, () => `No key found for require on ${edge}`); | |||
clenfest
approved these changes
Dec 15, 2021
Some times, when a @requires is just after a key, we can simply collect the required field before taking the key. Other times, we encounter a @require in a type T of a subgraph A, and have to jump to subgraph B to get the required field. In that case, once we've collected the required fields, we need to "resume" query on T in A, and that means using a key there. The code dealing with that post-require "return key" later case was incorrect, essentially using a key on subgraph B instead of one on subgraph A. As a consequence, subgraphs that shouldn't have been allowed to compose (because they were missing the needed key on A) were allowed to composed, and the code later failed during query planning. This commit fixes this issue, and adds tests for that case. One of the test introduced _fails_ as of this commit, but that is due to the problem describe on apollographql#376 and the fix will be in a followup commit.
Fixes apollographql#376 Co-authored-by: Sylvain Lebresne <sylvain@apollographql.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR has the same fix than in #1305, but for
main.However, testing this also uncovered another issue with
@requiresonmain(fed 2 code), and this PR also has this additional fix. So first commit is that "other" fix (but it includes all the tests), and second commit is the fix of #1305.