Destination does not exists if passing empty string in type safe arguments in jetpack compose navigation [339481310]

Fixed

Feature Request

Status Update

No update yet.

Description

st...@warting.se

created issue #1

May 9, 2024 12:59PM

Component used: Navigation
Version used: 2.8.0-alpha08

If passing an empty string as an safe argument the destination does not exists

@Serializable
object LoginSelector

@Serializable
data class RegisterUser(val email: String)

NavHost(
navController = navController,
startDestination = LoginSelector,
modifier = modifier
) {
composable<LoginSelector> {
Button(onClick = {
navController.navigate(RegisterUser("foo")) // Works!
}) {
Text(text = "Login1")
}

Button(onClick = {
navController.navigate(RegisterUser("")) // Doesn't work. Destination does not exists
}) {
Text(text = "Login2")
}

Button(onClick = {
navController.navigate(RegisterUser(null)) // works if making email nullable
}) {
Text(text = "Login3")
}
}

composable<RegisterUser> { backStackEntry ->
val emailLogin: RegisterUser = backStackEntry.toRoute()
Text(text =emailLogin.email)
}
}

Comments

jb...@google.com <jb...@google.com> May 9, 2024 02:11PM

Assigned to cl...@google.com.

do...@google.com <do...@google.com> #2May 10, 2024 07:29AM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

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

cl...@google.com <cl...@google.com> #3May 14, 2024 10:11PM

Status: Won't Fix (Intended Behavior)

Base Navigation does not allow empty strings for path parameters, because path parameters are required.

val route = "example.com/{firstName}/{lastName}"

val firstName = ""
val lastName = "Doe"
navController.navigate("example.com/$firstName/$lastName")

In the example, you'd be navigating with example.com//Doe, which is not a valid URL.

In safe args, email is non-nullable and therefore a path parameter.

data class RegisterUser(val email: String)

In alignment with base navigation, email cannot be an empty string.

Though maybe we could improve the error messaging.

Message last modified on May 14, 2024 10:12PM

st...@warting.se <st...@warting.se> #4May 21, 2024 11:18AM

According to RFC 3986 (section 3.3) it seems like en empty path can exists by using double slashes.

https://www.rfc-editor.org/rfc/rfc3986#section-3.3

And with that it seems like

https://developer.android.com/reference/android/net/Uri.html#getPathSegments() has a bug where those are filtered out. According to the documentations it should return "decoded path segments, each without a leading or trailing '/'"

val androidUri = android.net.Uri.parse("android-app://androidx.navigation/com.example.hellonavigation.RegisterUser//Doe")
androidUri.pathSegments // {'com.example.hellonavigation.RegisterUser', 'Doe'}
androidUri.path!!.split("/") // {'', 'com.example.hellonavigation.RegisterUser', '' , 'Doe'}

cl...@google.com <cl...@google.com> #5Jun 7, 2024 06:43PM

Status: Assigned (reopened)

I understand the desire for support on empty strings in path. When originally debating support for this in safe args, the sources you linked were part of our references.

Re: #comment4 Contrary to your interpretation, we didn't interpret any confirmation in section 3.3 that // was valid path. Would you like to elaborate on how you got your conclusion?

Furthermore Uri.getPathSegments ignores the // and does not include "" in its return value. However I couldn't find an open bug on this stating that the omission was unintended.

With that said, in the spirit of type safety I think it makes sense to support empty strings for path parameter since it is a valid String value. Type safety implementation delegates to deeplinks, so this support will apply to String routes as well.

Message last modified on Jun 10, 2024 04:38PM

st...@warting.se <st...@warting.se> #6Jun 10, 2024 11:45AM

I might be wrong, but i thought this did:
path-abempty = *( "/" segment )

ap...@google.com <ap...@google.com> #7Jun 18, 2024 07:28PM

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

commit 77e7bde966e9c9efc0d2f3a1afb0f697802cf57c
Author: Clara Fok <clarafok@google.com>
Date: Tue Jun 18 11:20:37 2024

Add support for empty path strings

Previously, empty strings in path arguments are not allowed since path arguments are considered as required, and Uri.getPathSegments() does not return empty strings.

However, support for empty path strings is added since empty string is a valid string value in the world of type safety apis.

Test: ./gradlew navigation:navigation-common:cC
Test: ./gradlew navigation:navigation-runtime:cC
Bug: 339481310
Relnote: "Navigation now supports navigating with empty strings in path arguments."

Change-Id: Ic5dbdd3b4786f62f4544a70b6786a269da8a37cc

M navigation/navigation-common/src/androidTest/java/androidx/navigation/NavDeepLinkTest.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/3136075

cl...@google.com <cl...@google.com> #8Jun 18, 2024 08:29PM

Marked as fixed.

Fixed internally and will be available in navigation 2.8.0-rc01

pr...@google.com <pr...@google.com> #9Jun 26, 2024 05:43PM

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

androidx.navigation:navigation-common:2.8.0-beta04
androidx.navigation:navigation-runtime:2.8.0-beta04