Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vdk-core: BaseVdkError exception propagation flaw fix #917

Merged
merged 1 commit into from
Jul 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from typing import List
from typing import Optional

from click import ClickException
from vdk.internal.builtin_plugins.run.run_status import ExecutionStatus
from vdk.internal.core import errors
from vdk.internal.core.errors import ErrorMessage
Expand Down Expand Up @@ -78,11 +77,7 @@ def get_exception_to_raise(self):
Returns main exception to be used as a failure reason for the data job.
"""
if self.exception:
return (
Exception(self.exception.message)
if isinstance(self.exception, ClickException)
else self.exception
)
return self.exception
step_exceptions = map(lambda x: x.exception, self.steps_list)
step_exception = next(filter(lambda e: e is not None, step_exceptions), None)
if step_exception:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
from pathlib import Path
from typing import cast

from click import ClickException
from vdk.api.data_job import IStandaloneDataJob
from vdk.api.job_input import IJobInput
from vdk.api.plugin.core_hook_spec import CoreHookSpecs
Expand Down Expand Up @@ -150,9 +149,7 @@ def __exit__(self, exc_type, exc_value, exc_traceback):
# if at least one hook implementation returned handled, means we do not need to log the exception
if not (True in handled):
log.exception("Exiting with exception.")
exit_code = (
exc_value.exit_code if isinstance(exc_value, ClickException) else 1
)
exit_code = 2
else:
exit_code = 0
else:
Expand Down
3 changes: 1 addition & 2 deletions projects/vdk-core/src/vdk/internal/cli_entry.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import click
import click_log
from click import ClickException
from click_plugins import with_plugins
from pkg_resources import iter_entry_points
from vdk.api.plugin.core_hook_spec import CoreHookSpecs
Expand Down Expand Up @@ -143,7 +142,7 @@ def vdk_main(
# if at least one hook implementation returned handled, means we do not need to log the exception
if not (True in handled):
log.exception("Exiting with exception.")
exit_code = e.exit_code if isinstance(e, ClickException) else 1
exit_code = 1
else:
exit_code = 0
return exit_code
Expand Down
4 changes: 1 addition & 3 deletions projects/vdk-core/src/vdk/internal/core/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
from typing import Any
from typing import cast

from click import ClickException


class ResolvableBy(str, Enum):
"""
Expand Down Expand Up @@ -51,7 +49,7 @@ class ResolvableBy(str, Enum):
log = logging.getLogger(__name__)


class BaseVdkError(ClickException):
class BaseVdkError(Exception):
"""For all errors risen from our "code" (vdk)

There are two child branches in exception hierarchy:
Expand Down
15 changes: 15 additions & 0 deletions projects/vdk-core/tests/functional/run/test_run_errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,21 @@ def initialize_job(self, context: JobContext):
)


def test_run_exception_handled(tmp_termination_msg_file: pathlib.Path):
errors.clear_intermediate_errors()

class ExceptionHandler:
@staticmethod
@hookimpl
def vdk_exception(self, exception: Exception) -> bool:
return True

runner = CliEntryBasedTestRunner(ExceptionHandler())

result: Result = runner.invoke(["run", util.job_path("simple-job")])
cli_assert_equal(0, result)


def test_run_job_plugin_fails(tmp_termination_msg_file):
errors.clear_intermediate_errors()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,6 @@ def test_router_raise_error_chained_ingest_plugins_not_registered():
router.send_tabular_data_for_ingestion(rows=["b"], column_names=["a"])

error_msg = exc_info.value
assert (
"Could not create new processor plugin of type pre-ingest-test"
in error_msg.message
assert "Could not create new processor plugin of type pre-ingest-test" in str(
error_msg
)