`helper.exists()` does not return a boolean and does not work with empty values

[kumy]

Important notices

Before you add a new report, we ask you kindly to acknowledge the following:

• [x] I have read the contributing guide lines at https://github.com/opnsense/core/blob/master/CONTRIBUTING.md
• [x] I am convinced that my issue is new after having checked both open and closed issues at https://github.com/opnsense/core/issues?q=is%3Aissue

Describe the bug

While working a new template for a plugin I'm working on, I went to the helpers.exists() function. Given the function name and the docstring, we should expect this function to return a boolean. But this returns a string, which is technically incorrect. This cause issue with existing node path with empty values.

https://github.com/opnsense/core/blob/66e62f4a899c5847faf47427f6670077103c1de7/src/opnsense/service/modules/addons/template_helpers.py#L76-L82

To Reproduce (rough untested steps - sorry)

Steps to reproduce the behavior:

1. In a template use {{ helpers.exists('path.to.some.existing.node') }} and {{ helpers.exists('path.to.some.non.existing.node') }}
2. In config.xml set the path path.to.some.existing.node to "empty string".

Expected behavior

• {{ helpers.exists('path.to.some.existing.node) }} should return boolean True. It actually returns the string None.
• {{ helpers.exists('path.to.some.non.existing.node) }} should return boolean False. It actually returns the string None.

Describe alternatives you considered The working alternative code is helpers.exists(conf ~ '.' ~ path) != 'None'

Some tests.

# existing path
00={{ conf ~ '.' ~ path }}
01={{ helpers.exists(conf ~ '.' ~ path) }}
02={{ helpers.exists(conf ~ '.' ~ path) is not none }}
03={{ helpers.exists(conf ~ '.' ~ path) != 'None' }}
04={{ helpers.getNodeByTag(conf ~ '.' ~ path) }}
05={{ helpers.getNodeByTag(conf ~ '.' ~ path) != "global default" }}
06={{ helpers.exists(conf ~ '.foo.' ~ path) and helpers.getNodeByTag(conf ~ '.foo.' ~ path) != "global default" }}
07={{ helpers.exists(conf ~ '.foo.' ~ path) != 'None' and helpers.getNodeByTag(conf ~ '.foo.' ~ path) != "global default" }}

# non existing path
10={{ conf ~ '.foo.' ~ path }}
11={{ helpers.exists(conf ~ '.foo.' ~ path) }}
12={{ helpers.exists(conf ~ '.foo.' ~ path) is not none }}
13={{ helpers.exists(conf ~ '.foo.' ~ path) != 'None' }}
14={{ helpers.getNodeByTag(conf ~ '.foo.' ~ path) }}
15={{ helpers.getNodeByTag(conf ~ '.foo.' ~ path) != "global default" }}
16={{ helpers.exists(conf ~ '.foo.' ~ path) and helpers.getNodeByTag(conf ~ '.foo.' ~ path) != "global default" }}
17={{ helpers.exists(conf ~ '.foo.' ~ path) != 'None' and helpers.getNodeByTag(conf ~ '.foo.' ~ path) != "global default" }}

With an empty value: (system.backup.restic.repositories.minio.advanced.http_user_agent is empty string in config.xml)

00=system.backup.restic.repositories.minio.advanced.http_user_agent
01=None
02=False
03=True
04=None
05=True
06=None
07=True

10=system.backup.restic.repositories.minio.foo.advanced.http_user_agent
11=None
12=False
13=True
14=None
15=True
16=None
17=True

With a non empty value: (system.backup.restic.general.advanced.http_user_agent is non-empty string in config.xml)

00=system.backup.restic.general.advanced.http_user_agent
01=Kumy test
02=True
03=True
04=Kumy test
05=True
06=None
07=True

10=system.backup.restic.general.foo.advanced.http_user_agent
11=None
12=False
13=True
14=None
15=True
16=None
17=True

And when the value is really the string None (with is not an empty value)

# existing path
00=system.backup.restic.repositories.minio.advanced.http_user_agent
01=None
02=True
03=False
04=None
05=True
06=None
07=True

# non existing path
10=system.backup.restic.repositories.minio.foo.advanced.http_user_agent
11=None
12=False
13=True
14=None
15=True
16=None
17=True

Fixing proposal

This could be a way to fix it:

def exists(self, tag):
    """
    check if node exists in dictionary structure
    :param tag: tag in dot notation (section.item)
    :return: boolean
    """
    return self.getNodeByTag(tag) is not None

With this patch we get:

# existing path/empty value
00=system.backup.restic.repositories.minio.advanced.http_user_agent
01=False
02=True
03=True
04=None
05=True
06=False
07=True

# non existing path
10=system.backup.restic.repositories.minio.foo.advanced.http_user_agent
11=False
12=True
13=True
14=None
15=True
16=False
17=True

# existing path/non-empty value
00=system.backup.restic.general.advanced.http_user_agent
01=True
02=True
03=True
04=Kumy test
05=True
06=False
07=True

# non existing path
10=system.backup.restic.general.foo.advanced.http_user_agent
11=False
12=True
13=True
14=None
15=True
16=False
17=True

And when the value is really the string None (with is not an empty value)

# existing path
00=system.backup.restic.repositories.minio.advanced.http_user_agent
01=True
02=True
03=True
04=None
05=True
06=False
07=True

# non existing path
10=system.backup.restic.repositories.minio.foo.advanced.http_user_agent
11=False
12=True
13=True
14=None
15=True
16=False
17=True

Additional context

Will it break existing uses? :shrug:

Is there any existing unit test for this method?

[AdSchellevis]

@kumy the helper functions aren't covered by unit tests at the moment, but since we normally only use exists() to validate if a container exists, your suggested change should be safe. If you open a PR I'll merge it.

[kumy]

@AdSchellevis please find PR #7918