From 712b530103ca556f7b1eceffb94bc4ccb0963b94 Mon Sep 17 00:00:00 2001 From: Conner Crosby Date: Thu, 19 Sep 2024 12:13:57 -0400 Subject: [PATCH] Fix missing MatchErrors due to hash collisions (#4307) --- .../vars/rule_var_naming_fails_files/bar.yml | 3 + .../vars/rule_var_naming_fails_files/foo.yml | 3 + src/ansiblelint/rules/var_naming.py | 55 +++++++++++++++---- src/ansiblelint/utils.py | 13 ++--- tox.ini | 2 +- 5 files changed, 56 insertions(+), 20 deletions(-) create mode 100644 examples/playbooks/vars/rule_var_naming_fails_files/bar.yml create mode 100644 examples/playbooks/vars/rule_var_naming_fails_files/foo.yml diff --git a/examples/playbooks/vars/rule_var_naming_fails_files/bar.yml b/examples/playbooks/vars/rule_var_naming_fails_files/bar.yml new file mode 100644 index 0000000000..8ed7b20343 --- /dev/null +++ b/examples/playbooks/vars/rule_var_naming_fails_files/bar.yml @@ -0,0 +1,3 @@ +--- +CamelCaseIsBad: bar +ALL_CAPS_ARE_BAD_TOO: bar diff --git a/examples/playbooks/vars/rule_var_naming_fails_files/foo.yml b/examples/playbooks/vars/rule_var_naming_fails_files/foo.yml new file mode 100644 index 0000000000..cf0cb2cb47 --- /dev/null +++ b/examples/playbooks/vars/rule_var_naming_fails_files/foo.yml @@ -0,0 +1,3 @@ +--- +CamelCaseIsBad: foo +ALL_CAPS_ARE_BAD_TOO: foo diff --git a/src/ansiblelint/rules/var_naming.py b/src/ansiblelint/rules/var_naming.py index d28883a340..8ec2ac859d 100644 --- a/src/ansiblelint/rules/var_naming.py +++ b/src/ansiblelint/rules/var_naming.py @@ -117,6 +117,7 @@ def get_var_naming_matcherror( ident: str, *, prefix: Prefix | None = None, + file: Lintable, ) -> MatchError | None: """Return a MatchError if the variable name is not valid, otherwise None.""" if not isinstance(ident, str): # pragma: no cover @@ -124,6 +125,7 @@ def get_var_naming_matcherror( tag="var-naming[non-string]", message="Variables names must be strings.", rule=self, + lintable=file, ) if ident in ANNOTATION_KEYS or ident in self.allowed_special_names: @@ -136,6 +138,7 @@ def get_var_naming_matcherror( tag="var-naming[non-ascii]", message=f"Variables names must be ASCII. ({ident})", rule=self, + lintable=file, ) if keyword.iskeyword(ident): @@ -143,6 +146,7 @@ def get_var_naming_matcherror( tag="var-naming[no-keyword]", message=f"Variables names must not be Python keywords. ({ident})", rule=self, + lintable=file, ) if ident in self.reserved_names: @@ -150,6 +154,7 @@ def get_var_naming_matcherror( tag="var-naming[no-reserved]", message=f"Variables names must not be Ansible reserved names. ({ident})", rule=self, + lintable=file, ) if ident in self.read_only_names: @@ -157,6 +162,7 @@ def get_var_naming_matcherror( tag="var-naming[read-only]", message=f"This special variable is read-only. ({ident})", rule=self, + lintable=file, ) # We want to allow use of jinja2 templating for variable names @@ -165,6 +171,7 @@ def get_var_naming_matcherror( tag="var-naming[no-jinja]", message="Variables names must not contain jinja2 templating.", rule=self, + lintable=file, ) if not bool(self.re_pattern.match(ident)) and ( @@ -174,6 +181,7 @@ def get_var_naming_matcherror( tag="var-naming[pattern]", message=f"Variables names should match {self.re_pattern_str} regex. ({ident})", rule=self, + lintable=file, ) if ( @@ -186,6 +194,7 @@ def get_var_naming_matcherror( tag="var-naming[no-role-prefix]", message=f"Variables names from within roles should use {prefix.value}_ as a prefix.", rule=self, + lintable=file, ) return None @@ -199,9 +208,8 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: # If the Play uses the 'vars' section to set variables our_vars = data.get("vars", {}) for key in our_vars: - match_error = self.get_var_naming_matcherror(key) + match_error = self.get_var_naming_matcherror(key, file=file) if match_error: - match_error.filename = str(file.path) match_error.lineno = ( key.ansible_pos[1] if isinstance(key, AnsibleUnicode) @@ -219,9 +227,9 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: match_error = self.get_var_naming_matcherror( key, prefix=prefix, + file=file, ) if match_error: - match_error.filename = str(file.path) match_error.message += f" (vars: {key})" match_error.lineno = ( key.ansible_pos[1] @@ -235,9 +243,9 @@ def matchplay(self, file: Lintable, data: dict[str, Any]) -> list[MatchError]: match_error = self.get_var_naming_matcherror( key, prefix=prefix, + file=file, ) if match_error: - match_error.filename = str(file.path) match_error.message += f" (vars: {key})" match_error.lineno = ( key.ansible_pos[1] @@ -266,7 +274,6 @@ def matchtask( """Return matches for task based variables.""" results = [] prefix = Prefix() - filename = "" if file is None else str(file.path) if file and file.parent and file.parent.kind == "role": prefix = Prefix(file.parent.path.name) ansible_module = task["action"]["__ansible_module__"] @@ -283,9 +290,9 @@ def matchtask( match_error = self.get_var_naming_matcherror( key, prefix=prefix, + file=file or Lintable(""), ) if match_error: - match_error.filename = filename match_error.lineno = our_vars[LINE_NUMBER_KEY] match_error.message += f" (vars: {key})" results.append(match_error) @@ -298,9 +305,12 @@ def matchtask( and x != "cacheable", task["action"].keys(), ): - match_error = self.get_var_naming_matcherror(key, prefix=prefix) + match_error = self.get_var_naming_matcherror( + key, + prefix=prefix, + file=file or Lintable(""), + ) if match_error: - match_error.filename = filename match_error.lineno = task["action"][LINE_NUMBER_KEY] match_error.message += f" (set_fact: {key})" results.append(match_error) @@ -311,10 +321,10 @@ def matchtask( match_error = self.get_var_naming_matcherror( registered_var, prefix=prefix, + file=file or Lintable(""), ) if match_error: match_error.message += f" (register: {registered_var})" - match_error.filename = filename match_error.lineno = task[LINE_NUMBER_KEY] results.append(match_error) @@ -325,15 +335,17 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: results: list[MatchError] = [] raw_results: list[MatchError] = [] meta_data: dict[AnsibleUnicode, Any] = {} - filename = "" if file is None else str(file.path) if str(file.kind) == "vars" and file.data: meta_data = parse_yaml_from_file(str(file.path)) for key in meta_data: prefix = Prefix(file.role) if file.role else Prefix() - match_error = self.get_var_naming_matcherror(key, prefix=prefix) + match_error = self.get_var_naming_matcherror( + key, + prefix=prefix, + file=file, + ) if match_error: - match_error.filename = filename match_error.lineno = key.ansible_pos[1] match_error.message += f" (vars: {key})" raw_results.append(match_error) @@ -413,6 +425,25 @@ def test_invalid_var_name_varsfile( assert result.tag == expected_errors[idx][0] assert result.lineno == expected_errors[idx][1] + def test_invalid_vars_diff_files( + default_rules_collection: RulesCollection, + ) -> None: + """Test rule matches.""" + results = Runner( + Lintable("examples/playbooks/vars/rule_var_naming_fails_files"), + rules=default_rules_collection, + ).run() + expected_errors = ( + ("var-naming[pattern]", 2), + ("var-naming[pattern]", 3), + ("var-naming[pattern]", 2), + ("var-naming[pattern]", 3), + ) + assert len(results) == len(expected_errors) + for idx, result in enumerate(results): + assert result.tag == expected_errors[idx][0] + assert result.lineno == expected_errors[idx][1] + def test_var_naming_with_role_prefix( default_rules_collection: RulesCollection, ) -> None: diff --git a/src/ansiblelint/utils.py b/src/ansiblelint/utils.py index 289dc56099..3f56f8d9fe 100644 --- a/src/ansiblelint/utils.py +++ b/src/ansiblelint/utils.py @@ -358,6 +358,7 @@ def taskshandlers_children( raise MatchError( message="A malformed block was encountered while loading a block.", rule=RuntimeErrorRule(), + lintable=lintable, ) for task_handler in v: # ignore empty tasks, `-` @@ -408,23 +409,21 @@ def _validate_task_handler_action_for_role(self, th_action: dict[str, Any]) -> N """Verify that the task handler action is valid for role include.""" module = th_action["__ansible_module__"] + lintable = Lintable( + self.rules.options.lintables[0] if self.rules.options.lintables else ".", + ) if "name" not in th_action: raise MatchError( message=f"Failed to find required 'name' key in {module!s}", rule=self.rules.rules[0], - lintable=Lintable( - ( - self.rules.options.lintables[0] - if self.rules.options.lintables - else "." - ), - ), + lintable=lintable, ) if not isinstance(th_action["name"], str): raise MatchError( message=f"Value assigned to 'name' key on '{module!s}' is not a string.", rule=self.rules.rules[1], + lintable=lintable, ) def roles_children( diff --git a/tox.ini b/tox.ini index fa2dc62718..e88b390046 100644 --- a/tox.ini +++ b/tox.ini @@ -74,7 +74,7 @@ setenv = PRE_COMMIT_COLOR = always # Number of expected test passes, safety measure for accidental skip of # tests. Update value if you add/remove tests. (tox-extra) - PYTEST_REQPASS = 893 + PYTEST_REQPASS = 894 FORCE_COLOR = 1 pre: PIP_PRE = 1 allowlist_externals =