-
-
Notifications
You must be signed in to change notification settings - Fork 834
[batch-delegate] Error paths use their batched indices #2951
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
Conversation
|
...to use DataLoader for the actual GraphQL results, rather than the external values This allows us to properly transduce streams. TODO: 1. We should now be able to add the stream directive for batchDelegateToSchema. 2. Refactor error parsing, now we are relying on the onLocatedError option, but this is no longer necessary. Note that additional tests for batched errors are now passing (see #2951) 3. Bring back createBatchDelegateFn, that may be useful 4. The Receiver is now created separately for each list item, which means that the initialPathDepth probably does not match? This will require a separate argument to the Receiver constructor for adjustment 5. Bring back valuesFromResults and fix documentation, we now operate on GraphQL results, so should be fewer pitfalls (see #2829)
...to use DataLoader for the actual GraphQL results, rather than the external values This allows us to properly transduce streams. TODO: 1. We should now be able to add the stream directive for batchDelegateToSchema. 2. Refactor error parsing, now we are relying on the onLocatedError option, but this is no longer necessary. Note that additional tests for batched errors are now passing (see #2951) 3. Bring back createBatchDelegateFn, that may be useful 4. The Receiver is now created separately for each list item, which means that the initialPathDepth probably does not match? This will require a separate argument to the Receiver constructor for adjustment 5. Bring back valuesFromResults and fix documentation, we now operate on GraphQL results, so should be fewer pitfalls (see #2829)
e1d3faf to
614c08c
Compare
b1c69a3 to
a9e6cd1
Compare
and fix them by reworking batch delegation
and fix them by reworking batch delegation
use lazy query planner add tests from #2951 and fix them by reworking batch delegation create memoized, static buidDelegationPlan buildDelegationPlan can be used to calculate the rounds of delegation necessary to completely merge an object given the stitching metadata stored within the schema and a given set of fieldNodes TODO: this function could be extracted to work on the stiched schema itself rather than the extracted metadata, and might be useful as part of a graphiql-type interface
refactor externalObject internals into separate file add tests from #2951 and fix them by reworking batch delegation create memoized, static buidDelegationPlan buildDelegationPlan can be used to calculate the rounds of delegation necessary to completely merge an object given the stitching metadata stored within the schema and a given set of fieldNodes TODO: this function could be extracted to work on the stiched schema itself rather than the extracted metadata, and might be useful as part of a graphiql-type interface
3f6ec05 to
50609df
Compare
87dec5a to
d99f20b
Compare
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/theguild/graphql-tools/GeNL2PjXdSzn3dCqiZb2QZwSyn9J |
refactor externalObject internals into separate file add tests from #2951 and fix them by reworking batch delegation buildDelegationPlan can be used to calculate the rounds of delegation necessary to completely merge an object given the stitching metadata stored within the schema and a given set of fieldNodes TODO: change buildDelegationPlan function to method on MergedTypeInfo as class? move back to stitch package?
refactor externalObject internals into separate file add tests from #2951 and fix them by reworking batch delegation buildDelegationPlan can be used to calculate the rounds of delegation necessary to completely merge an object given the stitching metadata stored within the schema and a given set of fieldNodes TODO: change buildDelegationPlan function to method on MergedTypeInfo as class? move back to stitch package?
refactor externalObject internals into separate file add tests from #2951 and fix them by reworking batch delegation buildDelegationPlan can be used to calculate the rounds of delegation necessary to completely merge an object given the stitching metadata stored within the schema and a given set of fieldNodes TODO: change buildDelegationPlan function to method on MergedTypeInfo as class? move back to stitch package?
4946aeb to
4be9073
Compare
d34fb65 to
b8bf584
Compare
|
Thanks for the PR! |
Description
Reproduction test case. When using an n-plus-one delegateToSchema, the result is as expected. When using batchDelegateToSchema, error paths are incorrect.
We expect the path to have index
1as the error is for the second object, however it is the same error as at index0and is deduplicated in the batched request. The error returned has this deduplicated index instead of its actual index.FAIL packages/batch-delegate/tests/errorPaths.test.ts ● preserves error path indices › using batchDelegateToSchema expect(received).toMatchObject(expected) - Expected - 1 + Received + 1 @@ -28,11 +28,11 @@ "id": "1", }, "message": "Not Found", "path": Array [ "objects", - 1, + 0, "property", ], }, ], } 149 | 150 | expect(getProperty).toBeCalledTimes(1); > 151 | expect(result).toMatchObject(expected); | ^ 152 | }); 153 | }); 154 | at Object.<anonymous> (packages/batch-delegate/tests/errorPaths.test.ts:151:20)Related #2950
Type of change
Please delete options that are not relevant.
Screenshots/Sandbox (if appropriate/relevant):
Adding links to sandbox or providing screenshots can help us understand more about this PR and take action on it as appropriate
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Environment:
@graphql-tools/...:Checklist:
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...