Adding Serialization fixes

[Daeda88]

The current implementation of serializing classes does not work correctly when dealing with nested classes.

This PR aims to fix this by rewriting the encoding/decoding. This introduces EncodingSettings and DecodingSettings to Database, Firestore and Functions. On Firestore additional async support has been added

[nbransby]

are their any breaking changes in here?

[Daeda88]

There are a few where encodeDefaults: Boolean was changed to encodeSettings: EncodeSettings. On some places I did add convenience extension methods, but that would require an import (and I also forgot in a few places). Ill try and add legacy accessors. This is gonna conflict with the where PR though, so it would be great if we can get that merged first so I can track the changes more easily.

Btw, if you're worried about API changes, consider adding https://github.com/Kotlin/binary-compatibility-validator to the project. We use it for Kaluga and it makes it much easier to spot changes

[nbransby]

Btw, if you're worried about API changes, consider adding https://github.com/Kotlin/binary-compatibility-validator to the project. We use it for Kaluga and it makes it much easier to spot changes

Yes we need this!

[Daeda88]

Made changes to keep API stable

[nbransby]

The Java SDK has been updated to support latest firebase libs from android bom 32.7.0 and I have removed the jvmMain sources on master

[Daeda88]

@nbransby seems publication to maven central failed: https://central.sonatype.com/artifact/dev.gitlive/firebase-java-sdk/versions

[nbransby]

@nbransby seems publication to maven central failed: https://central.sonatype.com/artifact/dev.gitlive/firebase-java-sdk/versions

fixed this now

[nbransby]

Found some more time to review...

I think we should keep named arguments instead of the EncodeSettings class, revert https://github.com/GitLiveApp/firebase-kotlin-sdk/pull/449#issuecomment-1873835429 and update the examples to some of the places the new arguments are added.

Having to wrap settings in EncodeSettings(...) is necessary boilerplate in the public API, people will likely only want to set 1 or 2 settings and named arguments with default values works great for that.

[Daeda88]

Done

[Daeda88]

Im trying a builder approach instead, as suggested here https://github.com/GitLiveApp/firebase-kotlin-sdk/pull/448#discussion_r1450851712

The advantage of this compared to named arguments is that it is much easier to maintain.

[Daeda88]

@nbransby in case you werent aware, the builder approach is in the latest commit. Ive also tracked master again

[nbransby]

Should all functions that take a builder lambda be marked as inline to avoid the lambda allocation?

[nbransby]

Whats the point of the Async nested classes and the Deferred.convert function?

[Daeda88]

Should all functions that take a builder lambda be marked as inline to avoid the lambda allocation?

I dont think that actually avoids anything. We can make the lambda nullable I guess

Whats the point of the Async nested classes and the Deferred.convert function?

Well, to improve async behaviour. For instance, logging or grouping. E.g.

suspend fun deleteSomeDocs() {
    val toExecute = listOf(
       firestore.collectionn("Foo").document("Bar").async.delete()
       firestore.collectionn("Foo").document("BarBar").async.delete()
    )
    println("Started deletion")
    // do some stuff in the mean time
    toExectute.awaitAll()
}

Deferred convert is a mapper for deferred. The Android sdk would return an AndroidDocumentReference and you'd want to return a Kotlin DocumentReference, so it should be mapped.

[nbransby]

I dont think that actually avoids anything. We can make the lambda nullable I guess

It does, thats the whole point of inline functions

[nbransby]

Well, to improve async behaviour. For instance, logging or grouping. E.g.

As this is just a generic async utility function and unrelated to firebase we shouldn't be introducing it to our public API, you are welcome to add it to the test sources if it makes writing tests easier

[nbransby]

Deferred convert is a mapper for deferred. The Android sdk would return an AndroidDocumentReference and you'd want to return a Kotlin DocumentReference, so it should be mapped.

Same with this, move it to test sources if you use it in the tests

[Daeda88]

@nbransby Ive removed the Async classes and made some changes so all builders funs are now inline. Ive annotated methods with @PublishedAPI if only used by the inline methods. The actual implementations of builders are now hidden from the end users.

In addition, Ive made changes to fix runTransaction on database, so it now properly uses encode/decode settings

[Daeda88]

Ive included changes proposed by #456

[nbransby]

Looking good, whats the purpose of the Base... and Native... classes such as NativeQuery and s BaseTransaction? @Daeda88

[Daeda88]

Looking good, whats the purpose of the Base... and Native... classes such as NativeQuery and s BaseTransaction? @Daeda88

