这是indexloc提供的服务,不要输入任何密码
Skip to content

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

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

Merged
merged 92 commits into from
Jan 3, 2025

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

@nozzlegear
Copy link
Owner Author

nozzlegear commented Oct 11, 2024

Remaining tasks:

  • Unit tests to ensure that obsoleted methods did not break with this PR.
  • Unit tests to ensure proper serialization of the GraphRequest, GraphExtensions and GraphUserError objects.
  • Unit tests for disabling error handling in the GraphService
  • Unit tests for enabling error handling in the GraphService
  • Make PostAsync and PostAsync<T> return a GraphResult<T>.
  • Documentation for this release
  • Make it work with .NET 472? (Remove all #IF NET8_0_OR_GREATER checks.) (Not possible at the moment, the generated GraphQL schema uses default interfaces which aren't supported in .NET 472.)
  • Use the new IDependencyContainer constructor in the GraphServiceFactory.

@nozzlegear nozzlegear force-pushed the graphservice-consolidation branch 2 times, most recently from e6492c3 to 4daf9f1 Compare November 16, 2024 05:25
@nozzlegear nozzlegear force-pushed the graphservice-consolidation branch 3 times, most recently from 05eb6ac to 22d58a9 Compare November 20, 2024 08:16
@nozzlegear nozzlegear force-pushed the graphservice-consolidation branch from 22d58a9 to 82af200 Compare November 21, 2024 08:36
@nozzlegear nozzlegear force-pushed the graphservice-consolidation branch 2 times, most recently from d7f7a5c to bc127ce Compare November 29, 2024 05:13
@clement911
Copy link
Collaborator

@nozzlegear I know that you are actively working on this and wondered if you saw this: https://shopify.dev/changelog/content-type-application-graphql-is-deprecated

@matnun-br FYI

@nozzlegear
Copy link
Owner Author

@clement911 Oh yes, I did see that! Thanks for the reminder, I want to incorporate that.

@nozzlegear nozzlegear force-pushed the graphservice-consolidation branch 2 times, most recently from 106d636 to a9a161f Compare December 17, 2024 05:13
…to protected

This change was done largely to facilitate easier direct testing of the ParseGraphExtensions method (without the convoluted step of sending requests through other unrelated methods first), but it makes sense to allow inheriting classes to access these two utility functions.
Just a nice change to have in unit tests for the equality checks.
…lable `T`

The System.Text.Json deserializer will only return null in very limited edge-cases, chiefly when the json string is the json literal `null` or the deserialized type is a nullable primitive (e.g. `int?`, `bool?`) and the json structure doesn't match. Even an empty curly brace will not trip the json literal `null` case. However, the GraphService itself has multiple checks in place to ensure that the json string is not null, an empty curly brace or even a mismatched type – it expects `object` and will only operate on `object`, and it will throw an exception if it doesn't find and object. Given this, we can make the assumption that the deserialization will succeed and the object will not be null.
…y throw when GraphResult.Json is not a JsonElement
@nozzlegear nozzlegear force-pushed the graphservice-consolidation branch from 7431b9a to 28ada09 Compare January 3, 2025 06:31
@nozzlegear nozzlegear merged commit 8a572b7 into master Jan 3, 2025
@nozzlegear nozzlegear deleted the graphservice-consolidation branch January 3, 2025 06:59
@nozzlegear
Copy link
Owner Author

Merged! 🎉 I'll check the tests, draft a new Github/Nuget release and begin updating the documentation tomorrow afternoon.

@dg107
Copy link

dg107 commented Jan 23, 2025

I'm sure I'm the last visual basic programmer on the planet, but now this class is crashing as VB can't handle there being a query property and a Query property.

@nozzlegear
Copy link
Owner Author

@dg107 Oh, I didn't know that. Thanks for the heads up! I mostly use F# in my personal projects so I know the pain of devs assuming if something works in C# it must work in everything else. 😜

I'll do some digging and see if I can't get this fixed somehow.

@nozzlegear
Copy link
Owner Author

Hey @dg107 (and @CopilotDaniel) – I've created a separate issue for the Visual Basic problem. I'm not sure if you've seen it, can you check the solution I've proposed there and let me know if it'll work for you?

Here's the link: #1144

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Deprecated and released
Development

Successfully merging this pull request may close these issues.

3 participants