Seeking input on improved GraphQL support

[nozzlegear]

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?

[dkershner6]

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.

[dkershner6]

For those familiar, a good analog in the .Net world would be:

GraphQL is to SqlClient as REST is to EntityFramework.

[dkershner6]

I will also note that even Shopify themselves have not integrated graph into their python library: https://github.com/Shopify/shopify_python_api

[clement911]

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?

[clement911]

By the way I'm happy to submit a PR for this but waiting for your feedback first...

[dkershner6]

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.

[clement911]

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.

[dkershner6]

Well, the request side is fairly simple on REST, but sure, we do have several .

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.

[clement911]

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/)

[clement911]

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
    }
  }
}

[dkershner6]

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.

[dkershner6]

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...

[clement911]

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?

[clement911]

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

[clement911]

I raised an issue on their repo graphql-dotnet/graphql-client#120

[dkershner6]

Good steps. Even if we have to fork, likely worth it. Derek Kershner

…

________________________________ From: Clement Gutel <notifications@github.com> 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#120<graphql-dotnet/graphql-client#120> — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#366?email_source=notifications&email_token=AGE2OG3MH6RCFUTIKYIXKELPXXW47A5CNFSM4HPR5W5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWOARRQ#issuecomment-496765126>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AGE2OG3INU5L6LQNZ447ZLDPXXW47ANCNFSM4HPR5W5A>.

[clement911]

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..

[clement911]

Might be worth considering forking it to add the Extensions and the anonymous object overloads.

[clement911]

@nozzlegear any feedback?

[dkershner6]

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 <notifications@github.com> 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) @nozzlegear<https://github.com/nozzlegear> any feedback? — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#366?email_source=notifications&email_token=AGE2OG4AGC2VGFHLUN6NQDTPX64QXA5CNFSM4HPR5W5KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWSDSRY#issuecomment-497301831>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AGE2OG7R7RI5PRSC7AYAXHTPX64QXANCNFSM4HPR5W5A>.

[clement911]

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)
                }
            });

[nozzlegear]

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.

[nozzlegear]

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.

[clement911]

Nice to see ShopifySharp mentioned on Here’s Everything We Announced at Shopify Unite 2019 👍

[nozzlegear]

Yes, that was very cool!

[clement911]

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.

[clement911]

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.

[ishahrier]

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

[giustiandrea]

Hello guys

is there any news?

Thanks

[clement911]

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.

[nozzlegear]

No news yet, I've been dragging my feet on graphql because I don't personally use it, but I realize it's very useful -- not to mention what Shopify seems to be moving to themselves. I hope to make a decision/some progress on this soon but I don't know what that's going to look like right now.

[nodrogyad]

I'd just like to add my vote for a solution like @clement911 has suggested. I'm starting to use a bunch of GraphQL and would really benefit from better integration. Of particular interest to me is the leaky bucket integration. Even better would to inject cursor management into a supplied query. BTW, as this has been lingering for quite a while graphql-client has moved along and implemented Extensions.

[evaldas-raisutis]

Any news?

I've recently had to implement graphql Shopify client at work. There has been some kinks that came to light in comparison with rest API, specifically the fact that all requests in rest API have fixed cost, so using semaphore slim made sense to limit requests.

However, since graphql queries each may have unique expected cost, that rate limiter needs to take query cost against current bucket capacity into account, and each response may include actual cost, which also needs to be taken into account to adjust current capacity. And obviously the capacity needs to drip over time. I have come to terms that it would not be possible to implement graphql rate limiter according to existing policy interface, because it only supports fixed cost requests, where each unit of cost is interpreted as a thread.

So instead it's built using a queue of task sources, where each GrantAsync request passes through the execution policy. If the policy determines there is sufficient capacity, it immediately permits GrantAsync by returning a completed task. If not, a task source is added to a queue and it's task is return to the caller to be awaited.

A function ExecuteNext picks the next task from this queue, checks if the task's expected query cost is lower than current bucket capacity and resolves it, allowing the caller to proceed through GrantAsync.

This function is executed when bucket is dripped, as well as after bucket state is modified after a successful request, since the response will include actual query cost.

As for classes, we have chosen not to support dynamic types (at least not yet), because I'm just not a fan for some reason. Instead, client user defines all of input and output classes to match their use cases. This makes it rather difficult to write re-usable models, but that's how it is.

[nozzlegear]

Yes, things have changed for me and I've finally had a chance to learn/use Shopify's GraphQL API with one of my current clients. I've got a few ideas of my own for improving the GraphQL support in ShopifySharp. I also like @clement911's solution so we may look at implementing that or something very similar as soon as time is available.

[clement911]

I'm going to have a go at implementing the GraphQL rate limit

[clement911]

Here is my PR for GraphQL rate limits: #667

[loadGent00]

Howdy!

I see y'all added support for the rate limits for GraphQL. Do you think you will have better support here soon or in v6? Maybe implementation of the variables instead of string interpolation?

[clement911]

I just haven't had the time to do it so far, especially given that we can manipulate query string in the mean time.
But I think variables will be the next logical thing to add.