[Navigation] Allow case-insensitive matches of Enum args in deep links #152

Bradleycorn · 2021-04-02T23:03:19Z

Proposed Changes

Deep Link arguments that are Enum types would only match and get parsed if the casing of the argument value was an exact match to the casing of the Enum value. Since enum values are typically all upper case, this required deep links to contain upper case values as well, meaning if you have an enum with a value of say Bitmap.Config.ALPHA_8, a deep link that has an argument of type Bitmap.Config will only match if the URL contains /ALPHA_8 and will not match a url with /alpha_8.

This PR updates the EnumType value parsing to include case-insensitive matching. It will first try to parse an enum value using a case-sensitive match, and if no match is found, it will then try to parse the enum value using a case-insensitive match.

Given the following enum:

 enum class Colors {
   RED,
   BLUE,
   GREEN
}

And a destination with an Enum argument and deep link:

<argument
        android:name="destColor"
        app:argType="com.my.app.Colors"
        android:defaultValue="RED" />
  
<deepLink
        app:uri="https://myapp.com/{destColor}" />

This PR updates the EnumType parsing so that a url like https://myapp.com/green will be matched and the destColor will be set to Colors.GREEN

Testing

There were no existing tests for the EnumType.parseValue() method.
I added a test in NavTypeTest to test EnumType.parseValue() asserting that both case-sensitive and case-insensitive values are matched.

Test: /gradlew test connnectedCheck

Issues Fixed

https://issuetracker.google.com/issues/135857840
Fixes: b/135857840

jbw0033 · 2021-04-12T18:49:29Z

navigation/navigation-common/src/main/java/androidx/navigation/NavType.kt

 return constant
 }
+ if (enumConstant.name.equals(value, ignoreCase = true)) {
+ caseInsensitiveMatch = constant
+ break


I am missing why we need to break here instead of returning the match?

Good call. We don't need to break here. But it probably shouldn't return straight away either. I would think it should continue looping the remaining enum values for an exact match and return that if found, and only return the caseInsensitiveMatch if it doesn't end up finding an exact match.

If you have a deeplink url of https://myapp.com/someenumvalue
and an enum:

enum class MyEnum { SomeEnumValue someenumvalue }

We would not want the case insensitive match on the frst item to be returned when the 2nd item will be an exact match.
I've updated the PR to do that. If that's still not right let me know and I can make further updates

I don't think we need to match to that level. We specifically want the Enums to be case insensitive here, so distinguishing by case after that match may not be reasonable.

I would argue that we do want the first item to be returned to discourage this type of confusing distinction. If we look for the exact match, it would allow the following:

enum class MyEnum { RED REd Red red (etc..) }

Now we are back in the case where all of these could potentially point to different destinations, which is the problem with this bug in the first place. Maybe provide it and hope no one does this type of thing, but I tend to err on the side that if you can do it someone will. @ianhanniballake curious on your thoughts?

OK. If it should just do a simple case-insensitive match, then I can probably update it to use firstOrNull() which IMO will make it more concise/readable as well.

Yes, let's use firstOrNull() here - the first case insensitive match is the one we want to use.

@jbw0033 @ianhanniballake - Updated to do a simple case-insensitive match.

Bradleycorn · 2021-04-13T01:27:34Z

@jbw0033 - I pushed an update and the lint check fails with fatal: Invalid revision range 660c9cdf14696101b4caa8da5cfe22375697246b..HEAD. Not sure how to address that?

dlam · 2021-04-13T03:18:29Z

@Bradleycorn I'm not sure why this is happening yet, but it looks like it's comparing from a commit from April 2 and failing to find the sha in the checkout.

Could you try rebasing your changes on top of latest on androidx-main and see if that resolves the issue?

Bradleycorn · 2021-04-13T11:35:27Z

Could you try rebasing your changes on top of latest on androidx-main and see if that resolves the issue?

I figured that might be the issue. I'll try it.

…hing values are found when doing the case-sensitive matchiing.

…on as it is found. Instead keep looking for an exact match.

Bradleycorn · 2021-04-13T11:42:31Z

@dlam Rebasing seems to have fixed it, thanks!

Bradleycorn · 2021-04-13T12:21:53Z

uh oh. @dlam now the build is failing, claiming that the version of Fragment is not sufficient. But as you can see from the changes in the PR, I haven't touched any of that. Looks like the build for the Paging lib failed as well.

dlam · 2021-04-13T17:35:06Z

uh oh. @dlam now the build is failing, claiming that the version of Fragment is not sufficient. But as you can see from the changes in the PR, I haven't touched any of that. Looks like the build for the Paging lib failed as well.

Yep feel free to just ignore that it's a known failure on HEAD atm, working on fixing this :)

googlebot added the cla: yes label Apr 2, 2021

dlam requested a review from jbw0033 April 5, 2021 23:48

jbw0033 requested changes Apr 12, 2021

View reviewed changes

Bradleycorn added 2 commits April 13, 2021 07:39

Updates the EnumType parser to do a case-insensitive match if no matc…

9e15800

…hing values are found when doing the case-sensitive matchiing.

When parsing an EnumTuype, don't return a case insenstive match as so…

fbfeb2b

…on as it is found. Instead keep looking for an exact match.

jbw0033 requested a review from ianhanniballake April 13, 2021 15:07

update EnumType parser do just do a simple case-insensitive match

c7f5aef

jbw0033 approved these changes Apr 13, 2021

View reviewed changes

copybara-service bot closed this in fd55939 Apr 21, 2021

Bradleycorn deleted the 135857840-deeplink-enum-parsing branch April 21, 2021 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Navigation] Allow case-insensitive matches of Enum args in deep links #152

[Navigation] Allow case-insensitive matches of Enum args in deep links #152

Bradleycorn commented Apr 2, 2021

jbw0033 Apr 12, 2021

Bradleycorn Apr 13, 2021 •

edited

jbw0033 Apr 13, 2021

Bradleycorn Apr 13, 2021

ianhanniballake Apr 13, 2021

Bradleycorn Apr 13, 2021

Bradleycorn commented Apr 13, 2021

dlam commented Apr 13, 2021 •

edited

Bradleycorn commented Apr 13, 2021

Bradleycorn commented Apr 13, 2021

Bradleycorn commented Apr 13, 2021

dlam commented Apr 13, 2021

[Navigation] Allow case-insensitive matches of Enum args in deep links #152

[Navigation] Allow case-insensitive matches of Enum args in deep links #152

Conversation

Bradleycorn commented Apr 2, 2021

Proposed Changes

Testing

Issues Fixed

jbw0033 Apr 12, 2021

Choose a reason for hiding this comment

Bradleycorn Apr 13, 2021 • edited

Choose a reason for hiding this comment

jbw0033 Apr 13, 2021

Choose a reason for hiding this comment

Bradleycorn Apr 13, 2021

Choose a reason for hiding this comment

ianhanniballake Apr 13, 2021

Choose a reason for hiding this comment

Bradleycorn Apr 13, 2021

Choose a reason for hiding this comment

Bradleycorn commented Apr 13, 2021

dlam commented Apr 13, 2021 • edited

Bradleycorn commented Apr 13, 2021

Bradleycorn commented Apr 13, 2021

Bradleycorn commented Apr 13, 2021

dlam commented Apr 13, 2021

Bradleycorn Apr 13, 2021 •

edited

dlam commented Apr 13, 2021 •

edited