Serialize sealed classes

[iruizmar]

Solves #190 and #61

I haven't unit tested it because it's saying to me that "Tests events were not received". If you have any idea on how to fix this, I could certainly add the testing.

[nbransby]

Hey @iruizmar this look good!

Lets get the tests working for you so you can add coverage for your changes, how are you trying to run the tests exactly?

[iruizmar]

Hey @nbransby ! 👋🏻 Thanks for the quick response.

I'm just trying to run them through Android Studio by clicking on Play on the test suite class:

I tried running the allTest task on the firebase-common module too and no test were run either:

[nbransby]

Have you tried in IJ instead?

[iruizmar]

Same experience there :(. Are you doing the same or are you running them any other way?

[iruizmar]

I think I got them running now by using the firebase-common:check task instead.
Let me work on the test and come back to you.
Thanks!

[iruizmar]

Hey @nbransby! 👋🏻
I added the testing.

As you can see on the tests, the serialization is adding a type and value parameters. I'm sure the type parameter is expected to discretise which child class you're serializing, but I'm not sure about the value one. You can see related doc here.

I would expect the resultant map to be:

{ "type" : "child", "map" : { "key": "value" }, "bool" : true }

and not

{ "type" : "child", "value" : { "map" : { "key": "value" }, "bool" : true } }

WDYT? It's not a problem when you're encoding and decoding here because it's decoding the same way, though.

Please, let me know if there's something else to be done.

[nbransby]

Maybe the docs have been simplified for reading as if it did add type at the same level then it could clash with a property called type if the class had such. But best way to verify that would be to see what the json serializer produces in a kotlin playground for the examples given in the docs

[iruizmar]

I tested the Json.encodeToString(childObject) output and it's the expected

{"type":"child","map":{"key":"value"},"bool":true}

Without the value outer object.
Do you know:

1. If it is a problem for us that this value property exists?
2. What could be inserting it?

[nbransby]

Yes I believe it will be a problem as perhaps only the properties under value is what gets persisted to firestore/rtdb etc
best way to check would be to write another test which actually writes and reads a document to/from firestore

[iruizmar]

That I tested on my app directly because it is actually the use case I have. And the answer is yes, it's working but it is including the value thing. I can see this on the firestore inspector:

And the retrieval from firestore in the app is working correctly too.

[nbransby]

Yeah which kinda sucks as you would expect the format in firestore to be the same as the json format, out of interest what happens with the json format if you do have a property called type on the class?

We are going to need to modify the FirebaseEncoder and FirebaseDecoder to stop it adding the value properly

[iruizmar]

Hey @nbransby

Trying to encode with json an object with a type property throws IllegalStateException.
But there's a workaround to change the type property being the discriminator: https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/json.md#class-discriminator-for-polymorphism

Could you point me towards where that value property is added? I tried to debug it but I haven't been lucky.

[nbransby]

So we probably need to support a classDiscriminator parameter just like we support shouldEncodeElementDefault.

As for where the value property is added I don't believe that's explicitly done in our code and is likely just a quirk of our (mis?)use of the serialization APIs.

I had a look at the code myself but it's been quite a while since I worked on it so afraid there's not much more I can give in the way of pointers

[iruizmar]

Hey!

So this will require a bit more of work than I expected 😓 .
Do you reckon it's wise to include the classDiscriminator in the encode function even if it would only make sense when serialising polymorphic classes?

I'll try to investigate the origin of the value thing and let you know.

[nbransby]

Do you reckon it's wise to include the classDiscriminator in the encode function even if it would only make sense when serialising polymorphic classes?

yes can make it optional just like shouldEncodeElementDefault you will also need it in decode.

[iruizmar]

Hey man @nbransby
Could you, please, guide me on how are you running the tests on the project?
When running gradle firebase-common:check task, only Js and iOS tests are being run and not Android. It's quite cumbersome because I can't debug non-Android executions.

[nbransby]

Can plus use the play buttons in the gutter for the tests you want to run but you need to make sure you have the emulator setup correctly for it to succeed, you can check the GitHub pull request action in .GitHub to see the setup required

[iruizmar]

This is what I see when clicking on the play icon for tests:

Trying to run any of the android ones leads to "Test events were not received". This happens both on Android Studio and Intellij. I have the emulator running with those specs.

[nbransby]

Its because they are not unit tests but connected device tests that run on the emulator you can run the task connectedDebugAndroidTest or read up on them to find out to run and debug individual tests from the ide

[iruizmar]

Hey @nbransby 👋🏻
I think I achieved the expected functionality. I had to modify some of the encoding/decoding logic. I took inspiration on how it's done on kotlinx.serialization.json.
Let me know if you see something else.

[nbransby]

What about support for classDiscriminator? Via an annotation is cool and is prefered to an optional config param but do you think we should still provide the alternative like kotlinx.serialization.json does?

[nbransby]

Also need to add the details about the FIrebaseClassDiscriminator annotation to the readme 👍

[iruizmar]

I'm OK with adding the classDiscriminator as a parameter if you feel it's needed. But what kotlinx.serialization is doing is having a global configuration for all serializations, not providing them in the encodeX function.

Right now, the serialization of firebase-kotlin-sdk is not configurable globally, and the only way would be to pass it to the encode and decode functions.

Is that what you're looking for?

I added the section on the Readme :)

[iruizmar]

@nbransby Any news on this one? Does the docs look good enough for you?

[nbransby]

I took inspiration on how it's done on kotlinx.serialization.json.

@iruizmar could you add comments with links to the code that inspired you? that would help me and others understand the code in the future

[iruizmar]

@nbransby Done! It's basically the whole Polymorphic.kt file

[nbransby]

Thanks @iruizmar, good stuff!