[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 into
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.
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. |
…e link ## 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](https://issuetracker.google.com/issues/185527157) This is an imported pull request from androidx#204. Resolves JetBrains#204 Github-Pr-Head-Sha: 8339589 GitOrigin-RevId: 0df2dce Change-Id: Ie43ace08a5ace320820479f41750712476dae880
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