Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: skip parsing GraphQL requests if the request url doesn't match #1871

Merged
merged 6 commits into from
Nov 24, 2023

Conversation

mattcosta7
Copy link
Contributor

When we have a specified graphql endpoint, and that endpoint doesn't match we don't need to parse the request at all. This avoids weird console.errors that may apply when the user mixes rest and d graphql handlers. This fix cannot be easily applied to the base graphql handler since it doesn't have a dedicated endpoint, but that's a decent tradeoff for now, as it provides a way to better scope those errors and avoid user confusion in the short term.

Maybe we should consider forcing graphql calls to link a specific endpoint prior to handling which would avoid this issue

@kettanaito curious for thoughts on this type of thing.

It would help me cleanup some code in a work related project, if we didn't have to add new overrides for the graphql cases, and could just avoid matching on the endpoint if graphql.link was given

@mattcosta7
Copy link
Contributor Author

Will come back to this in a little bit to cleanup the tests - want to add a bit more validation to them that both the graphql and http handlers work well, but my laptop died before I could push and I don't have a power cable with me

My use case I'm optimizing for is when using graphl.link and mixing http request with body parameter 'query' with graphql queries, in a test suite that will fail on calls to console in tests (except where explicitly expected)

@mattcosta7 mattcosta7 marked this pull request as ready for review November 17, 2023 21:48
@kettanaito
Copy link
Member

Hi, @mattcosta7. Thanks for opening this.

As an optimization route, I think this has a place to be. From the RequestHandler perspective, the parsing phase always happens before the predicate phase since it provides the predicate with additional information (such as HTTP request params and cookies and GraphQL request query and variables). I understand that in cases when we know the exact endpoint, we may want to short-circuit the parsing phase if we know the endpoint doesn't match. But we have to be careful about decisions like that so the promise we establish in RequestHandler remains true.

Overall, I wrote about the Request handler phases and I think it's a good read in the context of this change.

So, with the proposed change, we will kind of do the predicate before parsing to opt-out of parsing if the actual request URL doesn't match the GraphQL handler's endpoint. As things are right now, this does make sense but let's also consider #1804, which I'd like to ship rather soon to allow people to have custom request predicate functions. It looks like in that context, opting out from the parsing phase before the predicate phase won't be possible (as we don't know what the user-provided predicate may be for a GraphQL request handler).

For example, consider a use case where I want to create a request handler for GraphQL requests that contain a user field in their query:

// Note, this already features a yet non-existing 
// custom predicate API.
graphql.query(({ query }) => query.includes('user'), resolver)

This highlights the need for the parsing (query) to happen before the predicate (query.includes('user')).

But I think it shouldn't be a problem due to the nature of endpoint-based GraphQL interception:

  • For handlers like graphql.query and graphql.mutation (that are endpoint-less), the preemptive URL matching will always pass (since their URL predicate is literally /.+/).
  • For endpoint-scoped handlers created via graphql.link(endpoint), the user already has the expectation of: 1) the endpoint must match; 2) then, my custom predicate must match. So it looks safe to ignore the custom predicate (i.e. parsing) if the endpoint doesn't match.

Do you have any thoughts about this given this additional info?

@kettanaito kettanaito changed the title fix(graphqlhandler): avoid parse error when req url doesn't match fix(graphql): skip parsing if request url doesn't match Nov 17, 2023
@mattcosta7
Copy link
Contributor Author

Thanks for the discussion here! Traveling for the rest of the day and much of the next week but I'll get more thoughts here soon

@mattcosta7
Copy link
Contributor Author

Hi, @mattcosta7. Thanks for opening this.

As an optimization route, I think this has a place to be. From the RequestHandler perspective, the parsing phase always happens before the predicate phase since it provides the predicate with additional information (such as HTTP request params and cookies and GraphQL request query and variables). I understand that in cases when we know the exact endpoint, we may want to short-circuit the parsing phase if we know the endpoint doesn't match. But we have to be careful about decisions like that so the promise we establish in RequestHandler remains true.

Yep, this makes sense! I've slightly modified this implementation to handle the match on the url as part of the parse method, and return it from there, a bit more similar to how the HttpHandler does this. In the event that there's an endpoint provided via graphql.link we'll avoid request parsing, because we've determined that the handler doesn't match already, so there's no need to continue with the graphql parsing attempt, since. we can't match. As you mentioned later, this has no impact on endpoint-less graphql requests (I'm curious a bit as to why we'd need endpoint-less requests, given that every graphql server must live somewhere on a network - but that's not something we need to deal with at this point)

