graphql responses are now http 200 (fix #1368) #2064

ecthiender · 2019-04-24T14:41:35Z

All graphql clients expect responses to be HTTP 200. Non-200 responses are considered transport layer errors. Changed all graphql responses in /v1/graphql endpoint to be 200.

Description

This change only works on the new endpoint: /v1/graphql. The behaviour of /v1alpha1/graphql remains the same.

Affected components

Server
Tests

Related Issues

#1368

Solution and Design

Steps to test and verify

Make any wrong request to /v1/graphql endpoint
It returns HTTP 200 status.

Limitations, known bugs & workarounds

Console changes are pending.
Have select tests for /v1alpha1/graphql over websocket

All graphql clients expect responses to be HTTP 200. Non-200 responses are considered transport layer errors. Changed all graphql responses in **/v1/graphql** endpoint to be 200. TODO: run tests for both endpoints

hasura-bot · 2019-04-24T18:39:11Z

Review app for commit 2e194ac deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-2e194ac

- run all graphql tests on /v1/graphql and /v1alpha1/graphql

netlify · 2019-04-25T13:30:35Z

Deploy preview for hasura-docs ready!

Built with commit 246ff07

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

hasura-bot · 2019-04-25T14:04:09Z

Review app for commit 7507cf2 deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-7507cf2

hasura-bot · 2019-04-25T14:43:19Z

Review app for commit 8c34961 deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-8c34961

hasura-bot · 2019-04-26T10:26:53Z

Review app for commit 9f1e04c deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-9f1e04c

rakeshkky

LGTM

0x777 · 2019-04-30T04:55:31Z

@ecthiender Can you check the error response format for subscriptions for /v1/graphql?

Are they as per the apollo-ws protocol?
Are they consistent with the errors in http?

shahidhk · 2019-04-30T11:08:15Z

@0x777 You're right, subscription errors are not GraphQL compliant.

{
  "type": "error",
  "id": "1",
  "payload": {
    "path": "$.selectionSet.articles.selectionSet.id1",
    "error": "field \"id1\" not found in type: 'articles'",
    "code": "validation-failed"
  }
}

is what the response is right now. It should be:

{
  "type": "error",
  "id": "1",
  "payload": {
    "message": "field \"id1\" not found in type: 'articles'",
    "extensions": {
      "code": "validation-failed",
      "path": "$.selectionSet.articles.selectionSet.id1"
    }
  }
}

Need to run some more tests to confirm the behaviour.

hasura-bot · 2019-04-30T11:30:52Z

Review app for commit 1db897a deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-1db897a9

hasura-bot · 2019-05-02T08:05:32Z

Review app for commit d80356b deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-d80356b8

hasura-bot · 2019-05-03T07:01:26Z

Review app for commit 979dc2d deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-979dc2db

- /v1alpha1/graphql returns older response structure - /v1/graphql returns newer response structure

hasura-bot · 2019-05-03T11:50:41Z

Review app for commit 41e581c deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-41e581c8

- run all tests only for v1/graphql - run specific tests for v1alpha1/graphql to check error response structures

hasura-bot · 2019-05-07T14:47:37Z

Review app for commit 1531ed9 deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-1531ed96

0x777 · 2019-05-08T10:47:44Z

server/src-lib/Hasura/GraphQL/Transport/WebSocket.hs

        (H.statusMessage $ qeStatus qErr) []
        (BL.toStrict $ J.encode $ encodeGQLErr False qErr)

    checkPath =


checkPath should return ErrRespType. You don't have to look at paths twice.

hasura-bot · 2019-05-08T12:56:43Z

Review app for commit eedc41f deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-eedc41f2

hasura-bot · 2019-05-09T08:36:32Z

Review app for commit bd16413 deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-bd16413d

hasura-bot · 2019-05-09T12:31:03Z

Review app for commit 246ff07 deployed to Heroku: https://hge-ci-pull-2064.herokuapp.com
Docker image for server: hasura/graphql-engine:pull2064-246ff076

hasura-bot · 2019-05-10T06:05:13Z

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

Changes compared to `/v1alpha1/graphql` * Changed all graphql responses in **/v1/graphql** endpoint to be 200. All graphql clients expect responses to be HTTP 200. Non-200 responses are considered transport layer errors. * Errors in http and websocket layer are now consistent and have similar structure.

fix all graphql responses to be http 200 (fix hasura#1368)

2e194ac

All graphql clients expect responses to be HTTP 200. Non-200 responses are considered transport layer errors. Changed all graphql responses in **/v1/graphql** endpoint to be 200. TODO: run tests for both endpoints

ecthiender requested a review from 0x777 as a code owner April 24, 2019 14:41

ecthiender added c/server Related to server s/do-not-merge Do not merge this pull request to master labels Apr 24, 2019

ecthiender changed the title ~~fix graphql responses to be http 200 (fix #1368)~~ graphql responses are now http 200 (fix #1368) Apr 24, 2019

ecthiender added 2 commits April 25, 2019 16:47

run all graphql tests on both graphql endpoints

daed9e6

- run all graphql tests on /v1/graphql and /v1alpha1/graphql

fix faulty test spec (did not have response key)

7507cf2

ecthiender removed the s/do-not-merge Do not merge this pull request to master label Apr 25, 2019

add docs

8c34961

ecthiender requested a review from rikinsk-zz as a code owner April 25, 2019 14:11

ecthiender requested a review from rakeshkky April 25, 2019 14:11

add v1/graphql/explain endpoint

9f1e04c

rakeshkky previously approved these changes Apr 26, 2019

View reviewed changes

Merge branch 'master' into fix-1368

54badc9

shahidhk added 5 commits April 30, 2019 13:15

Merge branch 'master' into fix-1368

936493f

docs url changes

1de10db

community content url changes

f12ca6f

install-manifests url changes

aa5ba9f

Merge branch 'master' into fix-1368

a6bae57

shahidhk dismissed rakeshkky’s stale review via a6bae57 April 30, 2019 10:17

ecthiender requested a review from shahidhk as a code owner April 30, 2019 10:17

shahidhk added 2 commits April 30, 2019 15:50

Merge branch 'master' into fix-1368

e65599b

fix tests

1db897a

ecthiender and others added 2 commits May 2, 2019 12:19

use graphql compliant errors in websocket

ae72963

fix nested errors keys

d80356b

shahidhk added 2 commits May 2, 2019 14:46

change response type for validation errors

8010d0d

add start failed error

979dc2d

handle both graphql endpoints for websocket transport

41e581c

- /v1alpha1/graphql returns older response structure - /v1/graphql returns newer response structure

ecthiender added 2 commits May 7, 2019 18:37

fix tests for v1alpha1/graphql and v1/graphql

d2470ae

- run all tests only for v1/graphql - run specific tests for v1alpha1/graphql to check error response structures

Merge branch 'master' of github.com:hasura/graphql-engine into fix-1368

1531ed9

0x777 reviewed May 8, 2019

View reviewed changes

fix review comments and test

eedc41f

Merge branch 'master' into fix-1368

bd16413

Merge branch 'master' into fix-1368

246ff07

0x777 approved these changes May 9, 2019

View reviewed changes

0x777 merged commit a21f6cd into hasura:master May 10, 2019

shahidhk mentioned this pull request Dec 19, 2019

Return non 200 http status code on error #3571

Closed

graphql responses are now http 200 (fix #1368) #2064

graphql responses are now http 200 (fix #1368) #2064

Uh oh!

Conversation

ecthiender commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Affected components

Related Issues

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

Uh oh!

hasura-bot commented Apr 24, 2019

Uh oh!

netlify bot commented Apr 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasura-bot commented Apr 25, 2019

Uh oh!

hasura-bot commented Apr 25, 2019

Uh oh!

hasura-bot commented Apr 26, 2019

Uh oh!

rakeshkky left a comment

Choose a reason for hiding this comment

Uh oh!

0x777 commented Apr 30, 2019

Uh oh!

shahidhk commented Apr 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hasura-bot commented Apr 30, 2019

Uh oh!

hasura-bot commented May 2, 2019

Uh oh!

hasura-bot commented May 3, 2019

Uh oh!

hasura-bot commented May 3, 2019

Uh oh!

hasura-bot commented May 7, 2019

Uh oh!

0x777 May 8, 2019

Choose a reason for hiding this comment

Uh oh!

hasura-bot commented May 8, 2019

Uh oh!

hasura-bot commented May 9, 2019

Uh oh!

hasura-bot commented May 9, 2019

Uh oh!

hasura-bot commented May 10, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ecthiender commented Apr 24, 2019 •

edited

Loading

netlify bot commented Apr 25, 2019 •

edited

Loading

shahidhk commented Apr 30, 2019 •

edited

Loading