Add more log statements for authorization#13691
Closed
adarshsanjeev wants to merge 1 commit intoapache:masterfrom
Closed
Add more log statements for authorization#13691adarshsanjeev wants to merge 1 commit intoapache:masterfrom
adarshsanjeev wants to merge 1 commit intoapache:masterfrom
Conversation
paul-rogers
requested changes
Jan 18, 2023
| } catch (Exception e) { | ||
| log.error(e, "Planner threw an exception during authorization"); | ||
| throw new RuntimeException("Could not authorize resources for query", e); | ||
| } |
Contributor
There was a problem hiding this comment.
Why do we need this?
This code translates the unauthorized exception into a runtime exception, which will change the code flow. The desired flow is that we throw the unauthorized exception which is caught by a Jersey handler up the hander stack. (I seem to recall issues when I tried to intercept that error as you've done here.)
If we want to log auth errors, that would be the place. Else, in the SQL resource, we already log errors. And, the statement logs failures at the end of the query process.
As a result, this step seems unnecessary. What problem is this trying to solve?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds logging and for the case that an exception is thrown during authorization
This PR has: