Skip to content

fix: treat empty-string JSON env vars as absent in Configuration #965

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.

Sign up for GitHub

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

Open
vdusek wants to merge 1 commit into
base: master
Choose a base branch
from
+17 −3
Open

fix: treat empty-string JSON env vars as absent in Configuration #965

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 3 additions & 3 deletions src/apify/_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,9 @@ def _load_storage_keys(data: None | str | ActorStorages) -> ActorStorages | None
`ActorStorages` dict when set programmatically.
Returns:
Normalized storage mapping, or `None` if the input is `None`.
Normalized storage mapping, or `None` if the input is `None` or an empty string.
"""
if data is None:
if data is None or data == '':
return None
storage_mapping = json.loads(data) if isinstance(data, str) else data
return {
Expand Down Expand Up @@ -470,7 +470,7 @@ class Configuration(CrawleeConfiguration):
alias='apify_charged_actor_event_counts',
description='Counts of events that were charged for the actor',
),
BeforeValidator(lambda data: json.loads(data) if isinstance(data, str) else data or None),
BeforeValidator(lambda data: json.loads(data) if isinstance(data, str) and data else data or None),
] = None

actor_storages: Annotated[
Expand Down
14 changes: 14 additions & 0 deletions tests/unit/actor/test_configuration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,13 @@ def test_actor_pricing_info_env_var_empty_becomes_none(monkeypatch: pytest.Monke
assert config.actor_pricing_info is None


def test_charged_event_counts_env_var_empty_string_becomes_none(monkeypatch: pytest.MonkeyPatch) -> None:
"""Test that an empty env var for charged_event_counts is converted to None instead of crashing."""
monkeypatch.setenv('APIFY_CHARGED_ACTOR_EVENT_COUNTS', '')
config = ApifyConfiguration()
assert config.charged_event_counts is None


def test_actor_storage_json_env_var(monkeypatch: pytest.MonkeyPatch) -> None:
"""Test that actor_storages_json is parsed from JSON env var."""
datasets = {'default': 'default_dataset_id', 'custom': 'custom_dataset_id'}
Expand All @@ -392,3 +399,10 @@ def test_actor_storage_json_env_var(monkeypatch: pytest.MonkeyPatch) -> None:
assert config.actor_storages['datasets'] == datasets
assert config.actor_storages['request_queues'] == request_queues
assert config.actor_storages['key_value_stores'] == key_value_stores


def test_actor_storages_env_var_empty_string_becomes_none(monkeypatch: pytest.MonkeyPatch) -> None:
"""Test that an empty env var for actor_storages is converted to None instead of crashing."""
monkeypatch.setenv('ACTOR_STORAGES_JSON', '')

@Pijukatel Pijukatel Jun 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Isn't this actually less defensive?

Is there any actual use case for having 'ACTOR_STORAGES_JSON' be set as an empty string? The env variable clearly states it should be a json. So either have json there, or don't set the variable.

Code should not try to guess the intention from an ambiguous action. And here we are guessing that an empty env variable probably means the user did not want to set it up in the first place...

config = ApifyConfiguration()
assert config.actor_storages is None
Loading