Add `kotlin.serialization` implementation of `Serializer` #124

hiddewie · 2021-05-30T14:49:32Z

(Followup from #121 which had to be closed because I deleted my Github fork.)

This PR adds support for Kotlin serialization, see issue #13.

This PR includes #120 (this is required in order to use the stable version of kotlin.serialization).

In summary:

Adds a class KotlinSerializer which implements Axon Serializer.
Configurable RevisionResolver, Converter, and Json, with the same defaults as the Jackson serializer implementation.
Because Json is configurable, applications can define how JSON should be serialized based on business requirements.
Adds a DSL function kotlinSerializer to create a KotlinSerializer with configuration options.
Useful errors when the .serializer() method cannot be found on a companion object of a to-serialize object.
Class -> KSerializer is cached for performance and to avoid too much reflection.
Simple test to show that the implementation works.

Ref https://kotlinlang.org/docs/serialization.html with basic overview.
Ref https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/serialization-guide.md with serialization guide.

- Dependencies with optional mark - Add test case (and code fixes) for nullable serializer code - 2021 license

…to kotlin-serializer

smcvb

Bunch of comments on here. The biggest one is that it does not contain how this Serializer should be used for Axon's internal objects. For this to be useable within Axon, I am afraid it must support Axon's objects like the Message, TrackingToken, and SagaEntry.

kotlin/src/main/kotlin/org/axonframework/extensions/kotlin/serialization/KotlinSerializer.kt

smcvb · 2021-05-31T09:52:41Z

kotlin/src/main/kotlin/org/axonframework/extensions/kotlin/serialization/KotlinSerializer.kt

+ * @see KotlinSerializer
+ * @see kotlinSerializer
+ */
+class KotlinSerializerConfiguration {


What's the driving force to use this format instead of a constructor with default values?

These properties are all var, so mutable. This makes a clean DSL possible, without invoking the constructor directly. But the DSL is totally optional and I could remove it.

Honestly, I wouldn't know what's more "Kotlin-like".

If I'd look at the other Axon components, the settings are done through the Builder of a piece of infrastructure.
We've quite recently allowed Builder configuration directly through Axon's overall Configurer (for the PooledStreamingEventProcessor, to be exact), which we feel might be the way forward.

I'd wager that such a separate config object does not align very well with that idea.
However, a stated earlier, I am not aware what would be considered "Kotlin-like".

@sandjelkovic, what's your opinion on this?

I removed this Configuration class, and used the constructor with default values. It is no problem to add it back later if it is needed.

The Configuration class could be seen as a builder pattern for a Kotlin-style DSL. The Configuration class contains the same properties as the constructor, and all mutable. In the DSL builder function you could configure the Configuration class (or builder, however you want to call it), and that will then invoke the constructor of the actual (immutable) service.

kotlin/src/main/kotlin/org/axonframework/extensions/kotlin/serialization/KotlinSerializer.kt

smcvb · 2021-05-31T10:07:10Z

kotlin/src/main/kotlin/org/axonframework/extensions/kotlin/serialization/KotlinSerializer.kt

+                        typeForClass(type)
+                    )
+
+                else ->


I would've assumed the Serializer could default to byte[] instead of throwing an exception.
Doing so will have this serializer follow the same paradigm as the JacksonSerializer and XStreamSerializer.

Several spots in the framework assume that the expectedRepresentation is byte[].
So enabling this option up would make this serializer more valuable throughout the framework.

Pretty confident this will require introducing the registerConverters method (as seen on line 128 of the JacksonSerializer too).
This method ensures Json specific ContentTypeConverters are added to the set-up.

Furthermore, I assume this will warrant introducing content type converters going from JsonElement to byte[], and vice versa.

I totally forgot about the (extra) converters. I will add some defaults to convert between byte arrays <-> Strings and JSON Nodes 👍

I made the JsonElement type the exception, just like JsonNode in the Jackson serializer. All other target types are serialized to String, and then converted with the chaining converter.

By default String, Byte array, Json Element and InputStream already work through service discovery, so that is fine.

If any other converters are needed I can add them.

Thanks for validating @hiddewie 👍
Adding tests to validate this works (as in, the conversion between those types through this serializer) is a massive plus.

kotlin/src/main/kotlin/org/axonframework/extensions/kotlin/serialization/KotlinSerializer.kt

smcvb · 2021-05-31T10:31:03Z

kotlin/src/main/kotlin/org/axonframework/extensions/kotlin/serialization/KotlinSerializer.kt

+            // Class<T>: T must be non-null
+            val kClass = (this as Class<Any>).kotlin
+
+            val companion = kClass.companionObject


I assume that the hard requirement on the @Serializable annotation makes it pretty tough to use this serializer for Axon objects, like the TrackingToken.
I've skimmed the Kotlin Serializer documentation somewhat but didn't spot a solution for this just yet.
Do you, perchance, have a solution in mind for this, @hiddewie?

This is the toughest question :)

