feat: add extract_errors to Postgres #13997
Conversation
|
@etr2460, I did some small changes to |
| @@ -1133,54 +1133,41 @@ def get_function_names(cls, database: "Database") -> List[str]: | |||
| return database.get_df("SHOW FUNCTIONS")["Function"].tolist() | |||
|
|
|||
| @classmethod | |||
| def extract_errors(cls, ex: Exception) -> List[Dict[str, Any]]: | |||
| def extract_errors(cls, ex: Exception) -> List[SupersetError]: | |||
| raw_message = cls._extract_error_message(ex) | |||
|
|
|||
| column_match = re.search(COLUMN_NOT_RESOLVED_ERROR_REGEX, raw_message) | |||
There was a problem hiding this comment.
We can implement this error and TABLE_DOES_NOT_EXIST_ERROR_REGEX in custom_errors, and get rid of the extract_errors method here, like I did for Postgres. I didn't want to change it in this PR to keep it smaller.
Codecov Report
@@ Coverage Diff @@
## master #13997 +/- ##
==========================================
+ Coverage 79.21% 79.26% +0.05%
==========================================
Files 936 936
Lines 47408 47405 -3
Branches 5940 5938 -2
==========================================
+ Hits 37554 37577 +23
+ Misses 9727 9702 -25
+ Partials 127 126 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
betodealmeida
force-pushed
the
ch6434a
branch
2 times, most recently
from
10090ee to
03f524a
Compare
|
|
||
| self.login("admin") | ||
| data = { | ||
| "sqlalchemy_uri": "postgres://username:password@localhost:12345/db", | ||
| "sqlalchemy_uri": "postgres://username:password@locahost:12345/db", |
There was a problem hiding this comment.
| "sqlalchemy_uri": "postgres://username:password@locahost:12345/db", | |
| "sqlalchemy_uri": "postgres://username:password@badhost:12345/db", |
| @mock.patch("superset.databases.commands.test_connection.is_hostname_valid") | ||
| @mock.patch("superset.databases.commands.test_connection.is_port_open") | ||
| @mock.patch("superset.databases.commands.test_connection.is_host_up") | ||
| def test_test_connection_failed_closed_port( |
There was a problem hiding this comment.
Don't we still want to keep this test for the port being closed?
There was a problem hiding this comment.
We don't, we don't test this explicitly anymore, but instead rely on the driver to give this information. Because of this we only need one API test now.
* feat: add extract_errors to Postgres * Add unit tests * Fix lint * Fix unit tests
* feat: add extract_errors to Postgres * Add unit tests * Fix lint * Fix unit tests
mistercrunch
added
🏷️ bot
SUMMARY
This PR adds a new error
TEST_CONNECTION_INVALID_USERNAME_ERROR, for when the username provided to connect to a given database is invalid.It also removes the
_diagnosemethod, which tests if the host is up or down, leveragingextract_errorsfor that instead. I implemented the functionality for Postgres, and will implement in other popular engines in a next PR.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
Added unit tests.
ADDITIONAL INFORMATION