You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/02/20 15:17:15 UTC

[GitHub] [incubator-superset] dpgaspar opened a new pull request #9178: [core] Fix, sanitize errors returned from testconn

dpgaspar opened a new pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178
 
 
   ### CATEGORY
   
   Choose one
   
   - [X] Bug Fix
   - [X] Enhancement (new features, refinement)
   - [ ] Refactor
   - [ ] Add tests
   - [ ] Build / Development Environment
   - [ ] Documentation
   
   ### SUMMARY
   Test connection API `testconn` does not sanitize errors generated from SQLAlchemy or dbapi drivers. This PR distinguish between URI parse errors, drInivers not installed on the system and OperationalError.
   
   ### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
   Before:
   
   
   
   After:
   
   Connection failed (OperationalError):
   ![image](https://user-images.githubusercontent.com/4025227/74947794-4c457480-53f3-11ea-8637-0c489dae98d6.png)
   
   Invalid URI:
   ![image](https://user-images.githubusercontent.com/4025227/74947842-5f584480-53f3-11ea-8e9c-89872554966d.png)
   
   Invalid driver:
   ![image](https://user-images.githubusercontent.com/4025227/74947910-75fe9b80-53f3-11ea-89c6-d186dde1574e.png)
   
   
   ### ADDITIONAL INFORMATION
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   
   ### REVIEWERS
   

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


[GitHub] [incubator-superset] villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#discussion_r382086135
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -1330,11 +1335,32 @@ def testconn(self):
             with closing(engine.connect()) as conn:
                 conn.scalar(select([1]))
                 return json_success('"OK"')
-        except Exception as e:
-            logger.exception(e)
+        except NoSuchModuleError as e:
+            logger.info(f"Invalid driver {e}")
+            driver_name = make_url(uri).drivername
             return json_error_response(
-                "Connection failed!\n\n" f"The error message returned was:\n{e}", 400
+                _(
+                    "Could not load database driver: %(driver_name)s",
+                    driver_name=driver_name,
+                ),
+                400,
             )
+        except ArgumentError as e:
+            logger.info(f"Invalid URI {e}")
+            return json_error_response(
+                _(
+                    "Invalid connnection string, a valid string follows: \n"
+                    " 'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
+                )
+            )
+        except OperationalError as e:
+            logger.warning(f"Connection failed {e}")
+            return json_error_response(
+                _("Connection failed, please check your connection settings."), 400
+            )
+        except Exception as e:
+            logger.error(f"Unexpected error {e}")
 
 Review comment:
   Same here

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


[GitHub] [incubator-superset] villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#discussion_r382636933
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -1330,11 +1335,32 @@ def testconn(self):
             with closing(engine.connect()) as conn:
                 conn.scalar(select([1]))
                 return json_success('"OK"')
-        except Exception as e:
-            logger.exception(e)
+        except NoSuchModuleError as e:
+            logger.info("Invalid driver %s", e)
+            driver_name = make_url(uri).drivername
             return json_error_response(
-                "Connection failed!\n\n" f"The error message returned was:\n{e}", 400
+                _(
+                    "Could not load database driver: %(driver_name)s",
+                    driver_name=driver_name,
+                ),
+                400,
             )
+        except ArgumentError as e:
+            logger.info("Invalid URI %s", e)
+            return json_error_response(
+                _(
+                    "Invalid connection string, a valid string usually follows: \n"
+                    "'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
 
 Review comment:
   Lol, you are absolutely right 🤦‍♂ carry on 😆 

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


[GitHub] [incubator-superset] codecov-io commented on issue #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#issuecomment-589595657
 
 
   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9178?src=pr&el=h1) Report
   > Merging [#9178](https://codecov.io/gh/apache/incubator-superset/pull/9178?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/e55fe43ca67a29518674a1a2137a3dbd4f166864?src=pr&el=desc) will **decrease** coverage by `0.03%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/9178/graphs/tree.svg?width=650&token=KsB0fHcx6l&height=150&src=pr)](https://codecov.io/gh/apache/incubator-superset/pull/9178?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #9178      +/-   ##
   ==========================================
   - Coverage   59.16%   59.13%   -0.04%     
   ==========================================
     Files         372      372              
     Lines       11935    11938       +3     
     Branches     2927     2925       -2     
   ==========================================
   - Hits         7061     7059       -2     
   - Misses       4694     4699       +5     
     Partials      180      180
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9178?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [...ontend/src/components/ListView/TableCollection.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9178/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvVGFibGVDb2xsZWN0aW9uLnRzeA==) | `88.88% <0%> (-3.71%)` | :arrow_down: |
   | [...frontend/src/views/dashboardList/DashboardList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9178/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2Rhc2hib2FyZExpc3QvRGFzaGJvYXJkTGlzdC50c3g=) | `69.72% <0%> (-3.43%)` | :arrow_down: |
   | [...uperset-frontend/src/views/chartList/ChartList.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9178/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3ZpZXdzL2NoYXJ0TGlzdC9DaGFydExpc3QudHN4) | `74.48% <0%> (-0.26%)` | :arrow_down: |
   | [...et-frontend/src/components/ConfirmStatusChange.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9178/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvQ29uZmlybVN0YXR1c0NoYW5nZS50c3g=) | `100% <0%> (ø)` | :arrow_up: |
   | [...rset-frontend/src/components/ListView/ListView.tsx](https://codecov.io/gh/apache/incubator-superset/pull/9178/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NvbXBvbmVudHMvTGlzdFZpZXcvTGlzdFZpZXcudHN4) | `92.4% <0%> (+0.19%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9178?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9178?src=pr&el=footer). Last update [e55fe43...df5ad14](https://codecov.io/gh/apache/incubator-superset/pull/9178?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

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


[GitHub] [incubator-superset] villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#discussion_r382136492
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -1330,11 +1335,32 @@ def testconn(self):
             with closing(engine.connect()) as conn:
                 conn.scalar(select([1]))
                 return json_success('"OK"')
-        except Exception as e:
-            logger.exception(e)
+        except NoSuchModuleError as e:
+            logger.info("Invalid driver %s", e)
+            driver_name = make_url(uri).drivername
             return json_error_response(
-                "Connection failed!\n\n" f"The error message returned was:\n{e}", 400
+                _(
+                    "Could not load database driver: %(driver_name)s",
+                    driver_name=driver_name,
+                ),
+                400,
             )
+        except ArgumentError as e:
+            logger.info("Invalid URI %s", e)
+            return json_error_response(
+                _(
+                    "Invalid connection string, a valid string usually follows: \n"
+                    "'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
 
 Review comment:
   This got me thinking, how about moving this example connstring to `BaseEngineContext` so engine specs can override it if they feel like it?

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


[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#discussion_r382144763
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -1330,11 +1335,32 @@ def testconn(self):
             with closing(engine.connect()) as conn:
                 conn.scalar(select([1]))
                 return json_success('"OK"')
-        except Exception as e:
-            logger.exception(e)
+        except NoSuchModuleError as e:
+            logger.info("Invalid driver %s", e)
+            driver_name = make_url(uri).drivername
             return json_error_response(
-                "Connection failed!\n\n" f"The error message returned was:\n{e}", 400
+                _(
+                    "Could not load database driver: %(driver_name)s",
+                    driver_name=driver_name,
+                ),
+                400,
             )
+        except ArgumentError as e:
+            logger.info("Invalid URI %s", e)
+            return json_error_response(
+                _(
+                    "Invalid connection string, a valid string usually follows: \n"
+                    "'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
 
 Review comment:
   Will look into that, need to address tests also. This is still draft

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


[GitHub] [incubator-superset] villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#discussion_r382082901
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -1330,11 +1335,32 @@ def testconn(self):
             with closing(engine.connect()) as conn:
                 conn.scalar(select([1]))
                 return json_success('"OK"')
-        except Exception as e:
-            logger.exception(e)
+        except NoSuchModuleError as e:
+            logger.info(f"Invalid driver {e}")
+            driver_name = make_url(uri).drivername
             return json_error_response(
-                "Connection failed!\n\n" f"The error message returned was:\n{e}", 400
+                _(
+                    "Could not load database driver: %(driver_name)s",
+                    driver_name=driver_name,
+                ),
+                400,
             )
+        except ArgumentError as e:
+            logger.info(f"Invalid URI {e}")
+            return json_error_response(
+                _(
+                    "Invalid connnection string, a valid string follows: \n"
+                    " 'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
 
 Review comment:
   The exact connection string varies from engine to engine. While adding a valid example to each engine spec might be overkill, it might be good to state that a valid connection string *usually* follows this pattern.

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


[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#discussion_r382494918
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -1330,11 +1335,32 @@ def testconn(self):
             with closing(engine.connect()) as conn:
                 conn.scalar(select([1]))
                 return json_success('"OK"')
-        except Exception as e:
-            logger.exception(e)
+        except NoSuchModuleError as e:
+            logger.info("Invalid driver %s", e)
+            driver_name = make_url(uri).drivername
             return json_error_response(
-                "Connection failed!\n\n" f"The error message returned was:\n{e}", 400
+                _(
+                    "Could not load database driver: %(driver_name)s",
+                    driver_name=driver_name,
+                ),
+                400,
             )
+        except ArgumentError as e:
+            logger.info("Invalid URI %s", e)
+            return json_error_response(
+                _(
+                    "Invalid connection string, a valid string usually follows: \n"
+                    "'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
 
 Review comment:
   Not super comfortable with `BaseEngineSpec` but, If a user does not provide a valid URI we can't infer the engine, so I think it's not a good place for that error message. Note this will eventually be migrated, and then we can create some nice custom Exceptions for the API to use. 
   Is this valid reasoning?

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


[GitHub] [incubator-superset] dpgaspar commented on issue #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on issue #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#issuecomment-589162305
 
 
   Thank you, once more @villebro, addressed the comments

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


[GitHub] [incubator-superset] villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#discussion_r382087324
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -1330,11 +1335,32 @@ def testconn(self):
             with closing(engine.connect()) as conn:
                 conn.scalar(select([1]))
                 return json_success('"OK"')
-        except Exception as e:
-            logger.exception(e)
+        except NoSuchModuleError as e:
+            logger.info(f"Invalid driver {e}")
+            driver_name = make_url(uri).drivername
             return json_error_response(
-                "Connection failed!\n\n" f"The error message returned was:\n{e}", 400
+                _(
+                    "Could not load database driver: %(driver_name)s",
+                    driver_name=driver_name,
+                ),
+                400,
             )
+        except ArgumentError as e:
+            logger.info(f"Invalid URI {e}")
+            return json_error_response(
+                _(
+                    "Invalid connnection string, a valid string follows: \n"
+                    " 'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
 
 Review comment:
   nit: leading space in example connection string

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


[GitHub] [incubator-superset] villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#discussion_r382086041
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -1330,11 +1335,32 @@ def testconn(self):
             with closing(engine.connect()) as conn:
                 conn.scalar(select([1]))
                 return json_success('"OK"')
-        except Exception as e:
-            logger.exception(e)
+        except NoSuchModuleError as e:
+            logger.info(f"Invalid driver {e}")
+            driver_name = make_url(uri).drivername
             return json_error_response(
-                "Connection failed!\n\n" f"The error message returned was:\n{e}", 400
+                _(
+                    "Could not load database driver: %(driver_name)s",
+                    driver_name=driver_name,
+                ),
+                400,
             )
+        except ArgumentError as e:
+            logger.info(f"Invalid URI {e}")
+            return json_error_response(
+                _(
+                    "Invalid connnection string, a valid string follows: \n"
+                    " 'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
+                )
+            )
+        except OperationalError as e:
+            logger.warning(f"Connection failed {e}")
 
 Review comment:
   Same here

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


[GitHub] [incubator-superset] dpgaspar merged pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178
 
 
   

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


[GitHub] [incubator-superset] villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#discussion_r382086487
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -1330,11 +1335,32 @@ def testconn(self):
             with closing(engine.connect()) as conn:
                 conn.scalar(select([1]))
                 return json_success('"OK"')
-        except Exception as e:
-            logger.exception(e)
+        except NoSuchModuleError as e:
+            logger.info(f"Invalid driver {e}")
 
 Review comment:
   I think the convention is to avoid f-strings for logging: https://docs.python.org/3/howto/logging.html#optimization

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


[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#discussion_r382102952
 
 

 ##########
 File path: superset/views/core.py
 ##########
 @@ -1330,11 +1335,32 @@ def testconn(self):
             with closing(engine.connect()) as conn:
                 conn.scalar(select([1]))
                 return json_success('"OK"')
-        except Exception as e:
-            logger.exception(e)
+        except NoSuchModuleError as e:
+            logger.info(f"Invalid driver {e}")
+            driver_name = make_url(uri).drivername
             return json_error_response(
-                "Connection failed!\n\n" f"The error message returned was:\n{e}", 400
+                _(
+                    "Could not load database driver: %(driver_name)s",
+                    driver_name=driver_name,
+                ),
+                400,
             )
+        except ArgumentError as e:
+            logger.info(f"Invalid URI {e}")
+            return json_error_response(
+                _(
+                    "Invalid connnection string, a valid string follows: \n"
+                    " 'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
 
 Review comment:
   Done

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


[GitHub] [incubator-superset] villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn

Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9178: [core] Fix, sanitize errors returned from testconn
URL: https://github.com/apache/incubator-superset/pull/9178#discussion_r382135824
 
 

 ##########
 File path: superset/views/database/validators.py
 ##########
 @@ -36,7 +36,7 @@ def sqlalchemy_uri_validator(
     except (ArgumentError, AttributeError):
         raise exception(
             _(
-                "Invalid connnection string, a valid string follows: "
+                "Invalid connection string, a valid string usually follows: "
                 " 'DRIVER://USER:PASSWORD@DB-HOST/DATABASE-NAME'"
 
 Review comment:
   Not yours, but the leading space is here, too.

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