[Navigation] Check that all required arguments are present in the link#204
[Navigation] Check that all required arguments are present in the link#204osipxd wants to merge 4 commits intoandroidx:androidx-mainfrom
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
osipxd
changed the title
osipxd
force-pushed
the
185527157-check-required-args-in-deep-links
branch
from
482b430 to
32f58c4
Compare
Done |
|
Looks like there's a conflict - could you rebase your changes again? |
|
Yes, I'll rebase it soon |
osipxd
force-pushed
the
185527157-check-required-args-in-deep-links
branch
2 times, most recently
from
fa397d1 to
9010b95
Compare
|
Requires #210 to be merged first |
osipxd
force-pushed
the
185527157-check-required-args-in-deep-links
branch
2 times, most recently
from
2a2d790 to
967d504
Compare
osipxd
force-pushed
the
185527157-check-required-args-in-deep-links
branch
from
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
osipxd
force-pushed
the
185527157-check-required-args-in-deep-links
branch
from
e11326c to
803d037
Compare
osipxd
force-pushed
the
185527157-check-required-args-in-deep-links
branch
from
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. |
5 participants
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