[Bug] [Dynamic Links] Dynamic Links callback in Kotlin incorrectly marked as non-null

asos-edgeorge commented 3 years ago

[READ] Step 1: Are you in the right place?

Issues filed here should be about bugs in the code in this repository. If you have a general question, need help debugging, or fall into some other category use one of these other channels:

For general technical questions, post a question on StackOverflow with the firebase tag.
For general Firebase discussion, use the firebase-talk google group.
For help troubleshooting your application that does not fall under one of the above categories, reach out to the personalized Firebase support channel.

[REQUIRED] Step 2: Describe your environment

Android Studio version: Arctic Fox 2020.3.1 patch 2
Firebase Component: Dynamic Links KTX
Component version: 20.1.1

[REQUIRED] Step 3: Describe the problem

When running the sample code for Dynamic Links in Kotlin, the result of the addOnSuccessListener is not correctly marked as a nullable type, despite it being possible to return as null.

Steps to reproduce:

Follow the sample code setup in Kotlin
Call the sample code with an intent that doesn't return pendingDynamicLinkData
Observe that null is returned for the non-nullable type param of the success callback

Relevant Code:

Firebase.dynamicLinks
        .getDynamicLink(intent)
        .addOnSuccessListener(this) { pendingDynamicLinkData ->

            // **BUG** Android Studio reports this check to always be true
            //  as the typing of pendingDynamicLinkData is not nullable
            if (pendingDynamicLinkData != null) {
               // some code here ...
            }
        }
        .addOnFailureListener(this) { e -> Log.w(TAG, "getDynamicLink:onFailure", e) }

google-oss-bot commented 3 years ago

I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.

asos-edgeorge commented 3 years ago

Just to help visualize what I am seeing here

aguatno commented 3 years ago

Hi @asos-edgeorge thanks for reporting. I tried to reproduce the issue, but I wasn't able to get the same stack trace. It would be really helpful if you could get us some minimum viable steps to reproduce on a quickstart or share a runnable app that demonstrates the issue thanks.

asos-edgeorge commented 3 years ago

@aguatno Hi, is observing the typing of pendingDynamicLinkData not enough? In Kotlin, the IDE shows it as a non-nullable type but both the Firebase Dynamic Links integration guide and my example image show it being nullable.

asos-edgeorge commented 3 years ago

For steps to reproduce receiving null for the pendingDynamicLinkData, I noticed that calling this method on a sample app where no signing certificate SHA-1 was present in Firebase for the given package name would return null.

atulgpt commented 3 years ago

Hi @aguatno adding more to what @asos-edgeorge has provided, In my case when I updated the play store dependency, Task class from package com.google.android.gms.tasks is now marked with @RecentlyNonNull and @RecentlyNullable annotations. So the API is defined in the below manner

@NonNull
public abstract Task<TResult> addOnSuccessListener(@RecentlyNonNull OnSuccessListener<? super TResult> var1);

and OnSuccessListener interface is defned a below:

public interface OnSuccessListener<TResult> {
    void onSuccess(@RecentlyNonNull TResult var1);
}

as you can see that now OnSuccessListener returns result TResult var1 which is marked @RecentlyNonNull and hence linter gives warning as shown in the ScreenShot attached by the @asos-edgeorge but in reality Firebase.dynamicLinks.getDynamicLink(intent) may return null even after the success that's is why app gets crashed in rutime

The ideal fix would not return null when the operation is considered to be a success

argzdev commented 3 years ago

Hi @asos-edgeorge and @atulgpt, thanks for the information.

It looks like with or without the SHA-1, the pendingDynamicLinkData will still be received with values. However, I was able to reproduce the issue by just opening the app. Since opening the app has not received any intent, so it will run the code with a null and as you've mentioned that null is returned for the non-nullable type param of the success callback.

With that said, if I understand correctly, you'd like to have the annotation changed from @NonNull to @Nullable to avoid incorrect assumptions right?

atulgpt commented 3 years ago

@argzdev Actually thing is that it uses Tasks api which is coming from Google play services and I am not sure if the annotation of the task api coming from some other module can be changed by the Firebase modules

argzdev commented 3 years ago

@atulgpt good catch. Since the annotations can't be changed, then the ideal fix would be the one you've mentioned. However, I'm thinking this might introduce new issues such that it will always trigger addOnFailureListener(assuming that null values will trigger onFailureListener instead of onSuccessListener) when opening the app normally everytime and not from a dynamic link.

