-
Notifications
You must be signed in to change notification settings - Fork 384
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
[#4886] feat(server,core): Supports to list roles by object #5023
Changes from 1 commit
3ccc9d5
92be694
dc2a910
00d31a2
ea048c8
130f517
cda140b
51014d5
2ad0607
f0dfc54
9de9732
79570fd
e93787d
eefdf8d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -184,6 +184,12 @@ void testManageRoles() { | |
String[] roleNames = metalake.listRoleNames(); | ||
Arrays.sort(roleNames); | ||
|
||
Assertions.assertEquals( | ||
Lists.newArrayList(anotherRoleName, roleName), Arrays.asList(roleNames)); | ||
|
||
// List roles by the object | ||
roleNames = metalake.listRoleNamesByObject(metalakeObject); | ||
Arrays.sort(roleNames); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you test list roles for different objects other than metalake? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added the test cases of catalog, schema and fileset. |
||
Assertions.assertEquals( | ||
Lists.newArrayList(anotherRoleName, roleName), Arrays.asList(roleNames)); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,8 +48,27 @@ enum Type { | |
* @return The list of entities | ||
* @throws IOException When occurs storage issues, it will throw IOException. | ||
*/ | ||
default <E extends Entity & HasIdentifier> List<E> listEntitiesByRelation( | ||
Type relType, NameIdentifier nameIdentifier, Entity.EntityType identType) throws IOException { | ||
return listEntitiesByRelation(relType, nameIdentifier, identType, true /* allFields*/); | ||
} | ||
|
||
/** | ||
* List the entities according to a give entity in a specific relation. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo: give-> given There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK. |
||
* | ||
* @param relType The type of relation. | ||
* @param nameIdentifier The given entity identifier | ||
* @param identType The given entity type. | ||
* @param allFields Some fields may have a relatively high acquisition cost, EntityStore provide | ||
* an optional setting to avoid fetching these high-cost fields to improve the performance. If | ||
* true, the method will fetch all the fields, Otherwise, the method will fetch all the fields | ||
* except for high-cost fields. | ||
* @return The list of entities | ||
* @throws IOException When occurs storage issues, it will throw IOException. | ||
*/ | ||
<E extends Entity & HasIdentifier> List<E> listEntitiesByRelation( | ||
Type relType, NameIdentifier nameIdentifier, Entity.EntityType identType) throws IOException; | ||
Type relType, NameIdentifier nameIdentifier, Entity.EntityType identType, boolean allFields) | ||
throws IOException; | ||
|
||
/** | ||
* insert a relation between two entities | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -369,9 +369,7 @@ public List<TagEntity> associateTagsWithMetadataObject( | |
|
||
@Override | ||
public <E extends Entity & HasIdentifier> List<E> listEntitiesByRelation( | ||
SupportsRelationOperations.Type relType, | ||
NameIdentifier nameIdentifier, | ||
Entity.EntityType identType) { | ||
Type relType, NameIdentifier nameIdentifier, Entity.EntityType identType, boolean allFields) { | ||
switch (relType) { | ||
case OWNER_REL: | ||
List<E> list = Lists.newArrayList(); | ||
|
@@ -382,20 +380,23 @@ public <E extends Entity & HasIdentifier> List<E> listEntitiesByRelation( | |
case METADATA_OBJECT_ROLE_REL: | ||
return (List<E>) | ||
RoleMetaService.getInstance() | ||
.listRolesByMetadataObjectIdentAndType(nameIdentifier, identType); | ||
.listRolesByMetadataObjectIdentAndType(nameIdentifier, identType, allFields); | ||
case ROLE_GROUP_REL: | ||
if (identType == Entity.EntityType.ROLE) { | ||
return (List<E>) GroupMetaService.getInstance().listGroupsByRoleIdent(nameIdentifier); | ||
} else { | ||
throw new IllegalArgumentException( | ||
String.format("ROLE_GROUP_REL doesn't support type %s", identType.name())); | ||
String.format( | ||
"ROLE_GROUP_REL doesn't support type %s or loading all fields", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible that ROLE_GROUP_REL does not support all fields? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now, we don't support all fields. We can support fetch all fields in the next pull request. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the meaning of this error message, I cannot quite follow your meaning. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed the error message, remove wrong message |
||
identType.name())); | ||
} | ||
case ROLE_USER_REL: | ||
if (identType == Entity.EntityType.ROLE) { | ||
return (List<E>) UserMetaService.getInstance().listUsersByRoleIdent(nameIdentifier); | ||
} else { | ||
throw new IllegalArgumentException( | ||
String.format("ROLE_USER_REL doesn't support type %s", identType.name())); | ||
String.format( | ||
"ROLE_USER_REL doesn't support type %s or loading all fields", identType.name())); | ||
} | ||
default: | ||
throw new 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.
It is better to design the APIs like tags, list roles from different entity object.
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.
Different from Tag API, role is related to authorization. Only we enable authorization, entity can list roles. So I prefer making a dedicated API to do this. WDYT?
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.
Do you honor authorization here? I don't see you honor the authorization here even you put the code 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, I can refer to the Tag to modify 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.
Think twice. Different from tag, tag don't support
metalake
type. role supportsmetalake
.If we add
listObjectRoleNames
API. It may make users confusing.What's the difference between
listObjectRoleNames
andlistRoleNames
for a metalake.If it's ok, I can modify this logic.
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.
We don't want to expose
GravitinoMetalake
. If we want to list roles forGravitinoMetalake
, it's hard.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 about using different method name like "listBindingRoleNames"?
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.
You can add more descriptions to tell about the difference.
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, I modified this code according to your suggestion.