So with expect/actuals classes you cannot implement code on the common side. Even though with a lot of the code, you kind of want it (e.g., most methods need to encode/decode their values). Base<ClassName> solves this by introducing an abstract class that the expect/actuals classes implement. It makes code much more maintainable and also testable since they can be mocked. Native<Class> is a solution to allow for common constructors. This is useful for the abstract classes (e.g. BaseDatabaseReference) that may map the common behaviour. All in all, its to reduce code duplication.

[nbransby]

Even though with a lot of the code, you kind of want it (e.g., most methods need to encode/decode their values). Base solves this by introducing an abstract class that the expect/actuals classes implement.

I assume this is to share code internally within the library? I also assume we don't need to share additional internal state (the only state our wrapper classes should ever hold is the reference to the native instance[^1] in which case inheritance is probably not the right tool here, wouldn't internal extension functions be a better fit? Want to prevent polluting the public API with inheritance chains when their only purpose is internal code sharing.

[^1]: in fact one day it would be nice if our wrapper classes were compiled away at build time)

[Daeda88]

Even though with a lot of the code, you kind of want it (e.g., most methods need to encode/decode their values). Base solves this by introducing an abstract class that the expect/actuals classes implement.

I assume this is to share code internally within the library? I also assume we don't need to share additional internal state (the only state our wrapper classes should ever hold is the reference to the native instance1 in which case inheritance is probably not the right tool here, wouldn't internal extension functions be a better fit? Want to prevent polluting the public API with inheritance chains when their only purpose is internal code sharing.

Footnotes

1. in fact one day it would be nice if our wrapper classes were [compiled away at build time](https://github.com/GitLiveApp/firebase-kotlin-sdk/issues/132)) [↩](#user-content-fnref-1-6c88b1acbb299166cc14a4d10e5c5c3e)

There's no state being stored whatsoever. This is about common code having common behaviour. To give an example with the changes:

Old:

// Common
expect class DocumentReference internal constructor(nativeValue: NativeDocumentReference) {
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean = true, merge: Boolean = false)
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean = true, vararg mergeFields: String)
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean = true, vararg mergeFieldPaths: FieldPath)

    suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean = true, merge: Boolean = false)
    suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean = true, vararg mergeFields: String)
    suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean = true, vararg mergeFieldPaths: FieldPath)
}

// Android
actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) {
    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, merge: Boolean) = when(merge) {
        true -> android.set(encode(data, encodeDefaults)!!, SetOptions.merge())
        false -> android.set(encode(data, encodeDefaults)!!)
    }.await().run { Unit }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        android.set(encode(data, encodeDefaults)!!, SetOptions.mergeFields(*mergeFields))
            .await().run { Unit }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        android.set(encode(data, encodeDefaults)!!, SetOptions.mergeFieldPaths(mergeFieldPaths.map { it.android }))
            .await().run { Unit }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, merge: Boolean) = when(merge) {
        true -> android.set(encode(strategy, data, encodeDefaults)!!, SetOptions.merge())
        false -> android.set(encode(strategy, data, encodeDefaults)!!)
    }.await().run { Unit }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        android.set(encode(strategy, data, encodeDefaults)!!, SetOptions.mergeFields(*mergeFields))
            .await().run { Unit }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        android.set(encode(strategy, data, encodeDefaults)!!, SetOptions.mergeFieldPaths(mergeFieldPaths.map { it.android }))
            .await().run { Unit }
}

// iOS
actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) {
    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, merge: Boolean) =
        await { ios.setData(encode(data, encodeDefaults)!! as Map<Any?, *>, merge, it) }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        await { ios.setData(encode(data, encodeDefaults)!! as Map<Any?, *>, mergeFields.asList(), it) }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        await { ios.setData(encode(data, encodeDefaults)!! as Map<Any?, *>, mergeFieldPaths.map { it.ios }, it) }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, merge: Boolean) =
        await { ios.setData(encode(strategy, data, encodeDefaults)!! as Map<Any?, *>, merge, it) }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        await { ios.setData(encode(strategy, data, encodeDefaults)!! as Map<Any?, *>, mergeFields.asList(), it) }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        await { ios.setData(encode(strategy, data, encodeDefaults)!! as Map<Any?, *>, mergeFieldPaths.map { it.ios }, it) }
}

// JS
actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) {
    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, merge: Boolean) =
        rethrow { setDoc(js, encode(data, encodeDefaults)!!, json("merge" to merge)).await() }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        rethrow { setDoc(js, encode(data, encodeDefaults)!!, json("mergeFields" to mergeFields)).await() }

    actual suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        rethrow { setDoc(js, encode(data, encodeDefaults)!!, json("mergeFields" to mergeFieldPaths.map { it.js }.toTypedArray())).await() }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, merge: Boolean) =
        rethrow { setDoc(js, encode(strategy, data, encodeDefaults)!!, json("merge" to merge)).await() }
    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFields: String) =
        rethrow { setDoc(js, encode(strategy, data, encodeDefaults)!!, json("mergeFields" to mergeFields)).await() }

    actual suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) =
        rethrow { setDoc(js, encode(strategy, data, encodeDefaults)!!, json("mergeFields" to mergeFieldPaths.map { it.js }.toTypedArray())).await() }
}