I think this type of change may need further discussions. Is this the type of flow you'd like to see?

atulgpt commented 3 years ago

@argzdev This change is not something we can do it in one go. But what matters is that we keep this issue on our radar and plan to fix it in a proper manner(keeping it backwards compatible).

Regarding your question, actually, it should not come in addOnFailureListener as the absence of dynamic link data is not actually a failure case hence should not come there. We might have the change the output of onSuccess that also captures(model) absent dynamic data without being null(But not sure are we okay with not going with backwards compatible way)

argzdev commented 3 years ago

Hi @atulgpt, thanks for the input, this is very insightful. With that said, I'll leave this as a feature request, so that our engineers can have a look and discuss this further for future plans.

kazougagh commented 2 years ago

Hi @atulgpt @argzdev , I've updated my project dependencies to the latest version and now my app crash because the addOnSuccessListener callback retuns null value for pendingDynamicLinkData. Do you have a workaround that we can use while waiting for the final fix ? The signing certificate SHA-1 is present in Firebase.

java.lang.NullPointerException: Parameter specified as non-null is null: method kotlin.jvm.internal.Intrinsics.checkNotNullParameter, parameter pendingDynamicLinkData at com.pemPem.MainActivity$handleDynamicsLink$1.onSuccess(Unknown Source:2)

Thanks for you response. Kamal

argzdev commented 2 years ago

Hi @kazougagh, do you mean to say this worked on previous versions, may I ask which version was it initially working so I can investigate. In the meantime, if you follow the our quickstart guide.

Code snippet from quickstart:

.addOnSuccessListener(this) { pendingDynamicLinkData ->
                    // Get deep link from result (may be null if no link is found)
                    var deepLink: Uri? = null
                    if (pendingDynamicLinkData != null) {
                        deepLink = pendingDynamicLinkData.link
                    }

                    // Handle the deep link. For example, open the linked
                    // content, or apply promotional credit to the user's
                    // account.
                    // ...

                    // [START_EXCLUDE]
                    // Display deep link in the UI
                    if (deepLink != null) {
                        Snackbar.make(findViewById(android.R.id.content),
                                "Found deep link!", Snackbar.LENGTH_LONG).show()

                        linkReceiveTextView.text = deepLink.toString()
                    } else {
                        Log.d(TAG, "getDynamicLink: no link found")
                    }
                    // [END_EXCLUDE]
                }

You can see that there are null checkers to avoid this crashing.

kazougagh commented 2 years ago

Hi @argzdev Yes, I have the exact same code as yours. It was working fine before my update. Just a warning saying

Condition 'pendingDynamicLinkData != null' is always 'true'

But it was not always true.

I have updated this dependencies implementation 'com.google.android.gms:play-services-auth:19.2.0' to implementation 'com.google.android.gms:play-services-auth:20.0.0'

and I have noticed that the callback change from @RecentlyNonNull to @NonNull. @NonNull public abstract Task<TResult> addOnSuccessListener(@RecentlyNonNull Activity var1, @RecentlyNonNull OnSuccessListener<? super TResult> var2);

to

@NonNull public abstract Task<TResult> addOnSuccessListener(@NonNull Activity var1, @NonNull OnSuccessListener<? super TResult> var2);

And now the app crashes everytime the callback addOnSuccessListener is called with a null value. It crashes before the null checkers is reached.

Let me know if you need more information.

argzdev commented 2 years ago

Thanks for explaining @kazougagh, may I know what steps did you do to reproduce the issue? Or if you could provide an MCVE, that'll be very helpful for us to investigate this.

kazougagh commented 2 years ago

Here is my code @argzdev

Gradle files

// Top-level build file where you can add configuration options common to all sub-projects/modules.
buildscript {

    repositories {
        google()
        jcenter()
    }

    dependencies {
        classpath("com.android.tools.build:gradle:7.0.4")
        classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:1.5.20"
        classpath 'com.google.gms:google-services:4.3.10'
        classpath 'com.google.firebase:firebase-crashlytics-gradle:2.8.1'
        classpath 'com.google.firebase:perf-plugin:1.4.0'
    }
}

 // App-level build file
