SOLR-17623: SimpleOrderedMap equals, hashCode (and for Map.Entry)

And ensure a NamedList is never equal to a SimpleOrderedMap

solr/solrj/src/java/org/apache/solr/common/util/NamedList.java

@@ -18,6 +18,7 @@
 
 import java.io.IOException;
 import java.io.Serializable;
+import java.util.AbstractMap;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
@@ -566,35 +567,11 @@ public SolrParams toSolrParams() {
    * Helper class implementing Map.Entry<String, T> to store the key-value relationship in
    * NamedList (the keys of which are String-s)
    */
-  public static final class NamedListEntry<T> implements Map.Entry<String, T> {
-
-    public NamedListEntry() {}
-
+  @Deprecated // use AbstractMap.SimpleEntry or Map.entry() (albeit no nulls)
+  public static final class NamedListEntry<T> extends AbstractMap.SimpleEntry<String, T> {
     public NamedListEntry(String _key, T _value) {
-      key = _key;
-      value = _value;
-    }
-
-    @Override
-    public String getKey() {
-      return key;
-    }
-
-    @Override
-    public T getValue() {
-      return value;
-    }
-
-    @Override
-    public T setValue(T _value) {
-      T oldValue = value;
-      value = _value;
-      return oldValue;
+      super(_key, _value);
     }
-
-    private String key;
-
-    private T value;
   }
 
   /** Iterates over the Map and sequentially adds its key/value pairs */
@@ -627,53 +604,63 @@ public NamedList<T> clone() {
   /** Support the Iterable interface */
   @Override
   public Iterator<Map.Entry<String, T>> iterator() {
+    return new Iterator<>() {
 
-    final NamedList<T> list = this;
+      int idx = 0;
 
-    Iterator<Map.Entry<String, T>> iter =
-        new Iterator<Map.Entry<String, T>>() {
+      @Override
+      public boolean hasNext() {
+        return idx < NamedList.this.size();
+      }
 
-          int idx = 0;
+      @Override
+      public Map.Entry<String, T> next() {
+        return new Map.Entry<>() {
+          final int index = idx++;
 
           @Override
-          public boolean hasNext() {
-            return idx < list.size();
+          public String getKey() {
+            return NamedList.this.getName(index);
           }
 
           @Override
-          public Map.Entry<String, T> next() {
-            final int index = idx++;
-            Map.Entry<String, T> nv =
-                new Map.Entry<String, T>() {
-                  @Override
-                  public String getKey() {
-                    return list.getName(index);
-                  }
-
-                  @Override
-                  public T getValue() {
-                    return list.getVal(index);
-                  }
-
-                  @Override
-                  public String toString() {
-                    return getKey() + "=" + getValue();
-                  }
-
-                  @Override
-                  public T setValue(T value) {
-                    return list.setVal(index, value);
-                  }
-                };
-            return nv;
+          public T getValue() {
+            return NamedList.this.getVal(index);
           }
 
           @Override
-          public void remove() {
-            throw new UnsupportedOperationException();
+          public T setValue(T value) {
+            return NamedList.this.setVal(index, value);
+          }
+
+          // The following implement the Map.Entry specification:
+
+          @Override
+          public boolean equals(Object o) {
+            return o instanceof Map.Entry<?, ?> e
+                && Objects.equals(getKey(), e.getKey())
+                && Objects.equals(getValue(), e.getValue());
+          }
+
+          @Override
+          public int hashCode() {
+            var key = getKey();
+            var value = getValue();
+            return (key == null ? 0 : key.hashCode()) ^ (value == null ? 0 : value.hashCode());
+          }
+
+          @Override
+          public String toString() {
+            return getKey() + "=" + getValue();
           }
         };
-    return iter;
+      }
+
+      @Override
+      public void remove() {
+        throw new UnsupportedOperationException();
+      }
+    };
   }
 
   /**
@@ -830,7 +817,11 @@ public int hashCode() {
 
   @Override
   public boolean equals(Object obj) {
+    if (obj == this) return true;
     if (!(obj instanceof NamedList<?> nl)) return false;
+    if (obj instanceof SimpleOrderedMap<?>) {
+      return false;
+    }
     return this.nvPairs.equals(nl.nvPairs);
   }
 

solr/solrj/src/java/org/apache/solr/common/util/SimpleOrderedMap.java

@@ -56,12 +56,8 @@ public SimpleOrderedMap(int sz) {
   /**
    * Creates an instance backed by an explicitly specified list of pairwise names/values.
    *
-   * <p>TODO: this method was formerly public, now that it's not we can change the impl details of
-   * this class to be based on a Map.Entry[]
-   *
    * @param nameValuePairs underlying List which should be used to implement a SimpleOrderedMap;
    *     modifying this List will affect the SimpleOrderedMap.
-   * @lucene.internal
    */
   private SimpleOrderedMap(List<Object> nameValuePairs) {
     super(nameValuePairs);
@@ -80,6 +76,19 @@ public SimpleOrderedMap<T> clone() {
     return new SimpleOrderedMap<>(newList);
   }
 
+  @SuppressWarnings("EqualsDoesntCheckParameterClass")
+  @Override
+  public boolean equals(Object obj) {
+    return obj == this || new InnerMap().equals(obj); // Use AbstractMap's code
+  }
+
+  @Override
+  public int hashCode() {
+    return new InnerMap().hashCode(); // Use AbstractMap's code
+  }
+
+  // toString is inherited and implements the Map contract (and this is tested)
+
   @Override
   public boolean isEmpty() {
     return nvPairs.isEmpty();

solr/solrj/src/test/org/apache/solr/common/util/SimpleOrderedMapTest.java

@@ -17,6 +17,7 @@
 package org.apache.solr.common.util;
 
 import java.util.Collection;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Set;
 import org.apache.solr.SolrTestCase;
@@ -38,6 +39,37 @@ public void testPut() {
     assertEquals(2, map.nvPairs.get(3));
   }
 
+  @Test
+  public void testEquals() {
+    var map2 = new SimpleOrderedMap<Integer>();
+    map2.put("one", 1);
+    map2.put("two", 2);
+
+    var map3 = map2.clone();
+    assertNotSame(map2, map3);
+    assertEquals(map2, map3);
+    map3.put("three", 3);
+    assertNotEquals(map2, map3);
+    map3.remove("one");
+    map3.remove("three");
+    map3.add("one", 1); // now it's a different order
+    assertNotEquals(map2.toString(), map3.toString());
+    assertEquals(map2, map3); // but still equals despite different order
+    assertEquals(map2.hashCode(), map3.hashCode());
+
+    var nl2 = new NamedList<>(map2);
+    assertNotEquals(map2, nl2);
+    assertNotEquals(nl2, map2);
+  }
+
+  public void testToString() {
+    SimpleOrderedMap<Object> map = new SimpleOrderedMap<>();
+    map.add("one", 1);
+    map.add("two", 2);
+    map.add("aNull", null);
+    assertEquals(new LinkedHashMap<>(map).toString(), map.toString());
+  }
+
   public void testPutReturnOldValue() {
     map.put("one", 1);
     int oldValue = map.put("one", 2);