ardatan / graphql-tools

:wrench: Utility library for GraphQL to build, stitch and mock GraphQL schemas in the SDL-first approach
https://www.graphql-tools.com
MIT License
5.36k stars 818 forks source link

Stitching schema with wrapSchema seems swallowing or not passing through the error #6477

Open inpercima opened 3 months ago

inpercima commented 3 months ago

Issue workflow progress

Progress of the issue based on the Contributor Workflow


Describe the bug

We have the need to combine several remote schemas into one. We use stitching for this. For the basis and understanding of the following code: We need both http and ws functionalities and authentication. We have a Graphql-Playground activated.

Basically, everything works, but if an error and data are returned from the remote schema, the error is no longer present in various constellations after the wrapSchema or shortly before, although the query from the remote schema does contain the error in between.

To Reproduce Steps to reproduce the behavior:

The base: To handle ws subscriptions and query the rest over http we use hybrid executor to use WS for subscriptions

Also we use Introspecting Schemas using Executors

Stitching: We stitch a local schema and many other remote schemas to one and use for this stichSchemas and wrapSchema.

Full code: The most of the code is more described in the links above.

/**
   * Stitches the local schema and the schemas from remote APIs to one new schema.
   * @see https://the-guild.dev/graphql/stitching/docs/getting-started/remote-subschemas
   */
  private async stitch(localSchema: GraphQLSchema): Promise<GraphQLSchema> {
    const remoteSchemas = await Promise.all(
      // remoteApis includes the URL to the services
      remoteApis().map(async (remoteApi: RemoteApi) => this.createRemoteSchema(remoteApi)),
    );

    return stitchSchemas({
      subschemas: [localSchema, ...remoteSchemas.filter(Boolean)],
    });
  }

  /**
   * Fetches the schema from a remote service and
   * wrap it with transformations (renaming) in name and type to a new schema.
   */
  private async createRemoteSchema(remoteApi: RemoteApi): Promise<GraphQLSchema> {
    try {
      const httpExecutor: AsyncExecutor = async ({ document, variables, operationName, extensions }) => {
        const query = print(document);
        const fetchResult = await fetch(remoteApi.url, {
          method: 'POST',
          headers: {
            /**
             * @see https://chillicream.com/docs/hotchocolate/v13/migrating/migrate-from-12-to-13#http-transport
             */
            Accept: 'application/json',
            Authorization: this.authHeaderToken,
            'Content-Type': 'application/json; charset=utf-8',
          },
          body: JSON.stringify({ query, variables, operationName, extensions }),
        });
        return fetchResult.json();
      };

      /**
       * @see https://the-guild.dev/graphql/stitching/docs/getting-started/remote-subschemas#create-a-hybrid-executor-to-use-ws-for-subscriptions
       * @see https://stackoverflow.com/questions/75987086/apollo-server-subscription-middleware-to-intercept-request-body-to-and-request-s
       */
      const subscriptionClient = remoteApi.ws
        ? createClient({
            /**
             * The webSocketImpl is necessary to work.
             *
             * @see https://stackoverflow.com/questions/72116940/apollo-graphql-graphqlwslink-subscriptions-troubles-cannot-get-websocket-imp
             * @see https://the-guild.dev/graphql/ws/recipes#client-usage-in-node-with-custom-headers-not-possible-in-browsers
             */
            webSocketImpl: WebSocket,
            url: remoteApi.ws,
            lazyCloseTimeout: 50000,
            shouldRetry: () => true,
            connectionParams: async () => {
              return {
                Authorization: this.authHeaderToken,
                Accept: 'application/json',
              };
            },
            lazy: true,
            /**
             * onNonLazyError is used if lazy is set to false
             */
            // eslint-disable-next-line @typescript-eslint/no-empty-function
            onNonLazyError: () => {},
            on: {
              connected: () => {
                console.debug(`graphql-ws connected`);
              },
              error: err => console.log(err),
            },
          })
        : ({} as Client);

      const wsExecutor: AsyncExecutor = remoteApi.ws
        ? async ({ document, variables, operationName, extensions }) =>
            observableToAsyncIterable({
              subscribe: observer => ({
                unsubscribe: subscriptionClient.subscribe(
                  {
                    query: print(document),
                    variables: variables as Record<string, any>,
                    operationName,
                    extensions,
                  },
                  {
                    next: data => observer.next?.(data as any),
                    error(err) {
                      if (!observer.error) return;
                      if (err instanceof Error) {
                        observer.error(err);
                      } else if (Array.isArray(err)) {
                        observer.error(new Error(err.map(({ message }) => message).join(', ')));
                      } else {
                        observer.error(new Error(`Socket closed with event: ${err}`));
                      }
                    },
                    complete: () => observer.complete?.(),
                  },
                ),
              }),
            })
        : ({} as AsyncExecutor);

      const executor: AsyncExecutor = async executorRequest => {
        // subscription operations should be handled by the wsExecutor
        if (remoteApi.ws && executorRequest.operationType === 'subscription') {
          return wsExecutor(executorRequest);
        }
        // all other operations should be handles by the httpExecutor
        return httpExecutor(executorRequest);
      };

      return wrapSchema({
        schema: await schemaFromExecutor(executor),
        executor: executor,
        transforms: [
          new RenameTypes(type => remoteApi.prefix + type, { renameBuiltins: false, renameScalars: false }),
          new RenameRootFields((operation, name) => remoteApi.prefix + name),
        ],
      });
    } catch (error) {
      this.logger.error(`failed connecting '${remoteApi.name}'`);
      return error;
    }
  }

While debugging and logging I can see on executing httpExecutor(executorRequest) that all data we need, error and data is given.

// all other operations should be handles by the httpExecutor
const debug = httpExecutor(executorRequest);
debug.then(result => console.log(result));
return httpExecutor(executorRequest);

Behavior A: If an error is given and the data is filled with an element which is null, the result with error is given in httpExecutor and Graphql-Playground.

// logging from httpExecutor section
{
    errors: [ { message: 'Specified key "X" is already used.' } ],
    data: { createProduct: null }
}
// logging from Graphql-Playground
"errors": [
    {
        "message": "Specified key \"X\" is already used.",
        "path": [
             "createProduct"
        ],
        ...
    }
],
"data": {
    "createProduct": null
}

Behavior B: If and error is given and the data is filled with an element which is not null, the result with error is given in the httpExecutor only and not passed through to the Graphql-Playground or other clients connecting to the server.

// logging from httpExecutor section
{
    errors: [ { message: 'Specified key "PO" is already used.' } ]
    data: { updateProduct: { id: 'a1e12327-6456-43df-b789-b4ab32d23012' }
}
// logging from  Graphql-Playground
"data": {
    "updateProduct": {
        "id": "a1e12327-6456-43df-b789-b4ab32d23012"
    }
}

So the question is, what happen in wrapSchema or schemaFromExecutor that the error is swallowing or not passing through?

Expected behavior

The error should always present even if data is given.

Environment:

Additional context

I found some other older issues (2020) related to this and some resolutions working with the problem but these all 4 years old and I hope there is another solution:

We tested now the transformResult and this seems to help but I don't think that should be necessary, right? Or is this the way do handle the error?

ardatan commented 3 months ago

Thanks for creating the issue. Would you create a PR with a failing test?

inpercima commented 3 months ago

@ardatan Thank you for your feedback. I'll see that I create a simple demonstration and consider how to create a failed test for this one.