ShopifySharp icon indicating copy to clipboard operation
ShopifySharp copied to clipboard

Experimental: explicitly building object properties for create/update methods

Open nozzlegear opened this issue 5 years ago • 10 comments

I'm working on a ShopifySharp.Experimental package that I plan to publish to Nuget in the next few days. I want to use this package as a testing ground for new ideas that won't necessarily make it into ShopifySharp on the first iteration. I won't promise stability in this package between releases, I expect there will be breaking changes quite often, so don't use it in a production app.

The first experiment I'm doing is to fix the major issue this package has with updating/creating objects (#379) (#373) (#367) (#284). You have to be able to send null to the Shopify API, but it's far too easy to send null for a property that you didn't intend. In practice, I've added a hard rule for my apps that first I need to pull in the entire Shopify object, set the one single property I want to change on it, and then send that entire object back to Shopify -- just to make sure I don't accidentally erase a field.

In my experiment, I'm setting up an F# module which will explicitly build each property of an object so you know exactly what will be sent when creating and updating. Null will never be sent if you don't explicitly add the property and set it to null. Other properties will not be sent at all if you don't add them.

Here's what the intended usage looks like once I've got this finished:

open ShopifySharp.Experimental 

let webhook = 
	newWebhook
	|> address "https://example.com"
	|> topic "app/uninstalled"

// Serialized to JSON, this looks like:
// { "address": "https://example.com", "topic": "app/uninstalled" }

Note that the format, metafield_namespaces, created_at, updated_at fields are not null like they would be if you serialized the regular ShopifySharp.Webhook class -- they just aren't in the JSON at all.

If you want to set a property to null, you'll use makePropertyNull:

let webhook = 
	newWebhook
	|> address "https://example.com"
	|> topic "app/uninstalled"
	|> makePropertyNull WebhookProperty.Format

// Serialized to JSON, this looks like:
// { "address": "https://example.com", "topic": "app/uninstalled", "format": null }

If you need to remove a property from the object for whatever reason, you'll use removeProperty:

let webhook = 
	newWebhook
	|> address "https://example.com"
	|> topic "app/uninstalled"
	|> format "json"
	|> removeProperty WebhookProperty.Format

// Serialized to JSON, this looks like:
// { "address": "https://example.com", "topic": "app/uninstalled" }

In the background, the webhook builder is just using an F# Map<WebhookProperty, obj option> which is roughly equivalent to a C# IReadOnlyDictionary. So if you really want to dig in to add custom properties or raw values that don't match the types on the helper functions, you'd be able to do that:

let webhook = 
	newWebhook
	|> address "https://example.com"
	|> Map.add (WebhookProperty.CustomProperty "some_property") (Some (box 5))

// Serialized to JSON, this looks like:
// { "address": "https://example.com", "some_property": 5 }

This makes liberal use of F# types, which can sometimes be difficult to use from C#. If this experiment makes it into the main package, I'd wrap all of the F# stuff in a fluent-style class for C#. I haven't specced that out yet, but in my head it'd look something like this:

var webhookBuilder = WebhookBuilder.NewWebhook()
	.WithAddress("https://example.com")
	.WithTopic("app/uninstalled")
	.WithFormat("json")
	.WithNullProperty(WebhookProperty.Topic)
	.WithCustomProperty("some_property", 5);

One thing I haven't figured out is how I'll merge the F# code into the C# package. I'd personally like to create a ShopifySharp.Functional package, which is where I imagine this F# webhook builder would be published. But I don't know that I want to have ShopifySharp itself add a dependency for the F# package, or bring in the FSharp.Core dependency.

nozzlegear avatar Aug 06 '19 16:08 nozzlegear

The F# version of this has been published on NuGet. It includes functions for building webhooks, orders, customers, addresses, line items, metafields, transactions and more. The ShopifySharp.Experimental.Webhooks and ShopifySharp.Experimental.Orders modules include extensions of WebhookService and OrderService respectively, which have additional methods for create/update API calls using the builder functions (instead of the traditional ShopifySharp classes we've always used).

As a reminder, the experimental package does not strictly adhere to SemVer and it's possible that breaking changes will be introduced in the future.

nozzlegear avatar Sep 28 '19 02:09 nozzlegear

I'm planning to add the C# fluent versions of the object builder function soon.

nozzlegear avatar Sep 28 '19 02:09 nozzlegear

I personally haven't ran into this problem too much because this is pretty cool. If I understand correctly, the experimental services are built on top of the regular service (meaning, they inherit from them) and therefore the normal library will not be impacted?

