diff --git a/airflow-core/src/airflow/ui/src/components/DagImportErrorModal.test.tsx b/airflow-core/src/airflow/ui/src/components/DagImportErrorModal.test.tsx new file mode 100644 index 00000000000..5d4a5765315 --- /dev/null +++ b/airflow-core/src/airflow/ui/src/components/DagImportErrorModal.test.tsx @@ -0,0 +1,50 @@ +/*! + * 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 "@testing-library/jest-dom/vitest"; +import { render, screen, within } from "@testing-library/react"; +import { describe, expect, it, vi } from "vitest"; + +import type { ImportErrorResponse } from "openapi/requests/types.gen"; + +import { Wrapper } from "src/utils/Wrapper"; + +import { DagImportErrorModal } from "./DagImportErrorModal"; + +const importError: ImportErrorResponse = { + bundle_name: "dags-folder", + file_token: "token", + filename: "dags/broken_dag.py", + import_error_id: 1, + source_reference: null, + stack_trace: 'Traceback (most recent call last):\n File "dags/broken_dag.py", line 1', + timestamp: "2025-01-01T00:00:00Z", +}; + +describe("DagImportErrorModal", () => { + it("shows the import error metadata and stack trace in a titled dialog", () => { + render(, { wrapper: Wrapper }); + + // i18n resources are not loaded in unit tests, so the title renders as its key. + const dialog = screen.getByRole("dialog", { name: "importErrors.dagImportError" }); + + expect(dialog).toHaveTextContent("dags-folder"); + expect(dialog).toHaveTextContent("dags/broken_dag.py"); + expect(within(dialog).getByText(/Traceback \(most recent call last\)/u)).toBeInTheDocument(); + }); +}); diff --git a/airflow-core/src/airflow/ui/src/components/DagImportErrorModal.tsx b/airflow-core/src/airflow/ui/src/components/DagImportErrorModal.tsx index 0340c50f012..08570db8056 100644 --- a/airflow-core/src/airflow/ui/src/components/DagImportErrorModal.tsx +++ b/airflow-core/src/airflow/ui/src/components/DagImportErrorModal.tsx @@ -16,15 +16,16 @@ * specific language governing permissions and limitations * under the License. */ -import { ClipboardRoot, Heading, HStack, Text } from "@chakra-ui/react"; +import { ClipboardRoot, HStack, Text } from "@chakra-ui/react"; import { useTranslation } from "react-i18next"; import { LuFileWarning } from "react-icons/lu"; import { PiFilePy } from "react-icons/pi"; import type { ImportErrorResponse } from "openapi/requests/types.gen"; -import { ClipboardIconButton, Modal } from "src/system-components"; +import { ClipboardIconButton } from "src/system-components"; +import { ErrorModal } from "src/components/ErrorModal"; import Time from "src/components/Time"; type Props = { @@ -37,21 +38,11 @@ export const DagImportErrorModal = ({ importError, onClose, open }: Props) => { const { t: translate } = useTranslation(["dashboard", "components"]); return ( - - - {translate("importErrors.dagImportError", { count: 1 })} - - ), - }} - lazyMount - onOpenChange={onClose} + } + onClose={onClose} open={open} - scrollBehavior="inside" - size="lg" + title={translate("importErrors.dagImportError", { count: 1 })} > @@ -73,6 +64,6 @@ export const DagImportErrorModal = ({ importError, onClose, open }: Props) => { {importError.stack_trace} - + ); }; diff --git a/airflow-core/src/airflow/ui/src/components/ErrorModal.test.tsx b/airflow-core/src/airflow/ui/src/components/ErrorModal.test.tsx new file mode 100644 index 00000000000..5b8883791ab --- /dev/null +++ b/airflow-core/src/airflow/ui/src/components/ErrorModal.test.tsx @@ -0,0 +1,55 @@ +/*! + * 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 "@testing-library/jest-dom/vitest"; +import { fireEvent, render, screen, waitFor, within } from "@testing-library/react"; +import { describe, expect, it, vi } from "vitest"; + +import { BaseWrapper } from "src/utils/Wrapper"; + +import { ErrorModal } from "./ErrorModal"; + +describe("ErrorModal", () => { + it("names the dialog after its title and renders the error details", () => { + render( + + Full error details + , + { wrapper: BaseWrapper }, + ); + + const dialog = screen.getByRole("dialog", { name: "Something failed" }); + + expect(within(dialog).getByText("Full error details")).toBeInTheDocument(); + }); + + it("calls onClose when the close button is clicked", async () => { + const onClose = vi.fn(); + + render( + + Full error details + , + { wrapper: BaseWrapper }, + ); + + fireEvent.click(screen.getByRole("button", { name: "Close" })); + + await waitFor(() => expect(onClose).toHaveBeenCalled()); + }); +}); diff --git a/airflow-core/src/airflow/ui/src/components/ErrorModal.tsx b/airflow-core/src/airflow/ui/src/components/ErrorModal.tsx new file mode 100644 index 00000000000..2eff490c0a6 --- /dev/null +++ b/airflow-core/src/airflow/ui/src/components/ErrorModal.tsx @@ -0,0 +1,55 @@ +/*! + * 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 type { ReactNode } from "react"; + +import { Heading, HStack } from "@chakra-ui/react"; + +import { Dialog, Modal } from "src/system-components"; + +type Props = { + readonly children: ReactNode; + readonly icon?: ReactNode; + readonly onClose: () => void; + readonly open: boolean; + readonly title: ReactNode; +}; + +export const ErrorModal = ({ children, icon, onClose, open, title }: Props) => ( + + {icon} + + {title} + + + ), + }} + lazyMount + onOpenChange={onClose} + open={open} + scrollBehavior="inside" + size="lg" + > + {children} + +); diff --git a/airflow-core/src/airflow/ui/src/pages/XCom/XComEntry.test.tsx b/airflow-core/src/airflow/ui/src/pages/XCom/XComEntry.test.tsx new file mode 100644 index 00000000000..ba6d3c1ef04 --- /dev/null +++ b/airflow-core/src/airflow/ui/src/pages/XCom/XComEntry.test.tsx @@ -0,0 +1,216 @@ +/*! + * 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 { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import "@testing-library/jest-dom/vitest"; +import { act, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; +import { http, HttpResponse } from "msw"; +import { setupServer } from "msw/node"; +import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; + +import { Wrapper } from "src/utils/Wrapper"; + +import { XComEntry } from "./XComEntry"; + +const server = setupServer(); +const entryUrl = "/api/v2/dags/test_dag/dagRuns/test_run/taskInstances/test_task/xcomEntries/:xcomKey"; +const entryProps = { dagId: "test_dag", mapIndex: -1, runId: "test_run", taskId: "test_task" }; + +// i18n resources are not loaded in unit tests, so translated text renders as its key. +const errorTitle = "error.title"; + +beforeAll(() => server.listen({ onUnhandledRequest: "bypass" })); +afterEach(() => server.resetHandlers()); +afterAll(() => server.close()); + +describe("XComEntry", () => { + it.each([ + { detail: "XCom entry was deleted", status: 404 }, + { detail: "Access to this XCom entry is denied", status: 403 }, + { detail: "Unable to deserialize the XCom value", status: 500 }, + ])("shows a $status error without removing another entry", async ({ detail, status }) => { + server.use( + http.get(entryUrl, ({ params }) => + params.xcomKey === "failed_entry" + ? HttpResponse.json({ detail }, { status }) + : HttpResponse.json({ key: "healthy_entry", value: "Another XCom value" }), + ), + ); + + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + + render( + + + + , + { wrapper: Wrapper }, + ); + + const trigger = await screen.findByRole("button", { name: `${errorTitle} ${String(status)}` }); + + expect(trigger).toHaveTextContent(String(status)); + expect(trigger).not.toHaveTextContent(detail); + expect(await screen.findByText("Another XCom value")).toBeInTheDocument(); + expect(screen.getAllByRole("button", { name: /copy/iu })).toHaveLength(1); + expect(screen.queryByTestId("error-alert")).not.toBeInTheDocument(); + + fireEvent.click(trigger); + + const dialog = await screen.findByRole("dialog", { name: errorTitle }); + const alert = within(dialog).getByTestId("error-alert"); + + expect(alert).toHaveTextContent(String(status)); + expect(alert).toHaveTextContent(detail); + }); + + it("closes the full error details from the modal close button", async () => { + server.use( + http.get(entryUrl, () => HttpResponse.json({ detail: "XCom entry was deleted" }, { status: 404 })), + ); + + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + + render( + + + , + { wrapper: Wrapper }, + ); + + fireEvent.click(await screen.findByRole("button", { name: `${errorTitle} 404` })); + + const dialog = await screen.findByRole("dialog", { name: errorTitle }); + + fireEvent.click(within(dialog).getByRole("button", { name: "Close" })); + + await waitFor(() => expect(screen.queryByRole("dialog")).not.toBeInTheDocument()); + expect(screen.getByRole("button", { name: `${errorTitle} 404` })).toBeInTheDocument(); + }); + + it("shows a refetch error instead of allowing a stale value to be copied", async () => { + server.use(http.get(entryUrl, () => HttpResponse.json({ key: "return_value", value: "Stale value" }))); + + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + + render( + + + , + { wrapper: Wrapper }, + ); + + await screen.findByText("Stale value"); + + server.use( + http.get(entryUrl, () => HttpResponse.json({ detail: "XCom entry was deleted" }, { status: 404 })), + ); + + await act(() => queryClient.invalidateQueries()); + + expect(await screen.findByTestId("xcom-entry-error")).toHaveTextContent("404"); + expect(screen.queryByText("Stale value")).not.toBeInTheDocument(); + expect(screen.queryByRole("button", { name: /copy/iu })).not.toBeInTheDocument(); + }); + + it("keeps new error details closed after a successful refetch", async () => { + server.use( + http.get(entryUrl, () => HttpResponse.json({ detail: "XCom entry was deleted" }, { status: 404 })), + ); + + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + + render( + + + , + { wrapper: Wrapper }, + ); + + fireEvent.click(await screen.findByRole("button", { name: `${errorTitle} 404` })); + await screen.findByRole("dialog", { name: errorTitle }); + + server.use( + http.get(entryUrl, () => HttpResponse.json({ key: "return_value", value: "Recovered value" })), + ); + await act(() => queryClient.invalidateQueries()); + await screen.findByText("Recovered value"); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + + server.use( + http.get(entryUrl, () => + HttpResponse.json({ detail: "Unable to deserialize the XCom value" }, { status: 500 }), + ), + ); + await act(() => queryClient.invalidateQueries()); + + expect(await screen.findByRole("button", { name: `${errorTitle} 500` })).toBeInTheDocument(); + expect(screen.queryByRole("dialog")).not.toBeInTheDocument(); + }); + + it("exposes the full validation error for structured details", async () => { + server.use( + http.get(entryUrl, () => + HttpResponse.json( + { detail: [{ loc: ["query", "map_index"], msg: "Invalid map index", type: "value_error" }] }, + { status: 422 }, + ), + ), + ); + + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + + render( + + + , + { wrapper: Wrapper }, + ); + + fireEvent.click(await screen.findByRole("button", { name: `${errorTitle} 422` })); + + const dialog = await screen.findByRole("dialog", { name: errorTitle }); + + expect(within(dialog).getByTestId("error-alert")).toHaveTextContent("query.map_index Invalid map index"); + }); + + it("shows a network error without an HTTP status or copy button", async () => { + server.use(http.get(entryUrl, () => HttpResponse.error())); + + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + + render( + + + , + { wrapper: Wrapper }, + ); + + const trigger = await screen.findByRole("button", { name: errorTitle }); + + expect(trigger).toHaveTextContent(errorTitle); + expect(trigger).not.toHaveTextContent("undefined"); + expect(screen.queryByRole("button", { name: /copy/iu })).not.toBeInTheDocument(); + + fireEvent.click(trigger); + + const dialog = await screen.findByRole("dialog", { name: errorTitle }); + + expect(within(dialog).getByTestId("error-alert")).toHaveTextContent("Network Error"); + }); +}); diff --git a/airflow-core/src/airflow/ui/src/pages/XCom/XComEntry.tsx b/airflow-core/src/airflow/ui/src/pages/XCom/XComEntry.tsx index bc3a2562bea..aad31bc6aff 100644 --- a/airflow-core/src/airflow/ui/src/pages/XCom/XComEntry.tsx +++ b/airflow-core/src/airflow/ui/src/pages/XCom/XComEntry.tsx @@ -16,13 +16,19 @@ * specific language governing permissions and limitations * under the License. */ -import { Skeleton, HStack, Text, Link } from "@chakra-ui/react"; +import { useState } from "react"; + +import { Skeleton, Button, HStack, Text, Link } from "@chakra-ui/react"; +import { useTranslation } from "react-i18next"; +import { FiAlertCircle } from "react-icons/fi"; import { useXcomServiceGetXcomEntry } from "openapi/queries"; import type { XComResponseNative } from "openapi/requests/types.gen"; import { ClipboardIconButton, ClipboardRoot } from "src/system-components"; +import { ErrorAlert, type ExpandedApiError } from "src/components/ErrorAlert"; +import { ErrorModal } from "src/components/ErrorModal"; import RenderedJsonField from "src/components/RenderedJsonField"; import { urlRegex } from "src/constants/urlRegex"; @@ -66,8 +72,43 @@ const renderTextWithLinks = (text: string) => { ); }; +const XComError = ({ error }: { readonly error: unknown }) => { + const { t: translate } = useTranslation("common"); + const [errorOpen, setErrorOpen] = useState(false); + const { status } = error as Partial; + const errorTitle = translate("error.title"); + + return ( + <> + + } + onClose={() => setErrorOpen(false)} + open={errorOpen} + title={errorTitle} + > + + + + ); +}; + export const XComEntry = ({ dagId, mapIndex, open = false, runId, taskId, xcomKey }: XComEntryProps) => { - const { data, isLoading } = useXcomServiceGetXcomEntry({ + const { data, error, isLoading } = useXcomServiceGetXcomEntry({ dagId, dagRunId: runId, deserialize: true, @@ -76,6 +117,11 @@ export const XComEntry = ({ dagId, mapIndex, open = false, runId, taskId, xcomKe taskId, xcomKey, }); + + if (Boolean(error)) { + return ; + } + // When deserialize=true, the API returns a stringified representation // so we don't need to JSON.stringify it again const xcomValue = data?.value;