-
-
Notifications
You must be signed in to change notification settings - Fork 307
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
Update doesn’t allow resetting field to null #284
Comments
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:
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. |
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. |
This is related to #62 I agree with @dkershner6. Solution 2 is much simpler. I would create a enum like
and add it as an optional parameter to all Update() methods, with the default value being NonNullPropertiesOnly. e.g.
I guess this should be non breaking? |
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. |
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 <[email protected]>
Sent: Friday, August 17, 2018 4:35 PM
To: nozzlegear/ShopifySharp <[email protected]>
Cc: Bart Coppens <[email protected]>; Author <[email protected]>
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>.
|
I'm good with Enum and @clement911 , but can we make it: I'm a lazy one. ;) And...I think props is implied. |
Fair enough. |
I join others and thank you a million for the awesome library. |
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! |
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. |
How would the I have another potential proposal, which seems very promising unless I'm missing something!
An anonymous object selector is used to tell the library which properties should be updated. It is similar to what I proposed in #366 (comment) to deserialize GraphQL responses. |
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 |
Got it. It's a like bit like F# Option with None/Some.
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. |
After playing a bit, I think the selector implementation would not be as straight forward as I thought. |
I'm working on an experimental solution for the null field issues in #388. |
Been thinking about this for a while as well. |
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. |
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?
The text was updated successfully, but these errors were encountered: