hypfvieh / dbus-java

Improved version of java DBus library provided by freedesktop.org (https://dbus.freedesktop.org/doc/dbus-java/)
https://hypfvieh.github.io/dbus-java/
MIT License
185 stars 73 forks source link

Receving signals doesn't work after upgrading to 5.1.0 #268

Closed AsamK closed 1 month ago

AsamK commented 1 month ago

After upgrading to dbus-java 5.1.0 (from 5.0.0) receiving signals doesn't work anymore. I didn't see anything related in the changelog.

Could not find suitable constructor for class org.asamk.Signal$MessageReceivedV2 with argument-types: [class java.lang.Long, class java.lang.String, class java.util.ArrayList, class java.lang.String, class java.util.LinkedHashMap]

The constructor is defined here (which worked until 5.0.0): https://github.com/AsamK/signal-cli/blob/master/src/main/java/org/asamk/Signal.java#L201 public MessageReceivedV2( String objectpath, long timestamp, String sender, byte[] groupId, String message, final Map<String, Variant<?>> extras ) throws DBusException

I tried removing the first objectpath path parameter and changing the byte[] to a List<Byte>, but that didn't work either.

Do I need to adapt anything in my code, or is this a bug?

hypfvieh commented 1 month ago

No bug I am aware of. The code responsible for finding the constructor is part of DBusSignal class which hasn't changed much between 5.0.0 and 5.1.0 (just one line:, a stream .collect(toList()) was changed to .toList(). The list is only used for caching constructor arguments and should not change the behavior at all).

Do you have a test or sample setup I can use to reproduce this problem?

AsamK commented 1 month ago

I've created a small sample application here: https://github.com/AsamK/dbus-java-test-case Can be run with ./gradlew run

By default it uses dbus-java 5.0.0 and successfully prints Received: message. But when changing the version to 5.1.0 (in gradle/libs.versions.toml) it prints [DBus-Signal-Receiver-1] WARN org.freedesktop.dbus.messages.DBusSignal - Could not find suitable constructor for class org.example.Signal$MessageReceivedV2 with argument-types: [class java.lang.Long, class java.lang.String, class java.util.ArrayList, class java.lang.String, class java.util.LinkedHashMap]

hypfvieh commented 1 month ago

Thanks a lot. I'll already started digging into this. It pretty much looks like that the changes required to fix the Variant handling has some side effects on other operations as well.

Right now, the method which converts the DBus signature to constructor argument types generates the wrong type. The signature in this example is xsaysa{sv}, where ay symbols the byte[] used in constructor.

With the changes for Variant ay is not translated to byte[] but to ArrayList.

Converting primitives to wrapper types is required for Variant because primitives cannot be used as generic types. However, in this case it is not the expected result.

The problem I have to solve now is to find a way to detect which kind of type should be used. Always using the array type can be wrong as well in case the constructor wants to have a List (or Collection).

I'll investigate this further and will report back when I have a solution.

hypfvieh commented 1 month ago

I just pushed changes which should fix this issue, patched snapshot version should be available soon. Would be great if you can try that one before I'll create a new release.

The changes for this issue are a bit larger than expected. But I think the handling of signal constructors in combination with array, array of primitives or Collection types should now be working in all flavors.

It was required to investigate the constructor arguments in more detail, so that the library is able to convert a signature like ai (array of int) to int[], Integer[] or Collection<Integer> depending on constructor signature.

When there are multiple constructors matching the this signature (e.g. you have int[] foo, String bar and List<Integer> foo, String bar), the first constructor found when investigating the class is used (constructors are usually found in order of appearance in code) . I added some documentation to explain that in more detail (see using-signals.md)

AsamK commented 1 month ago

Thanks, I just tried it and it still doesn't work. I've updated the test repo to use 5.1.1-SNAPSHOT and see the same Could not find suitable constructor error.

hypfvieh commented 1 month ago

Sorry... I was so focused on getting every corner case right that I missed the main goal. I revisited my changes and fixed some issues. In my tests with your sample code everything is working now. I also updated my unit test to match your sample code so I can assure that this behavior will not break in the future. Updated Snapshot should be available soon.

AsamK commented 1 month ago

Thanks, that works now as expected.

I found another difference between 5.0.0 and 5.1.0 with generic methods that return a byte[]:

Exception in thread "main" java.lang.ClassCastException: class java.util.ArrayList cannot be cast to class [B (java.util.ArrayList and [B are in module java.base of loader 'bootstrap')

I've updated the sample repo with new methods. Methods that have a return type of byte[] still work, but methods with <A> A or Variant<?> don't work anymore. Looks like they default to using ArrayList now instead of plain arrays.

hypfvieh commented 1 month ago

I'm afraid, but this is the expected behavior since 5.1.0 when using Variant. This is also documented in the README and documentation and also this issue

AsamK commented 1 month ago

Ok, then I'll adapt my code.