Remove omitempty from Client lists

[ForbesLindesay]

See alexkappa/terraform-provider-auth0#50

Auth0 requires you to explicitly specify the empty array

[alexkappa]

Hi @ForbesLindesay, thank you for taking a stab at this.

I'm not sure that we can remove omitempty for all these fields. It would make PATCHing a lot more difficult as the unset values would be misinterpreted as empty values. For example, updating a client like the following:

api.Client.Update(id, &management.Client{
    Description: auth0.String("Some description")
})

Will have the side effect of setting callbacks, allowed_origins and other such values to an empty array.

The exception of ClientGrant, where scopes has no omitempty is because both POST and PATCH need scopes to be defined - or omitted defaulting to an empty array.

[ForbesLindesay]

OK, but how do you explicitly set allowed_origins to the empty array? Currently when you set it to the empty array, nothing is sent to Auth0 so the change does not apply.

[alexkappa]

Honestly, I don't know of a way to support this correctly as the semantics of omitempty make it difficult to support such operations. Perhaps turning []<type> to *[]<type>, but I'm afraid usage would suffer greatly and it would be a major BC break.

[ForbesLindesay]

Can we please do that across the board? I understand this could be a breaking change, but the current behaviour is completely broken too. I think we should always be treating empty arrays as separate from null values. The same goes for empty strings too. I should be able to use the API to set an app's description to the empty string, but I think the omitempty won't let me do that either.

[alexkappa]

I understand, this is not correct and we have to fix it. I did some preliminary work on the sliceptr branch, perhaps we can start there and add to other resources.

It is a backwards incompatible change though so we need to think about the rollout strategy with gopkg and module support.

I’d also like to see how other packages do this sort of thing with slices and maps, cause it will have a big impact in usability.

Strings, ints, floats, bools, times etc are correctly supported as they are already defined as pointers, therefore values such as “”, 0, false will be marshaled correctly.

[ForbesLindesay]

I'm still learning go, so I'm not certain of the most idiomatic ways of doing things, but it seems to me that arrays already have nil as the default value, not the empty array, so it should be fine to just have the json serialisation omit nil but keep the empty array. e.g.

package main

import "fmt"

func main() {
	var s []int
	fmt.Println("s", s, len(s), cap(s), s == nil)
	var t []int = []int {}
	fmt.Println("t", t, len(t), cap(t), t == nil)
}

Prints:

s [] 0 0 true
t [] 0 0 false

Note that because I set t to the empty array, t == nil was false. The strings/numbers needed to be pointers to add nil as a possible value, but the arrays/slices do not.

[alexkappa]

so it should be fine to just have the json serialisation omit nil but keep the empty array

I really wish it was that easy, but unfortunately json.Marshal will omit the empty slice if omitempty is defined, even if the slice not nil.

type A struct{
	S []string `json:"s,omitempty"`
}

func main() {
	for _, a := range []*A{
		{},
		{S: nil},
		{S: []string{}},
		{S: []string{""}},
		{S: []string{"foo"}},
		{S: make([]string, 0, 0)},
		{S: make([]string, 0, 1)},
		{S: make([]string, 1, 1)},
	}{
		b, _ := json.Marshal(a)
		fmt.Println(string(b), a.S, len(a.S), cap(a.S), a.S == nil)
	}
}

The output is:

{} [] 0 0 true
{} [] 0 0 true
{} [] 0 0 false
{"s":[""]} [] 1 1 false
{"s":["foo"]} [foo] 1 1 false
{} [] 0 0 false
{} [] 0 1 false
{"s":[""]} [] 1 1 false

https://play.golang.com/p/XNl3cEC_Q8p

I've been looking around in other client SDKs, like aws-sdk-go, go-github etc, and they all follow the convention that protobuf came up with – which is to define non-repeated fields as pointers. I can't see any mention of slices/maps, but they don't define them as pointers. It would also be quite weird, as slices and maps are already pointers.

The closest reference I could find with marshalling an empty slice, is the proposal of omitnil in golang/go/issues/22480.

I am sure I'm missing something, as we can't be the only ones hitting this issue.

[ForbesLindesay]

The omitnil looks like it would fix the issue, but it doesn't look like it's being very actively worked on. One option is to write a custom JSON marshaler for the Client struct that omits the nil slices but still serializes the empty slices correctly. I think the simplest option is probably to use a pointer to a slice.

[ForbesLindesay]

It looks like aws-sdk-go uses their own JSON encoding module: https://godoc.org/github.com/aws/aws-sdk-go/private/protocol/json/jsonutil that is pretty much a complete re-implementation

[ForbesLindesay]

go-github are using the standard json encoding module: https://github.com/google/go-github/blob/4e1a504644bfd34564773b2f8c4c2ba91c8e21ec/github/github.go#L323-L331

[ForbesLindesay]

go-github are getting round the issue by having methods for "add assignees" and "remove assignees" rather than having a "set assignees" method: https://github.com/google/go-github/blob/60d040d2dafa18fa3e86cbf22fbc3208ef9ef1e0/github/issues_assignees.go#L51-L85

If we wanted to mimic that approach, we would have a method for each list to add/remove items, but that doesn't seem great.

[ForbesLindesay]

The aws json implementation omits nil slices here: https://github.com/aws/aws-sdk-go/blob/a59596beb512ad4327d1e109267224f01653d877/private/protocol/json/jsonutil/build.go#L121-L123

[bishtawi]

What if we change the lists to be pointers, like how all the primitives (bool, string, int) are pointers? That way a nil value would still be omitted when serializing to JSON, but the empty list would get serialized? Apart from being a breaking change to anyone downstream who uses this lib, are there any negative effects?

[sergiught]

Hello everyone, as discussed above the changes proposed in this PR will have unintended side effects. We have captured all the relevant information under a JIRA ticket and we'll work on fixing this in an upcoming version.