-
Notifications
You must be signed in to change notification settings - Fork 13.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(15482): Propagate SupersetSecurityException error #15874
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, cc @betodealmeida
Codecov Report
@@ Coverage Diff @@
## master #15874 +/- ##
==========================================
- Coverage 77.08% 76.95% -0.13%
==========================================
Files 984 984
Lines 51787 51784 -3
Branches 7031 7031
==========================================
- Hits 39918 39851 -67
- Misses 11644 11708 +64
Partials 225 225
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Co-authored-by: John Bodley <john.bodley@airbnb.com> (cherry picked from commit 65728ea)
Co-authored-by: John Bodley <john.bodley@airbnb.com>
Co-authored-by: John Bodley <john.bodley@airbnb.com>
Co-authored-by: John Bodley <john.bodley@airbnb.com>
SUMMARY
I'm not sure why in #15482 we construct a new
SupersetError
(with corresponding message, error type, and level) rather than merely leveraging the mandatorySupersetError
which raised as part of theSupersetSecurityException
.These errors contain specifics (message, error type, etc.) which differ depending on the type of exception (example)—which also may be custom for security manager overrides—and thus should be preserved.
This PR merely uses the previous
SupersetError
(which is guaranteed to exist per the SupersetSecurityException signature) rather than constructing a new error.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION