Possible error in the equals method for collection #6641

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.HashMap;
 import java.util.HashSet;
 import java.util.List;
@@ -412,12 +411,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;
   }