Write a hint for empty arrays/dicts into customvar_flat

[yhabteab]

Just inserts empty custom vars of type array & map with NULL value in to the customvar_flat table.

fixes #597

[julianbrost]

Why does Icinga DB Web use the customvar_flat table for displaying there instead of customvar?

From my understanding, customvar_flat is an extra table to allow Icinga DB Web to implement some filtering capabilities on arrays/dictionaries in there. But after all, it's deliberately lossy, see also for example how in your example, you can't distinguish between vars["example.com"] and vars["example"]["com"] in customvar_flat.

[lippserd]

I second @julianbrost's opinion.

[nilmerg]

Because of deny lists and protection rules. They are evaluated on the database, based on the same rules the user is comfortable with using them in the restrictions and search bar.

[julianbrost]

I'm not sure if I fully understand and how restrictions play into this. Is this change also useful for the filter/restriction evaluation you're doing? Or is this more like that you want to display information that's as close to to underlying data that will be used for restrictions?

[nilmerg]

It's an implementation detail, nothing more. We don't need to go into detail for this, just think about custom variables that shouldn't be visible, that aren't fetched from the db at all instead of filtering them out later on.

[julianbrost]

Wait, are you talking about some mechanism that controls the visibility of individual custom vars for users? If so, I wasn't even aware that this existed and I thought you were referring to host/service permission filters based on custom vars.

[nilmerg]

https://icinga.com/docs/icinga-db-web/latest/doc/04-Security/#denylists
https://icinga.com/docs/icinga-db-web/latest/doc/04-Security/#variable-paths

[yhabteab]

But after all, it's deliberately lossy, see also for example how in your example, you can't distinguish between vars["example.com"] and vars["example"]["com"] in customvar_flat.

This also appears to be another bug that Alex didn't classify as such on Icingadb side, though it doesn't relate to this fix.

[julianbrost]

I mean the primary user of all of this is Icinga DB Web, so if you say you want/need that, we can change it.

But after all, it's deliberately lossy, see also for example how in your example, you can't distinguish between vars["example.com"] and vars["example"]["com"] in customvar_flat.

This also appears to be another bug that Alex didn't classify as such on Icingadb side, though it doesn't relate to this fix.

