pierrejeambrun commented on code in PR #42959:
URL: https://github.com/apache/airflow/pull/42959#discussion_r1799497345
##########
airflow/api_fastapi/views/public/dags.py:
##########
@@ -92,6 +93,28 @@ async def get_dags(
)
+@dags_router.get(
+ "/tags",
+ response_model=list[DAGTagResponse],
+ responses=create_openapi_http_exception_doc([400, 401, 403]),
+)
+async def get_dag_tags(
+ tags: QueryTagsFilter,
+ session: Annotated[Session, Depends(get_session)],
+) -> list[DAGTagResponse]:
+ """Get all DAG tags."""
+ dag_tag_names =
session.query(distinct(DagTag.name)).order_by(DagTag.name).all()
Review Comment:
cc: @ephraimbuddy do we have guidelines for using the newer `select`
construct vs the old `session.query` in the codebase ? Can we freely use both ?
##########
airflow/api_fastapi/views/public/dags.py:
##########
@@ -92,6 +93,28 @@ async def get_dags(
)
+@dags_router.get(
+ "/tags",
+ response_model=list[DAGTagResponse],
+ responses=create_openapi_http_exception_doc([400, 401, 403]),
+)
+async def get_dag_tags(
+ tags: QueryTagsFilter,
+ session: Annotated[Session, Depends(get_session)],
+) -> list[DAGTagResponse]:
+ """Get all DAG tags."""
+ dag_tag_names =
session.query(distinct(DagTag.name)).order_by(DagTag.name).all()
Review Comment:
`DagTag.name` is primary key, I don't think we need the `distinct` clause
##########
airflow/api_fastapi/views/public/dags.py:
##########
@@ -92,6 +93,28 @@ async def get_dags(
)
+@dags_router.get(
+ "/tags",
+ response_model=list[DAGTagResponse],
+ responses=create_openapi_http_exception_doc([400, 401, 403]),
+)
+async def get_dag_tags(
+ tags: QueryTagsFilter,
+ session: Annotated[Session, Depends(get_session)],
+) -> list[DAGTagResponse]:
+ """Get all DAG tags."""
+ dag_tag_names =
session.query(distinct(DagTag.name)).order_by(DagTag.name).all()
+ if not dag_tag_names:
+ return []
+ selected_dag_tags = {}
+ if tags.value:
+ selected_dag_tags = {tag: True for tag in tags.value}
+ return [
+ DAGTagResponse(name=tag_name_row[0],
selected=selected_dag_tags.get(tag_name_row[0], False))
+ for tag_name_row in dag_tag_names
+ ]
Review Comment:
I don't think we can actually return a 400. We should remove that from the
doc (create_openapi_http_exception_doc)
##########
airflow/api_fastapi/views/public/dags.py:
##########
@@ -92,6 +93,28 @@ async def get_dags(
)
+@dags_router.get(
+ "/tags",
+ response_model=list[DAGTagResponse],
+ responses=create_openapi_http_exception_doc([400, 401, 403]),
+)
+async def get_dag_tags(
+ tags: QueryTagsFilter,
+ session: Annotated[Session, Depends(get_session)],
+) -> list[DAGTagResponse]:
+ """Get all DAG tags."""
+ dag_tag_names =
session.query(distinct(DagTag.name)).order_by(DagTag.name).all()
+ if not dag_tag_names:
+ return []
+ selected_dag_tags = {}
+ if tags.value:
+ selected_dag_tags = {tag: True for tag in tags.value}
+ return [
Review Comment:
Filtering is done automatically vias the `QueryTagsFilter` through the query
directly, you can check implementation of other endpoints.
Doing the filtering in memory like this is suboptimal.
##########
airflow/api_fastapi/views/public/dags.py:
##########
@@ -92,6 +93,28 @@ async def get_dags(
)
+@dags_router.get(
+ "/tags",
+ response_model=list[DAGTagResponse],
+ responses=create_openapi_http_exception_doc([400, 401, 403]),
+)
+async def get_dag_tags(
+ tags: QueryTagsFilter,
+ session: Annotated[Session, Depends(get_session)],
+) -> list[DAGTagResponse]:
+ """Get all DAG tags."""
+ dag_tag_names =
session.query(distinct(DagTag.name)).order_by(DagTag.name).all()
+ if not dag_tag_names:
+ return []
+ selected_dag_tags = {}
+ if tags.value:
+ selected_dag_tags = {tag: True for tag in tags.value}
+ return [
+ DAGTagResponse(name=tag_name_row[0],
selected=selected_dag_tags.get(tag_name_row[0], False))
+ for tag_name_row in dag_tag_names
Review Comment:
I do not understand the `selected` field in the response. We query tags with
a filter on the names, then we should get back in response a collection of
tags, maybe paginated if the request says so.
This should be more homogenous to other list endpoints that the API have,
you can take a look at them to implement something similar. (get_dags for
instance)
--
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-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
