Supplying an empty list for a startDestination's argument causes app to crash [341723133]

Fixed

Bug

Status Update

No update yet.

Description

do...@google.com

created issue #1

May 20, 2024 05:03PM

Component used: Navigation

Version used: 2.8.0-beta01

Devices/Android versions reproduced on: all

When specifying the startDestination for a NavHost, if the destination class is instantiated with an empty list for one of its properties, the app will crash.

Repro code:

@Composable
fun Content() {
   val navController = rememberNavController()
   NavHost(navController = navController, startDestination = Home(usernames = emptyList())) {
       composable<Home> { backStackEntry ->
           val home: Home = backStackEntry.toRoute()
           for (username in home.usernames) {
               Text(username)
           }
       }
   }
}

@Serializable
data class Home(
   val usernames: List<String>,
)

Expected outcome:

App runs and displays an empty screen

Actual outcome:

App crashes with the following error in logcat:

java.lang.IllegalArgumentException: navigation destination com.example.hellonavigation.Home is not a direct child of this NavGraph

Possibly related to: https://b.corp.google.com/issues/339481310

Comments

jb...@google.com <jb...@google.com> May 21, 2024 02:01PM

Assigned to cl...@google.com.

cl...@google.com <cl...@google.com> #2May 21, 2024 08:29PM

The linked issue is due to empty string values "" or listOf("").

In this case, navigation cannot find the destination because the route is missing required argument usernames. This is due to emptyList() being handled as a lack of value.

In the old string route world, the route pattern would be

"com.package.Home?usernames={usernames}"

and the startDestination route with empty list is

"com.package.Home"

Which is illegal even in the old world of string routes - you would get the exact same IllegalArgumentException.

With that said, I think we can handle empty lists better. Since built in list NavTypes are appended as query parameters, I think we can parse this into

"com.package.Home?usernames="

Which is a valid route in base navigation.

ap...@google.com <ap...@google.com> #3Jun 5, 2024 12:36AM

Project: platform/frameworks/support
Branch: androidx-main

commit 0f731acf7753ca32ba38c44c5f325549fb920de1
Author: Clara Fok <clarafok@google.com>
Date: Mon Jun 03 14:14:38 2024

Add emptyCollection API for CollectionNavType

To support empty collections (i.e. emptyList()) when navigating with args, we need a way to create an empty collection of type T.

Test: existing tests
Bug: 341723133
Relnote: "Added new abstract emptyCollection() method for CollectionNavType to support navigating with a empty collection of type T."
Change-Id: Ie4d8464050a7164176c2aee55518b03a5db704ad

M navigation/navigation-common/api/2.8.0-beta03.txt
M navigation/navigation-common/api/current.txt
M navigation/navigation-common/api/restricted_2.8.0-beta03.txt
M navigation/navigation-common/api/restricted_current.txt
M navigation/navigation-common/src/androidTest/java/androidx/navigation/serialization/RouteFilledTest.kt
M navigation/navigation-common/src/main/java/androidx/navigation/CollectionNavType.kt
M navigation/navigation-common/src/main/java/androidx/navigation/NavType.kt
M navigation/navigation-common/src/test/java/androidx/navigation/serialization/NavArgumentGeneratorTest.kt
M navigation/navigation-common/src/test/java/androidx/navigation/serialization/RoutePatternTest.kt
M navigation/navigation-runtime/src/androidTest/java/androidx/navigation/NavControllerRouteTest.kt
M navigation/navigation-testing/src/androidTest/java/androidx/navigation/testing/TestSavedStateHandleFactory.kt

https://android-review.googlesource.com/3112740

ap...@google.com <ap...@google.com> #4Jun 5, 2024 02:11AM

Project: platform/frameworks/support
Branch: androidx-main

commit cc3786dcd8fb1dc3db40d2dbcf878003a6eb5fdb
Author: Clara Fok <clarafok@google.com>
Date: Fri May 31 17:55:16 2024

Refactor NavDeepLink.parseInputParams

