feat(databases): test connection api #10723

lilykuang · 2020-08-28T23:54:37Z

SUMMARY

Add test connection functionality to /api/v1/database/test_connection

TEST PLAN

python test

ADDITIONAL INFORMATION

codecov-commenter · 2020-08-29T05:22:14Z

Codecov Report

Merging #10723 into master will increase coverage by 0.15%.
The diff coverage is 85.55%.

@@            Coverage Diff             @@
##           master   #10723      +/-   ##
==========================================
+ Coverage   65.40%   65.56%   +0.15%     
==========================================
  Files         802      803       +1     
  Lines       37872    37956      +84     
  Branches     3561     3561              
==========================================
+ Hits        24770    24884     +114     
+ Misses      12996    12966      -30     
  Partials      106      106

Flag	Coverage Δ
#cypress	`56.03% <ø> (-0.07%)`	⬇️
#javascript	`61.65% <ø> (ø)`
#python	`61.18% <85.55%> (+0.27%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/app.py	`80.15% <ø> (ø)`
superset/tasks/schedules.py	`75.74% <0.00%> (ø)`
superset/views/base_api.py	`98.25% <ø> (ø)`
superset/views/core.py	`74.48% <0.00%> (ø)`
superset/databases/api.py	`89.36% <70.96%> (-3.68%)`	⬇️
superset/databases/commands/test_connection.py	`94.73% <94.73%> (ø)`
superset/databases/commands/exceptions.py	`90.62% <100.00%> (+0.96%)`	⬆️
superset/databases/dao.py	`100.00% <100.00%> (ø)`
superset/databases/schemas.py	`99.34% <100.00%> (+0.03%)`	⬆️
...rontend/src/SqlLab/components/QueryAutoRefresh.jsx	`65.90% <0.00%> (-6.82%)`	⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b4a992...5d2e1f7. Read the comment docs.

willbarrett

A good bit of progress here! I've made a couple of recommendations on code structure. I think we could do a better job of centralizing the error handling coming out of SQLAlchemy to one location. We could potentially raise custom error classes that bring with them an associated response code and message - it looks like the language is the same between the endpoints, and that would DRY things up further. Happy to discuss the feedback if that would be helpful!

willbarrett · 2020-09-01T23:02:40Z

superset/databases/dao.py

+logger = logging.getLogger(__name__)
+
+
+class DatabaseDAO(BaseDAO):


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

willbarrett · 2020-09-01T23:03:05Z

superset/databases/dao.py

