superset-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
Subject [GitHub] [incubator-superset] willbarrett commented on a change in pull request #10723: feat(databases): test connection api
Date Tue, 01 Sep 2020 23:13:21 GMT

willbarrett commented on a change in pull request #10723:
URL: https://github.com/apache/incubator-superset/pull/10723#discussion_r481478830



##########
File path: superset/databases/dao.py
##########
@@ -0,0 +1,75 @@
+# 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 logging
+from contextlib import closing
+
+from flask import g
+from sqlalchemy import select
+
+from superset import app
+from superset.dao.base import BaseDAO
+from superset.extensions import db
+from superset.models.core import Database
+from superset.security.analytics_db_safety import check_sqlalchemy_uri
+
+logger = logging.getLogger(__name__)
+
+
+class DatabaseDAO(BaseDAO):

Review comment:
       This probably makes more sense as a Command rather than a DAO since it is more of a
business operation.

##########
File path: superset/databases/dao.py
##########
@@ -0,0 +1,75 @@
+# 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 logging
+from contextlib import closing
+
+from flask import g
+from sqlalchemy import select
+
+from superset import app
+from superset.dao.base import BaseDAO
+from superset.extensions import db
+from superset.models.core import Database
+from superset.security.analytics_db_safety import check_sqlalchemy_uri
+
+logger = logging.getLogger(__name__)
+
+
+class DatabaseDAO(BaseDAO):
+    model_cls = Database
+
+    @staticmethod
+    def test_connection(  # pylint: disable=too-many-arguments, too-many-return-statements
+        db_name: str,
+        uri: str,
+        server_cert: str,
+        extra: str,
+        impersonate_user: bool,
+        encrypted_extra: str,
+    ) -> None:
+        if app.config["PREVENT_UNSAFE_DB_CONNECTIONS"]:
+            check_sqlalchemy_uri(uri)
+        # 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:
+            existing_database = (
+                db.session.query(Database)
+                .filter_by(database_name=db_name)
+                .one_or_none()
+            )
+            if existing_database and uri == existing_database.safe_sqlalchemy_uri():
+                uri = existing_database.sqlalchemy_uri_decrypted
+
+        # this is the database instance that will be tested
+        database = Database(

Review comment:
       Here through line 70 could also potentially be a DAO method - something like `build_db_for_connection_test`

##########
File path: superset/databases/api.py
##########
@@ -335,3 +351,88 @@ def select_star(
             return self.response(404, message="Table not found on the database")
         self.incr_stats("success", self.select_star.__name__)
         return self.response(200, result=result)
+
+    @expose("/test_connection", methods=["POST"])
+    @protect()
+    @safe
+    @event_logger.log_this
+    @statsd_metrics
+    def test_connection(  # pylint: disable=too-many-return-statements
+        self,
+    ) -> FlaskResponse:
+        """Tests a database connection
+        ---
+        post:
+          description: >-
+            Tests a database connection
+          requestBody:
+            description: Database schema
+            required: true
+            content:
+              application/json:
+                schema:
+                  type: object
+                  properties:
+                    encrypted_extra:
+                      type: object
+                    extras:
+                      type: object
+                    name:
+                      type: string
+                    server_cert:
+                      type: string
+          responses:
+            200:
+              description: Database Test Connection
+              content:
+                application/json:
+                  schema:
+                    type: object
+                    properties:
+                      message:
+                        type: string
+            400:
+              $ref: '#/components/responses/400'
+            422:
+              $ref: '#/components/responses/422'
+            500:
+              $ref: '#/components/responses/500'
+        """
+        uri = request.json.get("uri")
+        try:
+            DatabaseDAO.test_connection(
+                db_name=request.json.get("name"),
+                uri=uri,
+                server_cert=request.json.get("server_cert"),
+                extra=json.dumps(request.json.get("extras", {})),
+                impersonate_user=request.json.get("impersonate_user"),
+                encrypted_extra=json.dumps(request.json.get("encrypted_extra", {})),
+            )
+            return self.response(200, message="OK")
+        except CertificateException as ex:
+            logger.info("Certificate exception")
+            return self.response(500, message=ex.message)
+        except (NoSuchModuleError, ModuleNotFoundError):
+            logger.info("Invalid driver")
+            driver_name = make_url(uri).drivername
+            message = "Could not load database driver: %s" % (driver_name)
+            return self.response(400, message=message, driver_name=driver_name)
+        except ArgumentError:
+            logger.info("Invalid URI")
+            return self.response_422(
+                message="Invalid connection string, a valid string usually follows:\n"
+                "'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
+            )
+        except OperationalError:
+            logger.warning("Connection failed")
+            return self.response(
+                500, message="Connection failed, please check your connection settings"

Review comment:
       I think we're missing the `_()` function that will hook these messages up with their
associated translations.

##########
File path: superset/views/core.py
##########
@@ -1162,7 +1133,7 @@ def testconn(  # pylint: disable=too-many-return-statements,no-self-use
             logger.warning("Stopped an unsafe database connection")
             return json_error_response(_(str(ex)), 400)
         except Exception as ex:  # pylint: disable=broad-except
-            logger.error("Unexpected error %s", type(ex).__name__)
+            logger.warning("Unexpected error %s", type(ex).__name__)

Review comment:
       👍 

##########
File path: superset/databases/api.py
##########
@@ -335,3 +351,88 @@ def select_star(
             return self.response(404, message="Table not found on the database")
         self.incr_stats("success", self.select_star.__name__)
         return self.response(200, result=result)
+
+    @expose("/test_connection", methods=["POST"])
+    @protect()
+    @safe
+    @event_logger.log_this
+    @statsd_metrics
+    def test_connection(  # pylint: disable=too-many-return-statements
+        self,
+    ) -> FlaskResponse:
+        """Tests a database connection
+        ---
+        post:
+          description: >-
+            Tests a database connection
+          requestBody:
+            description: Database schema
+            required: true
+            content:
+              application/json:
+                schema:
+                  type: object
+                  properties:
+                    encrypted_extra:
+                      type: object
+                    extras:
+                      type: object
+                    name:
+                      type: string
+                    server_cert:
+                      type: string
+          responses:
+            200:
+              description: Database Test Connection
+              content:
+                application/json:
+                  schema:
+                    type: object
+                    properties:
+                      message:
+                        type: string
+            400:
+              $ref: '#/components/responses/400'
+            422:
+              $ref: '#/components/responses/422'
+            500:
+              $ref: '#/components/responses/500'
+        """
+        uri = request.json.get("uri")
+        try:
+            DatabaseDAO.test_connection(
+                db_name=request.json.get("name"),
+                uri=uri,
+                server_cert=request.json.get("server_cert"),
+                extra=json.dumps(request.json.get("extras", {})),
+                impersonate_user=request.json.get("impersonate_user"),
+                encrypted_extra=json.dumps(request.json.get("encrypted_extra", {})),
+            )
+            return self.response(200, message="OK")
+        except CertificateException as ex:
+            logger.info("Certificate exception")
+            return self.response(500, message=ex.message)
+        except (NoSuchModuleError, ModuleNotFoundError):
+            logger.info("Invalid driver")
+            driver_name = make_url(uri).drivername
+            message = "Could not load database driver: %s" % (driver_name)

Review comment:
       I think we could use an f-string here.

##########
File path: superset/databases/dao.py
##########
@@ -0,0 +1,75 @@
+# 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 logging
+from contextlib import closing
+
+from flask import g
+from sqlalchemy import select
+
+from superset import app
+from superset.dao.base import BaseDAO
+from superset.extensions import db
+from superset.models.core import Database
+from superset.security.analytics_db_safety import check_sqlalchemy_uri
+
+logger = logging.getLogger(__name__)
+
+
+class DatabaseDAO(BaseDAO):
+    model_cls = Database
+
+    @staticmethod
+    def test_connection(  # pylint: disable=too-many-arguments, too-many-return-statements
+        db_name: str,
+        uri: str,
+        server_cert: str,
+        extra: str,
+        impersonate_user: bool,
+        encrypted_extra: str,
+    ) -> None:
+        if app.config["PREVENT_UNSAFE_DB_CONNECTIONS"]:
+            check_sqlalchemy_uri(uri)
+        # 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:
+            existing_database = (

Review comment:
       This database lookup would be a good candidate for a DAO method.

##########
File path: superset/databases/api.py
##########
@@ -335,3 +351,88 @@ def select_star(
             return self.response(404, message="Table not found on the database")
         self.incr_stats("success", self.select_star.__name__)
         return self.response(200, result=result)
+
+    @expose("/test_connection", methods=["POST"])
+    @protect()
+    @safe
+    @event_logger.log_this
+    @statsd_metrics
+    def test_connection(  # pylint: disable=too-many-return-statements
+        self,
+    ) -> FlaskResponse:
+        """Tests a database connection
+        ---
+        post:
+          description: >-
+            Tests a database connection
+          requestBody:
+            description: Database schema
+            required: true
+            content:
+              application/json:
+                schema:
+                  type: object
+                  properties:
+                    encrypted_extra:
+                      type: object
+                    extras:
+                      type: object
+                    name:
+                      type: string
+                    server_cert:
+                      type: string
+          responses:
+            200:
+              description: Database Test Connection
+              content:
+                application/json:
+                  schema:
+                    type: object
+                    properties:
+                      message:
+                        type: string
+            400:
+              $ref: '#/components/responses/400'
+            422:
+              $ref: '#/components/responses/422'
+            500:
+              $ref: '#/components/responses/500'
+        """
+        uri = request.json.get("uri")
+        try:
+            DatabaseDAO.test_connection(
+                db_name=request.json.get("name"),
+                uri=uri,
+                server_cert=request.json.get("server_cert"),
+                extra=json.dumps(request.json.get("extras", {})),
+                impersonate_user=request.json.get("impersonate_user"),
+                encrypted_extra=json.dumps(request.json.get("encrypted_extra", {})),
+            )
+            return self.response(200, message="OK")
+        except CertificateException as ex:

Review comment:
       This breaks encapsulation a little bit. We're relying on SQLAlchemy errors to decide
the response format. Ideally we'd capture these errors in the DAO or Command and transform
them into a Superset-specific exception. That could allow us to group, for instance, the module
loading errors at a lower level.

##########
File path: superset/databases/api.py
##########
@@ -335,3 +351,88 @@ def select_star(
             return self.response(404, message="Table not found on the database")
         self.incr_stats("success", self.select_star.__name__)
         return self.response(200, result=result)
+
+    @expose("/test_connection", methods=["POST"])
+    @protect()
+    @safe
+    @event_logger.log_this
+    @statsd_metrics
+    def test_connection(  # pylint: disable=too-many-return-statements
+        self,
+    ) -> FlaskResponse:
+        """Tests a database connection
+        ---
+        post:
+          description: >-
+            Tests a database connection
+          requestBody:
+            description: Database schema
+            required: true
+            content:
+              application/json:
+                schema:
+                  type: object
+                  properties:
+                    encrypted_extra:
+                      type: object
+                    extras:
+                      type: object
+                    name:
+                      type: string
+                    server_cert:
+                      type: string
+          responses:
+            200:
+              description: Database Test Connection
+              content:
+                application/json:
+                  schema:
+                    type: object
+                    properties:
+                      message:
+                        type: string
+            400:
+              $ref: '#/components/responses/400'
+            422:
+              $ref: '#/components/responses/422'
+            500:
+              $ref: '#/components/responses/500'
+        """
+        uri = request.json.get("uri")
+        try:
+            DatabaseDAO.test_connection(
+                db_name=request.json.get("name"),
+                uri=uri,
+                server_cert=request.json.get("server_cert"),
+                extra=json.dumps(request.json.get("extras", {})),
+                impersonate_user=request.json.get("impersonate_user"),
+                encrypted_extra=json.dumps(request.json.get("encrypted_extra", {})),
+            )
+            return self.response(200, message="OK")
+        except CertificateException as ex:
+            logger.info("Certificate exception")
+            return self.response(500, message=ex.message)

Review comment:
       Since this is a connection test endpoint, let's return a 400 for this instead of a
500. We expect connection failures.




----------------------------------------------------------------
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



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


Mime
View raw message