Android-SingleSignOn icon indicating copy to clipboard operation
Android-SingleSignOn copied to clipboard

Kotlin coroutine support missing

Open dave-kennedy opened this issue 5 years ago • 15 comments

I always get an exception on line 124 of NextcloudRetrofitServiceMethod.invoke():

        // Build/parse dynamic parameters
        for(int i = 0; i < parameterAnnotationsArray.length; i++) {
            Annotation annotation = parameterAnnotationsArray[i][0]; // <----- Blows up

I tried debugging this method, but every time it hits a breakpoint in here the app crashes.

Here's my Retrofit:

interface BookmarkService {

    @GET("bookmark")
    suspend fun getBookmarks(
        @Query("folder") folderId: Int,
        @Query("page") page: Int,
        @Query("tags") tags: List<String>
    ): DtoBookmarks

    @GET("folder?layers=1")
    suspend fun getFolders(
        @Query("root") parentFolderId: Int
    ): DtoFolders
}

These are my dependencies:

    implementation 'com.github.nextcloud:Android-SingleSignOn:0.5.0-rc4'
    implementation 'com.squareup.retrofit2:retrofit:2.6.4'
    implementation 'com.squareup.retrofit2:converter-gson:2.6.4'

Let me know if you need any more info.

dave-kennedy avatar Feb 23 '20 18:02 dave-kennedy

@dave-kennedy Thanks for your report. Do you happen to have your App somewhere on GitHub where we can test this issue?

Do you have a crashlog for us?

David-Development avatar Feb 23 '20 19:02 David-Development

@David-Development I sent you an invite on GitLab.

dave-kennedy avatar Feb 23 '20 20:02 dave-kennedy

Just out of curiousity: Which app do you maintain @dave-kennedy ? NCBookmarks? NC Bookmarks Viewer? Is it open source?

Sorry for off topic 😉

stefan-niedermann avatar Feb 25 '20 22:02 stefan-niedermann

@stefan-niedermann I wasn't really happy with the existing apps and started my own. When I feel it's ready I'll open it up to the public.

dave-kennedy avatar Feb 26 '20 05:02 dave-kennedy

@dave-kennedy Thank you for the invitation and sorry for the delay. Looks like I don't have permission to see the code 🤔

Bildschirmfoto 2020-02-26 um 20 16 43

David-Development avatar Feb 26 '20 19:02 David-Development

Sorry about that. I'm still learning how GitLab works. You should be good to go.

dave-kennedy avatar Feb 27 '20 02:02 dave-kennedy

No worries :) I finally managed to take a look into it and it looks like that the getParameterAnnotations () method returns 4 annotations instead of the 3 that are expected. And the last one is empty (see screenshot below). https://github.com/nextcloud/Android-SingleSignOn/blob/master/src/main/java/com/nextcloud/android/sso/api/NextcloudRetrofitServiceMethod.java#L81

Bildschirmfoto 2020-02-29 um 15 36 06

At this point I believe that it is an issue with Kotlin -> Java.. But it needs some more investigation. @tobiasKaminsky do you have experience with Kotlin -> Java?

David-Development avatar Feb 29 '20 14:02 David-Development

I sent an invite to Tobias as well.

dave-kennedy avatar Feb 29 '20 19:02 dave-kennedy

Okay, I looked into it again and I found out that the fourth parameter is actually from kotlin.. (kotlin.coroutines.continuation)

Bildschirmfoto 2020-03-02 um 18 08 55

Another issue is that we are loosing the return type. While the retrofit api spec has the DtoBookmark type, the sso library only sees the Object type. Not sure if that is a problem.

I managed to remove/filter the fourth empty argument however now I get the following exception:

