You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "amoghrajesh (via GitHub)" <gi...@apache.org> on 2026/05/26 11:16:04 UTC
[PR] Unify task/asset state storage between Core API and Execution API [airflow]
amoghrajesh opened a new pull request, #67547:
URL: https://github.com/apache/airflow/pull/67547
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: http://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [x] Yes: claude sonnet 4.6
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
### What problem are we solving?
Task and asset state written by workers (via the Execution API) is stored as JSON-encoded strings in the DB — e.g., the integer `42` is stored as `"42"`, and the string `"hello"` as `'"hello"'`. The core API read path returned the raw DB string without decoding it, so callers saw `'"hello"'` instead of `"hello"` and `"42"` (a string) instead of `42` (an integer).
The write path had the inverse bug: values posted to the core API were stored without `json.dumps`, so a worker reading them back via the execution API would hit a `JSONDecodeError`.
### Current behaviour
- GET `/dags/{dag_id}/dagRuns/{run_id}/taskInstances/{task_id}/states` and the equivalent asset state endpoints return raw JSON-encoded strings in the value field (e.g. "\"hello\"", "42" as a string).
- PUT on these endpoints stores the value as-is without JSON-encoding, so values written via the Core API are unreadable by workers.
- The value field on request/response models is typed as str, with no null or size validation.
Classic example is if I run this task:
```python
@task
def my_task(**context: Context):
task_state = context["task_state"]
task_state.set("job_id", "12345")
task_state.set("secret-dict", {"key": "value"})
task_state.set("int_value", 42)
print("Fetching task states I stored earlier")
print("job_id:", task_state.get("job_id"))
print("secret-dict:", task_state.get("secret-dict"))
print("int_value:", task_state.get("int_value"))
my_task()
```
Core API call to get all task states returned this:
```shell
{
"task_states": [
{
"key": "int_value",
"value": "42",
"updated_at": "2026-05-26T10:07:01.872139Z",
"expires_at": "2026-06-25T10:07:01.868443Z"
},
{
"key": "job_id",
"value": "\"12345\"",
"updated_at": "2026-05-26T10:07:01.855922Z",
"expires_at": "2026-06-25T10:07:01.851283Z"
},
{
"key": "secret-dict",
"value": "{\"key\": \"value\"}",
"updated_at": "2026-05-26T10:07:01.864312Z",
"expires_at": "2026-06-25T10:07:01.860810Z"
}
],
"total_entries": 3
}
```
### Proposed change
- `value: str` widened to `value: JsonValue` on all four core API datamodels (TaskStateBody, TaskStateResponse, AssetStateBody, AssetStateResponse).
- Write paths now call `json.dumps(body.value)` before passing to the backend, matching the execution API as introduced in https://github.com/apache/airflow/pull/67418
- Read paths now call `json.loads(r.value)` when constructing responses, so callers receive native types (int, dict, list, bool, str).
- Body models now have validators rejecting `null`, `non-finite floats (NaN, Inf, -Inf)`, and values whose serialized form exceeds 65535 bytes (preserving the old max_length constraint).
NOTE: The UI PR: https://github.com/apache/airflow/pull/67292 sends in raw values as entered by user, so no impact there.
### Testing
#### Test 1: Set using task execution, Get using core API and task execution should have same response type
<img width="1708" height="855" alt="image" src="https://github.com/user-attachments/assets/f9708969-bf6b-4a3f-8df4-dc797728479c" />
<img width="1301" height="1213" alt="image" src="https://github.com/user-attachments/assets/9006df0a-8ea6-4c11-9fc9-f30730e702ed" />
#### Test 2: Value set using core API, getting via task execution and core API should have same response type
Set values using curl for this task:
```python
@task
def my_task(**context: Context):
task_state = context["task_state"]
print("Fetching task states that are set by API")
print("str_value:", task_state.get("str_value"), type(task_state.get("str_value")))
print("dict-value:", task_state.get("dict-value"), type(task_state.get("dict-value")))
print("int_value:", task_state.get("int_value"), type(task_state.get("int_value")))
my_task()
```
<img width="1708" height="855" alt="image" src="https://github.com/user-attachments/assets/8b91ba8d-3b6f-4892-8040-deda6270f0c1" />
<img width="1455" height="1098" alt="image" src="https://github.com/user-attachments/assets/5f9c42c1-8bcc-401b-bb54-aa28975c32e7" />
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #67547:
URL: https://github.com/apache/airflow/pull/67547#issuecomment-4571645869
### Backport failed to create: airflow-ctl/v0-1-test. View the failure log <a href='https://github.com/apache/airflow/actions/runs/26622437021'> Run details </a>
Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.
In matter of doubt please ask in [#release-management](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>❌</td>
<td>airflow-ctl/v0-1-test</td>
<td><a href="https://github.com/apache/airflow/commit/c1299bd3437d356dac0821a6ea2dac4d1e3f8e5e"><img src='https://img.shields.io/badge/Commit-c1299bd-red' alt='Commit Link'></a></td>
</tr>
</table>
You can attempt to backport this manually by running:
```bash
cherry_picker c1299bd airflow-ctl/v0-1-test
```
This should apply the commit to the airflow-ctl/v0-1-test branch and leave the commit in conflict state marking
the files that need manual conflict resolution.
After you have resolved the conflicts, you can continue the backport process by running:
```bash
cherry_picker --continue
```
If you don't have cherry-picker installed, see the [installation guide](https://github.com/apache/airflow/blob/main/dev/README.md#how-to-backport-pr-with-cherry-picker-cli).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #67547:
URL: https://github.com/apache/airflow/pull/67547#discussion_r3309085505
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/task_state.py:
##########
@@ -42,4 +46,15 @@ class TaskStateCollectionResponse(BaseModel):
class TaskStateBody(StrictBaseModel):
"""Request body for setting a task state value."""
- value: str = Field(max_length=65535)
+ value: JsonValue
+
+ @field_validator("value")
+ @classmethod
+ def value_is_json_representable(cls, v: JsonValue) -> JsonValue:
+ if v is None:
+ raise ValueError("value cannot be null")
+ if isinstance(v, float) and not math.isfinite(v):
+ raise ValueError("value must be a finite number; NaN and Inf are not JSON representable")
+ if len(json.dumps(v)) > _MAX_SERIALIZED_BYTES:
Review Comment:
Good catch, making the change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #67547:
URL: https://github.com/apache/airflow/pull/67547#discussion_r3309071339
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_state.py:
##########
@@ -87,7 +88,9 @@ def list_task_states(
)
rows = session.execute(paginated).all()
entries = [
- TaskStateResponse(key=r.key, value=r.value, updated_at=r.updated_at, expires_at=r.expires_at)
+ TaskStateResponse(
+ key=r.key, value=json.loads(r.value), updated_at=r.updated_at, expires_at=r.expires_at
Review Comment:
Both write paths (task execution and core rest API) guarantee the DB always contains valid JSON: the execution API does `json.dumps(value)` before storing, and the Core API PUT does `json.dumps(body.value)` before storing. json.loads on the read path is always safe.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_state.py:
##########
@@ -87,7 +88,9 @@ def list_task_states(
)
rows = session.execute(paginated).all()
entries = [
- TaskStateResponse(key=r.key, value=r.value, updated_at=r.updated_at, expires_at=r.expires_at)
+ TaskStateResponse(
+ key=r.key, value=json.loads(r.value), updated_at=r.updated_at, expires_at=r.expires_at
Review Comment:
Both write paths (task execution and core rest API) guarantee the DB always contains valid JSON: the execution API does `json.dumps(value)` before storing, and the Core API PUT does `json.dumps(body.value)` before storing.`json.loads` on the read path is always safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #67547:
URL: https://github.com/apache/airflow/pull/67547#discussion_r3322191207
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_task_state.py:
##########
@@ -191,6 +193,18 @@ def test_overwrites_existing_key(self, test_client):
def test_empty_body_returns_422(self, test_client):
assert test_client.put(f"{BASE_URL}/job_id", json={}).status_code == 422
+ def test_null_value_returns_422(self, test_client):
+ assert test_client.put(f"{BASE_URL}/job_id", json={"value": None}).status_code == 422
+
+ @pytest.mark.parametrize("bad_float", [float("nan"), float("inf"), float("-inf")])
+ def test_non_finite_float_returns_422(self, test_client, bad_float):
+ with pytest.raises(ValueError, match="Out of range float values are not JSON compliant"):
Review Comment:
Dropped `test_non_finite_float_returns_422` from both files which I forgot to do earlier.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #67547:
URL: https://github.com/apache/airflow/pull/67547#discussion_r3309076929
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_state.py:
##########
@@ -127,7 +130,7 @@ def get_task_state(
detail=f"Task state key {key!r} not found",
)
return TaskStateResponse(
- key=row.key, value=row.value, updated_at=row.updated_at, expires_at=row.expires_at
+ key=row.key, value=json.loads(row.value), updated_at=row.updated_at, expires_at=row.expires_at
Review Comment:
Same as above. No such rows can exist since both write paths (execution API and Core API PUT) always store `json.dumps(...)` before writing to the DB. `json.loads` on the read path is always safe.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #67547:
URL: https://github.com/apache/airflow/pull/67547#discussion_r3309104103
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_task_state.py:
##########
@@ -191,6 +193,18 @@ def test_overwrites_existing_key(self, test_client):
def test_empty_body_returns_422(self, test_client):
assert test_client.put(f"{BASE_URL}/job_id", json={}).status_code == 422
+ def test_null_value_returns_422(self, test_client):
+ assert test_client.put(f"{BASE_URL}/job_id", json={"value": None}).status_code == 422
+
+ @pytest.mark.parametrize("bad_float", [float("nan"), float("inf"), float("-inf")])
+ def test_non_finite_float_returns_422(self, test_client, bad_float):
+ with pytest.raises(ValueError, match="Out of range float values are not JSON compliant"):
Review Comment:
Made changes.
- Endpoint tests now status code instead of catching ValueError
- Added a new `test_non_finite_float_rejected_by_validator` which tests the validator directly with both top-level and nested non-finite floats
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #67547:
URL: https://github.com/apache/airflow/pull/67547#discussion_r3309054207
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_state.py:
##########
@@ -87,7 +88,9 @@ def list_task_states(
)
rows = session.execute(paginated).all()
entries = [
- TaskStateResponse(key=r.key, value=r.value, updated_at=r.updated_at, expires_at=r.expires_at)
+ TaskStateResponse(
+ key=r.key, value=json.loads(r.value), updated_at=r.updated_at, expires_at=r.expires_at
Review Comment:
3.3 hasn't shipped, so the pre-#67418 row scenario cannot exist in any deployed cluster.
For the custom backend concern: the core API currently calls `get_state_backend()`, but that's a known issue I already flagged in the backlog and will be fixing in next PRs, ie: the core API routes should be DB-direct (same as XCom), never routing through a custom backend. Once that is fixed, the core API only ever reads rows written by the execution API into the database, either direct data or references, which always stores `json.dumps(value)`, so `json.loads` on read is always safe. The `JSONDecodeError` risk goes away entirely at that point.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #67547:
URL: https://github.com/apache/airflow/pull/67547#discussion_r3309149569
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_task_state.py:
##########
@@ -191,6 +193,18 @@ def test_overwrites_existing_key(self, test_client):
def test_empty_body_returns_422(self, test_client):
assert test_client.put(f"{BASE_URL}/job_id", json={}).status_code == 422
+ def test_null_value_returns_422(self, test_client):
+ assert test_client.put(f"{BASE_URL}/job_id", json={"value": None}).status_code == 422
+
+ @pytest.mark.parametrize("bad_float", [float("nan"), float("inf"), float("-inf")])
+ def test_non_finite_float_returns_422(self, test_client, bad_float):
+ with pytest.raises(ValueError, match="Out of range float values are not JSON compliant"):
Review Comment:
Handled in 1e8e0ea38c
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/task_state.py:
##########
@@ -42,4 +46,15 @@ class TaskStateCollectionResponse(BaseModel):
class TaskStateBody(StrictBaseModel):
"""Request body for setting a task state value."""
- value: str = Field(max_length=65535)
+ value: JsonValue
+
+ @field_validator("value")
+ @classmethod
+ def value_is_json_representable(cls, v: JsonValue) -> JsonValue:
+ if v is None:
+ raise ValueError("value cannot be null")
+ if isinstance(v, float) and not math.isfinite(v):
+ raise ValueError("value must be a finite number; NaN and Inf are not JSON representable")
+ if len(json.dumps(v)) > _MAX_SERIALIZED_BYTES:
Review Comment:
Handled in 1e8e0ea38c
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #67547:
URL: https://github.com/apache/airflow/pull/67547#issuecomment-4571638788
Thanks for reviews, merging this one
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh merged PR #67547:
URL: https://github.com/apache/airflow/pull/67547
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on PR #67547:
URL: https://github.com/apache/airflow/pull/67547#issuecomment-4571663883
No need to backport this
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on code in PR #67547:
URL: https://github.com/apache/airflow/pull/67547#discussion_r3307553783
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/asset_state.py:
##########
@@ -85,7 +86,9 @@ def list_asset_states(
session=session,
)
rows = session.execute(paginated).all()
- entries = [AssetStateResponse(key=r.key, value=r.value, updated_at=r.updated_at) for r in rows]
+ entries = [
+ AssetStateResponse(key=r.key, value=json.loads(r.value), updated_at=r.updated_at) for r in rows
Review Comment:
Same `json.loads` 500 risk noted on `task_state.py:92` -- one bad row in the asset's state table will 500 the whole list endpoint. Worth defensive decoding here, especially since asset state is meant to be a long-lived watermark (so legacy rows from pre-#67418 are more likely to still be around than transient task state).
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_state.py:
##########
@@ -87,7 +88,9 @@ def list_task_states(
)
rows = session.execute(paginated).all()
entries = [
- TaskStateResponse(key=r.key, value=r.value, updated_at=r.updated_at, expires_at=r.expires_at)
+ TaskStateResponse(
+ key=r.key, value=json.loads(r.value), updated_at=r.updated_at, expires_at=r.expires_at
Review Comment:
`json.loads(r.value)` will raise `JSONDecodeError` on any row whose `value` column is not valid JSON, and FastAPI surfaces that as a generic 500. Two ways this hits prod:
1. Rows written by the execution API between #66073 (2026-05-04) and #67418 (2026-05-25) stored raw strings; #67418 switched to `json.dumps(...)` but didn't migrate existing rows. Anyone who ran on a pre-#67418 build now poisons reads.
2. The `BaseStateBackend` interface only requires `set(scope, key, value: str, ...)`. A custom backend that stores its own value format (e.g., escaped/quoted, msgpack-decoded-to-str, etc.) would have worked under the old `value: str` contract and now breaks.
In a list endpoint this is worse than in `get`: one bad row poisons the whole page, so users can't even paginate past it.
Suggest wrapping the decode in a try/except (skip+log, or return a sentinel) so a single legacy/odd row doesn't 500 the whole listing. Same pattern needed at `get_task_state` (line 133), and in `asset_state.py` at lines 90 and 121.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/asset_state.py:
##########
@@ -115,7 +118,7 @@ def get_asset_state(
status_code=status.HTTP_404_NOT_FOUND,
detail=f"Asset state key {key!r} not found",
)
- return AssetStateResponse(key=row.key, value=row.value, updated_at=row.updated_at)
+ return AssetStateResponse(key=row.key, value=json.loads(row.value), updated_at=row.updated_at)
Review Comment:
Same `json.loads` 500 risk as the list endpoint (line 90). A legacy or custom-backend row turns `GET` into a 500.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/task_state.py:
##########
@@ -127,7 +130,7 @@ def get_task_state(
detail=f"Task state key {key!r} not found",
)
return TaskStateResponse(
- key=row.key, value=row.value, updated_at=row.updated_at, expires_at=row.expires_at
+ key=row.key, value=json.loads(row.value), updated_at=row.updated_at, expires_at=row.expires_at
Review Comment:
Same `json.loads` 500 risk as the list endpoint above (see comment on line 92). A row written by an execution-API build between #66073 and #67418, or by a custom `BaseStateBackend`, will turn a `GET` into a 500 instead of returning the value. Worth catching `JSONDecodeError` explicitly here too.
##########
airflow-core/src/airflow/api_fastapi/core_api/datamodels/task_state.py:
##########
@@ -42,4 +46,15 @@ class TaskStateCollectionResponse(BaseModel):
class TaskStateBody(StrictBaseModel):
"""Request body for setting a task state value."""
- value: str = Field(max_length=65535)
+ value: JsonValue
+
+ @field_validator("value")
+ @classmethod
+ def value_is_json_representable(cls, v: JsonValue) -> JsonValue:
+ if v is None:
+ raise ValueError("value cannot be null")
+ if isinstance(v, float) and not math.isfinite(v):
+ raise ValueError("value must be a finite number; NaN and Inf are not JSON representable")
+ if len(json.dumps(v)) > _MAX_SERIALIZED_BYTES:
Review Comment:
The non-finite check on line 56 only catches top-level floats. Nested non-finite values pass through:
```python
TaskStateBody.model_validate({"value": {"a": float("nan")}}) # passes
# then json.dumps({"a": float("nan")}) -> '{"a": NaN}' -- not valid JSON
```
Simpler + complete fix: drop the `isinstance(v, float) and not math.isfinite(v)` branch and pass `allow_nan=False` to the size check:
```python
try:
serialized = json.dumps(v, allow_nan=False)
except ValueError as e:
raise ValueError("value contains non-finite numbers; NaN and Inf are not JSON representable") from e
if len(serialized) > _MAX_SERIALIZED_BYTES:
raise ValueError(...)
```
That catches NaN/Inf anywhere in the structure with a single pass and the existing top-level guard becomes redundant. Same fix needed in `asset_state.py:57` -- the two validators are line-for-line identical.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_task_state.py:
##########
@@ -191,6 +193,18 @@ def test_overwrites_existing_key(self, test_client):
def test_empty_body_returns_422(self, test_client):
assert test_client.put(f"{BASE_URL}/job_id", json={}).status_code == 422
+ def test_null_value_returns_422(self, test_client):
+ assert test_client.put(f"{BASE_URL}/job_id", json={"value": None}).status_code == 422
+
+ @pytest.mark.parametrize("bad_float", [float("nan"), float("inf"), float("-inf")])
+ def test_non_finite_float_returns_422(self, test_client, bad_float):
+ with pytest.raises(ValueError, match="Out of range float values are not JSON compliant"):
Review Comment:
This test doesn't actually verify the validator behavior. The matched message `"Out of range float values are not JSON compliant"` is CPython's stdlib `json` error message (raised by FastAPI's request-body serializer with `allow_nan=False` before the request ever reaches the route), not the validator's `"value must be a finite number; NaN and Inf are not JSON representable"`.
Consequence: the test passes even if the validator is removed entirely, so it gives false confidence in the new check. Two ways to fix:
1. To actually exercise the validator, you have to bypass FastAPI's JSON parsing -- e.g. directly construct `TaskStateBody(value=float("nan"))` and `pytest.raises(ValidationError, match="value must be a finite number")`.
2. Or, if the intent is to confirm the endpoint rejects non-finite floats (regardless of layer), assert `response.status_code == 422` instead of expecting `ValueError` to propagate.
Same issue in `test_asset_state.py:185`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #67547:
URL: https://github.com/apache/airflow/pull/67547#discussion_r3309080205
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/asset_state.py:
##########
@@ -85,7 +86,9 @@ def list_asset_states(
session=session,
)
rows = session.execute(paginated).all()
- entries = [AssetStateResponse(key=r.key, value=r.value, updated_at=r.updated_at) for r in rows]
+ entries = [
+ AssetStateResponse(key=r.key, value=json.loads(r.value), updated_at=r.updated_at) for r in rows
Review Comment:
Same as task state — both write paths always store json.dumps(...), so no non-JSON rows can exist. The long-lived watermark concern doesn't apply since 3.3 hasn't shipped yet.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/asset_state.py:
##########
@@ -115,7 +118,7 @@ def get_asset_state(
status_code=status.HTTP_404_NOT_FOUND,
detail=f"Asset state key {key!r} not found",
)
- return AssetStateResponse(key=row.key, value=row.value, updated_at=row.updated_at)
+ return AssetStateResponse(key=row.key, value=json.loads(row.value), updated_at=row.updated_at)
Review Comment:
Same as above — no non-JSON rows can exist since both write paths always do json.dumps(...) before storing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Unify task/asset state storage between Core API and Execution API [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #67547:
URL: https://github.com/apache/airflow/pull/67547#discussion_r3309078130
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/asset_state.py:
##########
@@ -115,7 +118,7 @@ def get_asset_state(
status_code=status.HTTP_404_NOT_FOUND,
detail=f"Asset state key {key!r} not found",
)
- return AssetStateResponse(key=row.key, value=row.value, updated_at=row.updated_at)
+ return AssetStateResponse(key=row.key, value=json.loads(row.value), updated_at=row.updated_at)
Review Comment:
Same as task state — both write paths always store json.dumps(...), so no non-JSON rows can exist. The long-lived watermark concern doesn't apply since 3.3 hasn't shipped yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org