android {
     compileSdkVersion 32

     defaultConfig {
        applicationId "com.xxx"
        minSdkVersion 21
        targetSdkVersion 32`
      ...
   }
}

dependencies {

  // Google/Android
    implementation 'com.google.android.gms:play-services-auth:20.0.0'
   ...

   // Firebase
    implementation platform('com.google.firebase:firebase-bom:29.0.1')
    implementation 'com.google.firebase:firebase-dynamic-links-ktx'
    ...
}

Code

private fun handleDynamicsLink() {
        Firebase.dynamicLinks
            .getDynamicLink(intent)
            .addOnSuccessListener(this) { pendingDynamicLinkData ->
                // Get deep link from result (may be null if no link is found)
                var deepLink: Uri? = null
                if (pendingDynamicLinkData != null) {
                    deepLink = pendingDynamicLinkData.link
                }
                if (deepLink != null) {
                    handleDeepLink(pendingDynamicLinkData.link!!)
                }
            }
            .addOnFailureListener(this) { e -> PemLog.e("MainActivity - getDynamicLink:onFailure", e) }
    }

Stacktrace each time I launch the app

FATAL EXCEPTION: main
    Process: com.xxx.qa, PID: 3108
    java.lang.NullPointerException: Parameter specified as non-null is null: method kotlin.jvm.internal.Intrinsics.checkNotNullParameter, parameter pendingDynamicLinkData
        at com.xxx.MainActivity.handleDynamicsLink$lambda-10(Unknown Source:7)
        at com.xxx.MainActivity.$r8$lambda$_pMx9pt1BPlrpOlBFCV6GWRiM4Y(Unknown Source:0)
        at com.xxx.MainActivity$$ExternalSyntheticLambda5.onSuccess(Unknown Source:4)
        at com.google.android.gms.tasks.zzm.run(com.google.android.gms:play-services-tasks@@18.0.0:1)
        at android.os.Handler.handleCallback(Handler.java:790)
        at android.os.Handler.dispatchMessage(Handler.java:99)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:6977)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:528)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:910)

Note that it crashes before I receive the null value. I'll try to give you a MCVE as soon as I can. Kamal

atulgpt commented 2 years ago

Hi, @kazougagh is somehow you are making null type into not null type as Kotlin assert that parameter should not be null when type parameter is not-nullable. Can you modify the code as below to see if it still crashes or not

private fun handleDynamicsLink() {
        Firebase.dynamicLinks
            .getDynamicLink(intent)
            .addOnSuccessListener(this) { pendingDynamicLinkData: PendingDynamicLinkData? ->
                // Get deep link from result (may be null if no link is found)
                var deepLink: Uri? = null
                if (pendingDynamicLinkData != null) {
                    deepLink = pendingDynamicLinkData.link
                }
                if (deepLink != null) {
                    handleDeepLink(pendingDynamicLinkData.link!!)
                }
            }
            .addOnFailureListener(this) { e -> PemLog.e("MainActivity - getDynamicLink:onFailure", e) }
    }

kazougagh commented 2 years ago

@atulgpt It stops crashing indeed.

But I think the PendingDynamicLinkData should be @Nullable in the first place.

Thanks for the support !
Kamal

madhall commented 2 years ago

Hi, I started facing the same issue on updating the dependencies, I am guessing a whole lot of people are looking for a solution or will be looking for a solution soon....

@atulgpt your solution works, changing pendingDynamicLinkData to pendingDynamicLinkData: PendingDynamicLinkData?

The Firebase guides (https://firebase.google.com/docs/dynamic-links/android/receive) here need to be updated... will try to figure out how to request a change! cheers

Naoki-png commented 2 years ago

You can solve this problem by adding these dependencies.

implementation 'com.google.android.gms:play-services-base:18.0.1'
implementation 'com.google.android.gms:play-services-tasks:18.0.1'

https://developers.google.com/android/guides/releases#december_16_2021

thatfiredev commented 2 years ago

Thanks @atulgpt for posting a workaround and thanks @madhall for suggesting adding it to our guides. Our doc page(https://firebase.google.com/docs/dynamic-links/android/receive) has been updated to include that.

argzdev commented 1 year ago

Hi folks, we'd like to inform you that the Firebase Dynamic Links service will be shutdown on August 25, 2025. In the meantime, only critical or security issues will be fixed in the SDK.

More at https://firebase.google.com/support/dynamic-links-faq

firebase / firebase-android-sdk