Instantiating `Void` does no longer work with AGP 8

[stefan-niedermann]

Instantiating `Void` does no longer work with AGP 8

[stefan-niedermann]

Awesome, thanks!

Here my proposal for the next steps:

• I'll do a more up to date variant of your PR for AGP 8 ➡️ #629
• Optionally remove all deprecations because we have a breaking change anyway #560
• After AGP 8 is in master, we need a new release
• I'll then update my libs to use the new SSO version
• I'll prepare a PR for the Notes Android App to update all dependencies

[stefan-niedermann]

@AndyScherzinger absolutely! #629 is from today and that's what I meant with "I'll do a more up to date variant" 😄

However, I definitely wanted to get Java 17 in, too (it's really about time). But the Drone testing environment threw an error because it runs on Java 11.

Any help is highly appreciated as I don't quite understand the ghcr.io/nextcloud/continuous-integration-androidandroidbase image being used here (tag 3 seems to be more current than tag 5??), but let's discuss that in #629

[stefan-niedermann]

One possible solution I could imagine is to provide a new type like EmptyResponse from the SSO lib, which could be used in the 3rd party apps to declare this kind of type. Of course it would need to be documented to use this type instead of Void...

[stefan-niedermann]

Reminder that updating ProGuard rules might be necessary after fixing this: https://github.com/nextcloud/Android-SingleSignOn/pull/545/files

[stefan-niedermann]

FYI: I am currently trying to make the problem reproducible in the sample app on the agp-8 branch, but it still works there... 🤔

[AndyScherzinger]

Are all parts compiling targeting java17?

[stefan-niedermann]

Hehe, Unit tests fail on agp-8, the sample app works - I can only assume that the Java runtime for Android does not (yet) ship the protected modules. So: It is reproducible now with the agp-8 branch when running the unit tests.

Edit:

I can only assume that the Java runtime for Android does not (yet) ship the protected modules.

This was wrong. Even though the Void instance is initialized static according to the code, it will actually get instantiated when it is read the first time. So the exception is only getting thrown when Void is actually used.

[AndyScherzinger]

@stefan-niedermann looks good to me. How would I need to test this?

[stefan-niedermann]

A bit complicated! My use case is in the Deck Android app (we need REST endpoints with no return body), but it uses SSO as direct and transitive dependency.

Maybe utilizing the SSO sample app and try to request a Deck endpoint with Observable and / or Call response types?

As you see, this PR was coded blindly, testing is quite hard and help is appreciated.

[AndyScherzinger]

Are you aware of any of those used by the notes app maybe? Than I could give them a try

[stefan-niedermann]

@AndyScherzinger please do not hesitate to call me as soon as possible if you have any questions, because I can not guarantee that I continue to understand this complex topic if we don't care about it for another year 😅😅 I have a guest account on the talk instance on cloud.nextcloud.com which we can use.

I have finally been able to understand and reliable reproduce the original issue:

1. Checkout void-reproducible-error branch of this repository (which is not meant to be merged but exists only for demonstration purposes!)
2. See MainActivity and search there for // FIXME VOID:
3. Follow the steps

You will see, that one and the same request will be successfull when using Call and will fail when using Observable.

This was the reason, we introduced the Void reflection workaround which hurts us today with higher Gradle versions and target Sdks and all because recent Java versions with module system do not allow reflective access to Void anymore.

Understanding this, we need to get rid of the Void workaround.

Here we have two options:

1. Accept and document that void responses do not work with Observable but only with Call or
2. Find an alternative for Void: For this reason I blindly implemented the EmptyResponse branch. Blindly because I haven't been able to reproduce this until now

The first solution is okay for me, because we are currently migrating Deck Android from Observable to Call anyway. The second solution is also okay for me, but we have to test it the same way as in the void-reproducible-error branch to ensure it actually works. I have merged void-reproducible-error and 541-empty-responses (PR) into void-reproducible-error-with-empty-responses (not meant to be merged!) to prove that the solution works and yes. It works.

[AndyScherzinger]

I'd have the tendency to say we should go for Option 1 but would leave the final decision and say to you (being the most prominent consumer of the API) and @tobiasKaminsky being in the lead of another app affected by this (notes client)

[stefan-niedermann]

Thank you for your feedback!

Personally I would recommend Option 2 because

• the solution for Observable requests is there (PR) and works (just needs to get rebased and merged)
• Option 1 would require additional work (namely preparing a PR which removes the reflective Void access and updates the documentation - but I can do that, too)
• Calls are not affected by the EmptyResponse solution and can still use Void (or EmptyResponse) if wanted

Both options are valid though and both are a breaking change.

@tobiasKaminsky looking forward to a decision so I can clean up all the branches and finally fix this issue 🚀 Of course I can also offer you a call to explain the topic before if you want 🙂

[AndyScherzinger]

Well, if you put it this way, than option 2 seems to be favorable since it is more flexible in terms of devs having more of a choice on how to implement it - while yeah this can be seen as a good or bad thing.

[tobiasKaminsky]

I also like Option 2, as then there is still a breaking change to used EmptyResponse instead of Void, but all other stays the same.
With Option 1 we might kill other apps that rely on a "void" mechanism.

[tobiasKaminsky]

Thanks for this great work!

[AndyScherzinger]

Shouldn't we go straight for #629 @stefan-niedermann ?

[AndyScherzinger]

Talk Android runs on Java17 already -> https://github.com/nextcloud/talk-android/blob/master/.drone.yml

But apart from that @tobiasKaminsky knows the drone containers best