Store non-string Variable API values as JSON instead of a Python repr - #71015
Closed
ColtenOuO wants to merge 1 commit into
Closed
Store non-string Variable API values as JSON instead of a Python repr#71015ColtenOuO wants to merge 1 commit into
ColtenOuO wants to merge 1 commit into
Conversation
The value field accepts any JSON type, but every write path assumed a string: creating a variable stored its repr and patching one crashed, so values could no longer be read back with deserialize_json.
ColtenOuO
requested review from
bugraoz93,
choo121600,
ephraimbuddy,
henry3260,
jason810496,
pierrejeambrun,
rawwar and
shubhamraj-git
as code owners
August 3, 2026 20:02
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes: #71010
Sumarry
VariableBody.valueis typedJsonValue, so the Variables API accepts any JSON type— that was deliberate, added in #49844 so the API could take the same files the CLI
imports. What that PR did not change is the storage path, which still assumes a
string. Three write paths, three different outcomes:
The create case is the dangerous one: it reports success, the UI shows something that
looks plausible, and the failure only surfaces days later when a Dag calls
Variable.get(key, deserialize_json=True)and hits aJSONDecodeErrorwith nothingpointing back at the write.
Beyond the report,
boolandnullare corrupted too (True/Nonerather thantrue/null), andintis only correct by coincidence —str(7)happens to bevalid JSON.
Fix
Variable.setfalls back tostr(value)andVariable.val's setter callsbytes(value, "utf-8"); both already assume a string. So rather than teaching eachconsumer to serialize, the body serializes once and every consumer keeps its existing
assumption:
Strings pass through untouched — they are already the stored form, and re-encoding
one would add a layer of quotes on every write.
indent=2is what the bulk path was already producing for dicts and lists, so thebytes it writes are unchanged; only the two broken paths move. That also makes the
serialize_jsonbranch inBulkVariableServiceredundant — it has to go, or thevalue would be encoded twice.
The issue suggests narrowing the field to
str | Noneinstead. That would undo#49844 and reopen #49837, so this keeps JSON values accepted and stores them properly.
Was generative AI tooling used to co-author this PR?