Skip to content
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

makeRemoteExecutableSchema squares remote query field executions #1346

Closed
meiamsome opened this issue Apr 6, 2020 · 9 comments
Closed

makeRemoteExecutableSchema squares remote query field executions #1346

meiamsome opened this issue Apr 6, 2020 · 9 comments

Comments

@meiamsome
Copy link

meiamsome commented Apr 6, 2020

The current default implementation of makeRemmoteExecutableSchema's createResolver will result in a query with n fields selected at the query being executed n times, meaning that the remote server will see n^2 executions of the fields at the query level.

For example, with this schema:

type Query {
  fieldA: Int!
  fieldB: Int!
  field3: Int!
}

Executing this request:

query {
  fieldA
  fieldB
  field3
}

Will result in the fetcher being executed three times, and each execution will receive a document with all three fields at the query level. This means that each field will be evaluated 3 times on the server, resulting in 9 total field executions.

To illustrate the problem a bit better, I have made this commit that adds two tests that would both be acceptable solutions to the problem - either call the fetcher a single time for all fields, or call the fetcher n times but send a different request document to each: meiamsome@f81b20e

Currently running these tests results in the following failures:

  1) when query for multiple fields
       forwards one upstream query:

      AssertionError: expected [ Array(3) ] to have a length of 1 but got 3
      + expected - actual

      -3
      +1

  2) when query for multiple fields
       forwards three upstream queries:

      AssertionError: expected '{\n  fieldA\n  fieldB\n  field3\n}\n' to equal '{\n  fieldA\n}\n'
      + expected - actual

       {
         fieldA
      -  fieldB
      -  field3
       }
@yaacovCR
Copy link
Collaborator

yaacovCR commented Apr 6, 2020

@yaacovCR
Copy link
Collaborator

yaacovCR commented Apr 7, 2020

The issue of n^2 queries won't be solved by batching and should be fixed in makeRemoteExecutableSchema.

In the meantime, in the alpha versions, you should be able to do use the new wrapSchema function instead of makeRemoteExecutableSchema, which hopefully should not have this problem, as follows:

const remoteSchema = wrapSchema({
  schema: schemaFromIntrospection,
  link: <your_link>,
  transforms: <your transforms>
});

The intended benefit of wrapSchema is that you can create a remote schema and apply transforms with only one round of delegation. A side benefit, hopefully, is that it does not have the n^2 problem.

I would refactor makeRemoteExecutableSchema to use wrapSchema under the hood. But, I am having trouble doing that in a backwards compatible way, as makeRemoteExecutableSchema takes these custom resolver arguments.

Getting closer, though.

@yaacovCR yaacovCR closed this as completed Apr 7, 2020
@yaacovCR yaacovCR reopened this Apr 7, 2020
@yaacovCR
Copy link
Collaborator

yaacovCR commented Apr 7, 2020

Thanks for bringing this up. The n^2 problem should be fixed on next branch via #1352

Folding into #1306.

@yaacovCR yaacovCR closed this as completed Apr 7, 2020
@yaacovCR yaacovCR mentioned this issue Apr 7, 2020
22 tasks
@ardatan
Copy link
Owner

ardatan commented Apr 7, 2020

We can add his tests as well so we will make sure it is working correctly later @yaacovCR

yaacovCR added a commit that referenced this issue Apr 7, 2020
@yaacovCR
Copy link
Collaborator

yaacovCR commented Apr 7, 2020

@ardatan, we definitely do need a lot more tests!

In the meantime, @meiamsome might you be able to confirm that this is fixed used graphql-tools@next? v5-rc.3 should have this bug fix.

@yaacovCR yaacovCR reopened this Apr 7, 2020
@meiamsome
Copy link
Author

I will have a look at this and hopefully get back to you some time tomorrow or so. Thanks for the quick response so far!

ardatan added a commit that referenced this issue Apr 23, 2020
@ardatan ardatan added waiting-for-release Fixed/resolved, and waiting for the next stable release and removed waiting on contributor labels Apr 23, 2020
ardatan added a commit that referenced this issue Apr 23, 2020
* Add tests for #1346

* Remove unnecessary dep

* Fix format
@yaacovCR
Copy link
Collaborator

Should be fixed with latest alpha, npm install graphql-tools@alpha.

@yaacovCR
Copy link
Collaborator

May be fixed in v5, but tests are in alpha, thanks @ardatan

@yaacovCR
Copy link
Collaborator

Closed by #1419.

@yaacovCR yaacovCR removed the waiting-for-release Fixed/resolved, and waiting for the next stable release label May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants