fix: DH-18265 - correct bugs in replace() function

src/main/java/io/deephaven/hash/KeyedDoubleObjectHash.java

@@ -251,19 +251,25 @@ public synchronized V replace(double key, V value) {
   }
 
   public synchronized boolean replace(Double key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!doubleKeyDef.equalDoubleKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + doubleKeyDef.getDoubleKey(newValue));
     }
-    return internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   public synchronized boolean replace(double key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!doubleKeyDef.equalDoubleKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + doubleKeyDef.getDoubleKey(newValue));
     }
-    return internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedDoubleObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   private static final int NORMAL = 0;

src/main/java/io/deephaven/hash/KeyedIntObjectHash.java

@@ -248,19 +248,25 @@ public synchronized V replace(int key, V value) {
   }
 
   public synchronized boolean replace(Integer key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!intKeyDef.equalIntKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + intKeyDef.getIntKey(newValue));
     }
-    return internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   public synchronized boolean replace(int key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!intKeyDef.equalIntKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + intKeyDef.getIntKey(newValue));
     }
-    return internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedIntObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   private static final int NORMAL = 0;
@@ -304,7 +310,8 @@ protected V internalPut(V value, int mode, V oldValue) {
           }
           return null;
         } else if (candidate != DELETED && intKeyDef.equalIntKey(key, candidate)) {
-          if (mode != KeyedIntObjectHash.IF_ABSENT) {
+          if (mode != KeyedIntObjectHash.IF_ABSENT
+              && (oldValue == null || candidate.equals(oldValue))) {
             state[index] = value;
             _indexableList = null;
           }

src/main/java/io/deephaven/hash/KeyedLongObjectHash.java

@@ -249,19 +249,25 @@ public synchronized V replace(long key, V value) {
   }
 
   public synchronized boolean replace(Long key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!longKeyDef.equalLongKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + longKeyDef.getLongKey(newValue));
     }
-    return internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   public synchronized boolean replace(long key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!longKeyDef.equalLongKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + longKeyDef.getLongKey(newValue));
     }
-    return internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue) != null;
+    return internalPut(newValue, KeyedLongObjectHash.REPLACE, oldValue).equals(oldValue);
   }
 
   private static final int NORMAL = 0;
