Issue #268: Improved handling of signals in combination with array/pr…

…imitive array or Collections in constructor
hypfvieh · Sep 14, 2024 · 10becbd · 10becbd
1 parent af40040
commit 10becbd
Show file tree

Hide file tree

Showing 12 changed files with 514 additions and 38 deletions.
diff --git a/README.md b/README.md
@@ -135,6 +135,8 @@ The library will remain open source and MIT licensed and can still be used, fork
    - Added support for `EmitsChangedSignal` ([PR#267](https://github.com/hypfvieh/dbus-java/issues/267)), thanks to [GeVa2072](https://github.com/GeVa2072)
    - Tighten PMD rules to disallow usage of `var` keyword
    - Updated Maven plugins
+   - Improved documentation
+   - Fixed issue with arrays, primitive arrays and `Collection` when used in signal constructors ([#268](https://github.com/hypfvieh/dbus-java/issues/268))
 
 ##### Changes in 5.1.0 (2024-08-01):
    - Use Junit BOM thanks to [spannm](https://github.com/spannm) ([PR#248](https://github.com/hypfvieh/dbus-java/issues/248))

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
@@ -654,7 +654,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 Collection<?> || parameter.getClass().isArray()) {
             if (_type instanceof ParameterizedType pt) {
                 parameter = ArrayFrob.convert(parameter, (Class<? extends Object>) pt.getRawType());
             } else if (_type instanceof GenericArrayType gat) {

diff --git a/...va-core/src/main/java/org/freedesktop/dbus/connections/base/ConnectionMessageHandler.java b/...va-core/src/main/java/org/freedesktop/dbus/connections/base/ConnectionMessageHandler.java
@@ -86,6 +86,9 @@ private void handleMessage(final DBusSignal _signal, boolean _useThreadPool) {
                         rs = _signal;
                     }
                     if (rs == null) {
+                        if (getConnectionConfig().getUnknownSignalHandler() != null) {
+                            getConnectionConfig().getUnknownSignalHandler().accept(_signal);
+                        }
                         return;
                     }
                     ((DBusSigHandler<DBusSignal>) h).handle(rs);

diff --git a/...-java-core/src/main/java/org/freedesktop/dbus/connections/impl/BaseConnectionBuilder.java b/...-java-core/src/main/java/org/freedesktop/dbus/connections/impl/BaseConnectionBuilder.java
@@ -1,13 +1,20 @@
 package org.freedesktop.dbus.connections.impl;
 
-import org.freedesktop.dbus.connections.*;
+import org.freedesktop.dbus.connections.AbstractConnection;
+import org.freedesktop.dbus.connections.BusAddress;
+import org.freedesktop.dbus.connections.IDisconnectCallback;
 import org.freedesktop.dbus.connections.base.ReceivingService;
-import org.freedesktop.dbus.connections.config.*;
+import org.freedesktop.dbus.connections.config.ReceivingServiceConfig;
+import org.freedesktop.dbus.connections.config.ReceivingServiceConfigBuilder;
+import org.freedesktop.dbus.connections.config.TransportConfig;
+import org.freedesktop.dbus.connections.config.TransportConfigBuilder;
 import org.freedesktop.dbus.connections.transports.TransportBuilder;
 import org.freedesktop.dbus.exceptions.DBusException;
+import org.freedesktop.dbus.messages.DBusSignal;
 import org.freedesktop.dbus.messages.constants.Endian;
 
 import java.nio.ByteOrder;
+import java.util.function.Consumer;
 
 /**
  * Base class for connection builders containing commonly used options.
@@ -160,6 +167,21 @@ public R withDisconnectCallback(IDisconnectCallback _disconnectCallback) {
         return self();
     }
 
+    /**
+     * Configures a consumer which will receive any signal which could not be handled.
+     * <p>
+     * By default, no handler is configured, so unknown/unhandled signals will be
+     * ignored and only be logged as warn message.
+     * </p>
+     * @param _handler callback which receives all unknown signals
+     * @return this
+     * @since 5.1.1 - 2024-09-11
+     */
+    public R withUnknownSignalHandler(Consumer<DBusSignal> _handler) {
+        connectionConfig.setUnknownSignalHandler(_handler);
+        return self();
+    }
+
     public abstract C build() throws DBusException;
 
     /**

diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/ConnectionConfig.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/connections/impl/ConnectionConfig.java
@@ -1,11 +1,15 @@
 package org.freedesktop.dbus.connections.impl;
 
 import org.freedesktop.dbus.connections.IDisconnectCallback;
+import org.freedesktop.dbus.messages.DBusSignal;
+
+import java.util.function.Consumer;
 
 public class ConnectionConfig {
     private boolean exportWeakReferences;
     private boolean importWeakReferences;
     private IDisconnectCallback disconnectCallback;
+    private Consumer<DBusSignal> unknownSignalHandler;
 
     public boolean isExportWeakReferences() {
         return exportWeakReferences;
@@ -31,4 +35,12 @@ public void setDisconnectCallback(IDisconnectCallback _disconnectCallback) {
         disconnectCallback = _disconnectCallback;
     }
 
+    public Consumer<DBusSignal> getUnknownSignalHandler() {
+        return unknownSignalHandler;
+    }
+
+    public void setUnknownSignalHandler(Consumer<DBusSignal> _unknownSignalHandler) {
+        unknownSignalHandler = _unknownSignalHandler;
+    }
+
 }
diff --git a/dbus-java-core/src/main/java/org/freedesktop/dbus/messages/DBusSignal.java b/dbus-java-core/src/main/java/org/freedesktop/dbus/messages/DBusSignal.java
@@ -217,7 +217,9 @@ public DBusSignal createReal(AbstractConnectionBase _conn) throws DBusException
         Constructor<? extends DBusSignal> con = null;
         Type[] types = null;
 
-        Object[] parameters = getParameters();
+        List<Type[]> constructorArgs = list.stream().map(c -> c.types).toList();
+
+        Object[] parameters = getParameters(constructorArgs);
 
         // Get all classes required in constructor in order
         // Primitives will always be wrapped in their wrapper classes

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
@@ -12,16 +12,14 @@
 import org.freedesktop.dbus.messages.constants.ArgumentType;
 import org.freedesktop.dbus.messages.constants.Endian;
 import org.freedesktop.dbus.messages.constants.HeaderField;
-import org.freedesktop.dbus.types.UInt16;
-import org.freedesktop.dbus.types.UInt32;
-import org.freedesktop.dbus.types.UInt64;
-import org.freedesktop.dbus.types.Variant;
+import org.freedesktop.dbus.types.*;
 import org.freedesktop.dbus.utils.Hexdump;
 import org.freedesktop.dbus.utils.LoggingHelper;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.lang.reflect.Array;
+import java.lang.reflect.ParameterizedType;
 import java.lang.reflect.Type;
 import java.nio.charset.StandardCharsets;
 import java.text.MessageFormat;
@@ -43,7 +41,7 @@ public class Message {
     public static final byte            PROTOCOL               = 1;
 
     /** Default extraction options. */
-    private static final ExtractOptions DEFAULT_OPTIONS        = new ExtractOptions(false, false);
+    private static final ExtractOptions DEFAULT_OPTIONS        = new ExtractOptions(false, List.of());
 
     /** Position of data offset in int array. */
     private static final int            OFFSET_DATA            = 1;
@@ -407,7 +405,7 @@ public String toString() {
         sb.append(' ');
         Object[] largs = null;
         try {
-            largs = getParameters();
+            largs = extractArgs(null);
         } catch (DBusException _ex) {
             logger.debug("", _ex);
         }
@@ -650,8 +648,8 @@ private int appendOne(byte[] _sigb, int _sigofs, Object _data) throws DBusExcept
                         default -> throw new MarshallingException("Primitive array being sent as non-primitive array.");
                     }
                     appendBytes(primbuf);
-                } else if (_data instanceof List<?> lst) {
-                    Object[] contents = lst.toArray();
+                } else if (_data instanceof Collection<?> coll) {
+                    Object[] contents = coll.toArray();
                     int diff = i;
                     ensureBuffers(contents.length * 4);
                     for (Object o : contents) {
@@ -912,8 +910,7 @@ private Object extractOne(byte[] _signatureBuf, byte[] _dataBuf, int[] _offsets,
                 rv = decontents;
                 break;
             case VARIANT:
-                ExtractOptions opts = new ExtractOptions(_options.contained(), false);
-                rv = extractVariant(_dataBuf, _offsets, opts, (sig, obj) -> {
+                rv = extractVariant(_dataBuf, _offsets, _options, (sig, obj) -> {
                     logger.trace("Creating Variant with SIG: {} - Value: {}", sig, obj);
                     return new Variant<>(obj, sig);
                 });
@@ -1076,7 +1073,12 @@ private Object optimizePrimitives(byte[] _signatureBuf, byte[] _dataBuf, int[] _
             throws DBusException {
         Object rv = null;
 
-        if (_options.primitiveListToArray()) {
+        int offsetPos = _offsets[OFFSET_SIG] - 1; // need to extract one because extractArray will already update offset position
+        boolean optimize = _options.arrayConvert() != null
+            && _options.arrayConvert().size() > offsetPos
+            && _options.arrayConvert().get(offsetPos) == ConstructorArgType.PRIMITIVE_ARRAY;
+
+        if (optimize) {
             switch (_signatureBuf[_offsets[OFFSET_SIG]]) {
                 case BYTE:
                     rv = new byte[_length];
@@ -1313,17 +1315,99 @@ public long getReplySerial() {
      * @throws DBusException on failure
      */
     public Object[] getParameters() throws DBusException {
-        if (null == args && null != body) {
-            String sig = getSig();
-            if (null != sig && 0 != body.length) {
-                args = extract(sig, body, 0, DEFAULT_OPTIONS);
-            } else {
-                args = new Object[0];
-            }
+        return getParameters(null);
+    }
+
+    /**
+     * Parses and returns the parameters to this message as an Object array.
+     *
+     * This method takes a list of Type[] where each entry of the list represents a constructor call.
+     * The constructor arguments are used to determine if a collection of array of
+     * primitive type should be used when calling the constructor.
+     *
+     * @param _constructorArgs list of desired constructor arguments
+     * @return object array
+     * @throws DBusException on failure
+     */
+    public Object[] getParameters(List<Type[]> _constructorArgs) throws DBusException {
+        if (null == args && body != null) {
+            args = extractArgs(_constructorArgs);
         }
         return args;
     }
 
+    /**
+     * Creates a object array containing all objects which should be used to call a constructor.
+     *
+     * @param _constructorArgs list of desired constructor arguments
+     * @return object array
+     * @throws DBusException on failure
+     */
+    private Object[] extractArgs(List<Type[]> _constructorArgs) throws DBusException {
+        String sig = getSig();
+
+        ExtractOptions options = DEFAULT_OPTIONS;
+        if (_constructorArgs != null && !_constructorArgs.isEmpty()) {
+            List<Type> dataType = new ArrayList<>();
+            Marshalling.getJavaType(getSig(), dataType, -1);
+            options = new ExtractOptions(DEFAULT_OPTIONS.contained(), usesPrimitives(_constructorArgs, dataType));
+        }
+
+        if (sig != null && body != null && body.length != 0) {
+            return extract(sig, body, 0, options);
+        }
+        return new Object[0];
+    }
+
+    /**
+     * Compares a list of Type[] with a list of desired types.
+     * This is used to decide if an array of primitive types,
+     * an array of object types or a Collection/List of a object type is used in the constructor.
+     *
+     * @param _constructorArgs list of constructor types to check
+     * @param _dataType list of desired types
+     *
+     * @return List of ConstructorArgType
+     */
+    static List<ConstructorArgType> usesPrimitives(List<Type[]> _constructorArgs, List<Type> _dataType) {
+        OUTER: for (Type[] ptype : _constructorArgs) {
+            if (ptype.length == _dataType.size()) {
+                List<ConstructorArgType> argTypes = new ArrayList<>();
+
+                for (int i = 0; i < ptype.length; i++) {
+                    // this is a list type and an array should be used
+                    if (ptype[i] instanceof Class<?> clz && clz.isArray()
+                            && _dataType.get(i) instanceof ParameterizedType pt
+                            && pt.getRawType() == List.class
+                            && pt.getActualTypeArguments() != null
+                            && pt.getActualTypeArguments().length > 0
+                            && pt.getActualTypeArguments()[0] instanceof Class<?> tClz) {
+
+                        argTypes.add(tClz.isPrimitive() ? ConstructorArgType.PRIMITIVE_ARRAY : ConstructorArgType.ARRAY);
+                    } else if (ptype[i] instanceof Class<?> clz
+                        && _dataType.get(i) instanceof ParameterizedType pt
+                        && clz == pt.getRawType()
+                        && Collection.class.isAssignableFrom(clz)) {
+
+                        // the constructor wants some sort of collection
+                        argTypes.add(ConstructorArgType.COLLECTION);
+
+                    } else if (ptype[i] instanceof Class<?> clz
+                        && _dataType.get(i) instanceof Class<?> tClz
+                        && clz != tClz) {
+                        // constructor class type does not match, must be wrong constructor, try next
+                        continue OUTER;
+                    } else {
+                        // not a list/array and type matches, no conversion needed
+                        argTypes.add(ConstructorArgType.NOT_ARRAY_TYPE);
+                    }
+                }
+                return argTypes;
+            }
+        }
+        return List.of();
+    }
+
     public void setArgs(Object[] _args) {
         this.args = _args;
     }
@@ -1593,19 +1677,21 @@ Object extractOne(byte[] _signatureBuf, byte[] _dataBuf, int[] _offsets, Extract
 
     /**
      * Additional options to optimize value extraction.
-     *
+     * @param contained boolean to indicate if nested lists should be resolved (false usually)
+     * @param arrayConvert use Collection, array or array of primitives
      * @since 5.1.0 - 2024-05-18
      */
     record ExtractOptions(
-        /** boolean to indicate if nested lists should be resolved (false usually). */
         boolean contained,
-        /** Convert List of Object-Wrapper types to primitive arrays (e.g. List&lt;Integer> to int[]). */
-        boolean primitiveListToArray
+        List<ConstructorArgType> arrayConvert
         ) {
 
         static ExtractOptions copyWithContainedFlag(ExtractOptions _toCopy, boolean _containedFlag) {
-            return new ExtractOptions(_containedFlag, _toCopy.primitiveListToArray());
+            return new ExtractOptions(_containedFlag, _toCopy.arrayConvert());
         }
     }
 
+    enum ConstructorArgType {
+        PRIMITIVE_ARRAY, ARRAY, COLLECTION, NOT_ARRAY_TYPE;
+    }
 }
diff --git a/dbus-java-tests/src/test/java/org/freedesktop/dbus/messages/MessageTest.java b/dbus-java-tests/src/test/java/org/freedesktop/dbus/messages/MessageTest.java
@@ -1,10 +1,16 @@
 package org.freedesktop.dbus.messages;
 
+import org.freedesktop.dbus.messages.Message.ConstructorArgType;
 import org.freedesktop.dbus.test.AbstractBaseTest;
+import org.freedesktop.dbus.types.DBusListType;
 import org.freedesktop.dbus.types.UInt32;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
 
+import java.lang.reflect.Type;
 import java.util.List;
+import java.util.stream.Stream;
 
 public class MessageTest extends AbstractBaseTest {
 
@@ -49,4 +55,45 @@ public void testReadMessageHeader() throws Exception {
 
     }
 
+    static Stream<ParameterData> parameterSource() {
+        return Stream.of(
+            new ParameterData("Byte array constructor",
+                List.of(new Type[] {Byte[].class, String.class}, new Type[] {Integer.class, String.class}),
+                List.of(new DBusListType(Byte.class), String.class),
+                List.of(ConstructorArgType.ARRAY,  ConstructorArgType.NOT_ARRAY_TYPE)),
+            new ParameterData("Primitive Byte array constructor",
+                List.of(new Type[] {byte[].class, String.class}, new Type[] {Integer.class, String.class}),
+                List.of(new DBusListType(byte.class), String.class),
+                List.of(ConstructorArgType.PRIMITIVE_ARRAY,  ConstructorArgType.NOT_ARRAY_TYPE)),
+            new ParameterData("Byte array and List of Array constructor",
+                List.of(new Type[] {byte[].class, String.class}, new Type[] {new DBusListType(Byte.class), String.class}),
+                List.of(new DBusListType(byte.class), String.class),
+                List.of(ConstructorArgType.PRIMITIVE_ARRAY,  ConstructorArgType.NOT_ARRAY_TYPE)), // if both variations are present, the first matching will be used
+            new ParameterData("Byte array and different second argument",
+                List.of(new Type[] {byte[].class, long.class}, new Type[] {byte[].class, String.class}),
+                List.of(new DBusListType(byte.class), String.class),
+                List.of(ConstructorArgType.PRIMITIVE_ARRAY, ConstructorArgType.NOT_ARRAY_TYPE)), // if both variations are present, the first matching will be used
+            new ParameterData("Byte List constructor",
+                List.of(new Type[] {List.class, int.class}, new Type[] {Long.class}),
+                List.of(new DBusListType(Byte.class), int.class),
+                List.of(ConstructorArgType.COLLECTION,  ConstructorArgType.NOT_ARRAY_TYPE)),
+            new ParameterData("No arrays in constructor",
+                List.of(new Type[] {Integer.class, String.class}, new Type[] {Integer.class, Integer.class}),
+                List.of(String.class, String.class, Integer.class),
+                List.of())
+            );
+    }
+
+    @ParameterizedTest(name = "{0}")
+    @MethodSource("parameterSource")
+    void testExtractParameter(ParameterData _data) {
+         assertEquals(_data.expected(), Message.usesPrimitives(_data.constructorArgs(), _data.wanted()));
+    }
+
+    record ParameterData(String name, List<Type[]> constructorArgs, List<Type> wanted, List<ConstructorArgType> expected) {
+        @Override
+        public String toString() {
+            return name;
+        }
+    }
 }