Integrate graphql-http, refactor and comply with spec (v3) #1589

enisdenjo · 2022-08-12T15:21:57Z

This PR encapsulates graphql-http integration, adjustments to comply with the spec and my take on graphql-yoga following my investigation and current understanding.

Please note that this PR is a proposal and would love to discuss this and adjust accordingly!

Key takeways:

Integration with graphql-http made graphql-yoga spec compliant as per graphql-http server compliance tests
Change media type to 'application/graphql-response+json' graphql/graphql-over-http#215 (should we support application/graphql+json for backwards compatibility? why?)
application/json response content-type should NOT use 4xx or 5xx status codes, always 200
application/graphql-response+json response content-type should use 4xx or 5xx on request errors
application/graphql-response+json response content-type is recommended to use 200 status code if the data field is present (even if null, but defined)
- Which is why GraphQL execution errors should not be altering the status codes through the http extensions (only parsing, validation and preparation errors should)
- “If the client is not permitted to issue the GraphQL request then the server SHOULD reply with 403, 401 or similar appropriate status code.”, means the client never got the opportunity get its operation executed. Authentication should happen before execution (also why throwing errors with 401 http extensions from resolvers should not be respected)
application/graphql-response+json is a response content-type, requests should always use application/json
Removed onResultProcess plugin for now. I reckon it should be reintroduced as per @n1ru4l suggestion of giving the user the ability to alter the result before responding.
Merged onRequestParse and onRequestParseDone plugins and renamed it to onPrepare (better name in my opinion since you're "preparing" stuff for execution, like the parameters or an early result).
onRequest and onRespond plugins stay as is, they're very useful
Centralised a lot of code, making it easier to read and navigate
Currently private methods parseX and resultX can be made public and reused to construct own request handlers with the plugins
%future added value for everything I might've forgot to mention 😅

# Conflicts: # packages/graphql-yoga/__tests__/node.spec.ts # packages/graphql-yoga/src/error.ts # packages/graphql-yoga/src/plugins/requestParser/POSTJson.ts # packages/graphql-yoga/src/server.ts

changeset-bot · 2022-08-12T15:22:00Z

⚠️ No Changeset found

Latest commit: db3aca3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

ardatan

~~This is per HTTP specification not GraphQL over HTTP, because this has nothing to do with GraphQL over HTTP since this validation is about the contract between client and server.~~
Sorry I've put this comment in a wrong place. I can write here something else :D
Why do we stop supporting multipart defer/stream and event stream responses. I think we are missing a lot of points of using plugins(modularized structure) and hooks.

enisdenjo · 2022-08-12T15:38:19Z

This is per HTTP specification not GraphQL over HTTP, because this has nothing to do with GraphQL over HTTP since this validation is about the contract between client and server.

@ardatan I don't follow, which validation?

ardatan · 2022-08-12T15:51:20Z

packages/graphql-yoga/__tests__/accept-header.spec.ts

@@ -166,7 +166,11 @@ describe('accept header', () => {
        },
      },
    )
-    expect(response.status).toEqual(406)


This is per HTTP specification not GraphQL over HTTP, because this has nothing to do with GraphQL over HTTP since this validation is about the contract between client and server.

Even though, GraphQL over HTTP says we should return 406 in this case as I understand;
https://github.com/graphql/graphql-over-http/blob/main/spec/GraphQLOverHTTP.md#body

Because when accepting application/json the server should not use 4xx and 5xx (see spec).

But we don't accept application/json, do we?

We do accept it, or do you mean we don't accept it in this situation? The server is set up now to respect the accept headers, if the client requests application/json and makes an invalid operation - the server will error out respecting the accept requirements.

ardatan · 2022-08-12T15:52:01Z

packages/graphql-yoga/__tests__/accept-header.spec.ts

+    expect(response.status).toEqual(200)
+    const body = await response.json()
+    expect(body.errors).toEqual([
+      { message: 'Subscriptions are not supported' },


Message is not clear for me. It is not supported because Accept doesn't conform the response's content type. 406 explains this better.

Same as above, when accepting application/json the server should not use 4xx and 5xx (see spec).

Same with above, we don't accept application/json in this case, so we should return Not Acceptable to the client. Also I think error message is too generic. It is hard to understand that the problem is the accepted mime type.

#1589 (comment). I know that as per the HTTP spec 406 is obvious, but I am trying to follow the GQL over HTTP spec... I can improve the error message, it is indeed vague.

ardatan · 2022-08-12T15:52:20Z

packages/graphql-yoga/__tests__/accept-header.spec.ts

@@ -202,6 +206,10 @@ describe('accept header', () => {
        accept: 'application/json',
      },
    })
-    expect(response.status).toEqual(406)
+    expect(response.status).toEqual(200)


Same for this.
https://github.com/dotansimha/graphql-yoga/pull/1589/files#r944599273

Same as above, when accepting application/json the server should not use 4xx and 5xx (see spec).

ardatan · 2022-08-12T15:53:23Z

packages/graphql-yoga/__tests__/error-masking.spec.ts

@@ -145,12 +145,12 @@ describe('error masking', () => {
    }
  })

