Status Update
Comments
fi...@gmail.com <fi...@gmail.com> #2
Branch: androidx-master-dev
commit b90079595f33f58fece04026a97faa0d243acdb1
Author: Yuichi Araki <yaraki@google.com>
Date: Wed Sep 18 16:55:49 2019
Change the way to detect mismatch between POJO and query
This fixes cursor mismatch warnings with expandProjection.
Bug: 140759491
Test: QueryMethodProcessorTest
Change-Id: I7659002e5e0d1ef60fc1af2a625c0c36da0664d8
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
M room/compiler/src/test/kotlin/androidx/room/processor/QueryMethodProcessorTest.kt
M room/compiler/src/test/kotlin/androidx/room/testing/TestProcessor.kt
jb...@google.com <jb...@google.com>
cl...@google.com <cl...@google.com> #3
fi...@gmail.com <fi...@gmail.com> #4
Branch: androidx-master-dev
commit bdde5a1a970ddc9007b28de4aa29d60ffa588f08
Author: Yigit Boyar <yboyar@google.com>
Date: Thu Apr 16 16:47:05 2020
Re-factor how errors are dismissed when query is re-written
This CL changes how we handle errors/warnings if query is
re-written.
There was a bug in expandProjection where we would report warnings
for things that Room already fixes automatically (
The solution to that problem (I7659002e5e0d1ef60fc1af2a625c0c36da0664d8)
solved it by deferring validating of columns until after re-write
decision is made. Unfortunately, this required changing PojoRowAdapter
to have a dummy mapping until it is validating, make it hard to use
as it does have a non-null mapping which is not useful.
This CL partially reverts that change and instead rely on the log
deferring logic we have in Context. This way, we don't need to break
the stability of PojoRowAdapter while still having the ability to
drop warnings that room fixes. This will also play nicer when we
have different query re-writing options that can use more information
about the query results.
Bug: 153387066
Bug: 140759491
Test: existing tests pass
Change-Id: I2ec967c763d33d7a3ff02c1a13c6953b460d1e5f
M room/compiler/src/main/kotlin/androidx/room/log/RLog.kt
M room/compiler/src/main/kotlin/androidx/room/processor/QueryMethodProcessor.kt
M room/compiler/src/main/kotlin/androidx/room/solver/TypeAdapterStore.kt
M room/compiler/src/main/kotlin/androidx/room/solver/query/result/PojoRowAdapter.kt
cl...@google.com <cl...@google.com> #5
Custom nav type works. Your serializeAsValue
can be
override fun serializeAsValue(value: String?) = Uri.encode(value ?: "null")
You'll need to pass in your custom nav type to the typeMap
fi...@gmail.com <fi...@gmail.com> #6
Thank you for your response.
Serializing null
as "null"
makes null
indistinguishable from "null"
. Of course it works when entering from a deep link.
https://example.com
-->param
isnull
https://example.com?param=null
-->param
is"null"
But it doesn't work as expected when using internal navigation, i.e. navigate(NotHome())
.
NotHome(param = null)
-->param
is"null"
(exptected:null
)NotHome(param = "null")
-->param
is"null"
Probably this is the limitation of Navigation component that serializeAsValue
is always called (no option to omit an argument from being serialized)?
fi...@gmail.com <fi...@gmail.com> #7
Even forcing the property to be not-nullable also fails when the deep link is
https://example.com?param=null
(java.lang.IllegalStateException: Unexpected null value for non-nullable argument param).
By the way I want to highlight this. This causes us not to be able to use String
property, because anytime there can be "null"
querystring that will make the app crash.
cl...@google.com <cl...@google.com> #8
It is less about serializeAsValue
and more about the built-in NavType.StringType
adhering to kotlin's null.toString
convention.
Though I'm curious why you can't do this
val param = bse.toRoute<NotHome>().param ?: "null"
Text("param=$param")
By the way I want to highlight this. This causes us not to be able to use String property, because anytime there can be "null" querystring that will make the app crash.
Yes, when you explicitly declare a non nullable String
type, a null value is not allowed. That is part of type-safety. And to ensure deeplinks are type-safe, you should add deeplinks via the new
composable<NotHome>(
deepLinks = listOf(navDeepLink<NotHome>(basePath = "example.com") { ... }),
) { bse ->
...
}
fi...@gmail.com <fi...@gmail.com> #9
Though I'm curious why you can't do this
It still makes "null"
indistinguishable from null
. This is not ideal from an idealist's point of view :D. However, from practical point of view, if param
is a search query or an initial value of a TextField
, "null"
within a deep link does mean "null"
while the absence (null
) or ""
both mean ""
, it will never be null
, so it is String
instead of String?
. This is not an obstacle, since we can make a custom NavType
for String
that is consistent and symmetric, it is just inconvenient because we need to do this manually by passing a custom NavType
to every destination declaration to avoid crash.
This is just my opinion when evaluating the beta version. In the end, the decision is yours.
to ensure deeplinks are type-safe, you should add deeplinks via the new type-safe deepLink Builder
Either the deep link is declared with full placeholders or with navDeepLink<NotHome>(basePath = "example.com")
, in case NotHome.param
is String
(not nullable) and is optional:
- Deep links from outside (which we can't control) such as
https://example.com?param=null
will make the app crash instead of fall back to the default destination (start destination of the root graph). - Internal navigation such as
navigate(NotHome("null"))
will also throw an exception, thus"null"
is considered an invalid argument that needs to be checked prior to each call tonavigate
.- This is the same problem as
navigate(NotHome(""))
ifparam
is a required argument, because in this case the (filled) route isandroid-app://androidx.navigation/com.example.nav.NotHome/
which doesn't match the patternandroid-app://androidx.navigation/com.example.nav.NotHome/{param}
. This also needs to be checked prior to each call tonavigate
. Otherwise, an exception might be thrown. (EDIT: Fixed in 2.8.0-beta04)
- This is the same problem as
In the end, to be safe, a string argument must always be String?
, or String
with custom NavType
.
Regards,
cl...@google.com <cl...@google.com> #10
null
means ""
is a value of empty string - they certainly do not both mean ""
.
I'm getting some contradictory messages here - you want to support deeplinks that pass in null (which may or may not be "null" given that its a string deepLink), yet you want the param to be String
instead of String?
. You seem to be saying we will support deeplinks passing in null
, but we also don't.
Granted that kotlin's null.toString()
returns "null"
, how do you know for sure that the source of the deeplink, which you don't control, didn't pass in null
instead of "null"
? In that case your use of String
would be incorrect.
The mix use of "null"
with non-nullable types also kind of circumvents Kotlin's effort to ensure
fi...@gmail.com <fi...@gmail.com> #11
I did change my mind over time. The "last state of my mind" about String
and String?
is:
-
If the type is
String
:
Expected: It should never be null.
Actual: String"null"
in the URI (deep link) is treated asnull
so that runtime error occurs, becauseString
type is not nullable. -
If the type is
String?
: I don't thinkString?
make sense, because a string parameter cannot be expressed as null in a URI (deep link), except by omitting both the key/name and the value (i.e. absence of parameter). However, absence of parameter in Navigation component means fallback to the default value. Meanwhile, default values can be anything, they are not alwaysnull
. So, I have no strong opinion about this.
Conclusion: My primary concern is about number 1, because it can make apps crash.
il...@google.com <il...@google.com> #12
Similar to the work done in IntType
does not support), perhaps there's a similar improvement we can do here where we should only use the default StringType
for String?
types (where "null"
would still be parsed as null
) and instead have a separate, internal type specifically for usage of String
in a Serializable class that does not support null
and won't ever parse any string, including "null"
as null
.
That would prevent any crashing and ensure that developers are in full control of whether they want String?
or String
in their Serializable type and ensure that no matter which option they choose, their app would not crash.
cl...@google.com <cl...@google.com>
ap...@google.com <ap...@google.com> #13
Branch: androidx-main
commit 3182ffe35c0ba338648d588005f3678951ab82c1
Author: Clara Fok <clarafok@google.com>
Date: Tue Sep 03 16:25:41 2024
Add built-in safe args support for List<String?>?
List of nullable String that matches NavType.StringType where both null and "null" are parsed and stored as null.
Test: ./gradlew navigation:navigation-common:test
Test: ./gradlew navigation:navigation-runtime:cC
Bug: 348936238
Change-Id: Id94a3b2c3fe5db739010df10007ae17276e76fc2
M navigation/navigation-common/src/main/java/androidx/navigation/serialization/NavTypeConverter.kt
M navigation/navigation-common/src/test/java/androidx/navigation/serialization/NavArgumentGeneratorTest.kt
M navigation/navigation-runtime/src/androidTest/java/androidx/navigation/NavControllerRouteTest.kt
ap...@google.com <ap...@google.com> #14
Branch: androidx-main
commit cc1a43e0b5dbc68b88a632061e5c9e774d14cb5a
Author: Clara Fok <clarafok@google.com>
Date: Tue Sep 03 16:11:09 2024
Add built-in safe args support for Array<String?>?
Array of nullable String that matches NavType.StringType behavior where both null and "null" are parsed and stored as null.
Test: ./gradlew navigation:navigation-common:test
Test: ./gradlew navigation:navigation-runtime:cC
Bug: 348936238
Change-Id: I3028258519d7f8bee27639a277f45a1e6d173aed
M navigation/navigation-common/src/main/java/androidx/navigation/serialization/NavTypeConverter.kt
M navigation/navigation-common/src/test/java/androidx/navigation/serialization/NavArgumentGeneratorTest.kt
M navigation/navigation-runtime/src/androidTest/java/androidx/navigation/NavControllerRouteTest.kt
ap...@google.com <ap...@google.com> #15
Branch: androidx-main
commit e42295d9c89d79407ba9579a75cef565e224f763
Author: Clara Fok <clarafok@google.com>
Date: Thu Aug 29 14:58:22 2024
Add safe args support for non-nullable String
Previously, non-nullable String and nullable Strings in safe args have the same behavior regarding string "null" where it is parsed into null in adherence to kotlin's "null".toString() convention. If users wanted non-nullable Strings to parse "null" as "null", they would need custom types.
Now safe args has added an internal non-nullable String NavType so that users do not need to provide their custom NavType. When users pass in a type String (non-nullable), we delegate to the internal NavType so that "null" will be parsed as "null.
The behavior for nullable Strings remains unchanged.
Test: ./gradlew navigation:navigation-common:test
Test: ./gradlew navigation:navigation-runtime:cC
Bug: 348936238
Relnote: "Navigation safe args has added support for non-nullable Strings such that \"null\" values will be parsed and stored into the bundle as is. This departs from existing behavior where \"null\" values are parsed into null. This change only applies to non-nullable String type. Nullable strings remain unchanged."
Change-Id: I08b4a9ce5e295a1bddb7baaf9b09af6367a2faa9
M navigation/navigation-common/src/main/java/androidx/navigation/NavType.kt
M navigation/navigation-common/src/main/java/androidx/navigation/serialization/NavTypeConverter.kt
M navigation/navigation-common/src/test/java/androidx/navigation/serialization/NavArgumentGeneratorTest.kt
M navigation/navigation-common/src/test/java/androidx/navigation/serialization/NavTypeConverterTest.kt
M navigation/navigation-runtime/src/androidTest/java/androidx/navigation/NavControllerRouteTest.kt
M navigation/navigation-testing/src/test/java/androidx/navigation/testing/TestSavedStateHandleFactory.kt
cl...@google.com <cl...@google.com> #16
Fixed internally and will be available in navigation 2.8.1
na...@google.com <na...@google.com> #17
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.navigation:navigation-runtime:2.8.1
androidx.navigation:navigation-testing:2.8.1
na...@google.com <na...@google.com> #18
The following release(s) address this bug.It is possible this bug has only been partially addressed:
androidx.navigation:navigation-common:2.9.0-alpha01
Description
Component used: Navigation
Version used: 2.8.0-beta03
Devices/Android versions reproduced on: emulator
Suppose the class for the route is
NotHome
.Suppose the destination is set up like this.
When the link is
https://example.com?param=NULL
,param
is string'NULL'
as expected. But if the link ishttps://example.com?param=null
,param
is reallynull
instead of string'null'
.