Overall, I wrote about the Request handler phases and I think it's a good read in the context of this change.

So, with the proposed change, we will kind of do the predicate before parsing to opt-out of parsing if the actual request URL doesn't match the GraphQL handler's endpoint. As things are right now, this does make sense but let's also consider #1804, which I'd like to ship rather soon to allow people to have custom request predicate functions. It looks like in that context, opting out from the parsing phase before the predicate phase won't be possible (as we don't know what the user-provided predicate may be for a GraphQL request handler).

For example, consider a use case where I want to create a request handler for GraphQL requests that contain a user field in their query:

// Note, this already features a yet non-existing 
// custom predicate API.
graphql.query(({ query }) => query.includes('user'), resolver)

This highlights the need for the parsing (query) to happen before the predicate (query.includes('user')).

But I think it shouldn't be a problem due to the nature of endpoint-based GraphQL interception:

  • For handlers like graphql.query and graphql.mutation (that are endpoint-less), the preemptive URL matching will always pass (since their URL predicate is literally /.+/).
  • For endpoint-scoped handlers created via graphql.link(endpoint), the user already has the expectation of: 1) the endpoint must match; 2) then, my custom predicate must match. So it looks safe to ignore the custom predicate (i.e. parsing) if the endpoint doesn't match.

Do you have any thoughts about this given this additional info?

Yep! I think this is correct.

For 'endpoint-less' queries, the custom predicate approach should still be valid, and not impact much else, since we'll only skip parsing the request body once the request cannot match - and with those, they will always match.

I don't think it would make sense that a handler defining an endpoint (via link in this case) would then want a custom predicate that totally ignored it, but instead only be called for requests to that endpoint

* which simplifies the return type of the resolver
* when the request is to a non-matching endpoint
*/
| {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could also nest instead of merging match and these properties. I don't have a strong preference, besides not wanting to name the object containing these here.

it could be graphqlAttributes or parsedGraphqlRequest but this is already a parsed result, so it feels a bit weird

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as the existing typings tests pass, I have no objection how we handle this.

@kettanaito
Copy link
Member

I don't think it would make sense that a handler defining an endpoint (via link in this case) would then want a custom predicate that totally ignored it, but instead only be called for requests to that endpoint

Agree. That'd be a no-op and intention conflict.

When we have a specified graphql endpoint, and that endpoint doesn't match we don't need to parse
the request at all.  This avoids weird console.errors that may apply when the user mixes rest and d
graphql handlers. This fix cannot be easily applied to the base `graphql` handler since it doesn't
have a dedicated endpoint, but that's a decent tradeoff for now, as it provides a way to better
scope those errors and avoid user confusion in the short term.

Maybe we should consider forcing graphql calls to `link` a specific endpoint prior to handling which
would avoid this issue
@mattcosta7 mattcosta7 force-pushed the msw-avoids-logging-graphql-parse-with-link branch from cf15442 to e44a35b Compare November 19, 2023 04:19
@mattcosta7 mattcosta7 changed the title fix(graphql): skip parsing if request url doesn't match fix(graphql): skip gql parsing if request url doesn't match Nov 19, 2023
Copy link
Member

@kettanaito kettanaito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No objections from my side. Superb work 👏

@kettanaito
Copy link
Member

One thing, don't merge this yet. I'd like to pair it with #1865.

@mattcosta7
Copy link
Contributor Author

One thing, don't merge this yet. I'd like to pair it with #1865.

Sounds good! Feel free to merge whenever it makes sense to!

@kettanaito kettanaito changed the title fix(graphql): skip gql parsing if request url doesn't match fix: skip parsing GraphQL requests if the request url doesn't match Nov 21, 2023
@kettanaito kettanaito merged commit 565e802 into main Nov 24, 2023
10 checks passed
@kettanaito kettanaito deleted the msw-avoids-logging-graphql-parse-with-link branch November 24, 2023 16:18
@kettanaito
Copy link
Member

Released: v2.0.9 🎉

This has been released in v2.0.9!

Make sure to always update to the latest version (npm i msw@latest) to get the newest features and bug fixes.


Predictable release automation by @ossjs/release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants