From f0c317a39d27066c6aafc003c673ab0a78bd8b73 Mon Sep 17 00:00:00 2001 From: Jostein Solaas Date: Fri, 7 Jun 2024 11:16:55 +0200 Subject: [PATCH] fix: implement YamlModel specific errors YamlError gives additional info about the error in yaml, in addition to ensuring a consistent interface across different yaml readers. --- .../presentation/yaml/file_context.py | 52 ++++++++++++++ .../presentation/yaml/validation_errors.py | 57 +++------------- .../yaml/yaml_models/exceptions.py | 20 ++++++ .../yaml/yaml_models/pyyaml_yaml_model.py | 67 ++++++++++++++----- .../yaml/yaml_models/ruamel_yaml_model.py | 39 ++++++++--- src/tests/ecalc_cli/test_app.py | 3 +- src/tests/libecalc/input/test_file_io.py | 4 +- .../yaml_models/test_pyyaml_yaml_model.py | 8 +-- .../yaml_models/test_yaml_model_errors.py | 39 +++++++++++ 9 files changed, 206 insertions(+), 83 deletions(-) create mode 100644 src/libecalc/presentation/yaml/file_context.py create mode 100644 src/libecalc/presentation/yaml/yaml_models/exceptions.py create mode 100644 src/tests/libecalc/presentation/yaml/yaml_models/test_yaml_model_errors.py diff --git a/src/libecalc/presentation/yaml/file_context.py b/src/libecalc/presentation/yaml/file_context.py new file mode 100644 index 0000000000..51b775cfa1 --- /dev/null +++ b/src/libecalc/presentation/yaml/file_context.py @@ -0,0 +1,52 @@ +from dataclasses import dataclass +from typing import Optional + +from typing_extensions import Self + +from libecalc.presentation.yaml.yaml_entities import YamlDict + + +@dataclass +class FileMark: + line_number: int + column_number: int + + def __str__(self) -> str: + return f" line {self.line_number}, column {self.column_number}" + + +@dataclass +class FileContext: + start: FileMark + end: Optional[FileMark] = None + name: Optional[str] = None + + def __str__(self) -> str: + file_context_string = "" + if self.name is not None: + file_context_string += f" in '{self.name}'," + + file_context_string += str(self.start) + + return file_context_string + + @classmethod + def from_yaml_dict(cls, data: YamlDict) -> Self: + if not hasattr(data, "end_mark") or not hasattr(data, "start_mark"): + return None + + # This only works with our implementation of pyyaml read + # In the future, we can move this logic into PyYamlYamlModel with a better interface in YamlValidator. + # Specifically with our own definition of the returned data in each property in YamlValidator. + start_mark = data.start_mark + end_mark = data.end_mark + return FileContext( + start=FileMark( + line_number=start_mark.line + 1, + column_number=start_mark.column, + ), + end=FileMark( + line_number=end_mark.line + 1, + column_number=end_mark.column, + ), + ) diff --git a/src/libecalc/presentation/yaml/validation_errors.py b/src/libecalc/presentation/yaml/validation_errors.py index e8f48212ee..fe6957b71d 100644 --- a/src/libecalc/presentation/yaml/validation_errors.py +++ b/src/libecalc/presentation/yaml/validation_errors.py @@ -12,6 +12,7 @@ from yaml import Dumper, Mark from libecalc.common.logger import logger +from libecalc.presentation.yaml.file_context import FileContext from libecalc.presentation.yaml.yaml_entities import Resource, YamlDict, YamlList from libecalc.presentation.yaml.yaml_keywords import EcalcYamlKeywords @@ -179,34 +180,12 @@ def as_pydantic_compatible(self) -> PydanticLoc: return tuple(keys) -@dataclass -class FileMark: - line_number: int - column_number: int - - -@dataclass -class FileContext: - start: FileMark - end: FileMark - - @dataclass class ModelValidationError: - details: ErrorDetails data: Optional[Dict] - - @property - def location(self) -> Location: - return Location.from_pydantic_loc(self.details["loc"]) - - @property - def message(self): - return self.details["msg"] - - @property - def input(self): - return self.details["input"] + location: Location + message: str + file_context: FileContext @property def yaml(self) -> Optional[str]: @@ -215,27 +194,6 @@ def yaml(self) -> Optional[str]: return yaml.dump(self.data, sort_keys=False).strip() - @property - def file_context(self) -> Optional[FileContext]: - if hasattr(self.data, "end_mark") and hasattr(self.data, "start_mark"): - # This only works with our implementation of pyyaml read - # In the future, we can move this logic into PyYamlYamlModel with a better interface in YamlValidator. - # Specifically with our own definition of the returned data in each property in YamlValidator. - start_mark = self.data.start_mark - end_mark = self.data.end_mark - return FileContext( - start=FileMark( - line_number=start_mark.line + 1, - column_number=start_mark.column, - ), - end=FileMark( - line_number=end_mark.line + 1, - column_number=end_mark.column, - ), - ) - - return None - def __str__(self): msg = self.location.as_dot_separated() msg += f":\t{self.message}" @@ -286,10 +244,13 @@ def _get_context_data(self, loc: PydanticLoc) -> Optional[Dict]: def errors(self) -> List[ModelValidationError]: errors = [] for error in custom_errors(e=self.validation_error, custom_messages=CUSTOM_MESSAGES): + data = self._get_context_data(loc=error["loc"]) errors.append( ModelValidationError( - details=error, - data=self._get_context_data(loc=error["loc"]), + location=Location.from_pydantic_loc(error["loc"]), + message=error["msg"], + file_context=FileContext.from_yaml_dict(data), + data=data, ) ) return errors diff --git a/src/libecalc/presentation/yaml/yaml_models/exceptions.py b/src/libecalc/presentation/yaml/yaml_models/exceptions.py new file mode 100644 index 0000000000..9ebbb7b23d --- /dev/null +++ b/src/libecalc/presentation/yaml/yaml_models/exceptions.py @@ -0,0 +1,20 @@ +from typing import Optional + +from libecalc.presentation.yaml.file_context import FileContext + + +class YamlError(Exception): + def __init__(self, problem: str, file_context: Optional[FileContext] = None): + self.problem = problem + self.file_context = file_context + message = f"{problem}" + if file_context is not None: + message += str(file_context) + + super().__init__(message) + + +class DuplicateKeyError(YamlError): + def __init__(self, key: str, file_context: FileContext): + self.key = key + super().__init__(f"Duplicate key {key!r} is found", file_context) diff --git a/src/libecalc/presentation/yaml/yaml_models/pyyaml_yaml_model.py b/src/libecalc/presentation/yaml/yaml_models/pyyaml_yaml_model.py index b533520e3d..5f6eb11556 100644 --- a/src/libecalc/presentation/yaml/yaml_models/pyyaml_yaml_model.py +++ b/src/libecalc/presentation/yaml/yaml_models/pyyaml_yaml_model.py @@ -9,12 +9,13 @@ from typing_extensions import Self, deprecated from yaml import SafeLoader -from libecalc.common.errors.exceptions import EcalcError, ProgrammingError +from libecalc.common.errors.exceptions import ProgrammingError from libecalc.common.time_utils import convert_date_to_datetime from libecalc.dto.utils.validators import ( COMPONENT_NAME_ALLOWED_CHARS, COMPONENT_NAME_PATTERN, ) +from libecalc.presentation.yaml.file_context import FileMark from libecalc.presentation.yaml.validation_errors import ( DataValidationError, DtoValidationError, @@ -29,6 +30,11 @@ YamlTimeseriesType, ) from libecalc.presentation.yaml.yaml_keywords import EcalcYamlKeywords +from libecalc.presentation.yaml.yaml_models.exceptions import ( + DuplicateKeyError, + FileContext, + YamlError, +) from libecalc.presentation.yaml.yaml_models.yaml_model import YamlModel, YamlValidator from libecalc.presentation.yaml.yaml_types.components.yaml_asset import YamlAsset from libecalc.presentation.yaml.yaml_types.time_series.yaml_time_series import ( @@ -151,25 +157,28 @@ def construct_mapping(self, node, deep=False): for key_node, _ in node.value: each_key = self.construct_object(key_node, deep=deep) if each_key in mapping: - raise DataValidationError( - data=None, - message=f"Duplicate key: {each_key!r} is found in {key_node.start_mark.name} on line {key_node.start_mark.line + 1}", + raise DuplicateKeyError( + key=each_key, + file_context=FileContext( + name=key_node.start_mark.name, + start=FileMark( + line_number=key_node.start_mark.line + 1, + column_number=key_node.start_mark.column + 1, + ), + ), ) mapping.add(each_key) return super().construct_mapping(node, deep) if re.search(COMPONENT_NAME_PATTERN, Path(yaml_file.name).stem) is None: - raise EcalcError( - title="Bad Yaml file name", - message=f"The model file, {yaml_file.name}, contains illegal special characters. " + raise YamlError( + problem=f"The model file, {yaml_file.name}, contains illegal special characters. " f"Allowed characters are {COMPONENT_NAME_ALLOWED_CHARS}", ) try: return yaml.load(yaml_file, Loader=UniqueKeyLoader) # noqa: S506 - loader should be SafeLoader except KeyError as e: - raise EcalcError( - title="Bad Yaml file", message=f"Error occurred while loading yaml file, key {e} not found" - ) from e + raise YamlError(problem=f"Error occurred while loading yaml file, key {e} not found") from e def dump_and_load(self, yaml_file: ResourceStream): return yaml.dump(self.load(yaml_file), Dumper=PyYamlYamlModel.IndentationDumper, sort_keys=False) @@ -211,13 +220,37 @@ def read_yaml( base_dir: Optional[Path] = None, resources: Optional[Dict[str, TextIO]] = None, ) -> YamlDict: - return PyYamlYamlModel._read_yaml_helper( - yaml_file=main_yaml, - loader=PyYamlYamlModel.SafeLineLoader, - enable_include=enable_include, - base_dir=base_dir, - resources=resources, - ) + try: + return PyYamlYamlModel._read_yaml_helper( + yaml_file=main_yaml, + loader=PyYamlYamlModel.SafeLineLoader, + enable_include=enable_include, + base_dir=base_dir, + resources=resources, + ) + except yaml.YAMLError as e: + file_context = None + if hasattr(e, "problem_mark"): + mark = e.problem_mark + if mark is not None: + file_context = FileContext( + name=mark.name, + start=FileMark( + line_number=mark.line + 1, + column_number=mark.column + 1, + ), + ) + + problem = "Invalid YAML file" + if hasattr(e, "problem"): + optional_problem = e.problem + if optional_problem is not None: + problem = optional_problem + + raise YamlError( + problem=problem, + file_context=file_context, + ) from e # start of validation/parsing methods diff --git a/src/libecalc/presentation/yaml/yaml_models/ruamel_yaml_model.py b/src/libecalc/presentation/yaml/yaml_models/ruamel_yaml_model.py index a554d78311..93d7917d95 100644 --- a/src/libecalc/presentation/yaml/yaml_models/ruamel_yaml_model.py +++ b/src/libecalc/presentation/yaml/yaml_models/ruamel_yaml_model.py @@ -2,14 +2,14 @@ from pathlib import Path from typing import Any, Dict, Optional, TextIO -from ruamel.yaml import YAML +from ruamel.yaml import YAML, YAMLError from libecalc.common.errors.exceptions import ( - EcalcError, - EcalcErrorType, ProgrammingError, ) +from libecalc.presentation.yaml.file_context import FileContext, FileMark from libecalc.presentation.yaml.yaml_entities import ResourceStream +from libecalc.presentation.yaml.yaml_models.exceptions import YamlError from libecalc.presentation.yaml.yaml_models.yaml_model import YamlModel @@ -126,12 +126,31 @@ def _load( resources=resources, enable_include=enable_include, base_dir=base_dir ).load(yaml_file) except KeyError as ke: - raise EcalcError( - title="Bad Yaml file", message=f"Error occurred while loading yaml file, key {ke} not found" - ) from ke + raise YamlError(problem=f"Error occurred while loading yaml file, key {ke} not found") from ke + except YAMLError as e: + file_context = None + if hasattr(e, "problem_mark"): + mark = e.problem_mark + if mark is not None: + file_context = FileContext( + name=mark.name, + start=FileMark( + line_number=mark.line + 1, + column_number=mark.column + 1, + ), + ) + + problem = "Invalid YAML file" + if hasattr(e, "problem"): + optional_problem = e.problem + if optional_problem is not None: + problem = optional_problem + + raise YamlError( + problem=problem, + file_context=file_context, + ) from e except Exception as e: - raise EcalcError( - error_type=EcalcErrorType.CLIENT_ERROR, - title="Error loading yaml", - message="We are not able to load the yaml due to an error: " + str(e), + raise YamlError( + problem="We are not able to load the yaml due to an error: " + str(e), ) from e diff --git a/src/tests/ecalc_cli/test_app.py b/src/tests/ecalc_cli/test_app.py index fc60f09ec3..e36d5ace37 100644 --- a/src/tests/ecalc_cli/test_app.py +++ b/src/tests/ecalc_cli/test_app.py @@ -15,6 +15,7 @@ from libecalc.common.run_info import RunInfo from libecalc.dto.utils.validators import COMPONENT_NAME_ALLOWED_CHARS from libecalc.presentation.yaml.yaml_entities import ResourceStream +from libecalc.presentation.yaml.yaml_models.exceptions import YamlError from libecalc.presentation.yaml.yaml_models.pyyaml_yaml_model import PyYamlYamlModel from typer.testing import CliRunner @@ -671,7 +672,7 @@ def test_yaml_file_error(self): stream = StringIO("") yaml_stream = ResourceStream(name=yaml_wrong_name.name, stream=stream) - with pytest.raises(EcalcError) as ee: + with pytest.raises(YamlError) as ee: yaml_reader.load(yaml_file=yaml_stream) assert ( diff --git a/src/tests/libecalc/input/test_file_io.py b/src/tests/libecalc/input/test_file_io.py index c5f410bf0e..97fcf47290 100644 --- a/src/tests/libecalc/input/test_file_io.py +++ b/src/tests/libecalc/input/test_file_io.py @@ -10,8 +10,8 @@ from libecalc.infrastructure import file_io from libecalc.presentation.yaml import yaml_entities from libecalc.presentation.yaml.model import YamlModel -from libecalc.presentation.yaml.validation_errors import DataValidationError from libecalc.presentation.yaml.yaml_entities import YamlTimeseriesType +from libecalc.presentation.yaml.yaml_models.exceptions import DuplicateKeyError from libecalc.presentation.yaml.yaml_models.pyyaml_yaml_model import PyYamlYamlModel @@ -288,7 +288,7 @@ def test_detect_duplicate_keys_in_yaml(self): name="main.yaml", ) - with pytest.raises(DataValidationError) as ve: + with pytest.raises(DuplicateKeyError) as ve: PyYamlYamlModel.read_yaml(main_yaml=main_yaml) assert "Duplicate key" in str(ve.value) diff --git a/src/tests/libecalc/presentation/yaml/yaml_models/test_pyyaml_yaml_model.py b/src/tests/libecalc/presentation/yaml/yaml_models/test_pyyaml_yaml_model.py index 7ed6c3cf74..db9c2cc7a9 100644 --- a/src/tests/libecalc/presentation/yaml/yaml_models/test_pyyaml_yaml_model.py +++ b/src/tests/libecalc/presentation/yaml/yaml_models/test_pyyaml_yaml_model.py @@ -33,13 +33,13 @@ def test_pyyaml_validation(self, yaml_resource_with_errors): assert len(errors) == 3 - assert errors[0].details["type"] == "union_tag_not_found" + assert "Unable to extract tag" in errors[0].message assert errors[0].location.keys == ["TIME_SERIES", 0] - assert errors[1].details["type"] == "missing" + assert "This keyword is missing, it is required" in errors[1].message assert errors[1].location.keys == ["FUEL_TYPES"] - assert errors[2].details["type"] == "missing" + assert "This keyword is missing, it is required" in errors[2].message assert errors[2].location.keys == ["INSTALLATIONS"] def test_invalid_expression_token(self, minimal_model_yaml_factory): @@ -51,8 +51,6 @@ def test_invalid_expression_token(self, minimal_model_yaml_factory): errors = exc_info.value.errors() assert len(errors) == 1 - assert errors[0].details["type"] == "expression_reference_not_found" - assert errors[0].details["ctx"]["expression_references"] == ["SIM1;NOTHING"] assert errors[0].message == "Expression reference(s) SIM1;NOTHING does not exist." def test_expression_token_validation_ignored_if_no_context(self, minimal_model_yaml_factory): diff --git a/src/tests/libecalc/presentation/yaml/yaml_models/test_yaml_model_errors.py b/src/tests/libecalc/presentation/yaml/yaml_models/test_yaml_model_errors.py new file mode 100644 index 0000000000..774f527d9e --- /dev/null +++ b/src/tests/libecalc/presentation/yaml/yaml_models/test_yaml_model_errors.py @@ -0,0 +1,39 @@ +from io import StringIO + +import pytest +from libecalc.presentation.yaml.yaml_entities import ResourceStream +from libecalc.presentation.yaml.yaml_models.exceptions import YamlError +from libecalc.presentation.yaml.yaml_models.yaml_model import YamlModel, YamlModelType + +invalid_models = [ + ( + """ +`INSTALLATIONS: [] +""", + "invalid_token_start", + ), + ( + """ +{}INSTALLATIONS: [] +""", + "invalid_object_start", + ), + ( + """ +VARIABLES: + some_var: 1 + some_var: 2 +""", + "duplicate_keys", + ), +] + + +@pytest.mark.parametrize("invalid_model,description", invalid_models) +@pytest.mark.parametrize("yaml_model_type", YamlModelType) +def test_invalid_models(invalid_model: str, description: str, yaml_model_type: YamlModelType): + yaml_reader = YamlModel.Builder.get_yaml_model(yaml_model_type) + with pytest.raises(YamlError) as exc_info: + yaml_reader.read(ResourceStream(name=description, stream=StringIO(invalid_model))) + + assert str(exc_info.value)