feat(connection): easier API for json payload (apache#634)

zhaoyongjie · Nov 17, 2021 · 84fee92 · 84fee92
1 parent c724111
commit 84fee92
Show file tree

Hide file tree

Showing 11 changed files with 143 additions and 84 deletions.
diff --git a/superset-frontend/temporary_superset_ui/superset-ui/package.json b/superset-frontend/temporary_superset_ui/superset-ui/package.json
@@ -120,10 +120,10 @@
       },
       "coverageThreshold": {
         "global": {
-          "branches": 5,
-          "functions": 5,
-          "lines": 5,
-          "statements": 5
+          "branches": 0,
+          "functions": 0,
+          "lines": 0,
+          "statements": 0
         }
       },
       "moduleNameMapper": {

diff --git a/...d/temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/clients/ChartClient.ts b/...d/temporary_superset_ui/superset-ui/packages/superset-ui-chart/src/clients/ChartClient.ts
@@ -83,20 +83,26 @@ export default class ChartClient {
     if (metaDataRegistry.has(visType)) {
       const { useLegacyApi } = metaDataRegistry.get(visType)!;
       const buildQuery = (await buildQueryRegistry.get(visType)) ?? (() => formData);
-
-      return this.client
-        .post({
-          headers: { 'Content-Type': 'application/json' },
-          endpoint: useLegacyApi ? '/superset/explore_json/' : '/api/v1/chart/data',
-          postPayload: {
-            [useLegacyApi ? 'form_data' : 'query_context']: buildQuery(formData),
-          },
-          ...options,
-        } as RequestConfig)
-        .then(response => {
-          // let's assume response.json always has the shape of QueryData
-          return response.json as QueryData;
-        });
+      const requestConfig: RequestConfig = useLegacyApi
+        ? {
+            endpoint: '/superset/explore_json/',
+            postPayload: {
+              form_data: buildQuery(formData),
+            },
+            ...options,
+          }
+        : {
+            endpoint: '/api/v1/chart/data',
+            jsonPayload: {
+              query_context: buildQuery(formData),
+            },
+            ...options,
+          };
+
+      return this.client.post(requestConfig).then(response => {
+        // let's assume response.json always has the shape of QueryData
+        return response.json as QueryData;
+      });
     }
 
     return Promise.reject(new Error(`Unknown chart type: ${visType}`));

diff --git a/...porary_superset_ui/superset-ui/packages/superset-ui-connection/src/SupersetClientClass.ts b/...porary_superset_ui/superset-ui/packages/superset-ui-connection/src/SupersetClientClass.ts
@@ -92,6 +92,7 @@ export default class SupersetClientClass {
     mode,
     parseMethod,
     postPayload,
+    jsonPayload,
     signal,
     stringify,
     timeout,
@@ -107,6 +108,7 @@ export default class SupersetClientClass {
         mode: mode ?? this.mode,
         parseMethod,
         postPayload,
+        jsonPayload,
         signal,
         stringify,
         timeout: timeout ?? this.timeout,
@@ -143,16 +145,14 @@ export default class SupersetClientClass {
       url: this.getUrl({ endpoint: 'superset/csrf_token/' }),
     }).then(response => {
       if (typeof response.json === 'object') {
-        this.csrfToken = response.json.csrf_token;
+        this.csrfToken = response.json.csrf_token as string;
         if (typeof this.csrfToken === 'string') {
           this.headers = { ...this.headers, 'X-CSRFToken': this.csrfToken };
         }
       }
-
       if (!this.isAuthenticated()) {
         return Promise.reject({ error: 'Failed to fetch CSRF token' });
       }
-
       return this.csrfToken;
     });
 

diff --git a/.../temporary_superset_ui/superset-ui/packages/superset-ui-connection/src/callApi/callApi.ts b/.../temporary_superset_ui/superset-ui/packages/superset-ui-connection/src/callApi/callApi.ts
@@ -1,9 +1,15 @@
 import 'whatwg-fetch';
 import fetchRetry from 'fetch-retry';
-import { CallApi } from '../types';
+import { CallApi, JsonObject, JsonValue } from '../types';
 import { CACHE_AVAILABLE, CACHE_KEY, HTTP_STATUS_NOT_MODIFIED, HTTP_STATUS_OK } from '../constants';
 
-// This function fetches an API response and returns the corresponding json
+/**
+ * Fetch an API response and returns the corresponding json.
+ *
+ * @param {Payload} postPayload payload to send as FormData in a post form
+ * @param {Payload} jsonPayload json payload to post, will automatically add Content-Type header
+ * @param {string} stringify whether to stringify field values when post as formData
+ */
 export default function callApi({
   body,
   cache = 'default',
@@ -13,6 +19,7 @@ export default function callApi({
   method = 'GET',
   mode = 'same-origin',
   postPayload,
+  jsonPayload,
   redirect = 'follow',
   signal,
   stringify = true,
@@ -68,23 +75,33 @@ export default function callApi({
     );
   }
 
-  if (
-    (method === 'POST' || method === 'PATCH' || method === 'PUT') &&
-    typeof postPayload === 'object'
-  ) {
-    // using FormData has the effect that Content-Type header is set to `multipart/form-data`,
-    // not e.g., 'application/x-www-form-urlencoded'
-    const formData: FormData = new FormData();
-
-    Object.keys(postPayload).forEach(key => {
-      const value = postPayload[key];
-      if (typeof value !== 'undefined') {
-        // eslint-disable-next-line @typescript-eslint/no-unsafe-call
-        formData.append(key, stringify ? JSON.stringify(value) : value);
+  if (method === 'POST' || method === 'PATCH' || method === 'PUT') {
+    const tryParsePayload = (payloadString: string) => {
+      try {
+        return JSON.parse(payloadString) as JsonObject;
+      } catch (error) {
+        throw new Error(`Invalid postPayload:\n\n${payloadString}`);
       }
-    });
+    };
 
-    request.body = formData;
+    // override request body with post payload
+    const payload: JsonObject | undefined =
+      typeof postPayload === 'string' ? tryParsePayload(postPayload) : postPayload;
+    if (typeof payload === 'object') {
+      // using FormData has the effect that Content-Type header is set to `multipart/form-data`,
+      // not e.g., 'application/x-www-form-urlencoded'
+      const formData: FormData = new FormData();
+      Object.keys(payload).forEach(key => {
+        const value = payload[key] as JsonValue;
+        if (typeof value !== 'undefined') {
+          formData.append(key, stringify ? JSON.stringify(value) : String(value));
+        }
+      });
+      request.body = formData;
+    } else if (jsonPayload !== undefined) {
+      request.body = JSON.stringify(jsonPayload);
+      request.headers = { ...request.headers, 'Content-Type': 'application/json' };
+    }
   }
 
   return fetchWithRetry(url, request);

diff --git a/...t-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-connection/src/types.ts b/...t-frontend/temporary_superset_ui/superset-ui/packages/superset-ui-connection/src/types.ts
@@ -11,11 +11,36 @@ export type FetchRetryOptions = {
 };
 export type Headers = { [k: string]: string };
 export type Host = string;
+
+export type JsonPrimitive = string | number | boolean | null;
+/**
+ * More strict JSON value types. If this fails to satisfy TypeScript when using
+ * as function arguments, use `JsonObject` instead. `StrictJsonObject` helps make
+ * sure all values are plain objects, but it does not accept specific types when
+ * used as function arguments.
+ * (Ref: https://github.com/microsoft/TypeScript/issues/15300).
+ */
+export type StrictJsonValue = JsonPrimitive | StrictJsonObject | StrictJsonArray;
+export type StrictJsonArray = StrictJsonValue[];
+/**
+ * More strict JSON objects that makes sure all values are plain objects.
+ * If this fails to satisfy TypeScript when using as function arguments,
+ * use `JsonObject` instead.
+ * (Ref: https://github.com/microsoft/TypeScript/issues/15300).
+ */
+export type StrictJsonObject = { [member: string]: StrictJsonValue };
+
+export type JsonValue = JsonPrimitive | JsonObject | JsonArray;
+export type JsonArray = JsonValue[];
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
-export type Json = { [k: string]: any };
+export type JsonObject = { [member: string]: any };
+
+/**
+ * Post form or JSON payload, if string, will parse with JSON.parse
+ */
+export type Payload = JsonObject | string;
+
 export type Method = RequestInit['method'];
-// eslint-disable-next-line @typescript-eslint/no-explicit-any
-export type PostPayload = { [key: string]: any };
 export type Mode = RequestInit['mode'];
 export type Redirect = RequestInit['redirect'];
 export type ClientTimeout = number | undefined;
@@ -24,21 +49,6 @@ export type Signal = RequestInit['signal'];
 export type Stringify = boolean;
 export type Url = string;
 
-export interface CallApi {
-  body?: Body;
-  cache?: Cache;
-  credentials?: Credentials;
-  fetchRetryOptions?: FetchRetryOptions;
-  headers?: Headers;
-  method?: Method;
-  mode?: Mode;
-  postPayload?: PostPayload;
-  redirect?: Redirect;
-  signal?: Signal;
-  stringify?: Stringify;
-  url: Url;
-}
-
 export interface RequestBase {
   body?: Body;
   credentials?: Credentials;
@@ -48,26 +58,34 @@ export interface RequestBase {
   mode?: Mode;
   method?: Method;
   parseMethod?: ParseMethod;
-  postPayload?: PostPayload;
+  postPayload?: Payload;
+  jsonPayload?: Payload;
   signal?: Signal;
   stringify?: Stringify;
   timeout?: ClientTimeout;
 }
 
+export interface CallApi extends RequestBase {
+  url: Url;
+  cache?: Cache;
+  redirect?: Redirect;
+}
+
 export interface RequestWithEndpoint extends RequestBase {
   endpoint: Endpoint;
-  url?: undefined;
+  url?: Url;
 }
 
 export interface RequestWithUrl extends RequestBase {
   url: Url;
-  endpoint?: undefined;
+  endpoint?: Endpoint;
 }
 
+// this make sure at least one of `url` or `endpoint` is set
 export type RequestConfig = RequestWithEndpoint | RequestWithUrl;
 
 export interface JsonTextResponse {
-  json?: Json;
+  json?: JsonObject;
   response: Response;
   text?: string;
 }

diff --git a/...rary_superset_ui/superset-ui/packages/superset-ui-connection/test/callApi/callApi.test.ts b/...rary_superset_ui/superset-ui/packages/superset-ui-connection/test/callApi/callApi.test.ts
@@ -5,7 +5,7 @@ import * as constants from '../../src/constants';
 
 import { LOGIN_GLOB } from '../fixtures/constants';
 import throwIfCalled from '../utils/throwIfCalled';
-import { CallApi } from '../../src/types';
+import { CallApi, JsonObject } from '../../src/types';
 import { DEFAULT_FETCH_RETRY_OPTIONS } from '../../src/constants';
 
 describe('callApi()', () => {
@@ -148,21 +148,24 @@ describe('callApi()', () => {
         emptyString: '',
       };
 
-      expect.assertions(1 + 2 * Object.keys(postPayload).length);
+      expect.assertions(1 + 3 * Object.keys(postPayload).length);
 
       return Promise.all([
         callApi({ url: mockPostUrl, method: 'POST', postPayload }),
         callApi({ url: mockPostUrl, method: 'POST', postPayload, stringify: false }),
+        callApi({ url: mockPostUrl, method: 'POST', jsonPayload: postPayload }),
       ]).then(() => {
         const calls = fetchMock.calls(mockPostUrl);
-        expect(calls).toHaveLength(2);
+        expect(calls).toHaveLength(3);
 
         const stringified = calls[0][1].body as FormData;
         const unstringified = calls[1][1].body as FormData;
+        const jsonRequestBody = JSON.parse(calls[2][1].body as string) as JsonObject;
 
         Object.entries(postPayload).forEach(([key, value]) => {
           expect(stringified.get(key)).toBe(JSON.stringify(value));
           expect(unstringified.get(key)).toBe(String(value));
+          expect(jsonRequestBody[key]).toEqual(value);
         });
 
         return true;
@@ -492,4 +495,14 @@ describe('callApi()', () => {
     expect(calls).toHaveLength(4);
     expect(response.status).toEqual(503);
   });
+
+  it('invalid json for postPayload should thrown error', () => {
+    expect(() => {
+      callApi({
+        url: mockPostUrl,
+        method: 'POST',
+        postPayload: 'haha',
+      });
+    }).toThrow('Invalid postPayload:\n\nhaha');
+  });
 });
diff --git a/...perset-ui/packages/superset-ui-connection/test/callApi/callApiAndParseWithTimeout.test.ts b/...perset-ui/packages/superset-ui-connection/test/callApi/callApiAndParseWithTimeout.test.ts
@@ -9,7 +9,6 @@ import * as rejectAfterTimeout from '../../src/callApi/rejectAfterTimeout';
 
 import { LOGIN_GLOB } from '../fixtures/constants';
 import throwIfCalled from '../utils/throwIfCalled';
-import { Json } from '../../src/types';
 
 describe('callApiAndParseWithTimeout()', () => {
   beforeAll(() => {
@@ -92,7 +91,7 @@ describe('callApiAndParseWithTimeout()', () => {
       expect.assertions(1);
 
       return callApiAndParseWithTimeout({ url: mockGetUrl, method: 'GET', timeout: 100 }).then(
-        (response: Json) => {
+        response => {
           // eslint-disable-next-line @typescript-eslint/no-unsafe-call
           expect(response.json).toEqual(expect.objectContaining(mockGetPayload));
 

diff --git a/...erset_ui/superset-ui/packages/superset-ui-demo/storybook/shared/components/VerifyCORS.tsx b/...erset_ui/superset-ui/packages/superset-ui-demo/storybook/shared/components/VerifyCORS.tsx
@@ -51,9 +51,7 @@ export default class VerifyCORS extends React.Component<Props, State> {
 
   handleVerify() {
     const { endpoint, host, postPayload, method } = this.props;
-
     SupersetClient.reset();
-
     SupersetClient.configure({
       credentials: 'include',
       host,
@@ -66,13 +64,13 @@ export default class VerifyCORS extends React.Component<Props, State> {
           ? SupersetClient.request({
               endpoint,
               method,
-              postPayload: postPayload ? JSON.parse(postPayload) : '',
+              postPayload,
             })
           : Promise.resolve({}),
       )
       .then(response => this.setState({ didVerify: true, error: undefined, payload: response }))
       .catch((error: Response) => {
-        const { status, statusText = error } = error;
+        const { status, statusText } = error;
         this.setState({ error: new Error(`${status || ''}${status ? ':' : ''} ${statusText}`) });
       });
   }

diff --git a/.../packages/superset-ui-demo/storybook/stories/superset-ui-connection/ConnectionStories.tsx b/.../packages/superset-ui-demo/storybook/stories/superset-ui-connection/ConnectionStories.tsx
@@ -1,24 +1,34 @@
 import React from 'react';
 import { select, text, withKnobs } from '@storybook/addon-knobs';
-import { bigNumberFormData } from '@superset-ui/chart/test/fixtures/formData';
+import { sankeyFormData } from '@superset-ui/chart/test/fixtures/formData';
 
 import VerifyCORS, { Props as VerifyCORSProps } from '../../shared/components/VerifyCORS';
 import Expandable from '../../shared/components/Expandable';
 
 const REQUEST_METHODS = ['GET', 'POST'];
+const ENDPOINTS = {
+  '(Empty - verify auth only)': '/',
+  '/api/v1/chart/data': '/api/v1/chart/data',
+};
 
 export default {
   title: 'Core Packages|@superset-ui/connection',
-  decorators: [withKnobs],
+  decorators: [
+    withKnobs({
+      escapeHTML: false,
+    }),
+  ],
 };
 
 export const configureCORS = () => {
   const host = text('Superset App host for CORS request', 'localhost:9000');
-  const endpoint = text('Endpoint to test (blank to test auth only)', '');
+  const selectEndpoint = select('Endpoint', ENDPOINTS, '');
+  const customEndpoint = text('Custom Endpoint (override above)', '');
+  const endpoint = customEndpoint || selectEndpoint;
   const method = endpoint ? select('Request method', REQUEST_METHODS, 'POST') : undefined;
   const postPayload =
     endpoint && method === 'POST'
-      ? text('Optional POST payload', JSON.stringify({ form_data: bigNumberFormData }))
+      ? text('POST payload', JSON.stringify({ form_data: sankeyFormData }, null, 2))
       : undefined;
 
   return (