Malfunctions around boolean parameters and defaulting

[maxb]

Vault has a few boolean API parameters, which default to true.

Not many, but some, such as auth/token/create's renewable.

At the moment the API models are autogenerated with the omitempty behaviour applied.

Generally this is a good thing.

But in this case:

• Parameter is false in request model => omitted on serialization => server defaults it to true
• Parameter is true in request model => included in serialization => server receives explicit setting true

There is no way to use this client library to set such things to false.

In general the same problem exists for other parameter types too, any time the Go zero value of the datatype would be meaningfully different to the API, compared with not specifying the parameter - e.g.:

• Setting a numeric parameter to zero, when it has a non-zero default in the API definition
• Setting a string parameter to the empty string, in an attempt to clear a value stored on the server

This could be a bit tricky to deal with.

[averche]

Yes, this is definitely a problem!

One solution I was thinking of is to make these into nullable in the OpenAPI doc (pointers in Go). This way if the value is nil or not specified in code, it will be omitted (per omittempty). If, on the other hand the value is specified, it will be set in the request JSON: https://go.dev/play/p/btklv8p22Dc

[maxb]

I don't think setting these parameters as nullable would be technically correct - there's a sentence in the part of spec you linked to:

In objects, a nullable property is not the same as an optional property,

I believe using nullable woud be making the spec claim that it would be appropriate to send literal JSON like {"renewable": null} to the Vault API - this is a the case of an optional property, rather than a nullable one.

But the sentence in the documentation continues:

but some tools may choose to map an optional property to the null value.

which I read as blessing to tweak the templates to use pointer types where needed, for optional properties.

I think the hard part will be figuring out which parameters need this treatment, as it wouldn't be a great user experience to do this for the majority of parameters.

[averche]

Perhaps another angle is to conditionally make the field a pointer if it has a default property defined in the OpenAPI spec. For example:

          "renewable": {
            "type": "boolean",
            "description": "Allow token to be renewed past its initial TTL up to system/mount maximum TTL",
            "default": true
          },

has a default and would result in

{
   // ...
   Renewable *bool `json:"renewable,omitempty"`
   // ...
}

[maxb]

I've been thinking more about this. It seems to be a really hard problem, to the point where I wonder if there even is a good solution.

Boolean parameters are only the tip of the iceberg. This problem can affect most/all types.

Similarly, defaulting is not a clear sign of which parameters have or do not have this problem.

Here are two case studies to prove this:

First: Consider the API update identity/group/id/{id} i.e. client.Identity.GroupUpdateById. There is a critical semantic difference between:

1. Setting policies to an empty array => removes all policies from the group
2. Not setting policies to anything => leaves the policies on the group unchanged

Ironically, even though Go's slice type does differentiate between a null and an empty slice, Go's JSON library does not provide a feature to omit null slices whilst serialising empty ones.

Second: Consider the API update transit/keys/:name/config i.e. client.Secrets.TransitConfigureKey. There is a critical semantic difference between:

1. Setting min_encryption_version to 0 => configures only the current key version to be available for encryption
2. Not setting min_encryption_version to anything => leaves previous policy unchanged

---

So, what can be done?

I have no suggestion that I actually like.

One option would be to use pointers for every single request body parameter. But then, even if we ship a helper function like:

func Ptr[T any](input T) *T {
	return &input
}

people's code still ends up looking like:

	resp, err := client.Secrets.PkiIssueWithRole(ctx, "example", schema.PkiIssueWithRoleRequest{
		CommonName:        vault.Ptr("something.example.maxb.eu"),
		AltNames:          vault.Ptr("other.example.maxb.eu,more.example.maxb.eu"),
		ExcludeCnFromSans: vault.Ptr(true),
		Format:            vault.Ptr("pem"),
		UriSans:           vault.Ptr([]string{"foo", "bar", "baz"}),
	})

which is a bit on the ugly side.

Another option would be to totally flip the API design, so that all parameter values need to be specified via setter functions:

	resp, err := client.Secrets.PkiIssueWithRole(ctx, "example").
		CommonName("something.example.maxb.eu").
		AltNames("other.example.maxb.eu,more.example.maxb.eu").
		ExcludeCnFromSans(true).
		Format("pem").
		UriSans([]string{"foo", "bar", "baz"}).
		Execute()

but that would mean generating a huge amount more lines of code, and I suspect need some complex code to do the actual JSON serialization afterwards.

Tricky...

[averche]

Similarly, defaulting is not a clear sign of which parameters have or do not have this problem.

You are right, I did not think of the edge cases you mentioned.

First: Consider the API update identity/group/id/{id} i.e. client.Identity.GroupUpdateById. There is a critical semantic difference between:

