feat: keep modal open when saving database failed (apache#11618)

superset-frontend/cypress-base/cypress/integration/database/helper.ts

@@ -0,0 +1,19 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+export const DATABASE_LIST = '/databaseview/list';

superset-frontend/cypress-base/cypress/integration/database/modal.test.ts

@@ -0,0 +1,73 @@
+/**
+ * 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 { DATABASE_LIST } from './helper';
+
+describe('Add database', () => {
+  beforeEach(() => {
+    cy.server();
+    cy.login();
+  });
+
+  it('should keep create modal open when error', () => {
+    cy.visit(DATABASE_LIST);
+
+    // open modal
+    cy.get('[data-test="btn-create-database"]').click();
+
+    // type values
+    cy.get('[data-test="database-modal"] input[name="database_name"]')
+      .focus()
+      .type('cypress');
+    cy.get('[data-test="database-modal"] input[name="sqlalchemy_uri"]')
+      .focus()
+      .type('bad_db_uri');
+
+    // click save
+    cy.get('[data-test="modal-confirm-button"]:not(:disabled)').click();
+
+    // should show error alerts and keep modal open
+    cy.get('.toast').contains('error');
+    cy.wait(1000); // wait for potential (incorrect) closing annimation
+    cy.get('[data-test="database-modal"]').should('be.visible');
+
+    // should be able to close modal
+    cy.get('[data-test="modal-cancel-button"]').click();
+    cy.get('[data-test="database-modal"]').should('not.be.visible');
+  });
+
+  it('should keep update modal open when error', () => {
+    // open modal
+    cy.get('[data-test="database-edit"]:last').click();
+
+    cy.get('[data-test="database-modal"]:last input[name="sqlalchemy_uri"]')
+      .focus()
+      .dblclick()
+      .type('{selectall}{backspace}bad_uri');
+
+    // click save
+    cy.get('[data-test="modal-confirm-button"]:not(:disabled)').click();
+
+    // should show error alerts
+    cy.get('.toast').contains('error').should('be.visible');
+
+    // modal should still be open
+    cy.wait(1000); // wait for potential (incorrect) closing annimation
+    cy.get('[data-test="database-modal"]').should('be.visible');
+  });
+});

superset-frontend/src/common/components/Modal/Modal.tsx

@@ -32,6 +32,7 @@ interface ModalProps {
   primaryButtonName?: string;
   primaryButtonType?: 'primary' | 'danger';
   show: boolean;
+  name?: string;
   title: React.ReactNode;
   width?: string;
   maxWidth?: string;
@@ -114,6 +115,7 @@ const CustomModal = ({
   primaryButtonName = t('OK'),
   primaryButtonType = 'primary',
   show,
+  name,
   title,
   width,
   maxWidth,
@@ -159,7 +161,7 @@ const CustomModal = ({
         </span>
       }
       footer={!hideFooter ? modalFooter : null}
-      wrapProps={{ 'data-test': `${title}-modal`, ...wrapProps }}
+      wrapProps={{ 'data-test': `${name || title}-modal`, ...wrapProps }}
       {...rest}
     >
       {children}

superset-frontend/src/utils/getClientErrorObject.ts

@@ -29,6 +29,8 @@ export type ClientErrorObject = {
   error: string;
   errors?: SupersetError[];
   link?: string;
+  // marshmallow field validation returns the error mssage in the format
+  // of { field: [msg1, msg2] }
   message?: string;
   severity?: string;
   stacktrace?: string;

superset-frontend/src/views/CRUD/data/database/DatabaseList.tsx

@@ -132,6 +132,7 @@ function DatabaseList({ addDangerToast, addSuccessToast }: DatabaseListProps) {
   if (canCreate) {
     menuData.buttons = [
       {
+        'data-test': 'btn-create-database',
         name: (
           <>
             <i className="fa fa-plus" /> {t('Database')}{' '}
@@ -295,6 +296,7 @@ function DatabaseList({ addDangerToast, addSuccessToast }: DatabaseListProps) {
                 >
                   <span
                     role="button"
+                    data-test="database-edit"
                     tabIndex={0}
                     className="action-button"
                     onClick={handleEdit}

superset-frontend/src/views/CRUD/data/database/DatabaseModal.tsx

@@ -175,7 +175,13 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
       .catch(response =>
         getClientErrorObject(response).then(error => {
           addDangerToast(
-            t('ERROR: Connection failed. ') + error?.message || '',
+            error?.message
+              ? `${t('ERROR: ')}${
+                  typeof error.message === 'string'
+                    ? error.message
+                    : (error.message as Record<string, string[]>).sqlalchemy_uri
+                }`
+              : t('ERROR: Connection failed. '),
           );
         }),
       );
@@ -202,22 +208,24 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
       }
 
       if (db && db.id) {
-        updateResource(db.id, update).then(() => {
-          if (onDatabaseAdd) {
-            onDatabaseAdd();
+        updateResource(db.id, update).then(result => {
+          if (result) {
+            if (onDatabaseAdd) {
+              onDatabaseAdd();
+            }
+            hide();
           }
-
-          hide();
         });
       }
     } else if (db) {
       // Create
-      createResource(db).then(() => {
-        if (onDatabaseAdd) {
-          onDatabaseAdd();
+      createResource(db).then(dbId => {
+        if (dbId) {
+          if (onDatabaseAdd) {
+            onDatabaseAdd();
+          }
+          hide();
         }
-
-        hide();
       });
     }
   };
@@ -307,6 +315,7 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
 
   return (
     <Modal
+      name="database"
       className="database-modal"
       disablePrimaryButton={disableSave}
       onHandledPrimaryAction={onSave}
@@ -356,7 +365,10 @@ const DatabaseModal: FunctionComponent<DatabaseModalProps> = ({
                 type="text"
                 name="sqlalchemy_uri"
                 value={db ? db.sqlalchemy_uri : ''}
-                placeholder={t('SQLAlchemy URI')}
+                autoComplete="off"
+                placeholder={t(
+                  'dialect+driver://username:password@host:port/database',
+                )}
                 onChange={onInputChange}
               />
               <Button buttonStyle="primary" onClick={testConnection} cta>

superset-frontend/src/views/CRUD/hooks.ts

@@ -211,6 +211,7 @@ export function useSingleViewResource<D extends object = any>(
           updateState({
             resource: json.result,
           });
+          return json.result;
         },
         createErrorHandler(errMsg =>
           handleErrorMsg(
@@ -243,13 +244,12 @@ export function useSingleViewResource<D extends object = any>(
           updateState({
             resource: json.result,
           });
-
           return json.id;
         },
         createErrorHandler(errMsg =>
           handleErrorMsg(
             t(
-              'An error occurred while fetching %ss: %s',
+              'An error occurred while creating %ss: %s',
               resourceLabel,
               JSON.stringify(errMsg),
             ),
@@ -277,6 +277,7 @@ export function useSingleViewResource<D extends object = any>(
           updateState({
             resource: json.result,
           });
+          return json.result;
         },
         createErrorHandler(errMsg =>
           handleErrorMsg(

superset/databases/schemas.py

@@ -131,15 +131,12 @@ def sqlalchemy_uri_validator(value: str) -> str:
     """
     try:
         make_url(value.strip())
-    except (ArgumentError, AttributeError):
+    except (ArgumentError, AttributeError, ValueError):
         raise ValidationError(
             [
                 _(
-                    "Invalid connection string, a valid string usually follows:"
-                    "'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
-                    "<p>"
-                    "Example:'postgresql://user:password@your-postgres-db/database'"
-                    "</p>"
+                    "Invalid connection string, a valid string usually follows: "
+                    "dirver://user:password@database-host/database-name"
                 )
             ]
         )

superset/models/core.py

@@ -46,6 +46,7 @@
 from sqlalchemy.engine import Dialect, Engine, url
 from sqlalchemy.engine.reflection import Inspector
 from sqlalchemy.engine.url import make_url, URL
+from sqlalchemy.exc import ArgumentError
 from sqlalchemy.ext.hybrid import hybrid_property
 from sqlalchemy.orm import relationship
 from sqlalchemy.pool import NullPool
@@ -646,7 +647,12 @@ def get_schema_access_for_csv_upload(  # pylint: disable=invalid-name
 
     @property
     def sqlalchemy_uri_decrypted(self) -> str:
-        conn = sqla.engine.url.make_url(self.sqlalchemy_uri)
+        try:
+            conn = sqla.engine.url.make_url(self.sqlalchemy_uri)
+        except (ArgumentError, ValueError):
+            # if the URI is invalid, ignore and return a placeholder url
+            # (so users see 500 less often)
+            return "dialect://invalid_uri"
         if custom_password_store:
             conn.password = custom_password_store(conn)
         else:

tests/databases/api_tests.py

@@ -289,17 +289,9 @@ def test_create_database_uri_validate(self):
         rv = self.client.post(uri, json=database_data)
         response = json.loads(rv.data.decode("utf-8"))
         self.assertEqual(rv.status_code, 400)
-        expected_response = {
-            "message": {
-                "sqlalchemy_uri": [
-                    "Invalid connection string, a valid string usually "
-                    "follows:'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
-                    "<p>Example:'postgresql://user:password@your-postgres-db/database'"
-                    "</p>"
-                ]
-            }
-        }
-        self.assertEqual(response, expected_response)
+        self.assertIn(
+            "Invalid connection string", response["message"]["sqlalchemy_uri"][0],
+        )
 
     def test_create_database_fail_sqllite(self):
         """
@@ -447,17 +439,9 @@ def test_update_database_uri_validate(self):
         rv = self.client.put(uri, json=database_data)
         response = json.loads(rv.data.decode("utf-8"))
         self.assertEqual(rv.status_code, 400)
-        expected_response = {
-            "message": {
-                "sqlalchemy_uri": [
-                    "Invalid connection string, a valid string usually "
-                    "follows:'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
-                    "<p>Example:'postgresql://user:password@your-postgres-db/database'"
-                    "</p>"
-                ]
-            }
-        }
-        self.assertEqual(response, expected_response)
+        self.assertIn(
+            "Invalid connection string", response["message"]["sqlalchemy_uri"][0],
+        )
 
     def test_delete_database(self):
         """