From 0b113dc97fcb95ea5603c8109825148711711332 Mon Sep 17 00:00:00 2001 From: Nicola Coretti Date: Wed, 23 Oct 2024 15:43:17 +0200 Subject: [PATCH] Make sure ExaError does not raise (#41) --- README.md | 3 - doc/changes/unreleased.md | 9 +- error-codes.json | 36 +++++ exasol/error/_error.py | 127 ++++++++++++------ .../error_message_builder.py | 24 ++-- noxconfig.py | 20 ++- test/unit/error_test.py | 127 ++++++++++++------ tests/test_error_code_format.py | 19 ++- 8 files changed, 259 insertions(+), 106 deletions(-) create mode 100644 error-codes.json diff --git a/README.md b/README.md index 676643d..b00549f 100644 --- a/README.md +++ b/README.md @@ -91,9 +91,6 @@ you can use the generate subcommand. ec generate NAME VERSION PACKAGE_ROOT > error-codes.json ``` -## Known Issues -* [Throws exception on invalid error code format](https://github.com/exasol/error-reporting-python/issues/27) - ### Information for Users * [User Guide](doc/user_guide/user_guide.md) diff --git a/doc/changes/unreleased.md b/doc/changes/unreleased.md index fdc4b02..d2c0d0f 100644 --- a/doc/changes/unreleased.md +++ b/doc/changes/unreleased.md @@ -1,8 +1,9 @@ # Unreleased -## Internal -* Relock Dependencies +## Fixes + +* [#27](https://github.com/exasol/error-reporting-python/issues/27) Make sure creating an error does not raise an exception. ## Feature @@ -11,3 +12,7 @@ ## Refactorings * #33: Updated to Python 3.10 + +## Internal +* Relock Dependencies + diff --git a/error-codes.json b/error-codes.json new file mode 100644 index 0000000..f91f30d --- /dev/null +++ b/error-codes.json @@ -0,0 +1,36 @@ +{ + "$schema": "https://schemas.exasol.com/error_code_report-1.0.0.json", + "projectName": "exasol-error-reporting", + "projectVersion": "0.4.0", + "errorCodes": [ + { + "identifier": "E-ERP-1", + "message": "Invalid error code {{code}}.", + "messagePlaceholders": [ + { + "placeholder": "code", + "description": "Error code which was causing the error." + } + ], + "mitigations": [ + "Ensure you follow the standard error code format." + ], + "sourceFile": "_error.py" + }, + { + "identifier": "E-ERP-2", + "message": "Unknown error/exception occurred.", + "messagePlaceholders": [ + { + "placeholder": "traceback", + "description": "Exception traceback which lead to the generation of this error." + } + ], + "description": "An unexpected error occurred during the creation of the error", + "mitigations": [ + "A good starting point would be to investigate the cause of the attached exception.\n\nTrackback:\n {{traceback}}" + ], + "sourceFile": "_error.py" + } + ] +} \ No newline at end of file diff --git a/exasol/error/_error.py b/exasol/error/_error.py index 2c3ca1f..7d4a603 100644 --- a/exasol/error/_error.py +++ b/exasol/error/_error.py @@ -1,10 +1,13 @@ import warnings from dataclasses import dataclass +from inspect import cleandoc from typing import Dict, Iterable, List, Mapping, Union +from pathlib import Path with warnings.catch_warnings(): warnings.simplefilter("ignore") from exasol_error_reporting_python import exa_error + from exasol_error_reporting_python.error_message_builder import InvalidErrorCode @dataclass(frozen=True) @@ -15,11 +18,11 @@ class Parameter: class Error: def __init__( - self, - code: str, - message: str, - mitigations: Union[str, Iterable[str]], - parameters: Dict[str, Union[str, Parameter]], + self, + code: str, + message: str, + mitigations: Union[str, Iterable[str]], + parameters: Dict[str, Union[str, Parameter]], ): # This function maybe flattened into or moved out of the constructor in the future. def build_error(code, msg, mitigations, params): @@ -47,11 +50,50 @@ def __str__(self) -> str: # See also see, Github Issue #28 https://github.com/exasol/error-reporting-python/issues/28 +# ATTENTION: In the event of an exception while creating an error, we encounter a chicken-and-egg problem regarding error definitions. +# Therefore, errors created by this library must be defined "statically" within this file. +# Details should then be used to create a low-level error. The information should also be used to create/update the error-codes.json +# as part of the release preparation. This is only necessary for this library, as it is the root, other libraries should use +# the `ec` command-line tool to update and create their project specifc error-codes.json file. +LIBRARY_ERRORS = { + "E-ERP-1": { + "identifier": "E-ERP-1", + "message": "Invalid error code {{code}}.", + "messagePlaceholders": [ + { + "placeholder": "code", + "description": "Error code which was causing the error." + } + ], + "mitigations": ["Ensure you follow the standard error code format."], + "sourceFile": Path(__file__).name + }, + "E-ERP-2": { + "identifier": "E-ERP-2", + "message": "Unknown error/exception occurred.", + "messagePlaceholders": [ + { + "placeholder": "traceback", + "description": "Exception traceback which lead to the generation of this error." + } + ], + "description": "An unexpected error occurred during the creation of the error", + "mitigations": [cleandoc(""" + A good starting point would be to investigate the cause of the attached exception. + + Trackback: + {{traceback}} + """)], + "sourceFile": Path(__file__).name + }, +} + + def ExaError( - code: str, - message: str, - mitigations: Union[str, List[str]], - parameters: Mapping[str, Union[str, Parameter]], + code: str, + message: str, + mitigations: Union[str, List[str]], + parameters: Mapping[str, Union[str, Parameter]], ) -> Error: """Create a new ExaError. @@ -66,33 +108,42 @@ def ExaError( Returns: An error type containing all relevant information. - - - Raises: - This function should not raise under any circumstances. - In case of error, it should report it also as an error type which is returned by this function. - Still the returned error should contain a references or information about the original call. - - Attention: - - FIXME: Due to legacy reasons this function currently still may raise an `ValueError` (Refactoring Required). - - Potential error scenarios which should taken into account are the following ones: - - * E-ERP-1: Invalid error code provided - params: - * Original ErrorCode - * Original error message - * Original mitigations - * Original parameters - * E-ERP-2 Unknown error/exception occurred - params: - * exception/msg - * Original ErrorCode - * Original error message - * Original mitigations - * Original parameters - - see also [Github Issue #27](https://github.com/exasol/error-reporting-python/issues/27) """ - return Error(code, message, mitigations, parameters) + try: + return Error(code, message, mitigations, parameters) + except InvalidErrorCode: + error_code = 'E-ERP-1' + error_details = LIBRARY_ERRORS[error_code] + return Error( + code=error_details['identifier'], + message=error_details['message'], + mitigations=error_details['mitigations'], + parameters={"code": code} + ) + except Exception as ex: + import traceback + tb = traceback.format_exc() + parameters = {"traceback": tb} + error_code = 'E-ERP-2' + error_details = LIBRARY_ERRORS[error_code] + return Error( + code=error_details['identifier'], + message=error_details['message'], + mitigations=error_details['mitigations'], + parameters=parameters + ) + + +def _create_error_code_definitions(version=None): + from exasol.error.version import VERSION + version = version or VERSION + return { + "$schema": "https://schemas.exasol.com/error_code_report-1.0.0.json", + "projectName": "exasol-error-reporting", + "projectVersion": version, + "errorCodes": [code for code in LIBRARY_ERRORS.values()] + } + + +if __name__ == "__main__": + pass diff --git a/exasol_error_reporting_python/error_message_builder.py b/exasol_error_reporting_python/error_message_builder.py index ec42d55..dd34d54 100644 --- a/exasol_error_reporting_python/error_message_builder.py +++ b/exasol_error_reporting_python/error_message_builder.py @@ -6,6 +6,12 @@ ERROR_CODE_FORMAT = "^[FEW]-[A-Z][A-Z0-9]+(-[A-Z][A-Z0-9]+)*-[0-9]+$" +class InvalidErrorCode(ValueError): + """ + Indicates that the error code does not comply with the commonly defined error code format. + """ + + class ErrorMessageBuilder: """ This class is a builder for Exasol error messages. @@ -13,7 +19,7 @@ class ErrorMessageBuilder: def __init__(self, error_code: str): if not re.compile(ERROR_CODE_FORMAT).match(error_code): - raise ValueError(f"{error_code} is an invalid error-code format") + raise InvalidErrorCode(f"{error_code} is an invalid error-code format") self._error_code = error_code self._message_builder = [] @@ -64,8 +70,10 @@ def ticket_mitigation(self): :return: self of the ErrorMessageBuilder object """ - self.mitigation("This is an internal error that should not happen. " - "Please report it by opening a GitHub issue.") + self.mitigation( + "This is an internal error that should not happen. " + "Please report it by opening a GitHub issue." + ) return self def parameter(self, placeholder: str, value: Any, description: str = None): @@ -97,7 +105,7 @@ def _add_parameters(self, text: str, arguments: Any): self._parameter_dict = { **self._parameter_dict, - **ParametersMapper.get_params_dict(text, arguments) + **ParametersMapper.get_params_dict(text, arguments), } def _replace_placeholder(self, text: str) -> str: @@ -108,8 +116,7 @@ def _replace_placeholder(self, text: str) -> str: :return: formatted text by replacing placeholders with arguments. """ - return PlaceholderHandler.replace_placeholder( - text, self._parameter_dict) + return PlaceholderHandler.replace_placeholder(text, self._parameter_dict) def __str__(self): result = [] @@ -125,8 +132,9 @@ def __str__(self): elif len(self._mitigations) > 1: mitigations = ["Known mitigations:"] for mitigation in self._mitigations: - mitigations.append("".join( - ("* ", self._replace_placeholder(mitigation)))) + mitigations.append( + "".join(("* ", self._replace_placeholder(mitigation))) + ) result.append("\n".join(mitigations)) return " ".join(result) diff --git a/noxconfig.py b/noxconfig.py index 79cdb7e..04e8140 100644 --- a/noxconfig.py +++ b/noxconfig.py @@ -5,6 +5,24 @@ from pathlib import Path from typing import Iterable +from exasol.toolbox.nox.plugin import hookimpl + + +class UpdateErrorCodes: + ERROR_CODES: Path = Path(__file__).parent / "error-codes.json" + + @hookimpl + def prepare_release_update_version(self, session, config, version): + import json + from exasol.error._error import _create_error_code_definitions + definitions = _create_error_code_definitions(f'{version}') + with open(UpdateErrorCodes.ERROR_CODES, 'w') as f: + json.dump(definitions, f) + + @hookimpl + def prepare_release_add_files(self, session, config): + return [self.ERROR_CODES] + @dataclass(frozen=True) class Config: @@ -14,7 +32,7 @@ class Config: doc: Path = Path(__file__).parent / "doc" version_file: Path = Path(__file__).parent / "exasol" / "error" / "version.py" path_filters: Iterable[str] = ("dist", ".eggs", "venv") - plugins = [] + plugins = [UpdateErrorCodes] PROJECT_CONFIG = Config() diff --git a/test/unit/error_test.py b/test/unit/error_test.py index 3e0f25d..7ae28b8 100644 --- a/test/unit/error_test.py +++ b/test/unit/error_test.py @@ -22,40 +22,51 @@ def does_not_raise(): "expected,data", [ ( - cleandoc( - """ - E-TEST-1: Not enough space on device '/dev/sda1'. Known mitigations: - * Delete something from '/dev/sda1'. - * Create larger partition. - """ - ), - Data( - code="E-TEST-1", - message="Not enough space on device {{device}}.", - mitigations=[ - "Delete something from {{device}}.", - "Create larger partition.", - ], - parameters={"device": Parameter("/dev/sda1", "name of the device")}, - ), - ), - ( - cleandoc( - """ + cleandoc( + """ E-TEST-1: Not enough space on device '/dev/sda1'. Known mitigations: * Delete something from '/dev/sda1'. * Create larger partition. """ - ), - Data( - code="E-TEST-1", - message="Not enough space on device {{device}}.", - mitigations=[ - "Delete something from {{device}}.", - "Create larger partition.", - ], - parameters={"device": "/dev/sda1"}, - ), + ), + Data( + code="E-TEST-1", + message="Not enough space on device {{device}}.", + mitigations=[ + "Delete something from {{device}}.", + "Create larger partition.", + ], + parameters={"device": Parameter("/dev/sda1", "name of the device")}, + ), + ), + ( + cleandoc( + """ + E-TEST-1: Not enough space on device '/dev/sda1'. Known mitigations: + * Delete something from '/dev/sda1'. + * Create larger partition. + """ + ), + Data( + code="E-TEST-1", + message="Not enough space on device {{device}}.", + mitigations=[ + "Delete something from {{device}}.", + "Create larger partition.", + ], + parameters={"device": "/dev/sda1"}, + ), + ), + ( + cleandoc( + "E-ERP-1: Invalid error code 'WRONGCODE'. Ensure you follow the standard error code format." + ), + Data( + code="WRONGCODE", + message="some error message", + mitigations=["unrecoverable ;P"], + parameters={}, + ), ), ], ) @@ -65,23 +76,19 @@ def test_exa_error_as_string(expected, data): assert actual == expected -@pytest.mark.xfail( - True, - reason="Old implementation does not avoid throwing exceptions, further refactoring is needed.", -) @pytest.mark.parametrize( "data", [ ( - Data( - code="BROKEN_ERROR_CODE", - message='"Not enough space on device {{device}}."', - mitigations=[ - "Delete something from {{device}}.", - "Create larger partition.", - ], - parameters={"device": Parameter("/dev/sda1", "name of the device")}, - ) + Data( + code="BROKEN_ERROR_CODE", + message='"Not enough space on device {{device}}."', + mitigations=[ + "Delete something from {{device}}.", + "Create larger partition.", + ], + parameters={"device": Parameter("/dev/sda1", "name of the device")}, + ) ), ], ) @@ -90,6 +97,40 @@ def test_exa_error_does_not_throw_error_on_invalid(data): _ = ExaError(data.code, data.message, data.mitigations, data.parameters) +@pytest.mark.parametrize( + "data", + [ + Data( + code="E-TEST-1", + message="some error message", + mitigations=["unrecoverable ;P"], + parameters={}, + ), + ], +) +def test_raising_message_builder(data): + from unittest.mock import patch + from exasol_error_reporting_python.exa_error import ExaError as Error + + actual_impl = Error.message_builder + + def builder(error_code): + if error_code == "E-ERP-2": + return actual_impl(error_code) + raise Exception(f"{error_code}") + + with patch("exasol_error_reporting_python.exa_error.ExaError") as mock: + mock.message_builder = builder + error = ExaError(data.code, data.message, data.mitigations, data.parameters) + actual = str(error) + expected = cleandoc(""" + E-ERP-2: Unknown error/exception occurred. A good starting point would be to investigate the cause of the attached exception. + + Trackback: + """) + assert expected in actual + + def test_using_the_old_api_produces_deprecation_warning(): with pytest.warns(DeprecationWarning): # due to the fact that the exasol.error package already imports the package at an earlier stage diff --git a/tests/test_error_code_format.py b/tests/test_error_code_format.py index b34564a..04a5dc4 100644 --- a/tests/test_error_code_format.py +++ b/tests/test_error_code_format.py @@ -2,8 +2,10 @@ import pytest -from exasol_error_reporting_python.error_message_builder import \ - ErrorMessageBuilder +from exasol_error_reporting_python.error_message_builder import ( + ErrorMessageBuilder, + InvalidErrorCode, +) class TestErrorCode(unittest.TestCase): @@ -12,19 +14,14 @@ def test_invalid_error_code_format(self): "A-EXA-100", "F1-EXA-100", "F-1EXA-100", - "F-EXA-EXA-F1" - "F-EXA-EXA", - "F-100" + "F-EXA-EXA-F1" "F-EXA-EXA", + "F-100", ] for error_code in invalid_error_code_list: - with pytest.raises(ValueError): + with pytest.raises(InvalidErrorCode): assert ErrorMessageBuilder(error_code) def test_valid_error_code_format(self): - valid_error_code_list = [ - "F-EXA-100", - "E-EXA-EXA-100", - "W-E6-EXA-1001" - ] + valid_error_code_list = ["F-EXA-100", "E-EXA-EXA-100", "W-E6-EXA-1001"] for error_code in valid_error_code_list: self.assertTrue(ErrorMessageBuilder(error_code))