Remove `Optional[]` types from `models.Host` fields & add tests

[pederhan]

Changes

Most fields in the Host class have had their Optional type removed, and a new "*" validator has been added to ensure these fields can still be instantiated with None (ensuring backwards compatibility). The validator guarantees that these fields are not None once the object itself has been instantiated, as it uses the field's default value or default factory to assign values to them.

Affected fields in Host:

• interfaces
• inventory
• properties
• proxy_pattern
• siteadmins
• sources
• tags

Furthermore, tests for Host.merge() as well as Host.importance* have been added. Since it's a bit unclear how Host.proxy_pattern and Host.macros should be handled when merging, they are not tested.

* This probably could have been a separate pull request, but it's a very minor change. See: c1cea92cbed8c3403e8394d5f6b6b7af502cc115

Rationale

Optional[<type>] fields make Host.merge() dangerous to run without adding None checks for each field.

Required to make Host.merge() safe without removing Optional currently:

if self.inventory is not None and other.inventory is not None: # this must be added
    for k, v in other.inventory.items():
        if k in self.inventory and v != self.inventory[k]:
            logging.warning("Same inventory ('%s') set multiple times for host: '%s'", k, self.hostname)
        else:
            self.inventory[k] = v

This None check does not currently exist (unsafe) and is one the primary motivations for making this pull request. None checks would have to be added to every single attribute access on both self and other for all fields that are Optional.

With this pull request:

for k, v in other.inventory.items(): # other.inventory and self.inventory guaranteed to be a dict
    if k in self.inventory and v != self.inventory[k]:
        logging.warning("Same inventory ('%s') set multiple times for host: '%s'", k, self.hostname)
    else:
        self.inventory[k] = v

Removing the possibility for these fields to be None during runtime makes the code easier to reason with, as well as removing potential pitfalls related to Optional types.

In order to not break existing tests and functionality, a new "*" validator has been added which returns the field's default value or runs its default factory if it receives a None value.

[mbakke]

Uff, I thought the "Rebase and merge" button would simply add the commits instead of creating a useless merge commit, but GitHub managed to rebase even though the branch was already up to date.

So now all the hashes are different, sorry about that.

[paalbra]

Nice catch and a good PR :+1: One of the problems, for future reference:

>>> from zabbix_auto_config.models import Host
>>> h1 = Host(**{"hostname": "foo", "enabled": True})
>>> h1.merge(Host(**{"hostname": "bar", "enabled": True, "inventory": None}))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "models.py", line 136, in merge
    for k, v in other.inventory.items():
AttributeError: 'NoneType' object has no attribute 'items'

Personally I wouldn't bother with the backwards compatibility though. It's not intended behavior/usage, and it's buggy, like stated in this PR. The project is in version major zero and it would be cleaner without the compatibility, which isn't strictly necessary? And I'm not sure how if it could slow down object creation (probably very negligible)? Anyhow: It doesn't hurt much to keep it compatible either.