[superset] 01/01: refactor: introduce api resource hooks, fix error owners

[su...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

suddjian pushed a commit to branch api-resource-hooks
in repository https://gitbox.apache.org/repos/asf/superset.git

commit ea37f5ed5aabb0fadcff7fd9b9317746b9951396
Author: David Aaron Suddjian <aa...@gmail.com>
AuthorDate: Thu Feb 18 01:31:37 2021 -0800

    refactor: introduce api resource hooks, fix error owners
---
 superset-frontend/src/chart/Chart.jsx              |  15 +-
 superset-frontend/src/chart/ChartErrorMessage.tsx  |  63 +++++++
 superset-frontend/src/common/hooks/apiResources.ts | 184 +++++++++++++++++++++
 .../dashboard/components/gridComponents/Chart.jsx  |   1 -
 .../src/explore/components/ExploreChartPanel.jsx   |   1 -
 5 files changed, 251 insertions(+), 13 deletions(-)

diff --git a/superset-frontend/src/chart/Chart.jsx b/superset-frontend/src/chart/Chart.jsx
index 3f1596c..8bda7a8 100644
--- a/superset-frontend/src/chart/Chart.jsx
+++ b/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}
diff --git a/superset-frontend/src/chart/ChartErrorMessage.tsx b/superset-frontend/src/chart/ChartErrorMessage.tsx
new file mode 100644
index 0000000..123d60f
--- /dev/null
+++ b/superset-frontend/src/chart/ChartErrorMessage.tsx
@@ -0,0 +1,63 @@
+/**
+ * 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 {
+  useApiV1Resource,
+  useResourceTransform,
+} from 'src/common/hooks/apiResources';
+import ErrorMessageWithStackTrace from 'src/components/ErrorMessage/ErrorMessageWithStackTrace';
+import { SupersetError } from 'src/components/ErrorMessage/types';
+import Chart from 'src/types/Chart';
+
+interface Props {
+  chartId: string;
+  error?: SupersetError;
+}
+
+function extractOwnerNames({ owners }: Chart) {
+  if (!owners) return null;
+  return owners.map(owner => `${owner.first_name} ${owner.last_name}`);
+}
+
+function useChartOwnerNames(chartId: string) {
+  const { result } = useResourceTransform(
+    useApiV1Resource<Chart>(`/api/v1/chart/${chartId}`),
+    extractOwnerNames,
+  );
+  return result;
+}
+
+/**
+ * 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 owners = useChartOwnerNames(chartId);
+  // don't mutate props
+  const ownedError = error && {
+    ...error,
+    extra: { ...error.extra, owners },
+  };
+
+  return <ErrorMessageWithStackTrace {...props} error={ownedError} />;
+};
diff --git a/superset-frontend/src/common/hooks/apiResources.ts b/superset-frontend/src/common/hooks/apiResources.ts
new file mode 100644
index 0000000..5ae54a7
--- /dev/null
+++ b/superset-frontend/src/common/hooks/apiResources.ts
@@ -0,0 +1,184 @@
+/**
+ * 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 state will need to keep
+    setResource(initialState);
+
+    // when this effect runs, the endpoint function 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,
+    });
+
+    // pass {} to fetchResource, only because makeApi requires it.
+    // we should change makeApi, and then remove that param.
+    // TODO support request cancellation in makeApi
+    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 useResourceTransform<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 from fetching an api resource
+const innerResult = <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> {
+  // get the api "result" field within the resource result
+  return useResourceTransform(
+    useApiResourceFullBody<{ result: RESULT }>(endpoint),
+    innerResult,
+  );
+}
diff --git a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
index 7dd01e9..b3c7975 100644
--- a/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
+++ b/superset-frontend/src/dashboard/components/gridComponents/Chart.jsx
@@ -361,7 +361,6 @@ export default class Chart extends React.Component {
             timeout={timeout}
             triggerQuery={chart.triggerQuery}
             vizType={slice.viz_type}
-            owners={slice.owners}
           />
         </div>
       </div>
diff --git a/superset-frontend/src/explore/components/ExploreChartPanel.jsx b/superset-frontend/src/explore/components/ExploreChartPanel.jsx
index 6962dfa..813b6a9 100644
--- a/superset-frontend/src/explore/components/ExploreChartPanel.jsx
+++ b/superset-frontend/src/explore/components/ExploreChartPanel.jsx
@@ -198,7 +198,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}