这是indexloc提供的服务,不要输入任何密码
Skip to content

Conversation

@paf31
Copy link
Contributor

@paf31 paf31 commented Dec 30, 2019

Fixes #1812

Description

Reports errors in parallel, treating each operation independently, when receiving batched requests.

WIP, because I would still like to figure out a good way to integrate this into the test suite, but I'm putting this PR up now so that I can get a test environment to share.

Affected components

  • Server
  • Tests

Related Issues

#1812

Solution and Design

Steps to test and verify

Here is a simple test case on the terminal:

$ cat | curl -X POST 'http://localhost:8080/v1/graphql' -d @- | jq -S
[
  {
    "query": "query first { user { email id name } }",
    "variables": null
  },
  {
    "query": "query second { role { id name } }",
    "variables": null
  }
]
[
  {
    "errors": [
      {
        "extensions": {
          "code": "validation-failed",
          "path": "$.selectionSet.user"
        },
        "message": "field \"user\" not found in type: 'query_root'"
      }
    ]
  },
  {
    "errors": [
      {
        "extensions": {
          "code": "validation-failed",
          "path": "$.selectionSet.role"
        },
        "message": "field \"role\" not found in type: 'query_root'"
      }
    ]
  }
]

Limitations, known bugs & workarounds

Server checklist

Catalog upgrade

Does this PR change Hasura Catalog version?

  • No

Metadata

Does this PR add a new Metadata feature?

  • No

GraphQL

  • No new GraphQL schema is generated

Breaking changes

  • No Breaking changes

@netlify
Copy link

netlify bot commented Dec 30, 2019

Deploy preview for hasura-docs ready!

Built with commit 534fe10

https://deploy-preview-3605--hasura-docs.netlify.com

@paf31
Copy link
Contributor Author

paf31 commented Dec 30, 2019

@thefliik If you are interested in trying out this change, there should be a test environment available soon once CI finishes. Thanks!

@hasura-bot
Copy link
Contributor

Review app for commit 63a55fc deployed to Heroku: https://hge-ci-pull-3605.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3605-63a55fc9

@paf31 paf31 marked this pull request as ready for review December 30, 2019 20:01
@paf31 paf31 requested a review from lexi-lambda as a code owner December 30, 2019 20:01
- id: '2'
query:
- query: |
mutation {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this is poor form (updating the test data set from a test like this), so I try to put back the original data at the end so that it doesn't affect any other tests.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the test setup/teardown actions run around each individual test, not around the whole test group, so I think this is unnecessary.

@hasura-bot
Copy link
Contributor

Review app for commit 534fe10 deployed to Heroku: https://hge-ci-pull-3605.herokuapp.com
Docker image for server: hasura/graphql-engine:pull3605-534fe101

@paf31
Copy link
Contributor Author

paf31 commented Dec 30, 2019

This seems to work well with apollo-client, verified using this little test case:

const ApolloClient = require('apollo-client').default;
const BatchHttpLink = require('apollo-link-batch-http').BatchHttpLink;
const InMemoryCache = require('apollo-cache-inmemory').InMemoryCache;

const fetch = require('node-fetch');
const gql = require('graphql-tag').default;

const link = new BatchHttpLink({ 
  uri: 'http://localhost:8080/v1/graphql', 
  fetch 
});

const client = new ApolloClient({ link, cache: new InMemoryCache() });

client.query({
  query: gql`query first { user { id } }`,
}).then(user => console.log(user))
  .catch(err => console.error(err.message));
  
client.query({
  query: gql`query second { role { id } }`,
}).then(role => console.log(role))
  .catch(err => console.error(err.message));

Using netcat, I can see the requests get batched, and then errors get handled independently on the client.

@lexi-lambda lexi-lambda merged commit 9e2c8b4 into hasura:master Dec 30, 2019
@hasura-bot
Copy link
Contributor

Review app https://hge-ci-pull-3605.herokuapp.com is deleted

@paf31 paf31 deleted the batching-errors branch December 30, 2019 23:28
@jorroll
Copy link

jorroll commented Dec 31, 2019

@thefliik If you are interested in trying out this change, there should be a test environment available soon once CI finishes. Thanks!
...
Review app https://hge-ci-pull-3605.herokuapp.com is deleted

@paf31 looks like I was too slow on the draw. I'm not actually using batching in any projects at the moment though, so I probably couldn't have provided any new feedback anyway.

👍

polRk pushed a commit to polRk/graphql-engine that referenced this pull request Feb 12, 2020
* Report errors in parallel when batching
* Add some more test cases
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

Successfully merging this pull request may close these issues.

Feature Request: Batching queries

4 participants