2020-03-02 18:21:25.579 6937-6978/com.bookmarks E/LoginFragment: Unable to get bookmarks
    java.lang.reflect.UndeclaredThrowableException
        at $Proxy0.getBookmarks(Unknown Source)
        at com.bookmarks.ui.login.LoginFragment.callApi(LoginFragment.kt:85)
        at com.bookmarks.ui.login.LoginFragment$onActivityResult$1$1$1.invokeSuspend(LoginFragment.kt:77)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(Dispatched.kt:241)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:594)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.access$runSafely(CoroutineScheduler.kt:60)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:740)
     Caused by: com.nextcloud.android.sso.exceptions.NextcloudHttpRequestFailedException: HTTP request failed with HTTP status-code: 302
        at com.nextcloud.android.sso.api.AidlNetworkRequest.performNetworkRequest(AidlNetworkRequest.java:202)
        at com.nextcloud.android.sso.api.NextcloudAPI.performNetworkRequest(NextcloudAPI.java:119)
        at com.nextcloud.android.sso.api.NextcloudAPI.performRequest(NextcloudAPI.java:98)
        at com.nextcloud.android.sso.api.NextcloudRetrofitServiceMethod.invoke(NextcloudRetrofitServiceMethod.java:214)
        at retrofit2.NextcloudRetrofitApiBuilder.lambda$create$0$NextcloudRetrofitApiBuilder(NextcloudRetrofitApiBuilder.java:30)
        at retrofit2.-$$Lambda$NextcloudRetrofitApiBuilder$U6rJlQEKMLSg7zB4KqSP_SBh3BM.invoke(Unknown Source:2)
        at java.lang.reflect.Proxy.invoke(Proxy.java:1006)
        at $Proxy0.getBookmarks(Unknown Source) 
        at com.bookmarks.ui.login.LoginFragment.callApi(LoginFragment.kt:85) 
        at com.bookmarks.ui.login.LoginFragment$onActivityResult$1$1$1.invokeSuspend(LoginFragment.kt:77) 
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33) 
        at kotlinx.coroutines.DispatchedTask.run(Dispatched.kt:241) 
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:594) 
        at kotlinx.coroutines.scheduling.CoroutineScheduler.access$runSafely(CoroutineScheduler.kt:60) 
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:740) 
     Caused by: java.lang.IllegalStateException: 
        at com.nextcloud.android.sso.InputStreamBinder.processRequest(InputStreamBinder.java:368)
        at com.nextcloud.android.sso.InputStreamBinder.performNextcloudRequestAndBodyStream(InputStreamBinder.java:175)
        at com.nextcloud.android.sso.InputStreamBinder.performNextcloudRequest(InputStreamBinder.java:153)
        at com.nextcloud.android.sso.aidl.IInputStreamService$Stub.onTransact(IInputStreamService.java:85)
        at android.os.Binder.execTransact(Binder.java:731)

In order to test the current state:

  • Clone SSO Repo into the root of the bookmarks repo (git clone https://github.com/nextcloud/Android-SingleSignOn.git) -> Checkout kotlin-support branch after that.
  • add this line to your settings.gradle: include ':Android-SingleSignOn'
  • add this line to your build.gradle: implementation project(':Android-SingleSignOn') and remove the dependency to the version from jitpack.
  • Open the class NextcloudRetrofitServiceMethod and have fun 😁

David-Development avatar Mar 02 '20 17:03 David-Development

@tobiasKaminsky Do you have any ideas why this is happening?

@dave-kennedy Did you manage to test it with the fix I provided?

David-Development avatar Mar 09 '20 17:03 David-Development

@tobiasKaminsky Do you have any ideas why this is happening?

No idea :/

tobiasKaminsky avatar Mar 10 '20 08:03 tobiasKaminsky

Apparently that continuation parameter is part of how coroutines work. This is an interesting read that explains what's going on: https://blog.kotlin-academy.com/a-little-reflection-about-coroutines-34050cbc4fe6

We might be able to copy how this type of interop is handled by Retrofit: https://github.com/square/retrofit/blob/master/retrofit/src/main/java/retrofit2/RequestFactory.java

All this reflection has my head spinning. I wonder if there isn't a simpler way to wrap requests with the necessary authentication info.

dave-kennedy avatar Mar 16 '20 03:03 dave-kennedy

Awesome, thank you so much for the research! I looked through the retrofit RequestFactory and they're basically doing the same as I'm doing right now.. They check if the last parameter is not set, then they do some type checking (I can't think of another case the last parameter will be null). In order to do the type check, they use the kotlin std libraries which I rather prefer not to include in our project. Other than that, they don't do anything differently after.

The article is very interesting and explains it quite well why the last parameter is added and why the return type is lost.. I think the merge request should be good to go as a fix then.. Or what do you guys think?

David-Development avatar Mar 16 '20 16:03 David-Development

Looks like we're making progress, but I'm getting this error now:

2020-03-25 11:16:20.507 14665-14733/com.bookmarks E/LoginFragment: Unable to get bookmarks
    java.lang.ClassCastException: com.google.gson.internal.LinkedTreeMap cannot be cast to com.bookmarks.data.DtoBookmarks
        at com.bookmarks.ui.login.LoginFragment.callApi(LoginFragment.kt:89)
        at com.bookmarks.ui.login.LoginFragment$onActivityResult$1$1$1.invokeSuspend(LoginFragment.kt:83)
        at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
        at kotlinx.coroutines.DispatchedTask.run(Dispatched.kt:241)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:594)
        at kotlinx.coroutines.scheduling.CoroutineScheduler.access$runSafely(CoroutineScheduler.kt:60)
        at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:740)

Normally, I'd tell Retrofit to use the GSON converter factory like this:

        val service = Retrofit.Builder()
            .addConverterFactory(GsonConverterFactory.create())
            .baseUrl(BuildConfig.API_URL)
            .build()
            .create(BookmarkService::class.java)

However, I don't see a way to do that with this lib and I wonder if that's causing the problem.

dave-kennedy avatar Mar 25 '20 17:03 dave-kennedy

@dave-kennedy Sorry for letting you wait that long. It looks like that problem is caused due to the loss of the return type. Gson is then not able to figure out into which type it is supposed to cast the response. I think we need to look into the retrofit implementation again on how they are able to use suspend in conjunction with the gson type casting.

David-Development avatar Apr 04 '20 09:04 David-Development