From cb1ad0ba5f167488c7c0ce3afe3be73dc30089b5 Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Wed, 1 Jul 2020 00:07:56 -0700 Subject: [PATCH 1/9] feat(query): add makeApi API generator Introduce a makeApi API generator to easily add typed query methods for new (and existing) API endpoints. --- .../shared/components/VerifyCORS.tsx | 32 +++-- packages/superset-ui-query/README.md | 11 +- .../src/api/legacy/fetchExploreJson.ts | 18 +-- .../src/api/legacy/getDatasourceMetadata.ts | 4 +- .../src/api/legacy/getFormData.ts | 6 +- .../superset-ui-query/src/api/legacy/index.ts | 2 + .../superset-ui-query/src/api/legacy/types.ts | 22 ++- packages/superset-ui-query/src/api/types.ts | 18 +++ .../src/api/v1/handleError.ts | 81 ++++++++++++ .../superset-ui-query/src/api/v1/index.ts | 35 ++++- .../superset-ui-query/src/api/v1/makeApi.ts | 94 +++++++++++++ .../src/api/v1/postChartData.ts | 21 --- .../superset-ui-query/src/api/v1/types.ts | 103 ++++++++++++++- .../src/extractQueryFields.ts | 1 + packages/superset-ui-query/src/index.ts | 27 +++- .../src/types/QueryFormData.ts | 2 + .../test/api/v1/getChartData.test.ts | 48 +++++++ .../test/api/v1/handleError.test.ts | 77 +++++++++++ .../test/api/v1/makeApi.test.ts | 125 ++++++++++++++++++ .../test/api/v1/postChartData.test.ts | 38 ------ 20 files changed, 654 insertions(+), 111 deletions(-) create mode 100644 packages/superset-ui-query/src/api/v1/handleError.ts create mode 100644 packages/superset-ui-query/src/api/v1/makeApi.ts delete mode 100644 packages/superset-ui-query/src/api/v1/postChartData.ts create mode 100644 packages/superset-ui-query/test/api/v1/getChartData.test.ts create mode 100644 packages/superset-ui-query/test/api/v1/handleError.test.ts create mode 100644 packages/superset-ui-query/test/api/v1/makeApi.test.ts delete mode 100644 packages/superset-ui-query/test/api/v1/postChartData.test.ts diff --git a/packages/superset-ui-demo/storybook/shared/components/VerifyCORS.tsx b/packages/superset-ui-demo/storybook/shared/components/VerifyCORS.tsx index 95d8fe7833..e057e137f8 100644 --- a/packages/superset-ui-demo/storybook/shared/components/VerifyCORS.tsx +++ b/packages/superset-ui-demo/storybook/shared/components/VerifyCORS.tsx @@ -1,18 +1,19 @@ import React, { ReactNode } from 'react'; -import { SupersetClient } from '@superset-ui/connection'; +import { SupersetClient, Method } from '@superset-ui/connection'; +import { makeApi, SupersetApiError } from '@superset-ui/query'; import ErrorMessage from './ErrorMessage'; export type Props = { children: ({ payload }: { payload?: object }) => ReactNode; endpoint?: string; host: string; - method?: 'POST' | 'GET'; + method?: Method; postPayload?: string; }; type State = { didVerify: boolean; - error?: Error; + error?: Error | SupersetApiError; payload?: object; }; @@ -58,21 +59,18 @@ export default class VerifyCORS extends React.Component { mode: 'cors', }) .init() - .then(() => + .then(() => { // Test an endpoint if specified - endpoint - ? SupersetClient.request({ - endpoint, - method, - postPayload, - }) - : Promise.resolve({}), - ) - .then(response => this.setState({ didVerify: true, error: undefined, payload: response })) - .catch((error: Response) => { - const { status, statusText } = error; - this.setState({ error: new Error(`${status || ''}${status ? ':' : ''} ${statusText}`) }); - }); + if (endpoint && postPayload) { + return makeApi({ + endpoint, + method, + })(postPayload); + } + return { error: 'Must provide valid endpoint and payload.' }; + }) + .then(result => this.setState({ didVerify: true, error: undefined, payload: result })) + .catch(error => this.setState({ error })); } render() { diff --git a/packages/superset-ui-query/README.md b/packages/superset-ui-query/README.md index c3cf310438..f784bc6485 100644 --- a/packages/superset-ui-query/README.md +++ b/packages/superset-ui-query/README.md @@ -3,7 +3,7 @@ [![Version](https://img.shields.io/npm/v/@superset-ui/query.svg?style=flat)](https://img.shields.io/npm/v/@superset-ui/query.svg?style=flat) [![David (path)](https://img.shields.io/david/apache-superset/superset-ui.svg?path=packages%2Fsuperset-ui-query&style=flat-square)](https://david-dm.org/apache-superset/superset-ui?path=packages/superset-ui-query) -Description +Utility to build query object for chart data. #### Example usage @@ -13,11 +13,4 @@ import { xxx } from '@superset-ui/query'; #### API -`fn(args)` - -- Do something - -### Development - -`@data-ui/build-config` is used to manage the build configuration for this package including babel -builds, jest testing, eslint, and prettier. +TODO diff --git a/packages/superset-ui-query/src/api/legacy/fetchExploreJson.ts b/packages/superset-ui-query/src/api/legacy/fetchExploreJson.ts index f858455f7d..cd6871cae3 100644 --- a/packages/superset-ui-query/src/api/legacy/fetchExploreJson.ts +++ b/packages/superset-ui-query/src/api/legacy/fetchExploreJson.ts @@ -1,27 +1,27 @@ -import { SupersetClient, RequestConfig } from '@superset-ui/connection'; +import { SupersetClient, Method, Url } from '@superset-ui/connection'; import { QueryFormData } from '../../types/QueryFormData'; import { LegacyChartDataResponse } from './types'; import { BaseParams } from '../types'; export interface Params extends BaseParams { - method?: 'GET' | 'POST'; - url?: string; + method?: Method; + url?: Url; formData: QueryFormData; } -export default function fetchExploreJson({ +export default async function fetchExploreJson({ client = SupersetClient, method = 'POST', requestConfig, url = '/superset/explore_json/', formData, }: Params) { - const fetchFunc = method === 'GET' ? client.get : client.post; - - return fetchFunc({ + const { json } = await client.request({ ...requestConfig, - // TODO: Have to transform formData as query string for GET + method, url, + // TODO: Have to transform formData as query string for GET postPayload: { form_data: formData }, - } as RequestConfig).then(({ json }) => json as LegacyChartDataResponse); + }); + return json as LegacyChartDataResponse; } diff --git a/packages/superset-ui-query/src/api/legacy/getDatasourceMetadata.ts b/packages/superset-ui-query/src/api/legacy/getDatasourceMetadata.ts index 43fc3779d7..200e93e540 100644 --- a/packages/superset-ui-query/src/api/legacy/getDatasourceMetadata.ts +++ b/packages/superset-ui-query/src/api/legacy/getDatasourceMetadata.ts @@ -1,4 +1,4 @@ -import { SupersetClient, RequestConfig } from '@superset-ui/connection'; +import { SupersetClient } from '@superset-ui/connection'; import { Datasource } from '../../types/Datasource'; import { BaseParams } from '../types'; @@ -15,6 +15,6 @@ export default function getDatasourceMetadata({ .get({ endpoint: `/superset/fetch_datasource_metadata?datasourceKey=${datasourceKey}`, ...requestConfig, - } as RequestConfig) + }) .then(response => response.json as Datasource); } diff --git a/packages/superset-ui-query/src/api/legacy/getFormData.ts b/packages/superset-ui-query/src/api/legacy/getFormData.ts index 2d709ab29b..5f484993b7 100644 --- a/packages/superset-ui-query/src/api/legacy/getFormData.ts +++ b/packages/superset-ui-query/src/api/legacy/getFormData.ts @@ -1,4 +1,4 @@ -import { SupersetClient, RequestConfig } from '@superset-ui/connection'; +import { SupersetClient } from '@superset-ui/connection'; import { BaseParams } from '../types'; import { QueryFormData } from '../../types/QueryFormData'; @@ -17,8 +17,8 @@ export default function getFormData({ .get({ endpoint: `/api/v1/form_data/?slice_id=${sliceId}`, ...requestConfig, - } as RequestConfig) - .then(response => response.json as QueryFormData); + }) + .then(({ json }) => json as QueryFormData); return overrideFormData ? promise.then(formData => ({ ...formData, ...overrideFormData })) diff --git a/packages/superset-ui-query/src/api/legacy/index.ts b/packages/superset-ui-query/src/api/legacy/index.ts index 402d01f2c3..874df6f910 100644 --- a/packages/superset-ui-query/src/api/legacy/index.ts +++ b/packages/superset-ui-query/src/api/legacy/index.ts @@ -1,3 +1,5 @@ export { default as fetchExploreJson } from './fetchExploreJson'; export { default as getFormData } from './getFormData'; export { default as getDatasourceMetadata } from './getDatasourceMetadata'; + +export * from './types'; diff --git a/packages/superset-ui-query/src/api/legacy/types.ts b/packages/superset-ui-query/src/api/legacy/types.ts index 863e6f4b73..5f4d715df1 100644 --- a/packages/superset-ui-query/src/api/legacy/types.ts +++ b/packages/superset-ui-query/src/api/legacy/types.ts @@ -1,5 +1,23 @@ -import { V1ChartDataResponseResult } from '../v1/types'; +/** + * 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 { ChartDataResponseResult } from '../v1/types'; -export interface LegacyChartDataResponse extends Omit { +export interface LegacyChartDataResponse extends Omit { data: Record[] | Record; } diff --git a/packages/superset-ui-query/src/api/types.ts b/packages/superset-ui-query/src/api/types.ts index fd48e9953d..90eddc9277 100644 --- a/packages/superset-ui-query/src/api/types.ts +++ b/packages/superset-ui-query/src/api/types.ts @@ -1,3 +1,21 @@ +/** + * 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 { RequestConfig, SupersetClientInterface, diff --git a/packages/superset-ui-query/src/api/v1/handleError.ts b/packages/superset-ui-query/src/api/v1/handleError.ts new file mode 100644 index 0000000000..889ff7ff48 --- /dev/null +++ b/packages/superset-ui-query/src/api/v1/handleError.ts @@ -0,0 +1,81 @@ +/** + * 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 { + SupersetApiError, + SupersetApiErrorPayload, + SupersetApiHttpErrorPayload, + SupersetApiHttpMultiErrorsPayload, +} from './types'; + +export type ErrorType = string | Error | Response | SupersetApiErrorPayload; + +/** + * Handle API request errors, convert to consistent Superset API error. + * @param error the catched error from SupersetClient.request(...) + */ +export default async function handleError(error: ErrorType): Promise { + // string is the error message itself + if (typeof error === 'string') { + throw new SupersetApiError({ message: error }); + } + // catch HTTP errors + if (error instanceof Response) { + const { status, statusText } = error; + if (status >= 400) { + let errorMessage = `${status} ${statusText}`; + try { + const json = (await error.json()) as + | SupersetApiHttpErrorPayload + | SupersetApiHttpMultiErrorsPayload; + const err = 'errors' in json ? json.errors[0] : json; + errorMessage = err.message || err.error_type || errorMessage; + } catch (error_) { + // pass + } + throw new SupersetApiError({ + status, + statusText, + message: errorMessage, + }); + } + } + // JS errors, normally happens before request was sent + if (error instanceof Error) { + throw new SupersetApiError({ + message: error.message || 'Unknown Error', + stack: error.stack, + // pass along the raw error so consumer code can inspect trace stack + originalError: error, + }); + } + // when API returns 200 but operation fails + // (see Python API json_error_response(...)) + if ('error' in error) { + const { error: message, ...rest } = error; + throw new SupersetApiError({ + message, + ...rest, + }); + } + // all unknown error + throw new SupersetApiError({ + message: 'Unknown Error', + originalError: error, + }); +} diff --git a/packages/superset-ui-query/src/api/v1/index.ts b/packages/superset-ui-query/src/api/v1/index.ts index 55e94e76b3..7eb35dd712 100644 --- a/packages/superset-ui-query/src/api/v1/index.ts +++ b/packages/superset-ui-query/src/api/v1/index.ts @@ -1,2 +1,33 @@ -// eslint-disable-next-line import/prefer-default-export -export { default as postChartData } from './postChartData'; +/** + * 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 './makeApi'; +import { QueryContext } from '../../types/Query'; +import { ChartDataResponse } from './types'; + +export const getChartData = makeApi({ + method: 'POST', + endpoint: '/api/v1/chart/data', +}); + +/** + * All v1 API + */ +export default { + getChartData, +}; diff --git a/packages/superset-ui-query/src/api/v1/makeApi.ts b/packages/superset-ui-query/src/api/v1/makeApi.ts new file mode 100644 index 0000000000..30d3886340 --- /dev/null +++ b/packages/superset-ui-query/src/api/v1/makeApi.ts @@ -0,0 +1,94 @@ +/** + * 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 { + SupersetClient, + Payload as SupersetPayload, + JsonObject, + JsonValue, + ParseMethod, + Endpoint, + Method, +} from '@superset-ui/connection'; +import handleError, { ErrorType } from './handleError'; +import { SupersetApiRequestOptions, SupersetApiErrorPayload, ParsedResponseType } from './types'; + +interface SupersetApiFactoryOptions { + endpoint: Endpoint; + method: Method; + requestType?: 'form' | 'json'; + responseType?: ParseMethod; +} + +/** + * Generate an API caller with predefined configs/typing and consistent + * return values. + */ +export default function makeApi< + Payload = SupersetPayload, + Result = JsonObject, + T extends ParseMethod = ParseMethod +>({ + endpoint, + method, + requestType = 'json', + responseType, + processResponse, + ...requestOptions +}: SupersetApiFactoryOptions & { + responseType?: T; + // further process response JSON or text + processResponse?(result: ParsedResponseType): Result; +}) { + async function request( + payload: Payload, + { client = SupersetClient }: SupersetApiRequestOptions = { client: SupersetClient }, + ): Promise { + try { + const requestConfig = { + ...requestOptions, + method, + endpoint, + postPayload: requestType === 'form' ? payload : undefined, + jsonPayload: requestType === 'json' ? payload : undefined, + }; + let result: JsonValue | Response; + const response = await client.request({ ...requestConfig, parseMethod: 'raw' }); + if (responseType === 'text') { + result = await response.text(); + } else if (responseType === 'raw' || responseType === null) { + result = response; + } else { + result = await response.json(); + // if response json has an "error" field + if (result && typeof result === 'object' && 'error' in result) { + return handleError(result as SupersetApiErrorPayload); + } + } + const typedResult = result as ParsedResponseType; + return (processResponse ? processResponse(typedResult) : typedResult) as Result; + } catch (error) { + return handleError(error as ErrorType); + } + } + + request.method = method; + request.endpoint = endpoint; + + return request; +} diff --git a/packages/superset-ui-query/src/api/v1/postChartData.ts b/packages/superset-ui-query/src/api/v1/postChartData.ts deleted file mode 100644 index 25635a385d..0000000000 --- a/packages/superset-ui-query/src/api/v1/postChartData.ts +++ /dev/null @@ -1,21 +0,0 @@ -import { SupersetClient } from '@superset-ui/connection'; -import { QueryContext } from '../../types/Query'; -import { BaseParams } from '../types'; -import { V1ChartDataResponse } from './types'; - -export interface Params extends BaseParams { - queryContext: QueryContext; -} - -export default async function postChartData({ - client = SupersetClient, - requestConfig, - queryContext, -}: Params) { - const { json } = await client.post({ - ...requestConfig, - endpoint: '/api/v1/chart/data', - jsonPayload: queryContext, - }); - return json as V1ChartDataResponse; -} diff --git a/packages/superset-ui-query/src/api/v1/types.ts b/packages/superset-ui-query/src/api/v1/types.ts index ab802fa5fc..b7254aa2fb 100644 --- a/packages/superset-ui-query/src/api/v1/types.ts +++ b/packages/superset-ui-query/src/api/v1/types.ts @@ -1,5 +1,102 @@ /* eslint-disable camelcase */ -export interface V1ChartDataResponseResult { +/** + * 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 { + SupersetClientClass, + SupersetClientInterface, + StrictJsonValue, + JsonValue, +} from '@superset-ui/connection'; + +export type ParsedResponseType = T extends 'text' + ? string + : T extends 'raw' | null + ? Response + : JsonValue; + +/** + * Runtime options when calling a Superset API. + */ +export interface SupersetApiRequestOptions { + client?: SupersetClientInterface | SupersetClientClass; +} + +/** + * API Error json response following the format in `json_error_response(...)` + * https://github.com/apache/incubator-superset/blob/8e23d4f369f35724b34b14def8a5a8bafb1d2ecb/superset/views/base.py#L94 + */ +export interface SupersetApiErrorPayload { + error: string; // error message returned from API + status?: number; + statusText?: string; + link?: string; +} + +/** + * API error json response following FlaskAppBuilder convention. E.g. + * response_404(message=...) -> { "message": ... } + */ +export interface SupersetApiHttpErrorPayload { + message: string; + error_type?: string; + extra?: StrictJsonValue; + level?: 'error' | 'warn' | 'info'; +} + +export interface SupersetApiHttpMultiErrorsPayload { + errors: SupersetApiHttpErrorPayload[]; +} + +export class SupersetApiError extends Error { + status?: number; + + statusText?: string; + + link?: string; + + originalError?: Error | string | Response; + + constructor({ + message, + status, + statusText, + link, + stack, + originalError, + }: Omit & { + message: string; + stack?: Error['stack']; + originalError?: Error | string | Response; // original JavaScript error captured + }) { + super(message); + this.name = 'SupersetApiError'; + this.stack = stack + ? [(this.stack || '').split('\n')[0], ...stack.split('\n').slice(1)].join('\n') + : this.stack; + this.status = status; + this.statusText = statusText; + this.link = link; + this.originalError = originalError; + } +} + +export interface ChartDataResponseResult { cache_key: string | null; cache_timeout: number | null; cache_dttm: string | null; @@ -12,6 +109,6 @@ export interface V1ChartDataResponseResult { status: 'stopped' | 'failed' | 'pending' | 'running' | 'scheduled' | 'success' | 'timed_out'; } -export interface V1ChartDataResponse { - result: V1ChartDataResponseResult[]; +export interface ChartDataResponse { + result: ChartDataResponseResult[]; } diff --git a/packages/superset-ui-query/src/extractQueryFields.ts b/packages/superset-ui-query/src/extractQueryFields.ts index 511f3b7578..a5608a5937 100644 --- a/packages/superset-ui-query/src/extractQueryFields.ts +++ b/packages/superset-ui-query/src/extractQueryFields.ts @@ -24,6 +24,7 @@ export default function extractQueryFields( Object.entries(residualFormData).forEach(entry => { const [key, residualValue] = entry; const normalizedKey = queryFieldAliases[key] || key; + // eslint-disable-next-line @typescript-eslint/no-unsafe-call finalQueryFields[normalizedKey] = (finalQueryFields[normalizedKey] || []).concat(residualValue); }); return finalQueryFields; diff --git a/packages/superset-ui-query/src/index.ts b/packages/superset-ui-query/src/index.ts index 86c38e3d00..23799686ed 100644 --- a/packages/superset-ui-query/src/index.ts +++ b/packages/superset-ui-query/src/index.ts @@ -1,4 +1,21 @@ -// API Calls +/** + * 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 * as ApiLegacy from './api/legacy'; import * as ApiV1 from './api/v1'; @@ -14,8 +31,8 @@ export * from './types/Datasource'; export * from './types/Metric'; export * from './types/Query'; -// API Calls -export { ApiLegacy, ApiV1 }; - -export * from './api/legacy/types'; export * from './api/v1/types'; +export { default as makeApi } from './api/v1/makeApi'; + +// API Callers +export { ApiLegacy, ApiV1 }; diff --git a/packages/superset-ui-query/src/types/QueryFormData.ts b/packages/superset-ui-query/src/types/QueryFormData.ts index d8286dd37f..04acf1e19b 100644 --- a/packages/superset-ui-query/src/types/QueryFormData.ts +++ b/packages/superset-ui-query/src/types/QueryFormData.ts @@ -43,6 +43,8 @@ export type BaseFormData = { limit?: number; /** limit number of row in the results */ row_limit?: string | number | null; + /** row offset for server side pagination */ + row_offset?: string | number | null; /** The metric used to order timeseries for limiting */ timeseries_limit_metric?: QueryFormResidualDataValue; /** Force refresh */ diff --git a/packages/superset-ui-query/test/api/v1/getChartData.test.ts b/packages/superset-ui-query/test/api/v1/getChartData.test.ts new file mode 100644 index 0000000000..3e887e13e4 --- /dev/null +++ b/packages/superset-ui-query/test/api/v1/getChartData.test.ts @@ -0,0 +1,48 @@ +/** + * 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 fetchMock from 'fetch-mock'; +import { buildQueryContext, ApiV1 } from '../../../src'; +import setupClientForTest from '../setupClientForTest'; + +describe('API v1 > getChartData()', () => { + beforeAll(setupClientForTest); + afterEach(fetchMock.restore); + + it('returns a promise of ChartDataResponse', async () => { + const response = { + result: [ + { + field1: 'abc', + field2: 'def', + }, + ], + }; + + fetchMock.post('glob:*/api/v1/chart/data', response); + + const result = await ApiV1.getChartData( + buildQueryContext({ + granularity: 'minute', + viz_type: 'word_cloud', + datasource: '1__table', + }), + ); + return expect(result).toEqual(response); + }); +}); diff --git a/packages/superset-ui-query/test/api/v1/handleError.test.ts b/packages/superset-ui-query/test/api/v1/handleError.test.ts new file mode 100644 index 0000000000..dbf1a4b031 --- /dev/null +++ b/packages/superset-ui-query/test/api/v1/handleError.test.ts @@ -0,0 +1,77 @@ +/** + * 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 'whatwg-fetch'; // for adding Response polyfill +import handleError, { ErrorType } from '../../../src/api/v1/handleError'; +import { SupersetApiError } from '../../../src/api/v1/types'; + +async function testHandleError(inputError: ErrorType, message: string) { + try { + await handleError(inputError); + } catch (error) { + const typedError = error as SupersetApiError; + expect(typedError).toBeInstanceOf(SupersetApiError); + expect(typedError.message).toContain(message); + } +} + +describe('handleError()', () => { + it('should handle message string', async () => { + expect.assertions(2); + await testHandleError('STOP', 'STOP'); + }); + + it('should handle HTTP error', async () => { + expect.assertions(2); + const mockResponse = new Response('Ha?', { status: 404, statusText: 'NOT FOUND' }); + await testHandleError(mockResponse, '404 NOT FOUND'); + }); + + it('should use message from HTTP error', async () => { + expect.assertions(2); + const mockResponse = new Response('{ "message": "BAD BAD" }', { + status: 500, + statusText: 'Server Error', + }); + await testHandleError(mockResponse, 'BAD BAD'); + }); + + it('should process multi errors in HTTP json', async () => { + expect.assertions(2); + const mockResponse = new Response('{ "errors": [{ "error_type": "NOT OK" }] }', { + status: 403, + statusText: 'Access Denied', + }); + await testHandleError(mockResponse, 'NOT OK'); + }); + + it('should handle regular JS error', async () => { + expect.assertions(2); + await testHandleError(new Error('What?'), 'What?'); + }); + + it('should handle { error: ... }', async () => { + expect.assertions(2); + await testHandleError({ error: 'Hmm' }, 'Hmm'); + }); + + it('should throw unknown error', async () => { + expect.assertions(2); + await testHandleError(new Date() as never, 'Unknown Error'); + }); +}); diff --git a/packages/superset-ui-query/test/api/v1/makeApi.test.ts b/packages/superset-ui-query/test/api/v1/makeApi.test.ts new file mode 100644 index 0000000000..1d820fefe7 --- /dev/null +++ b/packages/superset-ui-query/test/api/v1/makeApi.test.ts @@ -0,0 +1,125 @@ +/** + * 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 fetchMock from 'fetch-mock'; +import { JsonValue } from '@superset-ui/connection'; +import { makeApi, SupersetApiError } from '../../../src'; +import setupClientForTest from '../setupClientForTest'; + +describe('makeApi()', () => { + beforeAll(setupClientForTest); + afterEach(fetchMock.restore); + + it('should expose method and endpoint', () => { + const api = makeApi({ + method: 'GET', + endpoint: '/test', + }); + expect(api.method).toEqual('GET'); + expect(api.endpoint).toEqual('/test'); + }); + + it('should obtain json response by default', async () => { + expect.assertions(1); + const api = makeApi({ + method: 'GET', + endpoint: '/test', + }); + fetchMock.get('glob:*/test', { yes: 'ok' }); + expect(await api({})).toEqual({ yes: 'ok' }); + }); + + it('should allow custom parseResponse', async () => { + expect.assertions(2); + const responseJson = { items: [1, 2, 3] }; + fetchMock.post('glob:*/test', responseJson); + + const api = makeApi({ + method: 'POST', + endpoint: '/test', + processResponse: (json: typeof responseJson) => { + return json.items.reduce((a: number, b: number) => a + b); + }, + }); + expect(api.method).toEqual('POST'); + expect(await api({})).toBe(6); + }); + + it('should respect requestType', async () => { + expect.assertions(3); + const api = makeApi({ + method: 'POST', + endpoint: '/test-formdata', + requestType: 'form', + }); + fetchMock.post('glob:*/test-formdata', { test: 'ok' }); + + expect(await api({ request: 'test' })).toEqual({ test: 'ok' }); + + const expected = new FormData(); + expected.append('request', JSON.stringify('test')); + const received = fetchMock.lastOptions().body as FormData; + + expect(received).toBeInstanceOf(FormData); + expect(received.get('request')).toEqual(expected.get('request')); + }); + + it('should handle errors', async () => { + expect.assertions(1); + const api = makeApi({ + method: 'POST', + endpoint: '/test-formdata', + requestType: 'form', + }); + fetchMock.post('glob:*/test-formdata', { test: 'ok' }); + try { + await api(''); + } catch (error) { + expect((error as SupersetApiError).message).toContain('Invalid payload'); + } + }); + + it('should handle error on 200 response', async () => { + expect.assertions(1); + const api = makeApi({ + method: 'POST', + endpoint: '/test-200-error', + requestType: 'form', + }); + fetchMock.post('glob:*/test-200-error', { error: 'not ok' }); + try { + await api({}); + } catch (error) { + expect((error as SupersetApiError).message).toContain('not ok'); + } + }); + + it('should parse text', async () => { + expect.assertions(1); + const api = makeApi({ + method: 'PUT', + endpoint: '/test-parse-text', + requestType: 'form', + responseType: 'text', + processResponse: text => `${text}?`, + }); + fetchMock.put('glob:*/test-parse-text', 'ok'); + const result = await api({ field1: 11 }); + expect(result).toBe('ok?'); + }); +}); diff --git a/packages/superset-ui-query/test/api/v1/postChartData.test.ts b/packages/superset-ui-query/test/api/v1/postChartData.test.ts deleted file mode 100644 index 04c9f7957d..0000000000 --- a/packages/superset-ui-query/test/api/v1/postChartData.test.ts +++ /dev/null @@ -1,38 +0,0 @@ -import fetchMock from 'fetch-mock'; -import { buildQueryContext } from '../../../src'; -import { postChartData } from '../../../src/api/v1'; -import setupClientForTest from '../setupClientForTest'; - -describe('postChartData()', () => { - beforeAll(setupClientForTest); - - afterEach(fetchMock.restore); - - it('returns a promise of ChartDataResponse', () => { - fetchMock.post('glob:*/api/v1/chart/data', { - result: [ - { - field1: 'abc', - field2: 'def', - }, - ], - }); - - return expect( - postChartData({ - queryContext: buildQueryContext({ - granularity: 'minute', - viz_type: 'word_cloud', - datasource: '1__table', - }), - }), - ).resolves.toEqual({ - result: [ - { - field1: 'abc', - field2: 'def', - }, - ], - }); - }); -}); From 88c674bbcee70014d8031ffc4209d7b4f80f5adf Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Fri, 3 Jul 2020 01:37:14 -0700 Subject: [PATCH 2/9] Add support for searchParams --- .../src/SupersetClientClass.ts | 46 ++++++------ .../src/callApi/callApi.ts | 72 ++++++++++++------- .../src/callApi/parseResponse.ts | 5 +- packages/superset-ui-connection/src/types.ts | 26 ++++++- .../src/api/legacy/fetchExploreJson.ts | 8 +-- .../superset-ui-query/src/api/v1/makeApi.ts | 44 ++++++++++-- .../superset-ui-query/src/api/v1/types.ts | 3 +- .../test/api/legacy/fetchExploreJson.test.ts | 18 +++++ .../api/legacy/getDatasourceMetadata.test.ts | 18 +++++ .../test/api/legacy/getFormData.test.ts | 18 +++++ .../test/api/setupClientForTest.ts | 19 ++++- .../test/api/v1/makeApi.test.ts | 66 ++++++++++++++++- 12 files changed, 275 insertions(+), 68 deletions(-) diff --git a/packages/superset-ui-connection/src/SupersetClientClass.ts b/packages/superset-ui-connection/src/SupersetClientClass.ts index 03dd74bf0c..83f1d35854 100644 --- a/packages/superset-ui-connection/src/SupersetClientClass.ts +++ b/packages/superset-ui-connection/src/SupersetClientClass.ts @@ -1,3 +1,21 @@ +/** + * 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 callApiAndParseWithTimeout from './callApi/callApiAndParseWithTimeout'; import { ClientConfig, @@ -89,37 +107,23 @@ export default class SupersetClientClass { } async request({ - body, credentials, + mode, endpoint, - fetchRetryOptions, - headers, host, - method, - mode, - parseMethod, - postPayload, - jsonPayload, - signal, - stringify, - timeout, url, + headers, + timeout, + ...rest }: RequestConfig & { parseMethod?: T }) { await this.ensureAuth(); return callApiAndParseWithTimeout({ - body, + ...rest, credentials: credentials ?? this.credentials, - fetchRetryOptions, - headers: { ...this.headers, ...headers }, - method, mode: mode ?? this.mode, - parseMethod, - postPayload, - jsonPayload, - signal, - stringify, - timeout: timeout ?? this.timeout, url: this.getUrl({ endpoint, host, url }), + headers: { ...this.headers, ...headers }, + timeout: timeout ?? this.timeout, }); } diff --git a/packages/superset-ui-connection/src/callApi/callApi.ts b/packages/superset-ui-connection/src/callApi/callApi.ts index 6801bc17b2..9d8a8f10d9 100644 --- a/packages/superset-ui-connection/src/callApi/callApi.ts +++ b/packages/superset-ui-connection/src/callApi/callApi.ts @@ -1,8 +1,29 @@ import 'whatwg-fetch'; import fetchRetry from 'fetch-retry'; -import { CallApi, JsonObject, JsonValue } from '../types'; +import { CallApi, Payload, JsonValue } from '../types'; import { CACHE_AVAILABLE, CACHE_KEY, HTTP_STATUS_NOT_MODIFIED, HTTP_STATUS_OK } from '../constants'; +function tryParsePayload(payload: Payload) { + try { + return typeof payload === 'string' ? (JSON.parse(payload) as JsonValue) : payload; + } catch (error) { + throw new Error(`Invalid payload:\n\n${payload}`); + } +} + +/** + * Try appending search params to an URL. + */ +function getFullUrl(partialUrl: string, params: CallApi['searchParams']) { + const url = new URL(partialUrl); + if (params) { + const search = params instanceof URLSearchParams ? params : new URLSearchParams(params); + // will completely override any existing search params + url.search = search.toString(); + } + return url.href; +} + /** * Fetch an API response and returns the corresponding json. * @@ -23,9 +44,11 @@ export default async function callApi({ redirect = 'follow', signal, stringify = true, - url, + url: url_, + searchParams, }: CallApi): Promise { const fetchWithRetry = fetchRetry(fetch, fetchRetryOptions); + const url = `${getFullUrl(url_, searchParams)}`; const request = { body, @@ -53,7 +76,9 @@ export default async function callApi({ const etag = cachedResponse.headers.get('Etag') as string; request.headers = { ...request.headers, 'If-None-Match': etag }; } + const response = await fetchWithRetry(url, request); + if (response.status === HTTP_STATUS_NOT_MODIFIED) { const cachedFullResponse = await supersetCache.match(url); if (cachedFullResponse) { @@ -65,33 +90,32 @@ export default async function callApi({ supersetCache.delete(url); supersetCache.put(url, response.clone()); } + return response; } if (method === 'POST' || method === 'PATCH' || method === 'PUT') { - const tryParsePayload = (payloadString: string) => { - try { - return JSON.parse(payloadString) as JsonObject; - } catch (error) { - throw new Error(`Invalid payload:\n\n${payloadString}`); + if (postPayload && jsonPayload) { + throw new Error('Please provide only one of jsonPayload and postPayload'); + } + if (postPayload instanceof FormData) { + request.body = postPayload; + } else if (postPayload) { + const payload = tryParsePayload(postPayload); + if (payload && 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; } - }; - // 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) { + } + if (jsonPayload !== undefined) { request.body = JSON.stringify(jsonPayload); request.headers = { ...request.headers, 'Content-Type': 'application/json' }; } diff --git a/packages/superset-ui-connection/src/callApi/parseResponse.ts b/packages/superset-ui-connection/src/callApi/parseResponse.ts index ba509cb6eb..1367f4f5cd 100644 --- a/packages/superset-ui-connection/src/callApi/parseResponse.ts +++ b/packages/superset-ui-connection/src/callApi/parseResponse.ts @@ -14,10 +14,8 @@ export default async function parseResponse( const response = await apiPromise; // reject failed HTTP requests with the raw response if (!response.ok) { - // eslint-disable-next-line @typescript-eslint/no-throw-literal - throw response; + return Promise.reject(response); } - if (parseMethod === null || parseMethod === 'raw') { return response as ReturnType; } @@ -38,6 +36,5 @@ export default async function parseResponse( }; return result as ReturnType; } - throw new Error(`Expected parseResponse=json|text|raw|null, got '${parseMethod}'.`); } diff --git a/packages/superset-ui-connection/src/types.ts b/packages/superset-ui-connection/src/types.ts index 40f3caaed3..4740a1b17a 100644 --- a/packages/superset-ui-connection/src/types.ts +++ b/packages/superset-ui-connection/src/types.ts @@ -1,3 +1,21 @@ +/** + * 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 SupersetClientClass from './SupersetClientClass'; export type Body = RequestInit['body']; @@ -36,9 +54,10 @@ export type JsonArray = JsonValue[]; export type JsonObject = { [member: string]: any }; /** - * Post form or JSON payload, if string, will parse with JSON.parse + * Request payload, can be use in GET query string, Post form or POST JSON. + * If string, will parse with JSON.parse. */ -export type Payload = JsonObject | string; +export type Payload = JsonObject | string | null; export type Method = RequestInit['method']; export type Mode = RequestInit['mode']; @@ -57,8 +76,9 @@ export interface RequestBase { host?: Host; mode?: Mode; method?: Method; - postPayload?: Payload; jsonPayload?: Payload; + postPayload?: Payload | FormData; + searchParams?: Payload | URLSearchParams; signal?: Signal; stringify?: Stringify; timeout?: ClientTimeout; diff --git a/packages/superset-ui-query/src/api/legacy/fetchExploreJson.ts b/packages/superset-ui-query/src/api/legacy/fetchExploreJson.ts index cd6871cae3..f87e3c721a 100644 --- a/packages/superset-ui-query/src/api/legacy/fetchExploreJson.ts +++ b/packages/superset-ui-query/src/api/legacy/fetchExploreJson.ts @@ -1,11 +1,11 @@ -import { SupersetClient, Method, Url } from '@superset-ui/connection'; +import { SupersetClient, Method, Endpoint } from '@superset-ui/connection'; import { QueryFormData } from '../../types/QueryFormData'; import { LegacyChartDataResponse } from './types'; import { BaseParams } from '../types'; export interface Params extends BaseParams { method?: Method; - url?: Url; + endpoint?: Endpoint; formData: QueryFormData; } @@ -13,13 +13,13 @@ export default async function fetchExploreJson({ client = SupersetClient, method = 'POST', requestConfig, - url = '/superset/explore_json/', + endpoint = '/superset/explore_json/', formData, }: Params) { const { json } = await client.request({ ...requestConfig, method, - url, + endpoint, // TODO: Have to transform formData as query string for GET postPayload: { form_data: formData }, }); diff --git a/packages/superset-ui-query/src/api/v1/makeApi.ts b/packages/superset-ui-query/src/api/v1/makeApi.ts index 30d3886340..ae437dfc21 100644 --- a/packages/superset-ui-query/src/api/v1/makeApi.ts +++ b/packages/superset-ui-query/src/api/v1/makeApi.ts @@ -24,15 +24,31 @@ import { ParseMethod, Endpoint, Method, + RequestBase, } from '@superset-ui/connection'; import handleError, { ErrorType } from './handleError'; import { SupersetApiRequestOptions, SupersetApiErrorPayload, ParsedResponseType } from './types'; -interface SupersetApiFactoryOptions { +interface SupersetApiFactoryOptions extends Omit { + /** + * API endpoint, must be relative. + */ endpoint: Endpoint; + /** + * Request method: 'GET' | 'POST' | 'DELETE' | 'PUT' | ... + */ method: Method; - requestType?: 'form' | 'json'; - responseType?: ParseMethod; + /** + * How to send the payload: + * - form: set request.body as FormData + * - json: as JSON string with request Content-Type header set to application/json + * - search: add to search params + */ + requestType?: 'form' | 'json' | 'search'; +} + +function isPayloadless(method?: Method) { + return !method || method === 'GET' || method === 'DELETE' || method === 'HEAD'; } /** @@ -46,29 +62,43 @@ export default function makeApi< >({ endpoint, method, - requestType = 'json', + requestType: requestType_, responseType, processResponse, ...requestOptions }: SupersetApiFactoryOptions & { + /** + * How to parse response, choose from: 'json' | 'text' | 'raw'. + */ responseType?: T; - // further process response JSON or text + /** + * Further process parsed response + */ processResponse?(result: ParsedResponseType): Result; }) { async function request( payload: Payload, { client = SupersetClient }: SupersetApiRequestOptions = { client: SupersetClient }, ): Promise { + // use `search` payload (searchParams) when it's a GET request + const requestType = requestType_ || (isPayloadless(method) ? 'search' : 'json'); try { const requestConfig = { ...requestOptions, method, endpoint, - postPayload: requestType === 'form' ? payload : undefined, - jsonPayload: requestType === 'json' ? payload : undefined, }; + if (requestType === 'search') { + requestConfig.searchParams = payload; + } else if (requestType === 'form') { + requestConfig.postPayload = payload; + } else if (requestType === 'json') { + requestConfig.jsonPayload = payload; + } + let result: JsonValue | Response; const response = await client.request({ ...requestConfig, parseMethod: 'raw' }); + if (responseType === 'text') { result = await response.text(); } else if (responseType === 'raw' || responseType === null) { diff --git a/packages/superset-ui-query/src/api/v1/types.ts b/packages/superset-ui-query/src/api/v1/types.ts index b7254aa2fb..dadc62b13b 100644 --- a/packages/superset-ui-query/src/api/v1/types.ts +++ b/packages/superset-ui-query/src/api/v1/types.ts @@ -31,7 +31,8 @@ export type ParsedResponseType = T extends 'text' : JsonValue; /** - * Runtime options when calling a Superset API. + * Runtime options when calling a Superset API. Currently only allow overriding + * SupersetClient instance. */ export interface SupersetApiRequestOptions { client?: SupersetClientInterface | SupersetClientClass; diff --git a/packages/superset-ui-query/test/api/legacy/fetchExploreJson.test.ts b/packages/superset-ui-query/test/api/legacy/fetchExploreJson.test.ts index 89e9fbfa8a..972b0b85d9 100644 --- a/packages/superset-ui-query/test/api/legacy/fetchExploreJson.test.ts +++ b/packages/superset-ui-query/test/api/legacy/fetchExploreJson.test.ts @@ -1,3 +1,21 @@ +/** + * 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 fetchMock from 'fetch-mock'; import { fetchExploreJson } from '../../../src/api/legacy'; import setupClientForTest from '../setupClientForTest'; diff --git a/packages/superset-ui-query/test/api/legacy/getDatasourceMetadata.test.ts b/packages/superset-ui-query/test/api/legacy/getDatasourceMetadata.test.ts index a2f032f391..625efe7a2d 100644 --- a/packages/superset-ui-query/test/api/legacy/getDatasourceMetadata.test.ts +++ b/packages/superset-ui-query/test/api/legacy/getDatasourceMetadata.test.ts @@ -1,3 +1,21 @@ +/** + * 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 fetchMock from 'fetch-mock'; import setupClientForTest from '../setupClientForTest'; import { getDatasourceMetadata } from '../../../src/api/legacy'; diff --git a/packages/superset-ui-query/test/api/legacy/getFormData.test.ts b/packages/superset-ui-query/test/api/legacy/getFormData.test.ts index 867e3fa488..2926e0641c 100644 --- a/packages/superset-ui-query/test/api/legacy/getFormData.test.ts +++ b/packages/superset-ui-query/test/api/legacy/getFormData.test.ts @@ -1,3 +1,21 @@ +/** + * 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 fetchMock from 'fetch-mock'; import setupClientForTest from '../setupClientForTest'; import { getFormData } from '../../../src/api/legacy'; diff --git a/packages/superset-ui-query/test/api/setupClientForTest.ts b/packages/superset-ui-query/test/api/setupClientForTest.ts index 6265658995..295178271e 100644 --- a/packages/superset-ui-query/test/api/setupClientForTest.ts +++ b/packages/superset-ui-query/test/api/setupClientForTest.ts @@ -1,4 +1,21 @@ -// eslint-disable-next-line import/no-extraneous-dependencies +/** + * 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 fetchMock from 'fetch-mock'; import { SupersetClient } from '@superset-ui/connection'; diff --git a/packages/superset-ui-query/test/api/v1/makeApi.test.ts b/packages/superset-ui-query/test/api/v1/makeApi.test.ts index 1d820fefe7..a4fcedc9fb 100644 --- a/packages/superset-ui-query/test/api/v1/makeApi.test.ts +++ b/packages/superset-ui-query/test/api/v1/makeApi.test.ts @@ -17,7 +17,7 @@ * under the License. */ import fetchMock from 'fetch-mock'; -import { JsonValue } from '@superset-ui/connection'; +import { JsonValue, SupersetClientClass } from '@superset-ui/connection'; import { makeApi, SupersetApiError } from '../../../src'; import setupClientForTest from '../setupClientForTest'; @@ -34,6 +34,29 @@ describe('makeApi()', () => { expect(api.endpoint).toEqual('/test'); }); + it('should allow custom client', async () => { + expect.assertions(2); + const api = makeApi({ + method: 'GET', + endpoint: '/test-custom-client', + }); + const client = new SupersetClientClass({ baseUrl: 'http://foo/' }); + const mockResponse = { yes: 'ok' }; + const mockRequest = jest.fn(() => + Promise.resolve( + new Response(JSON.stringify(mockResponse), { + headers: { 'Content-Type': 'application/json' }, + }), + ), + ); + Object.assign(client, { + request: mockRequest, + }); + const result = await api(null, { client }); + expect(result).toEqual(mockResponse); + expect(mockRequest).toHaveBeenCalledTimes(1); + }); + it('should obtain json response by default', async () => { expect.assertions(1); const api = makeApi({ @@ -60,7 +83,7 @@ describe('makeApi()', () => { expect(await api({})).toBe(6); }); - it('should respect requestType', async () => { + it('should post FormData when requestType=form', async () => { expect.assertions(3); const api = makeApi({ method: 'POST', @@ -79,6 +102,29 @@ describe('makeApi()', () => { expect(received.get('request')).toEqual(expected.get('request')); }); + it('should use searchParams for method=GET (`requestType=search` implied)', async () => { + expect.assertions(1); + const api = makeApi({ + method: 'GET', + endpoint: '/test-get-search', + }); + fetchMock.get('glob:*/test-get-search*', { search: 'get' }); + await api({ p1: 1, p2: 2, p3: [1, 2] }); + expect(fetchMock.lastUrl()).toContain('/test-get-search?p1=1&p2=2&p3=1%2C2'); + }); + + it('should use searchParams for method=POST, requestType=search', async () => { + expect.assertions(1); + const api = makeApi({ + method: 'POST', + endpoint: '/test-post-search', + requestType: 'search', + }); + fetchMock.post('glob:*/test-post-search*', { search: 'post' }); + await api({ p1: 1, p3: [1, 2] }); + expect(fetchMock.lastUrl()).toContain('/test-post-search?p1=1&p3=1%2C2'); + }); + it('should handle errors', async () => { expect.assertions(1); const api = makeApi({ @@ -109,7 +155,7 @@ describe('makeApi()', () => { } }); - it('should parse text', async () => { + it('should parse text response when responseType=text', async () => { expect.assertions(1); const api = makeApi({ method: 'PUT', @@ -122,4 +168,18 @@ describe('makeApi()', () => { const result = await api({ field1: 11 }); expect(result).toBe('ok?'); }); + + it('should return raw resposnse when responseType=raw', async () => { + expect.assertions(2); + const api = makeApi({ + method: 'DELETE', + endpoint: '/test-raw-response', + responseType: 'raw', + processResponse: response => response.status, + }); + fetchMock.delete('glob:*/test-raw-response?*', 'ok'); + const result = await api({ field1: 11 }, {}); + expect(result).toEqual(200); + expect(fetchMock.lastUrl()).toContain('/test-raw-response?field1=11'); + }); }); From 49eb800c03fee19b7e32211b15f3341f2bf098cc Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Fri, 3 Jul 2020 02:31:14 -0700 Subject: [PATCH 3/9] Add 100% coverage for callApi --- .../src/SupersetClientClass.ts | 3 + .../src/callApi/callApi.ts | 9 +-- .../test/callApi/callApi.test.ts | 64 +++++++++++++++++++ 3 files changed, 72 insertions(+), 4 deletions(-) diff --git a/packages/superset-ui-connection/src/SupersetClientClass.ts b/packages/superset-ui-connection/src/SupersetClientClass.ts index 83f1d35854..461147c3d0 100644 --- a/packages/superset-ui-connection/src/SupersetClientClass.ts +++ b/packages/superset-ui-connection/src/SupersetClientClass.ts @@ -58,6 +58,9 @@ export default class SupersetClientClass { }: ClientConfig = {}) { const url = new URL( host || protocol ? `${protocol || 'https:'}//${host || 'localhost'}` : baseUrl, + // baseUrl for API could also be relative, so we provide current location.href + // as the base of baseUrl + window.location.href || 'http://localhost', ); this.baseUrl = url.href.replace(/\/+$/, ''); // always strip trailing slash this.host = url.host; diff --git a/packages/superset-ui-connection/src/callApi/callApi.ts b/packages/superset-ui-connection/src/callApi/callApi.ts index 9d8a8f10d9..2f20249c1a 100644 --- a/packages/superset-ui-connection/src/callApi/callApi.ts +++ b/packages/superset-ui-connection/src/callApi/callApi.ts @@ -12,16 +12,17 @@ function tryParsePayload(payload: Payload) { } /** - * Try appending search params to an URL. + * Try appending search params to an URL if needed. */ function getFullUrl(partialUrl: string, params: CallApi['searchParams']) { - const url = new URL(partialUrl); if (params) { + const url = new URL(partialUrl, window.location.href); const search = params instanceof URLSearchParams ? params : new URLSearchParams(params); // will completely override any existing search params url.search = search.toString(); + return url.href; } - return url.href; + return partialUrl; } /** @@ -96,7 +97,7 @@ export default async function callApi({ if (method === 'POST' || method === 'PATCH' || method === 'PUT') { if (postPayload && jsonPayload) { - throw new Error('Please provide only one of jsonPayload and postPayload'); + throw new Error('Please provide only one of jsonPayload or postPayload'); } if (postPayload instanceof FormData) { request.body = postPayload; diff --git a/packages/superset-ui-connection/test/callApi/callApi.test.ts b/packages/superset-ui-connection/test/callApi/callApi.test.ts index ff2e104510..eae78be591 100644 --- a/packages/superset-ui-connection/test/callApi/callApi.test.ts +++ b/packages/superset-ui-connection/test/callApi/callApi.test.ts @@ -488,4 +488,68 @@ describe('callApi()', () => { expect(error.message).toEqual('Invalid payload:\n\nhaha'); } }); + + it('should accept search params object', async () => { + expect.assertions(3); + window.location.href = 'http://localhost'; + fetchMock.get(`glob:*/get-search*`, { yes: 'ok' }); + const response = await callApi({ + url: '/get-search', + searchParams: { + abc: 1, + }, + method: 'GET', + }); + const result = await response.json(); + expect(response.status).toEqual(200); + expect(result).toEqual({ yes: 'ok' }); + expect(fetchMock.lastUrl()).toEqual(`http://localhost/get-search?abc=1`); + }); + + it('should accept URLSearchParams', async () => { + expect.assertions(2); + window.location.href = 'http://localhost'; + fetchMock.post(`glob:*/post-search*`, { yes: 'ok' }); + await callApi({ + url: '/post-search', + searchParams: new URLSearchParams({ + abc: '1', + }), + method: 'POST', + jsonPayload: { request: 'ok' }, + }); + expect(fetchMock.lastUrl()).toEqual(`http://localhost/post-search?abc=1`); + expect(fetchMock.lastOptions()).toEqual( + expect.objectContaining({ + body: JSON.stringify({ request: 'ok' }), + }), + ); + }); + + it('should throw when both payloads provided', async () => { + expect.assertions(1); + fetchMock.post('/post-both-payload', {}); + try { + await callApi({ + url: '/post-both-payload', + method: 'POST', + postPayload: { a: 1 }, + jsonPayload: '{}', + }); + } catch (error) { + expect((error as Error).message).toContain('provide only one of jsonPayload or postPayload'); + } + }); + + it('should accept FormData as postPayload', async () => { + expect.assertions(1); + fetchMock.post('/post-formdata', {}); + const payload = new FormData(); + await callApi({ + url: '/post-formdata', + method: 'POST', + postPayload: payload, + }); + expect(fetchMock.lastOptions().body).toBe(payload); + }); }); From e25d9f485dac81b576e0a19cd6db5a74be9de53e Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Fri, 3 Jul 2020 02:36:50 -0700 Subject: [PATCH 4/9] Add const DEFAULT_BASE_URL --- .../superset-ui-connection/src/SupersetClientClass.ts | 6 +++--- packages/superset-ui-connection/src/callApi/callApi.ts | 10 ++++++++-- packages/superset-ui-connection/src/constants.ts | 2 ++ 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/packages/superset-ui-connection/src/SupersetClientClass.ts b/packages/superset-ui-connection/src/SupersetClientClass.ts index 461147c3d0..08fa0b8424 100644 --- a/packages/superset-ui-connection/src/SupersetClientClass.ts +++ b/packages/superset-ui-connection/src/SupersetClientClass.ts @@ -31,7 +31,7 @@ import { RequestConfig, ParseMethod, } from './types'; -import { DEFAULT_FETCH_RETRY_OPTIONS } from './constants'; +import { DEFAULT_FETCH_RETRY_OPTIONS, DEFAULT_BASE_URL } from './constants'; export default class SupersetClientClass { credentials: Credentials; @@ -46,7 +46,7 @@ export default class SupersetClientClass { timeout: ClientTimeout; constructor({ - baseUrl = 'http://localhost', + baseUrl = DEFAULT_BASE_URL, host, protocol, headers = {}, @@ -60,7 +60,7 @@ export default class SupersetClientClass { host || protocol ? `${protocol || 'https:'}//${host || 'localhost'}` : baseUrl, // baseUrl for API could also be relative, so we provide current location.href // as the base of baseUrl - window.location.href || 'http://localhost', + window.location.href || DEFAULT_BASE_URL, ); this.baseUrl = url.href.replace(/\/+$/, ''); // always strip trailing slash this.host = url.host; diff --git a/packages/superset-ui-connection/src/callApi/callApi.ts b/packages/superset-ui-connection/src/callApi/callApi.ts index 2f20249c1a..8e9ddf2d28 100644 --- a/packages/superset-ui-connection/src/callApi/callApi.ts +++ b/packages/superset-ui-connection/src/callApi/callApi.ts @@ -1,7 +1,13 @@ import 'whatwg-fetch'; import fetchRetry from 'fetch-retry'; import { CallApi, Payload, JsonValue } from '../types'; -import { CACHE_AVAILABLE, CACHE_KEY, HTTP_STATUS_NOT_MODIFIED, HTTP_STATUS_OK } from '../constants'; +import { + CACHE_AVAILABLE, + CACHE_KEY, + HTTP_STATUS_NOT_MODIFIED, + HTTP_STATUS_OK, + DEFAULT_BASE_URL, +} from '../constants'; function tryParsePayload(payload: Payload) { try { @@ -16,7 +22,7 @@ function tryParsePayload(payload: Payload) { */ function getFullUrl(partialUrl: string, params: CallApi['searchParams']) { if (params) { - const url = new URL(partialUrl, window.location.href); + const url = new URL(partialUrl, window.location.href || DEFAULT_BASE_URL); const search = params instanceof URLSearchParams ? params : new URLSearchParams(params); // will completely override any existing search params url.search = search.toString(); diff --git a/packages/superset-ui-connection/src/constants.ts b/packages/superset-ui-connection/src/constants.ts index e856f48898..c0c9bd01f9 100644 --- a/packages/superset-ui-connection/src/constants.ts +++ b/packages/superset-ui-connection/src/constants.ts @@ -1,5 +1,7 @@ import { FetchRetryOptions } from './types'; +export const DEFAULT_BASE_URL = 'http://localhost'; + // HTTP status codes export const HTTP_STATUS_OK = 200; export const HTTP_STATUS_NOT_MODIFIED = 304; From ca3b1b811b21fc6e09a822c085e1f8a11eb9329a Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Fri, 3 Jul 2020 20:10:44 -0700 Subject: [PATCH 5/9] Adjust ESLint ignore rule for tests --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index f05c4d4600..c087c55606 100644 --- a/package.json +++ b/package.json @@ -170,7 +170,7 @@ } }, { - "files": "*.test.{js,jsx,ts,tsx}", + "files": "**/test/**/*", "rules": { "import/no-extraneous-dependencies": "off", "promise/param-names": "off", From 9c5f9eb20026a4f5bd0aa0c602f4a270cc4180ee Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Fri, 3 Jul 2020 22:16:39 -0700 Subject: [PATCH 6/9] Clean up --- .../src/SupersetClientClass.ts | 2 +- .../superset-ui-connection/src/callApi/callApi.ts | 10 ++-------- .../test/SupersetClientClass.test.ts | 13 +++++-------- 3 files changed, 8 insertions(+), 17 deletions(-) diff --git a/packages/superset-ui-connection/src/SupersetClientClass.ts b/packages/superset-ui-connection/src/SupersetClientClass.ts index 08fa0b8424..01b9ea80e7 100644 --- a/packages/superset-ui-connection/src/SupersetClientClass.ts +++ b/packages/superset-ui-connection/src/SupersetClientClass.ts @@ -60,7 +60,7 @@ export default class SupersetClientClass { host || protocol ? `${protocol || 'https:'}//${host || 'localhost'}` : baseUrl, // baseUrl for API could also be relative, so we provide current location.href // as the base of baseUrl - window.location.href || DEFAULT_BASE_URL, + window.location.href, ); this.baseUrl = url.href.replace(/\/+$/, ''); // always strip trailing slash this.host = url.host; diff --git a/packages/superset-ui-connection/src/callApi/callApi.ts b/packages/superset-ui-connection/src/callApi/callApi.ts index 8e9ddf2d28..2f20249c1a 100644 --- a/packages/superset-ui-connection/src/callApi/callApi.ts +++ b/packages/superset-ui-connection/src/callApi/callApi.ts @@ -1,13 +1,7 @@ import 'whatwg-fetch'; import fetchRetry from 'fetch-retry'; import { CallApi, Payload, JsonValue } from '../types'; -import { - CACHE_AVAILABLE, - CACHE_KEY, - HTTP_STATUS_NOT_MODIFIED, - HTTP_STATUS_OK, - DEFAULT_BASE_URL, -} from '../constants'; +import { CACHE_AVAILABLE, CACHE_KEY, HTTP_STATUS_NOT_MODIFIED, HTTP_STATUS_OK } from '../constants'; function tryParsePayload(payload: Payload) { try { @@ -22,7 +16,7 @@ function tryParsePayload(payload: Payload) { */ function getFullUrl(partialUrl: string, params: CallApi['searchParams']) { if (params) { - const url = new URL(partialUrl, window.location.href || DEFAULT_BASE_URL); + const url = new URL(partialUrl, window.location.href); const search = params instanceof URLSearchParams ? params : new URLSearchParams(params); // will completely override any existing search params url.search = search.toString(); diff --git a/packages/superset-ui-connection/test/SupersetClientClass.test.ts b/packages/superset-ui-connection/test/SupersetClientClass.test.ts index 41f5a0114a..67100e7114 100644 --- a/packages/superset-ui-connection/test/SupersetClientClass.test.ts +++ b/packages/superset-ui-connection/test/SupersetClientClass.test.ts @@ -27,14 +27,11 @@ describe('SupersetClientClass', () => { afterAll(fetchMock.restore); - it('new SupersetClientClass()', () => { - const client = new SupersetClientClass(); - expect(client).toBeInstanceOf(SupersetClientClass); - }); - - it('fallback protocol to https when setting only host', () => { - const client = new SupersetClientClass({ host: 'TEST-HOST' }); - expect(client.baseUrl).toEqual('https://test-host'); + describe('new SupersetClientClass()', () => { + it('fallback protocol to https when setting only host', () => { + const client = new SupersetClientClass({ host: 'TEST-HOST' }); + expect(client.baseUrl).toEqual('https://test-host'); + }); }); describe('.getUrl()', () => { From fcd6c7279c691ebf1f62c6f2c891b2111e551077 Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Sun, 5 Jul 2020 00:54:38 -0700 Subject: [PATCH 7/9] Hit 100% coverage --- .../test/callApi/callApi.test.ts | 11 ++++++++++ .../src/api/v1/handleError.ts | 8 ++++++- .../superset-ui-query/src/api/v1/makeApi.ts | 13 ++++++++--- .../superset-ui-query/src/api/v1/types.ts | 7 +++--- .../test/api/v1/handleError.test.ts | 22 +++++++++++++++++-- .../test/api/v1/makeApi.test.ts | 15 +++++++++++-- 6 files changed, 65 insertions(+), 11 deletions(-) diff --git a/packages/superset-ui-connection/test/callApi/callApi.test.ts b/packages/superset-ui-connection/test/callApi/callApi.test.ts index eae78be591..84dd94ce74 100644 --- a/packages/superset-ui-connection/test/callApi/callApi.test.ts +++ b/packages/superset-ui-connection/test/callApi/callApi.test.ts @@ -552,4 +552,15 @@ describe('callApi()', () => { }); expect(fetchMock.lastOptions().body).toBe(payload); }); + + it('should ignore "null" postPayload string', async () => { + expect.assertions(1); + fetchMock.post('/post-null-postpayload', {}); + await callApi({ + url: '/post-formdata', + method: 'POST', + postPayload: 'null', + }); + expect(fetchMock.lastOptions().body).toBeUndefined(); + }); }); diff --git a/packages/superset-ui-query/src/api/v1/handleError.ts b/packages/superset-ui-query/src/api/v1/handleError.ts index 889ff7ff48..e93b9ee7b0 100644 --- a/packages/superset-ui-query/src/api/v1/handleError.ts +++ b/packages/superset-ui-query/src/api/v1/handleError.ts @@ -37,8 +37,8 @@ export default async function handleError(error: ErrorType): Promise { // catch HTTP errors if (error instanceof Response) { const { status, statusText } = error; + let errorMessage = `${status} ${statusText}`; if (status >= 400) { - let errorMessage = `${status} ${statusText}`; try { const json = (await error.json()) as | SupersetApiHttpErrorPayload @@ -53,6 +53,12 @@ export default async function handleError(error: ErrorType): Promise { statusText, message: errorMessage, }); + } else { + throw new SupersetApiError({ + status, + statusText, + message: errorMessage, + }); } } // JS errors, normally happens before request was sent diff --git a/packages/superset-ui-query/src/api/v1/makeApi.ts b/packages/superset-ui-query/src/api/v1/makeApi.ts index ae437dfc21..784d61d90d 100644 --- a/packages/superset-ui-query/src/api/v1/makeApi.ts +++ b/packages/superset-ui-query/src/api/v1/makeApi.ts @@ -29,6 +29,8 @@ import { import handleError, { ErrorType } from './handleError'; import { SupersetApiRequestOptions, SupersetApiErrorPayload, ParsedResponseType } from './types'; +const validRequestTypes = new Set(['form', 'json', 'search']); + interface SupersetApiFactoryOptions extends Omit { /** * API endpoint, must be relative. @@ -76,12 +78,16 @@ export default function makeApi< */ processResponse?(result: ParsedResponseType): Result; }) { + // use `search` payload (searchParams) when it's a GET request + const requestType = requestType_ || (isPayloadless(method) ? 'search' : 'json'); + if (!validRequestTypes.has(requestType)) { + throw new Error('Invalid request payload type, choose from: form | json | search'); + } + async function request( payload: Payload, { client = SupersetClient }: SupersetApiRequestOptions = { client: SupersetClient }, ): Promise { - // use `search` payload (searchParams) when it's a GET request - const requestType = requestType_ || (isPayloadless(method) ? 'search' : 'json'); try { const requestConfig = { ...requestOptions, @@ -92,7 +98,7 @@ export default function makeApi< requestConfig.searchParams = payload; } else if (requestType === 'form') { requestConfig.postPayload = payload; - } else if (requestType === 'json') { + } else { requestConfig.jsonPayload = payload; } @@ -119,6 +125,7 @@ export default function makeApi< request.method = method; request.endpoint = endpoint; + request.requestType = requestType; return request; } diff --git a/packages/superset-ui-query/src/api/v1/types.ts b/packages/superset-ui-query/src/api/v1/types.ts index dadc62b13b..47928e4279 100644 --- a/packages/superset-ui-query/src/api/v1/types.ts +++ b/packages/superset-ui-query/src/api/v1/types.ts @@ -87,9 +87,10 @@ export class SupersetApiError extends Error { }) { super(message); this.name = 'SupersetApiError'; - this.stack = stack - ? [(this.stack || '').split('\n')[0], ...stack.split('\n').slice(1)].join('\n') - : this.stack; + this.stack = + stack && this.stack + ? [this.stack.split('\n')[0], ...stack.split('\n').slice(1)].join('\n') + : this.stack; this.status = status; this.statusText = statusText; this.link = link; diff --git a/packages/superset-ui-query/test/api/v1/handleError.test.ts b/packages/superset-ui-query/test/api/v1/handleError.test.ts index dbf1a4b031..b31e087650 100644 --- a/packages/superset-ui-query/test/api/v1/handleError.test.ts +++ b/packages/superset-ui-query/test/api/v1/handleError.test.ts @@ -42,6 +42,12 @@ describe('handleError()', () => { await testHandleError(mockResponse, '404 NOT FOUND'); }); + it('should handle HTTP error with status < 400', async () => { + expect.assertions(2); + const mockResponse = new Response('Ha haha?', { status: 302, statusText: 'Found' }); + await testHandleError(mockResponse, '302 Found'); + }); + it('should use message from HTTP error', async () => { expect.assertions(2); const mockResponse = new Response('{ "message": "BAD BAD" }', { @@ -60,9 +66,21 @@ describe('handleError()', () => { await testHandleError(mockResponse, 'NOT OK'); }); - it('should handle regular JS error', async () => { + it('should fallback to statusText', async () => { expect.assertions(2); + const mockResponse = new Response('{ "failed": "random ramble" }', { + status: 403, + statusText: 'Access Denied', + }); + await testHandleError(mockResponse, '403 Access Denied'); + }); + + it('should handle regular JS error', async () => { + expect.assertions(4); await testHandleError(new Error('What?'), 'What?'); + const emptyError = new Error(); + emptyError.stack = undefined; + await testHandleError(emptyError, 'Unknown Error'); }); it('should handle { error: ... }', async () => { @@ -72,6 +90,6 @@ describe('handleError()', () => { it('should throw unknown error', async () => { expect.assertions(2); - await testHandleError(new Date() as never, 'Unknown Error'); + await testHandleError(Promise.resolve('Some random things') as never, 'Unknown Error'); }); }); diff --git a/packages/superset-ui-query/test/api/v1/makeApi.test.ts b/packages/superset-ui-query/test/api/v1/makeApi.test.ts index a4fcedc9fb..92221c6a8f 100644 --- a/packages/superset-ui-query/test/api/v1/makeApi.test.ts +++ b/packages/superset-ui-query/test/api/v1/makeApi.test.ts @@ -32,6 +32,7 @@ describe('makeApi()', () => { }); expect(api.method).toEqual('GET'); expect(api.endpoint).toEqual('/test'); + expect(api.requestType).toEqual('search'); }); it('should allow custom client', async () => { @@ -71,7 +72,6 @@ describe('makeApi()', () => { expect.assertions(2); const responseJson = { items: [1, 2, 3] }; fetchMock.post('glob:*/test', responseJson); - const api = makeApi({ method: 'POST', endpoint: '/test', @@ -125,6 +125,17 @@ describe('makeApi()', () => { expect(fetchMock.lastUrl()).toContain('/test-post-search?p1=1&p3=1%2C2'); }); + it('should throw when requestType is invalid', () => { + expect(() => { + makeApi({ + method: 'POST', + endpoint: '/test-formdata', + // @ts-ignore + requestType: 'text', + }); + }).toThrow('Invalid request payload type'); + }); + it('should handle errors', async () => { expect.assertions(1); const api = makeApi({ @@ -145,7 +156,7 @@ describe('makeApi()', () => { const api = makeApi({ method: 'POST', endpoint: '/test-200-error', - requestType: 'form', + requestType: 'json', }); fetchMock.post('glob:*/test-200-error', { error: 'not ok' }); try { From 185fb80dfa7c76f2e9f1c8597b9d71b8359e9557 Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Sun, 5 Jul 2020 01:15:18 -0700 Subject: [PATCH 8/9] Return raw error response --- packages/superset-ui-query/src/api/v1/handleError.ts | 5 ++++- packages/superset-ui-query/src/api/v1/types.ts | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/superset-ui-query/src/api/v1/handleError.ts b/packages/superset-ui-query/src/api/v1/handleError.ts index e93b9ee7b0..9ccf2d8daa 100644 --- a/packages/superset-ui-query/src/api/v1/handleError.ts +++ b/packages/superset-ui-query/src/api/v1/handleError.ts @@ -38,20 +38,23 @@ export default async function handleError(error: ErrorType): Promise { if (error instanceof Response) { const { status, statusText } = error; let errorMessage = `${status} ${statusText}`; + let originalError; if (status >= 400) { try { const json = (await error.json()) as | SupersetApiHttpErrorPayload | SupersetApiHttpMultiErrorsPayload; + originalError = json; const err = 'errors' in json ? json.errors[0] : json; errorMessage = err.message || err.error_type || errorMessage; } catch (error_) { - // pass + originalError = error; } throw new SupersetApiError({ status, statusText, message: errorMessage, + originalError, }); } else { throw new SupersetApiError({ diff --git a/packages/superset-ui-query/src/api/v1/types.ts b/packages/superset-ui-query/src/api/v1/types.ts index 47928e4279..be1d16b7b1 100644 --- a/packages/superset-ui-query/src/api/v1/types.ts +++ b/packages/superset-ui-query/src/api/v1/types.ts @@ -71,7 +71,7 @@ export class SupersetApiError extends Error { link?: string; - originalError?: Error | string | Response; + originalError?: Error | Response | JsonValue; constructor({ message, @@ -83,7 +83,7 @@ export class SupersetApiError extends Error { }: Omit & { message: string; stack?: Error['stack']; - originalError?: Error | string | Response; // original JavaScript error captured + originalError?: SupersetApiError['originalError']; // original JavaScript error captured }) { super(message); this.name = 'SupersetApiError'; From c5aba1bb7eaa82e0048fbf916be6ba2b509d9cd6 Mon Sep 17 00:00:00 2001 From: Jesse Yang Date: Mon, 6 Jul 2020 17:49:24 -0700 Subject: [PATCH 9/9] More robust error handler --- .../src/api/v1/handleError.ts | 95 ++++++++++--------- .../superset-ui-query/src/api/v1/makeApi.ts | 4 +- .../superset-ui-query/src/api/v1/types.ts | 84 +++++++++++----- .../test/api/v1/handleError.test.ts | 73 ++++++++++++-- 4 files changed, 178 insertions(+), 78 deletions(-) diff --git a/packages/superset-ui-query/src/api/v1/handleError.ts b/packages/superset-ui-query/src/api/v1/handleError.ts index 9ccf2d8daa..8cfe6c40e7 100644 --- a/packages/superset-ui-query/src/api/v1/handleError.ts +++ b/packages/superset-ui-query/src/api/v1/handleError.ts @@ -16,75 +16,76 @@ * specific language governing permissions and limitations * under the License. */ -import { - SupersetApiError, - SupersetApiErrorPayload, - SupersetApiHttpErrorPayload, - SupersetApiHttpMultiErrorsPayload, -} from './types'; +import { SupersetApiError, SupersetApiErrorPayload, SupersetApiMultiErrorsPayload } from './types'; -export type ErrorType = string | Error | Response | SupersetApiErrorPayload; +export type ErrorInput = string | Error | Response | SupersetApiErrorPayload; /** * Handle API request errors, convert to consistent Superset API error. * @param error the catched error from SupersetClient.request(...) */ -export default async function handleError(error: ErrorType): Promise { +export default async function handleError(error: ErrorInput): Promise { + // already a Sueprset error + if (error instanceof SupersetApiError) { + throw error; + } // string is the error message itself if (typeof error === 'string') { throw new SupersetApiError({ message: error }); } - // catch HTTP errors - if (error instanceof Response) { - const { status, statusText } = error; - let errorMessage = `${status} ${statusText}`; - let originalError; - if (status >= 400) { - try { - const json = (await error.json()) as - | SupersetApiHttpErrorPayload - | SupersetApiHttpMultiErrorsPayload; - originalError = json; - const err = 'errors' in json ? json.errors[0] : json; - errorMessage = err.message || err.error_type || errorMessage; - } catch (error_) { - originalError = error; - } - throw new SupersetApiError({ - status, - statusText, - message: errorMessage, - originalError, - }); - } else { - throw new SupersetApiError({ - status, - statusText, - message: errorMessage, - }); - } - } // JS errors, normally happens before request was sent if (error instanceof Error) { throw new SupersetApiError({ message: error.message || 'Unknown Error', - stack: error.stack, - // pass along the raw error so consumer code can inspect trace stack originalError: error, }); } - // when API returns 200 but operation fails - // (see Python API json_error_response(...)) - if ('error' in error) { - const { error: message, ...rest } = error; + + let errorJson; + let originalError; + let errorMessage = 'Unknown Error'; + let status: number | undefined; + let statusText: string | undefined; + + // catch HTTP errors + if (error instanceof Response) { + status = error.status; + statusText = error.statusText; + errorMessage = `${status} ${statusText}`; + try { + errorJson = (await error.json()) as SupersetApiErrorPayload | SupersetApiMultiErrorsPayload; + originalError = errorJson; + } catch (error_) { + originalError = error; + } + } else if (error) { + errorJson = error; + } + + // when API returns 200 but operation fails (see Python API json_error_response(...)) + // or when frontend promise rejects with `{ error: ... }` + if (errorJson && ('error' in errorJson || 'message' in errorJson || 'errors' in errorJson)) { + let err; + if ('errors' in errorJson) { + err = errorJson.errors?.[0] || {}; + } else if (typeof errorJson.error === 'object') { + err = errorJson.error; + } else { + err = errorJson; + } + errorMessage = + err.message || (err.error as string | undefined) || err.error_type || errorMessage; throw new SupersetApiError({ - message, - ...rest, + status, + statusText, + message: errorMessage, + originalError, + ...err, }); } // all unknown error throw new SupersetApiError({ - message: 'Unknown Error', + message: errorMessage, originalError: error, }); } diff --git a/packages/superset-ui-query/src/api/v1/makeApi.ts b/packages/superset-ui-query/src/api/v1/makeApi.ts index 784d61d90d..b72562d021 100644 --- a/packages/superset-ui-query/src/api/v1/makeApi.ts +++ b/packages/superset-ui-query/src/api/v1/makeApi.ts @@ -26,7 +26,7 @@ import { Method, RequestBase, } from '@superset-ui/connection'; -import handleError, { ErrorType } from './handleError'; +import handleError, { ErrorInput } from './handleError'; import { SupersetApiRequestOptions, SupersetApiErrorPayload, ParsedResponseType } from './types'; const validRequestTypes = new Set(['form', 'json', 'search']); @@ -119,7 +119,7 @@ export default function makeApi< const typedResult = result as ParsedResponseType; return (processResponse ? processResponse(typedResult) : typedResult) as Result; } catch (error) { - return handleError(error as ErrorType); + return handleError(error as ErrorInput); } } diff --git a/packages/superset-ui-query/src/api/v1/types.ts b/packages/superset-ui-query/src/api/v1/types.ts index be1d16b7b1..270e7cb6e5 100644 --- a/packages/superset-ui-query/src/api/v1/types.ts +++ b/packages/superset-ui-query/src/api/v1/types.ts @@ -20,8 +20,9 @@ import { SupersetClientClass, SupersetClientInterface, - StrictJsonValue, + StrictJsonObject, JsonValue, + JsonObject, } from '@superset-ui/connection'; export type ParsedResponseType = T extends 'text' @@ -39,29 +40,53 @@ export interface SupersetApiRequestOptions { } /** - * API Error json response following the format in `json_error_response(...)` - * https://github.com/apache/incubator-superset/blob/8e23d4f369f35724b34b14def8a5a8bafb1d2ecb/superset/views/base.py#L94 + * Superset API error types. + * Ref: https://github.com/apache/incubator-superset/blob/318e5347bc6f88119725775baa4ab9a398a6f0b0/superset/errors.py#L24 + * + * TODO: migrate superset-frontend/src/components/ErrorMessage/types.ts over */ -export interface SupersetApiErrorPayload { - error: string; // error message returned from API - status?: number; - statusText?: string; - link?: string; +export enum SupersetApiErrorType { + // Generic unknown error + UNKNOWN_ERROR = 'UNKNOWN_ERROR', + + // Frontend errors + FRONTEND_CSRF_ERROR = 'FRONTEND_CSRF_ERROR', + FRONTEND_NETWORK_ERROR = 'FRONTEND_NETWORK_ERROR', + FRONTEND_TIMEOUT_ERROR = 'FRONTEND_TIMEOUT_ERROR', + + // DB Engine errors, + GENERIC_DB_ENGINE_ERROR = 'GENERIC_DB_ENGINE_ERROR', + + // Viz errors, + VIZ_GET_DF_ERROR = 'VIZ_GET_DF_ERROR', + UNKNOWN_DATASOURCE_TYPE_ERROR = 'UNKNOWN_DATASOURCE_TYPE_ERROR', + FAILED_FETCHING_DATASOURCE_INFO_ERROR = 'FAILED_FETCHING_DATASOURCE_INFO_ERROR', + + // Security access errors, + TABLE_SECURITY_ACCESS_ERROR = 'TABLE_SECURITY_ACCESS_ERROR', + DATASOURCE_SECURITY_ACCESS_ERROR = 'DATASOURCE_SECURITY_ACCESS_ERROR', + MISSING_OWNERSHIP_ERROR = 'MISSING_OWNERSHIP_ERROR', } /** - * API error json response following FlaskAppBuilder convention. E.g. - * response_404(message=...) -> { "message": ... } + * API Error json response from the backend (or fetch API in the frontend). + * See SIP-40 and SIP-41: https://github.com/apache/incubator-superset/issues/9298 */ -export interface SupersetApiHttpErrorPayload { - message: string; - error_type?: string; - extra?: StrictJsonValue; +export interface SupersetApiErrorPayload { + message?: string; // error message via FlaskAppBuilder, e.g. `response_404(message=...)` + error_type?: SupersetApiErrorType; level?: 'error' | 'warn' | 'info'; + extra?: StrictJsonObject; + /** + * Error message returned via `json_error_response`. + * Ref https://github.com/apache/incubator-superset/blob/8e23d4f369f35724b34b14def8a5a8bafb1d2ecb/superset/views/base.py#L94 + */ + error?: string | SupersetApiErrorPayload; + link?: string; } -export interface SupersetApiHttpMultiErrorsPayload { - errors: SupersetApiHttpErrorPayload[]; +export interface SupersetApiMultiErrorsPayload { + errors: SupersetApiErrorPayload[]; } export class SupersetApiError extends Error { @@ -69,31 +94,44 @@ export class SupersetApiError extends Error { statusText?: string; - link?: string; + errorType: SupersetApiErrorType; + + extra: JsonObject; originalError?: Error | Response | JsonValue; constructor({ - message, status, statusText, + message, link, + extra, stack, + error_type: errorType, originalError, }: Omit & { + status?: number; + statusText?: string; message: string; stack?: Error['stack']; - originalError?: SupersetApiError['originalError']; // original JavaScript error captured + // original JavaScript error or backend JSON response captured + originalError?: SupersetApiError['originalError']; }) { super(message); - this.name = 'SupersetApiError'; + const originalErrorStack = + stack || (originalError instanceof Error ? originalError.stack : undefined); this.stack = - stack && this.stack - ? [this.stack.split('\n')[0], ...stack.split('\n').slice(1)].join('\n') + originalErrorStack && this.stack + ? [this.stack.split('\n')[0], ...originalErrorStack.split('\n').slice(1)].join('\n') : this.stack; + this.name = 'SupersetApiError'; + this.errorType = errorType || SupersetApiErrorType.UNKNOWN_ERROR; + this.extra = extra || {}; + if (link) { + this.extra.link = link; + } this.status = status; this.statusText = statusText; - this.link = link; this.originalError = originalError; } } diff --git a/packages/superset-ui-query/test/api/v1/handleError.test.ts b/packages/superset-ui-query/test/api/v1/handleError.test.ts index b31e087650..e8219f2282 100644 --- a/packages/superset-ui-query/test/api/v1/handleError.test.ts +++ b/packages/superset-ui-query/test/api/v1/handleError.test.ts @@ -17,21 +17,38 @@ * under the License. */ import 'whatwg-fetch'; // for adding Response polyfill -import handleError, { ErrorType } from '../../../src/api/v1/handleError'; -import { SupersetApiError } from '../../../src/api/v1/types'; +import { JsonObject } from '@superset-ui/connection'; +import handleError, { ErrorInput } from '../../../src/api/v1/handleError'; +import { SupersetApiError, SupersetApiErrorType } from '../../../src/api/v1/types'; -async function testHandleError(inputError: ErrorType, message: string) { +async function testHandleError( + inputError: ErrorInput, + expected: string | JsonObject, +): Promise { try { await handleError(inputError); } catch (error) { const typedError = error as SupersetApiError; expect(typedError).toBeInstanceOf(SupersetApiError); - expect(typedError.message).toContain(message); + if (typeof expected === 'string') { + expect(typedError.message).toContain(expected); + } else { + expect(typedError).toEqual(expect.objectContaining(expected)); + } + return error; } + return new SupersetApiError({ message: 'Where is the error?' }); } describe('handleError()', () => { - it('should handle message string', async () => { + it('should throw error directly', async () => { + expect.assertions(3); + const input = new SupersetApiError({ message: 'timeout' }); + const output = await testHandleError(input, 'timeout'); + expect(input).toBe(output); + }); + + it('should handle error string', async () => { expect.assertions(2); await testHandleError('STOP', 'STOP'); }); @@ -57,6 +74,35 @@ describe('handleError()', () => { await testHandleError(mockResponse, 'BAD BAD'); }); + it('should handle response of single error', async () => { + expect.assertions(2); + const mockResponse = new Response( + '{ "error": "BAD BAD", "link": "https://superset.apache.org" }', + { + status: 403, + statusText: 'Access Denied', + }, + ); + await testHandleError(mockResponse, { + message: 'BAD BAD', + extra: { link: 'https://superset.apache.org' }, + }); + }); + + it('should handle single error object', async () => { + expect.assertions(2); + const mockError = { + error: { + message: 'Request timeout', + error_type: SupersetApiErrorType.FRONTEND_TIMEOUT_ERROR, + }, + }; + await testHandleError(mockError, { + message: 'Request timeout', + errorType: 'FRONTEND_TIMEOUT_ERROR', + }); + }); + it('should process multi errors in HTTP json', async () => { expect.assertions(2); const mockResponse = new Response('{ "errors": [{ "error_type": "NOT OK" }] }', { @@ -66,6 +112,20 @@ describe('handleError()', () => { await testHandleError(mockResponse, 'NOT OK'); }); + it('should handle invalid multi errors', async () => { + expect.assertions(4); + const mockResponse1 = new Response('{ "errors": [] }', { + status: 403, + statusText: 'Access Denied', + }); + const mockResponse2 = new Response('{ "errors": null }', { + status: 400, + statusText: 'Bad Request', + }); + await testHandleError(mockResponse1, '403 Access Denied'); + await testHandleError(mockResponse2, '400 Bad Request'); + }); + it('should fallback to statusText', async () => { expect.assertions(2); const mockResponse = new Response('{ "failed": "random ramble" }', { @@ -89,7 +149,8 @@ describe('handleError()', () => { }); it('should throw unknown error', async () => { - expect.assertions(2); + expect.assertions(4); await testHandleError(Promise.resolve('Some random things') as never, 'Unknown Error'); + await testHandleError(undefined as never, 'Unknown Error'); }); });