ShopifySharp
ShopifySharp copied to clipboard
nozzlegear
Seeking input on improved GraphQL support
Hey everyone! Last week I was contacted by a developer at Shopify who's been reaching out to various community-maintained Shopify packages, advocating for increased support for the GraphQL API. While ShopifySharp does have basic support for GraphQL, I'd love to hear some thoughts on how we can make this work and feel better in C#. If anyone has used a .NET GraphQL package that had a great experience, please let me know!
One of my primary goals with ShopifySharp is to give as much security around Shopify objects as possible. With the rest API, that means modeling each object with classes in C#, deciding the best types for certain properties, and so on. It's my understanding that with GraphQL, the developer can specify exactly what kind of object they want to receive (and send). So if you only need an order's total value, you can just specify that property and receive an object that looks like { totalValue: ... }. The Order class itself is unnecessary in this situation, since all of the properties except totalValue will be empty.
My big question: how can we provide intellisense and strong typing on the data that gets returned, when that object could be any shape the developer sees fit? Do we just make the method generic and have the developer come up with their own classes?
I wrestled with a lot of different scenarios while creating the basic support. Every solution I thought of was a balance of tradeoffs.
Either we can have an absolute ton of stuff, which is hard to name and maintain (and understand, to some extent). Or, we can do very little in the way of intellisense/objects/the actual cool features of .Net, which is essentially what we have now. FWIW, I am using what we have now in several production projects and it works great, but I have built some custom logic for myself on top of it to make it more easily reusable within a project.
I'm honestly just not sure that GraphQL is a great fit for .Net on the face of it, since .Net essentially stands for objects, and graphql is a force clearly against defined objects.
That being said, people do seem to be trying to smash the two together. I recommend taking a look here: https://graphql-dotnet.github.io/docs/getting-started/introduction
These guys are the furthest along, but you will likely at least some of the same feelings I have, that REST is a much better/more natural match.
Your idea on Generics/allowing people to specify the output would theoretically work, and would be fairly easy to add, but i'm not totally sure it would provide a ton of benefit.
I hate to sound like a downer, because I actually see huge potential here, its just the idea behind Graph is much more suited to languages like JS where objects are more fluid rather than static.
For those familiar, a good analog in the .Net world would be:
GraphQL is to SqlClient as REST is to EntityFramework.
I will also note that even Shopify themselves have not integrated graph into their python library: https://github.com/Shopify/shopify_python_api
I also don't think it's a good idea to create a bunch of entity classes. The GraphQL queries are too flexible to try to do that. Intellisense is provided already by GraphiQL instead of visual studio. You can design/test your queries in GraphiQL and then copy them into your c# code.
That being said, I think we can make it better. Here are some ideas. Note that I'm focusing on queries mostly. I haven't thought much at all about mutations.
The current GraphService expose two overloads:
Task<JToken> PostAsync(string body)
Task<JToken> PostAsync(JToken body)
Here is what I'm thinking in a nutshell
First define these classes:
GraphRequest
{
string Query;
Variable[] Variables;
}
GraphResponse<T>
{
T Data;
RateLimitInfo RateLimitInfo;
}
RateLimitInfo
{
int RequestedQueryCost
etc...
}
Variable
{
string Name;
object Value; //scalar, enum or input type https://graphql.org/learn/schema/#input-types
}
Then add this overload:
Task<GraphResponse<T>> PostAsync(GraphRequest<T>)
I would also like to be able to pass T = dynamic if I don't want to create a c# class and would rather work with a dynamic object response (or a JToken).
Finally, I also want to enhance the current execution policies to work with the graphql leaky bucket and limits, which are separate to the REST limits.
Thoughts?
By the way I'm happy to submit a PR for this but waiting for your feedback first...
Thoughts:
- The reason why I didn't wrap both sides was to mimic the REST side, where it is also unwrapped on the back side. If we want to diverge from that, we can, or you could wrap and implement on the backside to accomplish your rate limiter.
- I don't think it does much for my personal uses to wrap or unwrap, so I am pretty ambivalent to the change, besides a concern that it may be confusing with 3 overloads. The variables piece could be helpful though.
- I looked over how graphql-dotnet does it and I'm not too sold on that either.
- RE: Entities...I agree on too many permutations.
- RE: dynamic: seems possible to do, or with anonymous objects...I'd like to somehow find an anonymous objects silver bullet to solve all of this, but can't think of one.
Has anyone built a graphql API in Asp.net? It seems like that may be educational to know how they handle it.
I don't see any avoiding having a request type, so that variables can be passed. On the response side, it is not important for REST because all queries have the same cost. In graphql, the cost varies per request and it can be useful to access the cost programmatically. (For example I might build a query dynamically, and change it if it turns out it was rejected because it was too costly)
If you prefer not to use the request/response wrapper and don't need touse variables, then you can always use the current simpler overload. I personally don't mind having several overloads.
graphql-dotnet seems to be more designed for building graphql server. In our case we are designing a client only, so it shouldn't need to be as complicated.
Well, the request side is fairly simple on REST, but sure, we do have several <Options>.
I totally wanted the cost exposed originally, but didn't to mimick REST, but through use I haven't needed it as of yet, I just run a query and get the full response before committing the query to code for production. I don't think it would hurt anything to expose it though, and it would make the error handling easier as well (if we so chose).
I think overloads are fine too, was just calling it out.
GraphQL-Dotnet has both client and server, here is how the client works: https://github.com/graphql-dotnet/graphql-client It's actually pretty similar to what you are proposing, they even use dynamic as the response type inside their wrapper, and have a Cast (GetDataFieldsAs) as well. The main difference is a string wrapper for the query. If you haven't taken a peek, you definitely should before embarking on code. I went the JToken route because it's closer to the backend of the REST side, but open to whatever works.
I really do wish that query object could somehow be emulated by a .Net Object...that would solve SO much.
Oh I see, very similar indeed. I think I had seen it but forgot about it. It is a very simple package. I'd be tempted to take a dependency on it (https://www.nuget.org/packages/GraphQL.Client/)
I really do wish that query object could somehow be emulated by a .Net Object...that would solve SO much.
Maybe for simple plain queries there might be a clever way to make this work, but it would still break down for variables, fragments, function calls, directives, etc... I really don't think it can be done. Example:
query Hero($episode: Episode, $withFriends: Boolean!) {
hero(episode: $episode) {
name
friends @include(if: $withFriends) {
name
}
}
}
Yes, the advanced GraphQL guy who asked me to implement the JToken overload suggested it. It's a pretty light little package with no real dependencies of it's own, so not losing much by doing so.
I think the only advantage to your style approach is a few shopify specific things, but we may be able to accomplish those and use this package together.
I really do wish that query object could somehow be emulated by a .Net Object...that would solve SO much.
Maybe for simple plain queries there might be a clever way to make this work, but it would still break down for variables, fragments, function calls, directives, etc... I really don't think it can be done. Example:
query Hero($episode: Episode, $withFriends: Boolean!) { hero(episode: $episode) { name friends @include(if: $withFriends) { name } } }
Agreed. I think in order to get intellisense, we would need to somehow use Roslyn, which is a bit outside where I normally code. So close, but yet so far...
I think the only advantage to your style approach is a few shopify specific things, but we may be able to accomplish those and use this package together.
Looking into this, it could be a problem to retrieve the query cost. Their graphql response is non virtual and contains only 'Errors' and 'Data'. How will we add the additional 'Extensions' object that contain the query cost?
The 'Extensions' is actually in the spec, so they may be willing to add it to their package. https://graphql.github.io/graphql-spec/June2018/#sec-Response-Format
I raised an issue on their repo https://github.com/graphql-dotnet/graphql-client/issues/120
Good steps. Even if we have to fork, likely worth it.
Derek Kershner
From: Clement Gutel [email protected] Sent: Tuesday, May 28, 2019 8:02:39 PM To: nozzlegear/ShopifySharp Cc: Derek Kershner; Comment Subject: Re: [nozzlegear/ShopifySharp] Seeking input on improved GraphQL support (#366)
I raised an issue on their repo graphql-dotnet/graphql-client#120https://github.com/graphql-dotnet/graphql-client/issues/120
— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/nozzlegear/ShopifySharp/issues/366?email_source=notifications&email_token=AGE2OG3MH6RCFUTIKYIXKELPXXW47A5CNFSM4HPR5W5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWOARRQ#issuecomment-496765126, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AGE2OG3INU5L6LQNZ447ZLDPXXW47ANCNFSM4HPR5W5A.
I've been thinking about leveraging C# to specify the shape of the response and it turns out there is a really neat solution, that is already implemented in Json.Net. The idea is to specific the response via an anonymous type.
E.g:
JsonConvert.DeserializeAnonymousType(someJson, new
{
id = 0,
name = "",
channel = new
{
name = ""
}
});
I like this! We could have an overload like the following in the GraphService:
Task<GraphResponse<T>> PostAsync<T>(GraphRequest request, T anonymousObject)
That will work for arrays as well. That should be trivial to implement since we can just delegate all the tricky reflection work to Json.Net
It's a change that would have to be made in graphql-dotnet/graphql-client though (assuming we decide to use it). I'm a little bit worried that there is not much activity on this project..
Might be worth considering forking it to add the Extensions and the anonymous object overloads.
I'm liking that a little better, more graph like implementation of variables. Still not the golden bullet of 'query', but I think it's the best variables implementation, and trivial as you state, and uses pre existing dependency.
I'd be surprised if that client wasn't used much, but maybe it really is just the server that's popular.
Derek Kershner
From: Clement Gutel [email protected] Sent: Thursday, May 30, 2019 4:42:03 AM To: nozzlegear/ShopifySharp Cc: Derek Kershner; Comment Subject: Re: [nozzlegear/ShopifySharp] Seeking input on improved GraphQL support (#366)
@nozzlegearhttps://github.com/nozzlegear any feedback?
— You are receiving this because you commented. Reply to this email directly, view it on GitHubhttps://github.com/nozzlegear/ShopifySharp/issues/366?email_source=notifications&email_token=AGE2OG4AGC2VGFHLUN6NQDTPX64QXA5CNFSM4HPR5W5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWSDSRY#issuecomment-497301831, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AGE2OG7R7RI5PRSC7AYAXHTPX64QXANCNFSM4HPR5W5A.
To clarify, this is NOT for the query and variables. This is just to provide the type of the response without having to create a class. This is useful because many queries will be adhoc and it would be tedious to have to a create a class for each response type.
The vales are meaningless and are just used for type inference. Maybe it is more clear to use it like this:
JsonConvert.DeserializeAnonymousType(someJson, new
{
id = default(int),
name = default(string),
channel = new
{
name = default(string)
}
});
Thanks for the feedback everyone! As soon as I get a chance to read through everything here and take a look at the .NET GraphQL lib I'll provide my own feedback as well.
Just to add a little more context to the conversation, Shopify announced three new APIs at Unite that are GraphQL-only -- there is no REST implementation of them. Those are the Media API, Order Editing API, and Delivery Profiles API.
I got tired of waiting for https://github.com/graphql-dotnet/graphql-client (the build has been failing for several weeks and PRs are queueing up). That led me to create a very simple graphql client here: https://github.com/better-reports/graphql-sharp Critically, it supports strongly typed responses and extension objects, which are required to implement the leaky bucket.
Let me know if you agree to take a dependency on this lib. If so, I will integrate it in ShopifySharp, and have a go at the leaky bucket.
Hi guys How are we doing on graphql support.? If you guys push something , may be in a separate branch, then people like me can get a direction and even start helping you out :-)
thanks ish
No news as far I as know. Personally I'm still using REST but I'm thinking of refactoring my code to use GraphQL at some point because almost all new features are almost exclusively in GraphQL.