looking up entities with the name set to an empty string sometimes results in unexpected errors

[evgeni]

SUMMARY

Because of the "empty string is special to unset things" logic introduced in f50bd16, we do not immediately fail when such an input is provided, which leads to errors in the code later on that relies on the fact that the entity was properly found.

we probably should just not apply the "empty string to unset" logic to parameters that are set as required=True (like content_view in the content_view_version module, which the example below uses).

ISSUE TYPE

• Bug Report

ANSIBLE VERSION

all

COLLECTION VERSION

since f50bd16

KATELLO/FOREMAN VERSION

all

STEPS TO REPRODUCE

---
- hosts: localhost
  tasks:
    - name: lol
      theforeman.foreman.content_view_version:
        server_url: https://foreman.example.com
        validate_certs: false
        username: admin
        password: changeme
        organization: Default Organization
        content_view: ""

EXPECTED RESULTS

fatal: [localhost]: FAILED! => {"changed": false, "msg": "Found no results while searching for content_views with name=\"\""}

ACTUAL RESULTS

fatal: [localhost]: FAILED! => {"changed": false, "module_stderr": "/usr/lib/python3.12/site-packages/urllib3/connectionpool.py:1063: InsecureRequestWarning: Unverified HTTPS request is being made to host 'sat-stream-qa-rhel8.tanso.example.com'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/1.26.x/advanced-usage.html#ssl-warnings
  warnings.warn(
/usr/lib/python3.12/site-packages/urllib3/connectionpool.py:1063: InsecureRequestWarning: Unverified HTTPS request is being made to host 'sat-stream-qa-rhel8.tanso.example.com'. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/1.26.x/advanced-usage.html#ssl-warnings
  warnings.warn(
Traceback (most recent call last):
  File \"/home/evgeni/.ansible/tmp/ansible-tmp-1724857997.5646284-1871287-265642613579476/AnsiballZ_content_view_version.py\", line 107, in <module>
    _ansiballz_main()
  File \"/home/evgeni/.ansible/tmp/ansible-tmp-1724857997.5646284-1871287-265642613579476/AnsiballZ_content_view_version.py\", line 99, in _ansiballz_main
    invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
  File \"/home/evgeni/.ansible/tmp/ansible-tmp-1724857997.5646284-1871287-265642613579476/AnsiballZ_content_view_version.py\", line 47, in invoke_module
    runpy.run_module(mod_name='ansible_collections.theforeman.foreman.plugins.modules.content_view_version', init_globals=dict(_module_fqn='ansible_collections.theforeman.foreman.plugins.modules.content_view_version', _modlib_path=modlib_path),
  File \"<frozen runpy>\", line 226, in run_module
  File \"<frozen runpy>\", line 98, in _run_module_code
  File \"<frozen runpy>\", line 88, in _run_code
  File \"/tmp/ansible_theforeman.foreman.content_view_version_payload_2vsp0rrh/ansible_theforeman.foreman.content_view_version_payload.zip/ansible_collections/theforeman/foreman/plugins/modules/content_view_version.py\", line 275, in <module>
  File \"/tmp/ansible_theforeman.foreman.content_view_version_payload_2vsp0rrh/ansible_theforeman.foreman.content_view_version_payload.zip/ansible_collections/theforeman/foreman/plugins/modules/content_view_version.py\", line 238, in main
TypeError: type 'NoEntity' is not subscriptable
", "module_stdout": "", "msg": "MODULE FAILURE
See stdout/stderr for the exact error", "rc": 1}

[mdellweg]

So to understand this correctly, the module is failing under the right circumstances, but for the wrong reason, right?

To pick up your idea: Can we pass the required flag from the foreman_spec to find_resource_by and then change that to start with

def find_resource_by(self, resource, search_field, value, required=False, **kwargs):
    if not value:
        if required:
            raise WhateverRequiredEntityNotSpecifiedError()
        return NoEntity
    ...

[evgeni]

So to understand this correctly, the module is failing under the right circumstances, but for the wrong reason, right?

Yeah. Most importantly, it gives an ugly stack trace instead of erroring with "couldn't find resource X".

Can we pass the required flag from the foreman_spec to find_resource_by and then change that to start with

Yes, that's roughly the direction I was heading.

[evgeni]

Actually, I think this should be sufficient:

def find_resource_by(self, resource, search_field, value, required=False, **kwargs):
    if not value and not required:
        return NoEntity
…

As the rest of the code will already correctly error when nothing was found.

[mdellweg]

Right. My line of thought was "Nothing found." is a different complaint than "You didn't specify a required thing.".

[evgeni]

Yeah… but they did specify it ("" is a specification), so imho the error "nothing found with name ''" is the correct one here.

[mdellweg]

Fair. Can we spare a lookup request when we already know that "" will not find us anything?

[evgeni]

Maybe? ;-)