refactor: Introduce api resource hooks, fetch owners for chart errors (#13218)

* refactor: introduce api resource hooks, fix error owners

* comment

* vestigial comment

* not a function

* clarification

* vestigial comment

* filter the api for a leaner response

* reorganize code, add resource hook catalog

* better names

* implement state management in redux

* Revert "implement state management in redux"

This reverts commit d64d50a99f.

* add tests for hooks

superset-frontend/src/chart/Chart.jsx

@@ -25,9 +25,9 @@ import { isFeatureEnabled, FeatureFlag } from 'src/featureFlags';
 import { Logger, LOG_ACTIONS_RENDER_CHART } from '../logger/LogUtils';
 import Loading from '../components/Loading';
 import RefreshChartOverlay from '../components/RefreshChartOverlay';
-import ErrorMessageWithStackTrace from '../components/ErrorMessage/ErrorMessageWithStackTrace';
 import ErrorBoundary from '../components/ErrorBoundary';
 import ChartRenderer from './ChartRenderer';
+import { ChartErrorMessage } from './ChartErrorMessage';
 
 const propTypes = {
   annotationData: PropTypes.object,
@@ -49,9 +49,6 @@ const propTypes = {
   timeout: PropTypes.number,
   vizType: PropTypes.string.isRequired,
   triggerRender: PropTypes.bool,
-  owners: PropTypes.arrayOf(
-    PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
-  ),
   // state
   chartAlert: PropTypes.string,
   chartStatus: PropTypes.string,
@@ -152,17 +149,13 @@ class Chart extends React.PureComponent {
   }
 
   renderErrorMessage(queryResponse) {
-    const { chartAlert, chartStackTrace, dashboardId, owners } = this.props;
+    const { chartId, chartAlert, chartStackTrace, dashboardId } = this.props;
 
     const error = queryResponse?.errors?.[0];
-    if (error) {
-      const extra = error.extra || {};
-      extra.owners = owners;
-      error.extra = extra;
-    }
     const message = chartAlert || queryResponse?.message;
     return (
-      <ErrorMessageWithStackTrace
+      <ChartErrorMessage
+        chartId={chartId}
         error={error}
         subtitle={message}
         copyText={message}

superset-frontend/src/chart/ChartErrorMessage.tsx

@@ -0,0 +1,47 @@
+/**
+ * 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 React from 'react';
+import { useChartOwnerNames } from 'src/common/hooks/apiResources';
+import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
+import { SupersetError } from 'src/components/ErrorMessage/types';
+
+interface Props {
+  chartId: string;
+  error?: SupersetError;
+}
+
+/**
+ * fetches the chart owners and adds them to the extra data of the error message
+ */
+export const ChartErrorMessage: React.FC<Props> = ({
+  chartId,
+  error,
+  ...props
+}) => {
+  const { result: owners } = useChartOwnerNames(chartId);
+
+  // don't mutate props
+  const ownedError = error && {
+    ...error,
+    extra: { ...error.extra, owners },
+  };
+
+  return <ErrorMessageWithStackTrace {...props} error={ownedError} />;
+};

superset-frontend/src/common/hooks/apiResources/apiResources.test.ts

@@ -0,0 +1,165 @@
+/**
+ * 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 { makeApi } from '@superset-ui/core';
+import { act, renderHook } from '@testing-library/react-hooks';
+import {
+  ResourceStatus,
+  useApiResourceFullBody,
+  useApiV1Resource,
+  useTransformedResource,
+} from './apiResources';
+
+const fakeApiResult = {
+  id: 1,
+  name: 'fake api result',
+};
+
+const nameToAllCaps = (thing: any) => ({
+  ...thing,
+  name: thing.name.toUpperCase(),
+});
+
+jest.mock('@superset-ui/core', () => ({
+  ...jest.requireActual<any>('@superset-ui/core'),
+  makeApi: jest
+    .fn()
+    .mockReturnValue(jest.fn().mockResolvedValue(fakeApiResult)),
+}));
+
+describe('apiResource hooks', () => {
+  beforeAll(() => {
+    jest.useFakeTimers();
+  });
+
+  afterAll(() => {
+    jest.useRealTimers();
+  });
+
+  describe('useApiResourceFullBody', () => {
+    it('returns a loading state at the start', async () => {
+      const { result } = renderHook(() =>
+        useApiResourceFullBody('/test/endpoint'),
+      );
+      expect(result.current).toEqual({
+        status: ResourceStatus.LOADING,
+        result: null,
+        error: null,
+      });
+      await act(async () => {
+        jest.runAllTimers();
+      });
+    });
+
+    it('resolves to the value from the api', async () => {
+      const { result } = renderHook(() =>
+        useApiResourceFullBody('/test/endpoint'),
+      );
+      await act(async () => {
+        jest.runAllTimers();
+      });
+      expect(result.current).toEqual({
+        status: ResourceStatus.COMPLETE,
+        result: fakeApiResult,
+        error: null,
+      });
+    });
+
+    it('handles api errors', async () => {
+      const fakeError = new Error('fake api error');
+      (makeApi as any).mockReturnValue(jest.fn().mockRejectedValue(fakeError));
+      const { result } = renderHook(() =>
+        useApiResourceFullBody('/test/endpoint'),
+      );
+      await act(async () => {
+        jest.runAllTimers();
+      });
+      expect(result.current).toEqual({
+        status: ResourceStatus.ERROR,
+        result: null,
+        error: fakeError,
+      });
+    });
+  });
+
+  describe('useTransformedResource', () => {
+    it('applies a transformation to the resource', () => {
+      const { result } = renderHook(() =>
+        useTransformedResource(
+          {
+            status: ResourceStatus.COMPLETE,
+            result: fakeApiResult,
+            error: null,
+          },
+          nameToAllCaps,
+        ),
+      );
+      expect(result.current).toEqual({
+        status: ResourceStatus.COMPLETE,
+        result: {
+          id: 1,
+          name: 'FAKE API RESULT',
+        },
+        error: null,
+      });
+    });
+
+    it('works while loading', () => {
+      const nameToAllCaps = (thing: any) => ({
+        ...thing,
+        name: thing.name.toUpperCase(),
+      });
+      const { result } = renderHook(() =>
+        useTransformedResource(
+          {
+            status: ResourceStatus.LOADING,
+            result: null,
+            error: null,
+          },
+          nameToAllCaps,
+        ),
+      );
+      expect(result.current).toEqual({
+        status: ResourceStatus.LOADING,
+        result: null,
+        error: null,
+      });
+    });
+  });
+
+  describe('useApiV1Endpoint', () => {
+    it('resolves to the value from the api', async () => {
+      (makeApi as any).mockReturnValue(
+        jest.fn().mockResolvedValue({
+          meta: 'data',
+          count: 1,
+          result: fakeApiResult,
+        }),
+      );
+      const { result } = renderHook(() => useApiV1Resource('/test/endpoint'));
+      await act(async () => {
+        jest.runAllTimers();
+      });
+      expect(result.current).toEqual({
+        status: ResourceStatus.COMPLETE,
+        result: fakeApiResult,
+        error: null,
+      });
+    });
+  });
+});

superset-frontend/src/common/hooks/apiResources/apiResources.ts

@@ -0,0 +1,181 @@
+/**
+ * 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 { makeApi } from '@superset-ui/core';
+import { useEffect, useMemo, useRef, useState } from 'react';
+
+export enum ResourceStatus {
+  LOADING = 'loading',
+  COMPLETE = 'complete',
+  ERROR = 'error',
+}
+
+/**
+ * An object containing the data fetched from the API,
+ * as well as loading and error info
+ */
+export type Resource<T> = LoadingState | CompleteState<T> | ErrorState;
+
+// Trying out something a little different: a separate type per status.
+// This should let Typescript know whether a Resource has a result or error.
+// It's possible that I'm expecting too much from Typescript here.
+// If this ends up causing problems, we can change the type to:
+//
+// export type Resource<T> = {
+//   status: ResourceStatus;
+//   result: null | T;
+//   error: null | Error;
+// }
+
+type LoadingState = {
+  status: ResourceStatus.LOADING;
+  result: null;
+  error: null;
+};
+
+type CompleteState<T> = {
+  status: ResourceStatus.COMPLETE;
+  result: T;
+  error: null;
+};
+
+type ErrorState = {
+  status: ResourceStatus.ERROR;
+  result: null;
+  error: Error;
+};
+
+const initialState: LoadingState = {
+  status: ResourceStatus.LOADING,
+  result: null,
+  error: null,
+};
+
+/**
+ * A general-purpose hook to fetch the response from an endpoint.
+ * Returns the full response body from the API, including metadata.
+ *
+ * Note: You likely want {useApiV1Resource} instead of this!
+ *
+ * TODO Store the state in redux or something, share state between hook instances.
+ *
+ * TODO Include a function in the returned resource object to refresh the data.
+ *
+ * A core design decision here is composition > configuration,
+ * and every hook should only have one job.
+ * Please address new needs with new hooks if possible,
+ * rather than adding config options to this hook.
+ *
+ * @param endpoint The url where the resource is located.
+ */
+export function useApiResourceFullBody<RESULT>(
+  endpoint: string,
+): Resource<RESULT> {
+  const [resource, setResource] = useState<Resource<RESULT>>(initialState);
+  const cancelRef = useRef<() => void>(() => {});
+
+  useEffect(() => {
+    // If refresh is implemented, this will need to change.
+    // The previous values should stay during refresh.
+    setResource(initialState);
+
+    // when this effect runs, the endpoint has changed.
+    // cancel any current calls so that state doesn't get messed up.
+    cancelRef.current();
+    let cancelled = false;
+    cancelRef.current = () => {
+      cancelled = true;
+    };
+
+    const fetchResource = makeApi<{}, RESULT>({
+      method: 'GET',
+      endpoint,
+    });
+
+    fetchResource({})
+      .then(result => {
+        if (!cancelled) {
+          setResource({
+            status: ResourceStatus.COMPLETE,
+            result,
+            error: null,
+          });
+        }
+      })
+      .catch(error => {
+        if (!cancelled) {
+          setResource({
+            status: ResourceStatus.ERROR,
+            result: null,
+            error,
+          });
+        }
+      });
+
+    // Cancel the request when the component un-mounts
+    return () => {
+      cancelled = true;
+    };
+  }, [endpoint]);
+
+  return resource;
+}
+
+/**
+ * For when you want to transform the result of an api resource hook before using it.
+ *
+ * @param resource the Resource object returned from useApiV1Resource
+ * @param transformFn a callback that transforms the result object into the shape you want.
+ * Make sure to use a persistent function for this so it doesn't constantly recalculate!
+ */
+export function useTransformedResource<IN, OUT>(
+  resource: Resource<IN>,
+  transformFn: (result: IN) => OUT,
+): Resource<OUT> {
+  return useMemo(() => {
+    if (resource.status !== ResourceStatus.COMPLETE) {
+      // While incomplete, there is no result - no need to transform.
+      return resource;
+    }
+    return {
+      ...resource,
+      result: transformFn(resource.result),
+    };
+  }, [resource, transformFn]);
+}
+
+// returns the "result" field from a fetched API v1 endpoint
+const extractInnerResult = <T>(responseBody: { result: T }) =>
+  responseBody.result;
+
+/**
+ * A general-purpose hook to fetch a Superset resource from a v1 API endpoint.
+ * Handles request lifecycle and async logic so you don't have to.
+ *
+ * This returns the data under the "result" field in the API response body.
+ * If you need the full response body, use {useFullApiResource} instead.
+ *
+ * @param endpoint The url where the resource is located.
+ */
+export function useApiV1Resource<RESULT>(endpoint: string): Resource<RESULT> {
+  return useTransformedResource(
+    useApiResourceFullBody<{ result: RESULT }>(endpoint),
+    extractInnerResult,
+  );
+}

superset-frontend/src/common/hooks/apiResources/charts.ts

@@ -0,0 +1,39 @@
+/**
+ * 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 rison from 'rison';
+import Chart from 'src/types/Chart';
+import { useApiV1Resource, useTransformedResource } from './apiResources';
+
+function extractOwnerNames({ owners }: Chart) {
+  if (!owners) return null;
+  return owners.map(owner => `${owner.first_name} ${owner.last_name}`);
+}
+
+const ownerNamesQuery = rison.encode({
+  columns: ['owners.first_name', 'owners.last_name'],
+  keys: ['none'],
+});
+
+export function useChartOwnerNames(chartId: string) {
+  return useTransformedResource(
+    useApiV1Resource<Chart>(`/api/v1/chart/${chartId}?q=${ownerNamesQuery}`),
+    extractOwnerNames,
+  );
+}

superset-frontend/src/common/hooks/apiResources/index.ts

@@ -0,0 +1,29 @@
+/**
+ * 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.
+ */
+
+export {
+  useApiResourceFullBody,
+  useApiV1Resource,
+  useTransformedResource,
+} from './apiResources';
+
+// A central catalog of API Resource hooks.
+// Add new API hooks here, organized under
+// different files for different resource types.
+export { useChartOwnerNames } from './charts';

superset-frontend/src/dashboard/components/gridComponents/Chart.jsx

@@ -364,7 +364,6 @@ export default class Chart extends React.Component {
             timeout={timeout}
             triggerQuery={chart.triggerQuery}
             vizType={slice.viz_type}
-            owners={slice.owners}
           />
         </div>
       </div>

superset-frontend/src/explore/components/ExploreChartPanel.jsx

@@ -199,7 +199,6 @@ const ExploreChartPanel = props => {
           errorMessage={props.errorMessage}
           formData={props.form_data}
           onQuery={props.onQuery}
-          owners={props?.slice?.owners}
           queriesResponse={chart.queriesResponse}
           refreshOverlayVisible={props.refreshOverlayVisible}
           setControlValue={props.actions.setControlValue}