jason810496 commented on code in PR #56813:
URL: https://github.com/apache/airflow/pull/56813#discussion_r2448645561
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/xcom.py:
##########
@@ -193,8 +194,10 @@ def get_xcom_entries(
query = query.order_by(
XComModel.dag_id, XComModel.task_id, XComModel.run_id,
XComModel.map_index, XComModel.key
)
- xcoms = session.scalars(query)
- return XComCollectionResponse(xcom_entries=xcoms,
total_entries=total_entries)
+ xcoms = list(session.scalars(query))
+ return XComCollectionResponse(
+ xcom_entries=[XComResponse.from_orm(xcom) for xcom in xcoms],
total_entries=total_entries
+ )
Review Comment:
For this case, we even copy the result twice.
Not sure if it's possible just to use `cast` to resolve the MyPy issue?
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py:
##########
@@ -205,14 +206,26 @@ def patch_dag_run(
except Exception:
log.exception("error calling listener")
elif attr_name == "note":
- dag_run = session.get(DagRun, dag_run.id)
+ dag_run_pk: int = dag_run.id
+ dag_run = session.get(DagRun, dag_run_pk)
+ if dag_run is None:
+ raise HTTPException(
+ status.HTTP_404_NOT_FOUND,
+ f"DagRun with id: `{dag_run_pk}` was not found",
+ )
if dag_run.dag_run_note is None:
dag_run.note = (attr_value, user.get_id())
else:
dag_run.dag_run_note.content = attr_value
dag_run.dag_run_note.user_id = user.get_id()
- dag_run = session.get(DagRun, dag_run.id)
+ dag_run_pk_final: int = dag_run.id
+ dag_run = session.get(DagRun, dag_run_pk_final)
+ if dag_run is None:
+ raise HTTPException(
+ status.HTTP_404_NOT_FOUND,
+ f"DagRun with id: `{dag_run_pk_final}` was not found",
+ )
Review Comment:
Same as here, maybe `session.refersh(dag_run)` _might_ be enough?
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py:
##########
@@ -626,7 +646,7 @@ def get_list_dag_runs_batch(
session=session,
)
- dag_runs = session.scalars(dag_runs_select)
+ dag_runs = list(session.scalars(dag_runs_select))
Review Comment:
I am wondering if there is a better way to resolve MyPy error without coping
the result?
Since we add a lot of `list(...)` in this PR.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/dag_run.py:
##########
@@ -205,14 +206,26 @@ def patch_dag_run(
except Exception:
log.exception("error calling listener")
elif attr_name == "note":
- dag_run = session.get(DagRun, dag_run.id)
+ dag_run_pk: int = dag_run.id
+ dag_run = session.get(DagRun, dag_run_pk)
+ if dag_run is None:
+ raise HTTPException(
+ status.HTTP_404_NOT_FOUND,
+ f"DagRun with id: `{dag_run_pk}` was not found",
+ )
Review Comment:
It seems we don't need to get `dag_run` again. Since we already select
`dag_run` in line 167 and the not existed case at the beginning of the route.
```suggestion
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
