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

Use SettingsErrors when file can't be used as source #432

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
10 changes: 9 additions & 1 deletion pydantic_settings/sources.py
Original file line number Diff line number Diff line change
Expand Up @@ -1917,7 +1917,15 @@ def _read_files(self, files: PathType | None) -> dict[str, Any]:
for file in files:
file_path = Path(file).expanduser()
if file_path.is_file():
vars.update(self._read_file(file_path))
try:
settings = self._read_file(file_path)
except ValueError as e:
raise SettingsError(f'Failed to parse settings from {file_path}, {e}')
if not isinstance(settings, dict):
raise SettingsError(
f'Failed to parse settings from {file_path}, expecting an object (valid dictionnary)'
)
vars.update(settings)
Comment on lines +1920 to +1928
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@l00ptr Thanks for updating this.
As I mentioned before we can't merge this PR because it will introduce a breaking change. So, we have 2 options here:

1- Keep only the tests that test the current behavior(ValueError) and revert to raising SettingsError; then, we can merge the tests only.
2- Keep the PR open until V3 and merge it before V3.

I would prefer option 1.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hramezani i splitted those changes on 3 commits, let me know if i should create a specific MR for the 2 last commits (about SettingsError) and only keep the first one here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@l00ptr no need for a new PR.

Then we have to wait to merge it on V3. I will add a v3 label to the PR to not merge it until v3

return vars

@abstractmethod
Expand Down
29 changes: 29 additions & 0 deletions tests/test_source_json.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,15 @@
import json
from typing import Tuple, Type, Union

import pytest
from pydantic import BaseModel

from pydantic_settings import (
BaseSettings,
JsonConfigSettingsSource,
PydanticBaseSettingsSource,
SettingsConfigDict,
SettingsError,
)


Expand Down Expand Up @@ -67,6 +69,33 @@ def settings_customise_sources(
assert s.model_dump() == {}


def test_nondict_json_file(tmp_path):
p = tmp_path / '.env'
p.write_text(
"""
"noway"
"""
)

class Settings(BaseSettings):
foobar: str
model_config = SettingsConfigDict(json_file=p)

@classmethod
def settings_customise_sources(
cls,
settings_cls: Type[BaseSettings],
init_settings: PydanticBaseSettingsSource,
env_settings: PydanticBaseSettingsSource,
dotenv_settings: PydanticBaseSettingsSource,
file_secret_settings: PydanticBaseSettingsSource,
) -> Tuple[PydanticBaseSettingsSource, ...]:
return (JsonConfigSettingsSource(settings_cls),)

with pytest.raises(SettingsError, match='Failed to parse settings from .*, expecting an object'):
Settings()


def test_multiple_file_json(tmp_path):
p5 = tmp_path / '.env.json5'
p6 = tmp_path / '.env.json6'
Expand Down
25 changes: 25 additions & 0 deletions tests/test_source_toml.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
BaseSettings,
PydanticBaseSettingsSource,
SettingsConfigDict,
SettingsError,
TomlConfigSettingsSource,
)

Expand Down Expand Up @@ -77,6 +78,30 @@ def settings_customise_sources(
assert s.model_dump() == {}


@pytest.mark.skipif(sys.version_info <= (3, 11) and tomli is None, reason='tomli/tomllib is not installed')
def test_pyproject_nondict_toml(cd_tmp_path):
pyproject = cd_tmp_path / 'pyproject.toml'
pyproject.write_text(
"""
[tool.pydantic-settings]
foobar
"""
)

class Settings(BaseSettings):
foobar: str
model_config = SettingsConfigDict()

@classmethod
def settings_customise_sources(
cls, settings_cls: Type[BaseSettings], **_kwargs: PydanticBaseSettingsSource
) -> Tuple[PydanticBaseSettingsSource, ...]:
return (TomlConfigSettingsSource(settings_cls, pyproject),)

with pytest.raises(SettingsError, match='Failed to parse settings from'):
Settings()


@pytest.mark.skipif(sys.version_info <= (3, 11) and tomli is None, reason='tomli/tomllib is not installed')
def test_multiple_file_toml(tmp_path):
p1 = tmp_path / '.env.toml1'
Expand Down
25 changes: 25 additions & 0 deletions tests/test_source_yaml.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
BaseSettings,
PydanticBaseSettingsSource,
SettingsConfigDict,
SettingsError,
YamlConfigSettingsSource,
)

Expand Down Expand Up @@ -85,6 +86,30 @@ def settings_customise_sources(
assert s.nested.nested_field == 'world!'


@pytest.mark.skipif(yaml is None, reason='pyYaml is not installed')
def test_nondict_yaml_file(tmp_path):
p = tmp_path / '.env'
p.write_text('test invalid yaml')

class Settings(BaseSettings):
foobar: str
model_config = SettingsConfigDict(yaml_file=p)

@classmethod
def settings_customise_sources(
cls,
settings_cls: Type[BaseSettings],
init_settings: PydanticBaseSettingsSource,
env_settings: PydanticBaseSettingsSource,
dotenv_settings: PydanticBaseSettingsSource,
file_secret_settings: PydanticBaseSettingsSource,
) -> Tuple[PydanticBaseSettingsSource, ...]:
return (YamlConfigSettingsSource(settings_cls),)

with pytest.raises(SettingsError, match='Failed to parse settings from .*, expecting an object'):
Settings()


@pytest.mark.skipif(yaml is None, reason='pyYaml is not installed')
def test_yaml_no_file():
class Settings(BaseSettings):
Expand Down
Loading