From 5108a4cf2e5ab1be27370d63f0ca87aa472fe5e1 Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Tue, 16 Jul 2024 16:17:41 -0400 Subject: [PATCH 1/6] Fix exclusive_primary_alt_value_setting to set warn_error_options correctly --- core/dbt/config/utils.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/core/dbt/config/utils.py b/core/dbt/config/utils.py index 6f7f1266309..c37854c477b 100644 --- a/core/dbt/config/utils.py +++ b/core/dbt/config/utils.py @@ -49,5 +49,8 @@ def exclusive_primary_alt_value_setting( f"Only `{alt}` or `{primary}` can be specified{where}, not both" ) - if alt_options: - dictionary[primary] = alt_options + if alt in dictionary: + alt_value = dictionary.pop(alt) + if alt_value is None: + alt_value = [] + dictionary[primary] = alt_value From 254b06f1b88dbae8062604e0aac27d04a9e3d47b Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Tue, 16 Jul 2024 17:11:53 -0400 Subject: [PATCH 2/6] Add test --- .../configs/test_warn_error_options.py | 29 ++++++++++++++++++- 1 file changed, 28 insertions(+), 1 deletion(-) diff --git a/tests/functional/configs/test_warn_error_options.py b/tests/functional/configs/test_warn_error_options.py index 1808d8f26f0..c35ad5a9881 100644 --- a/tests/functional/configs/test_warn_error_options.py +++ b/tests/functional/configs/test_warn_error_options.py @@ -4,7 +4,8 @@ from dbt.cli.main import dbtRunner, dbtRunnerResult from dbt.events.types import DeprecatedModel -from dbt.tests.util import update_config_file +from dbt.flags import get_flags +from dbt.tests.util import run_dbt, update_config_file from dbt_common.events.base_types import EventLevel from tests.utils import EventCatcher @@ -202,3 +203,29 @@ def test_cant_set_both_exclude_and_warn( assert not result.success assert result.exception is not None assert "Only `warn` or `exclude` can be specified" in str(result.exception) + + +class TestEmptyWarnError: + @pytest.fixture(scope="class") + def models(self): + return {"my_model.sql": my_model_sql, "schema.yml": schema_yml} + + # This tests for a bug in creating WarnErrorOptions when warn or + # error are set to None (in yaml = warn:) + def test_project_flags(self, project): + project_flags = { + "flags": { + "send_anonymous_usage_stats": False, + "warn_error_options": { + "warn": None, + "error": None, + "silence": ["TestsConfigDeprecation"], + }, + } + } + update_config_file(project_flags, project.project_root, "dbt_project.yml") + run_dbt(["run"]) + flags = get_flags() + # Note: WarnErrorOptions is not a dataclass, so you won't get "silence" + # from to_dict or stringifying. + assert flags.warn_error_options.silence == ["TestsConfigDeprecation"] From b47ac8e1d36c959eb820911e47c189077a858b5c Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Tue, 16 Jul 2024 17:14:33 -0400 Subject: [PATCH 3/6] Changie --- .changes/unreleased/Fixes-20240716-171427.yaml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changes/unreleased/Fixes-20240716-171427.yaml diff --git a/.changes/unreleased/Fixes-20240716-171427.yaml b/.changes/unreleased/Fixes-20240716-171427.yaml new file mode 100644 index 00000000000..5d8db74a448 --- /dev/null +++ b/.changes/unreleased/Fixes-20240716-171427.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: Fix error constructing warn_error_options +time: 2024-07-16T17:14:27.837171-04:00 +custom: + Author: gshank + Issue: "10452" From 89c01dab0915eb13c3a30fcd25cf9ef933666419 Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Tue, 16 Jul 2024 18:41:27 -0400 Subject: [PATCH 4/6] Fix unit test --- tests/unit/config/test_utils.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/unit/config/test_utils.py b/tests/unit/config/test_utils.py index d2d7f99499d..41e7dfddd43 100644 --- a/tests/unit/config/test_utils.py +++ b/tests/unit/config/test_utils.py @@ -27,7 +27,6 @@ def test_alt_set(self, primary_key: str, alt_key: str, value: str): test_dict = {alt_key: value} exclusive_primary_alt_value_setting(test_dict, primary_key, alt_key) assert test_dict.get(primary_key) == value - assert test_dict.get(alt_key) == value def test_primary_and_alt_set(self, primary_key: str, alt_key: str, value: str): test_dict = {primary_key: value, alt_key: value} From 7310b79088c12282ba86f91a3a2ca353573fedf0 Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Wed, 17 Jul 2024 09:31:30 -0400 Subject: [PATCH 5/6] Replace conversion method --- core/dbt/cli/option_types.py | 9 ++--- core/dbt/config/project.py | 11 ++---- core/dbt/config/utils.py | 49 ++++++++++++++------------- tests/unit/config/test_utils.py | 59 +++++++++++++++------------------ 4 files changed, 58 insertions(+), 70 deletions(-) diff --git a/core/dbt/cli/option_types.py b/core/dbt/cli/option_types.py index 6c94116eb67..2d0efedbb9e 100644 --- a/core/dbt/cli/option_types.py +++ b/core/dbt/cli/option_types.py @@ -1,6 +1,6 @@ from click import Choice, ParamType -from dbt.config.utils import exclusive_primary_alt_value_setting, parse_cli_yaml_string +from dbt.config.utils import normalize_warn_error_options, parse_cli_yaml_string from dbt.events import ALL_EVENT_NAMES from dbt.exceptions import OptionNotYamlDictError, ValidationError from dbt_common.exceptions import DbtValidationError @@ -51,12 +51,7 @@ class WarnErrorOptionsType(YAML): def convert(self, value, param, ctx): # this function is being used by param in click include_exclude = super().convert(value, param, ctx) - exclusive_primary_alt_value_setting( - include_exclude, "include", "error", "warn_error_options" - ) - exclusive_primary_alt_value_setting( - include_exclude, "exclude", "warn", "warn_error_options" - ) + normalize_warn_error_options(include_exclude) return WarnErrorOptions( include=include_exclude.get("include", []), diff --git a/core/dbt/config/project.py b/core/dbt/config/project.py index 7f396477fd6..e1dfac4e10c 100644 --- a/core/dbt/config/project.py +++ b/core/dbt/config/project.py @@ -10,7 +10,7 @@ from dbt.adapters.contracts.connection import QueryComment from dbt.clients.yaml_helper import load_yaml_text from dbt.config.selectors import SelectorDict -from dbt.config.utils import exclusive_primary_alt_value_setting +from dbt.config.utils import normalize_warn_error_options from dbt.constants import ( DBT_PROJECT_FILE_NAME, DEPENDENCIES_FILE_NAME, @@ -828,13 +828,8 @@ def read_project_flags(project_dir: str, profiles_dir: str) -> ProjectFlags: if project_flags is not None: # handle collapsing `include` and `error` as well as collapsing `exclude` and `warn` # for warn_error_options - warn_error_options = project_flags.get("warn_error_options") - exclusive_primary_alt_value_setting( - warn_error_options, "include", "error", "warn_error_options" - ) - exclusive_primary_alt_value_setting( - warn_error_options, "exclude", "warn", "warn_error_options" - ) + warn_error_options = project_flags.get("warn_error_options", {}) + normalize_warn_error_options(warn_error_options) ProjectFlags.validate(project_flags) return ProjectFlags.from_dict(project_flags) diff --git a/core/dbt/config/utils.py b/core/dbt/config/utils.py index c37854c477b..df554f1c461 100644 --- a/core/dbt/config/utils.py +++ b/core/dbt/config/utils.py @@ -1,4 +1,4 @@ -from typing import Any, Dict, Optional +from typing import Any, Dict from dbt.clients import yaml_helper from dbt.events.types import InvalidOptionYAML @@ -24,33 +24,36 @@ def parse_cli_yaml_string(var_string: str, cli_option_name: str) -> Dict[str, An raise -def exclusive_primary_alt_value_setting( - dictionary: Optional[Dict[str, Any]], - primary: str, - alt: str, - parent_config: Optional[str] = None, +def normalize_warn_error_options( + dictionary: Dict[str, Any], ) -> None: - """Munges in place under the primary the options for the primary and alt values + """Fixes fields for warn_error_options from yaml format to fields + expected by the WarnErrorOptions class. + 'error' => 'include', 'warn' => 'exclude' - Sometimes we allow setting something via TWO keys, but not at the same time. If both the primary - key and alt key have values, an error gets raised. If the alt key has values, then we update - the dictionary to ensure the primary key contains the values. If neither are set, nothing happens. + Also validates that two different forms of accepted keys are not + both provided. """ - if dictionary is None: - return - - primary_options = dictionary.get(primary) - alt_options = dictionary.get(alt) + if "include" in dictionary and "error" in dictionary: + raise DbtExclusivePropertyUseError( + "Only `error` or `include` can be specified in `warn_error_options`, not both" + ) - if primary_options and alt_options: - where = f" in `{parent_config}`" if parent_config is not None else "" + if "warn" in dictionary and "exclude" in dictionary: raise DbtExclusivePropertyUseError( - f"Only `{alt}` or `{primary}` can be specified{where}, not both" + "Only `warn` or `exclude` can be specified in `warn_error_options`, not both" ) - if alt in dictionary: - alt_value = dictionary.pop(alt) - if alt_value is None: - alt_value = [] - dictionary[primary] = alt_value + convert = { + "error": "include", + "warn": "exclude", + } + for key in list(convert.keys()): + if key in dictionary: + value = dictionary.pop(key) + if value is None: + value = [] + dictionary[convert[key]] = value + if "silence" in dictionary and dictionary["silence"] is None: + dictionary["silence"] = [] diff --git a/tests/unit/config/test_utils.py b/tests/unit/config/test_utils.py index 41e7dfddd43..0f1dfe36d0c 100644 --- a/tests/unit/config/test_utils.py +++ b/tests/unit/config/test_utils.py @@ -1,40 +1,35 @@ import pytest -from dbt.config.utils import exclusive_primary_alt_value_setting +from dbt.config.utils import normalize_warn_error_options from dbt.exceptions import DbtExclusivePropertyUseError -class TestExclusivePrimaryAltValueSetting: - @pytest.fixture(scope="class") - def primary_key(self) -> str: - return "key_a" - - @pytest.fixture(scope="class") - def alt_key(self) -> str: - return "key_b" - - @pytest.fixture(scope="class") - def value(self) -> str: - return "I LIKE CATS" - - def test_primary_set(self, primary_key: str, alt_key: str, value: str): - test_dict = {primary_key: value} - exclusive_primary_alt_value_setting(test_dict, primary_key, alt_key) - assert test_dict.get(primary_key) == value - assert test_dict.get(alt_key) is None - - def test_alt_set(self, primary_key: str, alt_key: str, value: str): - test_dict = {alt_key: value} - exclusive_primary_alt_value_setting(test_dict, primary_key, alt_key) - assert test_dict.get(primary_key) == value - - def test_primary_and_alt_set(self, primary_key: str, alt_key: str, value: str): - test_dict = {primary_key: value, alt_key: value} +class TestNormalizeWarnErrorOptions: + def test_primary_set(self): + test_dict = { + "error": ["SomeWarning"], + } + normalize_warn_error_options(test_dict) + assert len(test_dict) == 1 + assert test_dict["include"] == ["SomeWarning"] + + def test_convert(self): + test_dict = {"warn": None, "silence": None, "include": ["SomeWarning"]} + normalize_warn_error_options(test_dict) + assert test_dict["exclude"] == [] + assert test_dict["include"] == ["SomeWarning"] + assert test_dict["silence"] == [] + + def test_both_keys_set(self): + test_dict = { + "warn": ["SomeWarning"], + "exclude": ["SomeWarning"], + } with pytest.raises(DbtExclusivePropertyUseError): - exclusive_primary_alt_value_setting(test_dict, primary_key, alt_key) + normalize_warn_error_options(test_dict) - def test_neither_primary_nor_alt_set(self, primary_key: str, alt_key: str): + def test_empty_dict(self): test_dict = {} - exclusive_primary_alt_value_setting(test_dict, primary_key, alt_key) - assert test_dict.get(primary_key) is None - assert test_dict.get(alt_key) is None + normalize_warn_error_options(test_dict) + assert test_dict.get("include") is None + assert test_dict.get("exclude") is None From 0baf0c8dd2da349a890d1194cd97a011d82d0313 Mon Sep 17 00:00:00 2001 From: Gerda Shank Date: Wed, 17 Jul 2024 14:53:36 -0400 Subject: [PATCH 6/6] Refactor normalize_warn_error_options --- core/dbt/config/utils.py | 59 ++++++++++++++++++--------------- tests/unit/config/test_utils.py | 41 ++++++++++++++++++++++- 2 files changed, 73 insertions(+), 27 deletions(-) diff --git a/core/dbt/config/utils.py b/core/dbt/config/utils.py index df554f1c461..c8bfc930b9b 100644 --- a/core/dbt/config/utils.py +++ b/core/dbt/config/utils.py @@ -1,4 +1,4 @@ -from typing import Any, Dict +from typing import Any, Dict, Optional from dbt.clients import yaml_helper from dbt.events.types import InvalidOptionYAML @@ -24,36 +24,43 @@ def parse_cli_yaml_string(var_string: str, cli_option_name: str) -> Dict[str, An raise -def normalize_warn_error_options( - dictionary: Dict[str, Any], +def exclusive_primary_alt_value_setting( + dictionary: Optional[Dict[str, Any]], + primary: str, + alt: str, + parent_config: Optional[str] = None, ) -> None: - """Fixes fields for warn_error_options from yaml format to fields - expected by the WarnErrorOptions class. - 'error' => 'include', 'warn' => 'exclude' + """Munges in place under the primary the options for the primary and alt values - Also validates that two different forms of accepted keys are not - both provided. + Sometimes we allow setting something via TWO keys, but not at the same time. If both the primary + key and alt key have values, an error gets raised. If the alt key has values, then we update + the dictionary to ensure the primary key contains the values. If neither are set, nothing happens. """ - if "include" in dictionary and "error" in dictionary: - raise DbtExclusivePropertyUseError( - "Only `error` or `include` can be specified in `warn_error_options`, not both" - ) + if dictionary is None: + return + + primary_options = dictionary.get(primary) + alt_options = dictionary.get(alt) - if "warn" in dictionary and "exclude" in dictionary: + if primary_options and alt_options: + where = f" in `{parent_config}`" if parent_config is not None else "" raise DbtExclusivePropertyUseError( - "Only `warn` or `exclude` can be specified in `warn_error_options`, not both" + f"Only `{alt}` or `{primary}` can be specified{where}, not both" ) - convert = { - "error": "include", - "warn": "exclude", - } - for key in list(convert.keys()): - if key in dictionary: - value = dictionary.pop(key) - if value is None: - value = [] - dictionary[convert[key]] = value - if "silence" in dictionary and dictionary["silence"] is None: - dictionary["silence"] = [] + if alt in dictionary: + alt_value = dictionary.pop(alt) + dictionary[primary] = alt_value + + +def normalize_warn_error_options(warn_error_options: Dict[str, Any]) -> None: + exclusive_primary_alt_value_setting( + warn_error_options, "include", "error", "warn_error_options" + ) + exclusive_primary_alt_value_setting( + warn_error_options, "exclude", "warn", "warn_error_options" + ) + for key in ("include", "exclude", "silence"): + if key in warn_error_options and warn_error_options[key] is None: + warn_error_options[key] = [] diff --git a/tests/unit/config/test_utils.py b/tests/unit/config/test_utils.py index 0f1dfe36d0c..88f73ad2fdb 100644 --- a/tests/unit/config/test_utils.py +++ b/tests/unit/config/test_utils.py @@ -1,9 +1,48 @@ import pytest -from dbt.config.utils import normalize_warn_error_options +from dbt.config.utils import ( + exclusive_primary_alt_value_setting, + normalize_warn_error_options, +) from dbt.exceptions import DbtExclusivePropertyUseError +class TestExclusivePrimaryAltValueSetting: + @pytest.fixture(scope="class") + def primary_key(self) -> str: + return "key_a" + + @pytest.fixture(scope="class") + def alt_key(self) -> str: + return "key_b" + + @pytest.fixture(scope="class") + def value(self) -> str: + return "I LIKE CATS" + + def test_primary_set(self, primary_key: str, alt_key: str, value: str): + test_dict = {primary_key: value} + exclusive_primary_alt_value_setting(test_dict, primary_key, alt_key) + assert test_dict.get(primary_key) == value + assert test_dict.get(alt_key) is None + + def test_alt_set(self, primary_key: str, alt_key: str, value: str): + test_dict = {alt_key: value} + exclusive_primary_alt_value_setting(test_dict, primary_key, alt_key) + assert test_dict.get(primary_key) == value + + def test_primary_and_alt_set(self, primary_key: str, alt_key: str, value: str): + test_dict = {primary_key: value, alt_key: value} + with pytest.raises(DbtExclusivePropertyUseError): + exclusive_primary_alt_value_setting(test_dict, primary_key, alt_key) + + def test_neither_primary_nor_alt_set(self, primary_key: str, alt_key: str): + test_dict = {} + exclusive_primary_alt_value_setting(test_dict, primary_key, alt_key) + assert test_dict.get(primary_key) is None + assert test_dict.get(alt_key) is None + + class TestNormalizeWarnErrorOptions: def test_primary_set(self): test_dict = {