airbnb / DeepLinkDispatch

A simple, annotation-based library for making deep link handling better on Android
http://nerds.airbnb.com/deeplinkdispatch/
4.37k stars 407 forks source link

Not supported valid URIs for deeplinks with same path and different number of parameters #303

Closed cvillaseca closed 3 years ago

cvillaseca commented 3 years ago

Hi,

Since we updated the library to version 5.X some deeplinks like myapp://vehicle/{vehicleId} have stopped working. The problem is that with the new version it doesn’t handle multiple links with the same structure, it only handles the first one is created in the registry class. For example:

public final class MyDeepLinkModuleRegistry extends BaseRegistry {
  public MyDeepLinkModuleRegistry() {
    super(Collections.unmodifiableList(Arrays.<DeepLinkEntry>asList(
      new DeepLinkEntry("myapp://vehicle/{make}/{model}", DeepLinkEntry.Type.METHOD, VehicleDetailNavigation.class, "newIntentForTaskStackBuilderMethods"),
      new DeepLinkEntry("myapp://vehicle/{vehicleId}", DeepLinkEntry.Type.METHOD, VehicleDetailNavigation.class, "newIntentForTaskStackBuilderMethods")
    )), Utils.readMatchIndexFromStrings( new String[] {matchIndex0(), }),
    new HashSet<String>(Arrays.<String>asList()));
  }
}

This is considered a valid deeplink

myapp://vehicle/vw/golf

But this deeplink won’t be valid. You can check this by using DeepLinkDelegate.supportsUri("myapp://vehicle/123")

myapp://vehicle/123

This was working in version 4.X but with version 5.X it's not.

Thank you 🙏

rossbacher commented 3 years ago

I'll have a look at that. I don't think we have a test case for this and thus it might have been missed.

stari4ek commented 3 years ago

~There is a chance that incremental build is involved. I had same issue. Bunch of "foo://navigate/*" declared, but only one was added to registry.~

It looks like it swallows errors, so only part of entries are added. And it stays unchanged on next build.

I had:

@AppDeepLink({"tv/{what}"})
public static Intent intentForTv(@NonNull Context context, @NonNull Bundle extras) {

Dropped placeholder from link:

@AppDeepLink({"tv"})
public static Intent intentForTv(@NonNull Context context, @NonNull Bundle extras) {

No errors, but registry got 2 entries only (10+ expected). Changed signature:

@AppDeepLink({"tv"})
public static Intent intentForTv(@NonNull Context context) {

Same broken registry. Forced to rebuild project => registry recovered.

Updated:

Maybe I'm hi-jacking the issue after all. According to initial report - registry is populated with both entries which is different from my case when registry become broken. I have disabled kapt.arguments.deepLink.incremental and do not have issues so far:

@AppDeepLink({
    "navigate/tv/{kind}",
    "navigate/tv/{kind}/{id}"
})

produces:

new DeepLinkEntry("myapp://navigate/tv/{kind}/{id}", DeepLinkEntry.Type.METHOD, DeepLinksTvNavigation.class, "intentForTv"),
new DeepLinkEntry("myapp://navigate/tv/{kind}", DeepLinkEntry.Type.METHOD, DeepLinksTvNavigation.class, "intentForTv"),

and both are resolved as expected.

rossbacher commented 3 years ago

Sorry it took that long but this is fixed now and in release https://github.com/airbnb/DeepLinkDispatch/releases/tag/5.3.0.

cvillaseca commented 3 years ago

Thank you for the fix @rossbacher 🙏