You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "sidshas03 (via GitHub)" <gi...@apache.org> on 2025/09/17 00:38:30 UTC
[PR] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
sidshas03 opened a new pull request, #55744:
URL: https://github.com/apache/airflow/pull/55744
Aligns the airflowctl client with the FastAPI contract for **POST `/assets/events`**.
## What changed
- Use **absolute path** `"/assets/events"` in `AssetsOperations.create_event` (was `"assets/events"`).
- Add a lightweight client unit test to assert **route + payload**:
- `airflow-ctl/tests/test_create_event_client.py`
- Ensure request/response usage matches the datamodel
(`CreateAssetEventsBody` → `AssetEventResponse`).
## Why
- Prevent routing issues caused by relative client paths.
- Keep `airflowctl` in sync with the server API and datamodel (complements the work in the assets create path).
## How to verify
```bash
python -m pytest -q airflow-ctl/tests -k create_event
Related:
Related to #55697
Companion to #55702 (asset creation path + API tests)
--
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] airflowctl: initialize extra for create assets event [airflow]
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #55744:
URL: https://github.com/apache/airflow/pull/55744#issuecomment-3309146060
Awesome work, congrats on your first merged pull request! You are invited to check our [Issue Tracker](https://github.com/apache/airflow/issues) for additional contributions.
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "sidshas03 (via GitHub)" <gi...@apache.org>.
sidshas03 commented on PR #55744:
URL: https://github.com/apache/airflow/pull/55744#issuecomment-3304167681
**Update:** I’ve scoped this PR to the single-line client fix.
**Latest commit:** `cbd3055` — *fix(airflowctl): use absolute "/assets/events" path for create\_event*. This changes only `airflow-ctl/src/airflowctl/api/operations.py` from `"assets/events"` → `"/assets/events"` to match the FastAPI route. ([GitHub][1])
**Addresses feedback:** This implements @bugraoz93’s suggestion to keep just the path change and drop the extra client test so this can merge quickly; I’ve resolved those threads. ([GitHub][2])
**Next:** Please take another look and let me know if anything else is needed. If you’d like, I can follow up with an integration test PR for `airflowctl create_event`.
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #55744:
URL: https://github.com/apache/airflow/pull/55744#discussion_r2356942058
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -243,8 +243,11 @@ def create_event(
) -> AssetEventResponse | ServerResponseError:
"""Create an asset event."""
try:
+ # Ensure extra is an object; DB/UI choke on null
+ if getattr(asset_event_body, "extra", None) is None:
Review Comment:
```suggestion
if asset_event_body.extra is None:
```
Even though I like `getattr` like safe calls, this actually brings danger to not fail the unit test. If `extra` is removed or changed name is changed in the datamodel, and creates a bug. Because this statement will return None in that case and we always add extra even though the API already changed it. This is why we use Pydantic which is kind of a data contract between the API and the client
You can try the behavior even with a dict
```python
a={}
if getattr(a, "extra", None) is None:
a["extra"] = {}
print(a)
```
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #55744:
URL: https://github.com/apache/airflow/pull/55744#discussion_r2360469597
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -243,8 +243,11 @@ def create_event(
) -> AssetEventResponse | ServerResponseError:
"""Create an asset event."""
try:
+ # Ensure extra is an object; DB/UI choke on null
Review Comment:
There is no `choke` on
```suggestion
# Ensure extra is initialised before sent to API
```
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "sidshas03 (via GitHub)" <gi...@apache.org>.
sidshas03 commented on PR #55744:
URL: https://github.com/apache/airflow/pull/55744#issuecomment-3300799432
@bugraoz93 @jason810496
- Moved the POST /assets tests into `test_assets.py`.
- Documented 422 on the endpoint.
- CTL now uses absolute "/assets" for the create call (consistency with events).
Keeping this PR scoped to asset creation + tests; `create_event` is handled in #55744. PR body updated to say “Related to #55697”.
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #55744:
URL: https://github.com/apache/airflow/pull/55744#discussion_r2360464341
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -243,8 +243,11 @@ def create_event(
) -> AssetEventResponse | ServerResponseError:
"""Create an asset event."""
try:
+ # Ensure extra is an object; DB/UI choke on null
+ if asset_event_body.extra is None:
+ asset_event_body.extra = {}
self.response = self.client.post(
- "assets/events", json=_date_safe_dict_from_pydantic(asset_event_body)
+ "/assets/events", json=_date_safe_dict_from_pydantic(asset_event_body)
Review Comment:
I mentioned a couple of times that this should not actually align with multiple of them, and we should use the other way around. That part is working. We don't need to update it
```suggestion
"assets/events", json=_date_safe_dict_from_pydantic(asset_event_body)
```
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "sidshas03 (via GitHub)" <gi...@apache.org>.
sidshas03 commented on PR #55744:
URL: https://github.com/apache/airflow/pull/55744#issuecomment-3308809172
> We need to merge this fix, so I added a couple of changes there. Thanks for the PR! I will merge once it is green
If any job turns red, I’ll take a look and fix only what the failing log shows.
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #55744:
URL: https://github.com/apache/airflow/pull/55744#discussion_r2356783703
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -244,7 +244,7 @@ def create_event(
"""Create an asset event."""
try:
self.response = self.client.post(
Review Comment:
```suggestion
if asset_event_body.extra is None:
asset_event_body.extra = {}
self.response = self.client.post(
```
I think this will fix the bug :) This is some kind of integration problem we are having with dictionary fields in airflowctl (There are a couple of cases we have dict extra fields, such as in `connections`. In some cases, in the database, they cannot be `null`, but it can be `null` on the datamodel, which ends up in error if it is passed as `None`
Please see the example below; it is the same problem. Let's patch it like this for now and we can come up with more generic solution to this
https://github.com/apache/airflow/blob/8d891bb98f318a15b063d9e9faa4a4fd6b49ef3d/airflow-ctl/src/airflowctl/api/operations.py#L573-L585
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "sidshas03 (via GitHub)" <gi...@apache.org>.
sidshas03 commented on code in PR #55744:
URL: https://github.com/apache/airflow/pull/55744#discussion_r2357103357
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -243,8 +243,11 @@ def create_event(
) -> AssetEventResponse | ServerResponseError:
"""Create an asset event."""
try:
+ # Ensure extra is an object; DB/UI choke on null
+ if getattr(asset_event_body, "extra", None) is None:
Review Comment:
Updated as suggested in **aeef5f7** — switched to a direct `asset_event_body.extra is None` check so we don’t mask schema drift; kept the absolute `"/assets/events"` path. Verified locally. Ready for re-review @bugraoz93 thanks!
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #55744:
URL: https://github.com/apache/airflow/pull/55744#discussion_r2356942058
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -243,8 +243,11 @@ def create_event(
) -> AssetEventResponse | ServerResponseError:
"""Create an asset event."""
try:
+ # Ensure extra is an object; DB/UI choke on null
+ if getattr(asset_event_body, "extra", None) is None:
Review Comment:
```suggestion
if asset_event_body.extra is None:
```
Even though I like `getattr` like safe calls, this actually brings danger to not fail the unit test. If `extra` is removed or changed name is changed in the datamodel (Pydantic models), it creates a bug. Because this statement will return None in that case and we always add extra even though the API already changed it. This is why we use Pydantic which is kind of a data contract between the API and the client
You can try the behavior even with a dict
```python
a={}
if getattr(a, "extra", None) is None:
a["extra"] = {}
print(a)
```
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #55744:
URL: https://github.com/apache/airflow/pull/55744#discussion_r2356121806
##########
airflow-ctl/tests/test_create_event_client.py:
##########
@@ -0,0 +1,36 @@
+import json
Review Comment:
I don't think we need this test, we already have unit tests here
https://github.com/apache/airflow/blob/1576db7baaee2890727ae3e0fda33eafce4e5e31/airflow-ctl/tests/airflow_ctl/api/test_operations.py#L334
I think this hasn't been caught because of it needs integration tests
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -238,13 +239,23 @@ def list_by_alias(self) -> AssetAliasCollectionResponse | ServerResponseError:
"""List all assets by alias from the API server."""
return super().execute_list(path="/assets/aliases", data_model=AssetAliasCollectionResponse)
+ def create(self, asset_body: CreateAssetBody) -> AssetResponse | ServerResponseError:
+ """Create an asset."""
+ try:
+ self.response = self.client.post(
+ "/assets", json=_date_safe_dict_from_pydantic(asset_body)
+ )
+ return AssetResponse.model_validate_json(self.response.content)
+ except ServerResponseError as e:
+ raise e
+
def create_event(
self, asset_event_body: CreateAssetEventsBody
) -> AssetEventResponse | ServerResponseError:
"""Create an asset event."""
try:
self.response = self.client.post(
- "assets/events", json=_date_safe_dict_from_pydantic(asset_event_body)
+ "/assets/events", json=_date_safe_dict_from_pydantic(asset_event_body)
Review Comment:
Maybe we only left this part in this PR and keep the create part here https://github.com/apache/airflow/blob/1576db7baaee2890727ae3e0fda33eafce4e5e31/airflow-ctl/tests/airflow_ctl/api/test_operations.py#L334
We can merge them separately and make it easier to review. We only need this line for the scope of the ticket so that this can quickly merged as fix
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "sidshas03 (via GitHub)" <gi...@apache.org>.
sidshas03 commented on code in PR #55744:
URL: https://github.com/apache/airflow/pull/55744#discussion_r2356309606
##########
airflow-ctl/tests/test_create_event_client.py:
##########
@@ -0,0 +1,36 @@
+import json
Review Comment:
Ack, removed. We already cover create_event in airflow_ctl/api/test_operations.py. I’ll add integration coverage in the follow up PR.
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on PR #55744:
URL: https://github.com/apache/airflow/pull/55744#issuecomment-3308748427
We need to merge this fix, so I added a couple of changes there. Thanks for the PR! I will merge once it is green
--
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] airflowctl: initialize extra for create assets event [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 merged PR #55744:
URL: https://github.com/apache/airflow/pull/55744
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "sidshas03 (via GitHub)" <gi...@apache.org>.
sidshas03 commented on code in PR #55744:
URL: https://github.com/apache/airflow/pull/55744#discussion_r2356310503
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -238,13 +239,23 @@ def list_by_alias(self) -> AssetAliasCollectionResponse | ServerResponseError:
"""List all assets by alias from the API server."""
return super().execute_list(path="/assets/aliases", data_model=AssetAliasCollectionResponse)
+ def create(self, asset_body: CreateAssetBody) -> AssetResponse | ServerResponseError:
+ """Create an asset."""
+ try:
+ self.response = self.client.post(
+ "/assets", json=_date_safe_dict_from_pydantic(asset_body)
+ )
+ return AssetResponse.model_validate_json(self.response.content)
+ except ServerResponseError as e:
+ raise e
+
def create_event(
self, asset_event_body: CreateAssetEventsBody
) -> AssetEventResponse | ServerResponseError:
"""Create an asset event."""
try:
self.response = self.client.post(
- "assets/events", json=_date_safe_dict_from_pydantic(asset_event_body)
+ "/assets/events", json=_date_safe_dict_from_pydantic(asset_event_body)
Review Comment:
Done! left only the "/assets/events" change here; everything else will go into the follow-up PR.
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "bugraoz93 (via GitHub)" <gi...@apache.org>.
bugraoz93 commented on code in PR #55744:
URL: https://github.com/apache/airflow/pull/55744#discussion_r2356783703
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -244,7 +244,7 @@ def create_event(
"""Create an asset event."""
try:
self.response = self.client.post(
Review Comment:
```suggestion
if asset_event_body.extra is None:
asset_event_body.extra = {}
self.response = self.client.post(
```
I think this will fix the bug :) This is some kind of integration problem we are having with extra fields in airflowctl. In some cases, in the database, they cannot be `null`, but it can be `null` on the datamodel, which ends up in error if it is passed as `None`
--
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] airflowctl: align create_event with API/datamodel (+ client test) [airflow]
Posted by "sidshas03 (via GitHub)" <gi...@apache.org>.
sidshas03 commented on code in PR #55744:
URL: https://github.com/apache/airflow/pull/55744#discussion_r2356806877
##########
airflow-ctl/src/airflowctl/api/operations.py:
##########
@@ -244,7 +244,7 @@ def create_event(
"""Create an asset event."""
try:
self.response = self.client.post(
Review Comment:
Applied the suggested fix in c3555b0 extra now defaults to {} when None; kept the absolute path. Tested locally and it works. Re-requesting review.
--
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