Do not show stacktraces on some intentionally-thrown errors #9056
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
superset/views/core.py
Outdated
| @@ -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), | |||
There was a problem hiding this 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.
There was a problem hiding this 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?
There was a problem hiding this 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!
There was a problem hiding this comment.
Yes, that clarifies it. Thank you!
There was a problem hiding this 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?
There was a problem hiding this 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.
There was a problem hiding this comment.
OK @villebro I'm confused. What change precisely are you looking for on this PR?
There was a problem hiding this 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
return json_error_response(
"Connection failed!\n\n"
f"The error message returned was:\n{e}",
400,
)
mistercrunch
added
🏷️ bot
CATEGORY
Choose one
SUMMARY
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.
The front-end of this endpoint was looking at the wrong key in the returned dictionary for the error message as well.
Eventually this should be moved under /api/v1, but I'll save that for a different PR.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION
REVIEWERS
@villebro @mistercrunch @etr2460