Update doesn’t allow resetting field to null

[limoniapps]

Been using ShopifySharp for a while now and suite happy with it!

Today I ran into unexpected behavior when attempting to update a PriceRule (but the behavior is true for all entities). I want to reset the EndsAt date from a value to null (no end date applies). The update doesn’t actually tale effect because EndsAt is never sent to Shopify. This is because the Json serializer is instructed to ignore nulls.

How would one go about updating a value to null with ShopifySharp?

Perhaps it would be useful to add a parameter to Update to ignore nulls or not?

[nozzlegear]

This is something that I had been worried about when I made the decision to make everything nullable and not send those null values to Shopify in 4.0. I wish I had thought of a more elegant solution before I made the release, but at the time I couldn't find one.

You're right, there's no way to send a null value to the Shopify API right now, but I want to fix that and make it possible. A couple solutions that I've written down over the last couple months include:

1. Adding overloads for each update and create function, accepting a dictionary rather than one of the classes. Any null or empty value in that dictionary would be sent to Shopify.
2. Let devs use their own custom JsonSerializer to handle serialization of the classes.
3. My favorite solution, add overloads that accept an expression to explicitly tell the underlying serializer which properties to send or not send. It would look something like this:

var customer = new Customer
{
  Value1 = 5,
  Value2 = null
};

// Tell ShopifySharp to only send the Value1 and Value2 properties, even if they're null
await service.UpdateAsync(id, customer, c => c.Value1, c.Value2);

I think #3 is the easiest and most explicit way to "reimplement" sending null values to Shopify while still supporting partial object updates, but I'd love to get your feedback and anybody else's feedback.

Which of those would you, personally, rather use? Or do you have an idea that might be better?

Edit: I should clarify, there doesn't have to be only one winner. I like both the dictionary and the expression and will probably add both. Just looking for some feedback on what everyone else likes.

[dkershner6]

I'd change 2 to just full or partial (like a default false bool). That seems easiest to use and implement, and if someone is pulling down the object first, they can just use true. Eventually, maybe we can detect if someone has a full object and do it automatically (an etag would help, but maybe another way).

The other two are far more flexible, but the use case of sending some null fields and not others seems pretty corner case.

[clement911]

This is related to #62

I agree with @dkershner6. Solution 2 is much simpler.

I would create a enum like

public enum UpdateBehavior
{
   NonNullPropertiesOnly,
   AllProperties
}

and add it as an optional parameter to all Update() methods, with the default value being NonNullPropertiesOnly.

e.g.

Task<Order> UpdateAsync(long orderId, 
Order order, 
UpdateBehavior updateBehavior = UpdateBehavior.NonNullPropertiesOnly);

I guess this should be non breaking?

[nozzlegear]

Good suggestions! I like adding an overload to turn this "feature" on or off. I think an enum is the way to go, just to make things explicit when using it. If we default to the current behavior then this won't be a breaking change and could be added in 4.x.

I'd also like to add the expression overload for explicitly setting which properties will be sent, as I have a few use cases for such a thing. However, it doesn't have to be implemented at the same time as the enum.

[limoniapps]

