This is an automated email from the ASF dual-hosted git repository.
jerryshao 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 9f18a9ef7f [#13216] fix(api): fix RemoveMetadataObject.equals
ClassCastException and compare locations (#13219)
9f18a9ef7f is described below
commit 9f18a9ef7f19515e667f04c9dea7ca2c08f2b69c
Author: YangJie <[email protected]>
AuthorDate: Sun Sep 20 05:35:17 2026 -0400
[#13216] fix(api): fix RemoveMetadataObject.equals ClassCastException and
compare locations (#13219)
### What changes were proposed in this pull request?
`RemoveMetadataObject.equals` now casts to `RemoveMetadataObject` and
compares `metadataObject` and `locations` with `Objects.equals`, and
`hashCode` includes `locations`. The constructor stores an
`ImmutableList` copy of `locations`, so the value object is immutable
and its hash key stays stable even if the caller later mutates the list
it passed in.
### Why are the changes needed?
After the `getClass()` guard the argument was cast to the sibling
`RenameMetadataObject`, so comparing two `RemoveMetadataObject`
instances (including `HashSet`/`contains`) always threw
`ClassCastException`, and `locations` was ignored. Comparing with
`Objects.equals` also avoids a `NullPointerException` when `locations`
is null (constructible via `remove(obj, null)`), and the defensive copy
keeps `locations` a stable hash key now that it participates in
`hashCode`.
Fix: #13216
### Does this PR introduce _any_ user-facing change?
No API change. Comparing two `RemoveMetadataObject` instances no longer
throws `ClassCastException`, `equals`/`hashCode` now take `locations`
into account, and a built `RemoveMetadataObject` is immutable.
### How was this patch tested?
Added `TestMetadataObjectChange`, which pins `equals`/`hashCode` for
`RemoveMetadataObject`: comparing two instances and `HashSet` membership
(fails on the pre-fix tree with `ClassCastException`), equality when
`locations` is null (fails pre-fix with `NullPointerException`), and
that mutating the caller's list after construction does not change the
object or its `HashSet` membership.
---
.../authorization/MetadataObjectChange.java | 18 ++--
.../authorization/TestMetadataObjectChange.java | 110 +++++++++++++++++++++
2 files changed, 120 insertions(+), 8 deletions(-)
diff --git
a/api/src/main/java/org/apache/gravitino/authorization/MetadataObjectChange.java
b/api/src/main/java/org/apache/gravitino/authorization/MetadataObjectChange.java
index f63daff764..dd49c7f7b4 100644
---
a/api/src/main/java/org/apache/gravitino/authorization/MetadataObjectChange.java
+++
b/api/src/main/java/org/apache/gravitino/authorization/MetadataObjectChange.java
@@ -19,6 +19,7 @@
package org.apache.gravitino.authorization;
import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import java.util.List;
import java.util.Objects;
@@ -157,7 +158,7 @@ public interface MetadataObjectChange {
private RemoveMetadataObject(MetadataObject metadataObject, List<String>
locations) {
this.metadataObject = metadataObject;
- this.locations = locations;
+ this.locations = locations == null ? null :
ImmutableList.copyOf(locations);
}
/**
@@ -180,28 +181,29 @@ public interface MetadataObjectChange {
/**
* Compares this RemoveMetadataObject instance with another object for
equality. The comparison
- * is based on the old metadata entity.
+ * is based on the metadata entity and the locations.
*
* @param o The object to compare with this instance.
- * @return true if the given object represents the same rename metadata
entity; false otherwise.
+ * @return true if the given object represents the same remove metadata
entity; false otherwise.
*/
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
- RenameMetadataObject that = (RenameMetadataObject) o;
- return metadataObject.equals(that.metadataObject);
+ RemoveMetadataObject that = (RemoveMetadataObject) o;
+ return Objects.equals(metadataObject, that.metadataObject)
+ && Objects.equals(locations, that.locations);
}
/**
* Generates a hash code for this RemoveMetadataObject instance. The hash
code is based on the
- * old metadata entity.
+ * metadata entity and the locations.
*
- * @return A hash code value for this update metadata entity operation.
+ * @return A hash code value for this remove metadata entity operation.
*/
@Override
public int hashCode() {
- return Objects.hash(metadataObject);
+ return Objects.hash(metadataObject, locations);
}
/**
diff --git
a/api/src/test/java/org/apache/gravitino/authorization/TestMetadataObjectChange.java
b/api/src/test/java/org/apache/gravitino/authorization/TestMetadataObjectChange.java
new file mode 100644
index 0000000000..5f87be4f16
--- /dev/null
+++
b/api/src/test/java/org/apache/gravitino/authorization/TestMetadataObjectChange.java
@@ -0,0 +1,110 @@
+/*
+ * 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.authorization;
+
+import com.google.common.collect.Lists;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.gravitino.MetadataObject;
+import org.apache.gravitino.MetadataObjects;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+public class TestMetadataObjectChange {
+
+ private MetadataObject tableObject() {
+ return MetadataObjects.of(
+ Lists.newArrayList("catalog", "schema", "table"),
MetadataObject.Type.TABLE);
+ }
+
+ @Test
+ void testRemoveEqualsWithDifferentLocations() {
+ MetadataObjectChange remove1 =
+ MetadataObjectChange.remove(tableObject(), Lists.newArrayList("loc1"));
+ MetadataObjectChange remove2 =
+ MetadataObjectChange.remove(tableObject(), Lists.newArrayList("loc2"));
+
+ // Before the fix, RemoveMetadataObject.equals cast the argument to
RenameMetadataObject and
+ // threw ClassCastException here.
+ Assertions.assertNotEquals(remove1, remove2);
+ }
+
+ @Test
+ void testRemoveEqualsSameLocations() {
+ MetadataObjectChange remove1 =
+ MetadataObjectChange.remove(tableObject(), Lists.newArrayList("loc1"));
+ MetadataObjectChange remove2 =
+ MetadataObjectChange.remove(tableObject(), Lists.newArrayList("loc1"));
+
+ Assertions.assertEquals(remove1, remove2);
+ Assertions.assertEquals(remove1.hashCode(), remove2.hashCode());
+ }
+
+ @Test
+ void testRemoveInHashSet() {
+ Set<MetadataObjectChange> changes =
+ new HashSet<>(
+ Lists.newArrayList(
+ MetadataObjectChange.remove(tableObject(),
Lists.newArrayList("loc1"))));
+
+ // HashSet.contains delegates to equals; before the fix this threw
ClassCastException.
+ Assertions.assertDoesNotThrow(
+ () ->
+ changes.contains(
+ MetadataObjectChange.remove(tableObject(),
Lists.newArrayList("loc2"))));
+ Assertions.assertFalse(
+ changes.contains(MetadataObjectChange.remove(tableObject(),
Lists.newArrayList("loc2"))));
+ Assertions.assertTrue(
+ changes.contains(MetadataObjectChange.remove(tableObject(),
Lists.newArrayList("loc1"))));
+ }
+
+ @Test
+ void testRemoveEqualsWithNullLocations() {
+ // remove(mo, null) is constructible; comparing two such instances must
not throw NPE.
+ MetadataObjectChange remove1 = MetadataObjectChange.remove(tableObject(),
null);
+ MetadataObjectChange remove2 = MetadataObjectChange.remove(tableObject(),
null);
+
+ Assertions.assertEquals(remove1, remove2);
+ Assertions.assertEquals(remove1.hashCode(), remove2.hashCode());
+
+ MetadataObjectChange removeWithLocations =
+ MetadataObjectChange.remove(tableObject(), List.of("a"));
+ Assertions.assertNotEquals(remove1, removeWithLocations);
+ Assertions.assertNotEquals(removeWithLocations, remove1);
+ }
+
+ @Test
+ void testRemoveDefensiveCopyStableHashKey() {
+ List<String> mutableList = Lists.newArrayList("loc1");
+ MetadataObjectChange.RemoveMetadataObject remove =
+ (MetadataObjectChange.RemoveMetadataObject)
+ MetadataObjectChange.remove(tableObject(), mutableList);
+
+ Set<MetadataObjectChange> changes = new HashSet<>();
+ changes.add(remove);
+
+ // Mutating the caller's list after remove(...) must not change the
object's hash key nor be
+ // reflected in getLocations().
+ mutableList.add("x");
+
+ Assertions.assertTrue(changes.contains(remove));
+ Assertions.assertEquals(Lists.newArrayList("loc1"), remove.getLocations());
+ }
+}
