ReadOnlyStringMap: Generalize equals/hashCode across implementations

rschmitt · rschmitt · commit 924ec08dba3f · 2025-05-21T17:24:06.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
@@ -36,6 +36,7 @@
 import org.apache.logging.log4j.spi.MutableThreadContextStack;
 import org.apache.logging.log4j.util.BiConsumer;
 import org.apache.logging.log4j.util.ReadOnlyStringMap;
+import org.apache.logging.log4j.util.ReadOnlyStringMapUtil;
 import org.apache.logging.log4j.util.TriConsumer;
 
 /**
@@ -213,5 +214,21 @@ 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) {
+                return ReadOnlyStringMapUtil.equals(this, (ReadOnlyStringMap) obj);
+            }
+            return super.equals(obj);
+        }
+
+        @Override
+        public int hashCode() {
+            return ReadOnlyStringMapUtil.hashCode(this);
+        }
     }
 }
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
@@ -24,6 +24,7 @@
 import org.apache.logging.log4j.util.BiConsumer;
 import org.apache.logging.log4j.util.PropertiesUtil;
 import org.apache.logging.log4j.util.ReadOnlyStringMap;
+import org.apache.logging.log4j.util.ReadOnlyStringMapUtil;
 import org.apache.logging.log4j.util.TriConsumer;
 
 /**
@@ -181,11 +182,7 @@ 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 ReadOnlyStringMapUtil.hashCode(this);
     }
 
     @Override
@@ -196,6 +193,9 @@ public boolean equals(final Object obj) {
         if (obj == null) {
             return false;
         }
+        if (obj instanceof ReadOnlyStringMap) {
+            return ReadOnlyStringMapUtil.equals(this, (ReadOnlyStringMap) obj);
+        }
         if (!(obj instanceof ThreadContextMap)) {
             return false;
         }
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/ReadOnlyStringMapUtil.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/ReadOnlyStringMapUtil.java
@@ -0,0 +1,72 @@
+/*
+ * 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.logging.log4j.util;
+
+import aQute.bnd.annotation.baseline.BaselineIgnore;
+import java.util.Map;
+
+/**
+ * Utility class for ReadOnlyStringMap implementations. Provides methods for equals and hashCode calculations.
+ *
+ * @since 2.25.0
+ */
+@InternalApi
+@BaselineIgnore("2.25.0")
+public final class ReadOnlyStringMapUtil {
+
+    private ReadOnlyStringMapUtil() {}
+
+    /**
+     * Compares two ReadOnlyStringMap instances for equality.
+     * Two ReadOnlyStringMap instances are considered equal if they have the same size
+     * and contain the same key-value pairs.
+     *
+     * @param map1 the first ReadOnlyStringMap to compare
+     * @param map2 the second ReadOnlyStringMap to compare
+     * @return true if the maps are equal, false otherwise
+     */
+    public static boolean equals(final ReadOnlyStringMap map1, final ReadOnlyStringMap map2) {
+        if (map1 == map2) {
+            return true;
+        }
+        if (map1 == null || map2 == null) {
+            return false;
+        }
+        if (map1.size() != map2.size()) {
+            return false;
+        }
+
+        // Convert to maps and compare
+        final Map<String, String> thisMap = map1.toMap();
+        final Map<String, String> otherMap = map2.toMap();
+        return thisMap.equals(otherMap);
+    }
+
+    /**
+     * Calculates the hash code for a ReadOnlyStringMap.
+     * The hash code is calculated based on the key-value pairs in the map.
+     *
+     * @param map the ReadOnlyStringMap to calculate the hash code for
+     * @return the hash code
+     */
+    public static int hashCode(final ReadOnlyStringMap map) {
+        if (map == null) {
+            return 0;
+        }
+        return map.toMap().hashCode();
+    }
+}
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,15 @@ 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()) {
-            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;
+        return ReadOnlyStringMapUtil.equals(this, (ReadOnlyStringMap) obj);
     }
 
     @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 ReadOnlyStringMapUtil.hashCode(this);
     }
 
     @Override
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
@@ -20,6 +20,7 @@
 import org.apache.logging.log4j.util.BiConsumer;
 import org.apache.logging.log4j.util.IndexedStringMap;
 import org.apache.logging.log4j.util.ReadOnlyStringMap;
