[GitHub] [incubator-superset] john-bodley commented on a change in pull request #7717: [WIP] Refactor testconn to use get_sqla_engine

[GitBox <gi...@apache.org>]

john-bodley commented on a change in pull request #7717: [WIP] Refactor testconn to use get_sqla_engine
URL: https://github.com/apache/incubator-superset/pull/7717#discussion_r301204546
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -2018,53 +2017,38 @@ def testconn(self):
             uri = request.json.get("uri")
             db_name = request.json.get("name")
             impersonate_user = request.json.get("impersonate_user")
-            database = None
+
+            # extras is sent as json, but required to be a string in the Database model
+            extra_str = json.dumps(request.json.get("extras", {}))
+
+            # if the database already exists in the database, only its safe (password-masked) URI
+            # would be shown in the UI and would be passed in the form data.
+            # so if the database already exists and the form was submitted with the safe URI,
+            # we assume we should retrieve the decrypted URI to test the connection.
             if db_name:
-                database = (
+                existing_database = (
                     db.session.query(models.Database)
                     .filter_by(database_name=db_name)
                     .first()
                 )
-                if database and uri == database.safe_sqlalchemy_uri():
-                    # the password-masked uri was passed
-                    # use the URI associated with this database
-                    uri = database.sqlalchemy_uri_decrypted
-
-            configuration = {}
-
-            if database and uri:
-                url = make_url(uri)
-                db_engine = models.Database.get_db_engine_spec_for_backend(
-                    url.get_backend_name()
-                )
-                db_engine.patch()
-
-                masked_url = database.get_password_masked_url_from_uri(uri)
-                logging.info("Superset.testconn(). Masked URL: {0}".format(masked_url))
-
-                configuration.update(
-                    db_engine.get_configuration_for_impersonation(
-                        uri, impersonate_user, username
-                    )
-                )
+                if existing_database and uri == existing_database.safe_sqlalchemy_uri():
+                    uri = existing_database.sqlalchemy_uri_decrypted
 
-            engine_params = request.json.get("extras", {}).get("engine_params", {})
-            connect_args = engine_params.get("connect_args")
-
-            if configuration and connect_args is not None:
-                connect_args["configuration"] = configuration
+            # this is the database instance that will be tested
+            database = models.Database(
+                extra=extra_str, impersonate_user=impersonate_user
 
 Review comment:
   Given that `extra_str` isn't used elsewhere I would probably just have all the content/context in one place, 
   
   ```
   database = models.Database(
       extra=json.dumps(request.json.get("extras", {})), 
       impersonate_user=impersonate_user,
   )
   ```

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org