From f67e3b325aaa6711a5fd8e49deec4c9365e0dd06 Mon Sep 17 00:00:00 2001 From: Sorin Sbarnea Date: Thu, 31 Aug 2023 12:01:42 +0100 Subject: [PATCH] Make schema validation violation messages more explicit Fixes: #3695 --- examples/rulebooks/rulebook-fail.yml | 6 +-- src/ansiblelint/rules/schema.py | 34 +++++++++-------- src/ansiblelint/schemas/main.py | 57 ++++++++++++++++++++++++++-- 3 files changed, 74 insertions(+), 23 deletions(-) diff --git a/examples/rulebooks/rulebook-fail.yml b/examples/rulebooks/rulebook-fail.yml index 11472b4770..b08cd03841 100644 --- a/examples/rulebooks/rulebook-fail.yml +++ b/examples/rulebooks/rulebook-fail.yml @@ -1,8 +1,8 @@ --- - name: Sample rulebooks hosts: all - that_should_not_be_here: foo - sources: # should be "sources" + that_should_not_be_here: foo # <-- this is not supported + sources: # <-- should be "sources" - name: listen for alerts ansible.eda.alertmanager: host: 0.0.0.0 @@ -18,4 +18,4 @@ - name: debug condition: event.alert.labels.job == "fastapi" action: - debug: sss + debug: sss # <-- this should be an object diff --git a/src/ansiblelint/rules/schema.py b/src/ansiblelint/rules/schema.py index 7ca8ce669a..08e9545f33 100644 --- a/src/ansiblelint/rules/schema.py +++ b/src/ansiblelint/rules/schema.py @@ -155,9 +155,8 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: if file.kind not in JSON_SCHEMAS: return result - errors = validate_file_schema(file) - if errors: - if errors[0].startswith("Failed to load YAML file"): + for error in validate_file_schema(file): + if error.startswith("Failed to load YAML file"): _logger.debug( "Ignored failure to load %s for schema validation, as !vault may cause it.", file, @@ -166,13 +165,14 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: result.append( MatchError( - message=errors[0], + message=error, lintable=file, rule=ValidateSchemaRule(), details=ValidateSchemaRule.description, tag=f"schema[{file.kind}]", ), ) + break if not result: result = super().matchyaml(file) @@ -194,27 +194,28 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: pytest.param( "examples/.collection/galaxy.yml", "galaxy", - [r"^'GPL' is not one of.*https://"], + [r".*'GPL' is not one of.*https://"], id="galaxy", ), pytest.param( "examples/roles/invalid_requirements_schema/meta/requirements.yml", "requirements", [ - r"^{'foo': 'bar'} is not valid under any of the given schemas.*https://", + # r".*{'foo': 'bar'} is not valid under any of the given schemas.*https://", + r".*{'foo': 'bar'} is not of type 'array'.*https://", ], id="requirements", ), pytest.param( "examples/roles/invalid_meta_schema/meta/main.yml", "meta", - [r"^False is not of type 'string'.*https://"], + [r".*False is not of type 'string'.*https://"], id="meta", ), pytest.param( "examples/playbooks/vars/invalid_vars_schema.yml", "vars", - [r"^'123' does not match any of the regexes.*https://"], + [r".* '123' does not match any of the regexes.*https://"], id="vars", ), pytest.param( @@ -227,7 +228,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: "examples/ee_broken/execution-environment.yml", "execution-environment", [ - r"^{'foo': 'bar'} is not valid under any of the given schemas.*https://", + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", ], id="execution-environment-broken", ), @@ -236,7 +237,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: "examples/broken_collection_meta_runtime/meta/runtime.yml", "meta-runtime", [ - r"^Additional properties are not allowed \('foo' was unexpected\).*https://", + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", ], id="meta-runtime-broken", ), @@ -250,7 +251,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: "examples/inventory/broken_dev_inventory.yml", "inventory", [ - r"^Additional properties are not allowed \('foo' was unexpected\).*https://", + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", ], id="inventory-broken", ), @@ -270,7 +271,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: "examples/broken/.ansible-lint", "ansible-lint-config", [ - r"^Additional properties are not allowed \('foo' was unexpected\).*https://", + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", ], id="ansible-lint-config-broken", ), @@ -284,7 +285,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: "examples/broken/ansible-navigator.yml", "ansible-navigator-config", [ - r"^Additional properties are not allowed \('ansible' was unexpected\).*https://", + r".*Additional properties are not allowed \('ansible' was unexpected\).*https://", ], id="ansible-navigator-config-broken", ), @@ -298,7 +299,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: "examples/roles/broken_argument_specs/meta/argument_specs.yml", "role-arg-spec", [ - r"^Additional properties are not allowed \('foo' was unexpected\).*https://", + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", ], id="role-arg-spec-broken", ), @@ -306,7 +307,7 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: "examples/changelogs/changelog.yaml", "changelog", [ - r"^Additional properties are not allowed \('foo' was unexpected\).*https://", + r".*Additional properties are not allowed \('foo' was unexpected\).*https://", ], id="changelog", ), @@ -314,7 +315,8 @@ def matchyaml(self, file: Lintable) -> list[MatchError]: "examples/rulebooks/rulebook-fail.yml", "rulebook", [ - r"^Additional properties are not allowed \('that_should_not_be_here' was unexpected\).*https://", + # r".*Additional properties are not allowed \('that_should_not_be_here' was unexpected\).*https://", + r".*'sss' is not of type 'object'.*https://", ], id="rulebook", ), diff --git a/src/ansiblelint/schemas/main.py b/src/ansiblelint/schemas/main.py index add87f9799..ab146cbb87 100644 --- a/src/ansiblelint/schemas/main.py +++ b/src/ansiblelint/schemas/main.py @@ -4,6 +4,7 @@ import json import logging import re +import typing from typing import TYPE_CHECKING import jsonschema @@ -19,6 +20,22 @@ from ansiblelint.file_utils import Lintable +def find_best_deep_match( + errors: jsonschema.ValidationError, +) -> jsonschema.ValidationError: + """Return the deepest schema validation error.""" + + def iter_validation_error( + err: jsonschema.ValidationError, + ) -> typing.Iterator[jsonschema.ValidationError]: + if err.context: + for e in err.context: + yield e + yield from iter_validation_error(e) + + return max(iter_validation_error(errors), key=_deep_match_relevance) + + def validate_file_schema(file: Lintable) -> list[str]: """Return list of JSON validation errors found.""" schema = {} @@ -29,10 +46,33 @@ def validate_file_schema(file: Lintable) -> list[str]: yaml_data = yaml_load_safe(file.content) json_data = json.loads(json.dumps(yaml_data)) schema = _schema_cache[file.kind] - jsonschema.validate( - instance=json_data, - schema=schema, - ) + + validator = jsonschema.validators.validator_for(schema) + v = validator(schema) + try: + error = next(v.iter_errors(json_data)) + except StopIteration: + return [] + if error.context: + error = find_best_deep_match(error) + message = f"{error.json_path} {error.message}" + + documentation_url = "" + for k in ("description", "markdownDescription"): + if k in schema: + # Find standalone URLs and also markdown urls. + match = re.search( + r"\[.*?\]\((https?://[^\s]+)\)|https?://[^\s]+", + schema[k], + ) + if match: + documentation_url = match[0] if match[0] else match[1] + break + if documentation_url: + if not message.endswith("."): + message += "." + message += f" See {documentation_url}" + return [message] except yaml.constructor.ConstructorError as exc: return [f"Failed to load YAML file '{file.path}': {exc.problem}"] except ValidationError as exc: @@ -54,3 +94,12 @@ def validate_file_schema(file: Lintable) -> list[str]: message += f" See {documentation_url}" return [message] return [] + + +def _deep_match_relevance(error: jsonschema.ValidationError) -> tuple[bool | int, ...]: + validator = error.validator + return ( + validator not in ("anyOf", "oneOf"), # type: ignore[comparison-overlap] + len(error.absolute_path), + -len(error.path), + )