fix: update empty slices in kong entities

[epikur-io]

Summary

Bugfix: Could not update service, route or plugin with empty slices because it was removed through the omitempty JSON struct tag.

Note: This bug is only fixed for services, routes or plugins. WIth other entities this problem may still occour.

^{Manuel Gugel <manuel_sebastian.gugel@mercedes-benz.com>, Mercedes-Benz Tech Innovation GmbH, imprint}

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

LUIROTH seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

---

LUIROTH seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[rainest]

As best I recall, go-kong fields are typically omitempty because it'd then have the opposite problem when PATCHing: if you provide Go with a patch struct that contains only the fields you want to change, the serialized JSON will include empty values and wipe out existing values when sent to Kong. We probably can't do this as such--even if we did want to require complete values for PATCHes, it'd be a major breaking change.

Are you able to use the Create() calls with an ID included? That issues a PUT and will knock out existing values by virtue of overwriting the full entity, though you'd need a bit more work client-side to pull down the existing object and update fields on the Go side.

Some way to override the normal JSON tags in a struct would be ideal, so that you selectively toggle omitempty off on some field for a particular request, but brief review of StackOverflow and such suggests this isn't readily possible, and would probably require a custom JSON marshaler.

Edit:

@czeslavo suggested using pointers to slices (or other types), which lets you distinguish between omission and an explicit empty slice (case b versus case c in https://go.dev/play/p/1cr0ZFBpvjK).

@zekth says https://github.com/LukaGiorgadze/gonull may let achieve the same, though I'm not fully sure what that would look like--the library docs are bit light.

Either of these would be a breaking type change, however--AFAIK there's nothing still that'd let us toggle between marshal results with the existing types, so it'd be a major release if so, and one I'd still be reluctant to make due to the upgrade burden. PUTing to clear these is probably still the better option.

[zekth]

Catching up on this, exposing fields with gonull.Nullable to external consumers is a bad idea IMO; and it was for unmarshalling rather than marshalling. @czeslavo in our case is the way to go with the struct field like:

OmitPointer    *[]*string `json:"omitpointer,omitempty"

In the current state of things i'd +1 @rainest on:

PUTing to clear these is probably still the better option.

If we want to not touch the types and have dynamic tags feature somehow; we need to add a custom marshaller in our PATCH operations:

go-kong/kong/route_service.go

Line 116 in 8f4d15c

req, err := s.client.NewRequest("PATCH", endpoint, nil, route)

So that would mean NewRequest would have to support either body interface{} or body string: https://github.com/Kong/go-kong/blob/main/kong/request.go#L65
Well, we can make 2 methods, but the burden here would be to make a custom marshaller for optional slices. At the same time i tend to like this idea.

[pmalek]

So that would mean NewRequest would have to support either body interface{} or body string: https://github.com/Kong/go-kong/blob/main/kong/request.go#L65

It does already, right? https://github.com/Kong/go-kong/blob/main/kong/request.go#L22-L37

---

I feel like this missing bit is omitunset where a zero value ( nil for pointer and zero value for non pointers) would marshal to an empty string. But that's probably what you were suggesting @zekth right?

[CyberGuestNo1]

Hi @rainest, @zekth and @pmalek,
thank you very much for your Feedback. I am the person who created the commit. Somehow my computer sent the wrong name.
Through testing we faced the same issues you addressed.
If we use the Create Method, we will always need to get all the existing data from kong first which will make it slower. And there is a small time window, in that we can overwrite another almost parallel request on another property (between get and create calls)

I also though of a custom Mashaller for the entities that can dynamically remove nil values but keep empty values. Would this work for you?

func (r *Route) MarshalJSON() ([]byte, error) {
	// Create an alias to avoid infinite recursion
	type Alias Route
	aux := &struct {
		*Alias
	}{
		Alias: (*Alias)(r),
	}

	// Marshal the struct to JSON bytes
	jsonBytes, err := json.Marshal(aux)
	if err != nil {
		return nil, err
	}

	// Unmarshal the JSON bytes into a map
	var data map[string]any
	err = json.Unmarshal(jsonBytes, &data)
	if err != nil {
		return nil, err
	}

	// Remove null values recursively from the map
	r.removeNullValues(data)

	// Marshal the modified map back to JSON bytes
	return json.Marshal(data)
}



func (r *Route) removeNullValues(data map[string]any) {
	for key, value := range data {
		if value == nil {
			delete(data, key)
		} else if nestedMap, ok := value.(map[string]any); ok {
			r.removeNullValues(nestedMap)
		}
	}
}

With this it should be possible ignore nil values but keep the empty values.

[zekth]

@pmalek my bad i haven't dug deep into the implementation, assuming the underlying method wasn't doing any reflection.

@CyberGuestNo1 for this case that would mean we won't be able to set back a value to its default from kong POV. Let's take example key-auth: https://docs.konghq.com/hub/kong-inc/key-auth/configuration/

If you want to fallback on kong default you'd need to send []string{"apikey"} otherwise it would not send the null value, which is technically what we would want to send in a likely case, and sending an empty slice isn't a solution neither.

After twisting this from one way to another, if we really want to achieve true patch experience we need to implement a nullable carrier from types like suggested earlier but that would be a big breaking change and that would only apply for PATCH operations. This is the kind of situation where json patch shines: https://jsonpatch.com/

[CyberGuestNo1]

@zekth yes you are right. We should not change the behaviour of the config in plugins or nested objects.
For our implementation it would already work if we handle the first layer or properties. Like this:

func (r *Route) MarshalJSON() ([]byte, error) {
	// Create an alias to avoid infinite recursion
	type Alias Route
	aux := &struct {
		*Alias
	}{
		Alias: (*Alias)(r),
	}

	// Marshal the struct to JSON bytes
	jsonBytes, err := json.Marshal(aux)
	if err != nil {
		return nil, err
	}

	// Unmarshal the JSON bytes into a map
	var data map[string]any
	err = json.Unmarshal(jsonBytes, &data)
	if err != nil {
		return nil, err
	}

	// Remove null values in first layer from the map
	for key, value := range data {
		if value == nil {
			delete(data, key)
		}
	}

	// Marshal the modified map back to JSON bytes
	return json.Marshal(data)
}

I think jsonpatch would make this issue much more complicated. The concrete issue we are facing is that we can't set the "methods" property of a route to an empty list with the Update Method.

[Vikash08Mishra]

I encountered similar issue for a PATCH request, just that in this case nil value should not be omitted. So while the above approach works for a given constraint (skipping nil but not empty), it may not work for others. I have similar suggestion but a slightly different approach captured in ticket. would be happy to get community feedback.
#481