-
Notifications
You must be signed in to change notification settings - Fork 311
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
[#5105] improvement(server,client): Error code optimization about access control API #5144
Conversation
import com.google.errorprone.annotations.FormatString; | ||
|
||
/** An exception thrown when a metadata object is invalid. */ | ||
public class IllegalMetadataObjectException extends IllegalArgumentException { |
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.
How do you define "illegal"?
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.
If a metadata object in the path doesn't exist, we will return NotFoundException.
If a metadata object in the request body doesn't exist, we will return IllegalException.
new UserResponse( | ||
DTOConverters.toDTO( | ||
accessControlManager.grantRolesToUser( | ||
metalake, request.getRoleNames(), user))))))); | ||
metalake, request.getRoleNames(), user)))); | ||
} catch (NoSuchRoleException nsr) { |
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.
I think it is better to handle IllegalRoleException
in the core, not convert to this exception in here.
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.
OK , just in my view, I usually throw IllegalxxxException
in the other Operation
class. It's ok to throw in the core module.
…t access control API
What changes were proposed in this pull request?
Error code optimization about access control API
Why are the changes needed?
Fix: #5105
Does this PR introduce any user-facing change?
Yes.
How was this patch tested?
Modify some UTs