From 4d24d4dc9a30b33937e152c4dcb68c130add478b Mon Sep 17 00:00:00 2001 From: Beto Dealmeida Date: Wed, 9 Jun 2021 18:05:31 -0700 Subject: [PATCH] fix: confirm overwrite and password on import (#15056) * fix: confirm overwrite and password on import * Add tests --- superset-frontend/src/views/CRUD/hooks.ts | 54 ++----- .../src/views/CRUD/utils.test.tsx | 145 ++++++++++++++++++ superset-frontend/src/views/CRUD/utils.tsx | 37 +++++ 3 files changed, 194 insertions(+), 42 deletions(-) create mode 100644 superset-frontend/src/views/CRUD/utils.test.tsx diff --git a/superset-frontend/src/views/CRUD/hooks.ts b/superset-frontend/src/views/CRUD/hooks.ts index aaa3599a6..785d608ac 100644 --- a/superset-frontend/src/views/CRUD/hooks.ts +++ b/superset-frontend/src/views/CRUD/hooks.ts @@ -20,7 +20,12 @@ import rison from 'rison'; import { useState, useEffect, useCallback } from 'react'; import { makeApi, SupersetClient, t, JsonObject } from '@superset-ui/core'; -import { createErrorHandler } from 'src/views/CRUD/utils'; +import { + createErrorHandler, + getAlreadyExists, + getPasswordsNeeded, + hasTerminalValidation, +} from 'src/views/CRUD/utils'; import { FetchDataConfig } from 'src/components/ListView'; import { FilterValue } from 'src/components/ListView/types'; import Chart, { Slice } from 'src/types/Chart'; @@ -384,40 +389,6 @@ export function useImportResource( setState(currentState => ({ ...currentState, ...update })); } - /* eslint-disable no-underscore-dangle */ - const isNeedsPassword = (payload: any) => - typeof payload === 'object' && - Array.isArray(payload._schema) && - payload._schema.length === 1 && - payload._schema[0] === 'Must provide a password for the database'; - - const isAlreadyExists = (payload: any) => - typeof payload === 'string' && - payload.includes('already exists and `overwrite=true` was not passed'); - - const getPasswordsNeeded = ( - errMsg: Record>, - ) => - Object.entries(errMsg) - .filter(([, validationErrors]) => isNeedsPassword(validationErrors)) - .map(([fileName]) => fileName); - - const getAlreadyExists = ( - errMsg: Record>, - ) => - Object.entries(errMsg) - .filter(([, validationErrors]) => isAlreadyExists(validationErrors)) - .map(([fileName]) => fileName); - - const hasTerminalValidation = ( - errMsg: Record>, - ) => - Object.values(errMsg).some( - validationErrors => - !isNeedsPassword(validationErrors) && - !isAlreadyExists(validationErrors), - ); - const importResource = useCallback( ( bundle: File, @@ -452,29 +423,28 @@ export function useImportResource( .then(() => true) .catch(response => getClientErrorObject(response).then(error => { - const errMsg = error.message || error.error; - if (typeof errMsg === 'string') { + if (!error.errors) { handleErrorMsg( t( 'An error occurred while importing %s: %s', resourceLabel, - parsedErrorMessage(errMsg), + error.message || error.error, ), ); return false; } - if (hasTerminalValidation(errMsg)) { + if (hasTerminalValidation(error.errors)) { handleErrorMsg( t( 'An error occurred while importing %s: %s', resourceLabel, - parsedErrorMessage(errMsg), + error.errors.map(payload => payload.message).join('\n'), ), ); } else { updateState({ - passwordsNeeded: getPasswordsNeeded(errMsg), - alreadyExists: getAlreadyExists(errMsg), + passwordsNeeded: getPasswordsNeeded(error.errors), + alreadyExists: getAlreadyExists(error.errors), }); } return false; diff --git a/superset-frontend/src/views/CRUD/utils.test.tsx b/superset-frontend/src/views/CRUD/utils.test.tsx new file mode 100644 index 000000000..24cb0ce97 --- /dev/null +++ b/superset-frontend/src/views/CRUD/utils.test.tsx @@ -0,0 +1,145 @@ +/** + * 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 { + isNeedsPassword, + isAlreadyExists, + getPasswordsNeeded, + getAlreadyExists, + hasTerminalValidation, +} from 'src/views/CRUD/utils'; + +const terminalErrors = { + errors: [ + { + message: 'Error importing database', + error_type: 'GENERIC_COMMAND_ERROR', + level: 'warning', + extra: { + 'metadata.yaml': { type: ['Must be equal to Database.'] }, + issue_codes: [ + { + code: 1010, + message: + 'Issue 1010 - Superset encountered an error while running a command.', + }, + ], + }, + }, + ], +}; + +const overwriteNeededErrors = { + errors: [ + { + message: 'Error importing database', + error_type: 'GENERIC_COMMAND_ERROR', + level: 'warning', + extra: { + 'databases/imported_database.yaml': + 'Database already exists and `overwrite=true` was not passed', + issue_codes: [ + { + code: 1010, + message: + 'Issue 1010 - Superset encountered an error while running a command.', + }, + ], + }, + }, + ], +}; + +const passwordNeededErrors = { + errors: [ + { + message: 'Error importing database', + error_type: 'GENERIC_COMMAND_ERROR', + level: 'warning', + extra: { + 'databases/imported_database.yaml': { + _schema: ['Must provide a password for the database'], + }, + issue_codes: [ + { + code: 1010, + message: + 'Issue 1010 - Superset encountered an error while running a command.', + }, + ], + }, + }, + ], +}; + +test('identifies error payloads indicating that password is needed', () => { + let needsPassword; + + needsPassword = isNeedsPassword({ + _schema: ['Must provide a password for the database'], + }); + expect(needsPassword).toBe(true); + + needsPassword = isNeedsPassword( + 'Database already exists and `overwrite=true` was not passed', + ); + expect(needsPassword).toBe(false); + + needsPassword = isNeedsPassword({ type: ['Must be equal to Database.'] }); + expect(needsPassword).toBe(false); +}); + +test('identifies error payloads indicating that overwrite confirmation is needed', () => { + let alreadyExists; + + alreadyExists = isAlreadyExists( + 'Database already exists and `overwrite=true` was not passed', + ); + expect(alreadyExists).toBe(true); + + alreadyExists = isAlreadyExists({ + _schema: ['Must provide a password for the database'], + }); + expect(alreadyExists).toBe(false); + + alreadyExists = isAlreadyExists({ type: ['Must be equal to Database.'] }); + expect(alreadyExists).toBe(false); +}); + +test('extracts DB configuration files that need passwords', () => { + const passwordsNeeded = getPasswordsNeeded(passwordNeededErrors.errors); + expect(passwordsNeeded).toEqual(['databases/imported_database.yaml']); +}); + +test('extracts files that need overwrite confirmation', () => { + const alreadyExists = getAlreadyExists(overwriteNeededErrors.errors); + expect(alreadyExists).toEqual(['databases/imported_database.yaml']); +}); + +test('detects if the error message is terminal or if it requires uses intervention', () => { + let isTerminal; + + isTerminal = hasTerminalValidation(terminalErrors.errors); + expect(isTerminal).toBe(true); + + isTerminal = hasTerminalValidation(overwriteNeededErrors.errors); + expect(isTerminal).toBe(false); + + isTerminal = hasTerminalValidation(passwordNeededErrors.errors); + expect(isTerminal).toBe(false); +}); diff --git a/superset-frontend/src/views/CRUD/utils.tsx b/superset-frontend/src/views/CRUD/utils.tsx index 716282f78..7e575c425 100644 --- a/superset-frontend/src/views/CRUD/utils.tsx +++ b/superset-frontend/src/views/CRUD/utils.tsx @@ -322,3 +322,40 @@ export const CardStyles = styled.div` text-decoration: none; } `; + +export /* eslint-disable no-underscore-dangle */ +const isNeedsPassword = (payload: any) => + typeof payload === 'object' && + Array.isArray(payload._schema) && + payload._schema.length === 1 && + payload._schema[0] === 'Must provide a password for the database'; + +export const isAlreadyExists = (payload: any) => + typeof payload === 'string' && + payload.includes('already exists and `overwrite=true` was not passed'); + +export const getPasswordsNeeded = (errors: Record[]) => + errors + .map(error => + Object.entries(error.extra) + .filter(([, payload]) => isNeedsPassword(payload)) + .map(([fileName]) => fileName), + ) + .flat(); + +export const getAlreadyExists = (errors: Record[]) => + errors + .map(error => + Object.entries(error.extra) + .filter(([, payload]) => isAlreadyExists(payload)) + .map(([fileName]) => fileName), + ) + .flat(); + +export const hasTerminalValidation = (errors: Record[]) => + errors.some( + error => + !Object.values(error.extra).some( + payload => isNeedsPassword(payload) || isAlreadyExists(payload), + ), + );