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/01/31 00:42:14 UTC
[GitHub] [incubator-superset] willbarrett opened a new pull request #9056:
Do not show stacktraces on some intentionally-thrown errors
willbarrett opened a new pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056
### CATEGORY
Choose one
- [X] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Some endpoints on Superset, notably the connection test endpoint, would display full stacktraces when a connection test failed. We want to display the underlying error message to help with configuration of database connections, but not the full stacktrace. The connection test endpoint was also returning a 500 when a connection did not succeed - a 400 would be more accurate, as the system is attempting to validate user input.
Eventually this should be moved under /api/v1, but I'll save that for a different PR.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
- [ ] Attempt a connection test that fails
- [ ] Verify that the resulting response does not contain a stacktrace
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [X] 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] willbarrett commented on a change in pull
request #9056: Do not show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056#discussion_r374226728
##########
File path: superset/views/core.py
##########
@@ -1363,7 +1363,9 @@ def testconn(self):
except Exception as e:
logging.exception(e)
return json_error_response(
- "Connection failed!\n\n" "The error message returned was:\n{}".format(e)
+ "Connection failed!\n\n"
+ "The error message returned was:\n{}".format(e),
Review comment:
Yes, that clarifies it. Thank you!
----------------------------------------------------------------
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
#9056: Do not show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056#discussion_r374763115
##########
File path: superset/views/core.py
##########
@@ -1363,7 +1363,9 @@ def testconn(self):
except Exception as e:
logging.exception(e)
return json_error_response(
- "Connection failed!\n\n" "The error message returned was:\n{}".format(e)
+ "Connection failed!\n\n"
+ "The error message returned was:\n{}".format(e),
Review comment:
Sorry, I might have been unclear in my comment. What I meant to say was that in the above case I think using an f-string should be ok, and that f-strings should mainly be avoided in logging messages. The same applies to translations, in which variable text should not be burned into the message prior to translation.
----------------------------------------------------------------
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] willbarrett commented on a change in pull
request #9056: Do not show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056#discussion_r374798394
##########
File path: superset/views/core.py
##########
@@ -1363,7 +1363,9 @@ def testconn(self):
except Exception as e:
logging.exception(e)
return json_error_response(
- "Connection failed!\n\n" "The error message returned was:\n{}".format(e)
+ "Connection failed!\n\n"
+ "The error message returned was:\n{}".format(e),
Review comment:
OK @villebro I'm confused. What change precisely are you looking for on this PR?
----------------------------------------------------------------
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
#9056: Do not show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056#discussion_r374763115
##########
File path: superset/views/core.py
##########
@@ -1363,7 +1363,9 @@ def testconn(self):
except Exception as e:
logging.exception(e)
return json_error_response(
- "Connection failed!\n\n" "The error message returned was:\n{}".format(e)
+ "Connection failed!\n\n"
+ "The error message returned was:\n{}".format(e),
Review comment:
Sorry @willbarrett , I might have been unclear in my comment. What I meant to say was that in the above case I think using an f-string should be ok, and that f-strings should mainly be avoided in logging messages. The same applies to translations, in which variable text should not be burned into the message prior to translation.
----------------------------------------------------------------
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
#9056: Do not show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056#discussion_r373761136
##########
File path: superset/views/core.py
##########
@@ -1363,7 +1363,9 @@ def testconn(self):
except Exception as e:
logging.exception(e)
return json_error_response(
- "Connection failed!\n\n" "The error message returned was:\n{}".format(e)
+ "Connection failed!\n\n"
+ "The error message returned was:\n{}".format(e),
Review comment:
As Superset moved to Python 3.6+, we now generally prefer to use f-strings over `"".format()` whenever possible. This is mostly due to f-strings being more readable, but they are usually also slightly more performant in normal usage. While the codebase is still filled with `format()`,when touching old code the convention has been to change them to f-strings. An exception to this rule is logging, which `pylint` (I believe) checks for, for which one should generally use `logger.debug('message: %s', c)` style. Check https://docs.python.org/3/howto/logging.html#optimization
Hope this helps!
----------------------------------------------------------------
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
#9056: Do not show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056#discussion_r374803610
##########
File path: superset/views/core.py
##########
@@ -1363,7 +1363,9 @@ def testconn(self):
except Exception as e:
logging.exception(e)
return json_error_response(
- "Connection failed!\n\n" "The error message returned was:\n{}".format(e)
+ "Connection failed!\n\n"
+ "The error message returned was:\n{}".format(e),
Review comment:
@willbarrett in my original comment I was pointing out when one should and should not use f-strings. In the case above I would personally write
```python
return json_error_response(
"Connection failed!\n\n"
f"The error message returned was:\n{e}",
400,
)
```
----------------------------------------------------------------
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 merged pull request #9056: Do not
show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
villebro merged pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056
----------------------------------------------------------------
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] willbarrett commented on a change in pull
request #9056: Do not show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056#discussion_r374248764
##########
File path: superset/views/core.py
##########
@@ -1363,7 +1363,9 @@ def testconn(self):
except Exception as e:
logging.exception(e)
return json_error_response(
- "Connection failed!\n\n" "The error message returned was:\n{}".format(e)
+ "Connection failed!\n\n"
+ "The error message returned was:\n{}".format(e),
Review comment:
Hmm...in this case @villebro the line number you honed in on is returning an error response, not a logger call. I think that means `"".format()` is appropriate following the logic you posted?
----------------------------------------------------------------
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] willbarrett commented on a change in pull
request #9056: Do not show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056#discussion_r373720015
##########
File path: superset/views/core.py
##########
@@ -1363,7 +1363,9 @@ def testconn(self):
except Exception as e:
logging.exception(e)
return json_error_response(
- "Connection failed!\n\n" "The error message returned was:\n{}".format(e)
+ "Connection failed!\n\n"
+ "The error message returned was:\n{}".format(e),
Review comment:
Thanks for the review Ville! I'm happy to use either approach. As I'm still somewhat new to Python, it would be very useful for me to understand the reasoning behind your suggestion. Would you mind expanding a little bit?
----------------------------------------------------------------
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] willbarrett commented on a change in pull
request #9056: Do not show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056#discussion_r374806466
##########
File path: superset/views/core.py
##########
@@ -1363,7 +1363,9 @@ def testconn(self):
except Exception as e:
logging.exception(e)
return json_error_response(
- "Connection failed!\n\n" "The error message returned was:\n{}".format(e)
+ "Connection failed!\n\n"
+ "The error message returned was:\n{}".format(e),
Review comment:
updated
----------------------------------------------------------------
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
#9056: Do not show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
villebro commented on a change in pull request #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056#discussion_r373713534
##########
File path: superset/views/core.py
##########
@@ -1363,7 +1363,9 @@ def testconn(self):
except Exception as e:
logging.exception(e)
return json_error_response(
- "Connection failed!\n\n" "The error message returned was:\n{}".format(e)
+ "Connection failed!\n\n"
+ "The error message returned was:\n{}".format(e),
Review comment:
While logging is usually done with `format` to save cycles, in this instance I believe using a regular f-string is the correct course of action.
----------------------------------------------------------------
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 #9056: Do not
show stacktraces on some intentionally-thrown errors
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9056: Do not show stacktraces on some intentionally-thrown errors
URL: https://github.com/apache/incubator-superset/pull/9056#issuecomment-580868220
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9056?src=pr&el=h1) Report
> Merging [#9056](https://codecov.io/gh/apache/incubator-superset/pull/9056?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/2629c779af10b18d669d9af8612b92b7138bdd38?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9056?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9056 +/- ##
=======================================
Coverage 59.45% 59.45%
=======================================
Files 369 369
Lines 11747 11747
Branches 2888 2888
=======================================
Hits 6984 6984
Misses 4584 4584
Partials 179 179
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/9056?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [.../assets/src/SqlLab/components/AceEditorWrapper.jsx](https://codecov.io/gh/apache/incubator-superset/pull/9056/diff?src=pr&el=tree#diff-c3VwZXJzZXQvYXNzZXRzL3NyYy9TcWxMYWIvY29tcG9uZW50cy9BY2VFZGl0b3JXcmFwcGVyLmpzeA==) | `54.21% <0%> (ø)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9056?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/9056?src=pr&el=footer). Last update [2629c77...f72bbb4](https://codecov.io/gh/apache/incubator-superset/pull/9056?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