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

[andrewhoff]

From @ckeyes88 on July 17, 2018 16:39

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.

Copied from original issue: getconversio#104

[andrewhoff]

From @ckeyes88 on July 17, 2018 20:40

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.

[andrewhoff]

From @ckeyes88 on July 19, 2018 17:10

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.

[andrewhoff]

From @dlebech on July 19, 2018 18:43

@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?

[andrewhoff]

From @ckeyes88 on July 19, 2018 20:55

@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.

[andrewhoff]

From @dlebech on July 21, 2018 6:59

@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 :)

[andrewhoff]

From @ckeyes88 on July 23, 2018 16:26

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.

[andrewhoff]

From @ckeyes88 on July 23, 2018 17:5

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

[andrewhoff]

From @dlebech on July 30, 2018 21:54

@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.

[gcfresh]

The complication with nullable fields and omit empty is applicable to other fields as well. For example, a variant update has a CompareAtPrice nullable field, which is also omitempty. As a result there currently is no method that allows setting the CompareAtPrice to null since null will be omitted.

This seems to be a very old issue as mentioned here.

Solutions to the problem are non-trivial and any suggestions are welcome. Some ideas that have been put forth include:

• Using pointers to nullable types. This may introduce new dependencies to the library to get all types supported. It would be a breaking change and would require versioning and a large overhaul to the entire library.
• Define custom types with fields that identify Set, Valid, Value similar to this.
• Creating new endpoint using a new model that would allow patching. Would still need to determine a suitable storage type for the nullable/omitempty fields.
• Define a fixed value in the existing container range to represent null. This may hide some functionality and may not be feasible for all types.

[BoldBrandonM]

Following up to echo the concerns here. This is an issue for us at Bold as well, as it can cause us two types of shortcomings:

• inability to send 'nil' values of non-reference primitive datatypes via clients (cannot set 'false' of a bool, or '0' of an int, etc.)
• inability to clear / remove values via null on the JSON for non-nullable datatypes (cannot set null via string, etc.)

These two cases and combinations of them have caused various issues on our side and will need to be addressed as a part of the next major version due to the breaking nature of the fixes required.

For posterity, the solution we are weighing most towards is a mixture of pointers and nullable types based on the constraints on Shopify (nullable fields will use a nullable type, and fields where nil values are relevant will use a pointer. As much as the concerns about increasing complexity of the library are valid, this seems to be the cleanest and most usable approach out of those listed above without limiting the capabilities of the client.

[c9845]

Is the Stripe style method of handling nullable values the solution here? Stripe uses pointers to values, and then when constructing the struct you use stripe.String() or stripe.Int64(), both of which return pointers to their underlying values.

For example: https://pkg.go.dev/github.com/stripe/stripe-go#ChargeParams and https://pkg.go.dev/github.com/stripe/stripe-go#String

[oliver006]

Yeah, that's a way to handle it. It's a massive change to the library, touching virtually every file and struct and all the tests, something I personally don't have time right now.

So that's why I'm currently leaning towards "leave as is" and get the 4.0 release done to unblock releasing the recently merged changes.

[c9845]

Oh yeah, its a huge breaking rewrite, I get it.

The question of implementation also becomes, do you use *string, *int, etc. for all fields, or only fields that are nullable per the Shopify API? If this is only implemented for some fields, this becomes even more work since someone would have to have very in-depth knowledge of which fields are nullable or not. I think just implementing pointers for nearly everything except the definitely-are-not-and-will-never-be nullable fields is the way to go, but regardless this is going to be a pain. I will try to look into rewriting this, but I would need to carve out a lot of time.

[oliver006]

I think just implementing pointers for nearly everything except the definitely-are-not-and-will-never-be nullable fields is the way to go, but regardless this is going to be a pain.

Agreed on all points.

Let me know if you're down to take on that larger rewrite and I'll hold off with the 4.0 release but I certainly don't have the time right now so in absence of someone stepping up I would just ship 4.0 without the nullable stuff.

[oliver006]

Ok, looks like it's not realistic to change all the fields for the upcoming 4.0 release.

I opened #275 to make at least a few changes. So far this covers fields raised in #182 only.

Any candidates from this thread that should be added? If yes please highlight which field exactly (a lot of field names are repeated in multiple places) and I'll make the change, otherwise we'll table this issue until 5.0 (or just close it).

[BoldBrandonM]

I believe there are quite a lot of fields which are affected but given they are not reported explicitly in issues for the project, i'd be fine if we just consider to idiomatically address it across the types in a future v5.0. We might be taking a step backwards in terms of usability if a subset of fields work in a different way.