PopUpTo to parent NavGraph crashes when child nav-graph has non-nullable arguments [249988437]

Fixed

Bug

Status Update

No update yet.

Description

da...@sucharda.cz

created issue #1

Sep 30, 2022 02:32PM

Component used: Navigation
Version used: 2.5.1 and 2.5.2
Last known working version: 2.3.5
Devices/Android versions reproduced on: Huawei P20 Light / AN 9 (EMUI 9.1.0)

So our navigation is kinda complex so I will try to simplify the configuration. If there is a nested graph with arguments which cannot be null (and no default value is supplied) and popUpTo is used to remove nested graph during destination (nested) navigation it tries to recreate nested graph as a parent for the destination being navigated to.

<navigation xmlns:android="http://schemas.android.com/apk/res/android"
    xmlns:app="http://schemas.android.com/apk/res-auto"
    android:id="@+id/parentNavGraph"
    app:startDestination="@id/parentFragment">

    <fragment
        android:id="@+id/parentFragment"
        android:label="parentFragment"
        tools:layout="@layout/fragment_parent">

        <action
            android:id="@+id/actionToChildGraph"
            app:destination="@id/childNavGraph"/>

    </fragment>

    <navigation
        android:id="@+id/childNavGraph"
        app:startDestination="@id/childFragment">

        // Note: this is a duplicate due to NavArgs since it will not see "childFragment" args
        <argument
            android:name="argumentLong"
            app:argType="long"
            app:nullable="false"/>

        <fragment
            android:id="@+id/childFragment"
            android:label="childFragment"
            tools:layout="@layout/fragment_child">

            <argument
                android:name="argumentLong"
                app:argType="long"
                app:nullable="false"/>

            <action
                android:id="@+id/actionToChildFragment2"
                app:destination="@id/childFragment2"
                app:popUpTo="@id/childNavgraph"
                app:popUpToInclusive="true"/>

        </fragment>

        <fragment
            android:id="@+id/childFragment2"
            android:label="childFragment2"
            tools:layout="@layout/fragment_child_2"/>

</navigation>

So now if the navigation is triggered like this:

findNavController().navigate(
  ChildFragmentDirections.actionToChildFragment2()
)

It will pop fragments from back stack, but it will not change parent for ChildFragment2 which will be set to childNavGraph and since it is not in the back stack anymore it will try to recreate it with default values. But argumentLong is not nullable and has no default value so it will crash:

// NOTE: this crash is from Navigation version 2.5.1
java.lang.NullPointerException: null cannot be cast to non-null type kotlin.Long
  at androidx.navigation.NavType$Companion$LongType$1.get(NavType.kt:352)
  at androidx.navigation.NavType$Companion$LongType$1.get(NavType.kt:342)
  at androidx.navigation.NavArgument.verify(NavArgument.kt:76)
  at androidx.navigation.NavDestination.addInDefaultArgs(NavDestination.kt:502)
  at androidx.navigation.NavController.addEntryToBackStack(NavController.kt:1865)
  at androidx.navigation.NavController.addEntryToBackStack$default(NavController.kt:1813)
  at androidx.navigation.NavController$navigate$4.invoke(NavController.kt:1721)
  at androidx.navigation.NavController$navigate$4.invoke(NavController.kt:1719)
  at androidx.navigation.NavController$NavControllerNavigatorState.push(NavController.kt:287)
  at androidx.navigation.fragment.FragmentNavigator.navigate(FragmentNavigator.kt:246)
  at androidx.navigation.fragment.FragmentNavigator.navigate(FragmentNavigator.kt:162)
  at androidx.navigation.NavController.navigateInternal(NavController.kt:260)
  at androidx.navigation.NavController.navigate(NavController.kt:1719)
  at androidx.navigation.NavController.navigate(NavController.kt:1545)
  at androidx.navigation.NavController.navigate(NavController.kt:1472)
  at androidx.navigation.NavController.navigate(NavController.kt:1930)

Note: Interesting thing is that it does not crash with non-nullable String argument for some reason (ah I see, it is because String NavType is nullable by default: StringType: NavType<String?> vs LongType: NavType<Long>).

