From fd4e344880b505cfe53a97a6373d329515bbc7a3 Mon Sep 17 00:00:00 2001 From: Brent Bovenzi Date: Mon, 6 Jun 2022 15:52:47 +0200 Subject: [PATCH] Fix useTasks crash on error (#24152) * Prevent UI from crashing on Get API error * add test * don't show API errors in test logs * use setMinutes inline --- airflow/www/jest-setup.js | 15 ++++ airflow/www/package.json | 1 + .../static/js/grid/api/useMappedInstances.js | 3 + airflow/www/static/js/grid/api/useTasks.js | 9 +- .../www/static/js/grid/api/useTasks.test.jsx | 89 +++++++++++++++++++ .../static/js/grid/details/content/Dag.jsx | 4 +- airflow/www/static/js/grid/index.jsx | 4 +- airflow/www/yarn.lock | 25 ++++++ 8 files changed, 142 insertions(+), 8 deletions(-) create mode 100644 airflow/www/static/js/grid/api/useTasks.test.jsx diff --git a/airflow/www/jest-setup.js b/airflow/www/jest-setup.js index 5ffa8890d55d3..a4679deed25ea 100644 --- a/airflow/www/jest-setup.js +++ b/airflow/www/jest-setup.js @@ -20,6 +20,21 @@ */ import '@testing-library/jest-dom'; +import axios from 'axios'; +import { setLogger } from 'react-query'; + +axios.defaults.adapter = require('axios/lib/adapters/http'); + +axios.interceptors.response.use( + (res) => res.data || res, +); + +setLogger({ + log: console.log, + warn: console.warn, + // ✅ no more errors on the console + error: () => {}, +}); // Mock global objects we use across the app global.stateColors = { diff --git a/airflow/www/package.json b/airflow/www/package.json index bf9cc10dbe75e..5252f360a89b1 100644 --- a/airflow/www/package.json +++ b/airflow/www/package.json @@ -58,6 +58,7 @@ "mini-css-extract-plugin": "1.6.0", "moment": "^2.29.2", "moment-locales-webpack-plugin": "^1.2.0", + "nock": "^13.2.4", "optimize-css-assets-webpack-plugin": "6.0.0", "style-loader": "^1.2.1", "stylelint": "^13.6.1", diff --git a/airflow/www/static/js/grid/api/useMappedInstances.js b/airflow/www/static/js/grid/api/useMappedInstances.js index 35eb1decb636e..cc42f2616de05 100644 --- a/airflow/www/static/js/grid/api/useMappedInstances.js +++ b/airflow/www/static/js/grid/api/useMappedInstances.js @@ -40,7 +40,10 @@ export default function useMappedInstances({ }), { keepPreviousData: true, + initialData: { taskInstances: [], totalEntries: 0 }, refetchInterval: isRefreshOn && autoRefreshInterval * 1000, + // staleTime should be similar to the refresh interval + staleTime: autoRefreshInterval * 1000, }, ); } diff --git a/airflow/www/static/js/grid/api/useTasks.js b/airflow/www/static/js/grid/api/useTasks.js index a635622e2787b..c214444dcb81e 100644 --- a/airflow/www/static/js/grid/api/useTasks.js +++ b/airflow/www/static/js/grid/api/useTasks.js @@ -21,14 +21,15 @@ import axios from 'axios'; import { useQuery } from 'react-query'; import { getMetaValue } from '../../utils'; -const tasksUrl = getMetaValue('tasks_api'); - export default function useTasks() { return useQuery( 'tasks', - () => axios.get(tasksUrl), + () => { + const tasksUrl = getMetaValue('tasks_api'); + return axios.get(tasksUrl); + }, { - placeholderData: { tasks: [] }, + initialData: { tasks: [], totalEntries: 0 }, }, ); } diff --git a/airflow/www/static/js/grid/api/useTasks.test.jsx b/airflow/www/static/js/grid/api/useTasks.test.jsx new file mode 100644 index 0000000000000..9fb5bc677b4b5 --- /dev/null +++ b/airflow/www/static/js/grid/api/useTasks.test.jsx @@ -0,0 +1,89 @@ +/*! + * 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. + */ + +/* global describe, test, expect, beforeEach, afterEach, jest */ + +import React from 'react'; +import { renderHook } from '@testing-library/react-hooks'; +import { QueryClient, QueryClientProvider } from 'react-query'; +import nock from 'nock'; + +import useTasks from './useTasks'; +import * as metaUtils from '../../utils'; + +const Wrapper = ({ children }) => { + const queryClient = new QueryClient({ + defaultOptions: { + queries: { + retry: 0, + }, + }, + }); + return ( + + {children} + + ); +}; + +const fakeUrl = 'http://fake.api'; + +describe('Test useTasks hook', () => { + let spy; + beforeEach(() => { + spy = jest.spyOn(metaUtils, 'getMetaValue').mockReturnValue(`${fakeUrl}`); + }); + + afterEach(() => { + spy.mockRestore(); + nock.cleanAll(); + }); + + test('initialData works normally', async () => { + const scope = nock(fakeUrl) + .get('/') + .reply(200, { totalEntries: 1, tasks: [{ taskId: 'task_id' }] }); + const { result, waitFor } = renderHook(() => useTasks(), { wrapper: Wrapper }); + + expect(result.current.data.totalEntries).toBe(0); + + await waitFor(() => result.current.isFetching); + + expect(result.current.data.totalEntries).toBe(0); + + await waitFor(() => !result.current.isFetching); + + expect(result.current.data.totalEntries).toBe(1); + scope.done(); + }); + + test('initialData persists even if there is an error', async () => { + const scope = nock(fakeUrl) + .get('/') + .replyWithError('something awful happened'); + const { result, waitFor } = renderHook(() => useTasks(), { wrapper: Wrapper }); + + expect(result.current.data.totalEntries).toBe(0); + + await waitFor(() => result.current.isError); + + expect(result.current.data.totalEntries).toBe(0); + scope.done(); + }); +}); diff --git a/airflow/www/static/js/grid/details/content/Dag.jsx b/airflow/www/static/js/grid/details/content/Dag.jsx index e527b494cf283..a3e31b7d31cea 100644 --- a/airflow/www/static/js/grid/details/content/Dag.jsx +++ b/airflow/www/static/js/grid/details/content/Dag.jsx @@ -40,10 +40,8 @@ import { SimpleStatus } from '../../components/StatusBox'; const dagDetailsUrl = getMetaValue('dag_details_url'); const Dag = () => { - const { data: taskData } = useTasks(); + const { data: { tasks, totalEntries } } = useTasks(); const { data: { dagRuns } } = useGridData(); - if (!taskData) return null; - const { tasks = [], totalEntries = '' } = taskData; // Build a key/value object of operator counts, the name is hidden inside of t.classRef.className const operators = {}; diff --git a/airflow/www/static/js/grid/index.jsx b/airflow/www/static/js/grid/index.jsx index b8c6db5324d4b..e106dc85b903c 100644 --- a/airflow/www/static/js/grid/index.jsx +++ b/airflow/www/static/js/grid/index.jsx @@ -45,11 +45,13 @@ shadowRoot.appendChild(mainElement); const queryClient = new QueryClient({ defaultOptions: { queries: { + notifyOnChangeProps: 'tracked', refetchOnWindowFocus: false, retry: 1, retryDelay: 500, - staleTime: 5 * 60 * 1000, // 5 minutes refetchOnMount: true, // Refetches stale queries, not "always" + staleTime: 5 * 60 * 1000, // 5 minutes + initialDataUpdatedAt: new Date().setMinutes(-6), // make sure initial data is already expired }, mutations: { retry: 1, diff --git a/airflow/www/yarn.lock b/airflow/www/yarn.lock index f342f70deb978..18715e9e1ff54 100644 --- a/airflow/www/yarn.lock +++ b/airflow/www/yarn.lock @@ -7794,6 +7794,11 @@ json-stable-stringify-without-jsonify@^1.0.1: resolved "https://registry.yarnpkg.com/json-stable-stringify-without-jsonify/-/json-stable-stringify-without-jsonify-1.0.1.tgz#9db7b59496ad3f3cfef30a75142d2d930ad72651" integrity sha1-nbe1lJatPzz+8wp1FC0tkwrXJlE= +json-stringify-safe@^5.0.1: + version "5.0.1" + resolved "https://registry.yarnpkg.com/json-stringify-safe/-/json-stringify-safe-5.0.1.tgz#1296a2d58fd45f19a0f6ce01d65701e2c735b6eb" + integrity sha512-ZClg6AaYvamvYEE82d3Iyd3vSSIjQ+odgjaTzRuO3s7toCdFKczob2i0zCh7JE8kWn17yvAWhUVxvqGwUalsRA== + json5@^0.5.0, json5@^0.5.1: version "0.5.1" resolved "https://registry.yarnpkg.com/json5/-/json5-0.5.1.tgz#1eade7acc012034ad84e2396767ead9fa5495821" @@ -8009,6 +8014,11 @@ lodash.mergewith@4.6.2: resolved "https://registry.yarnpkg.com/lodash.mergewith/-/lodash.mergewith-4.6.2.tgz#617121f89ac55f59047c7aec1ccd6654c6590f55" integrity sha512-GK3g5RPZWTRSeLSpgP8Xhra+pnjBC56q9FZYe1d5RN3TJ35dbkGy3YqBSMbyCrlbi+CM9Z3Jk5yTL7RCsqboyQ== +lodash.set@^4.3.2: + version "4.3.2" + resolved "https://registry.yarnpkg.com/lodash.set/-/lodash.set-4.3.2.tgz#d8757b1da807dde24816b0d6a84bea1a76230b23" + integrity sha512-4hNPN5jlm/N/HLMCO43v8BXKq9Z7QdAGc/VGrRD61w8gN9g/6jF9A4L1pbUgBLCffi0w9VsXfTOij5x8iTyFvg== + lodash.truncate@^4.4.2: version "4.4.2" resolved "https://registry.yarnpkg.com/lodash.truncate/-/lodash.truncate-4.4.2.tgz#5a350da0b1113b837ecfffd5812cbe58d6eae193" @@ -8543,6 +8553,16 @@ nice-try@^1.0.4: resolved "https://registry.yarnpkg.com/nice-try/-/nice-try-1.0.5.tgz#a3378a7696ce7d223e88fc9b764bd7ef1089e366" integrity sha512-1nh45deeb5olNY7eX82BkPO7SSxR5SSYJiPTrTdFUVYwAl8CKMA5N9PjTYkHiRjisVcxcQ1HXdLhx2qxxJzLNQ== +nock@^13.2.4: + version "13.2.4" + resolved "https://registry.yarnpkg.com/nock/-/nock-13.2.4.tgz#43a309d93143ee5cdcca91358614e7bde56d20e1" + integrity sha512-8GPznwxcPNCH/h8B+XZcKjYPXnUV5clOKCjAqyjsiqA++MpNx9E9+t8YPp0MbThO+KauRo7aZJ1WuIZmOrT2Ug== + dependencies: + debug "^4.1.0" + json-stringify-safe "^5.0.1" + lodash.set "^4.3.2" + propagate "^2.0.0" + node-domexception@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/node-domexception/-/node-domexception-1.0.0.tgz#6888db46a1f71c0b76b3f7555016b63fe64766e5" @@ -9658,6 +9678,11 @@ prop-types@^15.6.2, prop-types@^15.7.2: object-assign "^4.1.1" react-is "^16.13.1" +propagate@^2.0.0: + version "2.0.1" + resolved "https://registry.yarnpkg.com/propagate/-/propagate-2.0.1.tgz#40cdedab18085c792334e64f0ac17256d38f9a45" + integrity sha512-vGrhOavPSTz4QVNuBNdcNXePNdNMaO1xj9yBeH1ScQPjk/rhg9sSlCXPhMkFuaNNW/syTvYqsnbIJxMBfRbbag== + prr@~1.0.1: version "1.0.1" resolved "https://registry.yarnpkg.com/prr/-/prr-1.0.1.tgz#d3fc114ba06995a45ec6893f484ceb1d78f5f476"