Non-GraphQL requests with 'query' in URL params fail with AbortError

[LukeTillman]

Describe the bug

We have a setup where we talk to both a GraphQL API and some REST APIs as well, thus our server instance has both types of request handlers. If you make a regular REST request with ?query=some-not-graphql-value in the URL params and your server also has GraphQL request handlers, the first GraphQL handler will attempt to parse the request, which will fail and cause the request to error out.

For us, this was manifesting as AbortError: Aborted on those requests because it looks like the interceptor library aborts any requests where middleware throws errors. The underlying error causing it to abort the requests is a GraphQL syntax error when trying to parse: GraphQLError: Syntax Error: Unexpected Name ....

We were getting this from GET requests, but after tracing through the code, I think the same is probably true if you tried it with POST and had query in the request body that was not a GraphQL query. We saw this behavior using the Node server in Jest unit tests.

Environment

• msw: 0.20.5
• nodejs: 14.4.0

To Reproduce

Steps to reproduce the behavior:

1. Create a server and set it to .use any GraphQL request handler (e.g. in Jest).
2. Create a test that sends a regular fetch GET request to a URL with ?query=not-graphql in the URL.
3. See request fail when GraphQL handler parsing fails.

Expected behavior

The request shouldn't fail in this case, although I'm unsure of what the correct fix is. It could be as simple as just doing a try...catch around parsing and then just returning null so that processing moves on to the next handler if parsing fails. If the "predicate" step (i.e. URL matching I think?) happened before parsing, it would probably be possible for us to workaround it by specifying a URL for our GraphQL endpoint (i.e. use graphql.link) so that those GraphQL handlers wouldn't try to parse the request at all.

Happy to help with a PR or however I can.

[kettanaito]

Hey, @LukeTillman. Thank you for reaching out.

You're right, currently graphql.* request handlers will attempt to parse any GET request with a query parameter or a POST request which body has a query key. With this, it's possible that they would attempt to parse a non-GraphQL API request and fail due to syntax errors.

In the case of an application that combines both REST and GraphQL API mocking, I'd highly recommend scoping the GraphQL handlers to an exact endpoint via graphql.link. URL matching is already integrated into endpoint-scoped GraphQL request predicate. As you cannot have both conventional REST and GraphQL requests handling on a single server route, this eliminates the parsing of unrelated requests.

import { graphql } from 'msw'

const api = graphql.link('https://api.backend.com/graphql')

That being said, I see that at the moment the parsing step precedes the URL matching step:

msw/src/graphql.ts