All these expect/actuals funs can and should be split up into two parts:

1. A common behaviour, the encoding.
2. A platform specific behaviour, here set data

Using abstract classes to do part 1 in common code, and then forwarding the result to 2 to do something expect/actuals reduces the footprint greatly:

// Common
abstract class BaseDocumentReference {
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, merge: Boolean = false) = setEncoded(encode(data, encodeDefaults)!!, if (merge) SetOptions.Merge else SetOptions.Overwrite)
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFields: String) = setEncoded(encode(data, encodeDefaults)!!, SetOptions.MergeFields(mergeFields.asList()))
    suspend inline fun <reified T> set(data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) = setEncoded(encode(data, encodeDefaults)!!, SetOptions.MergeFieldPaths(mergeFieldPaths.asList()))

suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, merge: Boolean = false) = setEncoded(
        encode(strategy, data, encodeDefaults)!!, if (merge) SetOptions.Merge else SetOptions.Overwrite)
suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFields: String) = setEncoded(
        encode(strategy, data, encodeDefaults)!!, SetOptions.MergeFields(mergeFields.asList()))
suspend fun <T> set(strategy: SerializationStrategy<T>, data: T, encodeDefaults: Boolean, vararg mergeFieldPaths: FieldPath) = setEncoded(
        encode(strategy, data, encodeDefaults)!!, SetOptions.MergeFieldPaths(mergeFieldPaths.asList()))

    @PublishedApi
    internal abstract suspend fun setEncoded(encodedData: Any, setOptions: SetOptions)
}

expect class DocumentReference internal constructor(nativeValue: NativeDocumentReference) : BaseDocumentReference

// Android

val SetOptions.android: com.google.firebase.firestore.SetOptions? get() = when (this) {
    is SetOptions.Merge -> com.google.firebase.firestore.SetOptions.merge()
    is SetOptions.Overwrite -> null
    is SetOptions.MergeFields -> com.google.firebase.firestore.SetOptions.mergeFields(fields)
    is SetOptions.MergeFieldPaths -> com.google.firebase.firestore.SetOptions.mergeFieldPaths(encodedFieldPaths)
}
actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) : BaseDocumentReference() {
    override suspend fun setEncoded(encodedData: Any, setOptions: SetOptions) {
        val task = (setOptions.android?.let {
            android.set(encodedData, it)
        } ?: android.set(encodedData))
        task.await()
    }
}

// iOS
actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) : BaseDocumentReference() {
    override suspend fun setEncoded(encodedData: Any, setOptions: SetOptions) = await {
        when (setOptions) {
            is SetOptions.Merge -> ios.setData(encodedData as Map<Any?, *>, true, it)
            is SetOptions.Overwrite -> ios.setData(encodedData as Map<Any?, *>, false, it)
            is SetOptions.MergeFields -> ios.setData(encodedData as Map<Any?, *>, setOptions.fields, it)
            is SetOptions.MergeFieldPaths -> ios.setData(encodedData as Map<Any?, *>, setOptions.encodedFieldPaths, it)
        }
    }
}

// JS
val SetOptions.js: Json get() = when (this) {
    is SetOptions.Merge -> json("merge" to true)
    is SetOptions.Overwrite -> json("merge" to false)
    is SetOptions.MergeFields -> json("mergeFields" to fields.toTypedArray())
    is SetOptions.MergeFieldPaths -> json("mergeFields" to encodedFieldPaths.toTypedArray())
}

actual class DocumentReference actual constructor(internal actual val nativeValue: NativeDocumentReference) : BaseDocumentReference() {
    override suspend fun setEncoded(encodedData: Any, setOptions: SetOptions) = rethrow {
        setDoc(js, encodedData, setOptions.js).await()
    }
}

Consider how much more maintainable this is. We encode consistently on a single place, and we send to the platform logic in a single place. Before, for these 6 methods, we encoded for each method per platform (so 3 times 6 = 18 times) and for each method we had to write the same coroutine logic with error handling (also 18). With using abstract classes we get 6 encodings and 3 platform methods.

You can't do this with extension functions, because that would defeat the purpose where the common step still needs to be implemented on each method per platform.

