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

@@ -155,8 +163,17 @@ onConn (L.Logger logger) corsPolicy wsId requestHead = do
        (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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ecthiender commented Apr 24, 2019 •

edited

hasura-bot commented Apr 24, 2019

netlify bot commented Apr 25, 2019 •

edited

hasura-bot commented Apr 25, 2019

hasura-bot commented Apr 25, 2019

hasura-bot commented Apr 26, 2019

rakeshkky left a comment

0x777 commented Apr 30, 2019

shahidhk commented Apr 30, 2019 •

edited

hasura-bot commented Apr 30, 2019

hasura-bot commented May 2, 2019

hasura-bot commented May 3, 2019

hasura-bot commented May 3, 2019

hasura-bot commented May 7, 2019

0x777 May 8, 2019

hasura-bot commented May 8, 2019

hasura-bot commented May 9, 2019

hasura-bot commented May 9, 2019

hasura-bot commented May 10, 2019

		@@ -155,8 +163,17 @@ onConn (L.Logger logger) corsPolicy wsId requestHead = do
		(BL.toStrict $ J.encode $ encodeGQLErr False qErr)

		checkPath =

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

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

Conversation

ecthiender commented Apr 24, 2019 • edited

Description

Affected components

Related Issues

Solution and Design

Steps to test and verify

Limitations, known bugs & workarounds

hasura-bot commented Apr 24, 2019

netlify bot commented Apr 25, 2019 • edited

hasura-bot commented Apr 25, 2019

hasura-bot commented Apr 25, 2019

hasura-bot commented Apr 26, 2019

rakeshkky left a comment

Choose a reason for hiding this comment

0x777 commented Apr 30, 2019

shahidhk commented Apr 30, 2019 • edited

hasura-bot commented Apr 30, 2019

hasura-bot commented May 2, 2019

hasura-bot commented May 3, 2019

hasura-bot commented May 3, 2019

hasura-bot commented May 7, 2019

0x777 May 8, 2019

Choose a reason for hiding this comment

hasura-bot commented May 8, 2019

hasura-bot commented May 9, 2019

hasura-bot commented May 9, 2019

hasura-bot commented May 10, 2019

ecthiender commented Apr 24, 2019 •

edited

netlify bot commented Apr 25, 2019 •

edited

shahidhk commented Apr 30, 2019 •

edited