From c2b8990db7426c340a427588724987ed3ea539ff Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Wed, 15 Jan 2025 17:13:27 -0600 Subject: [PATCH 1/5] Move `WARN_ERROR` and `WARN_ERROR_OPTIONS` to properties of `EventManager` `WARN_ERROR` and `WARN_ERROR_OPTIONS` were introduced _before_ we had our `EventManager` architecture. We never moved them, because we never had a need to. However, we're working to wrap `warn_or_error` functionality into `fire_event`. As such, `EventManager` will need the context of `warn_error` and `warn_errror_options`. As a secondary effect, we're eliminating some globals / module variables, which will make our event architecture a little less complicated. --- dbt_common/events/event_manager.py | 5 +++++ dbt_common/events/functions.py | 8 +++----- tests/unit/test_functions.py | 8 ++++---- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/dbt_common/events/event_manager.py b/dbt_common/events/event_manager.py index 507588f3..39e04de1 100644 --- a/dbt_common/events/event_manager.py +++ b/dbt_common/events/event_manager.py @@ -4,12 +4,15 @@ from dbt_common.events.base_types import BaseEvent, EventLevel, msg_from_base_event, TCallback from dbt_common.events.logger import LoggerConfig, _Logger, _TextLogger, _JsonLogger, LineFormat +from dbt_common.helper_types import WarnErrorOptions class EventManager: def __init__(self) -> None: self.loggers: List[_Logger] = [] self.callbacks: List[TCallback] = [] + self.warn_error: bool = False + self.warn_error_options: WarnErrorOptions = WarnErrorOptions(include=[], exclude=[]) def fire_event(self, e: BaseEvent, level: Optional[EventLevel] = None) -> None: msg = msg_from_base_event(e, level=level) @@ -48,6 +51,8 @@ def flush(self) -> None: class IEventManager(Protocol): callbacks: List[TCallback] loggers: List[_Logger] + warn_error: bool + warn_error_options: WarnErrorOptions def fire_event(self, e: BaseEvent, level: Optional[EventLevel] = None) -> None: ... diff --git a/dbt_common/events/functions.py b/dbt_common/events/functions.py index 86d68237..6e88e1c2 100644 --- a/dbt_common/events/functions.py +++ b/dbt_common/events/functions.py @@ -3,7 +3,6 @@ from dbt_common.events.event_manager_client import get_event_manager from dbt_common.exceptions import EventCompilationError from dbt_common.invocation import get_invocation_id -from dbt_common.helper_types import WarnErrorOptions from dbt_common.utils.encoding import ForgivingJSONEncoder from dbt_common.events.base_types import BaseEvent, EventLevel, EventMsg from dbt_common.events.logger import LoggerConfig, LineFormat @@ -19,8 +18,6 @@ LOG_VERSION = 3 metadata_vars: Optional[Dict[str, str]] = None _METADATA_ENV_PREFIX = "DBT_ENV_CUSTOM_ENV_" -WARN_ERROR_OPTIONS = WarnErrorOptions(include=[], exclude=[]) -WARN_ERROR = False # This global, and the following two functions for capturing stdout logs are # an unpleasant hack we intend to remove as part of API-ification. The GitHub @@ -116,9 +113,10 @@ def msg_to_dict(msg: EventMsg) -> dict: def warn_or_error(event, node=None) -> None: event_name = type(event).__name__ - if WARN_ERROR or WARN_ERROR_OPTIONS.includes(event_name): + manager = get_event_manager() + if manager.warn_error or manager.warn_error_options.includes(event_name): raise EventCompilationError(event.message(), node) - elif not WARN_ERROR_OPTIONS.silenced(event_name): + elif not manager.warn_error_options.silenced(event_name): fire_event(event) diff --git a/tests/unit/test_functions.py b/tests/unit/test_functions.py index 6c4126a1..821499e2 100644 --- a/tests/unit/test_functions.py +++ b/tests/unit/test_functions.py @@ -3,7 +3,7 @@ from dbt_common.events import functions from dbt_common.events.base_types import EventLevel, WarnLevel from dbt_common.events.event_manager import EventManager -from dbt_common.events.event_manager_client import ctx_set_event_manager +from dbt_common.events.event_manager_client import ctx_set_event_manager, get_event_manager from dbt_common.exceptions import EventCompilationError from dbt_common.helper_types import WarnErrorOptions from tests.unit.utils import EventCatcher @@ -40,7 +40,7 @@ def valid_error_names() -> Set[str]: class TestWarnOrError: def test_fires_error(self, valid_error_names: Set[str]) -> None: - functions.WARN_ERROR_OPTIONS = WarnErrorOptions( + get_event_manager().warn_error_options = WarnErrorOptions( include="*", valid_error_names=valid_error_names ) with pytest.raises(EventCompilationError): @@ -52,7 +52,7 @@ def test_fires_warning( event_catcher: EventCatcher, set_event_manager_with_catcher: None, ) -> None: - functions.WARN_ERROR_OPTIONS = WarnErrorOptions( + get_event_manager().warn_error_options = WarnErrorOptions( include="*", exclude=list(valid_error_names), valid_error_names=valid_error_names ) functions.warn_or_error(Note(msg="hi")) @@ -65,7 +65,7 @@ def test_silenced( event_catcher: EventCatcher, set_event_manager_with_catcher: None, ) -> None: - functions.WARN_ERROR_OPTIONS = WarnErrorOptions( + get_event_manager().warn_error_options = WarnErrorOptions( include="*", silence=list(valid_error_names), valid_error_names=valid_error_names ) functions.warn_or_error(Note(msg="hi")) From 66ddf3101146666318c1271e9e4b5456f514ee79 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 16 Jan 2025 10:40:14 -0600 Subject: [PATCH 2/5] Move logic of `warn_or_error` into `fire_event` Now whenever `warn_or_error` is called, it actually just passes it through to `fire_event`. That is, `warn_or_error` will now be deprecated, and shouldn't be used. As `warn_or_error` optionally takes a "node", `fire_event` had to be updated to handle this extra argument. --- dbt_common/events/event_manager.py | 25 +++++++++++++++++++++---- dbt_common/events/functions.py | 15 +++++---------- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/dbt_common/events/event_manager.py b/dbt_common/events/event_manager.py index 39e04de1..67c77951 100644 --- a/dbt_common/events/event_manager.py +++ b/dbt_common/events/event_manager.py @@ -1,9 +1,10 @@ import os import traceback -from typing import List, Optional, Protocol, Tuple +from typing import Any, List, Optional, Protocol, Tuple from dbt_common.events.base_types import BaseEvent, EventLevel, msg_from_base_event, TCallback from dbt_common.events.logger import LoggerConfig, _Logger, _TextLogger, _JsonLogger, LineFormat +from dbt_common.exceptions.events import EventCompilationError from dbt_common.helper_types import WarnErrorOptions @@ -14,9 +15,21 @@ def __init__(self) -> None: self.warn_error: bool = False self.warn_error_options: WarnErrorOptions = WarnErrorOptions(include=[], exclude=[]) - def fire_event(self, e: BaseEvent, level: Optional[EventLevel] = None) -> None: + def fire_event( + self, e: BaseEvent, level: Optional[EventLevel] = None, node: Any = None + ) -> None: msg = msg_from_base_event(e, level=level) + if msg.info.level == "warn": + event_name = type(e).__name__ + if self.warn_error or self.warn_error_options.includes(event_name): + # This has the potential to create an infinite loop if the handling of the raised + # EventCompilationError fires an event as a warning instead of an error. + raise EventCompilationError(e.message(), node) + elif self.warn_error_options.silenced(event_name): + # Return early if the event is silenced + return + if os.environ.get("DBT_TEST_BINARY_SERIALIZATION"): print(f"--- {msg.info.name}") try: @@ -54,7 +67,9 @@ class IEventManager(Protocol): warn_error: bool warn_error_options: WarnErrorOptions - def fire_event(self, e: BaseEvent, level: Optional[EventLevel] = None) -> None: + def fire_event( + self, e: BaseEvent, level: Optional[EventLevel] = None, node: Any = None + ) -> None: ... def add_logger(self, config: LoggerConfig) -> None: @@ -71,7 +86,9 @@ def __init__(self) -> None: self.event_history: List[Tuple[BaseEvent, Optional[EventLevel]]] = [] self.loggers = [] - def fire_event(self, e: BaseEvent, level: Optional[EventLevel] = None) -> None: + def fire_event( + self, e: BaseEvent, level: Optional[EventLevel] = None, node: Any = None + ) -> None: self.event_history.append((e, level)) def add_logger(self, config: LoggerConfig) -> None: diff --git a/dbt_common/events/functions.py b/dbt_common/events/functions.py index 6e88e1c2..45551083 100644 --- a/dbt_common/events/functions.py +++ b/dbt_common/events/functions.py @@ -1,7 +1,6 @@ from pathlib import Path from dbt_common.events.event_manager_client import get_event_manager -from dbt_common.exceptions import EventCompilationError from dbt_common.invocation import get_invocation_id from dbt_common.utils.encoding import ForgivingJSONEncoder from dbt_common.events.base_types import BaseEvent, EventLevel, EventMsg @@ -12,7 +11,7 @@ import json import os import sys -from typing import Callable, Dict, Optional, TextIO, Union +from typing import Any, Callable, Dict, Optional, TextIO, Union from google.protobuf.json_format import MessageToDict LOG_VERSION = 3 @@ -111,13 +110,9 @@ def msg_to_dict(msg: EventMsg) -> dict: return msg_dict +# This function continues to exist to provide backwards compatibility def warn_or_error(event, node=None) -> None: - event_name = type(event).__name__ - manager = get_event_manager() - if manager.warn_error or manager.warn_error_options.includes(event_name): - raise EventCompilationError(event.message(), node) - elif not manager.warn_error_options.silenced(event_name): - fire_event(event) + fire_event(e=event, node=node) # an alternative to fire_event which only creates and logs the event value @@ -133,8 +128,8 @@ def fire_event_if( # this is where all the side effects happen branched by event type # (i.e. - mutating the event history, printing to stdout, logging # to files, etc.) -def fire_event(e: BaseEvent, level: Optional[EventLevel] = None) -> None: - get_event_manager().fire_event(e, level=level) +def fire_event(e: BaseEvent, level: Optional[EventLevel] = None, node: Any = None) -> None: + get_event_manager().fire_event(e, level=level, node=node) def get_metadata_vars() -> Dict[str, str]: From 8f0be3a8023f2fbf09275eb4226cdfc76b86e28b Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 16 Jan 2025 12:38:09 -0600 Subject: [PATCH 3/5] Preserve prior behavior of not all `warn` events being handled by `warn_or_error` Previously only `warn` events handed directly to `warn_or_error` got the `warn_or_error` treatment. In the prior commit we moved the `warn_or_error` handling logic to `fire_event` which meant _all_ warnings were suddenly being handled by the `warn_or_error` logic. This is undesirable as it would break a lot of dbt-core projects. This commit takes it a step back by only using the `warn_or_error` logic in `fire_event` if `force_warn_or_error_handling=True` is passed to `fire_event` (by default it is `False`). Calls to `warn_or_error` will _automatically_ pass `True` for this parameter. --- dbt_common/events/event_manager.py | 14 ++++++++++--- dbt_common/events/functions.py | 13 +++++++++--- tests/unit/test_functions.py | 32 +++++++++++++++++++++++++++++- 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/dbt_common/events/event_manager.py b/dbt_common/events/event_manager.py index 67c77951..07541b57 100644 --- a/dbt_common/events/event_manager.py +++ b/dbt_common/events/event_manager.py @@ -16,11 +16,15 @@ def __init__(self) -> None: self.warn_error_options: WarnErrorOptions = WarnErrorOptions(include=[], exclude=[]) def fire_event( - self, e: BaseEvent, level: Optional[EventLevel] = None, node: Any = None + self, + e: BaseEvent, + level: Optional[EventLevel] = None, + node: Any = None, + force_warn_or_error_handling: bool = False, ) -> None: msg = msg_from_base_event(e, level=level) - if msg.info.level == "warn": + if force_warn_or_error_handling and msg.info.level == "warn": event_name = type(e).__name__ if self.warn_error or self.warn_error_options.includes(event_name): # This has the potential to create an infinite loop if the handling of the raised @@ -68,7 +72,11 @@ class IEventManager(Protocol): warn_error_options: WarnErrorOptions def fire_event( - self, e: BaseEvent, level: Optional[EventLevel] = None, node: Any = None + self, + e: BaseEvent, + level: Optional[EventLevel] = None, + node: Any = None, + force_warn_or_error_handling: bool = False, ) -> None: ... diff --git a/dbt_common/events/functions.py b/dbt_common/events/functions.py index 45551083..6641d6b8 100644 --- a/dbt_common/events/functions.py +++ b/dbt_common/events/functions.py @@ -112,7 +112,7 @@ def msg_to_dict(msg: EventMsg) -> dict: # This function continues to exist to provide backwards compatibility def warn_or_error(event, node=None) -> None: - fire_event(e=event, node=node) + fire_event(e=event, node=node, force_warn_or_error_handling=True) # an alternative to fire_event which only creates and logs the event value @@ -128,8 +128,15 @@ def fire_event_if( # this is where all the side effects happen branched by event type # (i.e. - mutating the event history, printing to stdout, logging # to files, etc.) -def fire_event(e: BaseEvent, level: Optional[EventLevel] = None, node: Any = None) -> None: - get_event_manager().fire_event(e, level=level, node=node) +def fire_event( + e: BaseEvent, + level: Optional[EventLevel] = None, + node: Any = None, + force_warn_or_error_handling: bool = False, +) -> None: + get_event_manager().fire_event( + e, level=level, node=node, force_warn_or_error_handling=force_warn_or_error_handling + ) def get_metadata_vars() -> Dict[str, str]: diff --git a/tests/unit/test_functions.py b/tests/unit/test_functions.py index 821499e2..6b8c0c53 100644 --- a/tests/unit/test_functions.py +++ b/tests/unit/test_functions.py @@ -38,7 +38,37 @@ def valid_error_names() -> Set[str]: return {Note.__name__} -class TestWarnOrError: +class TestFireEvent: + @pytest.mark.parametrize( + "force_warn_or_error_handling,should_raise", + [ + (True, True), + (False, False), + ], + ) + def test_warning_handling( + self, + set_event_manager_with_catcher: None, + force_warn_or_error_handling: bool, + should_raise: bool, + ) -> None: + get_event_manager().warn_error = True + try: + functions.fire_event( + e=Note(msg="hi"), force_warn_or_error_handling=force_warn_or_error_handling + ) + except EventCompilationError: + assert ( + should_raise + ), "`fire_event` raised an error from a warning when it shouldn't have" + return + + assert ( + not should_raise + ), "`fire_event` didn't raise an error from a warning when it should have" + + +class TestDeprecatedWarnOrError: def test_fires_error(self, valid_error_names: Set[str]) -> None: get_event_manager().warn_error_options = WarnErrorOptions( include="*", valid_error_names=valid_error_names From b89efa0fd2405b9046a94114eee0ab36f2688f20 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 16 Jan 2025 17:38:20 -0600 Subject: [PATCH 4/5] Optionally have `fire_event` handle all warnings with `warn_or_error` logic We want to move to a world where _all_ warnings are handled by the `warn_or_error` logic that now lives in `fire_event`. However we can't by default do that as that would break many dbt-core projects currently. That is, not all warnings in dbt-core are fired with `warn_or_error`. Thus if a project has `--warn-error` set, some set of warnings which weren't previously being handled by `warn_or_error` would now be handled by `warn_or_error` if `fire_event` handled all warnings with `warn_or_error`, which would cause errors that were previously happening. Thus by default _we don't_ handle all warnings in `fire_event` with `warn_or_error`. Instead a warning in `fire_event` will only be handled with `warn_or_error` logic if either 1. `force_warn_or_error_handling` is passed as True OR 2. `require_warn_or_error_handing` is set to True (1) only happens if the event was initially sent via the old `warn_or_error` function (2) will only happen if a corresponding project flag set in dbt-core --- dbt_common/events/event_manager.py | 6 +++++- tests/unit/test_functions.py | 13 +++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/dbt_common/events/event_manager.py b/dbt_common/events/event_manager.py index 07541b57..41c6599c 100644 --- a/dbt_common/events/event_manager.py +++ b/dbt_common/events/event_manager.py @@ -14,6 +14,7 @@ def __init__(self) -> None: self.callbacks: List[TCallback] = [] self.warn_error: bool = False self.warn_error_options: WarnErrorOptions = WarnErrorOptions(include=[], exclude=[]) + self.require_warn_or_error_handling: bool = False def fire_event( self, @@ -24,7 +25,9 @@ def fire_event( ) -> None: msg = msg_from_base_event(e, level=level) - if force_warn_or_error_handling and msg.info.level == "warn": + if ( + force_warn_or_error_handling or self.require_warn_or_error_handling + ) and msg.info.level == "warn": event_name = type(e).__name__ if self.warn_error or self.warn_error_options.includes(event_name): # This has the potential to create an infinite loop if the handling of the raised @@ -70,6 +73,7 @@ class IEventManager(Protocol): loggers: List[_Logger] warn_error: bool warn_error_options: WarnErrorOptions + require_warn_or_error_handling: bool def fire_event( self, diff --git a/tests/unit/test_functions.py b/tests/unit/test_functions.py index 6b8c0c53..63566b74 100644 --- a/tests/unit/test_functions.py +++ b/tests/unit/test_functions.py @@ -40,19 +40,24 @@ def valid_error_names() -> Set[str]: class TestFireEvent: @pytest.mark.parametrize( - "force_warn_or_error_handling,should_raise", + "force_warn_or_error_handling,require_warn_or_error_handling,should_raise", [ - (True, True), - (False, False), + (True, True, True), + (True, False, True), + (False, True, True), + (False, False, False), ], ) def test_warning_handling( self, set_event_manager_with_catcher: None, force_warn_or_error_handling: bool, + require_warn_or_error_handling: bool, should_raise: bool, ) -> None: - get_event_manager().warn_error = True + manager = get_event_manager() + manager.warn_error = True + manager.require_warn_or_error_handling = require_warn_or_error_handling try: functions.fire_event( e=Note(msg="hi"), force_warn_or_error_handling=force_warn_or_error_handling From 3007a6c26075eb9086c67de34298678e2e57f470 Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Fri, 17 Jan 2025 14:41:20 -0600 Subject: [PATCH 5/5] Add changelog for 'fire_event' handling 'warn_or_error' logic --- .changes/unreleased/Breaking Changes-20250117-144053.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/unreleased/Breaking Changes-20250117-144053.yaml diff --git a/.changes/unreleased/Breaking Changes-20250117-144053.yaml b/.changes/unreleased/Breaking Changes-20250117-144053.yaml new file mode 100644 index 00000000..460e7f38 --- /dev/null +++ b/.changes/unreleased/Breaking Changes-20250117-144053.yaml @@ -0,0 +1,6 @@ +kind: Breaking Changes +body: Update `fire_event` to handle `warn_or_error` logic +time: 2025-01-17T14:40:53.08567-06:00 +custom: + Author: QMalcolm + Issue: "236"