ReadOnlyStringMap: Generalize equals/hashCode across implementations

rschmitt · rschmitt · commit ac43de4bbe0a · 2025-05-27T16:32:25.000-07:00
This change rewrites the `equals` and `hashCode` implementations throughout the `ReadOnlyStringMap` hierarchy to use a common implementation that supports value-based equality between implementations. Fixes #3669.
diff --git a/log4j-1.2-api/src/main/java/org/apache/log4j/bridge/LogEventWrapper.java b/log4j-1.2-api/src/main/java/org/apache/log4j/bridge/LogEventWrapper.java
@@ -213,5 +213,24 @@ public <V, S> void forEach(final TriConsumer<String, ? super V, S> action, final
         public <V> V getValue(final String key) {
             return (V) super.get(key);
         }
+
+        @Override
+        public boolean equals(final Object obj) {
+            if (obj == this) {
+                return true;
+            }
+            if (obj instanceof ReadOnlyStringMap) {
+                // Convert to maps and compare
+                final Map<String, String> thisMap = toMap();
+                final Map<String, String> otherMap = ((ReadOnlyStringMap) obj).toMap();
+                return thisMap.equals(otherMap);
+            }
+            return super.equals(obj);
+        }
+
+        @Override
+        public int hashCode() {
+            return toMap().hashCode();
+        }
     }
 }
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/DefaultThreadContextMap.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/DefaultThreadContextMap.java
@@ -181,21 +181,25 @@ public String toString() {
 
     @Override
     public int hashCode() {
-        final int prime = 31;
-        int result = 1;
-        final Object[] state = localState.get();
-        result = prime * result + ((state == null) ? 0 : getMap(state).hashCode());
-        return result;
+        return toMap().hashCode();
     }
 
     @Override
-    public boolean equals(final Object obj) {
+    public boolean equals(Object obj) {
         if (this == obj) {
             return true;
         }
         if (obj == null) {
             return false;
         }
+        if (obj instanceof ReadOnlyStringMap) {
+            if (size() != ((ReadOnlyStringMap) obj).size()) {
+                return false;
+            }
+
+            // Convert to maps and compare
+            obj = ((ReadOnlyStringMap) obj).toMap();
+        }
         if (!(obj instanceof ThreadContextMap)) {
             return false;
         }
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/SortedArrayStringMap.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/SortedArrayStringMap.java
@@ -408,39 +408,22 @@ public boolean equals(final Object obj) {
         if (obj == this) {
             return true;
         }
-        if (!(obj instanceof SortedArrayStringMap)) {
+        if (!(obj instanceof ReadOnlyStringMap)) {
             return false;
         }
-        final SortedArrayStringMap other = (SortedArrayStringMap) obj;
-        if (this.size() != other.size()) {
+        if (size() != ((ReadOnlyStringMap) obj).size()) {
             return false;
         }
-        for (int i = 0; i < size(); i++) {
-            if (!Objects.equals(keys[i], other.keys[i])) {
-                return false;
-            }
-            if (!Objects.equals(values[i], other.values[i])) {
-                return false;
-            }
-        }
-        return true;
+
+        // Convert to maps and compare
+        final Map<String, String> thisMap = toMap();
+        final Map<String, String> otherMap = ((ReadOnlyStringMap) obj).toMap();
+        return thisMap.equals(otherMap);
     }
 
     @Override
     public int hashCode() {
-        int result = 37;
-        result = HASHVAL * result + size;
-        result = HASHVAL * result + hashCode(keys, size);
-        result = HASHVAL * result + hashCode(values, size);
-        return result;
-    }
-
-    private static int hashCode(final Object[] values, final int length) {
-        int result = 1;
-        for (int i = 0; i < length; i++) {
-            result = HASHVAL * result + (values[i] == null ? 0 : values[i].hashCode());
-        }
-        return result;
+        return toMap().hashCode();
     }
 
     @Override
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/package-info.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/package-info.java
@@ -20,7 +20,7 @@
  * There are no guarantees for binary or logical compatibility in this package.
  */
 @Export
-@Version("2.24.1")
+@Version("2.25.0")
 package org.apache.logging.log4j.util;
 
 import org.osgi.annotation.bundle.Export;
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/FactoryTestStringMap.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/FactoryTestStringMap.java
@@ -114,4 +114,27 @@ public int size() {
     public Map<String, String> toMap() {
         return null;
     }
+
+    @Override
+    public boolean equals(final Object obj) {
+        if (obj == this) {
+            return true;
+        }
+        if (!(obj instanceof ReadOnlyStringMap)) {
+            return false;
+        }
+        if (size() != ((ReadOnlyStringMap) obj).size()) {
+            return false;
+        }
+
+        // Convert to maps and compare
+        final Map<String, String> thisMap = toMap();
+        final Map<String, String> otherMap = ((ReadOnlyStringMap) obj).toMap();
+        return thisMap.equals(otherMap);
+    }
+
+    @Override
+    public int hashCode() {
+        return toMap().hashCode();
+    }
 }
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/FactoryTestStringMapWithoutIntConstructor.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/FactoryTestStringMapWithoutIntConstructor.java
@@ -81,4 +81,27 @@ public void putValue(final String key, final Object value) {}
 
     @Override
     public void remove(final String key) {}
+
+    @Override
+    public boolean equals(final Object obj) {
+        if (obj == this) {
+            return true;
+        }
+        if (!(obj instanceof ReadOnlyStringMap)) {
+            return false;
+        }
+        if (size() != ((ReadOnlyStringMap) obj).size()) {
+            return false;
+        }
+
+        // Convert to maps and compare
+        final Map<String, String> thisMap = toMap();
+        final Map<String, String> otherMap = ((ReadOnlyStringMap) obj).toMap();
+        return thisMap.equals(otherMap);
+    }
+
+    @Override
+    public int hashCode() {
+        return toMap().hashCode();
+    }
 }
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMapTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMapTest.java
@@ -35,6 +35,7 @@
 import java.util.Map;
 import java.util.stream.Stream;
 import org.apache.logging.log4j.util.BiConsumer;
+import org.apache.logging.log4j.util.SortedArrayStringMap;
 import org.apache.logging.log4j.util.TriConsumer;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
@@ -850,6 +851,26 @@ void testForEachTriConsumer() {
         assertEquals(state.count, original.size());
     }
 
+    @Test
+    void testEqualityWithOtherImplementations() {
+        final JdkMapAdapterStringMap left = new JdkMapAdapterStringMap();
+        final SortedArrayStringMap right = new SortedArrayStringMap();
+        assertEquals(left, right);
+        assertEquals(left.hashCode(), right.hashCode());
+
+        left.putValue("a", "avalue");
+        left.putValue("B", "Bvalue");
+        right.putValue("B", "Bvalue");
+        right.putValue("a", "avalue");
+        assertEquals(left, right);
+        assertEquals(left.hashCode(), right.hashCode());
+
+        left.remove("a");
+        right.remove("a");
+        assertEquals(left, right);
+        assertEquals(left.hashCode(), right.hashCode());
+    }
+
     static Stream<Arguments> testImmutability() {
         return Stream.of(
                 Arguments.of(new HashMap<>(), false),
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMap.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/JdkMapAdapterStringMap.java
@@ -219,15 +219,21 @@ public boolean equals(final Object object) {
         if (object == this) {
             return true;
         }
-        if (!(object instanceof JdkMapAdapterStringMap)) {
+        if (!(object instanceof ReadOnlyStringMap)) {
             return false;
         }
-        final JdkMapAdapterStringMap other = (JdkMapAdapterStringMap) object;
-        return map.equals(other.map) && immutable == other.immutable;
+        if (size() != ((ReadOnlyStringMap) object).size()) {
+            return false;
+        }
+
+        // Convert to maps and compare
+        final Map<String, String> thisMap = toMap();
+        final Map<String, String> otherMap = ((ReadOnlyStringMap) object).toMap();
+        return thisMap.equals(otherMap);
     }
 
     @Override
     public int hashCode() {
-        return map.hashCode() + (immutable ? 31 : 0);
+        return toMap().hashCode();
     }
 }
diff --git a/log4j-perf-test/src/main/java/org/apache/logging/log4j/perf/nogc/OpenHashStringMap.java b/log4j-perf-test/src/main/java/org/apache/logging/log4j/perf/nogc/OpenHashStringMap.java
@@ -24,7 +24,6 @@
 import java.util.ConcurrentModificationException;
 import java.util.HashMap;
 import java.util.Map;
-import java.util.Objects;
 import org.apache.logging.log4j.spi.ThreadContextMap;
 import org.apache.logging.log4j.util.BiConsumer;
 import org.apache.logging.log4j.util.ReadOnlyStringMap;
@@ -301,27 +300,14 @@ public boolean equals(final Object obj) {
         if (!(obj instanceof ReadOnlyStringMap)) {
             return false;
         }
-        final ReadOnlyStringMap other = (ReadOnlyStringMap) obj;
-        if (other.size() != size()) {
+        if (size() != ((ReadOnlyStringMap) obj).size()) {
             return false;
         }
-        int pos = arraySize;
-        if (containsNullKey) {
-            if (!Objects.equals(getObjectValue(null), other.getValue(null))) {
-                return false;
-            }
-        }
-        --pos;
-        final K myKeys[] = this.keys;
-        for (; pos >= 0; pos--) {
-            K k;
-            if ((k = myKeys[pos]) != null) {
-                if (!Objects.equals(values[pos], other.getValue((String) k))) {
-                    return false;
-                }
-            }
-        }
-        return true;
+
+        // Convert to maps and compare
+        final Map<String, String> thisMap = toMap();
+        final Map<String, String> otherMap = ((ReadOnlyStringMap) obj).toMap();
+        return thisMap.equals(otherMap);
     }
 
     @Override
@@ -699,25 +685,7 @@ protected void rehash(final int newN) {
      */
     @Override
     public int hashCode() {
-        int result = 0;
-        for (int j = realSize(), i = 0, t = 0; j-- != 0; ) {
-            while (keys[i] == null) {
-                i++;
-            }
-            if (this != keys[i]) {
-                t = keys[i].hashCode();
-            }
-            if (this != values[i]) {
-                t ^= (values[i] == null ? 0 : values[i].hashCode());
-            }
-            result += t;
-            i++;
-        }
-        // Zero / null keys have hash zero.
-        if (containsNullKey) {
-            result += (values[arraySize] == null ? 0 : values[arraySize].hashCode());
-        }
-        return result;
+        return toMap().hashCode();
     }
 
     @SuppressWarnings("unchecked")
diff --git a/src/changelog/.2.x.x/3669_generalize_ReadOnlyStringMap_equality.xml b/src/changelog/.2.x.x/3669_generalize_ReadOnlyStringMap_equality.xml
@@ -0,0 +1,10 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="3669" link="https://github.com/apache/logging-log4j2/issues/3669"/>
+  <description format="asciidoc">
+    The ReadOnlyStringMap implementations now support equality comparisons against each other.
+  </description>
+</entry>

Original file line number	Diff line number	Diff line change
`@@ -219,15 +219,21 @@ public boolean equals(final Object object) {`
`219`	`219`	`if (object == this) {`
`220`	`220`	`return true;`
`221`	`221`	`}`
`222`		`- if (!(object instanceof JdkMapAdapterStringMap)) {`
	`222`	`+ if (!(object instanceof ReadOnlyStringMap)) {`
`223`	`223`	`return false;`
`224`	`224`	`}`
`225`		`- final JdkMapAdapterStringMap other = (JdkMapAdapterStringMap) object;`
`226`		`- return map.equals(other.map) && immutable == other.immutable;`
	`225`	`+ if (size() != ((ReadOnlyStringMap) object).size()) {`
	`226`	`+ return false;`
	`227`	`+ }`
	`228`	`+`
	`229`	`+ // Convert to maps and compare`
	`230`	`+ final Map<String, String> thisMap = toMap();`
	`231`	`+ final Map<String, String> otherMap = ((ReadOnlyStringMap) object).toMap();`
	`232`	`+ return thisMap.equals(otherMap);`
`227`	`233`	`}`
`228`	`234`
`229`	`235`	`@Override`
`230`	`236`	`public int hashCode() {`
`231`		`- return map.hashCode() + (immutable ? 31 : 0);`
	`237`	`+ return toMap().hashCode();`
`232`	`238`	`}`
`233`	`239`	`}`