Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
AsamK opened this issue Sep 8, 2024 · 9 comments
Closed

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

AsamK opened this issue Sep 8, 2024 · 9 comments

Comments

@AsamK
Copy link
Contributor

AsamK commented Sep 8, 2024

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
Copy link
Owner

hypfvieh commented Sep 8, 2024

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
Copy link
Contributor Author

AsamK commented Sep 11, 2024

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
Copy link
Owner

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 added a commit that referenced this issue Sep 14, 2024
…imitive array or Collections in constructor
@hypfvieh
Copy link
Owner

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
Copy link
Contributor Author

AsamK commented Sep 15, 2024

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 added a commit that referenced this issue Sep 15, 2024
- Improved detection of constructors when compatible types are used
  instead of equal types (e.g. CharSequence and String)
- Improved handling of primitive and wrapper array type (int[] /
  Integer[])
@hypfvieh
Copy link
Owner

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
Copy link
Contributor Author

AsamK commented Sep 16, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

AsamK commented Sep 16, 2024

Ok, then I'll adapt my code.

@AsamK AsamK closed this as completed Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants