feat: Expose document representation of sub-query request within GraphQLDataSourceProcessOptions so that it is available to RemoteGraphQLDataSource.process and RemoteGraphQLDataSource.willSendRequest #1878
Conversation
👷 Deploy request for apollo-federation-docs pending review.Visit the deploys page to approve it
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
theJC
changed the title
theJC
changed the title
|
I'm personally ok with adding this. The only thing I'd maybe suggest would be to make |
theJC
force-pushed
the
exposeSubqueryDocumentNode
branch
from
179fc9e to
fc5ccc1
Compare
|
Thanks for the feedback, it has been adjusted to be optional. @pcmanus -- Do you have any guidance on the best way to approach updating the tests? Are we best served by adding the representation of the documentNode into all the query plan results in build-query-plan.feature? |
|
@theJC As the patch stand, I don't know of other option than adding the representations all over in That said, following on my previous suggestion, one option might be to do "add an option to disable this" now. This would mean adding some new Personally, I kind of prefer that later option because as said before, I suspect we'll have some use case at some point where generated this somewhat redundant info is not desirable. But I won't object to just updating all the cucumber tests either. |
theJC
force-pushed
the
exposeSubqueryDocumentNode
branch
from
a891c2e to
a416627
Compare
|
@pcmanus I believe I've addressed your valuable input, let me know if there is anything else needed to make this PR mergeable. Thanks again for your help, |
There was a problem hiding this comment.
Sorry the delay. Patch looks great, thanks, though I'm afraid it needs rebasing (one of the reason is that the buildPlan.ts file has been reorganised a bit, which confuses the diff algorithm). It'd be awesome if you could:
- rebase it.
- add a quick line in
query-planner/CHANGELOG.md, and copy-paste it also ingateway-js/CHANGELOG.md.
and then we can ship it.
theJC
force-pushed
the
exposeSubqueryDocumentNode
branch
from
7c96e8e to
fa69b13
Compare
theJC
changed the title
|
Should be all set @pcmanus |
|
Perfect, thanks a lot of the contribution! |
Since apollographql#1878 (added in 2.1.0), the full AST of subgraph fetch operations has been store in query plans. However, this is not used by the gateway by default, while it noticeably increases the size of query plans, leading to either a less efficient query plan cache (if its size is limited) or increased memory consumption/pressue (if the cache is given plenty of size). This patch disable this option by default. Users that explicitely need to access the subgraph fetch ASTs can still make the trade-off of additional memory pressure by manually using the `GatewayConfig.queryPlannerConfig.exposeDocumentNodeInFetchNode` config option. Fixes apollographql#2227.
Since #1878 (added in 2.1.0), the full AST of subgraph fetch operations has been store in query plans. However, this is not used by the gateway by default, while it noticeably increases the size of query plans, leading to either a less efficient query plan cache (if its size is limited) or increased memory consumption/pressue (if the cache is given plenty of size). This patch disable this option by default. Users that explicitely need to access the subgraph fetch ASTs can still make the trade-off of additional memory pressure by manually using the `GatewayConfig.queryPlannerConfig.exposeDocumentNodeInFetchNode` config option. Fixes #2227
Fixes #1352
Provides access to the AST so that customer code processing a subgraph query no longer has to waste cycles re-parsing it back into a document when needed.