As far as I can see, there are two solutions:

Annotate the serializable types in Axon core with @Serializable. This adds the compile time requirement of having the Kotlin compiler plugin dependency in the build process. That may or may not be OK. Although, there are also Jackson annotations in the source code already. In runtime there is no Kotlin requirement.

Create handwritten serializers for every class that needs to be serializable. This is quite some (cumbersome, error prone) work, depending on the number of serializable classes in the Axon core. The serializer code can be maintained in this repository.

To illustrate option 2, I added a small example and custom serializer with test.

The third option is abandoning this approach because the Kotlin serialization is not wanted in the core framework, and it is not worth the effort to maintain the custom serializers.

(After thinking about option 1 a bit more: I have not tested adding @Serializable on a Java class and then running the Kotlin compiler plugin on it. Theoretically it should work but in practice I don't know)

Thanks for the insights here.
If you'd be at leisure to validate whether it is even possible to add the @Serializable annotation to a Java class, that would be amazing (and save me some personal investigation. 😅).

In the meantime, I'll start an internal discussion about whether we want to add the @Serializable annotation to the core of Axon Framework.

I tried a few things to test how the interoperability of Java classes with Kotlin annotations, and the Kotlin annotation processors work together.

What I did:

Complie Axon framewok locally (4.6.0-SNAPHSOT)

Add the Maven Kotlin compiler plugin which would compile the Kotlin annotations into serializer functions, see instructions here https://github.com/Kotlin/kotlinx.serialization#maven

Add @kotlinx.serialization.Serializable to ConfigToken

What I saw:

The annotation gets added (just like any other annotation) in the classfile

No serializer static method is generated.

What I did:

Copy the Java class and copy it to this repository, with the .java extension.

Let the Kotlin compiler, with the Kotlin serialization compiler plugin comple the .java class as well

What I saw:

The annotation gets added in the classfile

Still no serializer static method is generated.

What I did:

Let IntelliJ convert the .java file to a .kt Kotlin file (very ugly)

What I saw:

The annotation gets added in the classfile

A serializer method is generated.

So my conclusion is that the Kotlin serialization compiler plugin only works for Kotlin source files, and that the .serializer() static method cannot be generated for Java source files.

That is very unfortunate, I might open an issue with the Kotlin Serialization team to ask if that is supposed to work (ref Kotlin/kotlinx.serialization#1687).

Either way, we can not assume that every class that comes here will have a serializer attached to it (nor be a Kotlin class)

If there is no serializer defined, or just based on types, we can fall back on a registry that we can keep as a map of Type (Class, Class name, etc) and a Serializer. That map could be configurable so someone might add support for classes without @Serializable like Java Dates, and we can pre-populate it with AF serializers configuration. Or keep the AF serializers in a separate config, but that's a finer detail.

This and
this is what I mean, as we need to pass the serializer explicitly in the very broad generic setting anyway, we might as well keep it somewhere in a config map, similar as to how this cache is doing but is just caching all serializers.

smcvb · 2021-05-31T10:34:14Z

...in/src/test/kotlin/org/axonframework/extensions/kotlin/serialization/KotlinSerializerTest.kt

+import kotlin.test.assertNull
+
+class KotlinSerializerTest {
+


I am missing test cases that validate if the KotlinSerializer can be used for Axon objects, like the Message, TrackingToken, and SagaEntry.

Assuming that's because those objects aren't annotated with the @Serializable annotation.
However, the Serializer in Axon must de-/serialize those objects for it to be usable.

Indeed. See https://github.com/AxonFramework/extension-kotlin/pull/124/files#r645048334 for the comment about serializing internal classes.

I added one test case for the ConfigToken, and every internal class that should be serializable can be added in that way, if there is a serializer.

Gotcha. To make this Serializer implementation workable for Axon Framework entirely, these tests will be a requirement too.

Making these can wait until we've concluded the conversation on the Serializer implementation though.
On the subject of using the @Serializable annotation on Axon components, and if that even works for Java classes, that is.

…d byte arrays

sonarqubecloud · 2021-06-03T19:44:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

hiddewie · 2021-06-03T19:45:42Z

Thank you for the great comments @smcvb. The discussion in https://github.com/AxonFramework/extension-kotlin/pull/124/files#r645048334 remains unresolved which will require more changes whatever solution is chosen.

hiddewie · 2021-06-03T19:47:39Z

...kotlin/org/axonframework/extensions/kotlin/serialization/serializer/ConfigTokenSerializer.kt

+
+@OptIn(ExperimentalSerializationApi::class)
+@Serializer(forClass = ConfigToken::class)
+class ConfigTokenSerializer : KSerializer<ConfigToken> {


This is the custom serializer implemented manually for the internal Axon type. These type of implementations are needed if the @Serializable annotation is not used in combination with the compiler plugin.

I'd assume this should be constructed for every concrete implementation of serializable Axon objects in that case, correct?

This would at least enable moving further with this serializer, so that's good.

If we go this route, it will require some KDoc, of course ;-)

I just saw https://github.com/Kotlin/kotlinx.serialization/blob/f673bc2/docs/serializers.md#deriving-external-serializer-for-another-kotlin-class-experimental for the first time. This would allow us to skip the 'manual' implementation of the serializer.

Unfortunately I can't yet get it to work, I run into this bug in the Kotlin complier plugin (Kotlin/kotlinx.serialization#1680).

Have you tried Surrogate serializers and Delegating serializers?

They look much simpler and might just get the job done. Hopefully, they might just work and there would be no hidden issues like you encountered previously.

(#124 (comment)) I changed the fully-custom example ConfigTokenSerializer into a serializer using a ConfigTokenSurrogate. Its slightly better maintainable, but still a LOT of work to do (and maintain) for every Axon class.

The work means, for every Axon class:

Create a Kotlin class with @Serializable

Put all the fields of the class into the surrogate, and add converter methods to convert from the class to the surrogate, and back.

Manually implement a serializer for that class, using the SerialDescriptor generated by the surrogate.

During serialization, convert from the real value to a surrogate and serialize it.

During deserialization, deserialize the surrogate and convert from the surrogate to the value of the real class.

The serializers for surrogates are very simple and similar (only a few lines of code). However the surrogates themselves need to contain exactly the public data fields of the class they are a surrogate for. This will cause bugs if a field is forgotten in the surrogate.

smcvb · 2021-09-08T14:35:44Z

...kotlin/org/axonframework/extensions/kotlin/serialization/serializer/ConfigTokenSerializer.kt

+import kotlinx.serialization.encoding.encodeStructure
+import org.axonframework.eventhandling.tokenstore.ConfigToken
+
+@OptIn(ExperimentalSerializationApi::class)


Would you be able to elaborate a little on the ExperimentalSerializationApi you're "opting in" here?

The @Serializer annotation below is marked as experimental. It might change its implementation slightly between versions of Kotlin Serialization. (https://kotlin.github.io/kotlinx.serialization/kotlinx-serialization-core/kotlinx-serialization-core/kotlinx.serialization/-experimental-serialization-api/index.html)

We might wait to merge this feature until the used serialization API is marked fully stable, although the Axon extension-kotlin library is also marked as experimental itself.

So to clarify, only @Serializer which is used to hook custom serializer objects is marked as Experimental, the rest of Json serialization is not experimental anymore and is stable. Is this a correct assumption @hiddewie ?

Yes exactly. The following is stable:

@Serializable class XXX

But the following needs experimental opt in:

@Serializer(forClass = QQQ::class) class XXX

In particular doing this for Java classes gives compiler errors, and it might simply not be supported (see Kotlin/kotlinx.serialization#1687)

smcvb

Sorry for returning so late here. Times are rather busy, and some things sadly got precedence.
On any note, I think it is paramount we first tackle the Axon Serializable components predicament, specifically on whether that annotation would be an option. From my end, I'll chat with the team if we feel it has a time and place inside the core.

hiddewie · 2021-09-20T18:49:15Z

No problem :)

I investigated the compilation of the @Serializable annotation in Axon Java source code. That does not look like it will work.

Then I found https://github.com/Kotlin/kotlinx.serialization/blob/f673bc2/docs/serializers.md#deriving-external-serializer-for-another-kotlin-class-experimental, which could mean that we create singleton objects marked as @Serializer(forClass = ...) that would auto-generate the needed serializers within the compilation process of this library. Unfortunately it doesn't work yet: the compilation crashes.

smcvb · 2021-10-12T09:17:18Z

FYI, @hiddewie, I've adjusted the milestone to 0.3.0 instead of 0.2.0.

The intent for this is the desire to release 0.2.0 of the Kotlin extension.
We felt we couldn't wait much longer until this release.

This, by the way, doesn't mean a 0.3.0 release couldn't be around the corner, which might contain this serializer. ;-)

kotlin/src/main/kotlin/org/axonframework/extensions/kotlin/serialization/KotlinSerializer.kt

sandjelkovic · 2022-03-02T15:17:03Z

kotlin/src/main/kotlin/org/axonframework/extensions/kotlin/serialization/KotlinSerializer.kt

+            // Class<T>: T must be non-null
+            val kClass = (this as Class<Any>).kotlin
+
+            val companion = kClass.companionObject


Either way, we can not assume that every class that comes here will have a serializer attached to it (nor be a Kotlin class)

If there is no serializer defined, or just based on types, we can fall back on a registry that we can keep as a map of Type (Class, Class name, etc) and a Serializer. That map could be configurable so someone might add support for classes without @Serializable like Java Dates, and we can pre-populate it with AF serializers configuration. Or keep the AF serializers in a separate config, but that's a finer detail.

This and
this is what I mean, as we need to pass the serializer explicitly in the very broad generic setting anyway, we might as well keep it somewhere in a config map, similar as to how this cache is doing but is just caching all serializers.

sandjelkovic · 2022-03-02T15:23:56Z

...kotlin/org/axonframework/extensions/kotlin/serialization/serializer/ConfigTokenSerializer.kt

+
+@OptIn(ExperimentalSerializationApi::class)
+@Serializer(forClass = ConfigToken::class)
+class ConfigTokenSerializer : KSerializer<ConfigToken> {


Have you tried Surrogate serializers and Delegating serializers?

They look much simpler and might just get the job done. Hopefully, they might just work and there would be no hidden issues like you encountered previously.

sandjelkovic · 2022-03-02T15:29:47Z

...kotlin/org/axonframework/extensions/kotlin/serialization/serializer/ConfigTokenSerializer.kt

+import kotlinx.serialization.encoding.encodeStructure
+import org.axonframework.eventhandling.tokenstore.ConfigToken
+
+@OptIn(ExperimentalSerializationApi::class)


So to clarify, only @Serializer which is used to hook custom serializer objects is marked as Experimental, the rest of Json serialization is not experimental anymore and is stable. Is this a correct assumption @hiddewie ?

…work

hiddewie · 2022-04-16T12:46:31Z

Sorry for the long wait, I didn't forget the PR but it took some time until I could dive into it again!

I updated this branch to include all changes in master.

I updated the Kotlin serialization version to 1.3.2, but this makes no changes to everything I have observed so far.

In particular I experimented with a Surrogate serializer (#124 (comment)). This works, but still requires a lot of maintenance work to 'wrap' each Axon internal class with a surrogate class in this library, while copying the public fields manually into the surrogate class for serialization. Performance-wise this is also not ideal.

https://github.com/AxonFramework/extension-kotlin/pull/124/files#diff-cc9e7866a7929cd55740daaa367352b89517dc56787fa6eb48f5c2fcb783b1bbR12-R31

I tested the automatically-generated serializer for external classes functionality again, but this still gives compiler errors:

@Serializer(forClass = ConfigToken::class)
object ConfigTokenSerializer

If I manually type the simple class like ConfigToken using Kotlin, it does work. So there might again be a Kotlin <-> Java interop problem here, causing errors in the Kotlin compiler.

smcvb · 2022-09-12T12:28:45Z

Removing the milestone as this is still under discussion, while the release is intended to come soon. Will add this to the following release milestone for further discussions.

smcvb · 2024-07-17T12:16:23Z

Marking this pull request as obsolete through the introduction of #338.

hiddewie added 2 commits May 23, 2021 18:59

Add Kotlin serializer and basic test

87ee5ff

Comments:

43c5d0e

- Dependencies with optional mark - Add test case (and code fixes) for nullable serializer code - 2021 license

hiddewie mentioned this pull request May 30, 2021

Add kotlin.serialization implementation of Serializer #121

Closed

Merge branch 'master' of github.com:AxonFramework/extension-kotlin in…

4bf4ddf

…to kotlin-serializer

sandjelkovic added Priority 3: Could Status: In Progress Type: Feature labels May 31, 2021

sandjelkovic requested review from sandjelkovic, lfgcampos and smcvb May 31, 2021 08:15

smcvb added this to the Release 0.2.0 milestone May 31, 2021

smcvb assigned hiddewie May 31, 2021

smcvb requested changes May 31, 2021

View reviewed changes

hiddewie added 8 commits June 3, 2021 19:39

Merge remote-tracking branch 'origin/master' into kotlin-serializer

0af12e5

Add defaults

2b7735b

Add since and author

1256d2d

Handle unknown serialized type

f7d4ee9

Custom serializer

798b3fd

One more default

edb6903

Byte arrays

b8fbd02

Use chaining converters, add tests for input stream, JSON elements an…

c113040

…d byte arrays

hiddewie commented Jun 3, 2021

View reviewed changes

smcvb reviewed Sep 8, 2021

View reviewed changes

hiddewie mentioned this pull request Sep 20, 2021

@Serializable for Java classes/source code Kotlin/kotlinx.serialization#1687

Closed

hiddewie added 2 commits September 20, 2021 20:50

Remove the configuration DSL class

7acce4c

Merge remote-tracking branch 'origin/master' into kotlin-serializer

aa6804f

smcvb modified the milestones: Release 0.2.0, Release 0.3.0 Oct 12, 2021

smcvb mentioned this pull request Oct 12, 2021

Kotlin Serializer implementation #13

Closed

sandjelkovic reviewed Mar 2, 2022

View reviewed changes

hiddewie added 4 commits April 16, 2022 11:30

Merge remote-tracking branch 'origin/master' into kotlin-serializer

20479a8

Kotlin serialization 1.3.2

e0f411a

better check for JsonElement assignability

8d7b70d

Use surrogate in example. @Serializer(forClass = ...) still doesnt …

34844c8

…work

smcvb removed this from the Release 0.3.0 milestone Sep 12, 2022

smcvb added the Status: Under Discussion label Sep 12, 2022

sandjelkovic removed the request for review from lfgcampos March 20, 2023 09:01

lion7 mentioned this pull request May 28, 2024

Add kotlin.serialization implementation of Serializer #338

Merged

hiddewie closed this by deleting the head repository Jun 15, 2024

smcvb added Status: Obsolete and removed Status: In Progress Status: Under Discussion labels Jul 17, 2024

smcvb added this to the Release 4.10.0 milestone Jul 17, 2024

Add kotlin.serialization implementation of Serializer #124

Add kotlin.serialization implementation of Serializer #124

Conversation

hiddewie commented May 30, 2021

smcvb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smcvb Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

hiddewie Sep 20, 2021 • edited Loading

Choose a reason for hiding this comment

smcvb May 31, 2021 • edited Loading

Choose a reason for hiding this comment

smcvb May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smcvb Sep 8, 2021 • edited Loading

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

hiddewie Sep 20, 2021 • edited Loading

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

sonarqubecloud bot commented Jun 3, 2021

hiddewie commented Jun 3, 2021

Choose a reason for hiding this comment

smcvb Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiddewie Apr 16, 2022 • edited Loading

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

smcvb left a comment

Choose a reason for hiding this comment

hiddewie commented Sep 20, 2021

smcvb commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hiddewie commented Apr 16, 2022

smcvb commented Sep 12, 2022

smcvb commented Jul 17, 2024

Add `kotlin.serialization` implementation of `Serializer` #124

Add `kotlin.serialization` implementation of `Serializer` #124

smcvb Sep 8, 2021 •

edited

Loading

hiddewie Sep 20, 2021 •

edited

Loading

smcvb May 31, 2021 •

edited

Loading

smcvb May 31, 2021 •

edited

Loading

smcvb Sep 8, 2021 •

edited

Loading

hiddewie Sep 20, 2021 •

edited

Loading

smcvb Sep 8, 2021 •

edited

Loading

hiddewie Apr 16, 2022 •

edited

Loading