Add support for client that uses GET as transport mechanism

[salman-rb]

Current implementation always uses POST as the transport mechanism. Adding GET support enables usage of GET queries for caching simply via URL.

Some notes:

• I left the existing API for creating a new client as is, but the implementation could be much cleaner by introducing some sort of configuration struct when creating a new client
• The construction of the query parameters follows the logic from Apollo's client implementation, which can be found here https://github.com/apollographql/apollo-client/blob/8beb4820edc6352996e08f7f73bde3573f1eb666/src/link/http/rewriteURIForGET.ts
• Updated integration tests to use both sets of clients. Updating the tests to use a test suite would be cleaner

I have:

• Written a clear PR title and description (above)
• Signed the Khan Academy CLA
• Added tests covering my changes, if applicable
• Included a link to the issue fixed, if applicable
• Included documentation, for new features
• Added an entry to the changelog

[csilvers]

The code looks right to me!, though I'm not one of the genqlient experts. Your changes are clear and make a lot of sense.

I think the main issue here is more philosophical, about whether we want to expose this at all. It's really easy for variables to include personal information, such as passwords, and exposing those via GET -- where they're logged, if nothing else -- is potentially problematic. There are certainly legit uses for graphql over GET, but I'm worried this introduces an easy-to-use footgun. But arguably it's not our job as library authors to dictate that kind of thing.

[csilvers]

It's probably worthwhile to give an example of what this sends over the wire to the server, since it's not obvious what the server would need to do to handle such a request -- unless there's a standard for how to do graphql over GET? I know there's a standard for POST but I don't know about GET. (If there is a standard for GET, it may be worth linking to it.)

[salman-rb]

Its here, will link to it

https://graphql.org/learn/serving-over-http/#get-request

[benjaminjkraft]

Yeah, it's semi-standard. (The official spec has almost nothing about wire format, but the graphql-js and Apollo implementations are effectively the standard.) May as well include the link but in practice I think most servers will either support it or not.

[salman-rb]

Didn't link directly (not sure if this will be stable) but described the format in the FAQ

[csilvers]

I don't think this sentence is accurate for GET -- it's not using JSON in this case. This is a great place to describe what query params you pass to the server.

[salman-rb]

Good call out, will update

[salman-rb]

Updated. Left the description on query params in the FAQ. The code is fairly easy to follow if someone is curious on what the query parameters are exactly. I tend to think someone would care about this if they are using a graph server that doesn't support GET queries

[salman-rb]

The code looks right to me!, though I'm not one of the genqlient experts. Your changes are clear and make a lot of sense.

I think the main issue here is more philosophical, about whether we want to expose this at all. It's really easy for variables to include personal information, such as passwords, and exposing those via GET -- where they're logged, if nothing else -- is potentially problematic. There are certainly legit uses for graphql over GET, but I'm worried this introduces an easy-to-use footgun. But arguably it's not our job as library authors to dictate that kind of thing.

Definitely agree with your concerns. I believe the mechanism should be included given that its part of the graphql specification and there are good use cases for it. Maybe there is an opportunity to make it more specific at request time that you are using GET as the transport mechanism to avoid the foot gun, as where the client is first created might be different than where it gets used. I believe this would require changing some of the public APIs to include the request method, will wait for more feedback before making a change like that

[benjaminjkraft]

Thanks. I think this makes sense to include; while users can bring their own client this one is probably common enough it's worth implementing it once, correctly, so everyone can use.

One question is whether it's a good idea to allow GET to be used for mutations. I would think if you're doing that you're really in for trouble, whether as Craig says via logging your passwords, or simply in something not knowing the request isn't idempotent. If someone really wants the foot-gun, they can just copy the implementation and remove the check. Since the client has access to the query you can implement this by just checking if the first word (after whitespace) is "query" or "mutation" (I believe genqlient will never put a comment before the query it sends to the server, although that's perhaps slightly fragile; if it's ever an issue we could give the client the operation type or even the entire parsed query). We'd want to mention that in the documentation -- actually it's probably a good idea as a recommendation even if we don't require it.

FWIW I actually like having two separate client constructors, as it is now. It's possible it will become unwieldy, but I feel they're actually separate methods of serializing GraphQL over HTTP, not just two different request methods.

Lastly, I think you still need to sign the CLA? Or at least, you didn't check the checkbox :-) .

[salman-rb]

One question is whether it's a good idea to allow GET to be used for mutations. I would think if you're doing that you're really in for trouble, whether as Craig says via logging your passwords, or simply in something not knowing the request isn't idempotent. If someone really wants the foot-gun, they can just copy the implementation and remove the check. Since the client has access to the query you can implement this by just checking if the first word (after whitespace) is "query" or "mutation" (I believe genqlient will never put a comment before the query it sends to the server, although that's perhaps slightly fragile; if it's ever an issue we could give the client the operation type or even the entire parsed query). We'd want to mention that in the documentation -- actually it's probably a good idea as a recommendation even if we don't require it.

Updated to fail when a mutation is passed, agree that its pretty easy to copy paste and remove the check. Also added tests to cover the mutation query in both client versions.

FWIW I actually like having two separate client constructors, as it is now. It's possible it will become unwieldy, but I feel they're actually separate methods of serializing GraphQL over HTTP, not just two different request methods.

Fair enough, might be something to look out for if there are other customizations

Lastly, I think you still need to sign the CLA? Or at least, you didn't check the checkbox :-) .

Yup working on getting my employer (https://github.com/rainbow-me) to sign for the org. Hopefully can continue to help drive improvements as we start using this library. Looking at caching might be next on my list

[benjaminjkraft]

Great! Looks good to merge as soon as the CLA is sorted, let us know if you need anything from us on that!

[salman-rb]

Great! Looks good to merge as soon as the CLA is sorted, let us know if you need anything from us on that!

Should be sorted! Let me know if there are any issues