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.

Sign up for GitHub

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

Jump to bottom

Documentation or nullability annotation of FirebaseDynamicLinks.getDynamicLink() intent parameter wrong #2336

Closed
ubuntudroid opened this issue Jan 18, 2021 · 7 comments
Closed

Documentation or nullability annotation of FirebaseDynamicLinks.getDynamicLink() intent parameter wrong #2336

ubuntudroid opened this issue Jan 18, 2021 · 7 comments
Assignees
@eldhosembabu
Labels
api: dynamiclinks needs-attention

Comments

@ubuntudroid
Copy link

The documentation of

firebase-android-sdk/firebase-dynamic-links/src/main/java/com/google/firebase/dynamiclinks/FirebaseDynamicLinks.java

Line 102 in 6ccc9de

public abstract Task<PendingDynamicLinkData> getDynamicLink(@NonNull Intent intent);

states, that the intent parameter

can be null if the intent does not include the dynamic link. A non-null intent is necessary only when the app is launched directly using the dynamic link, such as when using App Links.

However the parameter is annotated with @NonNull. So either the documentation or the annotation is wrong and needs adaptation.
@google-oss-bot
Copy link
Contributor

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

@rlazo
Copy link
Collaborator

rlazo commented Jan 18, 2021

Hi @ubuntudroid, thanks for your report! The documentation is wrong, since the method implementing that interface does in fact expect the value to be non-null

firebase-android-sdk/firebase-dynamic-links/src/main/java/com/google/firebase/dynamiclinks/internal/FirebaseDynamicLinksImpl.java

Line 95 in 6ccc9de

public Task<PendingDynamicLinkData> getDynamicLink(@NonNull final Intent intent) {

I'll follow up here when we have a fix ready.

@rohandandavati
Copy link

b/178000308

@davidmigloz
Copy link
Contributor

What is the expected usage when you only care about the captured dynamic link after install? (e.g. because you have your own deep linking mechanism to handle app links that doesn't rely on Firebase Dynamic Links)

It'd good to have a getDynamicLink() without any parameters for that case (or allow to pass null as the documentation was suggesting before).

@eldhosembabu eldhosembabu mentioned this issue Apr 28, 2021
Merged
@eldhosembabu
Copy link
Contributor

Added a fix here: #2629

@mikehardy
Copy link

This just bit us on react-native-firebase. We were coded up to wrap the API per docs, but a user took a null pointer exception because the implementation expects non-null

invertase/react-native-firebase#5662

@mikehardy mikehardy mentioned this issue Aug 30, 2021
Merged
8 tasks
mikehardy added a commit to mikehardy/flutterfire that referenced this issue Aug 30, 2021
@mikehardy
fix(dynamic-links, android): null-check intent before calling getDyna…
6f049a4 
…micLink

firebase-android-sdk requires a `@NonNull` Intent argument to the `getDynamicLink(Intent)`
call, despite the documentation indicating otherwise

Related firebase/firebase-android-sdk#2336
Related invertase/react-native-firebase#5662
@mikehardy mikehardy mentioned this issue Aug 30, 2021
Merged
10 tasks
@eldhosembabu
Copy link
Contributor

#2629 is merged. Expect the fix to be rolled out in upcoming releases. So closing this issue now.

@firebase firebase locked and limited conversation to collaborators Oct 24, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@eldhosembabu eldhosembabu

Labels
api: dynamiclinks needs-attention
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@rlazo @ubuntudroid @mikehardy @eldhosembabu @davidmigloz @google-oss-bot @rohandandavati