ansible-test validate-modules: report bad-return-value-key for return values that cannot be accessed with Jinja's dot notation (#86079)

* Report bad-return-value-key for return values that cannot be accessed with Jinja's dot notation.

* Move constants into separate module.

* Add test to check FORBIDDEN_DICTIONARY_KEYS against current Python's key list.

* Remove unused constant.

* Apply suggestions from code review.

Co-authored-by: Matt Clay <matt@mystile.com>

* Add type annotations.

* Simplify typing.

Co-authored-by: Matt Clay <matt@mystile.com>

---------

Co-authored-by: Matt Clay <matt@mystile.com>

changelogs/fragments/86079-ansible-test-validate-modules-bad-return-keys.yml

@@ -0,0 +1,2 @@
+minor_changes:
+  - "ansible-test validate-modules sanity test - now reports bad return value keys that cannot be used with the dot notation in Jinja expressions (https://github.com/ansible/ansible/issues/86079)."

test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/constants.py

@@ -0,0 +1,67 @@
+# -*- coding: utf-8 -*-
+#
+# Copyright (C) 2015 Matt Martz <matt@sivel.net>
+# Copyright (C) 2015 Rackspace US, Inc.
+#
+#    This program is free software: you can redistribute it and/or modify
+#    it under the terms of the GNU General Public License as published by
+#    the Free Software Foundation, either version 3 of the License, or
+#    (at your option) any later version.
+#
+#    This program is distributed in the hope that it will be useful,
+#    but WITHOUT ANY WARRANTY; without even the implied warranty of
+#    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+#    GNU General Public License for more details.
+#
+#    You should have received a copy of the GNU General Public License
+#    along with this program.  If not, see <http://www.gnu.org/licenses/>.
+from __future__ import annotations
+
+import re
+
+
+REJECTLIST_DIRS = frozenset(('.git', 'test', '.github', '.idea'))
+SYS_EXIT_REGEX = re.compile(r'[^#]*sys.exit\s*\(.*')
+NO_LOG_REGEX = re.compile(r'(?:pass(?!ive)|secret|token|key)', re.I)
+
+# Everything that should not be used in a dictionary of a return value,
+# since it will make user's life harder.
+FORBIDDEN_DICTIONARY_KEYS = frozenset([
+    'clear',
+    'copy',
+    'fromkeys',
+    'get',
+    'items',
+    'keys',
+    'pop',
+    'popitem',
+    'setdefault',
+    'update',
+    'values',
+])
+
+
+REJECTLIST_IMPORTS = {
+    'requests': {
+        'new_only': True,
+        'error': {
+            'code': 'use-module-utils-urls',
+            'msg': ('requests import found, should use '
+                    'ansible.module_utils.urls instead')
+        }
+    },
+    r'boto(?:\.|$)': {
+        'new_only': True,
+        'error': {
+            'code': 'use-boto3',
+            'msg': 'boto import found, new modules should use boto3'
+        }
+    },
+}
+SUBPROCESS_REGEX = re.compile(r'subprocess\.Po.*')
+OS_CALL_REGEX = re.compile(r'os\.call.*')
+
+
+PLUGINS_WITH_RETURN_VALUES = ('module', )
+PLUGINS_WITH_EXAMPLES = ('module', )
+PLUGINS_WITH_YAML_EXAMPLES = ('module', )

test/lib/ansible_test/_util/controller/sanity/validate-modules/validate_modules/main.py