Possible fixes for us:

Do not use popUpTo to pop parent (navGraph) of a Fragment (node) from XML but use popBackStack from code instead.
Add default value to argumentLong.

Basically I am not sure if this an intended behavior or a bug. Gonna mark it as a bug because it worked in 2.3.5. I understand that Destination should have a parent but what is the reason to create a parent that does not exist and will never be displayed in this case, since it was already poped and thus it should not be displayed to the user.

At the moment two solutions come to mind:

Change new Fragment (node) parent to last destination in back stack after poping. Here I am not really sure if parent is designed this way and if it can be done without breaking something else.
Make LongType and other types nullable same as StringType. Again not sure if this can be done and it probably is not a proper solution (change).

There might of course be other solutions that I cannot see.

Thank you for your time and help.

Comments

da...@sucharda.cz <da...@sucharda.cz> #2Sep 30, 2022 02:38PM

Can you explain your use case?

jb...@google.com <jb...@google.com> #3Sep 30, 2022 04:37PM

I am using Safe Args from Navigation and also using SavedStateHandle to pass in arguments from the args bundle to my ViewModel. This gives me access to them through the constructor. Unfortunately I then have to use string keys to get values out of the SavedStateHandle instead of using the generated SafeArgs.

da...@sucharda.cz <da...@sucharda.cz> #4Oct 20, 2022 04:26PM

Dagger Hilt has been released in alpha. It greatly simplifies SavedStateHandle injection into ViewModel. Soon more and more people would want this feature.

da...@sucharda.cz <da...@sucharda.cz> #5Oct 20, 2022 04:30PM

Currently the combination of Hilt, SavedStateHandle and Safe args is not possible. The closest I've seen is to drop using safe args and just use magic strings. This also not ideal re dependency inversion as now you have to construct a mock SavedStateHandle for unit tests etc.

class ArticleViewModel @ViewModelInject constructor(
        private val repository: ArticleRepository,
        @Assisted savedStateHandle: SavedStateHandle
) {
    // FIXME: oops, magic "articleId" string
    private val articleId: Long = savedStateHandle["articleId"]
            ?: throw IllegalArgumentException("Article ID required")
}

jb...@google.com <jb...@google.com> Nov 8, 2022 05:32PM

Assigned to cl...@google.com.

cl...@google.com <cl...@google.com> #6Mar 15, 2023 08:32PM

Marked as fixed.

I thought I needed this, but then I realized that I can just create an empty Bundle, store the Fragment's arguments in it as a Bundle (so a Bundle inside a Bundle) and use that to initialize the SavedStateHandle. Then I can just retrieve the arguments from the SavedStateHandle and use the already existing fromBundle method.

What I like about this is that I get to have immutable init arguments, as long as I store the Fragment's arguments with a key that I would never use for other entries.

With a helper function and a base ViewModel class you can hide this entire process and have ViewModels that expose the arguments object as property/member of the ViewModel itself.

ap...@google.com <ap...@google.com> #7Mar 15, 2023 08:32PM

Could you provide an example of how you achieve that?

pr...@google.com <pr...@google.com> #8Mar 22, 2023 08:23PM

What I wrote requires the ability to provide a Bundle to initialize the SavedStateHandle and currently that's not possible with Hilt. You need to wait for this to be fixed: https://github.com/google/dagger/issues/1938 (https://android-review.googlesource.com/c/platform/frameworks/support/+/1427172).

If you do not use Hilt, then what I wrote becomes straightforward. Rather than passing the Fragment's arguments directly, you provide a Bundle with the Fragment's arguments in it:

Bundle().apply { putBundle(SOME_SPECIAL_BUNDLE_KEY, arguments) }

By doing so you'll be able to do the following in your ViewModel:

val fragmentArgs = savedStateHandle[SOME_SPECIAL_BUNDLE_KEY]
val args = YouFragmentArgs.fromBundle(fragmentArgs)

You still need to use a magic key for the Bundle, but it can be a project-wide constant and you only have to pass a single argument. The initialization is also the same for all the ViewModels, so you can easily write helpers to abstract away the process.