Lines 175 to 182 in 8a5f573

	predicate(req, parsed) {
	if (!parsed || !parsed.operationName) {
	return false
	}
	// Match the request URL against a given mask,
	// in case of an endpoint-specific request handler.
	const hasMatchingMask = matchRequestUrl(req.url, mask)

Which still makes such use cases as yours to happen. I think you are correct in assuming that we should handle parsing exceptions gracefully for both GraphQL and non-GraphQL requests. We may even consider doing this on the response retrieval step:

msw/src/utils/getResponse.ts

Lines 38 to 47 in 8a5f573

	.map<[RequestHandler<any, any>, any]>((requestHandler) => {
	// Parse the captured request to get additional information.
	// Make the predicate function accept all the necessary information
	// to decide on the interception.
	const parsedRequest = requestHandler.parse
	? requestHandler.parse(req)
	: null
	return [requestHandler, parsedRequest]
	})

Do you see any implications if we do so?

Fundamentally, there's an issue of finding a reliable way to determine a request is a GraphQL API request. I don't think that silencing the query parsing exception is the best solution, as that cuts off a debugging aspect. However, with the lack of a better way to distinguish between GraphQL and non-GraphQL requests that may be the most optimal decision.

[LukeTillman]

Hey @kettanaito, thanks for the quick response. I have two thoughts:

1. Agree that swallowing the parsing errors is not ideal. Definitely want to surface them somehow. It took me awhile to figure out what was happening since we were just seeing aborted queries.
2. I like the idea of propagating parsing errors back via the response. That's how a real GraphQL server handles situations like that, so I think that makes sense.

As far as I know, there really isn't any more reliable way to tell whether a request is meant as a GraphQL API request. I think if the you kept that logic (i.e. looking for a query parameter) as is, then the ideal situation might be:

1. Try to parse the request, but fail gracefully and capture the error.
2. Allow the predicate step to then run which with graphql.link would allow us to filter out requests that weren't meant to be handled via the URL.
3. If the predicate doesn't filter things out, propagate the parsing error via the response similar to what a regular GraphQL server would do.

For our GraphQL server (written in Go), parsing errors or malformed requests (e.g. querying a field that doesn't exist in GraphQL schema, etc.) usually return a 400 Bad Request along with a JSON payload that gives the error details.

[kettanaito]

Thank you for the feedback, @LukeTillman.

I like the idea of propagating parsing errors back via the response. That's how a real GraphQL server handles situations like that, so I think that makes sense.

That is a good idea, but without a reliable way to know if the request was to a GraphQL API, we won't know when to respond with the parsing error. We don't want to respond to a seemingly matching REST API request with a GraphQL parsing error, that may be confusing.

However, compared to the silent swallowing of the parsing error, it would be preferable to return a 400 response with a parsing error, surfacing the issue to the developer. Technically, we don't have a way to respond amidst the request handler execution at the moment. We'd have to consider if this is really needed, or another technical way to surface the parsing error.

[LukeTillman]

Yeah, agreed you don't want to return a parsing error for GraphQL to a REST request, but definitely would make it more obvious what's going on. You could also potentially "enhance" the error message that comes back in the payload to maybe give more information, maybe pointing to graphql.link to make sure GraphQL handlers are scoped to a particular endpoint.

If you wanted to make a larger change, long-term you could make using graphql.link mandatory so you always have a URL predicate and deprecate the "anonymously scoped" methods. Since you have to do that for REST handlers already, feels like maybe that would be OK, but since that would have a larger impact on users, I'm not sure if you want to consider going that route.

[LukeTillman]

@kettanaito Just saw the label and I'm happy to help contribute a PR if we've settled on an approach. It wasn't clear to me if we had, yet.

[msutkowski]

@LukeTillman I've just started on a PR over in #513 that touches some of these same things. I haven't handled this specific case yet as I just saw this issue, but perhaps both things can be addressed there. I ran into the exact issue that's being described here though, and left random thoughts about how things are processed as I was going.

I was thinking about the same process you're describing with enforcing link usage, because as of right now, I don't see a clear way to handle batches or differentiate between request types (graphql, rest, etc) when finding relevantHandlers. Any of your insight there would be appreciated as well, as I don't use graphql personally.

[pa3]

Hey, @kettanaito

That being said, I see that at the moment the parsing step precedes the URL matching step:

Does it make sense to break out the part of the predicate handler where the URL being matched and place it before the parsing step? So, that non-relevant handlers will be filtered out between existing filter and map here:

msw/src/utils/getResponse.ts

Lines 33 to 47 in 8a5f573

	const relevantHandlers = handlers
	.filter((requestHandler) => {
	// Skip a handler if it has been already used for a one-time response.
	return !requestHandler.shouldSkip
	})
	.map<[RequestHandler<any, any>, any]>((requestHandler) => {
	// Parse the captured request to get additional information.
	// Make the predicate function accept all the necessary information
	// to decide on the interception.
	const parsedRequest = requestHandler.parse
	? requestHandler.parse(req)
	: null
	return [requestHandler, parsedRequest]
	})

So that graphql.link will actually do the trick of avoiding the described bug? It also feels like the only reasonable way to deal with mixed graphql/rest setup without overloading parsing code with assumptions of possible input.

[kettanaito]

Hi, @pa3. Yes, that's a valid suggestion. We are currently reworking the request handler API (#561) and it will run the list of handlers exactly as you've described:

• handlers.filter(handler => handler.test(req)). Here graphql.link will do the URL matching first.
• Then relevant handlers will attempt to produce a response with handler.run() after.

With those changes, graphql.link should be able to resolve this issue. It's worth mentioning that .test() will parse first and test the predicate later anyway. That is the right execution order, as you may need additional information from parsing to conclude the predicate phase. It'd be up to the GraphQL request handler to ensure that any given endpoint URL is matched first, before parsing, as you don't need to parse queries destined to unspecified endpoints.

[pa3]

Sounds awesome! Thanks a ton for explanation, @kettanaito!