Unable to nullify string attribute using the update function...

[ckeyes88]

Currently, string values on any of the data structs have the omitempty tag associated with them. Because of this, if I try to set a string value to it's nil value "", json.Unmarshal and json.Marshal leave off that attribute. This is a problem when trying to update a string field that I want to unset.

Steps to reproduce (using a variant as example):

• Create a variant with v.InventoryManagement = "shopify"
• Set v.InventoryManagement = ""
• Call shopify.Variant.Updated(v)

Expected: The returned updated variant should have v.InventoryManagement == "" and that variant should no longer be set to have shopify track inventory.

Actual: The returned updated variant has v.InventoryManagement == "shopify" and that variant has no changes to it's inventory management in the admin console.

Proposed fix: Convert the string values to string pointers so that they can be explicitly nil-able.

If I'm thinking about the problem incorrectly, I'd love some feedback on how to unset string properties.

[ckeyes88]

I can confirm that when I remove the omitempty from Variant.InventoryManagement the API call works as expected. I suppose an alternative solution to using string pointers would be to just remove the omitempty tag on all string attributes.

[ckeyes88]

Update: As a work around I used the custom data structure workflow to create a variant that didn't have the omitempty tag associated with its strings. I still think a better solution would be to have primitives be pointers and not have the omitempty tag but that may not be correct.

[dlebech]

@ckeyes88 thanks for the comments and sorry for the trouble. I haven't been using this feature for a while, but according to the documentation, it sounds like the inventory_management is not meant to be set to the empty string:

If you track the inventory yourself using the admin, then set the value to "shopify". Valid values: shopify or the handle of a fulfillment service that has inventory management enabled. Must be the same fulfillment service referenced by the fulfillment_service property.

As such, I would be hesitant to remove the omitempty flag in this case for everyone, because it doesn't seem like the inventory management field is supposed to be empty in the default case? What do you think?

[ckeyes88]

@dlebech Thanks for the response! I agree that it should not be set to "" in the API but rather null. I'm certainly not a shopify API expert so I'll defer to your judgement. However, when a product is first created with no inventory management tracking the value is null which when using a primitive string value in Go defaults to the nil string value of "". Once inventory_management is set to any valid value the only way that I know of to stop tracking inventory for that product is to nullify the inventory_management field, which again with the string primitive would be to set it to the empty string and have json.Marshal convert that to null. This is why my initial thought was that primitives that are optional should be pointer values so that people can explicitly set them to nil if they want to remove them altogether.

Interested to hear your thoughts.

[dlebech]

@ckeyes88 I see your point. When inventory management is initially unset, it will remain unset forever because of the omitempty, but if you want to completely unset a previously set value, you're out of luck. That's the gist of it right?

If we removed omitempty and changed the string to a pointer, it would correctly marshal to null, but for partial updates of other fields (e.g. if I just wanted to update the Price), inventory management would suddenly always be reset because it is null. That might lead to some confusion?

Because of the support for partial updates, it feels like omitempty is the safest thing to do. Do you have an idea to get around this? Perhaps we could consider making explicit reset methods for use-cases like this where resetting a field to null is a valid thing to do. Something like ResetInventoryManagement.

Would love to hear what others have to say about this. I think it's a bit of a conundrum, even though it seems very simple :)

[ckeyes88]

Ya the more I thought about it the more I realized it wasn't as simple as changing the omitempty tag or changing the type to a pointer. I did a little research on this topic and while it definitely requires more work I came across an article that discussed this same issue (knowing whether the user explicitly set a value to nil or if they just left it off). I'll post it here for reference.

How to determine if a JSON key has been set to null or not provided

I suspect this is why, for example the AWS Golang sdk implements their own primitive datatypes i.e. aws.String which initially seemed really cumbersome but ultimately probably accomplishes something similar to what that article proposes in addition to adding pointer safety when the pointer is nil.

Thanks again for your feedback/thoughts.

[ckeyes88]

Just adding some more reference information from an issue on the Go-Github sdk discussing the exact same problem and potential solutions.

More fuel for the fire

[dlebech]

@ckeyes88 thanks for the links. This is perhaps an issue that can be tackled as part of some larger updates of the library since it also might require a breaking change.

[andrewhoff]

This issue was moved to bold-commerce#4