Fixed hashCode in DBusPath; Deprecated DBusMap; Removed usages of DBu…

…sMap
hypfvieh · May 19, 2024 · 80fb861 · 80fb861
1 parent a9fb077
commit 80fb861
Show file tree

Hide file tree

Showing 10 changed files with 114 additions and 38 deletions.
diff --git a/README.md b/README.md
@@ -30,6 +30,20 @@ If you need file descriptors as well you also have to add a proper implementatio
 
 If you don't know what abstract unixsockets are and you don't need file descriptors you'll probably you can use native-unixsockets.
 
+### Upgrade notes for 5.1.0
+Upgrading to 5.1.0 should be no problem in most cases.
+
+Beginning with 5.1.0 the behavior of `Variant<?>` has been altered to get a more consistent behavior.
+More about that can be found [here](https://hypfvieh.github.io/dbus-java/variant-handling.html) .
+
+If you used `DBusMap` before, you should use `LinkedHashMap` instead.
+
+All methods previously returned instances of `DBusMap` will now return `LinkedHashMap`.
+This is transparent if you didn't use `DBusMap` directly but using `Map` interface instead.
+
+In this version the broken `hashCode()` implementation has been fixed in `DBusPath` allowing proper usage
+of `DBusPath` as `Map` key. Before equal objects did not create the same `hashCode()` breaking the contract of `hashCode()` and `equals()`.
+
 ### Note to SPI providers
 If you have used the SPI to extend the MessageReader/Writer of dbus-java before dbus-java 4.x, you have to update your code.
 Old providers will not work with dbus-java 4.x/5.x because of changed SPI interfaces (sorry!).
@@ -121,7 +135,9 @@ The library will remain open source and MIT licensed and can still be used, fork
    - Fixed issues in InterfaceCodeGenerator regarding missing imports or wrong annotation content ([#257](https://github.com/hypfvieh/dbus-java/issues/257))
    - Fixed issues with `GetAll` on Properites using Annotations ([#258](https://github.com/hypfvieh/dbus-java/issues/258))
    - Changed behavior of de-serialization on Variants containing Collections (Lists). Collections which contained a object which also has a primitive representation the collection was always converted to an array of primitives (e.g. Variant<List<Integer>> got Variant<int[]> on de-serialization). This is usually not expected. When defining a Variant<List<Integer>> it is expected to return that same type when de-serialized. The wrong behavior also caused issues when using `GetAll` method in `Properties` interface. [More information](https://hypfvieh.github.io/dbus-java/variant-handling.html) 
-
+   - Deprecated `DBusMap` - all methods previously used or returned `DBusMap` will now return a `LinkedHashMap`
+   - Fixed `hashCode()` and `equals()` method in `DBusPath` (`hashCode()` was completely wrong and violating the `hashCode()` contract when e.g. used as key in maps)
+
 ##### Changes in 5.0.0 (2024-01-25):
    - **Updated minimum required Java version to 17**
    - Removed all classes and methods marked as deprecated in 4.x

diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/DBusMap.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/DBusMap.java
@@ -10,6 +10,7 @@
 import java.util.Set;
 import java.util.stream.Collectors;
 
+@Deprecated(forRemoval = true, since = "5.1.0 - 2024-05-19")
 public class DBusMap<K, V> implements Map<K, V> {
     // CHECKSTYLE:OFF
     Object[][] entries;

diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/DBusPath.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/DBusPath.java
@@ -19,16 +19,18 @@ public String toString() {
     }
 
     @Override
-    public boolean equals(Object _other) {
-        return _other instanceof DBusPath dp && getPath() != null && getPath().equals(dp.getPath());
+    public int hashCode() {
+        return Objects.hash(path);
     }
 
     @Override
-    public int hashCode() {
-        final int prime = 31;
-        int result = super.hashCode();
-        result = prime * result + Objects.hash(path);
-        return result;
+    public boolean equals(Object _obj) {
+        if (this == _obj) {
+            return true;
+        } else if (_obj == null || getClass() != _obj.getClass()) {
+            return false;
+        }
+        return Objects.equals(path, ((DBusPath) _obj).path);
     }
 
     @Override

diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/Marshalling.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/Marshalling.java
@@ -636,7 +636,7 @@ static Object deSerializeParameter(Object _parameter, Type _type, AbstractConnec
         }
 
         // make sure arrays are in the correct format
-        if (parameter instanceof Object[] || parameter instanceof List || parameter.getClass().isArray()) {
+        if (parameter instanceof Object[] || parameter instanceof List<?> || parameter.getClass().isArray()) {
             if (_type instanceof ParameterizedType pt) {
                 parameter = ArrayFrob.convert(parameter, (Class<? extends Object>) pt.getRawType());
             } else if (_type instanceof GenericArrayType gat) {
@@ -663,7 +663,7 @@ static Object deSerializeParameter(Object _parameter, Type _type, AbstractConnec
                 parameter = ArrayFrob.convert(parameter, o.getClass());
             }
         }
-        if (parameter instanceof DBusMap<?, ?> dmap) {
+        if (parameter instanceof Map<?, ?> dmap) {
             LOGGER.trace("Deserializing a Map");
 
             Type[] maptypes;
@@ -673,10 +673,13 @@ static Object deSerializeParameter(Object _parameter, Type _type, AbstractConnec
                 maptypes = parameter.getClass().getTypeParameters();
             }
 
-            for (int i = 0; i < dmap.entries.length; i++) {
-                dmap.entries[i][0] = deSerializeParameter(dmap.entries[i][0], maptypes[0], _conn);
-                dmap.entries[i][1] = deSerializeParameter(dmap.entries[i][1], maptypes[1], _conn);
+            Map<Object, Object> map = new LinkedHashMap<>();
+            for (Entry<?, ?> e : dmap.entrySet()) {
+                map.put(deSerializeParameter(e.getKey(), maptypes[0], _conn),
+                    deSerializeParameter(e.getValue(), maptypes[1], _conn));
             }
+
+            parameter = map;
         }
         return parameter;
     }

diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/ObjectPath.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/ObjectPath.java
@@ -30,15 +30,10 @@ public int hashCode() {
     public boolean equals(Object _obj) {
         if (this == _obj) {
             return true;
-        }
-        if (!super.equals(_obj)) {
-            return false;
-        }
-        if (getClass() != _obj.getClass()) {
+        } else if (!super.equals(_obj) || getClass() != _obj.getClass()) {
             return false;
         }
-        ObjectPath other = (ObjectPath) _obj;
-        return Objects.equals(source, other.source);
+        return Objects.equals(source, ((ObjectPath) _obj).source);
     }
 
 }
diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/messages/Message.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/messages/Message.java
@@ -25,10 +25,7 @@
 import java.lang.reflect.Type;
 import java.nio.charset.StandardCharsets;
 import java.text.MessageFormat;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 import java.util.concurrent.atomic.AtomicLong;
 import java.util.function.BiFunction;
 import java.util.stream.Collectors;
@@ -1128,12 +1125,16 @@ private Object optimizePrimitives(byte[] _signatureBuf, byte[] _dataBuf, int[] _
         if (_signatureBuf[_offsets[OFFSET_SIG]] == DICT_ENTRY1) {
             int ofssave = prepareCollection(_signatureBuf, _offsets, _size);
             long end = _offsets[OFFSET_DATA] + _size;
-            List<Object[]> entries = new ArrayList<>();
+
+            Map<Object, Object> map = new LinkedHashMap<>();
             while (_offsets[OFFSET_DATA] < end) {
                 _offsets[OFFSET_SIG] = ofssave;
-                entries.add((Object[]) _extractMethod.extractOne(_signatureBuf, _dataBuf, _offsets, ExtractOptions.copyWithContainedFlag(_options, true)));
+                Object[] data = (Object[]) _extractMethod.extractOne(_signatureBuf, _dataBuf, _offsets, ExtractOptions.copyWithContainedFlag(_options, true));
+
+                map.put(data[0], data[1]);
             }
-            rv = new DBusMap<>(entries.toArray(new Object[0][]));
+
+            rv = map;
         }
 
         if (rv == null) {

diff --git a/dbus-java-tests/src/test/java/org/freedesktop/dbus/test/DBusPathTest.java b/dbus-java-tests/src/test/java/org/freedesktop/dbus/test/DBusPathTest.java
@@ -0,0 +1,30 @@
+package org.freedesktop.dbus.test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+
+import org.freedesktop.dbus.DBusPath;
+import org.freedesktop.dbus.ObjectPath;
+import org.junit.jupiter.api.Test;
+
+public class DBusPathTest {
+
+    @Test
+    void testEquals() {
+        DBusPath p1 = new DBusPath("/bla");
+        DBusPath p2 = new DBusPath("/bla");
+
+        ObjectPath o1 = new ObjectPath("/bla", null);
+
+        assertEquals(p1, p2);
+        assertNotEquals(p1, o1);
+    }
+
+    @Test
+    void testHashCode() {
+        DBusPath p1 = new DBusPath("/bla");
+        DBusPath p2 = new DBusPath("/bla");
+
+        assertEquals(p1.hashCode(), p2.hashCode());
+    }
+}
diff --git a/dbus-java-tests/src/test/java/org/freedesktop/dbus/test/MarshallingTest.java b/dbus-java-tests/src/test/java/org/freedesktop/dbus/test/MarshallingTest.java
@@ -25,8 +25,10 @@
 import java.lang.reflect.*;
 import java.nio.file.Files;
 import java.util.ArrayList;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Map.Entry;
 
 public class MarshallingTest extends AbstractBaseTest {
 
@@ -142,6 +144,31 @@ public void testDeserializeParametersVariant() throws Exception {
         assertEquals(3, mt.getValue().get(2), "3 expected");
     }
 
+    @Test
+    public void testDeserializeMap() throws Exception {
+        DBusPath key = new DBusPath("/foo");
+        DBusPath val = new DBusPath("/bar");
+        Map<DBusPath, DBusPath> testMap = Map.of(key, val);
+
+        Type[] types = new Type[] {testMap.getClass()};
+
+        Object[] deSerializeParameters = Marshalling.deSerializeParameters(new Object[] {testMap}, types, null);
+
+        assertNotNull(deSerializeParameters);
+
+        Map<?, ?> deserializedMap = (Map<?, ?>) deSerializeParameters[0];
+
+        assertFalse(testMap == deserializedMap, "Expected new object");
+        assertEquals(LinkedHashMap.class, deserializedMap.getClass(), "Expected new LinkedHashMap");
+
+        Entry<?, ?> next = deserializedMap.entrySet().iterator().next();
+
+        assertEquals(key, next.getKey());
+        assertEquals(val, next.getValue());
+
+        assertTrue(deserializedMap.containsKey(key));
+    }
+
     /*
      ******************************************
      *

diff --git a/dbus-java-tests/src/test/java/org/freedesktop/dbus/test/MethodTest.java b/dbus-java-tests/src/test/java/org/freedesktop/dbus/test/MethodTest.java
@@ -46,12 +46,17 @@ public void testCallRemoteMethod() throws DBusException {
         pathm.put(path, path);
         Map<DBusPath, DBusPath> pm = tri.pathmaprv(pathm);
 
-        logger.debug(pathm.toString() + " => " + pm.toString());
-        logger.debug(pm.containsKey(path) + " " + pm.get(path) + " " + path.equals(pm.get(path)));
-        logger.debug(pm.containsKey(p) + " " + pm.get(p) + " " + p.equals(pm.get(p)));
+        assertNotEquals(pathm.getClass(), pm.getClass(), "Expected different map implementations");
 
-        assertTrue(pm.containsKey(path), "pathmaprv incorrect");
-        assertEquals(path, pm.get(path), "pathmaprv incorrect");
+        logger.debug("{} => {}", pathm, pm);
+        logger.debug("{} {} {}", pm.containsKey(path), pm.get(path), path.equals(pm.get(path)));
+        logger.debug("{} {} {}", pm.containsKey(p), pm.get(p), p.equals(pm.get(p)));
+
+        System.out.println("path hashcode: " + path.hashCode());
+        System.out.println("pm key hashcode: " + pm.keySet().iterator().next().hashCode());
+
+        assertTrue(pm.containsKey(path), "pathmaprv missing in map");
+        assertEquals(path, pm.get(path), "pathmaprv incorrect value");
     }
 
     @Test

diff --git a/dbus-java-tests/src/test/java/org/freedesktop/dbus/test/VariantTest.java b/dbus-java-tests/src/test/java/org/freedesktop/dbus/test/VariantTest.java
@@ -1,6 +1,5 @@
 package org.freedesktop.dbus.test;
 
-import org.freedesktop.dbus.DBusMap;
 import org.freedesktop.dbus.Marshalling;
 import org.freedesktop.dbus.connections.impl.BaseConnectionBuilder;
 import org.freedesktop.dbus.exceptions.DBusException;
@@ -24,10 +23,7 @@
 import java.nio.channels.SocketChannel;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.List;
-import java.util.Map;
+import java.util.*;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicReference;
@@ -58,7 +54,7 @@ void testVariant() {
 
     static List<VariantData> createTestData() {
         return List.of(
-            new VariantData(new Variant<>(Map.of("val1", 1, "val2", 2), "a{si}"), "String Int Map", DBusMapType.class, DBusMap.class, Map.of("val1", 1, "val2", 2)),
+            new VariantData(new Variant<>(Map.of("val1", 1, "val2", 2), "a{si}"), "String Int Map", DBusMapType.class, LinkedHashMap.class, Map.of("val1", 1, "val2", 2)),
             new VariantData(new Variant<>(List.of("str", "ing"), "as"), "String List Variant", DBusListType.class, ArrayList.class, List.of("str", "ing")),
             new VariantData(new Variant<>(List.of(1, 2), "ai"), "Integer List Variant", DBusListType.class, ArrayList.class, List.of(1, 2)),
             new VariantData(new Variant<>(List.of(true, true, false), "ab"), "Boolean List Variant", DBusListType.class, ArrayList.class, List.of(true, true, false))