Icinga/icinga-notifications

Fix default value behavior for channel plugins

Closed this issue · 5 comments

oxzi commented

We (@nilmerg, @yhabteab, and I) have just discussed this offline and came to the following design conclusion:

  • absent keys are resulting in using the default value and
  • a null value results in an empty value for optional key value pairs.

Those changes must be done in the daemon or, more precisely, the channel plugins and I am going to make the necessary changes tomorrow.

Originally posted by @oxzi in Icinga/icinga-notifications-web#154 (comment)

a null value results in an empty value for optional key value pairs.

Can you please elaborate on what this is supposed to achieve?

Note that giving null in JSON some meaning might become tricky in combination with Go's encoding/json as that already has a special interpretation of null:

The JSON null value unmarshals into an interface, map, pointer, or slice by setting that Go value to nil. Because null is often used in JSON to mean “not present,” unmarshaling a JSON null into any other Go type has no effect on the value and produces no error.

Live example: https://go.dev/play/p/Nl7S7kAXZjX

a null value results in an empty value for optional key value pairs.

Can you please elaborate on what this is supposed to achieve?

Initially, this was intended to indicate that the user does not want to use the default value (if any) of an optional field of a specific channel. However, this does not seem to work if GO does not decode JSON null values into the zero initialised value of a given type.

oxzi commented

Note that giving null in JSON some meaning might become tricky in combination with Go's encoding/json as that already has a special interpretation of null:

Valid point. However, when first setting a default (or fallback) value, the struct is already partially populated and gets only overwritten when a user-configured value is stored in the JSON. As no further reconfiguration takes place, this shouldn't be an issue.

oxzi commented

In the meantime, #206 was generated to implement this in Icinga Notifications. However, as @yhabteab pointed out #206 (comment), the second requirement - a null value results in an empty value for optional key value pairs - was not met.

Unfortunately, this behavior does not work well together with Go's json.Unmarshal:

Because null is often used in JSON to mean “not present,” unmarshaling a JSON null into any other Go type has no effect on the value and produces no error.

It would be possible to work around this, but this makes third party channel plugin development way harder.

For a concrete example, setting, i.e., the email channel's sender_name to the empty string in Icinga Notifications Web results in null being written to the database. Without being familiar with the 🕸️ implementation, this seems like additional, unnecessary work on their side.

Furthermore, what should the channel plugin do for the encryption field of the option type being null?

Can we reconsider this and drop the null equals empty - and thereby using the default value for the type - rule?

oxzi commented

We discussed this again with @nilmerg and came to dropping the "null equals empty field" rule. @nilmerg came up with an implementation in Icinga/icinga-notifications-web#230.