Update Product by explicit fields

[drovani]

I know there's work being done in F# (#388) for specifying which fields get sent back to in an Update call. In the meantime, I've quickly put together a way to pick which fields to use by listing them in a string array. The idea being that one doesn't need to do a GET to pull down the whole Product before doing an Update with just one or two fields.

For immediate application, I am using the Product Service. I have added a couple of test methods to validate that it is working. In an ideal setup, I might allow for an expression tree, but implementing that is outside of my current knowledge set.

Any feedback you have would be appreciated. I wouldn't mind expanding this to more services as needed.

This would solve #379 and possibly #475.

[nozzlegear]

This is cool! I especially like your use of nameof, that wasn't something I would have considered but now that I see it it's obvious.

I am indeed doing some work in the same area with F#, but I've recently started a new branch for bringing a fluent-style builder to C# and will instead base the F# stuff off of that. My intent is for it to look something like this:

var product = new ProductBuilder()
	.SetTitle("Some title")
	.SetPublishedAt(DateTimeOffset.Now);

It would do essentially what your pull request is doing too, it just modifies a hidden dictionary and decides which properties get serialized. That said, I definitely like your approach too, and I think having the two different approaches is beneficial, so I'd like to go ahead with your proposal.

Two thoughts after looking at the code:

1. I'd like to explore using both a blacklist and a whitelist to explicitly decide which props get serialized. They'd be exclusive, so if both a whitelist and a blacklist are used, prefer the whitelist.
2. Eventually the parameters and overloads for each method on each service are going to get out of hand, especially when I merge the PR with cancellation token support (Introduce Cancellation Token support across all public service APIs. (Resolves #238) #476). Maybe the create and update methods should accept a wrapper class that contains both the object itself, and another property to specify which fields should be serialized.

[clement911]

Personally I prefer this approach to have a SetXXX method for each property (SetTitle, SetPublishedAt).

• Instead of putting this logic in the ProductService, it would be best to centralize it in ShopifyService by adding an additional optional fields parameter to the ExecutePutAsync method:

  ShopifySharp/ShopifySharp/Services/ShopifyService.cs

  Line 235 in e1a09d1

  protected async Task<T> ExecutePutAsync<T>(string path, string resultRootElt, object jsonContent = null)

All service update methods can then simply call this method.

• Instead of a simple string[] fields, we could create a FieldsInfo class.
  Pseudo code:

enum FieldsMode { GivenFieldsOnly, AllExceptGivenFields }
class FieldsInfo
{
   string[] Fields;
   FieldsMode Mode;
}

This way we can ask to update all fields except a few.

• I also think it's worth exploring an Expression Tree version.
  I don't think it would be to hard to convert an expression to a list of string, or maybe even a list of PropertyInfo, which is what ToDictionary does with the fields array.

ExecutePutAsync could accept a lamda instead of string[]

[drovani]

• Instead of putting this logic in the ProductService, it would be best to centralize it in ShopifyService by adding an additional optional fields parameter to the ExecutePutAsync method:

  ShopifySharp/ShopifySharp/Services/ShopifyService.cs

  Line 235 in e1a09d1

  protected async Task<T> ExecutePutAsync<T>(string path, string resultRootElt, object jsonContent = null)

This brings up an interesting question - why is the WebhookService the only service that calls ExecutePutAsync<T>? The ProductService calls ExecuteRequestAsync<T>, which seems to duplicate work that ExecuteWithContentCoreAsync<T> already does.

ShopifySharp/ShopifySharp/Services/Product/ProductService.cs

Line 98 in e1a09d1

var response = await ExecuteRequestAsync<Product>(req, HttpMethod.Put, content, "product");

@nozzlegear - is there some historical reason for this, or is this just a case of technical debt not yet paid down?

[clement911]

I recently introduced base methods in the ShopifyService.cs, such as ExecutePutAsync and ExecuteGetAsync, because all services basically copy and paste the same logic.

We were quite pressed to release v5 and I didn't have time to go through all services but ideally they should all use the base class methods.

[drovani]

I've put together this idea of a generic UpdateBuilder<TShopifyObject> that allows for a fluid set of property expressions and values. It stores a dictionary internally to keep track as the user builds the data points to update. I think this is cleaner than making a Set_______ method for each property (I don't even want to think of the amount of boilerplate code that would require), and keeps things a little tighter than relying on the user to ensure that the property strings match.

I then added an overload for the ProductService.UpdateAsync to accept a UpdateBuilder<Product>, which then gets passed to the ShopifyService.ExecutePutAsync.

I'm not particularly happy with the duplication of the code that gets the JsonPropertyAttribute, since it's just a copy/paste from part of the ObjectExtensions class. Any suggestions on where that code could live?

I haven't tested this yet for nested properties (i.e. Update Product.Variants[].Ids), that'll be an iteration if there's positive feedback on this UpdateBuilder<T> object.

[clement911]

I saw that Stripe.net has come up with an interesting approach using a discriminated union AnyOf<T1,T2> and a special Empty parameter that can be used to explicitly set a field to null.. If we added that to all fields that can be set to null, and change the serialization settings to understand AnyOf<T1,T2>, this might work. Something to think about....

[drovani]

It's interesting, but would require a fundamental rewrite of all of the ShopifySharp POCO classes, wouldn't it?

Is there interest in me further expanding on this UpdateBuilder<TShopifyObject> class, or do you think there's a better way to do this without significant change required on the part of consumers? I have some free cycles this week, and wouldn't mind spending it plugging away on this project.