[#6641] fix(core): Possible error in the equals method for collection (#6644)

<!--
1. Title: [#<issue>] <type>(<scope>): <subject>
   Examples:
     - "[#123] feat(operator): support xxx"
     - "[#233] fix: check null before access result in xxx"
     - "[MINOR] refactor: fix typo in variable name"
     - "[MINOR] docs: fix typo in README"
     - "[#255] test: fix flaky test NameOfTheTest"
   Reference: https://www.conventionalcommits.org/en/v1.0.0/
2. If the PR is unfinished, please mark this PR as draft.
-->

### What changes were proposed in this pull request?

Possible error in the equals method for collection

### Why are the changes needed?

Fix: #6641

### Does this PR introduce _any_ user-facing change?

N/A

### How was this patch tested?

N/A

Author: sunxiaojian

api/build.gradle.kts

@@ -24,6 +24,7 @@ plugins {
 
 dependencies {
   implementation(libs.commons.lang3)
+  implementation(libs.commons.collections4)
   implementation(libs.guava)
   implementation(libs.slf4j.api)
 

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

@@ -22,9 +22,11 @@
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Sets;
+import java.util.Collection;
 import java.util.List;
 import java.util.Objects;
 import java.util.stream.Collectors;
+import org.apache.commons.collections4.CollectionUtils;
 import org.apache.gravitino.MetadataObject;
 import org.apache.gravitino.MetadataObjects;
 import org.apache.gravitino.MetadataObjects.MetadataObjectImpl;
@@ -168,7 +170,18 @@ public boolean equals(Object other) {
       }
 
       SecurableObject otherSecurableObject = (SecurableObject) other;
-      return super.equals(other) && Objects.equals(privileges, otherSecurableObject.privileges());
+      return super.equals(other)
+          && isEqualCollection(privileges, otherSecurableObject.privileges());
+    }
+
+    private boolean isEqualCollection(Collection<?> c1, Collection<?> c2) {
+      if (c1 == c2) {
+        return true;
+      }
+      if (c1 == null || c2 == null) {
+        return false;
+      }
+      return CollectionUtils.isEqualCollection(c1, c2);
     }
   }
 

common/src/main/java/org/apache/gravitino/utils/CollectionUtils.java

@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.gravitino.utils;
+
+import java.util.Collection;
+
+/** Utility class for working with collection. */
+public class CollectionUtils {
+  private CollectionUtils() {}
+
+  /**
+   * Returns true if the two collections are equal.
+   *
+   * @param c1 the first collection, may be null
+   * @param c2 the second collection, may be null
+   * @return true if the two collections are equal
+   */
+  public static boolean isEqualCollection(Collection<?> c1, Collection<?> c2) {
+    if (c1 == c2) {
+      return true;
+    }
+    if (c1 == null || c2 == null) {
+      return false;
+    }
+    return org.apache.commons.collections4.CollectionUtils.isEqualCollection(c1, c2);
+  }
+}

core/build.gradle.kts

@@ -33,6 +33,7 @@ dependencies {
   implementation(libs.commons.dbcp2)
   implementation(libs.commons.io)
   implementation(libs.commons.lang3)
+  implementation(libs.commons.collections4)
   implementation(libs.guava)
   implementation(libs.h2db)
   implementation(libs.mybatis)

core/src/main/java/org/apache/gravitino/meta/GroupEntity.java

@@ -29,6 +29,7 @@
 import org.apache.gravitino.HasIdentifier;
 import org.apache.gravitino.Namespace;
 import org.apache.gravitino.authorization.Group;
+import org.apache.gravitino.utils.CollectionUtils;
 
 public class GroupEntity implements Group, Entity, Auditable, HasIdentifier {
 
@@ -161,8 +162,8 @@ public boolean equals(Object o) {
         && Objects.equals(name, that.name)
         && Objects.equals(namespace, that.namespace)
         && Objects.equals(auditInfo, that.auditInfo)
-        && Objects.equals(roleNames, that.roleNames)
-        && Objects.equals(roleIds, that.roleIds);
+        && CollectionUtils.isEqualCollection(roleNames, that.roleNames)
+        && CollectionUtils.isEqualCollection(roleIds, that.roleIds);
   }
 
   @Override

core/src/main/java/org/apache/gravitino/meta/ModelVersionEntity.java

@@ -31,6 +31,7 @@
 import org.apache.gravitino.HasIdentifier;
 import org.apache.gravitino.NameIdentifier;
 import org.apache.gravitino.Namespace;
+import org.apache.gravitino.utils.CollectionUtils;
 
 @ToString
 public class ModelVersionEntity implements Entity, Auditable, HasIdentifier {
@@ -146,7 +147,7 @@ public boolean equals(Object o) {
     return Objects.equals(version, that.version)
         && Objects.equals(modelIdent, that.modelIdent)
         && Objects.equals(comment, that.comment)
-        && Objects.equals(aliases, that.aliases)
+        && CollectionUtils.isEqualCollection(aliases, that.aliases)
         && Objects.equals(uri, that.uri)
         && Objects.equals(properties, that.properties)
         && Objects.equals(auditInfo, that.auditInfo);

core/src/main/java/org/apache/gravitino/meta/RoleEntity.java

@@ -31,6 +31,7 @@
 import org.apache.gravitino.Namespace;
 import org.apache.gravitino.authorization.Role;
 import org.apache.gravitino.authorization.SecurableObject;
+import org.apache.gravitino.utils.CollectionUtils;
 
 public class RoleEntity implements Role, Entity, Auditable, HasIdentifier {
 
@@ -142,7 +143,7 @@ public boolean equals(Object o) {
         && Objects.equals(namespace, that.namespace)
         && Objects.equals(auditInfo, that.auditInfo)
         && Objects.equals(properties, that.properties)
-        && Objects.equals(securableObjects, that.securableObjects);
+        && CollectionUtils.isEqualCollection(securableObjects, that.securableObjects);
   }
 
   @Override

core/src/main/java/org/apache/gravitino/meta/TableEntity.java

@@ -29,6 +29,7 @@
 import org.apache.gravitino.Field;
 import org.apache.gravitino.HasIdentifier;
 import org.apache.gravitino.Namespace;
+import org.apache.gravitino.utils.CollectionUtils;
 
 /** A class representing a table entity in Apache Gravitino. */
 @ToString
@@ -135,7 +136,7 @@ public boolean equals(Object o) {
         && Objects.equal(name, baseTable.name)
         && Objects.equal(namespace, baseTable.namespace)
         && Objects.equal(auditInfo, baseTable.auditInfo)
-        && Objects.equal(columns, baseTable.columns);
+        && CollectionUtils.isEqualCollection(columns, baseTable.columns);
   }
 
   @Override

core/src/main/java/org/apache/gravitino/meta/UserEntity.java

@@ -30,6 +30,7 @@
 import org.apache.gravitino.HasIdentifier;
 import org.apache.gravitino.Namespace;
 import org.apache.gravitino.authorization.User;
+import org.apache.gravitino.utils.CollectionUtils;
 
 /** A class representing a user metadata entity in Apache Gravitino. */
 @ToString
@@ -164,8 +165,8 @@ public boolean equals(Object o) {
         && Objects.equals(name, that.name)
         && Objects.equals(namespace, that.namespace)
         && Objects.equals(auditInfo, that.auditInfo)
-        && Objects.equals(roleNames, that.roleNames)
-        && Objects.equals(roleIds, that.roleIds);
+        && CollectionUtils.isEqualCollection(roleNames, that.roleNames)
+        && CollectionUtils.isEqualCollection(roleIds, that.roleIds);
   }
 
   @Override

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

@@ -24,7 +24,6 @@
 import com.google.common.collect.Sets;
 import java.io.IOException;
 import java.util.Collections;
-import java.util.Comparator;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -408,12 +407,6 @@ private static List<SecurableObject> listSecurableObjects(RolePO po) {
                 }
               }
             });
-
-    // To ensure that the order of the returned securable objects remains consistent,
-    // the securable objects are sorted by fullName here,
-    // since the order of securable objects after grouping by is different each time.
-    securableObjects.sort(Comparator.comparing(MetadataObject::fullName));
-
     return securableObjects;
   }
 

core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java

@@ -57,6 +57,7 @@
 import org.apache.gravitino.storage.relational.po.UserPO;
 import org.apache.gravitino.storage.relational.session.SqlSessionFactoryHelper;
 import org.apache.gravitino.storage.relational.utils.SessionUtils;
+import org.apache.gravitino.utils.CollectionUtils;
 import org.apache.ibatis.session.SqlSession;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.Test;
@@ -623,13 +624,14 @@ void testUpdateRole() throws IOException {
     Assertions.assertEquals(grantRole.name(), roleEntity.name());
     Assertions.assertEquals("creator", grantRole.auditInfo().creator());
     Assertions.assertEquals("grantRole", grantRole.auditInfo().lastModifier());
-    Assertions.assertEquals(
-        Lists.newArrayList(
-            SecurableObjects.ofCatalog(
-                "catalog", Lists.newArrayList(Privileges.UseCatalog.allow())),
-            SecurableObjects.ofMetalake(
-                metalakeName, Lists.newArrayList(Privileges.CreateTable.allow()))),
-        grantRole.securableObjects());
+    Assertions.assertTrue(
+        CollectionUtils.isEqualCollection(
+            Lists.newArrayList(
+                SecurableObjects.ofCatalog(
+                    "catalog", Lists.newArrayList(Privileges.UseCatalog.allow())),
+                SecurableObjects.ofMetalake(
+                    metalakeName, Lists.newArrayList(Privileges.CreateTable.allow()))),
+            grantRole.securableObjects()));
 
     // revoke privileges from the role
     Function<RoleEntity, RoleEntity> revokeUpdater =
@@ -643,7 +645,10 @@ void testUpdateRole() throws IOException {
                   .build();
 
           List<SecurableObject> securableObjects = Lists.newArrayList(role.securableObjects());
-          securableObjects.remove(0);
+          securableObjects.removeIf(
+              securableObject ->
+                  securableObject.type() == SecurableObject.Type.CATALOG
+                      && securableObject.privileges().contains(Privileges.UseCatalog.allow()));
 
           return RoleEntity.builder()
               .withId(role.id())