chore: treat empty-string env vars as absent in Configuration#965
chore: treat empty-string env vars as absent in Configuration#965vdusek wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #965 +/- ##
==========================================
+ Coverage 89.90% 89.91% +0.01%
==========================================
Files 49 49
Lines 3091 3095 +4
==========================================
+ Hits 2779 2783 +4
Misses 312 312
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| 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', '') |
There was a problem hiding this comment.
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...
Description
The Apify platform sometimes sets some env vars to an empty string instead of leaving them unset (see #303, or this Discord thread). For fields whose type can't parse
'', validation raised and crashedActor.init().A few fields were already guarded against this with ad-hoc
BeforeValidatorlambdas. This PR consolidates them into a single shared_default_if_emptyhelper and defensively applies the same fallback to the remaining typed / JSON fields, so any field that starts arriving as''falls back to its default instead of crashing.Why
choreand notfixI verified against the current apify-worker code (
getEnvsForActorinact2_run_job.ts). Only four env vars are actually emitted as''today, and those were already handled:max_paid_dataset_items(maxItems ? String(...) : '')max_total_charge_usd(maxTotalChargeUsd ? String(...) : '')timeout_at(timeoutAt ? toISOString() : '')user_is_paying(isRunOfPayingUser ? '1' : '')The remaining fields are covered defensively. The worker always gives them a real value (
started_at,dedicated_cpus,proxy_port,web_server_port,is_at_home), never sets them at all (metamorph_after_sleep,test_pay_per_event), or returns at least{}for the JSON ones (charged_event_counts, and pricing). The worker also wraps every value inString(...), so an unset value reaches the Actor as the literal"undefined", not''.So this doesn't fix an observed crash. It's cleanup (consolidating the lambdas) plus hardening for the future, hence
chore.Changes
_default_if_empty(default=...)validator and replaced the existing ad-hoc lambdas (timeout_at,max_paid_dataset_items,max_total_charge_usd,user_is_paying) with it.started_at,dedicated_cpus,proxy_port,web_server_port,metamorph_after_sleep,is_at_home,test_pay_per_event.ACTOR_STORAGES_JSON,APIFY_CHARGED_ACTOR_EVENT_COUNTS): treat''as absent instead of lettingjson.loads('')raise, mirroring the existing_parse_actor_pricing_infobehavior.