+import org.apache.logging.log4j.util.ReadOnlyStringMapUtil;
 import org.apache.logging.log4j.util.TriConsumer;
 
 /**
@@ -114,4 +115,20 @@ 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;
+        }
+        return ReadOnlyStringMapUtil.equals(this, (ReadOnlyStringMap) obj);
+    }
+
+    @Override
+    public int hashCode() {
+        return ReadOnlyStringMapUtil.hashCode(this);
+    }
 }
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
@@ -19,6 +19,7 @@
 import java.util.Map;
 import org.apache.logging.log4j.util.BiConsumer;
 import org.apache.logging.log4j.util.ReadOnlyStringMap;
+import org.apache.logging.log4j.util.ReadOnlyStringMapUtil;
 import org.apache.logging.log4j.util.StringMap;
 import org.apache.logging.log4j.util.TriConsumer;
 
@@ -81,4 +82,20 @@ 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;
+        }
+        return ReadOnlyStringMapUtil.equals(this, (ReadOnlyStringMap) obj);
+    }
+
+    @Override
+    public int hashCode() {
+        return ReadOnlyStringMapUtil.hashCode(this);
+    }
 }
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
@@ -24,6 +24,7 @@
 import java.util.WeakHashMap;
 import org.apache.logging.log4j.util.BiConsumer;
 import org.apache.logging.log4j.util.ReadOnlyStringMap;
+import org.apache.logging.log4j.util.ReadOnlyStringMapUtil;
 import org.apache.logging.log4j.util.StringMap;
 import org.apache.logging.log4j.util.Strings;
 import org.apache.logging.log4j.util.TriConsumer;
@@ -219,15 +220,14 @@ 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;
+        return ReadOnlyStringMapUtil.equals(this, (ReadOnlyStringMap) object);
     }
 
     @Override
     public int hashCode() {
-        return map.hashCode() + (immutable ? 31 : 0);
+        return ReadOnlyStringMapUtil.hashCode(this);
     }
 }
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,10 +24,10 @@
 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;
+import org.apache.logging.log4j.util.ReadOnlyStringMapUtil;
 import org.apache.logging.log4j.util.StringMap;
 import org.apache.logging.log4j.util.TriConsumer;
 
@@ -301,27 +301,7 @@ public boolean equals(final Object obj) {
         if (!(obj instanceof ReadOnlyStringMap)) {
             return false;
         }
-        final ReadOnlyStringMap other = (ReadOnlyStringMap) obj;
-        if (other.size() != 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;
+        return ReadOnlyStringMapUtil.equals(this, (ReadOnlyStringMap) obj);
     }
 
     @Override
@@ -699,25 +679,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 ReadOnlyStringMapUtil.hashCode(this);
     }
 
     @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
`@@ -24,6 +24,7 @@`
`24`	`24`	`import java.util.WeakHashMap;`
`25`	`25`	`import org.apache.logging.log4j.util.BiConsumer;`
`26`	`26`	`import org.apache.logging.log4j.util.ReadOnlyStringMap;`
	`27`	`+import org.apache.logging.log4j.util.ReadOnlyStringMapUtil;`
`27`	`28`	`import org.apache.logging.log4j.util.StringMap;`
`28`	`29`	`import org.apache.logging.log4j.util.Strings;`
`29`	`30`	`import org.apache.logging.log4j.util.TriConsumer;`
`@@ -219,15 +220,14 @@ public boolean equals(final Object object) {`
`219`	`220`	`if (object == this) {`
`220`	`221`	`return true;`
`221`	`222`	`}`
`222`		`- if (!(object instanceof JdkMapAdapterStringMap)) {`
	`223`	`+ if (!(object instanceof ReadOnlyStringMap)) {`
`223`	`224`	`return false;`
`224`	`225`	`}`
`225`		`- final JdkMapAdapterStringMap other = (JdkMapAdapterStringMap) object;`
`226`		`- return map.equals(other.map) && immutable == other.immutable;`
	`226`	`+ return ReadOnlyStringMapUtil.equals(this, (ReadOnlyStringMap) object);`
`227`	`227`	`}`
`228`	`228`
`229`	`229`	`@Override`
`230`	`230`	`public int hashCode() {`
`231`		`- return map.hashCode() + (immutable ? 31 : 0);`
	`231`	`+ return ReadOnlyStringMapUtil.hashCode(this);`
`232`	`232`	`}`
`233`	`233`	`}`