-  it('non GraphQLError raised in onRequestParse is masked with the correct status code 500', async () => {
+  it('non GraphQLError raised in onPrepare is masked with the correct status code 500', async () => {


onPrepare looks too generic. onRequestParse is getting triggered after onRequest(request received) and when GraphQLParams is requested if any other plugin ended the response earlier.

onPrepare does the "preparation" part, manipulates the params and accepts early results (for caching).

Name seemed like a right fit IMHO; but debatable, I agree.

ardatan · 2022-08-12T15:56:55Z

packages/graphql-yoga/__tests__/error-masking.spec.ts

@@ -186,15 +189,14 @@ describe('error masking', () => {
    })
    const body = JSON.parse(await response.text())
    expect(body).toMatchInlineSnapshot(`
-            Object {
-              "data": null,


I think we should always have data even if it is null;
https://spec.graphql.org/draft/#sec-Response-Format

If the GraphQL response contains the data entry and it is null, then the server SHOULD reply with a 2xx status code and it is RECOMMENDED it replies with 200 status code.

If the request failed before execution, due to a syntax error, missing information, or validation error, this entry must not be present.

Also I found this one, which I think we should cover as well.

ardatan · 2022-08-12T15:57:36Z

packages/graphql-yoga/__tests__/error-masking.spec.ts

@@ -287,7 +286,7 @@ describe('error masking', () => {
        },
      ],
    })
-    expect(response.status).toEqual(200)
+    expect(response.status).toEqual(400)


Why 400? 4xx(Bad Request) should be thrown by the client's mistake, no?

I made a change to default to 200/400 when throwing instances of GraphQLError, and 200/500 when throwing instances of Error. The handler will still respect the http extensions (if not from resolvers).

See here:
https://github.com/enisdenjo/graphql-yoga/blob/39fbe6f575a0f7ba1a111bd4eb54da5b1ea074d2/packages/graphql-yoga/src/server.ts#L454-L476

ardatan · 2022-08-12T16:07:12Z

I still think a generic "plain" GraphQL over http server can already be created by using the parts deleted here with a few lines code.
If we need that kind of version of Yoga, we can basically move all those request parser + validaiton + processors into another treeshakable package then import it in both current Yoga package and we can have a section on our docs like you can basically create your own GraphQL over HTTP server like below;

import { DocumentNode, execute, ExecutionArgs, GraphQLSchema, parse, validate } from "graphql";
import { assertMutationViaGet, assertValidGraphQLParams, GraphQLParams, isGETRequest, isPOSTGraphQLStringRequest, isPOSTJsonRequest, isRegularResult, parseGETRequest, parsePOSTGraphQLStringRequest, parsePOSTJsonRequest } from 'graphql-yoga'; // Or any other core package we would introduce in this case

export async function handleGraphQLoverHTTPRequest(
  request: Request,
  schema: GraphQLSchema,
): Promise<Response> {

  let params: GraphQLParams | undefined;
  if (isGETRequest(request)) {
    params = parseGETRequest(request);
  }
  if (isPOSTJsonRequest(request)) {
    params = await parsePOSTJsonRequest(request);
  }
  if (isPOSTGraphQLStringRequest(request)) {
    params = await parsePOSTGraphQLStringRequest(request);
  }

  try {
    assertValidGraphQLParams(params);
  } catch (e) {
    return new Response(JSON.stringify(e), {
      status: e.extensions.http.status,
      headers: e.extensions.http.headers,
    })
  }

  let document: DocumentNode;
  try {
    document = parse(params.query!);
  } catch (e) {
    return new Response(JSON.stringify(e), {
      status: 400,
    })
  }

  try {
    assertMutationViaGet(request.method, document, params.operationName);
  } catch (e) {
    return new Response(JSON.stringify(e), {
      status: e.extensions.http.status,
      headers: e.extensions.http.headers,
    })
  }

  const errors = validate(schema, document);

  if (errors?.length > 0) {
    return new Response(JSON.stringify({ errors }), {
      status: 400,
      headers: {
        'Content-Type': 'application/json',
      }
    });
  }

  const executionArgs: ExecutionArgs = {
    schema,
    document,
    contextValue: {
      request,
    },
    variableValues: params.variables,
    operationName: params.operationName,
  };

  const result = await execute(executionArgs);

  if (isRegularResult(request, result)) {
    return new Response(JSON.stringify(result), {
      headers: {
        'Content-Type': 'application/json',
      }
    });
  }

  return new Response('Not implemented', {
    status: 500,
  })
}

ardatan · 2022-08-12T16:19:59Z

packages/graphql-yoga/__tests__/http-extensions.spec.ts

@@ -39,120 +32,4 @@ describe('GraphQLError.extensions.http', () => {
    expect(response.status).toBe(401)
    expect(response.headers.get('www-authenticate')).toBe('Bearer')
  })
-
-  it('picks the highest status code and headers for GraphQLErrors thrown within multiple resolvers', async () => {


We have this because we want to give the user to control the response parameters. Also we benefit from this in our plugins in the execution level.

Throwing errors from field resolvers will nullify only that field, for example:

query { a }

and resolver for a errors out, the response will be:

{ data: { a: null }, errors: [{ message: 'Woah!' }] }

As per the spec, if the data field is not-null, the response code must be 200. (For accepting application/json and for accepting application/graphql-response+json)

ardatan · 2022-08-12T16:21:12Z

packages/plugins/apq/__tests__/automatic-persisted-queries.spec.ts

@@ -57,6 +58,7 @@ describe('Automatic Persisted Queries', () => {
    const response = await request(yoga)
      .post('/graphql')
      .send({
+        query: '',


I don't think clients supporting APQ provide query. If they don't, this would break the support of those clients.

query is a required parameter per spec.

Yes but this is a different spec we follow. We are going beyond spec here and we want to support clients instead of introducing our own.

Note to be spec compliant query must not only be present but also must be the string representation of the GraphQL document. An empty string does not satisfy the spec (and is weird/confusing).

Persisted operations are deliberately not covered by the GraphQL-over-HTTP spec right now, this is because the spec is to increase interoperability but persisted operations (when used as an allowlist) have exactly the opposite aim. That’s fine - you can use persisted operations alongside a GraphQL compliant server, it’s just an alternative communication method (not spec compliant, but still perfectly fine). We’ll probably incorporate some wording for persisted operations in a post-1.0 release of the spec.

ardatan · 2022-08-12T16:24:47Z

packages/graphql-yoga/src/plugins/types.ts

 ) => PromiseOrValue<void>

-export interface OnRequestParseDoneEventPayload {
+export interface OnPrepareEventPayload {


onRequestParse and onRequestParseDone hooks are seperated because subsequent plugins can change the way of parsing before parsing has been done by Yoga's request parsers, so requestParser isn't called in here yet.
onRequestParseDone is called after the parameters are parsed in order to let the user change the parsed params for the execution. For example, persisted operations use this to manipulate the parameters for the following execution.

I understand that, my idea was to not have the user control the parsing. Yoga should implement all official and unofficial parsing techniques and do the parsing for the user, and have him manipulate the results only.

But this is debatable...

enisdenjo · 2022-08-12T16:54:19Z

Why do we stop supporting multipart defer/stream and event stream responses. I think we are missing a lot of points of using plugins(modularized structure) and hooks.

@ardatan not my intention to stop supporting them, bugs are failing the integration tests (I've been running yarn test). Will fix them. 👍

I still think a generic "plain" GraphQL over http server can already be created by using the parts deleted here with a few lines code.
If we need that kind of version of Yoga, we can basically move all those request parser + validaiton + processors into another treeshakable package then import it in both current Yoga package and we can have a section on our docs like you can basically create your own GraphQL over HTTP server like below;

@ardatan I follow. I just am of the opinion that users won't need that kind of granularity, I think the benefit of Yoga is not to do any heavy lifting - just plug-n-play.

Furthermore, your example is not spec compliant in terms of the accepted content-type dictating the status codes. It would be different depending on whether the server accepted application/json vs. application/graphql-json.

enisdenjo · 2022-08-16T12:36:49Z

Now with all tests passing (that are passing on v3 branch).

# Conflicts: # packages/graphql-yoga/src/plugins/requestValidation/usePreventMutationViaGET.ts

enisdenjo · 2022-09-20T12:03:11Z

Closing in favour of #1685. Might revisit sometimes in the future, but not soon.

enisdenjo added 22 commits August 8, 2022 17:59

WIP

dcef943

more WIP

f3d4f14

pass node spec

9f2b4bc

errors in json body, spec compliant tests

969f2be

use servercontext

2b8e6cb

typo

f9da15c

sse is acceptable

71881e2

use after ? as ssearch params

9202f7d

better accepts and json multipart

06ec6c2

unnecessary imports

06f4ddb

tests tests tests

c14302c

Merge branch 'v3' into v3-graphql-http

7f202b8

# Conflicts: # packages/graphql-yoga/__tests__/node.spec.ts # packages/graphql-yoga/src/error.ts # packages/graphql-yoga/src/plugins/requestParser/POSTJson.ts # packages/graphql-yoga/src/server.ts

error handling and spec tests

aab65d2

error masking tests

62e817a

pass along empty query string for persisted queries

48af902

graphqlerrors client fault, all other server fault

5e07f27

simplify and centralise parsing

f0220ca

respect graphql errors http extensions

4ff16f9

log

84aadff

internal error 500 only if on response hooks fail

64d589c

drop unused import

03b9b21

use published graphql-http

76e770b

ardatan reviewed Aug 12, 2022

View reviewed changes

enisdenjo marked this pull request as draft August 12, 2022 15:40

test adjustments

39fbe6f

ardatan reviewed Aug 12, 2022

View reviewed changes

yaacovCR mentioned this pull request Aug 12, 2022

Optional query discussion graphql/graphql-over-http#216

Open

dotansimha assigned enisdenjo Aug 14, 2022

enisdenjo added 12 commits August 16, 2022 11:35

Merge branch 'v3' into v3-graphql-http

e064869

more acceptable

96841e6

multipart is enabled if not explicitly disabled

ac26e6a

bump graphql-http and application/graphql-response+json

d909bab

fastify integration, multpartform request and TODO multipart response

352c572

request body can be function

1e9a658

"null" is valid json but not an object

d968a31

use status 413 for file upload size limit exceeded

19cc9a8

application/graphql-response+json is the default

7e371db

incremntal delivery support

1dae527

schema "might" be missing

5e85f85

bump graphql-http

547c356

enisdenjo added 2 commits August 17, 2022 12:44

simpler body parser

2a9c48c

Merge branch 'v3' into v3-graphql-http

db3aca3

# Conflicts: # packages/graphql-yoga/src/plugins/requestValidation/usePreventMutationViaGET.ts

theguild-bot mentioned this pull request Aug 20, 2022

Roadmap #1358

Closed

enisdenjo closed this Sep 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate graphql-http, refactor and comply with spec (v3) #1589

Integrate graphql-http, refactor and comply with spec (v3) #1589

enisdenjo commented Aug 12, 2022 •

edited

Loading

changeset-bot bot commented Aug 12, 2022 •

edited

Loading

ardatan left a comment •

edited

Loading

enisdenjo commented Aug 12, 2022

ardatan Aug 12, 2022

ardatan Aug 12, 2022

enisdenjo Aug 12, 2022

ardatan Aug 13, 2022

enisdenjo Aug 15, 2022

ardatan Aug 12, 2022

enisdenjo Aug 12, 2022

ardatan Aug 13, 2022

enisdenjo Aug 15, 2022

ardatan Aug 12, 2022

enisdenjo Aug 12, 2022

ardatan Aug 12, 2022 •

edited

Loading

enisdenjo Aug 12, 2022

ardatan Aug 12, 2022

enisdenjo Aug 12, 2022

ardatan Aug 13, 2022 •

edited

Loading

ardatan Aug 12, 2022 •

edited

Loading

enisdenjo Aug 12, 2022

ardatan commented Aug 12, 2022

ardatan Aug 12, 2022

enisdenjo Aug 12, 2022 •

edited

Loading

ardatan Aug 12, 2022

enisdenjo Aug 12, 2022 •

edited

Loading

ardatan Aug 12, 2022

benjie Aug 13, 2022

ardatan Aug 12, 2022

enisdenjo Aug 12, 2022

enisdenjo commented Aug 12, 2022

enisdenjo commented Aug 16, 2022

enisdenjo commented Sep 20, 2022

Integrate graphql-http, refactor and comply with spec (v3) #1589

Integrate graphql-http, refactor and comply with spec (v3) #1589

Conversation

enisdenjo commented Aug 12, 2022 • edited Loading

changeset-bot bot commented Aug 12, 2022 • edited Loading

⚠️ No Changeset found

ardatan left a comment • edited Loading

Choose a reason for hiding this comment

enisdenjo commented Aug 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardatan Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardatan Aug 13, 2022 • edited Loading

Choose a reason for hiding this comment

ardatan Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ardatan commented Aug 12, 2022

Choose a reason for hiding this comment

enisdenjo Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enisdenjo Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enisdenjo commented Aug 12, 2022

enisdenjo commented Aug 16, 2022

enisdenjo commented Sep 20, 2022

enisdenjo commented Aug 12, 2022 •

edited

Loading

changeset-bot bot commented Aug 12, 2022 •

edited

Loading

ardatan left a comment •

edited

Loading

ardatan Aug 12, 2022 •

edited

Loading

ardatan Aug 13, 2022 •

edited

Loading

ardatan Aug 12, 2022 •

edited

Loading

enisdenjo Aug 12, 2022 •

edited

Loading

enisdenjo Aug 12, 2022 •

edited

Loading