fix: preserve explicit empty values in querystrings and OPENAI_BASE_URL - #3910
rohitdeppi38 wants to merge 2 commits into
Conversation
`Querystring.stringify()` dropped an explicit empty-string scalar because `_stringify_item()` applied a truthiness test to the serialized value, which conflated `""` with `None`. In a query string `filter=` is distinct from omitting `filter` entirely, and the behaviour was also inconsistent across array formats, since `comma` serialization already retained empty strings. Check the value for `None` instead so that `None` is still omitted while an explicit empty string serializes to `filter=`. Closes openai#3837
When `OPENAI_BASE_URL` was set but empty, `os.environ.get()` returned `""`, so the subsequent `base_url is None` check never fired and the `https://api.openai.com/v1` default was skipped. The client was then constructed with an empty base URL and every request failed with `APIConnectionError`. An empty string is not a usable base URL, so fall back to the default as if the variable had not been set. This also restores `_base_url_was_default`, which drives base URL inheritance in `copy()`/`with_options()`. Closes openai#2927
ege-arhan
left a comment
There was a problem hiding this comment.
Verified the underlying bug on a local main checkout (no mocks): stringify({'filter': ''}) returns '' — identical to stringify({'filter': None}) — while False and 0 serialize fine ('a=false', 'a=0'), and a nested empty stringify({'a': {'b': ''}}) also collapses to ''. So the old if not serialised check conflated an explicit empty value with an omitted one, exactly as #3837 describes.
The fix reads right: if value is None keeps omitting None while serializing '' as filter=, and the array/repeat/indices/brackets cases flow through the same path so the parametrized test pins all four. The OPENAI_BASE_URL half (or None) matches too — an empty env var behaving as unset is saner than an empty base URL. Both new tests target the exact behaviors above.
Changes being requested
Two small, independent bug fixes. Both are in generated files, so I'm happy to
split them into separate PRs or move them upstream into the generator if that's
preferred.
1.
fix(qs): preserve explicit empty string values— closes #3837Querystring._stringify_item()applied a truthiness test to the serializedvalue, which conflated an explicit
""withNone:In a query string
filter=is meaningfully different from omittingfilterentirely, and the old behaviour was inconsistent across array formats —
commaserialization already retained empty strings while
repeat,indicesandbracketsdropped them.The fix checks the value for
Noneinstead, soNoneis still omitted and anexplicit empty string round-trips.
2.
fix(client): treat an empty OPENAI_BASE_URL as unset— closes #2927With
OPENAI_BASE_URL=""in the environment,os.environ.get()returned"",the following
base_url is Nonecheck never fired, and thehttps://api.openai.com/v1default was skipped. The client was constructed withan empty base URL and every request failed with
APIConnectionError: Connection error.An empty string is never a usable base URL, so it now falls back to the default
as though the variable were unset. This also restores
_base_url_was_default,which drives base URL inheritance through
copy()/with_options()and thedata-residency helpers. Applied to both
OpenAIandAsyncOpenAI.Additional context & links
Tests. Added regression coverage in
tests/test_qs.py(
test_empty_string,test_empty_string_in_arrayacross all four arrayformats) and
tests/test_client.py(test_empty_base_url_envfor the sync andasync clients). Six of these seven cases fail on
mainand pass with the fixes;the
commacase already passed, which is the inconsistency #3837 describes.Validation.
ruff check .,mypy .(1915 files) and pyright 1.1.413 areclean. The handwritten test modules touched by these paths —
test_qs.py,test_client.py,test_multipart_encoding.py,test_extract_files.py,test_module_client.pyand bothtest_data_residency_*.py— run with no newfailures (373 passed; the 3 failures present are pre-existing on
mainin thisWindows environment and are unrelated). Note that
_qs.stringify_items()alsobacks
_serialize_multipartform, so empty-string form fields are now preservedthere too; the multipart tests still pass.
Custom-code budget.
scripts/castiron/custom_code_budget.py checkreportssuccess: +6816 / -887 = 7703 custom lines against the 10000 limit, 2297 lines of
headroom. This PR contributes roughly a dozen lines and does not touch
.castiron-ratchet.json.