@@ -85,6 +85,19 @@ from .schema import (
 
 from .utils import CaptureStd, NoArgsAnsibleModule, compare_unordered_lists, parse_yaml, parse_isodate
 
+from .constants import (
+    REJECTLIST_DIRS,
+    SYS_EXIT_REGEX,
+    NO_LOG_REGEX,
+    FORBIDDEN_DICTIONARY_KEYS,
+    REJECTLIST_IMPORTS,
+    SUBPROCESS_REGEX,
+    OS_CALL_REGEX,
+    PLUGINS_WITH_RETURN_VALUES,
+    PLUGINS_WITH_EXAMPLES,
+    PLUGINS_WITH_YAML_EXAMPLES,
+)
+
 
 # Because there is no ast.TryExcept in Python 3 ast module
 TRY_EXCEPT = ast.Try
@@ -92,41 +105,9 @@ TRY_EXCEPT = ast.Try
 # string but we need unicode for Python 3
 REPLACER_WINDOWS = _REPLACER_WINDOWS.decode('utf-8')
 
-REJECTLIST_DIRS = frozenset(('.git', 'test', '.github', '.idea'))
-INDENT_REGEX = re.compile(r'([\t]*)')
-SYS_EXIT_REGEX = re.compile(r'[^#]*sys.exit\s*\(.*')
-NO_LOG_REGEX = re.compile(r'(?:pass(?!ive)|secret|token|key)', re.I)
-
-
-REJECTLIST_IMPORTS = {
-    'requests': {
-        'new_only': True,
-        'error': {
-            'code': 'use-module-utils-urls',
-            'msg': ('requests import found, should use '
-                    'ansible.module_utils.urls instead')
-        }
-    },
-    r'boto(?:\.|$)': {
-        'new_only': True,
-        'error': {
-            'code': 'use-boto3',
-            'msg': 'boto import found, new modules should use boto3'
-        }
-    },
-}
-SUBPROCESS_REGEX = re.compile(r'subprocess\.Po.*')
-OS_CALL_REGEX = re.compile(r'os\.call.*')
-
-
 LOOSE_ANSIBLE_VERSION = LooseVersion('.'.join(ansible_version.split('.')[:3]))
 
 
-PLUGINS_WITH_RETURN_VALUES = ('module', )
-PLUGINS_WITH_EXAMPLES = ('module', )
-PLUGINS_WITH_YAML_EXAMPLES = ('module', )
-
-
 def is_potential_secret_option(option_name):
     if not NO_LOG_REGEX.search(option_name):
         return False
@@ -905,6 +886,27 @@ class ModuleValidator(Validator):
                 msg=msg,
             )
 
+    def _validate_return_docs(self, returns: object, context: list[str] | None = None) -> None:
+        if not isinstance(returns, dict):
+            return
+        if context is None:
+            context = []
+
+        for rv, data in returns.items():
+            if isinstance(data, dict) and "contains" in data:
+                self._validate_return_docs(data["contains"], context + [rv])
+
+            if str(rv) in FORBIDDEN_DICTIONARY_KEYS or not str(rv).isidentifier():
+                msg = f"Return value key {rv!r}"
+                if context:
+                    msg += " found in %s" % " -> ".join(context)
+                msg += " should not be used for return values since it cannot be accessed with dot notation in Jinja"
+                self.reporter.error(
+                    path=self.object_path,
+                    code='bad-return-value-key',
+                    msg=msg,
+                )
+
     def _validate_docs(self):
         doc = None
         # We have three ways of marking deprecated/removed files.  Have to check each one
@@ -1145,6 +1147,7 @@ class ModuleValidator(Validator):
                 returns,
                 return_schema(for_collection=bool(self.collection), plugin_type=self.plugin_type),
                 'RETURN', 'return-syntax-error')
+            self._validate_return_docs(returns)
 
         elif self.plugin_type in PLUGINS_WITH_RETURN_VALUES:
             if self._is_new_module():

test/sanity/ignore.txt

@@ -18,6 +18,7 @@ lib/ansible/modules/copy.py validate-modules:undocumented-parameter
 lib/ansible/modules/dnf.py validate-modules:parameter-invalid
 lib/ansible/modules/dnf5.py validate-modules:parameter-invalid
 lib/ansible/modules/file.py validate-modules:undocumented-parameter
+lib/ansible/modules/getent.py validate-modules:bad-return-value-key  # used for documentation, not a real key
 lib/ansible/modules/git.py use-argspec-type-path
 lib/ansible/modules/git.py validate-modules:doc-required-mismatch
 lib/ansible/modules/package_facts.py validate-modules:doc-choices-do-not-match-spec

test/units/ansible_test/_util/controller/sanity/validate-modules/validate_modules/conftest.py

@@ -0,0 +1,17 @@
+from __future__ import annotations
+
+
+import pytest
+import sys
+
+from pathlib import Path
+
+
+@pytest.fixture(autouse=True, scope='session')
+def inject_ansible_test_validate_modules() -> None:
+    """Make ansible_test's validate-modules available on `sys.path` for unit testing ansible-test."""
+    test_lib = (
+        Path(__file__).parent / ".." / ".." / ".." / ".." / ".." / ".." / ".."
+        / "lib" / "ansible_test" / "_util" / "controller" / "sanity" / "validate-modules"
+    )
+    sys.path.insert(0, str(test_lib))

test/units/ansible_test/_util/controller/sanity/validate-modules/validate_modules/test_main.py

@@ -0,0 +1,10 @@
+"""Tests for validate-module's main module."""
+from __future__ import annotations
+
+
+def test_dict_members() -> None:
+    from validate_modules.constants import FORBIDDEN_DICTIONARY_KEYS  # type: ignore[import-not-found]
+
+    expected_keys = [key for key in dict.__dict__ if not key.startswith("__")]
+
+    assert FORBIDDEN_DICTIONARY_KEYS == frozenset(expected_keys)