Of course, instead of abstract classes this could be done by interface delegation, but at that point, we will have added extra state to the classes. Similarly im not too fond of using default implementations to the interfaces here, as it would basically constitute an abstract class.

[nbransby]

You can't do this with extension functions, because that would defeat the purpose where the common step still needs to be implemented on each method per platform.

Are you saying this pattern is not possible?

common

expect class Something {
  expect fun publicApi()
}

internal fun Something.sharedFunction() {}

platform

actual class Something() {
  actual fun publicApi() {
    sharedFunction()
    //do other platform specific stuff
  }
}

[Daeda88]

No, im saying its not correct to do it like that in my opinion, for maintainability reasons. Because if you use that approach, every time something changes to sharedFunction you have to account for it in 3 times as many places.

[Daeda88]

At the very least, we should have common

expect class Something {
  expect fun publicApi(parameter: String): Result
}

internal inline fun Something.sharedFunction(parameter: String, parsedMethod: (Any) -> Result): Result

platform

actual class Something() {
  actual fun publicApi(parameter: String) = sharedFunction(parameter) { any ->
     any.toResult()
  } 
}

But I dont think that solves the maintainability problem per se. You'll still have to write 3 times as many implementations.

[Daeda88]

If you want to account for value classes, I would propose to move to interfaces with default implementations, as that sort of meets both criteria (maintainability and maintaining possibility to inline) https://play.kotlinlang.org/#eyJ2ZXJzaW9uIjoiMS45LjIyIiwicGxhdGZvcm0iOiJqYXZhIiwiYXJncyI6IiIsIm5vbmVNYXJrZXJzIjp0cnVlLCJ0aGVtZSI6ImlkZWEiLCJjb2RlIjoiLyoqXG4gKiBZb3UgY2FuIGVkaXQsIHJ1biwgYW5kIHNoYXJlIHRoaXMgY29kZS5cbiAqIHBsYXkua290bGlubGFuZy5vcmdcbiAqL1xuXG5pbXBvcnQga290bGlueC5jb3JvdXRpbmVzLipcblxuaW50ZXJmYWNlIFRlc3Qge1xuICAgIHZhbCBzdHJpbmc6IFN0cmluZ1xuICAgIGZ1biB0ZXN0RnVuKCk6IFN0cmluZyB7IHJldHVybiBcInRlc3Qkc3RyaW5nXCIgfVxufVxuXG5ASnZtSW5saW5lXG52YWx1ZSBjbGFzcyBUZXN0Q2xhc3Mob3ZlcnJpZGUgdmFsIHN0cmluZzogU3RyaW5nKSA6IFRlc3RcblxuZnVuIG1haW4oKSB7XG4gICAgcHJpbnRsbihUZXN0Q2xhc3MoXCJIZWxsb1dvcmxkXCIpLnRlc3RGdW4oKSlcbn0ifQ==

[nbransby]

No, im saying its not correct to do it like that in my opinion, for maintainability reasons. Because if you use that approach, every time something changes to sharedFunction you have to account for it in 3 times as many places.

I'm a little confused here as I don't see how inheritance would make it any better, for reference, here's the inheritance version of my trivial example, as you can see the actual implementation doesn't change but we end up publishing BaseToShareStuffBetweenTheActualImplementations in our public API.

common

expect class Something : BaseToShareStuffBetweenTheActualImplementations {
  expect fun publicApi()
}

abstract class BaseToShareStuffBetweenTheActualImplementations {
  internal fun sharedFunction() {}
}

platform

actual class Something() {
  actual fun publicApi() {
    sharedFunction()
    //do other platform specific stuff
  }
}

[nbransby]

If you want to account for value classes, I would propose to move to interfaces with default implementations, as that sort of meets both criteria (maintainability and maintaining possibility to inline)

Definitely an option but we should hold fire on a refactor like this until the expect/actual language feature is stabilized

[Daeda88]

You example is the reverse of what we need though. It would be:

common

expect class Something : BaseToShareStuffBetweenTheActualImplementations()

abstract class BaseToShareStuffBetweenTheActualImplementations {
  fun publicApi() {
          doSomethingCommon()
          platformFunction()
    }
  internal abstract fun platformFunction()
}

platform

actual class Something() : BaseToShareStuffBetweenTheActualImplementations() {
  override fun platformFunction() {
    //do other platform specific stuff
  }
}

There's a big difference. The abstract class serves only to expose the public API methods and handle default implementation for it. It then splits up the public API method in something that can be done in common and something that must be done on the platform.

The public API of Something here exposes only publicAPI(). The difference between only and new logic is only that the publicAPI() method is now not an expect/actuals method but one inherited from BaseToShareStuffBetweenTheActualImplementations