Instantiating Void does no longer work with AGP 8

[stefan-niedermann]

We implemented a hacky way to instantiate Void using reflection. This does no longer work with new Java versions and encumbers users while upgrading to AGP 8 and newer Java versions.

Why did we implement this at all? See comment, TLDR; Using REST endpoints that return an empty body as Observable<Void> leads to a crash. (See #231).

CC @desperateCoder @David-Development @AndyScherzinger @tobiasKaminsky

[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

[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!

[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 ➡️ chore: AGP 8 and Java 17 #629
• Optionally remove all deprecations because we have a breaking change anyway chore: Remove deprecations #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

[AndyScherzinger]

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

[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

[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