[#3346] feat(core,server): Supports to list roles operations #4894

jerqi · 2024-09-09T09:28:08Z

What changes were proposed in this pull request?

Supports to list roles operations

Why are the changes needed?

Fix: #3346

Does this PR introduce any user-facing change?

Yes, will add the document later.

How was this patch tested?

Add UTs.

jerryshao · 2024-09-24T09:15:38Z

api/src/main/java/org/apache/gravitino/authorization/Role.java

+   *
+   * @return The scecurable objects count of the role.
+   */
+  default int securableObjectsCount() {


I think it would be better rename to count(), what do you think?

If we add more fields, it may be confusing for users. For example, if we add child role in the role in the future. If I use count, I don't know it's the child role count or objects count.

jerryshao · 2024-09-24T09:21:58Z

clients/client-java/src/main/java/org/apache/gravitino/client/GenericRole.java

+    this.roleDTO = roleDTO;
+    this.securableObjectsSupplier =
+        new Supplier<List<SecurableObject>>() {
+          private boolean waitToRequest = true;


waitForRequset.

jerryshao · 2024-09-24T09:22:47Z

clients/client-java/src/main/java/org/apache/gravitino/client/GenericRole.java

+            if (waitToRequest) {
+              securableObjects = gravitinoMetalake.getRole(roleDTO.name()).securableObjects();
+              waitToRequest = false;
+            }


Should we add a lock here to avoid concurrency issue here?

OK. I can add the lock later.

jerryshao · 2024-09-24T09:31:27Z

common/src/main/java/org/apache/gravitino/dto/authorization/RoleDTO.java

  @JsonProperty("securableObjects")
  private SecurableObjectDTO[] securableObjects;

+  @JsonProperty("securableObjectsCount")
+  private int securableObjectsCount;


Maybe we can also change to count here? Also, what is the serialization result of this field, "0" or "null'?

Name has similar reason above. This field is 0 if we don't have any objects in the role.

jerryshao · 2024-09-24T09:36:00Z

clients/client-java/src/main/java/org/apache/gravitino/client/GenericRole.java

+          @Override
+          public List<SecurableObject> get() {
+            if (waitToRequest) {
+              securableObjects = gravitinoMetalake.getRole(roleDTO.name()).securableObjects();


I think you can have a new API to get securable objects, rather than using get role API, you can follow what Tag did.

I ever thought. But from my view, our role API can already get the objects. It's a little weird to add a new API to fetch partial data.

It's weird to get a role in a role from client side.

xunliu

Problems that can be solved with an enumeration operation type. Get the 4 difficult concepts of SupportsDesiredFieldsHandlers.java, SupportsDesiredFields, skippingFields, and desiredFields.
I don't think it's a good design.

In particular, desiredFields and execute() in SupportsDesiredFields don't have a strong logical relationship

interface SupportsDesiredFields<R> {

  /**
   * The fields which could be desired.
   *
   * @return The fields which are desired.
   */
  Set<Field> desiredFields();

  /**
   * The return value of the handler.
   *
   * @return The return value of the handler.
   */
  R execute();
}

jerqi · 2024-09-25T04:20:34Z

@xunliu @jerqi List role operation only supports to list role names. Otherwise, it will have poor performance if we fetch objects, or it will be inconsistent with other interfaces if we don't fetch objects.

core/src/main/java/org/apache/gravitino/EntityStore.java

core/src/main/java/org/apache/gravitino/authorization/UserGroupManager.java

core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java

jerryshao · 2024-09-25T13:06:15Z

common/src/main/java/org/apache/gravitino/dto/authorization/RoleDTO.java

@@ -93,6 +95,10 @@ public Map<String, String> properties() {
   */
  @Override
  public List<SecurableObject> securableObjects() {
+    if (securableObjects == null) {


In which case will it return null here?

This won't be null. I modified.

jerryshao · 2024-09-25T13:07:18Z

common/src/main/java/org/apache/gravitino/dto/authorization/RoleDTO.java

@@ -186,9 +192,6 @@ public S withAudit(AuditDTO audit) {
    public RoleDTO build() {
      Preconditions.checkArgument(StringUtils.isNotBlank(name), "name cannot be null or empty");
      Preconditions.checkArgument(audit != null, "audit cannot be null");
-      Preconditions.checkArgument(
-          securableObjects != null && securableObjects.length != 0,
-          "securable objects can't null or empty");


Is it still removable, or should be checked?

Add check this field isn't null. If the securable objects are deleted, the collection may be empty. So I remove the check whether it is empty.

jerryshao · 2024-09-25T13:07:27Z

common/src/main/java/org/apache/gravitino/dto/responses/RoleResponse.java

@@ -64,8 +64,5 @@ public void validate() throws IllegalArgumentException {
    Preconditions.checkArgument(
        StringUtils.isNotBlank(role.name()), "role 'name' must not be null and empty");
    Preconditions.checkArgument(role.auditInfo() != null, "role 'auditInfo' must not be null");
-    Preconditions.checkArgument(
-        role.securableObjects() != null && !role.securableObjects().isEmpty(),
-        "role 'securableObjects' can't null or empty");


Is it still removable, or should be checked?

Ditto. Modified, too.

jerryshao · 2024-09-25T13:12:26Z

core/src/main/java/org/apache/gravitino/authorization/RoleManager.java

+      AuthorizationUtils.checkMetalakeExists(metalake);
+      Namespace namespace = AuthorizationUtils.ofRoleNamespace(metalake);
+      return store.list(namespace, RoleEntity.class, Entity.EntityType.ROLE, false).stream()
+          .map(entity -> (Role) entity)


I don't think we need this line.

jerryshao · 2024-09-25T13:13:28Z

core/src/main/java/org/apache/gravitino/authorization/RoleManager.java

+  String[] listRoleNames(String metalake) {
+    return Arrays.stream(listRolesInternal(metalake)).map(Role::name).toArray(String[]::new);
+  }
+
+  private Role[] listRolesInternal(String metalake) {


We can combine this two methods together, it is not necessary to separate for now.

jerryshao · 2024-09-25T13:15:37Z

core/src/main/java/org/apache/gravitino/authorization/RoleManager.java

+    try {
+      AuthorizationUtils.checkMetalakeExists(metalake);
+      Namespace namespace = AuthorizationUtils.ofRoleNamespace(metalake);
+      return store.list(namespace, RoleEntity.class, Entity.EntityType.ROLE, false).stream()


Seems like for role listing, we don't support allFields, only user interface supports this.

What I mean that for role, you don't even support this allFields field, so it is meaningless to set false here.

Removed this parameter.

### What changes were proposed in this pull request? Supports to list roles operations ### Why are the changes needed? Fix: #3346 ### Does this PR introduce _any_ user-facing change? Yes, will add the document later. ### How was this patch tested? Add UTs.

jerqi marked this pull request as draft September 9, 2024 09:28

jerqi force-pushed the ISSUE-3346 branch 3 times, most recently from 1981826 to 558ebba Compare September 9, 2024 11:15

jerqi marked this pull request as ready for review September 14, 2024 06:35

jerqi force-pushed the ISSUE-3346 branch 2 times, most recently from 944f676 to 3f2ee03 Compare September 19, 2024 10:05

jerqi closed this Sep 19, 2024

jerqi reopened this Sep 19, 2024

jerqi force-pushed the ISSUE-3346 branch from 3f2ee03 to 2668ee5 Compare September 19, 2024 13:45

jerqi changed the title ~~[#2246] feat(core,server): Supports to list roles operations~~ [#3346] feat(core,server): Supports to list roles operations Sep 23, 2024

jerqi force-pushed the ISSUE-3346 branch from 2668ee5 to 153f251 Compare September 23, 2024 15:15

jerqi added the branch-0.6 label Sep 24, 2024

jerqi force-pushed the ISSUE-3346 branch 2 times, most recently from a6f60dd to 8fa67f4 Compare September 24, 2024 03:13

jerqi requested review from yuqi1129, jerryshao and xunliu September 24, 2024 03:16

jerqi force-pushed the ISSUE-3346 branch 3 times, most recently from 792e078 to d1fcbb5 Compare September 24, 2024 09:05

jerqi self-assigned this Sep 24, 2024

jerryshao reviewed Sep 24, 2024

View reviewed changes

xunliu reviewed Sep 24, 2024

View reviewed changes

jerqi added 4 commits September 25, 2024 12:12

Supports to list roles

b66c5f1

rename and add lock

9a3e085

remove details option

3e1bbee

fix build

4889d62

jerqi force-pushed the ISSUE-3346 branch from 542174c to 4889d62 Compare September 25, 2024 04:12

Remove generic role

3646551

jerqi added 11 commits September 25, 2024 12:24

fix build

66e35ac

fix build

bb1a98a

fix ci

2b4d531

remove param

c80e34c

remove fieldset

f3d1bfc

remove unused method

6bfdf32

remove unused role pos

7e023cc

remove unused class

e71e38b

Modify modifier

0a431c8

address comment

66f0403

address comment

17d7369

jerqi requested review from xunliu and jerryshao September 25, 2024 10:57

xunliu reviewed Sep 25, 2024

View reviewed changes

jerqi added 2 commits September 25, 2024 20:08

address comments

0c8563c

fix style

ebf99ca

jerryshao reviewed Sep 25, 2024

View reviewed changes

jerqi added 3 commits September 25, 2024 21:22

address comments

7939c8c

address comment

719d7ae

address comment

889f20c

jerryshao approved these changes Sep 25, 2024

View reviewed changes

jerryshao merged commit 912a424 into apache:main Sep 26, 2024
26 checks passed

jerqi mentioned this pull request Sep 26, 2024

[#4873] feat(core): support list group #4879

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#3346] feat(core,server): Supports to list roles operations #4894

[#3346] feat(core,server): Supports to list roles operations #4894

jerqi commented Sep 9, 2024 •

edited

Loading

jerryshao Sep 24, 2024

jerqi Sep 24, 2024

jerryshao Sep 24, 2024

jerqi Sep 24, 2024

jerryshao Sep 24, 2024

jerqi Sep 24, 2024

jerryshao Sep 24, 2024

jerqi Sep 24, 2024

jerryshao Sep 24, 2024

jerqi Sep 24, 2024 •

edited

Loading

jerryshao Sep 24, 2024

xunliu left a comment •

edited

Loading

jerqi commented Sep 25, 2024

jerryshao Sep 25, 2024

jerqi Sep 25, 2024

jerryshao Sep 25, 2024

jerqi Sep 25, 2024 •

edited

Loading

jerryshao Sep 25, 2024

jerqi Sep 25, 2024

jerryshao Sep 25, 2024

jerqi Sep 25, 2024

jerryshao Sep 25, 2024

jerqi Sep 25, 2024

jerryshao Sep 25, 2024

jerqi Sep 25, 2024

jerryshao Sep 25, 2024

jerqi Sep 25, 2024

[#3346] feat(core,server): Supports to list roles operations #4894

[#3346] feat(core,server): Supports to list roles operations #4894

Conversation

jerqi commented Sep 9, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu left a comment • edited Loading

Choose a reason for hiding this comment

jerqi commented Sep 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerqi commented Sep 9, 2024 •

edited

Loading

jerqi Sep 24, 2024 •

edited

Loading

xunliu left a comment •

edited

Loading

jerqi Sep 25, 2024 •

edited

Loading