clement911 avatar Sep 28 '19 07:09 clement911

Correct! At least for now. If it works well enough I'd like to make the planned fluent version the recommended method for creating/updating objects, but I don't have any intention of removing the class-based methods already in the package. I've unfortunately been bitten by the problem this is trying to solve, very recently on a client project.

nozzlegear avatar Sep 28 '19 15:09 nozzlegear

Hi there!

First of all, thank you for your hard work!

I am very concerned that you decided to include F# code into the C# package. I completely understand that it can be a very elegant solution for this particular issue but I don't think it is really a good idea to include 2 languages in such a library.

I suggest the following solution. I suggest making all properties String. It will allow passing only required properties during update/create. As I understand, you have problems with such properties as Product.PublishedAt.

[JsonProperty("published_at")] public string PublishedAt { get; set; }

If we try to update a product it will not unpublish this product because it will not pass PublishedAt property to Shopify.

productService.UpdateAsync(new Product(){ Id = 123, Title = "123" };

If we need to unpublish a product we just need to set this property to String.Emtpy:

productService.UpdateAsync(new Product(){ Id = 123, PublishedAt = String.Empty };

In this case, we will get the following json:

...{ "id": 123, "published_at": "" }

If you don't like that instead of null we pass "" we can create a CustomJsonConvert which will replace "" to null:

[JsonProperty("published_at")] [JsonConverter(typeof(EmptyStringToNullConverter))] public string PublishedAt { get; set; } ... productService.UpdateAsync(new Product(){ Id = 123, PublishedAt = String.Empty }; ... ...{ "id": 123, "published_at": null }

What do you think about this approach?

v-dev-1 avatar Oct 02 '19 14:10 v-dev-1

Thanks for the feedback on this @v-dev-1! I'm not sure I follow your concerns about using F# for this. As you said, it's an elegant solution that so far is working very well for my own use-case. I'll be the first to admit that I don't have all the answers, and that what works for me may not work for everyone else.

But I don't think the language has anything to do with that. Nobody will be forced to use F#. The two languages are compatible, you can easily add an F# package to your C# project and use that package without writing any F# code.

If you're concerned about the main package being rewritten in F#, I have no plans or desire to do that. The language is only going to be used in the ShopifySharp.Experimental package. Granted, if you want to contribute to the experimental package you'd have to write F#, but if someone isn't comfortable with that I'd expect them to file a feature request or contribute to the planned C# fluent package.

Regarding your proposed approach for using strings and converters, it's been hard for us to find an elegant solution -- and we've been trying for years at this point. Your code looks good, it would definitely solve the issue for PublishedAt specifically, but it doesn't work in every case.

As a general example, we've had cases where a property is valid if it's null and will erase that value when sent -- but by default, all (nullable) values are null if they aren't set explicitly, so how do you as the developer communicate when you don't want to send a value at all versus when you want to send null. There are also properties that are invalid if they're null, and will throw an error when you send it to Shopify. If we dive in and start setting special behavior for each property, you'd never be sure what ShopifySharp is sending under the hood until you open up GitHub and dive into the code.

I think there is tremendous value in being able to explicitly build an object that contains exactly the properties and values you expect it to contain. I can confidently say that it fixed at least one major bug I had in a client project, when we wanted to update an order's note attributes but serializing an entire order object to do that would throw errors thanks to certain null values. I wrote a tiny C# wrapper class around the experimental package and now we know exactly what ShopifySharp is serializing and sending:

var updatedOrder = OrderBuilder
	.Begin()
	.AddNoteAttributes(updatedNoteAttributes)
    .Done();

And just to be clear, the new functional stuff I've introduced in the experimental package will never replace the create/update methods in ShopifySharp itself. It will never replace the classes or properties we've already written. The planned C# fluent package, which might use the experimental F# stuff under the hood, has a likelihood of becoming the recommended way to do creating and updating, but we'll always support using the same classes and methods available today.

nozzlegear avatar Oct 04 '19 16:10 nozzlegear

Hi @nozzlegear, Would like to know where we are with this one? whats currently the recommended way to upload a product price only without needing to upload all its props?

SDP190 avatar Aug 11 '22 22:08 SDP190

Related: https://github.com/nozzlegear/ShopifySharp/pull/642

clement911 avatar Aug 12 '22 01:08 clement911

Hi @nozzlegear, Any update here? looking out for this,

SDP190 avatar Aug 25 '23 04:08 SDP190