+        # with the safe URI, we assume we should retrieve the decrypted URI to test
+        # the connection.
+        if db_name:
+            existing_database = (


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

willbarrett · 2020-09-01T23:03:57Z

superset/databases/dao.py

+                uri = existing_database.sqlalchemy_uri_decrypted
+
+        # this is the database instance that will be tested
+        database = Database(


Here through line 70 could also potentially be a DAO method - something like build_db_for_connection_test

willbarrett · 2020-09-01T23:06:40Z

superset/databases/api.py

+                encrypted_extra=json.dumps(request.json.get("encrypted_extra", {})),
+            )
+            return self.response(200, message="OK")
+        except CertificateException as ex:


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.

willbarrett · 2020-09-01T23:07:12Z

superset/databases/api.py

+            return self.response(200, message="OK")
+        except CertificateException as ex:
+            logger.info("Certificate exception")
+            return self.response(500, message=ex.message)


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

willbarrett · 2020-09-01T23:09:55Z

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


willbarrett · 2020-09-01T23:11:50Z

superset/databases/api.py

+        except OperationalError:
+            logger.warning("Connection failed")
+            return self.response(
+                500, message="Connection failed, please check your connection settings"


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

willbarrett · 2020-09-01T23:12:03Z

superset/databases/api.py

+        except (NoSuchModuleError, ModuleNotFoundError):
+            logger.info("Invalid driver")
+            driver_name = make_url(uri).drivername
+            message = "Could not load database driver: %s" % (driver_name)


I think we could use an f-string here.

willbarrett

LGTM - thanks for all the work on this!

# Conflicts: # superset/databases/api.py # superset/databases/dao.py # tests/databases/api_tests.py

riahk · 2020-09-09T18:45:06Z

LGTM!!

* test connection api on databases * update test connection tests * update database api test and open api description * moved test connection to commands * update error message * fix isort * fix mypy * fix black * fix mypy pre commit

superset-github-bot bot added the preset-io label Aug 28, 2020

pull-request-size bot added the size/L label Aug 28, 2020

lilykuang force-pushed the lily/test-connection branch 3 times, most recently from be08ed2 to 9794ebd Compare September 1, 2020 01:51

lilykuang closed this Sep 1, 2020

lilykuang reopened this Sep 1, 2020

lilykuang force-pushed the lily/test-connection branch from 9794ebd to edc6c3f Compare September 1, 2020 16:37

lilykuang changed the title ~~feat: test databases connection api~~ feat(databases): test connection api Sep 1, 2020

lilykuang force-pushed the lily/test-connection branch from b5e32ec to 2493ae9 Compare September 1, 2020 22:08

willbarrett reviewed Sep 1, 2020

View reviewed changes

lilykuang force-pushed the lily/test-connection branch from 2493ae9 to 64317de Compare September 4, 2020 00:14

pull-request-size bot added size/XL and removed size/L labels Sep 4, 2020

lilykuang force-pushed the lily/test-connection branch from 346385a to 9a0e20f Compare September 4, 2020 00:49

pull-request-size bot added size/L and removed size/XL labels Sep 4, 2020

lilykuang force-pushed the lily/test-connection branch 2 times, most recently from c17e9f0 to 65ff87b Compare September 4, 2020 01:46

lilykuang requested a review from willbarrett September 4, 2020 01:47

lilykuang marked this pull request as ready for review September 4, 2020 01:58

Lily Kuang added 4 commits September 4, 2020 11:52

test connection api on databases

de48e29

update test connection tests

2e5fa06

update database api test and open api description

8ce1172

moved test connection to commands

2979056

lilykuang force-pushed the lily/test-connection branch from 65ff87b to df82f00 Compare September 4, 2020 18:52

update error message

ebee553

lilykuang force-pushed the lily/test-connection branch from df82f00 to ebee553 Compare September 4, 2020 19:06

willbarrett approved these changes Sep 4, 2020

View reviewed changes

willbarrett requested a review from dpgaspar September 4, 2020 22:16

Lily Kuang added 4 commits September 9, 2020 09:13

rebase master to test connection branch

81007dc

# Conflicts: # superset/databases/api.py # superset/databases/dao.py # tests/databases/api_tests.py

fix isort

abb7a5b

fix mypy

e36d592

fix black

20de464

lilykuang force-pushed the lily/test-connection branch from 7d185b1 to 20de464 Compare September 9, 2020 17:20

fix mypy pre commit

5d2e1f7

willbarrett merged commit 8a3ac70 into apache:master Sep 9, 2020

willbarrett deleted the lily/test-connection branch September 9, 2020 20:40

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.38.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(databases): test connection api #10723

feat(databases): test connection api #10723

lilykuang commented Aug 28, 2020 •

edited

Loading

codecov-commenter commented Aug 29, 2020 •

edited

Loading

willbarrett left a comment

willbarrett Sep 1, 2020

willbarrett Sep 1, 2020

willbarrett Sep 1, 2020

willbarrett Sep 1, 2020

willbarrett Sep 1, 2020

willbarrett Sep 1, 2020

willbarrett Sep 1, 2020

willbarrett Sep 1, 2020

willbarrett left a comment

riahk commented Sep 9, 2020

		logger = logging.getLogger(__name__)


		class DatabaseDAO(BaseDAO):

feat(databases): test connection api #10723

feat(databases): test connection api #10723

Conversation

lilykuang commented Aug 28, 2020 • edited Loading

SUMMARY

TEST PLAN

ADDITIONAL INFORMATION

codecov-commenter commented Aug 29, 2020 • edited Loading

Codecov Report

willbarrett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willbarrett left a comment

Choose a reason for hiding this comment

riahk commented Sep 9, 2020

lilykuang commented Aug 28, 2020 •

edited

Loading

codecov-commenter commented Aug 29, 2020 •

edited

Loading