graphql-tools

graphql-tools copied to clipboard

Issue workflow progress

Progress of the issue based on the Contributor Workflow

• [ ] 1. The issue provides a reproduction available on Github, Stackblitz or CodeSandbox

  Make sure to fork this template and run yarn generate in the terminal.

  Please make sure the GraphQL Tools package versions under package.json matches yours.

• [ ] 2. A failing test has been provided
• [ ] 3. A local solution has been provided
• [ ] 4. A pull request is pending review

---

Describe the bug

buildHTTPExecutor with retry option unset(null) is returning the non 2XX response as it is without converting it into a GraphQLError object. This cause the uses of delegateToSchema to return null value instead of a GraphQLError when subschema return 500 error. Currently, the non 2XX status response check is only done if retry is set, which I think is unnecessary. https://github.com/ardatan/graphql-tools/blob/8676f5456f13ed50478cb249cfb25e6483b12f4a/packages/executors/http/src/index.ts#L202-L205

To Reproduce Steps to reproduce the behavior:

    const executor = buildHTTPExecutor({
      fetch: (_url, init) => new Response(JSON.stringify({ data: init }), { status: 500 }),
    });
    const result = await executor({
      document: parse(/* GraphQL */ `
        query {
          hello
        }
      `),
    });

The current implementation result is

{
  data: {
    body: '{"query":"{\\n  hello\\n}"}',
    headers: {
      accept: 'application/graphql-response+json, application/json, multipart/mixed',
      'content-type': 'application/json',
    },
    method: 'POST',
  },
}

Expected behavior

I would expect it to be returning a GraphQLError by throwing an error upon non-2XX status response regardless of the retry option value. The error will then be caught by the existing catch clause. https://github.com/ardatan/graphql-tools/blob/8676f5456f13ed50478cb249cfb25e6483b12f4a/packages/executors/http/src/index.ts#L242-L322

Environment:

• OS: MacOS Ventura 13.2.1
• @graphql-tools/...: packages/executors/http/
• Deno: v1.34.3

Additional context

If the issue is reasonable, I could help by opening a PR.

If the issue is reasonable, I could help by opening a PR.

Thanks for reporting the issue! We'd love to accept a PR with a failing test!

I think I misunderstood the issue in the beginning. I think you are missing the case that a response can have a valid result with data and/or errors with a status code. In that case, your PR breaks this behavior and returns an error no matter what the actual response contains.

a response can have a valid result with data and/or errors with a status code. In that case, your PR breaks this behavior and returns an error no matter what the actual response contains.

I see, it makes sense. However, I think the current code will also break this behavior if buildHttpExecutor() is called by setting the retry to any number other than 0, am I right?

The check for retry should check if it is greater than 0. You're right.

Great. Also, I would say that the status code check should be verifying if it starts with 5 for http error instead of not starting with 2. Could you explain what does the Retry should respect HTTP Errors comment means? Does it mean that it should not retry if http error occurs?

https://github.com/ardatan/graphql-tools/blob/8676f5456f13ed50478cb249cfb25e6483b12f4a/packages/executors/http/src/index.ts#L202-L205

Also, I would say that the status code check should be verifying if it starts with 5 for http error instead of not starting with 2.

I am not sure with that. It is up to user because user can still want to pass some GraphQL data. If you want to throw an error in case of 5xx, you can pass a fetch function that throws in a 5xx response.

Retry should respect HTTP Errors

It means the executor should retry if the status code is not 2xx.

It is up to user because user can still want to pass some GraphQL data. If you want to throw an error in case of 5xx, you can pass a fetch function that throws in a 5xx response.

Yes, I totally agree with you here. Maybe I am missing something here, but the current implementation will throw an error and essentially return a GraphQLError when the status code is not 2xx after exhausting all the retry attempts. Does that not violate the idea of allowing GraphQL data to pass through regardless of the status code?

Maybe it should be checking if the retry attempts have been exhausted. If so, json parse and return the fetched result regardless of the status code.

@ardatan

checking if the retry attempts have been exhausted. If so, json parse and return the fetched result regardless of the status code.

I have added a failing test to my PR about my statement above. https://github.com/ardatan/graphql-tools/pull/5372/files

Kindly check it out when you have time :)

Closing this because it has becomes stale for some time.