[Navigation] Check that all required arguments are present in the link #204
Conversation
| val missingRequiredArguments = arguments.filterValues { !it.isDefaultValuePresent } | ||
| .keys | ||
| .filter { it !in navDeepLink.arguments } | ||
| require(missingRequiredArguments.isEmpty()) { | ||
| "Deep link ${navDeepLink.uriPattern} can't be used to open destination $this.\n" + | ||
| "Following required arguments are missing: $missingRequiredArguments" | ||
| } |
There was a problem hiding this comment.
It would be great to have a check like this to make sure deep links will be able to open this destination. But I'm not sure I've selected the right place for this. We can add the required parameters after deepLink is added to bypass this validation.
There was a problem hiding this comment.
Maybe also add validation after a new argument was added?
There was a problem hiding this comment.
Not sure why it wasn't caught in github, but this is failing a number of navigation-runtime test.
I am thinking that our sample xml isn't realistic enough and we may need to add an argument to the deepLink that goes to second_test.
navigation/navigation-common/src/androidTest/java/androidx/navigation/NavDeepLinkTest.kt
Outdated
Show resolved
Hide resolved
482b430
to
32f58c4
Compare
Done |
|
Looks like there's a conflict - could you rebase your changes again? |
|
Yes, I'll rebase it soon |
fa397d1
to
9010b95
Compare
|
Requires #210 to be merged first |
2a2d790
to
967d504
Compare
967d504
to
e11326c
Compare
|
It failed again after rebase. Something related to @dlam, can you take a look? |
|
I think this is on our side since the compose 1.0.1 release isn't yet available. I'll check on getting it resolved. |
Currently we can open destination with link that doesn't contain required arguments. In this case we will get crash in runtime. This commit adds check that all required arguments are present in link. Test: ./gradlew test connectedCheck Fixes: 185527157
e11326c
to
803d037
Compare
d1cced0
to
8339589
Compare
|
Thank you for bearing with us to get this PR out @osipxd! |
|
I believe this change introduced a bug. Please see here. |
|
@svenjacobs, looks like a duplicate of 198689811. This issue is already fixed. I think it will be included in the next version. |
Proposed Changes
Currently, we can open a destination with a link that doesn't contain the required arguments. In this case, we will get a crash in runtime. This commit adds check that all required arguments are present in the link.
After this change, deep links will require to contain all required arguments for a destination. Links not containing all required arguments will be ignored.
Testing
Test: ./gradlew test connectedCheck
Issues Fixed
Fixes: 185527157