Icinga / icinga-notifications

Icinga Notifications: new and improved notifications and incident management for Icinga (work in progress, not ready for production yet)
GNU General Public License v2.0
9 stars 0 forks source link

Fix default value behavior for channel plugins #205

Closed oxzi closed 2 months ago

oxzi commented 4 months ago

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

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 https://github.com/Icinga/icinga-notifications-web/issues/154#issuecomment-2135583142

julianbrost commented 3 months ago

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

yhabteab commented 3 months ago

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 3 months ago

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 2 months ago

In the meantime, #206 was generated to implement this in Icinga Notifications. However, as @yhabteab pointed out https://github.com/Icinga/icinga-notifications/pull/206#issuecomment-2165570555, 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 :spider_web: 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 2 months ago

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