Primitives should not be pointers

[jhorwit2]

It'd be nice to remove the requirement on pointers for everything. It becomes very burdensome on consumers of the SDK when they're unable to easily create objects and are required to work around pointers. It also significantly decreases the readability code using the SDK.

For example lets take the following struct (some fields have been removed).

type CreateBackendDetails struct {

	// The IP address of the backend server.
	// Example: `10.10.10.4`
	IpAddress *string `mandatory:"true" json:"ipAddress,omitempty"`

	// The communication port for the backend server.
	// Example: `8080`
	Port *int `mandatory:"true" json:"port,omitempty"`
}

The idiomatic way to create an object for this struct would be

cbd := &CreateBackendDetails{
  IpAddress: "127.0.0.1",
  Port: 80,
}

However with how the structs are setup you're required to do this.

ip := "127.0.0.1"
port := 80
cbd := &CreateBackendDetails{
  IpAddress: &ip,
  Port: &port,
}

As you can imagine on larger structs that becomes a lot of unneeded boilerplate.

[eginez]

I hear your pain, we debated extensively about this particular issue, however due to go lang zero value initialization it is impossible for us to tell difference from say: an empty string set by the user of the sdk and a string initialized to empty by the run time.
This seems to be an issue present on other projects, and they have taken similar decisions
eg: google/go-github#19. as well as protobuf: https://github.com/golang/protobuf

To alleviate the pain of using pointers we provide helper functions so you can more easily write idiomatic go code

cbd := &CreateBackendDetails{
  IpAddress: common.String("127.0.0.1"),
  Port: common.Int(80),
}

I am however happy to hear other ideas on how to handle this issue

[jhorwit2]

Are there any conditions where that matters? The original SDK didn't have any apparent problems with that.

What about maps? I see spots where those are defined as*map[foo]bar. Maps and slices can be nil.

https://play.golang.org/p/12pq3pQ0ul

[eginez]

It is imperative for our sdk to be able to tell if something has been set by the the user or not. Indeed we have not added helper functions for maps and slices yet, feel free to open an issue for that

[jhorwit2]

You don't need them for maps or slices. Please take a look at the playground I put together for you here that showcases this: https://play.golang.org/p/IniB6kOqQe

Are there specific reasons every field needs to be this way? The original terraform SDK doesn't treat values this way and works very well with the OCI API and is much more user friendly.

To be fair, I understand the need to have optional values as that comes up in Kubernetes core and other go projects fairly often. The typical solution for that is to have only those variables be optional since they denote something special. For example, take boolean fields like backend drain which has a default of false and there is no ternary value requirement. An optional value for this field appears to provide no value.

[eginez]

I missed that, you are correct, for containers(slices and maps) we should make it a non-pointer field. I'll create an issue for that in github, we have been tracking this issue internally as well
However nil != false != true. Conflating nil with false is not correct. I am a little confused with your statement. Are you saying that only optional values with no default should be pointers?

[jhorwit2]

I agree that nil != false != true. For fields like Backup which should only be true or false and don't need a ternary value (nil), I don't see any value in making it optional (have a pointer). Making the field type bool defaults to the correct value.

Here is a string example. A load balancers display name cannot be null or empty according to the docs so why have that be a pointer? It's redundant since the default string value is invalid for the same reason nil is. Unless I'm mistaken, the SDK currently would let me set the DisplayName to common.String(""). The mandatory struct tag is only checking nil. Having these ternary checks increases the complexity of the client itself as well since you now have more cases to check. mandatory for strings should be checking for empty string.

Most int cases I see don't require an optional value either. Lets take the backend port for example. 0 is not a valid port for a backend so you could simply check for that versus checking for nil.

A case where optional does make sense is the port for a healthchecker since if that's not specified it will default to the backend port as per the docs.

[eginez]

@jhorwit2 Sorry for the delay in responding to this. I understand that in some cases the zero values in a struct are acceptable values, however relying on zero values makes the sdk brittle given that there is no guarantee new fields defaults match zero values. Further it would make the sdk inconsistent where some values are expected to be pointers while others are not. Finally we do alleviate the usage of pointers by providing helper functions to, which should address your initial concern