pierrejeambrun commented on code in PR #51682:
URL: https://github.com/apache/airflow/pull/51682#discussion_r2161133729
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/dependencies.py:
##########
@@ -80,9 +95,26 @@ def get_dependencies(session: SessionDep, node_id: str |
None = None) -> BaseGra
"edges": edges,
}
- if node_id is not None:
+ # If ids_to_fetch is filled, filter for each connected component
+ if ids_to_fetch:
try:
- data = extract_single_connected_component(node_id, data["nodes"],
data["edges"])
+ # Join all connected components of the requested ids
+ all_nodes = []
+ all_edges = []
+ seen_nodes = set()
+ seen_edges = set()
+ for nid in ids_to_fetch:
+ comp = extract_single_connected_component(nid, data["nodes"],
data["edges"])
+ for n in comp["nodes"]:
+ if n["id"] not in seen_nodes:
+ all_nodes.append(n)
+ seen_nodes.add(n["id"])
+ for e in comp["edges"]:
+ edge_tuple = (e["source_id"], e["target_id"])
+ if edge_tuple not in seen_edges:
+ all_edges.append(e)
+ seen_edges.add(edge_tuple)
+ data = {"nodes": all_nodes, "edges": all_edges}
Review Comment:
I don't think we should do this manually.
`extract_single_connected_component` is actually calling
`extract_connected_components` which holds all the connected components of the
graph. We could directly use all of those and merge them to avoid computing
multiple times `extract_connected_components` (for each node id).
##########
airflow-core/src/airflow/ui/src/pages/AssetsGroupedList/AssetsGroupedList.tsx:
##########
@@ -0,0 +1,297 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Box, Link, IconButton, VStack, Flex, Heading, Text, Input } from
"@chakra-ui/react";
+import { useState, useMemo } from "react";
+import { Link as RouterLink } from "react-router-dom";
+
+import { useAssetServiceGetAssets } from "openapi/queries";
+import type { AssetResponse } from "openapi/requests/types.gen";
+import { DataTable } from "src/components/DataTable";
+import { useTableURLState } from "src/components/DataTable/useTableUrlState";
+import { ErrorAlert } from "src/components/ErrorAlert";
+import Time from "src/components/Time";
+
+import { CreateAssetEvent } from "../Asset/CreateAssetEvent";
+import { DependencyPopover } from "../AssetsList/DependencyPopover";
+
+// Ícones SVG inline para expandir/colapsar
+const ChevronDownIcon = () => (
+ <svg fill="currentColor" height="1em" viewBox="0 0 20 20" width="1em">
+ <path
+ clipRule="evenodd"
+ d="M5.23 7.21a.75.75 0 011.06.02L10 11.085l3.71-3.855a.75.75 0 111.08
1.04l-4.24 4.4a.75.75 0 01-1.08 0l-4.24-4.4a.75.75 0 01.02-1.06z"
+ fillRule="evenodd"
+ />
+ </svg>
+);
+
+const ChevronRightIcon = () => (
+ <svg fill="currentColor" height="1em" viewBox="0 0 20 20" width="1em">
+ <path
+ clipRule="evenodd"
+ d="M7.21 5.23a.75.75 0 011.06-.02l4.4 4.24a.75.75 0 010 1.08l-4.4
4.24a.75.75 0 11-1.04-1.08L11.085 10 7.23 6.29a.75.75 0 01-.02-1.06z"
+ fillRule="evenodd"
+ />
+ </svg>
+);
Review Comment:
We already do this at different places in the code. (We use chevron icons
from react-icon lib instead), you can do something similar.
##########
airflow-core/src/airflow/ui/src/queries/useMultiDependencyGraph.ts:
##########
@@ -0,0 +1,51 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useQueryClient, type UseQueryOptions } from "@tanstack/react-query";
+
+import {
+ useDependenciesServiceGetDependencies,
+ UseDependenciesServiceGetDependenciesKeyFn,
+} from "openapi/queries";
+import type { BaseGraphResponse } from "openapi/requests/types.gen";
+
+/**
+ * Hook to fetch the dependency graph for multiple assets in a single query.
+ * Expects the backend to accept nodeIds as a comma-separated string or array.
+ */
+export const useMultiDependencyGraph = (
+ assetIds: Array<string>,
+ options?: Omit<UseQueryOptions<BaseGraphResponse, unknown>, "queryFn" |
"queryKey">,
+) => {
+ const queryClient = useQueryClient();
+
+ const nodeIdsParam = assetIds.map((id) => `asset:${id}`).join(",");
+
+ const query = useDependenciesServiceGetDependencies({ nodeIds: nodeIdsParam
}, undefined, options);
+
+ query.data?.nodes.forEach((node) => {
+ const key = UseDependenciesServiceGetDependenciesKeyFn({ nodeId: node.id
});
+ const queryData = queryClient.getQueryData(key);
+
+ if (!Boolean(queryData)) {
+ queryClient.setQueryData(key, query.data);
+ }
+ });
Review Comment:
Nice idea but I think in its current form it could induce some bugs. Setting
the cache for a particular node_id, with the result of
`useDependenciesServiceGetDependencies` with many node ids. Because the
response returned for many node ids will hold more connected components than
just the 1 single connected component that we should retrieve from the `nodeId:
node.id` filter.
This could therefore display unrelated dependencies when displaying
dependencies of a particular node.
##########
airflow-core/src/airflow/ui/src/pages/AssetsList/AssetsList.tsx:
##########
@@ -92,64 +105,111 @@ const createColumns = (translate: (key: string) =>
string): Array<ColumnDef<Asse
},
];
-const NAME_PATTERN_PARAM = SearchParamsKeys.NAME_PATTERN;
-
export const AssetsList = () => {
const { t: translate } = useTranslation(["assets", "common"]);
const [searchParams, setSearchParams] = useSearchParams();
- const [namePattern, setNamePattern] =
useState(searchParams.get(NAME_PATTERN_PARAM) ?? undefined);
+ const [searchValue, setSearchValue] = useState(
+ searchParams.get(SearchParamsKeys.NAME_PATTERN) ??
+ searchParams.get(SearchParamsKeys.GROUP_PATTERN) ??
+ undefined,
+ );
const { setTableURLState, tableURLState } = useTableURLState();
const { pagination, sorting } = tableURLState;
const [sort] = sorting;
const orderBy = sort ? `${sort.desc ? "-" : ""}${sort.id}` : undefined;
- const { data, error, isLoading } = useAssetServiceGetAssets({
+ const {
+ data: dataByName,
+ error: errorByName,
+ isLoading: isLoadingByName,
+ } = useAssetServiceGetAssets({
limit: pagination.pageSize,
- namePattern,
+ namePattern: searchValue ?? undefined,
offset: pagination.pageIndex * pagination.pageSize,
orderBy,
});
+ const {
+ data: dataByGroup,
+ error: errorByGroup,
+ isLoading: isLoadingByGroup,
+ } = useAssetServiceGetAssets({
+ groupPattern: searchValue ?? undefined,
+ limit: pagination.pageSize,
+ offset: pagination.pageIndex * pagination.pageSize,
+ orderBy,
+ });
Review Comment:
We shouldn't do that manually in the UI. (call the endpoints with two
different parameters and then merge results in the UI).
The backend should support this. (You can write a custom filter that will do
the search accross both 'asset.name` and `asset.group`, or expand the
`search_param_factory` and `_SearchParam` to be able to take a
`list[ColumnElement]` as attribute. And perform a search across multiple
columns.
##########
airflow-core/src/airflow/ui/src/pages/AssetsGroupedList/AssetsGroupedList.tsx:
##########
@@ -0,0 +1,297 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Box, Link, IconButton, VStack, Flex, Heading, Text, Input } from
"@chakra-ui/react";
+import { useState, useMemo } from "react";
+import { Link as RouterLink } from "react-router-dom";
+
+import { useAssetServiceGetAssets } from "openapi/queries";
+import type { AssetResponse } from "openapi/requests/types.gen";
+import { DataTable } from "src/components/DataTable";
+import { useTableURLState } from "src/components/DataTable/useTableUrlState";
+import { ErrorAlert } from "src/components/ErrorAlert";
+import Time from "src/components/Time";
+
+import { CreateAssetEvent } from "../Asset/CreateAssetEvent";
+import { DependencyPopover } from "../AssetsList/DependencyPopover";
+
+// Ícones SVG inline para expandir/colapsar
+const ChevronDownIcon = () => (
+ <svg fill="currentColor" height="1em" viewBox="0 0 20 20" width="1em">
+ <path
+ clipRule="evenodd"
+ d="M5.23 7.21a.75.75 0 011.06.02L10 11.085l3.71-3.855a.75.75 0 111.08
1.04l-4.24 4.4a.75.75 0 01-1.08 0l-4.24-4.4a.75.75 0 01.02-1.06z"
+ fillRule="evenodd"
+ />
+ </svg>
+);
+
+const ChevronRightIcon = () => (
+ <svg fill="currentColor" height="1em" viewBox="0 0 20 20" width="1em">
+ <path
+ clipRule="evenodd"
+ d="M7.21 5.23a.75.75 0 011.06-.02l4.4 4.24a.75.75 0 010 1.08l-4.4
4.24a.75.75 0 11-1.04-1.08L11.085 10 7.23 6.29a.75.75 0 01-.02-1.06z"
+ fillRule="evenodd"
+ />
+ </svg>
+);
+
+const NameCell = ({ original }: { readonly original: AssetResponse }) => (
+ <Link asChild color="fg.info" fontWeight="bold">
+ <RouterLink to={`/assets/${original.id}`}>{original.name}</RouterLink>
+ </Link>
+);
+
+const LastAssetEventCell = ({ original }: { readonly original: AssetResponse
}) => {
+ const assetEvent = original.last_asset_event;
+ const timestamp = assetEvent?.timestamp;
+
+ if (timestamp === undefined || timestamp === null) {
+ return undefined;
+ }
+
+ return <Time datetime={timestamp} />;
+};
+
+const GroupCell = ({ original }: { readonly original: AssetResponse }) => {
+ const { group } = original;
+
+ if (!group) {
+ return undefined;
+ }
+
+ return (
+ <Link asChild color="fg.info" fontWeight="bold">
+ <RouterLink to={`/assets/group/${group}`}>{group}</RouterLink>
+ </Link>
+ );
+};
+
+const ConsumingDagsCell = ({ original }: { readonly original: AssetResponse })
=>
+ original.consuming_dags.length ? (
+ <DependencyPopover dependencies={original.consuming_dags} type="Dag" />
+ ) : undefined;
+
+const ProducingTasksCell = ({ original }: { readonly original: AssetResponse
}) =>
+ original.producing_tasks.length ? (
+ <DependencyPopover dependencies={original.producing_tasks} type="Task" />
+ ) : undefined;
+
+const TriggerCell = ({ original }: { readonly original: AssetResponse }) => (
+ <CreateAssetEvent asset={original} withText={false} />
+);
+
+const truncate = (str: string, max = 32) => (str.length > max ?
`${str.slice(0, max - 3)}...` : str);
+
+const nameCellRenderer = ({ row: { original } }: { row: { original:
AssetResponse } }) => (
+ <NameCell original={original} />
+);
+const lastAssetEventCellRenderer = ({ row: { original } }: { row: { original:
AssetResponse } }) => (
+ <LastAssetEventCell original={original} />
+);
+const groupCellRenderer = ({ row: { original } }: { row: { original:
AssetResponse } }) => (
+ <GroupCell original={original} />
+);
+const consumingDagsCellRenderer = ({ row: { original } }: { row: { original:
AssetResponse } }) => (
+ <ConsumingDagsCell original={original} />
+);
+const producingTasksCellRenderer = ({ row: { original } }: { row: { original:
AssetResponse } }) => (
+ <ProducingTasksCell original={original} />
+);
+const triggerCellRenderer = ({ row: { original } }: { row: { original:
AssetResponse } }) => (
+ <TriggerCell original={original} />
+);
+
+export const AssetsGroupedList = () => {
+ const { setTableURLState, tableURLState } = useTableURLState();
+ const { sorting } = tableURLState;
+ const [sort] = sorting;
+ const orderBy = sort ? `${sort.desc ? "-" : ""}${sort.id}` : undefined;
+
+ const { data, error, isLoading } = useAssetServiceGetAssets({
+ limit: 1000,
+ offset: 0,
+ orderBy,
+ });
+
+ const [search, setSearch] = useState("");
+ const [sortAsc, setSortAsc] = useState(true);
+
+ // Group assets by group
+ const grouped = useMemo(() => {
+ const filtered = (data?.assets ?? []).filter((asset) =>
+ asset.group.toLowerCase().includes(search.toLowerCase()),
+ );
+ const groupedObj = filtered.reduce<Record<string,
Array<AssetResponse>>>((acc, asset) => {
+ const groupName = asset.group;
+
+ acc[groupName] ??= [];
+ acc[groupName].push(asset);
+
+ return acc;
+ }, {});
Review Comment:
I think this logic can holds a few problems, first of the endpoint
`useAssetServiceGetAssets` is paginated. It means that you'll only retrieve the
first 50 items, and you need a mechanism to display the remaining items (cf
tables pagination component).
In addition, there is no guarantee at this point that all assets for a
specified group will be on the same page, you could end up with a group missing
some assets because those were not returned yet.
I think the easiest approach would be to add a backend ui endpoints to
list/retrieve asset groups directly. (I group would list all of it's child
asset and we would paginate on groups directly). Maybe there are other
approaches possible but I would need to think about it.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/ui/dependencies.py:
##########
@@ -41,12 +40,28 @@
),
dependencies=[Depends(requires_access_dag("GET",
DagAccessEntity.DEPENDENCIES))],
)
-def get_dependencies(session: SessionDep, node_id: str | None = None) ->
BaseGraphResponse:
- """Dependencies graph."""
+def get_dependencies(
+ node_id: str | None = None,
+ node_ids: str | None = Query(None, description="Comma-separated list of
node ids"),
+) -> BaseGraphResponse:
+ """Dependencies graph. Supports a single node_id or multiple node_ids
separated by commas."""
+ # Parse node_ids (priority to node_ids, fallback to node_id)
+ ids_to_fetch: list[str] = []
+ # If node_id contains commas, treat as multiple IDs (extra protection)
+ if node_ids:
+ ids_to_fetch = [nid.strip() for nid in node_ids.split(",") if
nid.strip()]
Review Comment:
No, FastAPI handles `node_ids: list[str] | None = None` natively. FastAPI
chose exploded form query params, i.e `?node_ids=1&node_ids=2` you will
directly receive `node_ids=["1","2"] in your function.
--
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]
