This is an automated email from the ASF dual-hosted git repository.
yuqi4733 pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/gravitino.git
The following commit(s) were added to refs/heads/main by this push:
new fdcb7ae187 [#6641] fix(core): Possible error in the equals method for
collection (#6644)
fdcb7ae187 is described below
commit fdcb7ae187d8f464fbc8dae1e07dc5ad385411d5
Author: Xiaojian Sun <[email protected]>
AuthorDate: Tue Mar 11 16:56:13 2025 +0800
[#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
---
api/build.gradle.kts | 1 +
.../gravitino/authorization/SecurableObjects.java | 15 ++++++++-
.../apache/gravitino/utils/CollectionUtils.java | 37 +++++++++++++---------
core/build.gradle.kts | 1 +
.../org/apache/gravitino/meta/GroupEntity.java | 5 +--
.../apache/gravitino/meta/ModelVersionEntity.java | 3 +-
.../java/org/apache/gravitino/meta/RoleEntity.java | 3 +-
.../org/apache/gravitino/meta/TableEntity.java | 3 +-
.../java/org/apache/gravitino/meta/UserEntity.java | 5 +--
.../relational/service/RoleMetaService.java | 7 ----
.../relational/service/TestRoleMetaService.java | 21 +++++++-----
11 files changed, 63 insertions(+), 38 deletions(-)
diff --git a/api/build.gradle.kts b/api/build.gradle.kts
index a9f2450863..b13745d9ff 100644
--- a/api/build.gradle.kts
+++ b/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)
diff --git
a/api/src/main/java/org/apache/gravitino/authorization/SecurableObjects.java
b/api/src/main/java/org/apache/gravitino/authorization/SecurableObjects.java
index b926964e15..5aa2fd8d2f 100644
--- a/api/src/main/java/org/apache/gravitino/authorization/SecurableObjects.java
+++ b/api/src/main/java/org/apache/gravitino/authorization/SecurableObjects.java
@@ -22,9 +22,11 @@ import com.google.common.base.Splitter;
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 class SecurableObjects {
}
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);
}
}
diff --git a/api/build.gradle.kts
b/common/src/main/java/org/apache/gravitino/utils/CollectionUtils.java
similarity index 54%
copy from api/build.gradle.kts
copy to common/src/main/java/org/apache/gravitino/utils/CollectionUtils.java
index a9f2450863..43ade6c119 100644
--- a/api/build.gradle.kts
+++ b/common/src/main/java/org/apache/gravitino/utils/CollectionUtils.java
@@ -16,22 +16,29 @@
* specific language governing permissions and limitations
* under the License.
*/
-plugins {
- `maven-publish`
- id("java")
- id("idea")
-}
-dependencies {
- implementation(libs.commons.lang3)
- implementation(libs.guava)
- implementation(libs.slf4j.api)
+package org.apache.gravitino.utils;
- testImplementation(libs.junit.jupiter.api)
- testImplementation(libs.junit.jupiter.params)
- testRuntimeOnly(libs.junit.jupiter.engine)
-}
+import java.util.Collection;
+
+/** Utility class for working with collection. */
+public class CollectionUtils {
+ private CollectionUtils() {}
-tasks.build {
- dependsOn("javadoc")
+ /**
+ * 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);
+ }
}
diff --git a/core/build.gradle.kts b/core/build.gradle.kts
index ef23950b07..c5f92a6ed7 100644
--- a/core/build.gradle.kts
+++ b/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)
diff --git a/core/src/main/java/org/apache/gravitino/meta/GroupEntity.java
b/core/src/main/java/org/apache/gravitino/meta/GroupEntity.java
index d9e4b633d5..b8cac5ba4c 100644
--- a/core/src/main/java/org/apache/gravitino/meta/GroupEntity.java
+++ b/core/src/main/java/org/apache/gravitino/meta/GroupEntity.java
@@ -29,6 +29,7 @@ import org.apache.gravitino.Field;
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 class GroupEntity implements Group, Entity,
Auditable, HasIdentifier {
&& 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
diff --git
a/core/src/main/java/org/apache/gravitino/meta/ModelVersionEntity.java
b/core/src/main/java/org/apache/gravitino/meta/ModelVersionEntity.java
index d6c4bfdf86..733c1315b5 100644
--- a/core/src/main/java/org/apache/gravitino/meta/ModelVersionEntity.java
+++ b/core/src/main/java/org/apache/gravitino/meta/ModelVersionEntity.java
@@ -31,6 +31,7 @@ import org.apache.gravitino.Field;
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 class ModelVersionEntity implements Entity,
Auditable, HasIdentifier {
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);
diff --git a/core/src/main/java/org/apache/gravitino/meta/RoleEntity.java
b/core/src/main/java/org/apache/gravitino/meta/RoleEntity.java
index a96f6e0f5a..c69df41eae 100644
--- a/core/src/main/java/org/apache/gravitino/meta/RoleEntity.java
+++ b/core/src/main/java/org/apache/gravitino/meta/RoleEntity.java
@@ -31,6 +31,7 @@ import org.apache.gravitino.HasIdentifier;
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 class RoleEntity implements Role, Entity, Auditable,
HasIdentifier {
&& 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
diff --git a/core/src/main/java/org/apache/gravitino/meta/TableEntity.java
b/core/src/main/java/org/apache/gravitino/meta/TableEntity.java
index 197eed6d33..f661cd049a 100644
--- a/core/src/main/java/org/apache/gravitino/meta/TableEntity.java
+++ b/core/src/main/java/org/apache/gravitino/meta/TableEntity.java
@@ -29,6 +29,7 @@ import org.apache.gravitino.Entity;
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 class TableEntity implements Entity, Auditable,
HasIdentifier {
&& 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
diff --git a/core/src/main/java/org/apache/gravitino/meta/UserEntity.java
b/core/src/main/java/org/apache/gravitino/meta/UserEntity.java
index c71d731a99..c9d7089f85 100644
--- a/core/src/main/java/org/apache/gravitino/meta/UserEntity.java
+++ b/core/src/main/java/org/apache/gravitino/meta/UserEntity.java
@@ -30,6 +30,7 @@ import org.apache.gravitino.Field;
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 class UserEntity implements User, Entity, Auditable,
HasIdentifier {
&& 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
diff --git
a/core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java
b/core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java
index 3f27902d0c..59ec4dea8c 100644
---
a/core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java
+++
b/core/src/main/java/org/apache/gravitino/storage/relational/service/RoleMetaService.java
@@ -24,7 +24,6 @@ import com.google.common.collect.Lists;
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 @@ public class RoleMetaService {
}
}
});
-
- // 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;
}
diff --git
a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java
b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java
index 4a5d586465..ea6b31e1df 100644
---
a/core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java
+++
b/core/src/test/java/org/apache/gravitino/storage/relational/service/TestRoleMetaService.java
@@ -57,6 +57,7 @@ import org.apache.gravitino.storage.relational.po.GroupPO;
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 @@ class TestRoleMetaService extends TestJDBCBackend {
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 @@ class TestRoleMetaService extends TestJDBCBackend {
.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())