@@ -305,7 +311,8 @@ protected V internalPut(V value, int mode, V oldValue) {
           }
           return null;
         } else if (candidate != DELETED && longKeyDef.equalLongKey(key, candidate)) {
-          if (mode != KeyedLongObjectHash.IF_ABSENT) {
+          if (mode != KeyedLongObjectHash.IF_ABSENT
+              && (oldValue == null || candidate.equals(oldValue))) {
             state[index] = value;
             _indexableList = null;
           }

src/main/java/io/deephaven/hash/KeyedObjectHash.java

@@ -700,11 +700,14 @@ public synchronized V replace(K key, V value) {
   }
 
   public synchronized boolean replace(K key, V oldValue, V newValue) {
+    if (oldValue == null) {
+      throw new NullPointerException("oldValue is null, but this map cannot hold null values");
+    }
     if (!keyDef.equalKey(key, newValue)) {
       throw new IllegalArgumentException(
           "key and value are inconsistent:" + key + " and " + keyDef.getKey(newValue));
     }
-    return internalPut(newValue, REPLACE, oldValue) != null;
+    return internalPut(newValue, REPLACE, oldValue).equals(oldValue);
   }
 
   public synchronized boolean add(V value) {
@@ -761,7 +764,7 @@ protected V internalPut(V value, int mode, V oldValue) {
           }
           return null;
         } else if (candidate != DELETED && keyDef.equalKey(key, candidate)) {
-          if (mode != IF_ABSENT) {
+          if (mode != IF_ABSENT && (oldValue == null || candidate.equals(oldValue))) {
             state[index] = value;
             _indexableList = null;
           }

src/test/java/io/deephaven/hash/TestKeyedIntObjectHashMap.java

@@ -118,6 +118,77 @@ public void testSimpleUnboxedPutIfAbsent() {
     assertTrue(m.get(o2.getId()) == o1);
   }
 
+  /*
+   ** tests the unboxed replace call
+   */
+  public void testSimpleUnboxedReplace() {
+    KeyedIntTestObject result;
+
+    final KeyedIntTestObjectMap m = new KeyedIntTestObjectMap(10);
+
+    final KeyedIntTestObject o1 = new KeyedIntTestObject(0);
+    final KeyedIntTestObject o2 = new KeyedIntTestObject(0);
+    final KeyedIntTestObject o3 = new KeyedIntTestObject(0);
+
+    result = m.putIfAbsent(0, o1);
+    assertNull(result);
+    assertSame(m.get(0), o1); // strict equality test
+
+    result = m.putIfAbsent(0, o2);
+    assertNotNull(result);
+    assertSame(m.get(0), o1); // strict equality test
+
+    result = m.put(0, o2);
+    assertNotNull(result);
+    assertSame(result, o1); // strict equality test
+    assertSame(m.get(0), o2); // strict equality test
+
+    assertFalse(m.replace(0, new KeyedIntTestObject(10), o3));
+    assertSame(m.get(0), o2); // strict equality test
+
+    assertTrue(m.replace(0, new KeyedIntTestObject(0), o3));
+    assertSame(m.get(0), o3); // strict equality test
+  }
+
+  /*
+   * Reproducer for bug documented in DH-18265
+   */
+  public void testDH18265() {
+    KeyedIntTestObject result;
+
+    final KeyedIntTestObjectMap m = new KeyedIntTestObjectMap(10);
+
+    // Setup the conditions for the bug to be triggered.
+    final int capacity = m.capacity();
+
+    // This will hash to 0 internally
+    final KeyedIntTestObject o1 = new KeyedIntTestObject(capacity);
+    result = m.putIfAbsent(capacity, o1);
+    assertNull(result);
+    assertSame(m.get(capacity), o1); // strict equality test
+
+    // This will also initially hash to 0, but will be double hashed to an empty slot.
+    final KeyedIntTestObject o2 = new KeyedIntTestObject(0);
+    result = m.putIfAbsent(0, o2);
+    assertNull(result);
+    assertSame(m.get(0), o2); // strict equality test
+
+    // Remove the first object, leaving a DELETED tombstone at the 0 slot.
+    result = m.remove(capacity);
+    assertNotNull(result);
+    assertSame(result, o1); // strict equality test
+
+    // This replace should fail, since we do not match old values.
+    final KeyedIntTestObject o3 = new KeyedIntTestObject(10);
+    final KeyedIntTestObject o4 = new KeyedIntTestObject(0);
+    assertFalse(m.replace(0, o3, o4));
+    assertSame(m.get(0), o2); // strict equality test
+
+    // This replace should succeed, since we match the old value.
+    assertTrue(m.replace(0, o2, o4));
+    assertSame(m.get(0), o4); // strict equality test
+  }
+
   /*
    ** tests for KeyedIntObjectMaps -- putIfAbsent(K, ValueFactory)
    */

src/test/java/io/deephaven/hash/TestKeyedLongObjectHashMap.java

@@ -121,6 +121,77 @@ public void testSimpleUnboxedPutIfAbsent() {
     assertTrue(m.get(o2.getId()) == o1);
   }
 
+  /*
+   ** tests the unboxed replace call
+   */
+  public void testSimpleUnboxedReplace() {
+    KeyedLongTestObject result;
+
+    final KeyedLongTestObjectMap m = new KeyedLongTestObjectMap(10);
+
+    final KeyedLongTestObject o1 = new KeyedLongTestObject(0);
+    final KeyedLongTestObject o2 = new KeyedLongTestObject(0);
+    final KeyedLongTestObject o3 = new KeyedLongTestObject(0);
+
+    result = m.putIfAbsent(0, o1);
+    assertNull(result);
+    assertSame(m.get(0), o1); // strict equality test
+
+    result = m.putIfAbsent(0, o2);
+    assertNotNull(result);
+    assertSame(m.get(0), o1); // strict equality test
+
+    result = m.put(0, o2);
+    assertNotNull(result);
+    assertSame(result, o1); // strict equality test
+    assertSame(m.get(0), o2); // strict equality test
+
+    assertFalse(m.replace(0, new KeyedLongTestObject(10), o3));
+    assertSame(m.get(0), o2); // strict equality test
+
+    assertTrue(m.replace(0, new KeyedLongTestObject(0), o3));
+    assertSame(m.get(0), o3); // strict equality test
+  }
+
+  /*
+   * Reproducer for bug documented in DH-18265
+   */
+  public void testDH18265() {
+    KeyedLongTestObject result;
+
+    final KeyedLongTestObjectMap m = new KeyedLongTestObjectMap(10);
+
+    // Setup the conditions for the bug to be triggered.
+    final int capacity = m.capacity();
+
+    // This will hash to 0 internally
+    final KeyedLongTestObject o1 = new KeyedLongTestObject(capacity);
+    result = m.putIfAbsent(capacity, o1);
+    assertNull(result);
+    assertSame(m.get(capacity), o1); // strict equality test
+
+    // This will also initially hash to 0, but will be double hashed to an empty slot.
+    final KeyedLongTestObject o2 = new KeyedLongTestObject(0);
+    result = m.putIfAbsent(0, o2);
+    assertNull(result);
+    assertSame(m.get(0), o2); // strict equality test
+
+    // Remove the first object, leaving a DELETED tombstone at the 0 slot.
+    result = m.remove(capacity);
+    assertNotNull(result);
+    assertSame(result, o1); // strict equality test
+
+    // This replace should fail, since we do not match old values.
+    final KeyedLongTestObject o3 = new KeyedLongTestObject(10);
+    final KeyedLongTestObject o4 = new KeyedLongTestObject(0);
+    assertFalse(m.replace(0, o3, o4));
+    assertSame(m.get(0), o2); // strict equality test
+
+    // This replace should succeed, since we match the old value.
+    assertTrue(m.replace(0, o2, o4));
+    assertSame(m.get(0), o4); // strict equality test
+  }
+
   /*
    ** tests for KeyedLongObjectMaps -- putIfAbsent(K, ValueFactory)
    */

src/test/java/io/deephaven/hash/TestKeyedObjectHashMap.java

@@ -274,4 +274,69 @@ public void run() {
       fail("Unhandled exception: " + x);
     }
   }
+
+  public void testSimpleKOMReplace() {
+    KeyedTestObject result;
+    final KeyedTestObjectMap m = new KeyedTestObjectMap(10);
+
+    final KeyedTestObject o1 = new KeyedTestObject("0");
+    final KeyedTestObject o2 = new KeyedTestObject("0");
+    final KeyedTestObject o3 = new KeyedTestObject("0");
+
+    result = m.putIfAbsent("0", o1);
+    assertNull(result);
+    assertSame(m.get("0"), o1); // strict equality test
+
+    result = m.putIfAbsent("0", o2);
+    assertNotNull(result);
+    assertSame(m.get("0"), o1); // strict equality test
+
+    result = m.put("0", o2);
+    assertNotNull(result);
+    assertSame(result, o1); // strict equality test
+    assertSame(m.get("0"), o2); // strict equality test
+
+    assertFalse(m.replace("0", new KeyedTestObject("10"), o3));
+    assertSame(m.get("0"), o2); // strict equality test
+
+    assertTrue(m.replace("0", new KeyedTestObject("0"), o3));
+    assertSame(m.get("0"), o3); // strict equality test
+  }
+
+  /*
+   * Reproducer for bug documented in DH-18265
+   */
+  public void testDH18265() {
+    KeyedTestObject result;
+
+    final KeyedTestObjectMap m = new KeyedTestObjectMap(10);
+
+    // This test relies on the fact that `FB` and `Ea` hash to the same value.
+
+    final KeyedTestObject o1 = new KeyedTestObject("FB");
+    result = m.putIfAbsent("FB", o1);
+    assertNull(result);
+    assertSame(m.get("FB"), o1); // strict equality test
+
+    // This hash collides with `FB`, but will be double hashed to an empty slot.
+    final KeyedTestObject o2 = new KeyedTestObject("Ea");
+    result = m.putIfAbsent("Ea", o2);
+    assertNull(result);
+    assertSame(m.get("Ea"), o2); // strict equality test
+
+    // Remove the first object, leaving a DELETED tombstone at the first slot.
+    result = m.remove("FB");
+    assertNotNull(result);
+    assertSame(result, o1); // strict equality test
+
+    // This replace should fail, since we do not match old values.
+    final KeyedTestObject o3 = new KeyedTestObject("AB");
+    final KeyedTestObject o4 = new KeyedTestObject("Ea");
+    assertFalse(m.replace("Ea", o3, o4));
+    assertSame(m.get("Ea"), o2); // strict equality test
+
+    // This replace should succeed, since we match the old value.
+    assertTrue(m.replace("Ea", o2, o4));
+    assertSame(m.get("Ea"), o4); // strict equality test
+  }
 }