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

Experimental: explicitly building object properties for create/update methods #388

Open
nozzlegear opened this issue Aug 6, 2019 · 10 comments
Labels
experimental Deals with experimental features or changes that may not be published in ShopifySharp itself functional Deals with the ShopifySharp.Functional package object updating Bugs and strange behavior around updating objects, primarly related to #284 stale Issues that have become inactive and need attention/review from @nozzlegear

Comments

@nozzlegear
Copy link
Owner

nozzlegear commented Aug 6, 2019

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 nozzlegear added object updating Bugs and strange behavior around updating objects, primarly related to #284 experimental Deals with experimental features or changes that may not be published in ShopifySharp itself functional Deals with the ShopifySharp.Functional package labels Aug 6, 2019
@nozzlegear nozzlegear pinned this issue Aug 6, 2019
@nozzlegear
Copy link
Owner Author

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
Copy link
Owner Author

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

@clement911
Copy link
Collaborator

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?

@nozzlegear
Copy link
Owner Author

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.

@v-dev-1
Copy link

v-dev-1 commented Oct 2, 2019

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?

@nozzlegear
Copy link
Owner Author

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.

@SDP190
Copy link

SDP190 commented Aug 11, 2022

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?

@clement911
Copy link
Collaborator

Related: #642

@SDP190
Copy link

SDP190 commented Aug 25, 2023

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

@nozzlegear nozzlegear unpinned this issue Oct 26, 2023
@Laurabee530 Laurabee530 added the stale Issues that have become inactive and need attention/review from @nozzlegear label Jan 25, 2024
@mysteryx93
Copy link

mysteryx93 commented Sep 18, 2024

Hi, I wrote the OntraportApi.Net library, which has many things in common with Shopify API (full CRM system to manage customers, forms, orders and all that).

If you look at OntraportBaseWrite, which is responsible for updating all object types, I don't have your issue. Because I'm not serializing random objects, but rather passing a dictionary of updated values. Each object property is managed by a separate object, parsing, formatting and tracking its state, for all various data types.

You could take a look at my design.

Here's an example of updating contacts data using my API; calling GetChanges on the object to pass only updated fields to the Update method.

public async Task EditCompanyField(string oldName, string newName)
{
    var contacts = await _ontraContacts.SelectMultipleAsync(
        new ApiSearchOptions().AddCondition(ApiContact.CompanyKey, "=", oldName));
    var tasks = contacts.Select(async x =>
    {
        x.Company = newName;
        return await _ontraContacts.UpdateAsync(x.Id.Value, x.GetChanges());
    });
    await Task.WhenAll(tasks);
}

Btw it was slightly frustrating to deal with a few inconsistencies in the API, but it looks like that's a lot worse worse in Shopify API. I have compassion for you :-)

I started looking at the API; it is arguably more complex than Ontraport; and GraphQL makes it even more complex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
experimental Deals with experimental features or changes that may not be published in ShopifySharp itself functional Deals with the ShopifySharp.Functional package object updating Bugs and strange behavior around updating objects, primarly related to #284 stale Issues that have become inactive and need attention/review from @nozzlegear
Projects
None yet
Development

No branches or pull requests

6 participants