At the moment I’ve changed my local implementation to not ignore nulls on update (similar to solution #1) since I implemented a repository pattern that always gets/inserts/updates full entities. I use a BaseShopifyService that implements the logic needed for 80% of the services; so I only had to change a few lines of code. I see the value of reducing the payload through a solution like #3, but I have not found any use cases for it in the apps I build. I’m sure others must have such use cases though. From: Joshua Harms <notifications@github.com> Sent: Friday, August 17, 2018 4:35 PM To: nozzlegear/ShopifySharp <ShopifySharp@noreply.github.com> Cc: Bart Coppens <bart.coppens@live.com>; Author <author@noreply.github.com> Subject: Re: [nozzlegear/ShopifySharp] Update doesn’t allow resetting field to null (#284) This is something that I had been worried about when I made the decision to make everything nullable and not send those null values to Shopify in 4.0. I wish I had thought of a more elegant solution before I made the release, but at the time I couldn't find one. You're right, there's no way to send a null value to the Shopify API right now, but I want to fix that and make it possible. A couple solutions that I've written down over the last couple months include: 1. Adding overloads for each update and create function, accepting a dictionary rather than one of the classes. Any null or empty value in that dictionary would be sent to Shopify. 2. Let devs use their own custom JsonSerializer to handle serialization of the classes. 3. My favorite solution, add overloads that accept an expression to explicitly tell the underlying serializer which properties to send or not send. It would look something like this: var customer = new Customer { Value1 = 5, Value2 = null }; // Tell ShopifySharp to only send the Value1 and Value2 properties, even if they're null await service.UpdateAsync(id, customer, c => c.Value1, c.Value2); I think #3<#3> is the easiest and most explicit way to "reimplement" sending null values to Shopify while still supporting partial object updates, but I'd love to get your feedback and anybody else's feedback. Which of those would you, personally, rather use? Or do you have an idea that might be better? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#284 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AO6kIuwmAatPBileJF7XatZFhSWC52qPks5uRtR6gaJpZM4WBDjm>.

[dkershner6]

@clement911 :

public enum UpdateBehavior
{
NonNullPropertiesOnly,
AllProperties
}

I'm good with Enum and @clement911 , but can we make it:
public enum UpdateBehavior { NonNullOnly, All }

I'm a lazy one. ;) And...I think props is implied.

[clement911]

Fair enough.
I always go back and forth between long explicit names and shorter ones...

[TOuhrouche]

I join others and thank you a million for the awesome library.
The issue at hand here applies to PublishedAt property of the product as well.
Not setting this field will automatically send a NULL value to Shopify which will hide the product from the online store.
In my opinion, the best option is to either offer a way to provide a custom serializer or an Enum as suggested above.

[nozzlegear]

Thanks for the suggestions again everyone, this is something I've run into myself so I feel your pain and realize it's a difficult limitation. I'm going to start working on this issue in particular very soon!

[nozzlegear]

Coming back to this much later, I've been thinking about introducing an object update function that's kinda similar to the server builder methods in aspnet core, but this one would explicitly set values to null or not null.

var cs = new CustomerService(...);
var updatedCustomer = cs.UpdateAsync(custId, cust => 
{
    cust.PropertyOne = SetValue('something');
    cust.PropertyTwo = SetValue(null);
})

This is also somewhat inspired by the way you might build or configure an object in some F# packages:

let cs = CustomerService(...)
let updatedCustomer = 
	beginUpdatedCustomer
	|> PropertyOne "something"
	|> PropertyTwo None
	|> cs.UpdateAsync custId

Not sure how useful this would be when compared to the enum method discussed above, but I'm trying to find something where you know exactly what is being serialized and sent to Shopify.

[clement911]

How would the SetValue know which property it is setting?
I'm a bit worried about how complicated this would get and potential edge cases.

I have another potential proposal, which seems very promising unless I'm missing something!

cust.PropertyOne  = "something";
cust.PropertyTwo = null;
var updatedCustomer = cs.UpdateAsync(custId, cust => 
{
    cust.PropertyOne,
    cust.PropertyTwo
})

An anonymous object selector is used to tell the library which properties should be updated.
This should also work for nested properties such as Address.
Internally the lib can use the selector to project the customer object, and serialize the result instead.

It is similar to what I proposed in #366 (comment) to deserialize GraphQL responses.

[nozzlegear]

SetValue would return a class, so the value of each property on that update object would either be null or this class. We'd turn it into a dictionary and any value that is that class would be sent (even if the underlying value is null):

public class UpdateValue<T>
{
	public T Value { get; set; }
}

public class ProductVariantUpdate
{
    public UpdateValue<decimal?> Price { get; set; }
    public UpdateValue<string> Title { get; set; }
    // and so on
}

var updatedVariant = await service.UpdateAsync(id, v => 
{
	v.Price = SetValue(5.55m);
	// ProductVariantUpdate.Price is now an instance of UpdateValue<decimal>
	// but ProductVariantUpdate.Title is still null. The service would only send
    // the values that are an instance of UpdateValue<T>. It would handle converting
    // the UpdateValue class to a raw value (so it sends 5.55 and not { value: 5.55 })
});

The immediate downside to this is that we have to reproduce every class as an "update" version of itself, duplicating all of the property names. That would be tedious but not impossible. It'd be great of C# had some method for replacing the property types of a class while keeping the same object structure, like TypeScript does.

That said, I do like the idea of using an anonymous selector! Would we be able to get intellisense on the value, so it knows cust in cust => { } is a customer (or whatever the object type is), or is it purely anonymous?

[clement911]

Got it. It's a like bit like F# Option with None/Some.
Still it's rather complicated.

Would we be able to get intellisense on the value, so it knows cust in cust => { } is a customer (or whatever the object type is), or is it purely anonymous?

Yes, it would know the type so you would get intellisense. I think it would be great because you can shape the request exactly how you want it and explicitly say which properties should be updated.
Pretty easy to implement too. I'll give it a go if you're cool with that approach?

[clement911]

After playing a bit, I think the selector implementation would not be as straight forward as I thought.
The problem is that when the object is projected with the selector, e.g () => new Product { Title = default }, we lose the [JsonProperty] attributes.
I still think it's possible to make it work but it would require an ExpressionVisitor to collect all the properties that are touched, as well as a custom contract resolver to only serialize properties that were touched.

[nozzlegear]

I'm working on an experimental solution for the null field issues in #388.

[TOuhrouche]

Been thinking about this for a while as well.
Wouldn't it make sense to expose a serialization strategy to be implemented as a contract resolver? This way, the library wouldn't need to encapsulate and abstract intelligence required to translate our intentions as developers. Instead, it gives us a way to implement our intentions via a contract resolver. In my case, I would leave the default inner workings of the library as they are until I get to the PublishAt property which I can ignore in certain cases.

[evaldas-raisutis]

What if all base resource classes (product, order, etc.) would provide a virtual field which would return a member selector for fields? By default it could mimic current behavior, but would enable developers to extend to their own classes and override the selector?

Alternatively, in a more structured manner, we could have a ISerializeMembers which would define which fields are included during serialization per given resource type. Developers could extend the types and provide their own rules for serialization, by for example extending Product with UnpublishedProduct and specifying that PublishedAt should be serialized at all times.