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

Deprecate most GraphService methods and converge on two standard methods #1051

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

nozzlegear
Copy link
Owner

@nozzlegear nozzlegear commented Apr 5, 2024

This change deprecates most GraphService.PostAsync and GraphService.SendAsync methods, of which there were at least six total. It introduces two new GraphService.PostAsync methods to replace them all, which standardize the usage of the GraphRequest class and serializing all requests to json.

Previously, some of the PostAsync/SendAsync methods would serialize the request to json, some wouldn't. Some would send the variables, some wouldn't. Some would let you set your own json wherein you could apply the variables yourself, but then they'd send the application/graphql content-type, which made Shopify ignore any variables sent.

That one has personally bit me each time I've tried to use the GraphService recently; it's been difficult to figure out which method should be used on the service when I want to send variables, without looking at the source code for ShopifySharp.

After this change, the new method for sending Graph requests using the GraphService going forward would look like this:

var query = 
"""
query GetStuff($productId: String!) {
  product(id: $productId) {
    id
    title
  }
}
""";
var variables = new
{
  productId = "gid://shopify/products/123456"
};
var graphRequest = new GraphRequest
{
  Query = query,
  Variables = variables,
  EstimatedGraphCost = 3
};

// Get a JsonElement back
var result = await graphService.PostAsync(graphRequest, cancellationToken);
// Or deserialize into whatever you want
var result = await graphResult.PostAsync<ShopifySharp.GraphQL.Product>(graphRequest, cancellationToken);

I don't want anyone to lose the ability to control how variables are serialized or deserialized though, so I've added an IGraphSerializer interface that can be passed to the GraphService constructor.

public class MyCustomGraphSerializer : IGraphSerializer
{
  public string SerializeToJson(IDictionary<string, object> variables)
  {
    throw new NotImplementedException();
  }
  
  public T DeserializeFromJson<T>(string json)
  {
    throw new NotImplementedException();
  }
}

// elsewhere
var serializer = new MyCustomGraphSerializer()
var graphService = new GraphService(domain, accessToken, graphSerializer: serializer);

I'm not 100% sure this is the exact interface I'll use for this, but it's something along the lines of what I'm thinking. Whether we use this interface or do something else for serialization/deserialization in the GraphService, I want it to play nicely with the factory and DI package.

@nozzlegear nozzlegear changed the title Deprecate most GraphService public methods and converge on two public methods Deprecate most GraphService methods and converge on two standard methods Apr 5, 2024
@clement911
Copy link
Collaborator

Should it be also be possible to provide variables via a dictionary?

@nozzlegear
Copy link
Owner Author

Oh yeah, I like that more @clement911! So maybe we change the GraphRequest class to look like this:

public class GraphRequest
{
  public string Query { get; set; }
  public Dictionary<string, object> Variables { get; set; }
  
  // Convenience methods for adding/setting variables?
  public GraphRequest AddVariable(string key, object value)
  {
    Variables.Add(key, value);
    return this;
  }
  
  public GraphRequest SetVariable(string key, object value)
  {
    Variables.Set(key, value);
    return this;
  }
}

Then you could just pass in an entire dictionary if you've already got one, use the convenience methods, or access the dictionary object directly.

@clement911
Copy link
Collaborator

I'm not sure that the AddVariable / SetVariable methods really are needed.
The user can simply use the Variables dictionary to manage variables, right?

@nozzlegear
Copy link
Owner Author

nozzlegear commented Apr 7, 2024 via email

… methods

This change deprecates most GraphService.PostAsync and GraphService.SendAsync methods, of which there were at least six total. It introduces two new GraphService.PostAsync methods to replace them all, which standardize the usage of the `GraphRequest` class and serializing all requests to json.

Previously, some of the PostAsync/SendAsync methods would serialize the request to json, some wouldn't. Some would send the variables, some wouldn't. Some would let you set your own json wherein you could apply the variables yourself, but then they'd send the `application/graphql` content-type, which made Shopify ignore any variables sent.
…dGraphResponse class for deserializing GraphQL responses

Changes:

- Introduced a new class named `ParsedGraphResponse<T>` to be used for deserializing Shopify's GraphQL responses. It includes two properties: `Data` of type `T` and `UserErrors` of type `ICollection<UserError>`.
- Modified the error handling logic in `GraphService` to use the new `ParsedGraphResponse<T>` class.
- Refactored all public methods in `GraphService` to use the same internal method for calling the GraphQL API. Tests will be added to ensure the obsoleted methods have not been broken.
Planning to use a camel case property name strategy with System.Text.Json instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants