fix(trait): bool trait props should be pointer, otherwise omitted in CR #1848

[tadayosi]

This fixes #1848

Release Note

NONE

[astefanutti]

I think there might be an underlying issue with how CLI boolean properties get mapped into trait structs. So before we patch all the traits, let me find out why.

[tadayosi]

@astefanutti I spotted the underlying issue here. So as I understand it it's essentially how mapstructure handles bool struct properties when they are annotated with omitempty.
https://github.com/tadayosi/samples-go/blob/master/pkg/mapper/mapper_test.go

I thought that BaseTrait.Enabled is originally typed as *bool because the author already noticed the limitation when creating it. If that's true I think it's natural that we all follow the rule of using *bool for all bool trait props.
https://github.com/apache/camel-k/blob/master/pkg/trait/trait_types.go#L133

[astefanutti]

@tadayosi thanks. My understanding is that the issue occurs for boolean fields for which the traits constructor default the value to true. When such a field is set to false from the CLI, it is omitted in the integration JSON, then when the trait is initialised, the field is initialised to true and not overwritten by the trait configuration unmarshalling (as the field has been omitted). Note that mapstruct is not involved here. So, indeed, this is a typical case where using a *bool is required. An alternative would be to remove json:",omitempty", but that leak defaulting into user-space.

[tadayosi]

@astefanutti Ah yes, my comment was incorrect. It is not mapstructure but JSON marshalling as you explained. Thanks for your investigation.

[tadayosi]

One more thing. In my test case affinity.pod-affinity=false is also covered which is by default false, so I don't think it's only an issue with the default value being true.

[astefanutti]

Can I suggest that we remove default trait fields initialisation from the trait factory methods? I now see it as an anti-pattern somehow, and the defaulting logic would be better move into the Configure method. That would also prevent using that lovely &[]bool{true}[0] construct 😉.

[astefanutti]

Do you mean that affinity.pod-affinity=false currently fails?

[tadayosi]

@astefanutti Yes, in the sense that {"podAffinity":false} doesn't show up in the integration's spec.traits.affinity.configuration. The test case TestConfigureTraits covers all the cases that originally failed.

[astefanutti]

@tadayosi ah right. I did not mention that in my review, as I already agree with the approach you've taken. I think that all the boolean fields should be declared as pointers, as even if the current default is false and it works, it's error prone if the default changes, and the user-provided value is not marshalled if it matches the default.

[tadayosi]

To be clear, the root cause is not whether the prop has a certain default value or not, but the way JSON marshalling handles "zero" primitive values when "omitempty" is added:
https://golang.org/pkg/encoding/json/#Marshal

The "omitempty" option specifies that the field should be omitted from the encoding if the field has an empty value, defined as false, 0, a nil pointer, a nil interface value, and any empty array, slice, map, or string.

So theoretically every prop (including bool and even int) on a trait should be declared as a pointer really. In practice, however, in most cases int props shouldn't have issues so long as assigning 0 to the prop doesn't make sense.

Tomorrow I'll follow up with Antonin's suggestion of moving the defaulting logic to Configure.

[tadayosi]

Can I suggest that we remove default trait fields initialisation from the trait factory methods? I now see it as an anti-pattern somehow, and the defaulting logic would be better move into the Configure method. That would also prevent using that lovely &[]bool{true}[0] construct 😉.

After investigation I realised that it's not as easy as I initially thought. It's because each trait's Configure method is invoked after the trait catalog's configure has already been invoked, and it is where the trait is initialised with the trait configuration from an integration.
https://github.com/apache/camel-k/blob/master/pkg/trait/trait_catalog.go#L109

It's still possible to move the defaulting logic to Configure method but the logic would then require checking on each property whether it's already initialised with the value from an integration to make sure it wouldn't override already configured values, and I'd say it's more error-prone than the current code.

Instead I just removed this fancy &[]bool{true}[0] initialisation with the latest commit for the time being.

[astefanutti]

I'd suggest to create the following method into the util.go file and use it so that the structs initialisation can be inlined:

func BoolP(b bool) *bool {
 	return &b
 }

For example, that would give:

return &affinityTrait{
 	BaseTrait: NewBaseTrait("affinity", 1300),
	PodAffinity: BoolP(false),
	PodAntiAffinity: BoolP(false),
 }

[astefanutti]

@tadayosi I see what you mean, that is convenient to keep the defaulting early in the trait struct creation.

[astefanutti]

Thanks!