Yeah, I knew I've recently seen something like that. However, that couldn't be changed in the customvar_flat table without changing the format in a way that would break existing (stored) filters (search, object restrictions, and as I've learned today, customvar denylists as well).

[yhabteab]

Why does Icinga DB Web use the customvar_flat table for displaying there instead of customvar?

I mean the primary user of all of this is Icinga DB Web, so if you say you want/need that, we can change it.

This PR isn't supposed to fix only the representation problem in the object details, but also for filtering. You already mentioned above that the customvar_flat table is intended to be used for filtering, but if these variables aren't properly flattened, you can't filter by them either. Wether it's wrong/correct to use the customvar_flat table to display the custom vars in object details is quite out of this scope.

@nilmerg also has clearly described in the referenced issue. Some custom variables are not visible in an object's detail and are not available as filter.

So in the end, this PR only fixes some minor bugs, though doesn't break nor introduce any undesirable behaviors.

[julianbrost]

There are two ways to review this. One would be to answer the question whether this writes flatname=$x and flatvalue=[] to customvar_flat if there is an empty list at $x. The other would be to understand the actual problem and see if this is the most viable solution.

Regarding filtering, can you provide an example how this change would actually be useful in this regard?

I tried to play around with list filters quickly, and added vars.mixed_list = "[]" (this should result in exactly the same contents in customvar_flat as this PR if it was an empty list) to one host and vars.mixed_list = ["foo", "bar"] to another. The behavior I got from Icinga DB (current icinga/icingaweb2:master image on Docker Hub) with that was quite strange: when I type "mixed" into the search bar, two visually indistinguishable suggestion "Host mixed_list" show up, when I actually use them, one expands to column host.vars.mixed_list and suggests {} as values, the other expands to column host.vars.mixed_list[*] and suggests foo/bar as values. Also, filtering for host.vars.mixed_list=* only gives the host where vars.mixed_list is an empty list, not the one where it actually contains values. With the customvar_flat table without this PR, what Icinga DB Web does works perfectly fine for "contains an element that matches the filter" (which does not have to know about empty lists, that's why these are not written to customvar_flat so far) and with this change, things get weird.

So does this PR do what it advertises? Probably yes. Am I convinced that what it does is actually useful? Not really.

[yhabteab]

when I type "mixed" into the search bar, two visually indistinguishable suggestion "Host mixed_list" show up,

This might yet another cosmetic bug that needs to be fixed but behaves exactly like how I would expect.

Also, filtering for host.vars.mixed_list=* only gives the host where vars.mixed_list is an empty list, not the one where it actually contains values.

This also behaves exactly like how I would expect it to behave, since your custom variable mixed_list that actually contains some values in it does not have mixed_list as a flat name, but mixed_list[0] or whatever. If you want to match checkables that have the custom variable mixed_list set in any way, you need to adjust your filter to something like this.

[julianbrost]

I understand what's going on and that's basically why I tried this, to check if Icinga DB Web would do any fancy magic that expands filter conditions in ways I didn't expect it to. For you, it works as expected because you know the implementation of the filters and what happens is exactly what was implemented.

But does it make sense for a user? Would users typically understand that to filter hosts that have vars.test set to anything (something I'd expect to be able to do if hosts.vars.test=* matched an empty list), they have to build an OR condition over the following three filters?

• hosts.vars.test=* - all hosts where it's a scalar value or an empty list or an empty dict (or a complex object like a function that got mangled to a string during serialization)
• hosts.vars.test[*]*=* - all hosts where it's a non-empty list
• hosts.vars.test.*=* - all hosts where it's a non-empty dictionary

Adapting your suggestion from the screenshot, this would be host.vars.test*=* here, which might be good enough in some cases but is not really correct as it would also match vars.testy as well.

[julianbrost]

It could make sense to always emit []/{} even for non-empty lists/dicts as a kind of "there is a list/dict here" marker without making any statement about its contents. That would make the host.vars.test=* filter just work for "give me everything where this is set to anything". On the other hand, it might be strange if host.vars.test=[] would also return hosts with non-empty lists there.

Maybe it could make sense to add some type column, that can then be used to mark "there's a list here" without providing a flatvalue at that particular flatname.

But in any case, it would make sense to understand what the underlying issues are. For displaying all vars that's clear, but for filtering, examples of what should be possible but can't be expressed at the moment would be helpful (buzzword: user stories).

[yhabteab]

This also behaves exactly like how I would expect it to behave, since your custom variable mixed_list that actually contains some values in it does not have mixed_list as a flat name, but mixed_list[0] or whatever.

Actually, Icinga DB Web should solve this internally automatically (filtering by mixed_list=* should also match custom variables that have mixed_list[*] or mixed_list.* as flatname), just like in the monitoring module, though @nilmerg can say more about this.

But in any case, it would make sense to understand what the underlying issues are. For displaying all vars that's clear, but for filtering, examples of what should be possible but can't be expressed at the moment would be helpful (buzzword: user stories).

Isn't the goal of Icinga DB to be able to replace the monitoring module? If the user was able to filter for something like _service_mixed_list={} or _service_mixed_list=* in monitoring module, shouldn't Icinga DB Web provide the same functionality?

[julianbrost]

Suggestion from a whiteboard discussion:

• Make flatvalue NULL-able.
• For an empty list vars.empty_list = [], emit flatname = 'empty_list[]', flatvalue=NULL.
• For an empty dict vars.empty_dict = {}, emit flatname = 'empty_dict.', flatvalue=NULL.

flatvalue=NULL should allows filter in Icinga DB Web work as. NULL is not a value, so it does not appear like there would be magic values appearing. Web may need some adaptions in the search bar to provide proper suggestions for flatvalue ending with [] and ..

[julianbrost]

Does this work properly without any extra changes in Icinga DB Web? Or is there already a PR?

[nilmerg]

The current attempt to solve the display and search issues with empty variables doesn't make much sense to me anymore.
Take a look at the search bar changes for example. We need the suffixes (. and []) to show the user that a given variable is an empty dict or list (respectively).
Though, if one object uses the name "foo" to define an integer and another object uses it to define a string, we just show the name "foo" once and then the user can differentiate by the search value.

That is, we ignore the fact that the value of a variable might be of interest (name conflict) and we don't (empty lists/dicts).

So the suffixes are an entirely new thing, just to indicate empty lists or dicts. There's no other use or benefit. The trailing dot also causes issues with other parts in the Web code base, which have no notion of "custom variables". (Which they shouldn't have, that's fine)

So let's drop the suffixes, please. Yes, the searchbar cannot indicate an empty dict or list then anymore.

Though, what I'd keep is that empty lists and dicts are written to the customvar_flat table. Otherwise they're not shown anymore in the searchbar and are not visible in an object detail.
I also don't see this as a deviation of what the customvar_flat table is meant to be. Technically, it contains all concrete custom variable values. And their paths. But not any of the intermediate values or types. That's also the reason why empty variables weren't there in the first place, as they have no concrete value, as they're empty. Though, isn't the empty value the concrete value then? So, inserting a flatvalue of NULL into the database for such variables, is fine to me with regard to these assumptions.

With a flatvalue of NULL for only empty dicts and lists, the search bar can show them, the user can filter for them and they can be displayed in an object detail.

[julianbrost]

Though, if one object uses the name "foo" to define an integer and another object uses it to define a string, we just show the name "foo" once and then the user can differentiate by the search value.

Isn't that just a side effect of everything being represented as strings in the database? When it's the same name mixed with values of type string and array on different objects, it's shown twice at the moment.

Though, isn't the empty value the concrete value then?

It's whatever we define it to be. For the current implementation (as in current master), there is a value if there is a scalar value at that path/flatname.

With a flatvalue of NULL for only empty dicts and lists, the search bar can show them, the user can filter for them and they can be displayed in an object detail.

So empty arrays and dicts will become indistinguishable in the database just like they already are in the current PHP implementation?

the search bar can show them, the user can filter for them

Can you briefly explain what filters could be performed and how these interact with empty arrays (or also empty dicts for that matter if you can't distinguish them).

[nilmerg]

So empty arrays and dicts will become indistinguishable in the database?

Yes. The only thing which gets lost then is still the (Array) suffix. The indication of 0 Items is still there in both cases.

Can you briefly explain what filters could be performed and how these interact with empty arrays (or also empty dicts for that matter if you can't distinguish them).

• dict!~* (flatname="empty_dict" AND flatvalue IS NULL)
• dict.key=value (flatname="dict.key" AND flatvalue="value")
• list!~* (flatname="empty_list" AND flatvalue IS NULL)
• list[*]~3 (flatname LIKE "list[%]" AND flatvalue = 3)

Of course, while just dict cannot occur as suggestion in the searchbar with a non-empty value, the latter two cases are both shown as Service list for example. To solve this, the list[*] filter could be labelled as Service list (Array) or Service list (contains).

[julianbrost]

So in regards to expressiveness of search filters, this would allow x!=* (or x!~* as that recently changed?) to be used as a filter for "there exists an empty array or dict at x"?

I'm not entirely sure what that did previously. Would it never match as flatvalue IS NULL could never match in the past? Or could x!=* be used to filter for "x is not set"?

[nilmerg]

(Yes.) Yes.

Yes. No.

[julianbrost]

So to summarize, the full list of things this PR (in combination with Icinga/icingadb-web#779) is supposed to change (assuming vars.empty to be an empty array/dict in the examples):

1. Make sure that "Host/Service var empty" shows up in the search suggestions (just like it already would if vars.empty was set to a string on another object).
2. host.vars.empty!~*/service.vars.empty!~* not filters for empty arrays/dicts (that filter never matches previously).
3. empty now shows up in the custom variable display as an empty thing (no differentiation between array and dict).

Sounds fine for me, even though treating empty arrays and dicts as the same feels somewhat artificially limited.

One remaining question regarding search filters: how are host.vars.empty~*x* and host.vars.empty!~*x* handled? There must be a row with flatvalue (NOT) LIKE '%x%' (which implies flatvalue IS NOT NULL) and therefore the new flatvalue = NULL rows are ignored and the result remains unaffected by this change?

[nilmerg]

Sounds fine for me, even though treating empty arrays and dicts as the same feels somewhat artificially limited.

It is. But that's a topic for #610.

One remaining question regarding search filters: how are host.vars.empty~x and host.vars.empty!~x handled? There must be a row with flatvalue (NOT) LIKE '%x%' (which implies flatvalue IS NOT NULL) and therefore the new flatvalue = NULL rows are ignored and the result remains unaffected by this change?

Correct.

[julianbrost]

Sounds fine for me, even though treating empty arrays and dicts as the same feels somewhat artificially limited.

It is. But that's a topic for #610.

Is it? If that was resolved, it would allow you to tell whether the empty thing is at vars["foo"]["bar"] or vars["foo.bar"] but not whether that empty thing is actually an array or dict.

[nilmerg]

I'd extend the expected outcome of course 😉 This is strictly about empty variables.

[julianbrost]

So I think the last few comments summarized in one sentence are: remove the [] and . suffixes here and adapt the icingadb-web PR accordingly (or is there any change needed at all without these new suffixes) and we'll see if that works properly/as expected.

[julianbrost]

The PR description itself shows outdated behavior, so please update that as well.

Overall, the PR on the Go side now does what I expect it to do given the recent comments. I'd wait though and only merge this after Icinga/icingadb-web#779 is fully reviewed (I don't find the current behavior of search filters very intuitive, some of that could probably be improved by better labels but not sure what exactly is supposed to be the scope of these two PRs in the end).

[julianbrost]

The Go part seems to work fine, but even with a freshly pulled icinga/icingaweb2:master image (which should include Icinga/icingadb-web#779), I still receive two visually identical suggestion in the search bar for lists that are empty on some hosts and non-empty on others. One renders in the URL as array, the other as array[*] (± URL enconding). Is this still expected/intended @nilmerg?

[nilmerg]

So empty arrays and dicts will become indistinguishable in the database?

Yes. The only thing which gets lost then is still the (Array) suffix. The indication of 0 Items is still there in both cases.

Can you briefly explain what filters could be performed and how these interact with empty arrays (or also empty dicts for that matter if you can't distinguish them).

• dict!~* (flatname="empty_dict" AND flatvalue IS NULL)
• dict.key=value (flatname="dict.key" AND flatvalue="value")
• list!~* (flatname="empty_list" AND flatvalue IS NULL)
• list[*]~3 (flatname LIKE "list[%]" AND flatvalue = 3)

Of course, while just dict cannot occur as suggestion in the searchbar with a non-empty value, the latter two cases are both shown as Service list for example. To solve this, the list[*] filter could be labelled as Service list (Array) or Service list (contains).

Yes.

[julianbrost]

To solve this, the list[*] filter could be labelled as Service list (Array) or Service list (contains).

I'd say should instead of could. At least to me, the current behavior looks more like a bug than intentional.

Contents of the review were adressed: there are now no more ./[] suffixes in flatname and flatvalue may be NULL.