improve native serialization and timestamp fixes #371

vpodlesnyak · 2023-04-11T12:11:51Z

added GeoPoints
added native serialization of DocumentReference
fix for Timestamp extensions
improve debugging

…extensions, improve debugging

vpodlesnyak · 2023-04-11T12:37:17Z

firebase-common/src/jsMain/kotlin/dev/gitlive/firebase/externals.kt

@@ -453,6 +453,8 @@ external object firebase {
            fun update(field: FieldPath, value: Any?, vararg moreFieldsAndValues: Any?): Promise<Unit>
            fun delete(): Promise<Unit>
            fun onSnapshot(next: (snapshot: DocumentSnapshot) -> Unit, error: (error: Error) -> Unit): ()->Unit
+
+            fun isEqual(other: DocumentReference): Boolean


proper equality checks help a lot when writing tests in the applications using this SDK

vpodlesnyak · 2023-04-11T12:40:05Z

README.md

@@ -88,6 +88,7 @@ db.collection("cities").document("LA").set(City.serializer(), city, encodeDefaul
 ```

 The `encodeDefaults` parameter is optional and defaults to `true`, set this to false to omit writing optional properties if they are equal to theirs default values.
+Using [@EncodeDefault](https://kotlinlang.org/api/kotlinx.serialization/kotlinx-serialization-core/kotlinx.serialization/-encode-default/) on properties is a recommended way to locally override the behavior set with `encodeDefaults`.


it helped me a lot so why not to mention it for other devs

vpodlesnyak · 2023-04-11T12:41:30Z

firebase-firestore/src/androidMain/kotlin/dev/gitlive/firebase/firestore/GeoPoint.kt

+actual typealias NativeGeoPoint = com.google.firebase.firestore.GeoPoint
+
+/** A class representing a Firebase GeoPoint. */
+@Serializable(with = GeoPointSerializer::class)


this approach is similar to a PR #312

vpodlesnyak · 2023-04-11T12:45:43Z

firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/Timestamp.kt

@@ -32,8 +32,8 @@ expect class Timestamp internal constructor(nativeValue: NativeTimestamp): BaseT
 }

 fun Timestamp.Companion.fromMilliseconds(milliseconds: Double): Timestamp =
-    Timestamp((milliseconds / 1000).toLong(), (milliseconds * 1000).toInt() % 1000000)
-fun Timestamp.toMilliseconds(): Double = seconds * 1000 + (nanoseconds / 1000.0)
+    Timestamp((milliseconds / 1000).toLong(), ((milliseconds % 1000) * 1000000).toInt())


a bug fix here. nanoseconds weren't calculated properly. added a test

vpodlesnyak · 2023-04-11T13:13:21Z

firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/firestore.kt

-fun Query.where(field: String, inArray: List<Any>? = null, arrayContainsAny: List<Any>? = null) = _where(field, inArray, arrayContainsAny)
-fun Query.where(path: FieldPath, inArray: List<Any>? = null, arrayContainsAny: List<Any>? = null) = _where(path, inArray, arrayContainsAny)
+/** @return a native value of a wrapper or self. */
+private val Any.value get() = when (this) {


this unwrapping is needed to properly support querying by natively supported classes. added a test

am I right to assume that this change is required to perform a query that requires providing a timestamp field?
e.g. where("endAt", greaterThan = Timestamp.now())

I am unable to do so in 1.8.1 right now and am hoping this fixes it :)

vpodlesnyak · 2023-04-11T13:15:55Z

@Reedyuk / @nbransby please take a look when you have time. this PR is a follow up for a PR #312. cheers

Reedyuk · 2023-04-12T17:53:41Z

@nbransby

esafirm · 2023-04-14T02:42:20Z

This PR should close #324 right?

nbransby · 2023-04-21T11:29:40Z

firebase-firestore/src/commonTest/kotlin/dev/gitlive/firebase/firestore/firestore.kt

+/** @return a map extracted from the encoded data. */
+expect fun encodedAsMap(encoded: Any?): Map<String, Any?>
+/** @return pairs as raw encoded data. */
+expect fun Map<String, Any?>.asEncoded(): Any
+


we have nativeMapOf and nativeAssertEquals that can be reused instead of adding additional calls

@nbransby thanks, I actually missed those. however in order for nativeMapOf and nativeAssertEquals to be available in firebase-firestore tests, the methods shall be moved from the tests folder of the firebase-common to a separate module (lets say firebase-common-test). shall I make this change?

good point - maybe would be easier to do it on a source set level instead of a separate module?

could add a commonTestUtil source set to firebase-common and include in the test source of firebase-firestore with:

val commonTest by getting { kotlin.srcDir("../firebase-common/src/commonTestUtil/kotlin") }

the suggested approach created a lot of noise - it required setting up not only common but platform tests. extracted a test-utils module instead

nbransby · 2023-05-24T22:23:07Z

I've noticed a separate issue regarding the TimestampSerializer, compiling a project with the following class defined:

@Serializable
class Data(val timestamp: Timestamp)

fails with

> Task :compileDebugKotlinAndroid FAILED
e: Supertypes of the following classes cannot be resolved. Please make sure you have the required dependencies in the classpath:
    class dev.gitlive.firebase.firestore.TimestampSerializer, unresolved supertypes: dev.gitlive.firebase.SpecialValueSerializer

Any ideas on that?

vpodlesnyak · 2023-05-24T22:48:47Z

@nbransby the Timestamp gets encoded/decoded in TimestampTests so it looks suspicious. I'll investigate further by creating a sample app

nbransby · 2023-05-25T08:05:00Z

Also is this suppose to work?

vpodlesnyak · 2023-05-29T23:01:51Z

@nbransby the serializer not found issue was caused by the fact firebase-common was referenced as implementation instead of api. added a fix

vpodlesnyak · 2023-05-29T23:09:02Z

firebase-firestore/src/commonMain/kotlin/dev/gitlive/firebase/firestore/Timestamp.kt

@@ -84,7 +95,7 @@ object DoubleAsTimestampSerializer : SpecialValueSerializer<Double>(
    toNativeValue = { value ->
        when(value) {
            serverTimestamp -> FieldValue.serverTimestamp.nativeValue
-            else -> Timestamp.fromMilliseconds(value)
+            else -> Timestamp.fromMilliseconds(value).nativeValue


a bug fix - the native value shall be extracted as in the line above

nbransby · 2023-05-30T11:24:22Z

e: Supertypes of the following classes cannot be resolved. Please make sure you have the required dependencies in the classpath:

can you use composition instead of inheritance for the logic shared in SpecialValueSerializer? As making the common module an api import will expose its public api to library consumers but it was designed to hold library internal shared code

vpodlesnyak · 2023-05-30T12:11:41Z

@nbransby

can you use composition instead of inheritance for the logic shared in SpecialValueSerializer? As making the common module an api import will expose its public api to library consumers but it was designed to hold library internal shared code

I might be wrong but I think the error is not related to composition/inheritance, but to whether the end user adds a firebase-common dependency. So we'd either have to make it api or instruct the user to add a firebase-common dependency.

Please also take a look at @FirebaseClassDiscriminator located in firebase-common, it looks like it is meant to be exposed

nbransby · 2023-05-30T14:10:12Z

composition won't expose SpecialValueSerializer in the public API of DoubleAsTimestampSerializer therefore the firebase-common dependency is not required to be api (or manually included by client/end user). The readme doesn't actually mention the firebase-common module and adding FirebaseClassDiscriminator there was not ideal but since its there now its probably best to keep the dependency as api.

Instead, for the firebase-common API not intended to be public we should create something like:
https://kotlinlang.org/api/kotlinx.serialization/kotlinx-serialization-core/kotlinx.serialization/-internal-serialization-api/

nbransby

Thanks for your hard work @vpodlesnyak

nbransby · 2023-06-10T17:46:47Z

I've noticed gradle build is failing on master now with:
e: file:///Users/nbransby/IdeaProjects/firebase-kotlin-sdk/firebase-app/src/commonTest/kotlin/dev/gitlive/firebase/firebase.kt:7:12 Expected property 'context' has no actual declaration in module <firebase-app_debugUnitTest> for JVM
Do you think that was caused by your changes with test-utils @vpodlesnyak ?
We should really starting gradle build in the CI instead of just gradle assemble!

vpodlesnyak · 2023-06-10T21:09:01Z

@nbransby

Thanks for your hard work @vpodlesnyak

my pleasure

build is failing

I see that the failure is caused by unitTests and there are no unit test folders in the repo. I think the error was caused by a commit 30cc1f94 where this bit was removed from the root build.gradle.kts

    tasks.whenTaskAdded {
        enabled = when(name) {
            "compileDebugUnitTestKotlinAndroid" -> false
            "compileReleaseUnitTestKotlinAndroid" -> false
            "testDebugUnitTest" -> false
            "testReleaseUnitTest" -> false
            else -> enabled
        }
    }

nbransby · 2023-06-11T12:33:28Z

good spot!

added GeoPoints, serialize DocumentReference natively, fix Timestamp …

aa8189f

…extensions, improve debugging

vpodlesnyak commented Apr 11, 2023

View reviewed changes

add a test for querying by natively supported class

3f28db6

vpodlesnyak commented Apr 11, 2023

View reviewed changes

make ios timestamp test stable

c03dc3f

Reedyuk approved these changes Apr 12, 2023

View reviewed changes

nbransby requested changes Apr 21, 2023

View reviewed changes

vpodlesnyak requested a review from nbransby May 3, 2023 08:27

Merge branch 'master' into native-firebase-classes-serialization

1fcb535

vpodlesnyak mentioned this pull request May 29, 2023

DoubleAsTimestampSerializer.serverTimestamp not accessible #380

Closed

vpodlesnyak added 3 commits May 29, 2023 23:56

fix SpecialValueSerializer not found issue

8c268f6

improve timestamp to milliseconds conversion

3bacb97

extract test-utils module

87a5862

vpodlesnyak commented May 29, 2023

View reviewed changes

vpodlesnyak added 3 commits May 30, 2023 01:09

cleanup

41ee794

add legacy to new format timestamp test

4d75507

add missing js setup for test-utils

d4365d6

use delegation instead of inheritance for special value serialization

4df30d3

nbransby approved these changes Jun 10, 2023

View reviewed changes

nbransby merged commit a3c295f into GitLiveApp:master Jun 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve native serialization and timestamp fixes #371

improve native serialization and timestamp fixes #371

vpodlesnyak commented Apr 11, 2023

vpodlesnyak Apr 11, 2023

vpodlesnyak Apr 11, 2023

vpodlesnyak Apr 11, 2023

vpodlesnyak Apr 11, 2023

vpodlesnyak Apr 11, 2023

walkingbrad Jun 5, 2023

vpodlesnyak commented Apr 11, 2023 •

edited

Loading

Reedyuk commented Apr 12, 2023

esafirm commented Apr 14, 2023

nbransby Apr 21, 2023

vpodlesnyak Apr 22, 2023

nbransby May 24, 2023

vpodlesnyak May 24, 2023

vpodlesnyak May 29, 2023

nbransby commented May 24, 2023

vpodlesnyak commented May 24, 2023

nbransby commented May 25, 2023

vpodlesnyak commented May 29, 2023

vpodlesnyak May 29, 2023

nbransby commented May 30, 2023

vpodlesnyak commented May 30, 2023

nbransby commented May 30, 2023

nbransby left a comment

nbransby commented Jun 10, 2023

vpodlesnyak commented Jun 10, 2023 •

edited

Loading

nbransby commented Jun 11, 2023

improve native serialization and timestamp fixes #371

improve native serialization and timestamp fixes #371

Conversation

vpodlesnyak commented Apr 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vpodlesnyak commented Apr 11, 2023 • edited Loading

Reedyuk commented Apr 12, 2023

esafirm commented Apr 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nbransby commented May 24, 2023

vpodlesnyak commented May 24, 2023

nbransby commented May 25, 2023

vpodlesnyak commented May 29, 2023

Choose a reason for hiding this comment

nbransby commented May 30, 2023

vpodlesnyak commented May 30, 2023

nbransby commented May 30, 2023

nbransby left a comment

Choose a reason for hiding this comment

nbransby commented Jun 10, 2023

vpodlesnyak commented Jun 10, 2023 • edited Loading

nbransby commented Jun 11, 2023

vpodlesnyak commented Apr 11, 2023 •

edited

Loading

vpodlesnyak commented Jun 10, 2023 •

edited

Loading