Refactored so that it is more readable. It should now be more obvious that when parsing repeated params, we actually call parseArgument first, and call parseArgumentForRepeatedParam for subsequent arg values.

This refactor will also make it easier to handle empty collections for upcoming changes.

Test: ./gradlew navigation:navigation-compose:cC
Bug: 341723133

Change-Id: Ib790ef41a41709842afc01da783ede8a70e251d8

M navigation/navigation-common/src/main/java/androidx/navigation/NavDeepLink.kt
M navigation/navigation-compose/src/androidTest/java/androidx/navigation/compose/NavHostControllerTest.kt

https://android-review.googlesource.com/3112741

ap...@google.com <ap...@google.com> #5Jun 6, 2024 06:19PM

Project: platform/frameworks/support
Branch: androidx-main

commit fb14567d675ea2f166937f5639430bb2aa0d3a71
Author: Clara Fok <clarafok@google.com>
Date: Mon Jun 03 16:08:07 2024

Handle empty CollectionNavType

For collection types, in the past if no value was passed in, the arg would be ignored and omitted from argument bundle. Now, a missing collection type value will default to the NavArgument's defaultValue, or an emptyList if there isn't a defaultValue.

What is considered "no value" is different between list of strings and list of non-strings. For clarity, this is how various routes are handled:

Given a route pattern "

www.test.com?list={myList}"

List<String>
1. "

www.test.com?list=" considered as listOf("")
2. "

www.test.com?list={myList}" considered as listOf("{myList}")
3. "

www.test.com?" considered as no value
4. "

www.test.com" considered as no value

non-String lists
1. "

www.test.com?list=" considered as no value (since it is invalid)
2. "

www.test.com?list={myList}" considered as no value
3. "

www.test.com?" considered as no value
4. "

www.test.com" considered as no value

Test: ./gradlew navigation:navigation-common:cC
Test: ./gradlew navigation:navigation-runtime:cC
Bug: 341723133
Relnote: "Fix how navigation handles missing arg values for CollectionNavTypes - instead of ignoring missing values, navigation will now default to its defaultValue, or an empty collection if there isn't a defaultValue. This fallback value will be added the NavBackStackEntry.argument bundle."

Change-Id: Id630fb148782fbb34d3c236f5d8bb363fba7ef91

M navigation/navigation-common/src/androidTest/java/androidx/navigation/NavDeepLinkTest.kt
M navigation/navigation-common/src/androidTest/java/androidx/navigation/test/NavArgument.kt
M navigation/navigation-common/src/main/java/androidx/navigation/NavDeepLink.kt
M navigation/navigation-runtime/src/androidTest/java/androidx/navigation/NavControllerRouteTest.kt

https://android-review.googlesource.com/3112742

ap...@google.com <ap...@google.com> #6Jun 6, 2024 06:19PM

Project: platform/frameworks/support
Branch: androidx-main

commit 1ae187912afa613f6c1dad408017ebc4b0c3cc41
Author: Clara Fok <clarafok@google.com>
Date: Mon Jun 03 16:52:11 2024

Add safe args tests for empty lists

Test: ./gradlew navigation:navigation-common:cC
Test ./gradlew navigation:navigation-runtime:cC
Bug: 341723133

Change-Id: I1c67ff34424a4559ed09ad2ef1f92c7baec2e23a

M navigation/navigation-common/src/androidTest/java/androidx/navigation/serialization/RouteFilledTest.kt
M navigation/navigation-runtime/src/androidTest/java/androidx/navigation/NavControllerRouteTest.kt

https://android-review.googlesource.com/3112743

cl...@google.com <cl...@google.com> #7Jun 6, 2024 06:41PM

Marked as fixed.

Fixed internally and will be available in navigation 2.8.0-beta03.

pr...@google.com <pr...@google.com> #8Jun 12, 2024 10:45PM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.navigation:navigation-common:2.8.0-beta03
androidx.navigation:navigation-compose:2.8.0-beta03
androidx.navigation:navigation-runtime:2.8.0-beta03