1. Setting `policies` to an empty array => removes all policies from the group
2. Not setting `policies` to anything => leaves the policies on the group unchanged

Do you think this applies to all/most of the array-type request parameters? If so, we can detect them with {{ if isArray }} in templates and change the type to pointers to slice instead (e.g. *[]string).

Second: Consider the API update transit/keys/:name/config i.e. client.Secrets.TransitConfigureKey. There is a critical semantic difference between:

1. Setting `min_encryption_version` to 0 => configures only the current key version to be available for encryption
2. Not setting `min_encryption_version` to anything => leaves previous policy unchanged

This seems like a very good example of something that should be flagged as "nullable" (an empty/null value has different semantic than the "zero" value).

So, what can be done?

Out of the two solutions you suggested, I think I prefer the first one. It is closer to what we used to have with the default-generated templates.

Maybe a good middle-ground solution could be to define the set of conditions under which a request field should be changed into a pointer by the generation templates. For example:

1. field has a default value
2. field is of an array type (not sure about this one)
3. field is nullable (this would require us to manually identify such edge cases and flag them as such in OpenAPI)

Most of the request fields would not fall into these 3 categories and should provide a better UX. But for the ones that do, we can provide a Ptr[T] helper as you suggested.

[maxb]

I have a new, slightly weird, proposal but I really like it and I think it could lead to the optimal Go developer experience:

What if we did one of two things, so we could insert our own semantics for omitting fields:

Option 1: Fork the encoding/json library

Option 2: Fork or rewrite just enough code to translate request model structs to map[string]interface{} instances ourselves - allowing us to decide whether to include or omit fields - before sending those maps to encoding/json to serialize.

In either option, the semantics we would implement would be:

• Maps: Omit nil maps, but render empty initialized maps.
• Slices: Omit nil slices, but render empty initialized slices
• Basic types: Before rendering a hypothetical Renewable bool struct field, check to see if a Renewable_SendZeroValue bool field exists in the same struct. Allow the {{name}}_SendZeroValue field to control whether zero values in {{name}} are omitted.

and then in the openapi-generator templates, generate the {{name}}_SendZeroValue fields alongside each existing field of a basic type.

Ways in which this is good

For most cases, developers continue to be able to use the library in the simple way they do today. The common case remains undisrupted by needing to handle the uncommon case.

All existing code continues to compile and work the same.

Developers wanting to set, or not set, array or object fields have the behaviour just work.

Ways in which this is bad

We need to do more work to get there.

A developer wanting to use the library to send a 0, "", or false parameter to the Vault API has to be aware of the need to set the {{name}}_SendZeroValue field too. (However the very nature of the Go language forces developers to learn about zero-value initialized structs.)

The {{name}}_SendZeroValue fields will exist for all basic typed fields - a lot of which will not trigger have any meaningful difference on the Vault server side, (However this is unavoidable unless we manually analyse every field in the Vault API and tag it with a "zero value has semantic meaning" flag.)

[averche]

Maps: Omit nil maps, but render empty initialized maps.
Slices: Omit nil slices, but render empty initialized slices

I like this idea :)

and then in the openapi-generator templates, generate the {{name}}_SendZeroValue fields alongside each existing field of a basic type.

My fist thought is I'm not particularly fond of having {{name}}_SendZeroValue alongside every single field. It also would not be immediately obvious to me, as a user of the library, what to do with this field. For example, the user might wonder what happens if Renewable = true and Renewable_SendZeroValue = true?

On the other hand, when a boolean field (e.g. Renewable *bool) is declared as a pointer, most Go developers would immediately make the correct assumption that this field will be omitted if nil and sent with the given value (true or false) otherwise.

I'll be away on a vacation for a few weeks but I'll think about this some more :)

[maxb]

There are absolutely tradeoffs here. No option is obviously the best.

My fist thought is I'm not particularly fond of having {{name}}_SendZeroValue alongside every single field.

It's not great, I agree. However it has the potential to be less intrusive into actual use cases than everything being a pointer type even for fields which don't have any need to be. _SendZeroValue can just be ignored and not used when not needed, whereas direct assignment to pointer types frustratingly is messy.

It also would not be immediately obvious to me, as a user of the library, what to do with this field.

I agree, but I think whichever option is taken, some dedicated documentation is going to be needed to explain the library's approach. If the pointer path was taken, the documentation would need explain how to set literal values, and explain why pointers have been used all over the place even when not really needed by the precise API semantics - the natural expectation would be the technique was only used where necessary.

For example, the user might wonder what happens if Renewable = true and Renewable_SendZeroValue = true?

I was thinking that _SendZeroValue would be ignored if the value was not the zero value for the type.

Anyway - I acknowledge this isn't a clear cut choice. It might be worth putting a description, and some sample code, in front of as many potential users as you can find.