tengqm commented on code in PR #5629:
URL: https://github.com/apache/gravitino/pull/5629#discussion_r1851406647
##########
authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java:
##########
@@ -480,8 +505,8 @@ protected void updatePolicyOwnerRole(RangerPolicy policy,
String ownerRoleName)
policyItem
.getAccesses()
.add(new
RangerPolicy.RangerPolicyItemAccess(ownerPrivilege.getName()));
- if (!policyItem.getRoles().contains(ownerRoleName)) {
- policyItem.getRoles().add(ownerRoleName);
+ if
(!policyItem.getRoles().contains(rangerRoleName(ownerRoleName))) {
+ policyItem.getRoles().add(rangerRoleName(ownerRoleName));
Review Comment:
Your current code may work as expected, what I was trying to point out is
that the pattern may be improved. A better logic should be this:
```java
RangerRoles roles = policyItem.getRoles();
if (!roles.contains(ownerRoleName)) {
roles.add(rangerRoleName(ownerRoleName));
policyItem.setRoles(roles);
}
```
In other words, you do a GET and then you do a SET. The thing(s) you get from
`getRoles()` is a reference in Java. That is why your current code works.
So ... my point was not that your code is wrong. It is not a pattern for
robust code IMHO.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
