Implement missing methods in forwarding class #10087
Merged
Conversation
This file contains 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
losipiuk
reviewed
Nov 26, 2021
| @@ -137,6 +137,7 @@ default void checkCanViewQueryOwnedBy(SystemSecurityContext context, Identity qu | |||
| * will not be called when the current user is the query owner. | |||
| * | |||
| * @throws AccessDeniedException if not allowed | |||
| * @deprecated Implement {@link #checkCanViewQueryOwnedBy(SystemSecurityContext, Identity)} instead. | |||
| */ | |||
There was a problem hiding this comment.
Add @Deprecated annotation. Or should we remove it everywhere?
There was a problem hiding this comment.
yes, both should be present
losipiuk
reviewed
Nov 26, 2021
core/trino-spi/src/main/java/io/trino/spi/security/SystemAccessControl.java
Show resolved
Hide resolved
losipiuk
approved these changes
Nov 26, 2021
findepi
force-pushed
the
findepi/update-fwding
branch
from
bcfb44e to
71c5f3d
Compare
kokosing
reviewed
Nov 28, 2021
| @@ -137,7 +137,9 @@ default void checkCanViewQueryOwnedBy(SystemSecurityContext context, Identity qu | |||
| * will not be called when the current user is the query owner. | |||
| * | |||
| * @throws AccessDeniedException if not allowed | |||
| * @deprecated Implement {@link #checkCanViewQueryOwnedBy(SystemSecurityContext, Identity)} instead. | |||
There was a problem hiding this comment.
I there are three of them. To filter and to kill query too.
Typically we have never deprecated anything in access control interfaces. There are so many methods here and we start dealing with deprecation it will get messy IMO. I think I would simply drop deprecated methods.
We use deprecation once for checkCanSetUser. But there semantic was changing, here we only change the parameter.
There was a problem hiding this comment.
if we plan to drop, we should try to deprecate first, if possible. Here it seems possible.
|
CI #8611 |
findepi
force-pushed
the
findepi/update-fwding
branch
from
59825d6 to
e8dd6a2
Compare
|
(just rebased) |
`filterViewQuery` was introduced as a better version of `filterViewQueryOwnedBy`, so they should be overloads of each other, if possible.
It was superseded by a version taking identities in f7aac0d.
kokosing
approved these changes
Nov 29, 2021
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.
No description provided.