From 42d7630e83203280a0c1c5827113fd381d3dbf57 Mon Sep 17 00:00:00 2001 From: Pedro Crespo-Valero <32402063+pcrespov@users.noreply.github.com> Date: Fri, 19 Jan 2024 18:56:46 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9BFixes=20`TypeError:=20issubclass()?= =?UTF-8?q?=20arg=201=20must=20be=20a=20class`=20while=20building=20BaseCu?= =?UTF-8?q?stomSettings=20classes=20(#5253)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/pytest_simcore/environment_configs.py | 15 +++- .../src/pytest_simcore/helpers/typing_env.py | 3 +- .../src/pytest_simcore/helpers/utils_envs.py | 52 ++++++++------ .../src/settings_library/base.py | 26 +++++-- packages/settings-library/tests/conftest.py | 9 +-- packages/settings-library/tests/test_base.py | 71 ++++++++++++++----- packages/settings-library/tests/test_email.py | 31 +++++--- 7 files changed, 143 insertions(+), 64 deletions(-) diff --git a/packages/pytest-simcore/src/pytest_simcore/environment_configs.py b/packages/pytest-simcore/src/pytest_simcore/environment_configs.py index 0fd2cddf4a4..9303ea382da 100644 --- a/packages/pytest-simcore/src/pytest_simcore/environment_configs.py +++ b/packages/pytest-simcore/src/pytest_simcore/environment_configs.py @@ -8,7 +8,7 @@ import pytest from .helpers.typing_env import EnvVarsDict -from .helpers.utils_envs import load_dotenv, setenvs_from_dict +from .helpers.utils_envs import delenvs_from_dict, load_dotenv, setenvs_from_dict @pytest.fixture(scope="session") @@ -23,3 +23,16 @@ def mock_env_devel_environment( env_devel_dict: EnvVarsDict, monkeypatch: pytest.MonkeyPatch ) -> EnvVarsDict: return setenvs_from_dict(monkeypatch, env_devel_dict) + + +@pytest.fixture +def all_env_devel_undefined( + monkeypatch: pytest.MonkeyPatch, env_devel_dict: EnvVarsDict +): + """Ensures that all env vars in .env-devel are undefined in the test environment + + NOTE: this is useful to have a clean starting point and avoid + the environment to influence your test. I found this situation + when some script was accidentaly injecting the entire .env-devel in the environment + """ + delenvs_from_dict(monkeypatch, env_devel_dict, raising=False) diff --git a/packages/pytest-simcore/src/pytest_simcore/helpers/typing_env.py b/packages/pytest-simcore/src/pytest_simcore/helpers/typing_env.py index 142dca2f5a2..fd9dd81f778 100644 --- a/packages/pytest-simcore/src/pytest_simcore/helpers/typing_env.py +++ b/packages/pytest-simcore/src/pytest_simcore/helpers/typing_env.py @@ -1,7 +1,8 @@ +from collections.abc import Iterable from typing import TypeAlias EnvVarsDict: TypeAlias = dict[str, str] -EnvVarsList: TypeAlias = list[str] +EnvVarsIterable: TypeAlias = Iterable[str] # SEE packages/pytest-simcore/tests/test_helpers_utils_envs.py diff --git a/packages/pytest-simcore/src/pytest_simcore/helpers/utils_envs.py b/packages/pytest-simcore/src/pytest_simcore/helpers/utils_envs.py index 630dbcdbc92..25eff962f73 100644 --- a/packages/pytest-simcore/src/pytest_simcore/helpers/utils_envs.py +++ b/packages/pytest-simcore/src/pytest_simcore/helpers/utils_envs.py @@ -3,44 +3,42 @@ """ import os -from copy import deepcopy from io import StringIO from pathlib import Path import dotenv import pytest -from .typing_env import EnvVarsDict, EnvVarsList +from .typing_env import EnvVarsDict, EnvVarsIterable # # monkeypatch using dict # -def delenvs_from_dict(monkeypatch: pytest.MonkeyPatch, envs: EnvVarsList): - for var in envs: - assert isinstance(var, str) - assert var is not None # None keys cannot be is defined w/o value - monkeypatch.delenv(var) - - def setenvs_from_dict( - monkeypatch: pytest.MonkeyPatch, envs: EnvVarsDict + monkeypatch: pytest.MonkeyPatch, envs: dict[str, str | bool] ) -> EnvVarsDict: + env_vars = {} + for key, value in envs.items(): assert isinstance(key, str) - assert ( - value is not None - ), f"{key=},{value=}" # None keys cannot be is defined w/o value - converted_value = value + assert value is not None, f"{key=},{value=}" + + v = value + if isinstance(value, bool): - converted_value = f"{'true' if value else 'false'}" - assert isinstance( - converted_value, str - ), f"client MUST explicitly stringify values since some cannot be done automatically e.g. json-like values. problematic {key=},{value=}" + v = "true" if value else "false" + + assert isinstance(v, str), ( + "caller MUST explicitly stringify values since some cannot be done automatically" + f"e.g. json-like values. Check {key=},{value=}" + ) - monkeypatch.setenv(key, converted_value) - return deepcopy(envs) + monkeypatch.setenv(key, v) + env_vars[key] = v + + return env_vars def load_dotenv(envfile_content_or_path: Path | str, **options) -> EnvVarsDict: @@ -57,6 +55,17 @@ def load_dotenv(envfile_content_or_path: Path | str, **options) -> EnvVarsDict: return {k: v or "" for k, v in dotenv.dotenv_values(**kwargs).items()} +def delenvs_from_dict( + monkeypatch: pytest.MonkeyPatch, + envs: EnvVarsIterable, + *, + raising: bool = True, +) -> None: + for key in envs: + assert isinstance(key, str) + monkeypatch.delenv(key, raising) + + # # monkeypath using envfiles ('.env' and also denoted as dotfiles) # @@ -76,7 +85,8 @@ def setenvs_from_envfile( def delenvs_from_envfile( monkeypatch: pytest.MonkeyPatch, content_or_path: str | Path, - raising: bool, + *, + raising: bool = True, **dotenv_kwags, ) -> EnvVarsDict: """Batch monkeypatch.delenv(...) on all env vars in an envfile""" diff --git a/packages/settings-library/src/settings_library/base.py b/packages/settings-library/src/settings_library/base.py index 577e20adbe8..b40f658c944 100644 --- a/packages/settings-library/src/settings_library/base.py +++ b/packages/settings-library/src/settings_library/base.py @@ -1,9 +1,16 @@ import logging from collections.abc import Sequence from functools import cached_property -from typing import Final, get_args - -from pydantic import BaseConfig, BaseSettings, Extra, ValidationError, validator +from typing import Final, get_args, get_origin + +from pydantic import ( + BaseConfig, + BaseSettings, + ConfigError, + Extra, + ValidationError, + validator, +) from pydantic.error_wrappers import ErrorList, ErrorWrapper from pydantic.fields import ModelField, Undefined @@ -90,7 +97,12 @@ def prepare_field(cls, field: ModelField) -> None: if args := get_args(field_type): field_type = next(a for a in args if a != type(None)) - if issubclass(field_type, BaseCustomSettings): + # Avoids issubclass raising TypeError. SEE test_issubclass_type_error_with_pydantic_models + is_not_composed = ( + get_origin(field_type) is None + ) # is not composed as dict[str, Any] or Generic[Base] + + if is_not_composed and issubclass(field_type, BaseCustomSettings): if auto_default_from_env: assert field.field_info.default is Undefined assert field.field_info.default_factory is None @@ -100,13 +112,13 @@ def prepare_field(cls, field: ModelField) -> None: field.default = None field.required = False # has a default now - elif issubclass(field_type, BaseSettings): + elif is_not_composed and issubclass(field_type, BaseSettings): msg = f"{cls}.{field.name} of type {field_type} must inherit from BaseCustomSettings" - raise ValueError(msg) + raise ConfigError(msg) elif auto_default_from_env: msg = f"auto_default_from_env=True can only be used in BaseCustomSettings subclassesbut field {cls}.{field.name} is {field_type} " - raise ValueError(msg) + raise ConfigError(msg) @classmethod def create_from_envs(cls, **overrides): diff --git a/packages/settings-library/tests/conftest.py b/packages/settings-library/tests/conftest.py index d6c9e524f89..725f19c534a 100644 --- a/packages/settings-library/tests/conftest.py +++ b/packages/settings-library/tests/conftest.py @@ -4,7 +4,6 @@ import sys from pathlib import Path -from typing import Optional import pytest import settings_library @@ -97,13 +96,9 @@ class _ApplicationSettings(BaseCustomSettings): # NOTE: by convention, an addon is disabled when APP_ADDON=None, so we make this # entry nullable as well - APP_OPTIONAL_ADDON: Optional[_ModuleSettings] = Field( - auto_default_from_env=True - ) + APP_OPTIONAL_ADDON: _ModuleSettings | None = Field(auto_default_from_env=True) # NOTE: example of a group that cannot be disabled (not nullable) - APP_REQUIRED_PLUGIN: Optional[PostgresSettings] = Field( - auto_default_from_env=True - ) + APP_REQUIRED_PLUGIN: PostgresSettings | None = Field(auto_default_from_env=True) return _ApplicationSettings diff --git a/packages/settings-library/tests/test_base.py b/packages/settings-library/tests/test_base.py index 164cc0e2c9a..8323087f3f5 100644 --- a/packages/settings-library/tests/test_base.py +++ b/packages/settings-library/tests/test_base.py @@ -3,12 +3,13 @@ # pylint: disable=unused-variable # pylint: disable=too-many-arguments +import inspect import json -from typing import Any, Callable, Optional +from typing import Any, Callable import pytest import settings_library.base -from pydantic import BaseModel, ValidationError +from pydantic import BaseModel, BaseSettings, ValidationError from pydantic.fields import Field from pytest import MonkeyPatch from pytest_mock import MockerFixture @@ -62,13 +63,13 @@ class M1(BaseCustomSettings): VALUE_DEFAULT: S = S(S_VALUE=42) VALUE_CONFUSING: S = None # type: ignore - VALUE_NULLABLE_REQUIRED: Optional[S] = ... # type: ignore - VALUE_NULLABLE_OPTIONAL: Optional[S] + VALUE_NULLABLE_REQUIRED: S | None = ... # type: ignore + VALUE_NULLABLE_OPTIONAL: S | None - VALUE_NULLABLE_DEFAULT_VALUE: Optional[S] = S(S_VALUE=42) - VALUE_NULLABLE_DEFAULT_NULL: Optional[S] = None + VALUE_NULLABLE_DEFAULT_VALUE: S | None = S(S_VALUE=42) + VALUE_NULLABLE_DEFAULT_NULL: S | None = None - VALUE_NULLABLE_DEFAULT_ENV: Optional[S] = Field(auto_default_from_env=True) + VALUE_NULLABLE_DEFAULT_ENV: S | None = Field(auto_default_from_env=True) VALUE_DEFAULT_ENV: S = Field(auto_default_from_env=True) class M2(BaseCustomSettings): @@ -78,10 +79,10 @@ class M2(BaseCustomSettings): # # defaults disabled but only explicit enabled - VALUE_NULLABLE_DEFAULT_NULL: Optional[S] = None + VALUE_NULLABLE_DEFAULT_NULL: S | None = None # defaults enabled but if not exists, it disables - VALUE_NULLABLE_DEFAULT_ENV: Optional[S] = Field(auto_default_from_env=True) + VALUE_NULLABLE_DEFAULT_ENV: S | None = Field(auto_default_from_env=True) # cannot be disabled VALUE_DEFAULT_ENV: S = Field(auto_default_from_env=True) @@ -200,8 +201,8 @@ def test_auto_default_to_none_logs_a_warning( S = create_settings_class("S") class SettingsClass(BaseCustomSettings): - VALUE_NULLABLE_DEFAULT_NULL: Optional[S] = None - VALUE_NULLABLE_DEFAULT_ENV: Optional[S] = Field(auto_default_from_env=True) + VALUE_NULLABLE_DEFAULT_NULL: S | None = None + VALUE_NULLABLE_DEFAULT_ENV: S | None = Field(auto_default_from_env=True) instance = SettingsClass.create_from_envs() assert instance.VALUE_NULLABLE_DEFAULT_NULL == None @@ -222,8 +223,8 @@ def test_auto_default_to_not_none( S = create_settings_class("S") class SettingsClass(BaseCustomSettings): - VALUE_NULLABLE_DEFAULT_NULL: Optional[S] = None - VALUE_NULLABLE_DEFAULT_ENV: Optional[S] = Field(auto_default_from_env=True) + VALUE_NULLABLE_DEFAULT_NULL: S | None = None + VALUE_NULLABLE_DEFAULT_ENV: S | None = Field(auto_default_from_env=True) instance = SettingsClass.create_from_envs() assert instance.VALUE_NULLABLE_DEFAULT_NULL == None @@ -260,12 +261,12 @@ def test_how_settings_parse_null_environs(monkeypatch: MonkeyPatch): } class SettingsClass(BaseCustomSettings): - VALUE_TO_NOTHING: Optional[str] - VALUE_TO_WORD_NULL: Optional[str] - VALUE_TO_WORD_NONE: Optional[str] - VALUE_TO_ZERO: Optional[str] + VALUE_TO_NOTHING: str | None + VALUE_TO_WORD_NULL: str | None + VALUE_TO_WORD_NONE: str | None + VALUE_TO_ZERO: str | None - INT_VALUE_TO_ZERO: Optional[int] + INT_VALUE_TO_ZERO: int | None instance = SettingsClass.create_from_envs() @@ -278,7 +279,7 @@ class SettingsClass(BaseCustomSettings): ) class SettingsClassExt(SettingsClass): - INT_VALUE_TO_NOTHING: Optional[int] + INT_VALUE_TO_NOTHING: int | None with pytest.raises(ValidationError) as err_info: SettingsClassExt.create_from_envs() @@ -289,3 +290,35 @@ class SettingsClassExt(SettingsClass): "msg": "value is not a valid integer", "type": "type_error.integer", } + + +def test_issubclass_type_error_with_pydantic_models(): + # There is a problem + # + # TypeError: issubclass() arg 1 must be a class + # + # SEE https://github.com/pydantic/pydantic/issues/545 + # + # >> issubclass(dict, BaseSettings) + # False + # >> issubclass(dict[str, str], BaseSettings) + # Traceback (most recent call last): + # File "", line 1, in + # File "/home/crespo/.pyenv/versions/3.10.13/lib/python3.10/abc.py", line 123, in __subclasscheck__ + # return _abc_subclasscheck(cls, subclass) + # TypeError: issubclass() arg 1 must be a class + # + + assert inspect.isclass(dict[str, str]) + assert not issubclass(dict, BaseSettings) + + # NOTE: this should be fixed by pydantic at some point. When this happens, this test will fail + with pytest.raises(TypeError): + issubclass(dict[str, str], BaseSettings) + + # here reproduces the problem with our settings that ANE and PC had + class SettingsClassThatFailed(BaseCustomSettings): + FOO: dict[str, str] | None = Field(default=None) + + SettingsClassThatFailed(FOO={}) + assert SettingsClassThatFailed(FOO=None) == SettingsClassThatFailed() diff --git a/packages/settings-library/tests/test_email.py b/packages/settings-library/tests/test_email.py index c12d035e517..b24b5d3c256 100644 --- a/packages/settings-library/tests/test_email.py +++ b/packages/settings-library/tests/test_email.py @@ -1,3 +1,8 @@ +# pylint: disable=redefined-outer-name +# pylint: disable=unused-argument +# pylint: disable=unused-variable +# pylint: disable=too-many-arguments + from typing import Any import pytest @@ -46,15 +51,25 @@ }, ], ) -def test_smtp_configuration_ok(cfg: dict[str, Any]): +def test_smtp_configuration_ok(cfg: dict[str, Any], all_env_devel_undefined: None): assert SMTPSettings.parse_obj(cfg) @pytest.mark.parametrize( "cfg", [ - {"SMTP_HOST": "test", "SMTP_PORT": 113, "SMTP_USERNAME": "test"}, - {"SMTP_HOST": "test", "SMTP_PORT": 113, "SMTP_PASSWORD": "test"}, + { + "SMTP_HOST": "test", + "SMTP_PORT": 111, + "SMTP_USERNAME": "test", + # password required if username provided + }, + { + "SMTP_HOST": "test", + "SMTP_PORT": 112, + "SMTP_PASSWORD": "test", + # username required if password provided + }, { "SMTP_HOST": "test", "SMTP_PORT": 113, @@ -63,26 +78,26 @@ def test_smtp_configuration_ok(cfg: dict[str, Any]): }, { "SMTP_HOST": "test", - "SMTP_PORT": 113, + "SMTP_PORT": 114, "SMTP_PROTOCOL": EmailProtocol.STARTTLS, "SMTP_USERNAME": "test", }, { "SMTP_HOST": "test", - "SMTP_PORT": 113, + "SMTP_PORT": 115, "SMTP_USERNAME": "", "SMTP_PASSWORD": "test", "SMTP_PROTOCOL": EmailProtocol.STARTTLS, }, { "SMTP_HOST": "test", - "SMTP_PORT": 113, + "SMTP_PORT": 116, "SMTP_USERNAME": "", "SMTP_PASSWORD": "test", "SMTP_PROTOCOL": EmailProtocol.TLS, }, ], ) -def test_smtp_configuration_fails(cfg: dict[str, Any]): +def test_smtp_configuration_fails(cfg: dict[str, Any], all_env_devel_undefined: None): with pytest.raises(ValidationError): - assert SMTPSettings.parse_obj(cfg) + SMTPSettings(**cfg)